Merge pull request #102 from brendankenny/errorstring

pipe manifest fetch/parse error strings up for display
GoogleChrome · Mar 31, 2016 · d1b8cfe · d1b8cfe
2 parents e0b795b + 78e8b7c
commit d1b8cfe
Show file tree

Hide file tree

Showing 27 changed files with 169 additions and 80 deletions.
diff --git a/audits/audit.js b/audits/audit.js
@@ -41,12 +41,14 @@ class Audit {
   /**
    * @param {(boolean|number|string)} value
    * @param {?(boolean|number|string)=} rawValue
+   * @param {string=} debugString Optional string to describe any error condition encountered.
    * @return {!AuditResult}
    */
-  static generateAuditResult(value, rawValue) {
+  static generateAuditResult(value, rawValue, debugString) {
     return {
-      value: value,
-      rawValue: rawValue,
+      value,
+      rawValue,
+      debugString,
       name: this.name,
       tags: this.tags,
       description: this.description

diff --git a/audits/manifest/background-color.js b/audits/manifest/background-color.js
@@ -42,7 +42,7 @@ class ManifestBackgroundColor extends Audit {
   }
 
   /**
-   * @param {!Manifest} manifest
+   * @param {!Manifest=} manifest
    * @return {boolean}
    */
   static hasBackgroundColorValue(manifest) {
@@ -56,7 +56,8 @@ class ManifestBackgroundColor extends Audit {
    * @return {!AuditResult}
    */
   static audit(artifacts) {
-    const hasBackgroundColor = ManifestBackgroundColor.hasBackgroundColorValue(artifacts.manifest);
+    const hasBackgroundColor = ManifestBackgroundColor
+        .hasBackgroundColorValue(artifacts.manifest.value);
 
     return ManifestBackgroundColor.generateAuditResult(hasBackgroundColor);
   }

diff --git a/audits/manifest/exists.js b/audits/manifest/exists.js
@@ -47,7 +47,9 @@ class ManifestExists extends Audit {
    */
   static audit(artifacts) {
     return ManifestExists.generateAuditResult(
-      typeof artifacts.manifest !== 'undefined'
+      typeof artifacts.manifest.value !== 'undefined',
+      undefined,
+      artifacts.manifest.debugString
     );
   }
 }

diff --git a/audits/manifest/icons-192.js b/audits/manifest/icons-192.js
@@ -47,7 +47,7 @@ class ManifestIcons192 extends Audit {
    */
   static audit(artifacts) {
     let hasIcons = false;
-    const manifest = artifacts.manifest;
+    const manifest = artifacts.manifest.value;
 
     if (manifest && manifest.icons.value) {
       const icons192 = manifest.icons.value.find(icon => {

diff --git a/audits/manifest/icons.js b/audits/manifest/icons.js
@@ -47,9 +47,9 @@ class ManifestIcons extends Audit {
    */
   static audit(artifacts) {
     let hasIcons = false;
-    const manifest = artifacts.manifest;
+    const manifest = artifacts.manifest.value;
 
-    if (manifest && manifest.icons && manifest.icons.value) {
+    if (manifest && manifest.icons.value) {
       hasIcons = (manifest.icons.value.length > 0);
     }
 

diff --git a/audits/manifest/name.js b/audits/manifest/name.js
@@ -47,7 +47,7 @@ class ManifestName extends Audit {
    */
   static audit(artifacts) {
     let hasName = false;
-    const manifest = artifacts.manifest;
+    const manifest = artifacts.manifest.value;
 
     if (manifest && manifest.name) {
       hasName = (!!manifest.name.value);

diff --git a/audits/manifest/short-name.js b/audits/manifest/short-name.js
@@ -47,7 +47,7 @@ class ManifestShortName extends Audit {
    */
   static audit(artifacts) {
     let hasShortName = false;
-    const manifest = artifacts.manifest;
+    const manifest = artifacts.manifest.value;
 
     if (manifest && manifest.short_name) {
       hasShortName = (!!manifest.short_name.value);

diff --git a/audits/manifest/start-url.js b/audits/manifest/start-url.js
@@ -47,7 +47,7 @@ class ManifestStartUrl extends Audit {
    */
   static audit(artifacts) {
     let hasStartUrl = false;
-    const manifest = artifacts.manifest;
+    const manifest = artifacts.manifest.value;
 
     if (manifest && manifest.start_url) {
       hasStartUrl = (!!manifest.start_url.value);

diff --git a/audits/manifest/theme-color.js b/audits/manifest/theme-color.js
@@ -47,7 +47,7 @@ class ManifestThemeColor extends Audit {
    */
   static audit(artifacts) {
     let hasThemeColor = false;
-    const manifest = artifacts.manifest;
+    const manifest = artifacts.manifest.value;
 
     if (manifest && manifest.theme_color) {
       hasThemeColor = (!!manifest.theme_color.value);

diff --git a/cli/printer.js b/cli/printer.js
@@ -33,6 +33,9 @@ const Printer = {
           lineItem += ` (${subitem.rawValue})`;
         }
         output.log(lineItem);
+        if (subitem.debugString) {
+          output.log(`    ${subitem.debugString}`);
+        }
       });
 
       output.log('');

diff --git a/closure/typedefs/Artifacts.js b/closure/typedefs/Artifacts.js
@@ -38,7 +38,7 @@ Artifacts.prototype.networkRecords;
 /** @type {?} */
 Artifacts.prototype.traceContents;
 
-/** @type {!Manifest} */
+/** @type {!ManifestNode<(!Manifest|undefined)>} */
 Artifacts.prototype.manifest;
 
 /** @type {!ServiceWorkerVersions} */

diff --git a/closure/typedefs/AuditResult.js b/closure/typedefs/AuditResult.js
@@ -32,6 +32,9 @@ AuditResult.prototype.value;
 /** @type {(boolean|number|string|undefined|null)} */
 AuditResult.prototype.rawValue;
 
+/** @type {(string|undefined)} */
+AuditResult.prototype.debugString;
+
 /** @type {string} */
 AuditResult.prototype.name;
 

diff --git a/closure/typedefs/Manifest.js b/closure/typedefs/Manifest.js
@@ -34,7 +34,7 @@ ManifestNode.prototype.raw;
 ManifestNode.prototype.value;
 
 /** @type {string} */
-ManifestNode.prototype.warning;
+ManifestNode.prototype.debugString;
 
 /**
  * @struct

diff --git a/extension/app/scripts.babel/app.js b/extension/app/scripts.babel/app.js
@@ -32,8 +32,6 @@ document.addEventListener('DOMContentLoaded', _ => {
 
   runPwaAudits(chrome).then(ret => {
     resultsEl.innerHTML = ret;
-  }, err => {
-    resultsEl.innerHTML = `<div class="error">Unable to audit page: ${err.message}</div>`;
   });
 });
 

diff --git a/extension/app/scripts.babel/pwa-check.js b/extension/app/scripts.babel/pwa-check.js
@@ -54,6 +54,15 @@ const aggregators = [
   require('../../../aggregators/is-sized-for-mobile-screen')
 ];
 
+function escapeHTML(str) {
+  return str.replace(/&/g, '&amp;')
+    .replace(/"/g, '&quot;')
+    .replace(/'/g, '&#39;')
+    .replace(/>/g, '&gt;')
+    .replace(/</g, '&lt;')
+    .replace(/`/g, '&#96;');
+}
+
 function createResultsHTML(results) {
   let resultsHTML = '';
 
@@ -65,15 +74,18 @@ function createResultsHTML(results) {
 
     let groupHTML = '';
     item.score.subItems.forEach(subitem => {
+      const debugString = subitem.debugString ? ` title="${escapeHTML(subitem.debugString)}"` : '';
+
       // TODO: make this work with numeric values.
       const status = subitem.value ?
-          '<span class="pass">Pass</span>' : '<span class="fail">Fail</span>';
-      groupHTML += `<li>${subitem.description}: ${status}</li>`;
+          `<span class="pass" ${debugString}>Pass</span>` :
+          `<span class="fail" ${debugString}>Fail</span>`;
+      groupHTML += `<li>${escapeHTML(subitem.description)}: ${status}</li>`;
     });
 
     resultsHTML +=
       `<li class="${groupClass}">
-        <span class="group-name">${item.name}</span>
+        <span class="group-name">${escapeHTML(item.name)}</span>
         <span class="group-score">(${score}%)</span>
         <ul>
           ${groupHTML}
@@ -91,5 +103,6 @@ export function runPwaAudits() {
     .then(results => Aggregator.aggregate(aggregators, results))
     .then(results => {
       return createResultsHTML(results);
-    });
+    })
+    .catch(err => `<div class="error">Unable to audit page: ${escapeHTML(err.message)}</div>`);
 }
diff --git a/extension/app/styles/lighthouse.css b/extension/app/styles/lighthouse.css
@@ -121,6 +121,11 @@ header h2 {
   margin: 0;
 }
 
+.results [title] {
+  cursor: help;
+  border-bottom: 1px dashed black;
+}
+
 .results .pass {
   color: #44A736;
 }

diff --git a/gatherers/manifest.js b/gatherers/manifest.js
@@ -35,15 +35,25 @@ class Manifest extends Gather {
       const finalURL = url.resolve(options.url, manifestURL);
 
       request(finalURL, function(err, response, body) {
-        if (err) {
-          return resolve('');
+        if (err || response.statusCode >= 400) {
+          return reject(`${response.statusCode}: ${response.statusMessage}`);
         }
 
         resolve(body);
       });
     });
   }
 
+  static _errorManifest(errorString) {
+    return {
+      manifest: {
+        raw: undefined,
+        value: undefined,
+        debugString: errorString
+      }
+    };
+  }
+
   static gather(options) {
     const driver = options.driver;
     /**
@@ -52,12 +62,26 @@ class Manifest extends Gather {
      * resource is tracked in issue #83
      */
     return driver.querySelector('head link[rel="manifest"]')
-      .then(node => node && node.getAttribute('href'))
-      .then(manifestURL => manifestURL && Manifest._loadFromURL(options, manifestURL))
-      .then(manifestContent => {
-        return {
-          manifest: manifestParser(manifestContent).value
-        };
+      .then(node => {
+        if (!node) {
+          return this._errorManifest('No <link rel="manifest"> found in DOM.');
+        }
+
+        return node.getAttribute('href').then(manifestURL => {
+          if (!manifestURL) {
+            return this._errorManifest('No href found on <link rel="manifest">.');
+          }
+
+          return Manifest._loadFromURL(options, manifestURL)
+            .then(manifestContent => {
+              return {
+                manifest: manifestParser(manifestContent)
+              };
+            })
+            .catch(reason => {
+              return this._errorManifest(`Unable to fetch manifest at ${manifestURL}: ${reason}.`);
+            });
+        });
       });
   }
 }