Repository: GoogleChrome/lighthouse
Pull request #6117 (2018-09-26 19:39:49)

core(driver): request smaller trace in m71+
Backstory in #3596. 

Results: cnn.com's trace goes from 69M to 47M.


--

* Using `Browser.getVersion` and parsing out the chrome milestone. We can augment this pattern later for `isMobile` and `os` like @patrickhulce suggested.
* I made a lil sendCommand mock response helper in driver-test. There are similar stubs for `on()` and `once()`. 
* we now default to the new `disabled-by-default-lighthouse` category and fallback to 'toplevel', as @brendankenny very wisely suggested
* ideally i'd like to drop the `SCHEDULABLE_TASK_TITLE` possibilities from _FOUR_ down to 2. But it requires updating a lot of fixture data. So we can do that in a followup.

patrickhulce: I'm mildly nervous about missing some random lantern-impacting event, but I think we've both been through it multiple times before.
paulirish: ptal

Patch:
diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
index f96df0b09..f38b3850b 100644
--- a/lighthouse-core/gather/driver.js
+++ b/lighthouse-core/gather/driver.js
@@ -83,7 +83,7 @@ class Driver {
   static get traceCategories() {
     return [
       '-*', // exclude default
-      'toplevel',
+      'disabled-by-default-lighthouse', // used instead of 'toplevel' in Chrome 71+
       'v8.execute',
       'blink.console',
       'blink.user_timing',
@@ -102,11 +102,13 @@ class Driver {
   }
 
   /**
-   * @return {Promise<string>}
+   * @return {Promise<LH.Crdp.Browser.GetVersionResponse & {milestone: number}>}
    */
-  getUserAgent() {
-    // FIXME: use Browser.getVersion instead
-    return this.evaluateAsync('navigator.userAgent');
+  async getBrowserVersion() {
+    const version = await this.sendCommand('Browser.getVersion');
+    const match = version.product.match(/\/(\d+)/); // eg 'Chrome/71.0.3577.0'
+    const milestone = match ? parseInt(match[1]) : 0;
+    return Object.assign(version, {milestone});
   }
 
   /**
@@ -919,10 +921,19 @@ class Driver {
    * @param {{additionalTraceCategories?: string|null}=} settings
    * @return {Promise<void>}
    */
-  beginTrace(settings) {
+  async beginTrace(settings) {
     const additionalCategories = (settings && settings.additionalTraceCategories &&
         settings.additionalTraceCategories.split(',')) || [];
     const traceCategories = this._traceCategories.concat(additionalCategories);
+
+    // In Chrome <71, gotta use the chatty 'toplevel' cat instead of our own.
+    // TODO(COMPAT): Once m71 ships to stable, drop this section
+    const milestone = (await this.getBrowserVersion()).milestone;
+    if (milestone < 71) {
+      const toplevelIndex = traceCategories.indexOf('disabled-by-default-lighthouse');
+      traceCategories.splice(toplevelIndex, 1, 'toplevel');
+    }
+
     const uniqueCategories = Array.from(new Set(traceCategories));
 
     // Check any domains that could interfere with or add overhead to the trace.
diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
index 0c4e9f9ae..2d045fcf9 100644
--- a/lighthouse-core/gather/gather-runner.js
+++ b/lighthouse-core/gather/gather-runner.js
@@ -366,7 +366,7 @@ class GatherRunner {
     return {
       fetchTime: (new Date()).toJSON(),
       LighthouseRunWarnings: [],
-      HostUserAgent: await options.driver.getUserAgent(),
+      HostUserAgent: (await options.driver.getBrowserVersion()).userAgent,
       NetworkUserAgent: '', // updated later
       BenchmarkIndex: 0, // updated later
       traces: {},
diff --git a/lighthouse-core/lib/traces/tracing-processor.js b/lighthouse-core/lib/traces/tracing-processor.js
index 399d7b127..21f9ac337 100644
--- a/lighthouse-core/lib/traces/tracing-processor.js
+++ b/lighthouse-core/lib/traces/tracing-processor.js
@@ -8,12 +8,16 @@
 // The ideal input response latency, the time between the input task and the
 // first frame of the response.
 const BASE_RESPONSE_LATENCY = 16;
-// m65 and earlier
-const SCHEDULABLE_TASK_TITLE = 'TaskQueueManager::ProcessTaskFromWorkQueue';
+// m71+ We added RunTask to `disabled-by-default-lighthouse`
+const SCHEDULABLE_TASK_TITLE_LH = 'RunTask';
+// m69-70 DoWork is different and we now need RunTask, see https://bugs.chromium.org/p/chromium/issues/detail?id=871204#c11
+const SCHEDULABLE_TASK_TITLE_ALT1 = 'ThreadControllerImpl::RunTask';
 // In m66-68 refactored to this task title, https://crrev.com/c/883346
-const SCHEDULABLE_TASK_TITLE_ALT1 = 'ThreadControllerImpl::DoWork';
-// m69+ DoWork is different and we now need RunTask, see https://bugs.chromium.org/p/chromium/issues/detail?id=871204#c11
-const SCHEDULABLE_TASK_TITLE_ALT2 = 'ThreadControllerImpl::RunTask';
+const SCHEDULABLE_TASK_TITLE_ALT2 = 'ThreadControllerImpl::DoWork';
+// m65 and earlier
+const SCHEDULABLE_TASK_TITLE_ALT3 = 'TaskQueueManager::ProcessTaskFromWorkQueue';
+
+
 const LHError = require('../lh-error');
 
 class TraceProcessor {
@@ -234,9 +238,10 @@ class TraceProcessor {
    * @return {boolean}
    */
   static isScheduleableTask(evt) {
-    return evt.name === SCHEDULABLE_TASK_TITLE ||
-      evt.name === SCHEDULABLE_TASK_TITLE_ALT1 ||
-      evt.name === SCHEDULABLE_TASK_TITLE_ALT2;
+    return evt.name === SCHEDULABLE_TASK_TITLE_LH ||
+    evt.name === SCHEDULABLE_TASK_TITLE_ALT1 ||
+    evt.name === SCHEDULABLE_TASK_TITLE_ALT2 ||
+    evt.name === SCHEDULABLE_TASK_TITLE_ALT3;
   }
 }
 


Test patch:
diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js
index 0f04bdc23..a8af8272f 100644
--- a/lighthouse-core/test/gather/driver-test.js
+++ b/lighthouse-core/test/gather/driver-test.js
@@ -6,12 +6,14 @@
 'use strict';
 
 let sendCommandParams = [];
+let sendCommandMockResponses = {};
 
 const Driver = require('../../gather/driver.js');
 const Connection = require('../../gather/connections/connection.js');
 const Element = require('../../lib/element.js');
 const assert = require('assert');
 const EventEmitter = require('events').EventEmitter;
+const {protocolGetVersionResponse} = require('./fake-driver');
 
 const connection = new Connection();
 const driverStub = new Driver(connection);
@@ -46,9 +48,23 @@ function createActiveWorker(id, url, controlledClients, status = 'activated') {
   };
 }
 
+function createOnceMethodResponse(method, response) {
+  assert.deepEqual(!!sendCommandMockResponses[method], false, 'stub response already defined');
+  sendCommandMockResponses[method] = response;
+}
+
 connection.sendCommand = function(command, params) {
   sendCommandParams.push({command, params});
+
+  const mockResponse = sendCommandMockResponses[command];
+  if (mockResponse) {
+    delete sendCommandMockResponses[command];
+    return Promise.resolve(mockResponse);
+  }
+
   switch (command) {
+    case 'Browser.getVersion':
+      return Promise.resolve(protocolGetVersionResponse);
     case 'DOM.getDocument':
       return Promise.resolve({root: {nodeId: 249}});
     case 'DOM.querySelector':
@@ -99,6 +115,7 @@ connection.sendCommand = function(command, params) {
 describe('Browser Driver', () => {
   beforeEach(() => {
     sendCommandParams = [];
+    sendCommandMockResponses = {};
   });
 
   it('returns null when DOM.querySelector finds no node', () => {
@@ -232,16 +249,33 @@ describe('Browser Driver', () => {
   });
 
   it('will use requested additionalTraceCategories', () => {
-    return driverStub.beginTrace({additionalTraceCategories: 'v8,v8.execute,toplevel'}).then(() => {
+    return driverStub.beginTrace({additionalTraceCategories: 'loading,xtra_cat'}).then(() => {
       const traceCmd = sendCommandParams.find(obj => obj.command === 'Tracing.start');
       const categories = traceCmd.params.categories;
-      assert.ok(categories.includes('blink'), 'contains default categories');
-      assert.ok(categories.includes('v8.execute'), 'contains added categories');
-      assert.ok(categories.indexOf('toplevel') === categories.lastIndexOf('toplevel'),
+      assert.ok(categories.includes('blink.user_timing'), 'contains default categories');
+      assert.ok(categories.includes('xtra_cat'), 'contains added categories');
+      assert.ok(categories.indexOf('loading') === categories.lastIndexOf('loading'),
           'de-dupes categories');
     });
   });
 
+  it('will adjust traceCategories based on chrome version', async () => {
+    // m70 doesn't have disabled-by-default-lighthouse, so 'toplevel' is used instead.
+    const m70ProtocolGetVersionResponse = Object.assign({}, protocolGetVersionResponse, {
+      product: 'Chrome/70.0.3577.0',
+      // eslint-disable-next-line max-len
+      userAgent: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3577.0 Safari/537.36',
+    });
+    createOnceMethodResponse('Browser.getVersion', m70ProtocolGetVersionResponse);
+
+    // eslint-disable-next-line max-len
+    await driverStub.beginTrace();
+    const traceCmd = sendCommandParams.find(obj => obj.command === 'Tracing.start');
+    const categories = traceCmd.params.categories;
+    assert.ok(categories.includes('toplevel'), 'contains old toplevel category');
+    assert.equal(categories.indexOf('disabled-by-default-lighthouse'), -1, 'excludes new cat');
+  });
+
   it('should send the Network.setExtraHTTPHeaders command when there are extra-headers', () => {
     return driverStub.setExtraHTTPHeaders({
       'Cookie': 'monster',
diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js
index 0055bbe6c..f90ed3b0f 100644
--- a/lighthouse-core/test/gather/fake-driver.js
+++ b/lighthouse-core/test/gather/fake-driver.js
@@ -5,9 +5,19 @@
  */
 'use strict';
 
-module.exports = {
-  getUserAgent() {
-    return Promise.resolve('Fake user agent');
+// https://chromedevtools.github.io/devtools-protocol/tot/Browser#method-getVersion
+const protocolGetVersionResponse = {
+  protocolVersion: '1.3',
+  product: 'Chrome/71.0.3577.0',
+  revision: '@fc334a55a70eec12fc77853c53979f81e8496c21',
+  // eslint-disable-next-line max-len
+  userAgent: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3577.0 Safari/537.36',
+  jsVersion: '7.1.314',
+};
+
+const fakeDriver = {
+  getBrowserVersion() {
+    return Promise.resolve(Object.assign({}, protocolGetVersionResponse, {milestone: 71}));
   },
   getBenchmarkIndex() {
     return Promise.resolve(125.2);
@@ -72,3 +82,6 @@ module.exports = {
     return Promise.resolve();
   },
 };
+
+module.exports = fakeDriver;
+module.exports.protocolGetVersionResponse = protocolGetVersionResponse;
diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js
index 76084d924..8ab6da53e 100644
--- a/lighthouse-core/test/gather/gather-runner-test.js
+++ b/lighthouse-core/test/gather/gather-runner-test.js
@@ -53,9 +53,6 @@ function getMockedEmulationDriver(emulationFn, netThrottleFn, cpuThrottleFn,
     }
     cleanBrowserCaches() {}
     clearDataForOrigin() {}
-    getUserAgent() {
-      return Promise.resolve('Fake user agent');
-    }
   };
   const EmulationMock = class extends Connection {
     sendCommand(command, params) {
@@ -126,7 +123,8 @@ describe('GatherRunner', function() {
     const options = {url, driver, config, settings};
 
     const results = await GatherRunner.run([], options);
-    expect(results.HostUserAgent).toEqual('Fake user agent');
+    expect(results.HostUserAgent).toEqual(fakeDriver.protocolGetVersionResponse.userAgent);
+    expect(results.HostUserAgent).toMatch(/Chrome\/\d+/);
   });
 
   it('collects network user agent as an artifact', async () => {
@@ -272,6 +270,7 @@ describe('GatherRunner', function() {
     };
     const createEmulationCheck = variable => (...args) => {
       tests[variable] = args;
+
       return true;
     };
     const driver = getMockedEmulationDriver(