Update Manifest gatherer to use gather error instead of -1 artifact (#…

…1624)
GoogleChrome · Feb 8, 2017 · 9f91ab4 · 9f91ab4
1 parent c6aeb33
commit 9f91ab4
Show file tree

Hide file tree

Showing 26 changed files with 266 additions and 247 deletions.
diff --git a/lighthouse-core/audits/cache-start-url.js b/lighthouse-core/audits/cache-start-url.js
@@ -37,21 +37,18 @@ class CacheStartUrl extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    let cacheHasStartUrl = false;
-    const manifest = artifacts.Manifest.value;
-    const cacheContents = artifacts.CacheContents;
-
-    if (!(manifest && manifest.start_url && manifest.start_url.value)) {
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
       return CacheStartUrl.generateAuditResult({
         rawValue: false,
-        debugString: 'start_url not present in Manifest'
       });
     }
 
-    if (!Array.isArray(cacheContents)) {
+    const manifest = artifacts.Manifest.value;
+    if (!(manifest.start_url && manifest.start_url.value)) {
       return CacheStartUrl.generateAuditResult({
         rawValue: false,
-        debugString: cacheContents.debugString || 'No cache detected'
+        debugString: 'start_url not present in Manifest'
       });
     }
 
@@ -67,7 +64,8 @@ class CacheStartUrl extends Audit {
     // here would be to resolve this more completely by asking the Service Worker about the start
     // URL. However that would also necessitate the cache contents gatherer relying on the manifest
     // gather rather than being independent of it.
-    cacheHasStartUrl = cacheContents.find(req => {
+    const cacheContents = artifacts.CacheContents;
+    const cacheHasStartUrl = cacheContents.find(req => {
       return (startURL === req || altStartURL === req);
     });
 

diff --git a/lighthouse-core/audits/manifest-background-color.js b/lighthouse-core/audits/manifest-background-color.js
@@ -37,22 +37,20 @@ class ManifestBackgroundColor extends Audit {
     };
   }
 
-  /**
-   * @param {!Manifest=} manifest
-   * @return {boolean}
-   */
-  static getBackgroundColorValue(manifest) {
-    return manifest !== undefined &&
-      manifest.background_color.value;
-  }
-
   /**
    * @param {!Artifacts} artifacts
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    const bgColor = ManifestBackgroundColor.getBackgroundColorValue(artifacts.Manifest.value);
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestBackgroundColor.generateAuditResult({
+        rawValue: false,
+      });
+    }
 
+    const manifest = artifacts.Manifest.value;
+    const bgColor = manifest.background_color.value;
     return ManifestBackgroundColor.generateAuditResult({
       rawValue: !!bgColor,
       extendedInfo: {

diff --git a/lighthouse-core/audits/manifest-display.js b/lighthouse-core/audits/manifest-display.js
@@ -48,16 +48,22 @@ class ManifestDisplay extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    const manifest = artifacts.Manifest.value;
-    const displayValue = manifest && manifest.display.value;
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestDisplay.generateAuditResult({
+        rawValue: false
+      });
+    }
 
+    const manifest = artifacts.Manifest.value;
+    const displayValue = manifest.display.value;
     const hasRecommendedValue = ManifestDisplay.hasRecommendedValue(displayValue);
 
     const auditResult = {
       rawValue: hasRecommendedValue,
       displayValue
     };
-    if (manifest && !hasRecommendedValue) {
+    if (!hasRecommendedValue) {
       auditResult.debugString = 'Manifest display property should be set.';
     }
     return ManifestDisplay.generateAuditResult(auditResult);

diff --git a/lighthouse-core/audits/manifest-exists.js b/lighthouse-core/audits/manifest-exists.js
@@ -40,6 +40,13 @@ class ManifestExists extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
+    if (!artifacts.Manifest) {
+      // Page has no manifest.
+      return ManifestExists.generateAuditResult({
+        rawValue: false
+      });
+    }
+
     return ManifestExists.generateAuditResult({
       rawValue: typeof artifacts.Manifest.value !== 'undefined',
       debugString: artifacts.Manifest.debugString

diff --git a/lighthouse-core/audits/manifest-icons-min-144.js b/lighthouse-core/audits/manifest-icons-min-144.js
@@ -38,8 +38,14 @@ class ManifestIconsMin144 extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    const manifest = artifacts.Manifest.value;
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestIconsMin144.generateAuditResult({
+        rawValue: false
+      });
+    }
 
+    const manifest = artifacts.Manifest.value;
     if (icons.doExist(manifest) === false) {
       return ManifestIconsMin144.generateAuditResult({
         rawValue: false

diff --git a/lighthouse-core/audits/manifest-icons-min-192.js b/lighthouse-core/audits/manifest-icons-min-192.js
@@ -41,8 +41,14 @@ class ManifestIconsMin192 extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    const manifest = artifacts.Manifest.value;
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestIconsMin192.generateAuditResult({
+        rawValue: false
+      });
+    }
 
+    const manifest = artifacts.Manifest.value;
     if (icons.doExist(manifest) === false) {
       return ManifestIconsMin192.generateAuditResult({
         rawValue: false

diff --git a/lighthouse-core/audits/manifest-name.js b/lighthouse-core/audits/manifest-name.js
@@ -39,15 +39,16 @@ class ManifestName extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    let hasName = false;
-    const manifest = artifacts.Manifest.value;
-
-    if (manifest && manifest.name) {
-      hasName = (!!manifest.name.value);
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestName.generateAuditResult({
+        rawValue: false
+      });
     }
 
+    const manifest = artifacts.Manifest.value;
     return ManifestName.generateAuditResult({
-      rawValue: hasName
+      rawValue: !!manifest.name.value
     });
   }
 }

diff --git a/lighthouse-core/audits/manifest-short-name-length.js b/lighthouse-core/audits/manifest-short-name-length.js
@@ -40,30 +40,34 @@ class ManifestShortNameLength extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    let isShortNameShortEnough = false;
-    let debugString;
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestShortNameLength.generateAuditResult({
+        rawValue: false
+      });
+    }
+
+    // When no shortname can be found we look for a name.
     const manifest = artifacts.Manifest.value;
-    const suggestedLength = 12;
+    const shortNameValue = manifest.short_name.value || manifest.name.value;
 
-    if (manifest) {
-      // When no shortname can be found we look for a name.
-      const shortNameValue = manifest.short_name.value || manifest.name.value;
-      if (!shortNameValue) {
-        return ManifestShortNameLength.generateAuditResult({
-          rawValue: false,
-          debugString: 'No short_name found.'
-        });
-      }
+    if (!shortNameValue) {
+      return ManifestShortNameLength.generateAuditResult({
+        rawValue: false,
+        debugString: 'No short_name found in manifest.'
+      });
+    }
 
-      // Historically, Chrome recommended 12 chars as the maximum length to prevent truncation.
-      // See #69 for more discussion.
-      // https://developer.chrome.com/apps/manifest/name#short_name
-      isShortNameShortEnough = shortNameValue.length <= suggestedLength;
+    // Historically, Chrome recommended 12 chars as the maximum length to prevent truncation.
+    // See #69 for more discussion.
+    // https://developer.chrome.com/apps/manifest/name#short_name
+    const suggestedLength = 12;
+    const isShortNameShortEnough = shortNameValue.length <= suggestedLength;
 
-      if (!isShortNameShortEnough) {
-        debugString = `${suggestedLength} chars is the suggested maximum homescreen label length`;
-        debugString += ` (Found: ${shortNameValue.length} chars).`;
-      }
+    let debugString;
+    if (!isShortNameShortEnough) {
+      debugString = `${suggestedLength} chars is the suggested maximum homescreen label length`;
+      debugString += ` (Found: ${shortNameValue.length} chars).`;
     }
 
     return ManifestShortNameLength.generateAuditResult({

diff --git a/lighthouse-core/audits/manifest-short-name.js b/lighthouse-core/audits/manifest-short-name.js
@@ -40,15 +40,17 @@ class ManifestShortName extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    let hasShortName = false;
-    const manifest = artifacts.Manifest.value;
-
-    if (manifest) {
-      hasShortName = !!(manifest.short_name.value || manifest.name.value);
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestShortName.generateAuditResult({
+        rawValue: false
+      });
     }
 
+    const manifest = artifacts.Manifest.value;
     return ManifestShortName.generateAuditResult({
-      rawValue: hasShortName
+      // When no shortname can be found we look for a name.
+      rawValue: !!(manifest.short_name.value || manifest.name.value)
     });
   }
 }

diff --git a/lighthouse-core/audits/manifest-start-url.js b/lighthouse-core/audits/manifest-start-url.js
@@ -40,15 +40,16 @@ class ManifestStartUrl extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    let hasStartUrl = false;
-    const manifest = artifacts.Manifest.value;
-
-    if (manifest && manifest.start_url) {
-      hasStartUrl = (!!manifest.start_url.value);
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestStartUrl.generateAuditResult({
+        rawValue: false
+      });
     }
 
+    const manifest = artifacts.Manifest.value;
     return ManifestStartUrl.generateAuditResult({
-      rawValue: hasStartUrl
+      rawValue: !!manifest.start_url.value
     });
   }
 }

diff --git a/lighthouse-core/audits/manifest-theme-color.js b/lighthouse-core/audits/manifest-theme-color.js
@@ -40,15 +40,16 @@ class ManifestThemeColor extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    let hasThemeColor = false;
-    const manifest = artifacts.Manifest.value;
-
-    if (manifest && manifest.theme_color) {
-      hasThemeColor = (!!manifest.theme_color.value);
+    if (!artifacts.Manifest || !artifacts.Manifest.value) {
+      // Page has no manifest or was invalid JSON.
+      return ManifestThemeColor.generateAuditResult({
+        rawValue: false
+      });
     }
 
+    const manifest = artifacts.Manifest.value;
     return ManifestThemeColor.generateAuditResult({
-      rawValue: hasThemeColor
+      rawValue: !!manifest.theme_color.value
     });
   }
 }

diff --git a/lighthouse-core/closure/typedefs/Manifest.js b/lighthouse-core/closure/typedefs/Manifest.js
@@ -33,7 +33,7 @@ ManifestNode.prototype.raw;
 /** @type {T} */
 ManifestNode.prototype.value;
 
-/** @type {string} */
+/** @type {string|undefined} */
 ManifestNode.prototype.debugString;
 
 /**

diff --git a/lighthouse-core/gather/gatherers/cache-contents.js b/lighthouse-core/gather/gatherers/cache-contents.js
@@ -45,26 +45,21 @@ function getCacheContents() {
 }
 
 class CacheContents extends Gatherer {
-  static _error(errorString) {
-    return {
-      raw: undefined,
-      value: undefined,
-      debugString: errorString
-    };
-  }
-
+  /**
+   * Creates an array of cached URLs.
+   * @param {!Object} options
+   * @return {!Promise<!Array<string>>}
+   */
   afterPass(options) {
     const driver = options.driver;
 
     return driver
         .evaluateAsync(`(${getCacheContents.toString()}())`)
         .then(returnedValue => {
-          if (!returnedValue) {
-            return CacheContents._error('Unable to retrieve cache contents');
+          if (!returnedValue || !Array.isArray(returnedValue)) {
+            throw new Error('Unable to retrieve cache contents');
           }
           return returnedValue;
-        }, _ => {
-          return CacheContents._error('Unable to retrieve cache contents');
         });
   }
 }

diff --git a/lighthouse-core/gather/gatherers/manifest.js b/lighthouse-core/gather/gatherers/manifest.js
@@ -26,35 +26,29 @@ const manifestParser = require('../../lib/manifest-parser');
  * the manifest parser.
  */
 class Manifest extends Gatherer {
-
-  static _errorManifest(errorString) {
-    return {
-      raw: undefined,
-      value: undefined,
-      debugString: errorString
-    };
-  }
-
+  /**
+   * Returns the parsed manifest or null if the page had no manifest. If the manifest
+   * was unparseable as JSON, manifest.value will be undefined and manifest.debugString
+   * will have the reason. See manifest-parser.js for more information.
+   * @param {!Object} options
+   * @return {!Promise<?Manifest>}
+   */
   afterPass(options) {
     return options.driver.sendCommand('Page.getAppManifest')
       .then(response => {
         // We're not reading `response.errors` however it may contain critical and noncritical
         // errors from Blink's manifest parser:
         //   https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError
         if (!response.data) {
-          let errorString;
-          // The driver returns an empty string for url and the data if
-          // the page has no manifest.
           if (response.url) {
-            errorString = `Unable to retrieve manifest at ${response.url}`;
+            throw new Error(`Unable to retrieve manifest at ${response.url}`);
           }
 
-          return Manifest._errorManifest(errorString);
+          // If both the data and the url are empty strings, the page had no manifest.
+          return null;
         }
 
         return manifestParser(response.data, response.url, options.url);
-      }, err => {
-        return Manifest._errorManifest('Unable to retrieve manifest: ' + err);
       });
   }
 }