Repository: GoogleChrome/lighthouse
Pull request #12462 (2021-05-11 00:52:42)

misc(treemap): fix colors
1) Changed the `stableHasher` function to recycle items, so that colors never run out.
2) I had added a fake jquery duplication to debug.json, but it turns out that there were visible duplicates but they were given `white` because of 1). So I removed the fake duplication.
3) Refactored ViewMode.highlight. I figured this property should be responsible for declaring what color a specific node should be.

ref #11254


Patch:
diff --git a/lighthouse-core/fraggle-rock/config/default-config.js b/lighthouse-core/fraggle-rock/config/default-config.js
index 38f3eb305..ac8d8e6d1 100644
--- a/lighthouse-core/fraggle-rock/config/default-config.js
+++ b/lighthouse-core/fraggle-rock/config/default-config.js
@@ -17,6 +17,7 @@ const artifacts = {
   AppCacheManifest: '',
   CacheContents: '',
   ConsoleMessages: '',
+  CSSUsage: '',
   Doctype: '',
   DOMStats: '',
   EmbeddedContent: '',
@@ -56,6 +57,7 @@ const defaultConfig = {
     {id: artifacts.AppCacheManifest, gatherer: 'dobetterweb/appcache'},
     {id: artifacts.CacheContents, gatherer: 'cache-contents'},
     {id: artifacts.ConsoleMessages, gatherer: 'console-messages'},
+    {id: artifacts.CSSUsage, gatherer: 'css-usage'},
     {id: artifacts.Doctype, gatherer: 'dobetterweb/doctype'},
     {id: artifacts.DOMStats, gatherer: 'dobetterweb/domstats'},
     {id: artifacts.EmbeddedContent, gatherer: 'seo/embedded-content'},
@@ -93,6 +95,7 @@ const defaultConfig = {
         artifacts.AppCacheManifest,
         artifacts.CacheContents,
         artifacts.ConsoleMessages,
+        artifacts.CSSUsage,
         artifacts.Doctype,
         artifacts.DOMStats,
         artifacts.EmbeddedContent,
diff --git a/lighthouse-core/gather/gatherers/css-usage.js b/lighthouse-core/gather/gatherers/css-usage.js
index cb2c8244e..c83a0d8b0 100644
--- a/lighthouse-core/gather/gatherers/css-usage.js
+++ b/lighthouse-core/gather/gatherers/css-usage.js
@@ -5,35 +5,46 @@
  */
 'use strict';
 
-const Gatherer = require('./gatherer.js');
+const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
 
 /**
  * @fileoverview Tracks unused CSS rules.
  */
-class CSSUsage extends Gatherer {
+class CSSUsage extends FRGatherer {
+  /** @type {LH.Gatherer.GathererMeta} */
+  meta = {
+    // TODO(FR-COMPAT): Add support for timespan.
+    supportedModes: ['snapshot', 'navigation'],
+  };
+
   /**
-   * @param {LH.Gatherer.PassContext} passContext
+   * @param {LH.Gatherer.FRTransitionalContext} context
    * @return {Promise<LH.Artifacts['CSSUsage']>}
    */
-  async afterPass(passContext) {
-    const driver = passContext.driver;
+  async getArtifact(context) {
+    const session = context.driver.defaultSession;
+    const executionContext = context.driver.executionContext;
 
     /** @type {Array<LH.Crdp.CSS.StyleSheetAddedEvent>} */
     const stylesheets = [];
     /** @param {LH.Crdp.CSS.StyleSheetAddedEvent} sheet */
     const onStylesheetAdded = sheet => stylesheets.push(sheet);
-    driver.on('CSS.styleSheetAdded', onStylesheetAdded);
+    session.on('CSS.styleSheetAdded', onStylesheetAdded);
+
+    await session.sendCommand('DOM.enable');
+    await session.sendCommand('CSS.enable');
+    await session.sendCommand('CSS.startRuleUsageTracking');
+
+    // Force style to recompute.
+    // Doesn't appear to be necessary in newer versions of Chrome.
+    await executionContext.evaluateAsync('getComputedStyle(document.body)');
 
-    await driver.sendCommand('DOM.enable');
-    await driver.sendCommand('CSS.enable');
-    await driver.sendCommand('CSS.startRuleUsageTracking');
-    await driver.executionContext.evaluateAsync('getComputedStyle(document.body)');
-    driver.off('CSS.styleSheetAdded', onStylesheetAdded);
+    session.off('CSS.styleSheetAdded', onStylesheetAdded);
 
     // Fetch style sheet content in parallel.
     const promises = stylesheets.map(sheet => {
       const styleSheetId = sheet.header.styleSheetId;
-      return driver.sendCommand('CSS.getStyleSheetText', {styleSheetId}).then(content => {
+      return session.sendCommand('CSS.getStyleSheetText', {styleSheetId}).then(content => {
         return {
           header: sheet.header,
           content: content.text,
@@ -42,9 +53,9 @@ class CSSUsage extends Gatherer {
     });
     const styleSheetInfo = await Promise.all(promises);
 
-    const ruleUsageResponse = await driver.sendCommand('CSS.stopRuleUsageTracking');
-    await driver.sendCommand('CSS.disable');
-    await driver.sendCommand('DOM.disable');
+    const ruleUsageResponse = await session.sendCommand('CSS.stopRuleUsageTracking');
+    await session.sendCommand('CSS.disable');
+    await session.sendCommand('DOM.disable');
 
     const dedupedStylesheets = new Map(styleSheetInfo.map(sheet => {
       return [sheet.content, sheet];
diff --git a/lighthouse-treemap/app/debug.json b/lighthouse-treemap/app/debug.json
index a5e65b92f..a9421d8a8 100644
--- a/lighthouse-treemap/app/debug.json
+++ b/lighthouse-treemap/app/debug.json
@@ -12221,8 +12221,7 @@
                             {
                               "name": "jquery/2.2.4/jquery.min.js",
                               "resourceBytes": 85500,
-                              "unusedBytes": 35751,
-                              "duplicatedNormalizedModuleName": "jquery/2.2.4/jquery.min.js"
+                              "unusedBytes": 35751
                             },
                             {
                               "name": "angular.js/1.3.20",
@@ -12454,16 +12453,9 @@
             },
             {
               "name": "https://www.coursehero.com/sym-assets/js/bundle-8ed4419-3cfb752.js",
-              "resourceBytes": 1311780,
+              "resourceBytes": 1111780,
               "unusedBytes": 685723,
               "children": [
-                {
-                  "_comment": "fake data, added for duplicatedNormalizedModuleName",
-                  "name": "jquery/2.2.4/jquery.min.js",
-                  "resourceBytes": 200000,
-                  "unusedBytes": 0,
-                  "duplicatedNormalizedModuleName": "jquery/2.2.4/jquery.min.js"
-                },
                 {
                   "name": "coursehero:///",
                   "resourceBytes": 1111780,
diff --git a/lighthouse-treemap/app/src/main.js b/lighthouse-treemap/app/src/main.js
index 7207ef72f..bcd9c6852 100644
--- a/lighthouse-treemap/app/src/main.js
+++ b/lighthouse-treemap/app/src/main.js
@@ -66,7 +66,6 @@ class TreemapViewer {
     this.documentUrl = options.lhr.requestedUrl;
     this.el = el;
     this.getHueForD1NodeName = TreemapUtil.stableHasher(TreemapUtil.COLOR_HUES);
-    this.getHueForModuleNodeName = TreemapUtil.stableHasher(TreemapUtil.COLOR_HUES);
 
     /* eslint-disable no-unused-expressions */
     /** @type {LH.Treemap.Node} */
@@ -198,8 +197,8 @@ class TreemapViewer {
     function createUnusedBytesViewMode(root) {
       if (root.unusedBytes === undefined) return;
 
-      /** @type {LH.Treemap.NodePath[]} */
-      const highlightNodePaths = [];
+      /** @type {LH.Treemap.Highlight[]} */
+      const highlights = [];
       for (const d1Node of root.children || []) {
         // Only highlight leaf nodes if entire node (ie a JS bundle) has greater than a certain
         // number of unused bytes.
@@ -213,14 +212,14 @@ class TreemapViewer {
             return;
           }
 
-          highlightNodePaths.push([root.name, ...path]);
+          highlights.push({path: [root.name, ...path]});
         });
       }
       return {
         id: 'unused-bytes',
         label: 'Unused Bytes',
         subLabel: TreemapUtil.formatBytes(root.unusedBytes),
-        highlightNodePaths,
+        highlights,
         enabled: true,
       };
     }
@@ -230,7 +229,7 @@ class TreemapViewer {
      * @return {LH.Treemap.ViewMode|undefined}
      */
     const createDuplicateModulesViewMode = (root) => {
-      /** @type {Map<string, Array<{node: LH.Treemap.Node, path: string[]}>>} */
+      /** @type {Map<string, Array<{node: LH.Treemap.Node, path: LH.Treemap.NodePath}>>} */
       const moduleNameToNodes = new Map();
       for (const d1Node of root.children || []) {
         TreemapUtil.walk(d1Node, (node, path) => {
@@ -243,11 +242,12 @@ class TreemapViewer {
         });
       }
 
+      const getHueForModuleNodeName = TreemapUtil.stableHasher(TreemapUtil.COLOR_HUES);
       let potentialByteSavings = 0;
 
-      /** @type {LH.Treemap.NodePath[]} */
-      const highlightNodePaths = [];
-      for (const nodesWithSameModuleName of moduleNameToNodes.values()) {
+      /** @type {LH.Treemap.Highlight[]} */
+      const highlights = [];
+      for (const [moduleName, nodesWithSameModuleName] of moduleNameToNodes.entries()) {
         if (nodesWithSameModuleName.length === 1) continue;
 
         const bytes = [];
@@ -262,13 +262,16 @@ class TreemapViewer {
         if (duplicatedBytes < DUPLICATED_MODULES_IGNORE_THRESHOLD) continue;
 
         for (const {path} of nodesWithSameModuleName) {
-          highlightNodePaths.push([root.name, ...path]);
+          highlights.push({
+            path: [root.name, ...path],
+            color: this.getColorFromHue(getHueForModuleNodeName(moduleName)),
+          });
         }
         potentialByteSavings += duplicatedBytes;
       }
 
       let enabled = true;
-      if (highlightNodePaths.length === 0) enabled = false;
+      if (highlights.length === 0) enabled = false;
       if (potentialByteSavings / root.resourceBytes < DUPLICATED_MODULES_IGNORE_ROOT_RATIO) {
         enabled = false;
       }
@@ -277,7 +280,7 @@ class TreemapViewer {
         id: 'duplicate-modules',
         label: 'Duplicate Modules',
         subLabel: enabled ? TreemapUtil.formatBytes(potentialByteSavings) : 'N/A',
-        highlightNodePaths,
+        highlights,
         enabled,
       };
     };
@@ -538,42 +541,40 @@ class TreemapViewer {
     return parts.join(' · ');
   }
 
+  /**
+   * @param {number} hue
+   */
+  getColorFromHue(hue) {
+    return TreemapUtil.hsl(hue, 60, 90);
+  }
+
   updateColors() {
     TreemapUtil.walk(this.currentTreemapRoot, node => {
-      let hue;
-      if (this.currentViewMode.id === 'duplicate-modules') {
-        hue = this.getHueForModuleNodeName(node.duplicatedNormalizedModuleName || '');
-      } else {
-        // Color a depth one node and all children the same color.
-        const depthOneNode = this.nodeToDepthOneNodeMap.get(node);
-        hue = this.getHueForD1NodeName(depthOneNode ? depthOneNode.name : node.name);
-      }
-
-      let backgroundColor = 'white';
-      let color = 'black';
-
-      if (hue !== undefined) {
-        const sat = 60;
-        const lig = 90;
-        backgroundColor = TreemapUtil.hsl(hue, sat, lig);
-        color = lig > 50 ? 'black' : 'white';
-      } else {
-        // Ran out of colors.
-      }
+      // Color a depth one node and all children the same color.
+      const depthOneNode = this.nodeToDepthOneNodeMap.get(node);
+      const hue = depthOneNode &&
+        this.getHueForD1NodeName(depthOneNode ? depthOneNode.name : node.name);
+      const depthOneNodeColor = hue !== undefined ? this.getColorFromHue(hue) : 'white';
 
-      // A view can set nodes to highlight. If so, don't color anything else.
-      if (this.currentViewMode.highlightNodePaths) {
+      let backgroundColor;
+      if (this.currentViewMode.highlights) {
+        // A view can set nodes to highlight. If so, don't color anything else.
         const path = this.nodeToPathMap.get(node);
-        const shouldHighlight = path && this.currentViewMode.highlightNodePaths
-          .some(pathToHighlight => TreemapUtil.pathsAreEqual(pathToHighlight, path));
-        if (!shouldHighlight) backgroundColor = 'white';
+        const highlight = path && this.currentViewMode.highlights
+          .find(highlight => TreemapUtil.pathsAreEqual(path, highlight.path));
+        if (highlight) {
+          backgroundColor = highlight.color || depthOneNodeColor;
+        } else {
+          backgroundColor = 'white';
+        }
+      } else {
+        backgroundColor = depthOneNodeColor;
       }
 
       // @ts-ignore: webtreemap will add a dom node property to every node.
       const dom = /** @type {HTMLElement?} */ (node.dom);
       if (dom) {
         dom.style.backgroundColor = backgroundColor;
-        dom.style.color = color;
       }
     });
   }
diff --git a/lighthouse-treemap/app/src/util.js b/lighthouse-treemap/app/src/util.js
index d09cceb4f..351913d67 100644
--- a/lighthouse-treemap/app/src/util.js
+++ b/lighthouse-treemap/app/src/util.js
@@ -148,21 +148,23 @@ class TreemapUtil {
    * The hash function is stable and deterministic, so the same key->item mapping will be
    * produced given the same call order.
    * @template T
-   * @param {T[]} items
-   * @return {(key: string) => T|undefined}
+   * @param {T[]} originalItems
+   * @return {(key: string) => T}
    */
-  static stableHasher(items) {
-    // Clone.
-    items = [...items];
+  static stableHasher(originalItems) {
+    let items = [...originalItems];
 
     /** @type {Map<string, T>} */
     const assignedItems = new Map();
     return key => {
       // Key has already been assigned an item.
-      if (assignedItems.has(key)) return assignedItems.get(key);
+      const alreadyAssignedItem = assignedItems.get(key);
+      if (alreadyAssignedItem !== undefined) return alreadyAssignedItem;
 
       // Ran out of items.
-      if (items.length === 0) return;
+      if (items.length === 0) {
+        items = [...originalItems];
+      }
 
       // Select a random item using a stable hash.
       const hash = [...key].reduce((acc, char) => acc + char.charCodeAt(0), 0);
diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts
index b9a45a342..dd63c33a1 100644
--- a/types/artifacts.d.ts
+++ b/types/artifacts.d.ts
@@ -20,7 +20,6 @@ declare global {
     export interface Artifacts extends BaseArtifacts, GathererArtifacts {}
 
     export type FRArtifacts = StrictOmit<Artifacts,
-      | 'CSSUsage'
       | 'Fonts'
       | 'FullPageScreenshot'
       | 'HTTPRedirect'
diff --git a/types/treemap.d.ts b/types/treemap.d.ts
index 7945036be..e1ec26251 100644
--- a/types/treemap.d.ts
+++ b/types/treemap.d.ts
@@ -17,13 +17,19 @@ declare global {
       value: string;
     }
 
+    interface Highlight {
+      path: NodePath;
+      /** If not set, will use the color based on the d1Node. */
+      color?: string;
+    }
+
     interface ViewMode {
       id: 'all' | 'unused-bytes' | 'duplicate-modules';
       label: string;
       subLabel: string;
-      partitionBy?: 'resourceBytes' | 'unusedBytes';
-      highlightNodePaths?: NodePath[];
       enabled: boolean;
+      partitionBy?: 'resourceBytes' | 'unusedBytes';
+      highlights?: Highlight[];
     }
 
     interface Node {


Test patch:
diff --git a/lighthouse-core/test/gather/gatherers/css-usage-test.js b/lighthouse-core/test/gather/gatherers/css-usage-test.js
new file mode 100644
index 000000000..b52c60dc7
--- /dev/null
+++ b/lighthouse-core/test/gather/gatherers/css-usage-test.js
@@ -0,0 +1,112 @@
+/**
+ * @license Copyright 2021 The Lighthouse Authors. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+'use strict';
+
+/* eslint-env jest */
+
+const CSSUsage = require('../../../gather/gatherers/css-usage.js');
+const {createMockDriver} = require('../../fraggle-rock/gather/mock-driver.js');
+
+describe('.getArtifact', () => {
+  it('gets CSS usage', async () => {
+    const driver = createMockDriver();
+    driver.defaultSession.on
+      .mockEvent('CSS.styleSheetAdded', {header: {styleSheetId: '1'}})
+      .mockEvent('CSS.styleSheetAdded', {header: {styleSheetId: '2'}});
+    driver.defaultSession.sendCommand
+      .mockResponse('DOM.enable')
+      .mockResponse('CSS.enable', undefined, 1) // Force events to emit
+      .mockResponse('CSS.startRuleUsageTracking')
+      .mockResponse('CSS.getStyleSheetText', {text: 'CSS text 1'})
+      .mockResponse('CSS.getStyleSheetText', {text: 'CSS text 2'})
+      .mockResponse('CSS.stopRuleUsageTracking', {
+        ruleUsage: [
+          {styleSheetId: '1', used: true},
+          {styleSheetId: '2', used: false},
+        ],
+      })
+      .mockResponse('CSS.disable')
+      .mockResponse('DOM.disable');
+
+    /** @type {LH.Gatherer.FRTransitionalContext} */
+    const context = {
+      driver: driver.asDriver(),
+      url: 'https://example.com',
+      gatherMode: 'snapshot',
+      dependencies: {},
+    };
+    const gatherer = new CSSUsage();
+    const artifact = await gatherer.getArtifact(context);
+
+    expect(artifact).toEqual({
+      stylesheets: [
+        {
+          header: {styleSheetId: '1'},
+          content: 'CSS text 1',
+        },
+        {
+          header: {styleSheetId: '2'},
+          content: 'CSS text 2',
+        },
+      ],
+      rules: [
+        {
+          styleSheetId: '1',
+          used: true,
+        },
+        {
+          styleSheetId: '2',
+          used: false,
+        },
+      ],
+    });
+  });
+
+  it('dedupes stylesheets', async () => {
+    const driver = createMockDriver();
+    driver.defaultSession.on
+      .mockEvent('CSS.styleSheetAdded', {header: {styleSheetId: '1'}})
+      .mockEvent('CSS.styleSheetAdded', {header: {styleSheetId: '1'}});
+    driver.defaultSession.sendCommand
+      .mockResponse('DOM.enable')
+      .mockResponse('CSS.enable', undefined, 1) // Force events to emit
+      .mockResponse('CSS.startRuleUsageTracking')
+      .mockResponse('CSS.getStyleSheetText', {text: 'CSS text 1'})
+      .mockResponse('CSS.getStyleSheetText', {text: 'CSS text 1'})
+      .mockResponse('CSS.stopRuleUsageTracking', {
+        ruleUsage: [
+          {styleSheetId: '1', used: true},
+        ],
+      })
+      .mockResponse('CSS.disable')
+      .mockResponse('DOM.disable');
+
+    /** @type {LH.Gatherer.FRTransitionalContext} */
+    const context = {
+      driver: driver.asDriver(),
+      url: 'https://example.com',
+      gatherMode: 'snapshot',
+      dependencies: {},
+    };
+    const gatherer = new CSSUsage();
+    const artifact = await gatherer.getArtifact(context);
+
+    expect(artifact).toEqual({
+      stylesheets: [
+        {
+          header: {styleSheetId: '1'},
+          content: 'CSS text 1',
+        },
+      ],
+      rules: [
+        {
+          styleSheetId: '1',
+          used: true,
+        },
+      ],
+    });
+  });
+});
diff --git a/lighthouse-treemap/test/util-test.js b/lighthouse-treemap/test/util-test.js
index 500768cc8..7fc2d1586 100644
--- a/lighthouse-treemap/test/util-test.js
+++ b/lighthouse-treemap/test/util-test.js
@@ -34,6 +34,7 @@ describe('TreemapUtil', () => {
   });
 
   it('stableHasher works', () => {
+    const values = [1, 2, 3, 4, 5];
     let hasher = TreemapUtil.stableHasher([1, 2, 3, 4, 5]);
     const expectedValues = [
       hasher('value0'),
@@ -43,13 +44,18 @@ describe('TreemapUtil', () => {
       hasher('value4'),
       hasher('value5'),
     ];
+
+    for (const expectedValue of expectedValues) {
+      expect(values).toContain(expectedValue);
+    }
+
     // Expect the same values using the same invocation.
     expect(hasher('value0')).toBe(expectedValues[0]);
     expect(hasher('value1')).toBe(expectedValues[1]);
     expect(hasher('value2')).toBe(expectedValues[2]);
     expect(hasher('value3')).toBe(expectedValues[3]);
     expect(hasher('value4')).toBe(expectedValues[4]);
-    expect(hasher('value5')).toBeUndefined();
+    expect(hasher('value5')).toBe(expectedValues[5]);
 
     // Repeat, expecting the same values.
     hasher = TreemapUtil.stableHasher([1, 2, 3, 4, 5]);
@@ -58,6 +64,9 @@ describe('TreemapUtil', () => {
     expect(hasher('value2')).toBe(expectedValues[2]);
     expect(hasher('value3')).toBe(expectedValues[3]);
     expect(hasher('value4')).toBe(expectedValues[4]);
-    expect(hasher('value5')).toBeUndefined();
+    expect(hasher('value5')).toBe(expectedValues[5]);
+
+    // Expect values array is not modified.
+    expect(values).toEqual([1, 2, 3, 4, 5]);
   });
 });