Repository: GoogleChrome/lighthouse
Pull request #12419 (2021-04-28 17:41:49)

core(fr): add fetcher to transitional driver
To make the robots.txt gatherer compatible with CSPs we should use the new fetcher. This PR makes fetcher available to FR gatherers.

Related
https://github.com/GoogleChrome/lighthouse/issues/10225


adamraine: I didn't feel very strongly, thought https://github.com/GoogleChrome/lighthouse/pull/12419#discussion_r622408557 was a good idea.

Patch:
diff --git a/lighthouse-core/audits/image-size-responsive.js b/lighthouse-core/audits/image-size-responsive.js
index 69f17aa20..98559e2aa 100644
--- a/lighthouse-core/audits/image-size-responsive.js
+++ b/lighthouse-core/audits/image-size-responsive.js
@@ -60,6 +60,18 @@ function isVisible(imageRect, viewportDimensions) {
   );
 }
 
+/**
+ * @param {{top: number, bottom: number, left: number, right: number}} imageRect
+ * @param {{innerWidth: number, innerHeight: number}} viewportDimensions
+ * @return {boolean}
+ */
+function isSmallerThanViewport(imageRect, viewportDimensions) {
+  return (
+    (imageRect.bottom - imageRect.top) <= viewportDimensions.innerHeight &&
+    (imageRect.right - imageRect.left) <= viewportDimensions.innerWidth
+  );
+}
+
 /**
  * @param {LH.Artifacts.ImageElement} image
  * @return {boolean}
@@ -239,6 +251,7 @@ class ImageSizeResponsive extends Audit {
       .filter(imageHasNaturalDimensions)
       .filter(image => !imageHasRightSize(image, DPR))
       .filter(image => isVisible(image.clientRect, artifacts.ViewportDimensions))
+      .filter(image => isSmallerThanViewport(image.clientRect, artifacts.ViewportDimensions))
       .map(image => getResult(image, DPR));
 
     /** @type {LH.Audit.Details.Table['headings']} */
diff --git a/lighthouse-core/fraggle-rock/gather/driver.js b/lighthouse-core/fraggle-rock/gather/driver.js
index 22d432c39..57259685f 100644
--- a/lighthouse-core/fraggle-rock/gather/driver.js
+++ b/lighthouse-core/fraggle-rock/gather/driver.js
@@ -7,6 +7,7 @@
 
 const ProtocolSession = require('./session.js');
 const ExecutionContext = require('../../gather/driver/execution-context.js');
+const Fetcher = require('../../gather/fetcher.js');
 
 /** @return {*} */
 const throwNotConnectedFn = () => {
@@ -37,6 +38,8 @@ class Driver {
     this._session = undefined;
     /** @type {ExecutionContext|undefined} */
     this._executionContext = undefined;
+    /** @type {Fetcher|undefined} */
+    this._fetcher = undefined;
 
     this.defaultSession = defaultSession;
   }
@@ -47,6 +50,12 @@ class Driver {
     return this._executionContext;
   }
 
+  /** @return {Fetcher} */
+  get fetcher() {
+    if (!this._fetcher) return throwNotConnectedFn();
+    return this._fetcher;
+  }
+
   /** @return {Promise<string>} */
   async url() {
     return this._page.url();
@@ -58,6 +67,7 @@ class Driver {
     const session = await this._page.target().createCDPSession();
     this._session = this.defaultSession = new ProtocolSession(session);
     this._executionContext = new ExecutionContext(this._session);
+    this._fetcher = new Fetcher(this._session, this._executionContext);
   }
 }
 
diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
index facaf43ad..8dfc27efc 100644
--- a/lighthouse-core/gather/driver.js
+++ b/lighthouse-core/gather/driver.js
@@ -71,15 +71,15 @@ class Driver {
 
   online = true;
 
-  // eslint-disable-next-line no-invalid-this
-  fetcher = new Fetcher(this);
-
   // eslint-disable-next-line no-invalid-this
   executionContext = new ExecutionContext(this);
 
   // eslint-disable-next-line no-invalid-this
   defaultSession = this;
 
+  // eslint-disable-next-line no-invalid-this
+  fetcher = new Fetcher(this.defaultSession, this.executionContext);
+
   /**
    * @param {Connection} connection
    */
diff --git a/lighthouse-core/gather/fetcher.js b/lighthouse-core/gather/fetcher.js
index 62755027c..87fb7784b 100644
--- a/lighthouse-core/gather/fetcher.js
+++ b/lighthouse-core/gather/fetcher.js
@@ -14,13 +14,16 @@
 /* global document */
 
 const log = require('lighthouse-logger');
+const {getBrowserVersion} = require('./driver/environment.js');
 
 class Fetcher {
   /**
-   * @param {import('./driver.js')} driver
+   * @param {LH.Gatherer.FRProtocolSession} session
+   * @param {import('./driver/execution-context.js')} executionContext
    */
-  constructor(driver) {
-    this.driver = driver;
+  constructor(session, executionContext) {
+    this.session = session;
+    this.executionContext = executionContext;
     /** @type {Map<string, (event: LH.Crdp.Fetch.RequestPausedEvent) => void>} */
     this._onRequestPausedHandlers = new Map();
     this._onRequestPaused = this._onRequestPaused.bind(this);
@@ -49,18 +52,18 @@ class Fetcher {
     if (this._enabled) return;
 
     this._enabled = true;
-    await this.driver.sendCommand('Fetch.enable', {
+    await this.session.sendCommand('Fetch.enable', {
       patterns: [{requestStage: 'Request'}, {requestStage: 'Response'}],
     });
-    await this.driver.on('Fetch.requestPaused', this._onRequestPaused);
+    await this.session.on('Fetch.requestPaused', this._onRequestPaused);
   }
 
   async disable() {
     if (!this._enabled) return;
 
     this._enabled = false;
-    await this.driver.off('Fetch.requestPaused', this._onRequestPaused);
-    await this.driver.sendCommand('Fetch.disable');
+    await this.session.off('Fetch.requestPaused', this._onRequestPaused);
+    await this.session.sendCommand('Fetch.disable');
     this._onRequestPausedHandlers.clear();
   }
 
@@ -81,7 +84,7 @@ class Fetcher {
       handler(event);
     } else {
       // Nothing cares about this URL, so continue.
-      this.driver.sendCommand('Fetch.continueRequest', {requestId: event.requestId}).catch(err => {
+      this.session.sendCommand('Fetch.continueRequest', {requestId: event.requestId}).catch(err => {
         log.error('Fetcher', `Failed to continueRequest: ${err.message}`);
       });
     }
@@ -104,7 +107,7 @@ class Fetcher {
     // `Network.loadNetworkResource` was introduced in M88.
     // The long timeout bug with `IO.read` was fixed in M92:
     // https://bugs.chromium.org/p/chromium/issues/detail?id=1191757
-    const milestone = await this.driver.getBrowserVersion().then(v => v.milestone);
+    const milestone = await getBrowserVersion(this.session).then(v => v.milestone);
     if (milestone >= 92) {
       return await this._fetchResourceOverProtocol(url, options);
     }
@@ -126,7 +129,7 @@ class Fetcher {
       if (elapsedTime > options.timeout) {
         throw new Error('Waiting for the end of the IO stream exceeded the allotted time.');
       }
-      ioResponse = await this.driver.sendCommand('IO.read', {handle});
+      ioResponse = await this.session.sendCommand('IO.read', {handle});
       const responseData = ioResponse.base64Encoded ?
         Buffer.from(ioResponse.data, 'base64').toString('utf-8') :
         ioResponse.data;
@@ -141,9 +144,9 @@ class Fetcher {
    * @return {Promise<LH.Crdp.IO.StreamHandle>}
    */
   async _loadNetworkResource(url) {
-    await this.driver.sendCommand('Network.enable');
-    const frameTreeResponse = await this.driver.sendCommand('Page.getFrameTree');
-    const networkResponse = await this.driver.sendCommand('Network.loadNetworkResource', {
+    await this.session.sendCommand('Network.enable');
+    const frameTreeResponse = await this.session.sendCommand('Page.getFrameTree');
+    const networkResponse = await this.session.sendCommand('Network.loadNetworkResource', {
       frameId: frameTreeResponse.frameTree.frame.id,
       url,
       options: {
@@ -151,7 +154,7 @@ class Fetcher {
         includeCredentials: true,
       },
     });
-    await this.driver.sendCommand('Network.disable');
+    await this.session.sendCommand('Network.disable');
 
     if (!networkResponse.resource.success || !networkResponse.resource.stream) {
       const statusCode = networkResponse.resource.httpStatusCode || '';
@@ -206,7 +209,7 @@ class Fetcher {
               return {name, value};
             });
 
-          await this.driver.sendCommand('Fetch.continueRequest', {
+          await this.session.sendCommand('Fetch.continueRequest', {
             requestId,
             headers,
           });
@@ -219,7 +222,7 @@ class Fetcher {
           return;
         }
 
-        const responseBody = await this.driver.sendCommand('Fetch.getResponseBody', {requestId});
+        const responseBody = await this.session.sendCommand('Fetch.getResponseBody', {requestId});
         if (responseBody.base64Encoded) {
           resolve(Buffer.from(responseBody.body, 'base64').toString());
         } else {
@@ -227,7 +230,7 @@ class Fetcher {
         }
 
         // Fail the request (from the page's perspective) so that the iframe never loads.
-        await this.driver.sendCommand('Fetch.failRequest', {requestId, errorReason: 'Aborted'});
+        await this.session.sendCommand('Fetch.failRequest', {requestId, errorReason: 'Aborted'});
       };
       this._setOnRequestPausedHandler(url, event => handlerAsync(event).catch(reject));
     });
@@ -270,19 +273,19 @@ class Fetcher {
     ]).finally(() => clearTimeout(timeoutHandle));
 
     // Temporarily disable auto-attaching for this iframe.
-    await this.driver.sendCommand('Target.setAutoAttach', {
+    await this.session.sendCommand('Target.setAutoAttach', {
       autoAttach: false,
       waitForDebuggerOnStart: false,
     });
 
-    const injectionPromise = this.driver.executionContext.evaluate(injectIframe, {
+    const injectionPromise = this.executionContext.evaluate(injectIframe, {
       args: [url],
       useIsolation: true,
     });
 
     const [fetchResult] = await Promise.all([racePromise, injectionPromise]);
 
-    await this.driver.sendCommand('Target.setAutoAttach', {
+    await this.session.sendCommand('Target.setAutoAttach', {
       flatten: true,
       autoAttach: true,
       waitForDebuggerOnStart: true,
diff --git a/types/gatherer.d.ts b/types/gatherer.d.ts
index 79a62196f..7f096c44a 100644
--- a/types/gatherer.d.ts
+++ b/types/gatherer.d.ts
@@ -9,6 +9,7 @@ import _CPUNode = require('../lighthouse-core/lib/dependency-graph/cpu-node');
 import _Simulator = require('../lighthouse-core/lib/dependency-graph/simulator/simulator');
 import Driver = require('../lighthouse-core/gather/driver');
 import ExecutionContext = require('../lighthouse-core/gather/driver/execution-context');
+import Fetcher = require('../lighthouse-core/gather/fetcher');
 
 declare global {
   module LH.Gatherer {
@@ -29,6 +30,7 @@ declare global {
     export interface FRTransitionalDriver {
       defaultSession: FRProtocolSession;
       executionContext: ExecutionContext;
+      fetcher: Fetcher;
     }
 
     /** The limited context interface shared between pre and post Fraggle Rock Lighthouse. */


Test patch:
diff --git a/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html b/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
index 67f2f705e..0f5633fd0 100644
--- a/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
+++ b/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
@@ -204,13 +204,13 @@
 <img src="lighthouse-480x318.jpg?iar2" width="120" height="80">
 
 <!-- FAIL(image-size-responsive): image is naturally 480x318 -->
-<img src="lighthouse-480x318.jpg?isr1" width="4800" height="3180" style="position: absolute;">
+<img src="lighthouse-480x318.jpg?isr1" width="360" height="240" style="position: absolute;">
 <!-- PASS(image-size-responsive) -->
 <img src="lighthouse-480x318.jpg?isr2" width="120" height="80" style="position: absolute;">
 <!-- PASS(image-size-responsive) -->
-<img src="lighthouse-480x318.jpg?isr3" width="960" height="636" style="image-rendering: pixelated; position: absolute;">
+<img src="lighthouse-480x318.jpg?isr3" width="360" height="240" style="image-rendering: pixelated; position: absolute;">
 <!-- PASS(image-size-responsive) -->
-<img src="lighthouse-480x318.jpg?isr4" srcset="lighthouse-480x318.jpg 2x" width="960" height="636" style="position: absolute;">
+<img src="lighthouse-480x318.jpg?isr4" srcset="lighthouse-480x318.jpg 2x" width="360" height="240" style="position: absolute;">
 
 <!-- FAIL(efficient-animated-content): animated gif found -->
 <img src="lighthouse-rotating.gif" width="811" height="462">
diff --git a/lighthouse-cli/test/smokehouse/test-definitions/dobetterweb/dbw-expectations.js b/lighthouse-cli/test/smokehouse/test-definitions/dobetterweb/dbw-expectations.js
index 30bd211c7..cd59dec04 100644
--- a/lighthouse-cli/test/smokehouse/test-definitions/dobetterweb/dbw-expectations.js
+++ b/lighthouse-cli/test/smokehouse/test-definitions/dobetterweb/dbw-expectations.js
@@ -415,7 +415,7 @@ const expectations = [
             screenshot: {
               width: 360,
               // Allow for differences in platforms.
-              height: '3755±5',
+              height: '1350±20',
               data: /^data:image\/jpeg;.{500,}/,
             },
             nodes: {
diff --git a/lighthouse-core/test/audits/image-size-responsive-test.js b/lighthouse-core/test/audits/image-size-responsive-test.js
index 0f1d7cdcc..153821b59 100644
--- a/lighthouse-core/test/audits/image-size-responsive-test.js
+++ b/lighthouse-core/test/audits/image-size-responsive-test.js
@@ -140,6 +140,18 @@ describe('Images: size audit', () => {
     },
   });
 
+  testImage('wider than the viewport', {
+    score: 1,
+    clientSize: [1000, 100],
+    naturalSize: [5, 5],
+  });
+
+  testImage('taller than the viewport', {
+    score: 1,
+    clientSize: [100, 1000],
+    naturalSize: [5, 5],
+  });
+
   describe('visibility', () => {
     testImage('has no client area', {
       score: 1,
diff --git a/lighthouse-core/test/fraggle-rock/gather/driver-test.js b/lighthouse-core/test/fraggle-rock/gather/driver-test.js
index 031d3e3ef..164875a5e 100644
--- a/lighthouse-core/test/fraggle-rock/gather/driver-test.js
+++ b/lighthouse-core/test/fraggle-rock/gather/driver-test.js
@@ -77,3 +77,14 @@ describe('.executionContext', () => {
     expect(driver.executionContext).toBeTruthy();
   });
 });
+
+describe('.fetcher', () => {
+  it('should fail if called before connect', () => {
+    expect(() => driver.fetcher).toThrow();
+  });
+
+  it('should create a fetcher on connect', async () => {
+    await driver.connect();
+    expect(driver.fetcher).toBeTruthy();
+  });
+});
diff --git a/lighthouse-core/test/gather/fetcher-test.js b/lighthouse-core/test/gather/fetcher-test.js
index 5787d124f..17960359d 100644
--- a/lighthouse-core/test/gather/fetcher-test.js
+++ b/lighthouse-core/test/gather/fetcher-test.js
@@ -5,30 +5,34 @@
  */
 'use strict';
 
+/* eslint-env jest */
+
+/** @type {number} */
+let browserMilestone;
+
+jest.mock('../../gather/driver/environment.js', () => ({
+  getBrowserVersion: jest.fn().mockImplementation(() => {
+    return Promise.resolve({milestone: browserMilestone});
+  }),
+}));
+
 const Fetcher = require('../../gather/fetcher.js');
 const Driver = require('../../gather/driver.js');
 const Connection = require('../../gather/connections/connection.js');
 const {createMockSendCommandFn} = require('../test-utils.js');
 
-/* eslint-env jest */
-
 /** @type {Connection} */
 let connectionStub;
 /** @type {Driver} */
 let driver;
 /** @type {Fetcher} */
 let fetcher;
-/** @type {number} */
-let browserMilestone;
 
 beforeEach(() => {
   connectionStub = new Connection();
   driver = new Driver(connectionStub);
-  fetcher = new Fetcher(driver);
+  fetcher = new Fetcher(driver.defaultSession, driver.executionContext);
   browserMilestone = 92;
-  driver.getBrowserVersion = jest.fn().mockImplementation(() => {
-    return Promise.resolve({milestone: browserMilestone});
-  });
 });
 
 describe('.fetchResource', () => {
diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
index 8ffb6f49f..a51d1b3e6 100644
--- a/lighthouse-core/test/results/sample_v2.json
+++ b/lighthouse-core/test/results/sample_v2.json
@@ -703,59 +703,14 @@
     },
     "image-size-responsive": {
       "id": "image-size-responsive",
-      "title": "Serves images with low resolution",
+      "title": "Serves images with appropriate resolution",
       "description": "Image natural dimensions should be proportional to the display size and the pixel ratio to maximize image clarity. [Learn more](https://web.dev/serve-responsive-images/).",
-      "score": 0,
+      "score": 1,
       "scoreDisplayMode": "binary",
       "details": {
         "type": "table",
-        "headings": [
-          {
-            "key": "url",
-            "itemType": "thumbnail",
-            "text": ""
-          },
-          {
-            "key": "elidedUrl",
-            "itemType": "url",
-            "text": "URL"
-          },
-          {
-            "key": "displayedSize",
-            "itemType": "text",
-            "text": "Displayed size"
-          },
-          {
-            "key": "actualSize",
-            "itemType": "text",
-            "text": "Actual size"
-          },
-          {
-            "key": "expectedSize",
-            "itemType": "text",
-            "text": "Expected size"
-          }
-        ],
-        "items": [
-          {
-            "url": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr1",
-            "elidedUrl": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr1",
-            "displayedSize": "4800 x 3180",
-            "actualSize": "480 x 318",
-            "actualPixels": 152640,
-            "expectedSize": "9600 x 6360",
-            "expectedPixels": 61056000
-          },
-          {
-            "url": "http://localhost:10200/dobetterweb/lighthouse-rotating.gif",
-            "elidedUrl": "http://localhost:10200/dobetterweb/lighthouse-rotating.gif",
-            "displayedSize": "811 x 462",
-            "actualSize": "811 x 462",
-            "actualPixels": 374682,
-            "expectedSize": "1622 x 924",
-            "expectedPixels": 1498728
-          }
-        ]
+        "headings": [],
+        "items": []
       }
     },
     "preload-fonts": {
@@ -5559,7 +5514,7 @@
         }
       ],
       "id": "best-practices",
-      "score": 0.19
+      "score": 0.25
     },
     "seo": {
       "title": "SEO",
@@ -7283,7 +7238,6 @@
       "lighthouse-core/lib/i18n/i18n.js | columnURL": [
         "audits[server-response-time].details.headings[0].label",
         "audits[image-aspect-ratio].details.headings[1].text",
-        "audits[image-size-responsive].details.headings[1].text",
         "audits[preload-fonts].details.headings[0].text",
         "audits[bootup-time].details.headings[0].text",
         "audits[network-rtt].details.headings[0].text",
@@ -7403,21 +7357,12 @@
       "lighthouse-core/audits/image-aspect-ratio.js | columnActual": [
         "audits[image-aspect-ratio].details.headings[3].text"
       ],
-      "lighthouse-core/audits/image-size-responsive.js | failureTitle": [
+      "lighthouse-core/audits/image-size-responsive.js | title": [
         "audits[image-size-responsive].title"
       ],
       "lighthouse-core/audits/image-size-responsive.js | description": [
         "audits[image-size-responsive].description"
       ],
-      "lighthouse-core/audits/image-size-responsive.js | columnDisplayed": [
-        "audits[image-size-responsive].details.headings[2].text"
-      ],
-      "lighthouse-core/audits/image-size-responsive.js | columnActual": [
-        "audits[image-size-responsive].details.headings[3].text"
-      ],
-      "lighthouse-core/audits/image-size-responsive.js | columnExpected": [
-        "audits[image-size-responsive].details.headings[4].text"
-      ],
       "lighthouse-core/audits/preload-fonts.js | failureTitle": [
         "audits[preload-fonts].title"
       ],