Repository: GoogleChrome/lighthouse
Pull request #5906 (2018-08-24 21:27:31)

tests(dbw): remove createShadowRoot call
master is failing again because of another canary change, `createShadowRoot` is going to be removed in M73, so I just swapped it for one of our other deprecations


Patch:
diff --git a/lighthouse-core/audits/uses-rel-preload.js b/lighthouse-core/audits/uses-rel-preload.js
index 9c4bc48b7..2ec6cb374 100644
--- a/lighthouse-core/audits/uses-rel-preload.js
+++ b/lighthouse-core/audits/uses-rel-preload.js
@@ -8,6 +8,7 @@
 const URL = require('../lib/url-shim');
 const Audit = require('./audit');
 const UnusedBytes = require('./byte-efficiency/byte-efficiency-audit');
+const CriticalRequestChains = require('../gather/computed/critical-request-chains');
 const i18n = require('../lib/i18n');
 
 const UIStrings = {
@@ -37,49 +38,49 @@ class UsesRelPreloadAudit extends Audit {
   }
 
   /**
-   * @param {LH.Artifacts.CriticalRequestNode} chains
-   * @param {number} maxLevel
-   * @param {number=} minLevel
+   * @param {LH.Artifacts.NetworkRequest} mainResource
+   * @param {LH.Gatherer.Simulation.GraphNode} graph
+   * @return {Set<string>}
    */
-  static _flattenRequests(chains, maxLevel, minLevel = 0) {
-    /** @type {Array<LH.Artifacts.NetworkRequest>} */
-    const requests = [];
-
-    /**
-     * @param {LH.Artifacts.CriticalRequestNode} chains
-     * @param {number} level
-     */
-    const flatten = (chains, level) => {
-      Object.keys(chains).forEach(chain => {
-        if (chains[chain]) {
-          const currentChain = chains[chain];
-          if (level >= minLevel) {
-            requests.push(currentChain.request);
-          }
-
-          if (level < maxLevel) {
-            flatten(currentChain.children, level + 1);
-          }
-        }
-      });
-    };
+  static getURLsToPreload(mainResource, graph) {
+    /** @type {Set<string>} */
+    const urls = new Set();
 
-    flatten(chains, 0);
+    graph.traverse((node, traversalPath) => {
+      if (node.type !== 'network') return;
+      // Don't include the node itself or any CPU nodes in the initiatorPath
+      const path = traversalPath.slice(1).filter(initiator => initiator.type === 'network');
+      if (!UsesRelPreloadAudit.shouldPreloadRequest(node.record, mainResource, path)) return;
+      urls.add(node.record.url);
+    });
 
-    return requests;
+    return urls;
   }
 
   /**
+   * We want to preload all first party critical requests at depth 2.
+   * Third party requests can be tricky to know the URL ahead of time.
+   * Critical requests at depth 1 would already be identified by the browser for preloading.
+   * Critical requests deeper than depth 2 are more likely to be a case-by-case basis such that it
+   * would be a little risky to recommend blindly.
    *
    * @param {LH.Artifacts.NetworkRequest} request
    * @param {LH.Artifacts.NetworkRequest} mainResource
+   * @param {Array<LH.Gatherer.Simulation.GraphNode>} initiatorPath
    * @return {boolean}
    */
-  static shouldPreload(request, mainResource) {
-    if (request.isLinkPreload || URL.NON_NETWORK_PROTOCOLS.includes(request.protocol)) {
-      return false;
-    }
-
+  static shouldPreloadRequest(request, mainResource, initiatorPath) {
+    const mainResourceDepth = mainResource.redirects ? mainResource.redirects.length : 0;
+
+    // If it's already preloaded, no need to recommend it.
+    if (request.isLinkPreload) return false;
+    // It's not critical, don't recommend it.
+    if (!CriticalRequestChains.isCritical(request, mainResource)) return false;
+    // It's not a request loaded over the network, don't recommend it.
+    if (URL.NON_NETWORK_PROTOCOLS.includes(request.protocol)) return false;
+    // It's not at the right depth, don't recommend it.
+    if (initiatorPath.length !== mainResourceDepth + 2) return false;
+    // We survived everything else, just check that it's a first party request.
     return URL.rootDomainsMatch(request.url, mainResource.url);
   }
 
@@ -168,28 +169,13 @@ class UsesRelPreloadAudit extends Audit {
     const URL = artifacts.URL;
     const simulatorOptions = {trace, devtoolsLog, settings: context.settings};
 
-    const [critChains, mainResource, graph, simulator] = await Promise.all([
-      // TODO(phulce): eliminate dependency on CRC
-      artifacts.requestCriticalRequestChains({devtoolsLog, URL}),
+    const [mainResource, graph, simulator] = await Promise.all([
       artifacts.requestMainResource({devtoolsLog, URL}),
       artifacts.requestPageDependencyGraph({trace, devtoolsLog}),
       artifacts.requestLoadSimulator(simulatorOptions),
     ]);
 
-    // get all critical requests 2 + mainResourceIndex levels deep
-    const mainResourceIndex = mainResource.redirects ? mainResource.redirects.length : 0;
-
-    const criticalRequests = UsesRelPreloadAudit._flattenRequests(critChains,
-      3 + mainResourceIndex, 2 + mainResourceIndex);
-
-    /** @type {Set<string>} */
-    const urls = new Set();
-    for (const networkRecord of criticalRequests) {
-      if (UsesRelPreloadAudit.shouldPreload(networkRecord, mainResource)) {
-        urls.add(networkRecord.url);
-      }
-    }
-
+    const urls = UsesRelPreloadAudit.getURLsToPreload(mainResource, graph);
     const {results, wastedMs} = UsesRelPreloadAudit.computeWasteWithGraph(urls, graph, simulator);
     // sort results by wastedTime DESC
     results.sort((a, b) => b.wastedMs - a.wastedMs);


Test patch:
diff --git a/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html b/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
index 649a1bdfe..0627ee7af 100644
--- a/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
+++ b/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
@@ -308,7 +308,7 @@ function deprecationsTest() {
   xhr.open('GET', 'dbw_tester.html', false);
   xhr.send();
 
-  window.webkitStorageInfo.PERSISTENT // FAIL
+  // One more failure in dbw_tester.js
 }
 
 function isOnHttps() {
diff --git a/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.js b/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.js
index 32e5a7773..74986abb7 100644
--- a/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.js
+++ b/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.js
@@ -10,17 +10,8 @@
 
 const params = new URLSearchParams(location.search);
 
-if (location.search === '' || params.has('dateNow')) {
-  // FAIL - Date.now() usage in another file.
-  const d = Date.now();
-}
-
 if (location.search === '' || params.has('deprecations')) {
-  const div = document.createElement('div');
-  div.createShadowRoot();
-  // FAIL(errors-in-console) - multiple shadow v0 roots.
-  // TODO: disabled until m64 is stable (when moved from deprecation warning to error)
-  // div.createShadowRoot();
+  window.webkitStorageInfo.PERSISTENT; // FAIL(deprecations)
 }
 
 })();
diff --git a/lighthouse-core/test/audits/uses-rel-preload-test.js b/lighthouse-core/test/audits/uses-rel-preload-test.js
index ab41e062d..9b0ce4889 100644
--- a/lighthouse-core/test/audits/uses-rel-preload-test.js
+++ b/lighthouse-core/test/audits/uses-rel-preload-test.js
@@ -24,13 +24,10 @@ describe('Performance: uses-rel-preload audit', () => {
   let mockGraph;
   let mockSimulator;
 
-  const mockArtifacts = (networkRecords, mockChain, mainResource = defaultMainResource) => {
+  const mockArtifacts = (networkRecords, mainResource = defaultMainResource) => {
     return {
       traces: {[UsesRelPreload.DEFAULT_PASS]: {traceEvents: []}},
       devtoolsLogs: {[UsesRelPreload.DEFAULT_PASS]: []},
-      requestCriticalRequestChains: () => {
-        return Promise.resolve(mockChain);
-      },
       requestLoadSimulator: () => mockSimulator,
       requestPageDependencyGraph: () => mockGraph,
       requestNetworkRecords: () => networkRecords,
@@ -40,21 +37,69 @@ describe('Performance: uses-rel-preload audit', () => {
     };
   };
 
-  function buildNode(requestId, url) {
-    return new NetworkNode({url, requestId});
-  }
-
   afterEach(() => {
     mockSimulator = undefined;
   });
 
   it('should suggest preload resource', () => {
-    const rootNode = buildNode(1, 'http://example.com');
-    const mainDocumentNode = buildNode(2, 'http://www.example.com:3000');
-    const scriptNode = buildNode(3, 'http://www.example.com/script.js');
-    const scriptAddedNode = buildNode(4, 'http://www.example.com/script-added.js');
-    const scriptSubNode = buildNode(5, 'http://sub.example.com/script-sub.js');
-    const scriptOtherNode = buildNode(6, 'http://otherdomain.com/script-other.js');
+    const mainResource = Object.assign({}, defaultMainResource, {
+      url: 'http://www.example.com:3000',
+      redirects: [''],
+    });
+
+    const networkRecords = [
+      {
+        requestId: '2',
+        resourceType: 'Document',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://example.com:3000',
+        redirects: [''],
+      },
+      {
+        requestId: '2:redirect',
+        resourceType: 'Document',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://www.example.com:3000',
+        redirects: [''],
+      },
+      {
+        requestId: '3',
+        resourceType: 'Script',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://www.example.com/script.js',
+      },
+      {
+        requestId: '4',
+        resourceType: 'Script',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://www.example.com/script-added.js',
+      },
+      {
+        requestId: '5',
+        resourceType: 'Script',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://sub.example.com/script-sub.js',
+      },
+      {
+        requestId: '6',
+        resourceType: 'Script',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://otherdomain.com/script-other.js',
+      },
+    ];
+
+    const rootNode = new NetworkNode(networkRecords[0]);
+    const mainDocumentNode = new NetworkNode(networkRecords[1]);
+    const scriptNode = new NetworkNode(networkRecords[2]);
+    const scriptAddedNode = new NetworkNode(networkRecords[3]);
+    const scriptSubNode = new NetworkNode(networkRecords[4]);
+    const scriptOtherNode = new NetworkNode(networkRecords[5]);
 
     mainDocumentNode.setIsMainDocument(true);
     mainDocumentNode.addDependency(rootNode);
@@ -101,68 +146,7 @@ describe('Performance: uses-rel-preload audit', () => {
       },
     };
 
-    const mainResource = Object.assign({}, defaultMainResource, {
-      url: 'http://www.example.com:3000',
-      redirects: [''],
-    });
-    const networkRecords = [
-      {
-        requestId: '2',
-        isLinkPreload: false,
-        url: 'http://www.example.com:3000',
-      },
-      {
-        requestId: '3',
-        isLinkPreload: false,
-        url: 'http://www.example.com/script.js',
-      },
-      {
-        requestId: '4',
-        isLinkPreload: false,
-        url: 'http://www.example.com/script-added.js',
-      },
-      {
-        requestId: '5',
-        isLinkPreload: false,
-        url: 'http://sub.example.com/script-sub.js',
-      },
-      {
-        requestId: '6',
-        isLinkPreload: false,
-        url: 'http://otherdomain.com/script-other.js',
-      },
-    ];
-
-    const chains = {
-      '1': {
-        children: {
-          '2': {
-            request: networkRecords[0],
-            children: {
-              '3': {
-                request: networkRecords[1],
-                children: {
-                  '4': {
-                    request: networkRecords[2],
-                    children: {},
-                  },
-                  '5': {
-                    request: networkRecords[3],
-                    children: {},
-                  },
-                  '6': {
-                    request: networkRecords[4],
-                    children: {},
-                  },
-                },
-              },
-            },
-          },
-        },
-      },
-    };
-
-    return UsesRelPreload.audit(mockArtifacts(networkRecords, chains, mainResource), {}).then(
+    return UsesRelPreload.audit(mockArtifacts(networkRecords, mainResource), {}).then(
       output => {
         assert.equal(output.rawValue, 1250);
         assert.equal(output.details.items.length, 2);
@@ -181,22 +165,8 @@ describe('Performance: uses-rel-preload audit', () => {
         url: 'http://www.example.com/script.js',
       },
     ];
-    const chains = {
-      '1': {
-        children: {
-          '2': {
-            children: {
-              '3': {
-                request: networkRecords[0],
-                children: {},
-              },
-            },
-          },
-        },
-      },
-    };
 
-    return UsesRelPreload.audit(mockArtifacts(networkRecords, chains), {}).then(output => {
+    return UsesRelPreload.audit(mockArtifacts(networkRecords), {}).then(output => {
       assert.equal(output.rawValue, 0);
       assert.equal(output.details.items.length, 0);
     });
@@ -211,22 +181,7 @@ describe('Performance: uses-rel-preload audit', () => {
       },
     ];
 
-    const chains = {
-      '1': {
-        children: {
-          '2': {
-            children: {
-              '3': {
-                request: networkRecords[0],
-                children: {},
-              },
-            },
-          },
-        },
-      },
-    };
-
-    return UsesRelPreload.audit(mockArtifacts(networkRecords, chains), {}).then(output => {
+    return UsesRelPreload.audit(mockArtifacts(networkRecords), {}).then(output => {
       assert.equal(output.rawValue, 0);
       assert.equal(output.details.items.length, 0);
     });
@@ -241,22 +196,7 @@ describe('Performance: uses-rel-preload audit', () => {
       },
     ];
 
-    const chains = {
-      '1': {
-        children: {
-          '2': {
-            children: {
-              '3': {
-                request: networkRecords[0],
-                children: {},
-              },
-            },
-          },
-        },
-      },
-    };
-
-    return UsesRelPreload.audit(mockArtifacts(networkRecords, chains), {}).then(output => {
+    return UsesRelPreload.audit(mockArtifacts(networkRecords), {}).then(output => {
       assert.equal(output.rawValue, 0);
       assert.equal(output.details.items.length, 0);
     });