pauls feedback

GoogleChrome · Feb 15, 2017 · 909d013 · 909d013
1 parent 8f93c45
commit 909d013
Show file tree

Hide file tree

Showing 8 changed files with 54 additions and 28 deletions.
diff --git a/lighthouse-cli/test/fixtures/byte-efficiency/tester.html b/lighthouse-cli/test/fixtures/byte-efficiency/tester.html
@@ -80,7 +80,7 @@ <h2>Byte efficiency tester page</h2>
 // PASSWARN: unused and a bit of savings
 generateInlineScriptWithSize(2000, '.kinda-unused { background: none; }\n');
 // FAIL: unused and lots of savings
-generateInlineScriptWithSize(10000, '.definitely-unused { background: none; }\n');
+generateInlineScriptWithSize(24000, '.definitely-unused { background: none; }\n');
 </script>
 
 </body>

diff --git a/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js b/lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js
@@ -20,12 +20,31 @@ const Audit = require('../audit');
 const Formatter = require('../../formatters/formatter');
 
 const KB_IN_BYTES = 1024;
+const WASTEFUL_THRESHOLD_IN_BYTES = 20 * KB_IN_BYTES;
 
 /**
  * @overview Used as the base for all byte efficiency audits. Computes total bytes
  *    and estimated time saved. Subclass and override `audit_` to return results.
  */
 class UnusedBytes extends Audit {
+  /**
+   * @param {number} bytes
+   * @return {number}
+   */
+  static bytesToKbString(bytes) {
+    return Math.round(bytes / KB_IN_BYTES).toLocaleString() + ' KB';
+  }
+
+  /**
+   * @param {number} bytes
+   * @param {percent} percent
+   */
+  static toSavingsString(bytes = 0, percent = 0) {
+    const kbDisplay = this.bytesToKbString(bytes);
+    const percentDisplay = Math.round(percent).toLocaleString() + '%';
+    return `${kbDisplay} _${percentDisplay}_`;
+  }
+
   /**
    * @param {!Artifacts} artifacts
    * @return {!AuditResult}
@@ -37,26 +56,30 @@ class UnusedBytes extends Audit {
       const debugString = result.debugString;
       const results = result.results
           .map(item => {
-            item.wastedKb = Math.round(item.wastedBytes / KB_IN_BYTES) + ' KB';
-            item.totalKb = Math.round(item.totalBytes / KB_IN_BYTES) + ' KB';
+            item.wastedKb = this.bytesToKbString(item.wastedBytes);
+            item.totalKb = this.bytesToKbString(item.totalBytes);
+            item.potentialSavings = this.toSavingsString(item.wastedBytes, item.wastedPercent);
             return item;
           })
           .sort((itemA, itemB) => itemB.wastedBytes - itemA.wastedBytes);
 
       const wastedBytes = results.reduce((sum, item) => sum + item.wastedBytes, 0);
-      const wastedKb = Math.round(wastedBytes / KB_IN_BYTES);
       // Only round to nearest 10ms since we're relatively hand-wavy
       const wastedMs = Math.round(wastedBytes / networkThroughput * 100) * 10;
 
       let displayValue = '';
-      if (wastedKb > 0 && wastedMs > 0) {
-        displayValue = `${wastedKb}KB (~${wastedMs}ms) potential savings`;
+      if (wastedBytes) {
+        const wastedKbDisplay = this.bytesToKbString(wastedBytes);
+        const wastedMsDisplay = wastedMs.toLocaleString() + 'ms';
+        displayValue = `Potential savings of ${wastedKbDisplay} (~${wastedMsDisplay})`;
       }
 
       return this.generateAuditResult({
         debugString,
         displayValue,
-        rawValue: typeof result.passes === 'undefined' ? !displayValue : !!result.passes,
+        rawValue: typeof result.passes === 'undefined' ?
+            wastedBytes < WASTEFUL_THRESHOLD_IN_BYTES :
+            !!result.passes,
         extendedInfo: {
           formatter: Formatter.SUPPORTED_FORMATS.TABLE,
           value: {results, tableHeadings: result.tableHeadings}

diff --git a/lighthouse-core/audits/byte-efficiency/unused-css-rules.js b/lighthouse-core/audits/byte-efficiency/unused-css-rules.js
@@ -156,8 +156,8 @@ class UnusedCSSRules extends Audit {
       url,
       numUnused,
       wastedBytes,
+      wastedPercent: percentUnused * 100,
       totalBytes,
-      potentialSavings: `${Math.round(percentUnused * 100)}%`,
     };
   }
 
@@ -183,9 +183,8 @@ class UnusedCSSRules extends Audit {
       tableHeadings: {
         url: 'URL',
         numUnused: 'Unused Rules',
-        totalKb: 'Original (KB)',
-        potentialSavings: 'Potential Savings (%)',
-        wastedKb: 'Savings (KB)',
+        totalKb: 'Original',
+        potentialSavings: 'Potential Savings',
       }
     };
   }

diff --git a/lighthouse-core/audits/byte-efficiency/uses-optimized-images.js b/lighthouse-core/audits/byte-efficiency/uses-optimized-images.js
@@ -56,7 +56,7 @@ class UsesOptimizedImages extends Audit {
    */
   static computeSavings(image, type) {
     const bytes = image.originalSize - image[type + 'Size'];
-    const percent = Math.round(100 * bytes / image.originalSize);
+    const percent = 100 * bytes / image.originalSize;
     return {bytes, percent};
   }
 
@@ -96,7 +96,7 @@ class UsesOptimizedImages extends Audit {
           hasAllEfficientImages = false;
         }
         if (jpegSavings.bytes > IGNORE_THRESHOLD_IN_BYTES) {
-          jpegSavingsLabel = `${jpegSavings.percent}%`;
+          jpegSavingsLabel = this.toSavingsString(jpegSavings.bytes, jpegSavings.percent);
         }
       }
 
@@ -107,7 +107,7 @@ class UsesOptimizedImages extends Audit {
         preview: {url: image.url, mimeType: image.mimeType},
         totalBytes: image.originalSize,
         wastedBytes: webpSavings.bytes,
-        webpSavings: `${webpSavings.percent}%`,
+        webpSavings: this.toSavingsString(webpSavings.bytes, webpSavings.percent),
         jpegSavings: jpegSavingsLabel
       });
       return results;
@@ -126,10 +126,9 @@ class UsesOptimizedImages extends Audit {
       tableHeadings: {
         preview: '',
         url: 'URL',
-        totalKb: 'Original (KB)',
-        webpSavings: 'WebP Savings (%)',
-        jpegSavings: 'JPEG Savings (%)',
-        wastedKb: 'Savings (KB)',
+        totalKb: 'Original',
+        webpSavings: 'WebP Savings',
+        jpegSavings: 'JPEG Savings',
       }
     };
   }

diff --git a/lighthouse-core/audits/byte-efficiency/uses-responsive-images.js b/lighthouse-core/audits/byte-efficiency/uses-responsive-images.js
@@ -75,8 +75,8 @@ class UsesResponsiveImages extends Audit {
       },
       totalBytes,
       wastedBytes,
+      wastedPercent: 100 * wastedRatio,
       isWasteful: wastedBytes > WASTEFUL_THRESHOLD_IN_BYTES,
-      potentialSavings: Math.round(100 * wastedRatio) + '%'
     };
   }
 
@@ -117,9 +117,8 @@ class UsesResponsiveImages extends Audit {
       tableHeadings: {
         preview: '',
         url: 'URL',
-        totalKb: 'Original (KB)',
-        potentialSavings: 'Potential Savings (%)',
-        wastedKb: 'Savings (KB)',
+        totalKb: 'Original',
+        potentialSavings: 'Potential Savings',
       }
     };
   }

diff --git a/lighthouse-core/formatters/partials/table.css b/lighthouse-core/formatters/partials/table.css
@@ -28,6 +28,11 @@
 .table_list tr:hover {
   background-color: #fafafa;
 }
+.table_list em {
+  color: #999;
+  font-style: normal;
+  margin-left: 10px;
+}
 .table_list code, .table_list pre {
   white-space: pre;
   font-family: monospace;

diff --git a/lighthouse-core/test/audits/byte-efficiency/unused-css-rules-test.js b/lighthouse-core/test/audits/byte-efficiency/unused-css-rules-test.js
@@ -114,9 +114,9 @@ describe('Best Practices: unused css rules audit', () => {
     });
 
     it('correctly computes potentialSavings', () => {
-      assert.ok(map({used: [], unused: [1, 2]}).potentialSavings, '100%');
-      assert.ok(map({used: [1, 2], unused: [1, 2]}).potentialSavings, '50%');
-      assert.ok(map({used: [1, 2], unused: []}).potentialSavings, '0%');
+      assert.equal(map({used: [], unused: [1, 2]}).wastedPercent, 100);
+      assert.equal(map({used: [1, 2], unused: [1, 2]}).wastedPercent, 50);
+      assert.equal(map({used: [1, 2], unused: []}).wastedPercent, 0);
     });
 
     it('correctly computes url', () => {
@@ -184,6 +184,7 @@ describe('Best Practices: unused css rules audit', () => {
           {styleSheetId: 'a', used: true},
           {styleSheetId: 'a', used: false},
           {styleSheetId: 'a', used: false},
+          {styleSheetId: 'a', used: false},
           {styleSheetId: 'b', used: true},
           {styleSheetId: 'b', used: false},
           {styleSheetId: 'c', used: false},
@@ -207,8 +208,8 @@ describe('Best Practices: unused css rules audit', () => {
       assert.equal(result.results.length, 2);
       assert.equal(result.results[0].totalBytes, 10 * 1024);
       assert.equal(result.results[1].totalBytes, 2050);
-      assert.equal(result.results[0].potentialSavings, '67%');
-      assert.equal(result.results[1].potentialSavings, '50%');
+      assert.equal(result.results[0].wastedPercent, 75);
+      assert.equal(result.results[1].wastedPercent, 50);
     });
 
     it('does not include duplicate sheets', () => {

diff --git a/lighthouse-core/test/audits/byte-efficiency/uses-optimized-images-test.js b/lighthouse-core/test/audits/byte-efficiency/uses-optimized-images-test.js
@@ -70,7 +70,7 @@ describe('Page uses optimized images', () => {
     const headings = auditResult.tableHeadings;
     assert.equal(auditResult.passes, false);
     assert.deepEqual(Object.keys(headings).map(key => headings[key]),
-        ['', 'URL', 'Original (KB)', 'WebP Savings (%)', 'JPEG Savings (%)', 'Savings (KB)'],
+        ['', 'URL', 'Original', 'WebP Savings', 'JPEG Savings'],
         'table headings are correct and in order');
   });