Skip to content

Commit

Permalink
always construct networkRecords from devtoolsLog (#2133)
Browse files Browse the repository at this point in the history 
simplify devtoolsLog recording, url redirect tracking, and eliminate explicit network recording
  • Loading branch information
@brendankenny @paulirish
brendankenny authored and paulirish committed May 5, 2017
1 parent 75d8f4d commit d7e4d1b
Show file tree
Hide file tree
Showing 10 changed files with 400 additions and 208 deletions.
165 changes: 96 additions & 69 deletions lighthouse-core/gather/driver.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,28 @@ class Driver {
this._connection = connection;
// currently only used by WPT where just Page and Network are needed
this._devtoolsLog = new DevtoolsLog(/^(Page|Network)\./);
this.online = true;
this._domainEnabledCounts = new Map();

/**
* Used for monitoring network status events during gotoURL.
* @private {?NetworkRecorder}
*/
this._networkStatusMonitor = null;

/**
* Used for monitoring url redirects during gotoURL.
* @private {?string}
*/
this._monitoredUrl = null;

connection.on('notification', event => {
this._devtoolsLog.record(event);
this.recordNetworkEvent(event.method, event.params);
if (this._networkStatusMonitor) {
this._networkStatusMonitor.dispatch(event.method, event.params);
}
this._eventEmitter.emit(event.method, event.params);
});
this.online = true;
this._domainEnabledCounts = new Map();
}

static get traceCategories() {
Expand All @@ -73,13 +88,6 @@ class Driver {
];
}

/**
* @return {!Array<{method: string, params: !Object}>}
*/
get devtoolsLog() {
return this._devtoolsLog.messages;
}

/**
* @return {!Promise<string>}
*/
Expand Down Expand Up @@ -346,25 +354,6 @@ class Driver {
});
}

/**
* If our main document URL redirects, we will update options.url accordingly
* As such, options.url will always represent the post-redirected URL.
* options.initialUrl is the pre-redirect URL that things started with
* @param {!Object} opts
*/
enableUrlUpdateIfRedirected(opts) {
this._networkRecorder.on('requestloaded', redirectRequest => {
// Quit if this is not a redirected request
if (!redirectRequest.redirectSource) {
return;
}
const earlierRequest = redirectRequest.redirectSource;
if (earlierRequest.url === opts.url) {
opts.url = redirectRequest.url;
}
});
}

/**
* Returns a promise that resolves when the network has been idle for
* `pauseAfterLoadMs` ms and a method to cancel internal network listeners and
Expand All @@ -380,25 +369,25 @@ class Driver {
const promise = new Promise((resolve, reject) => {
const onIdle = () => {
// eslint-disable-next-line no-use-before-define
this._networkRecorder.once('networkbusy', onBusy);
this._networkStatusMonitor.once('networkbusy', onBusy);
idleTimeout = setTimeout(_ => {
cancel();
resolve();
}, pauseAfterLoadMs);
};

const onBusy = () => {
this._networkRecorder.once('networkidle', onIdle);
this._networkStatusMonitor.once('networkidle', onIdle);
clearTimeout(idleTimeout);
};

cancel = () => {
clearTimeout(idleTimeout);
this._networkRecorder.removeListener('networkbusy', onBusy);
this._networkRecorder.removeListener('networkidle', onIdle);
this._networkStatusMonitor.removeListener('networkbusy', onBusy);
this._networkStatusMonitor.removeListener('networkidle', onIdle);
};

if (this._networkRecorder.isIdle()) {
if (this._networkStatusMonitor.isIdle()) {
onIdle();
} else {
onBusy();
Expand Down Expand Up @@ -490,14 +479,55 @@ class Driver {
}

/**
* Navigate to the given URL. Use of this method directly isn't advised: if
* Set up listener for network quiet events and URL redirects. Passed in URL
* will be monitored for redirects, with the final loaded URL passed back in
* _endNetworkStatusMonitoring.
* @param {string} startingUrl
* @return {!Promise}
* @private
*/
_beginNetworkStatusMonitoring(startingUrl) {
this._networkStatusMonitor = new NetworkRecorder([]);

// Update startingUrl if it's ever redirected.
this._monitoredUrl = startingUrl;
this._networkStatusMonitor.on('requestloaded', redirectRequest => {
// Ignore if this is not a redirected request.
if (!redirectRequest.redirectSource) {
return;
}
const earlierRequest = redirectRequest.redirectSource;
if (earlierRequest.url === this._monitoredUrl) {
this._monitoredUrl = redirectRequest.url;
}
});

return this.sendCommand('Network.enable');
}

/**
* End network status listening. Returns the final, possibly redirected,
* loaded URL starting with the one passed into _endNetworkStatusMonitoring.
* @return {string}
* @private
*/
_endNetworkStatusMonitoring() {
this._networkStatusMonitor = null;
const finalUrl = this._monitoredUrl;
this._monitoredUrl = null;
return finalUrl;
}

/**
* Navigate to the given URL. Direct use of this method isn't advised: if
* the current page is already at the given URL, navigation will not occur and
* so the returned promise will only resolve after the MAX_WAIT_FOR_FULLY_LOADED
* timeout. See https://github.com/GoogleChrome/lighthouse/pull/185 for one
* possible workaround.
* Resolves on the url of the loaded page, taking into account any redirects.
* @param {string} url
* @param {!Object} options
* @return {!Promise}
* @return {!Promise<string>}
*/
gotoURL(url, options = {}) {
const waitForLoad = options.waitForLoad || false;
Expand All @@ -506,16 +536,18 @@ class Driver {
const maxWaitMs = (options.flags && options.flags.maxWaitForLoad) ||
Driver.MAX_WAIT_FOR_FULLY_LOADED;

return this.sendCommand('Page.enable')
return this._beginNetworkStatusMonitoring(url)
.then(_ => this.sendCommand('Page.enable'))
.then(_ => this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}))
.then(_ => this.sendCommand('Page.navigate', {url}))
.then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs, maxWaitMs));
.then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs, maxWaitMs))
.then(_ => this._endNetworkStatusMonitoring());
}

/**
* @param {string} objectId Object ID for the resolved DOM node
* @param {string} propName Name of the property
* @return {!Promise<string>} The property value, or null, if property not found
* @param {string} objectId Object ID for the resolved DOM node
* @param {string} propName Name of the property
* @return {!Promise<string>} The property value, or null, if property not found
*/
getObjectProperty(objectId, propName) {
return new Promise((resolve, reject) => {
Expand All @@ -538,6 +570,18 @@ class Driver {
});
}

/**
* Return the body of the response with the given ID.
* @param {string} requestId
* @return {string}
*/
getRequestContent(requestId) {
return this.sendCommand('Network.getResponseBody', {
requestId,
// Ignoring result.base64Encoded, which indicates if body is already encoded
}).then(result => result.body);
}

/**
* @param {string} name The name of API whose permission you wish to query
* @return {!Promise<string>} The state of permissions, resolved in a promise.
Expand Down Expand Up @@ -618,9 +662,6 @@ class Driver {
throw new Error('DOM domain enabled when starting trace');
}

this._devtoolsLog.reset();
this._devtoolsLog.beginRecording();

// Enable Page domain to wait for Page.loadEventFired
return this.sendCommand('Page.enable')
.then(_ => this.sendCommand('Tracing.start', tracingOpts));
Expand All @@ -633,7 +674,6 @@ class Driver {
return new Promise((resolve, reject) => {
// When the tracing has ended this will fire with a stream handle.
this.once('Tracing.tracingComplete', streamHandle => {
this._devtoolsLog.endRecording();
this._readTraceFromStream(streamHandle)
.then(traceContents => resolve(traceContents), reject);
});
Expand Down Expand Up @@ -672,34 +712,21 @@ class Driver {
});
}

beginNetworkCollect(opts) {
return new Promise((resolve, reject) => {
this._networkRecords = [];
this._networkRecorder = new NetworkRecorder(this._networkRecords, this);
this.enableUrlUpdateIfRedirected(opts);

this.sendCommand('Network.enable').then(resolve, reject);
});
}

/**
* @param {!string} method
* @param {!Object<string, *>=} params
* Begin recording devtools protocol messages.
*/
recordNetworkEvent(method, params) {
if (!this._networkRecorder) return;

const regexFilter = /^Network\./;
if (!regexFilter.test(method)) return;
this._networkRecorder.dispatch(method, params);
beginDevtoolsLog() {
this._devtoolsLog.reset();
this._devtoolsLog.beginRecording();
}

endNetworkCollect() {
return new Promise((resolve, reject) => {
resolve(this._networkRecords);
this._networkRecorder = null;
this._networkRecords = [];
});
/**
* Stop recording to devtoolsLog and return log contents.
* @return {!Array<{method: string, params: (!Object<string, *>|undefined)}>}
*/
endDevtoolsLog() {
this._devtoolsLog.endRecording();
return this._devtoolsLog.messages;
}

enableRuntimeEvents() {
Expand Down
71 changes: 40 additions & 31 deletions lighthouse-core/gather/gather-runner.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
const log = require('../lib/log.js');
const Audit = require('../audits/audit');
const URL = require('../lib/url-shim');
const NetworkRecorder = require('../lib/network-recorder.js');

/**
* @typedef {!Object<string, !Array<!Promise<*>>>}
Expand All @@ -44,15 +45,15 @@ let GathererResults; // eslint-disable-line no-unused-vars
* 2. For each pass in the config:
* A. GatherRunner.beforePass()
* i. navigate to about:blank
* ii. all gatherer's beforePass()
* ii. all gatherers' beforePass()
* B. GatherRunner.pass()
* i. GatherRunner.loadPage()
* b. beginTrace (if requested) & beginNetworkCollect
* c. navigate to options.url (and wait for onload)
* ii. all gatherer's pass()
* i. beginTrace (if requested) & beginDevtoolsLog
* ii. GatherRunner.loadPage()
* a. navigate to options.url (and wait for onload)
* iii. all gatherers' pass()
* C. GatherRunner.afterPass()
* i. endTrace (if requested) & endNetworkCollect & endThrottling
* ii. all gatherer's afterPass()
* i. endTrace (if requested) & endDevtoolsLog & endThrottling
* ii. all gatherers' afterPass()
*
* 3. Teardown
* A. GatherRunner.disposeDriver()
Expand All @@ -76,23 +77,22 @@ class GatherRunner {
}

/**
* Loads options.url with specified options.
* Loads options.url with specified options. If the main document URL
* redirects, options.url will be updated accordingly. As such, options.url
* will always represent the post-redirected URL. options.initialUrl is the
* pre-redirect starting URL.
* @param {!Driver} driver
* @param {!Object} options
* @return {!Promise}
*/
static loadPage(driver, options) {
return Promise.resolve()
// Begin tracing only if requested by config.
.then(_ => options.config.recordTrace && driver.beginTrace(options.flags))
// Network is always recorded for internal use, even if not saved as artifact.
.then(_ => driver.beginNetworkCollect(options))
// Navigate.
.then(_ => driver.gotoURL(options.url, {
waitForLoad: true,
disableJavaScript: !!options.disableJavaScript,
flags: options.flags,
}));
return driver.gotoURL(options.url, {
waitForLoad: true,
disableJavaScript: !!options.disableJavaScript,
flags: options.flags,
}).then(finalUrl => {
options.url = finalUrl;
});
}

/**
Expand Down Expand Up @@ -199,9 +199,15 @@ class GatherRunner {
const status = 'Loading page & waiting for onload';
log.log('status', status, gatherernames);

const pass = GatherRunner.loadPage(driver, options).then(_ => {
log.log('statusEnd', status);
});
// Always record devtoolsLog.
driver.beginDevtoolsLog();

const pass = Promise.resolve()
// Begin tracing only if requested by config.
.then(_ => config.recordTrace && driver.beginTrace(options.flags))
// Navigate.
.then(_ => GatherRunner.loadPage(driver, options))
.then(_ => log.log('statusEnd', status));

return gatherers.reduce((chain, gatherer) => {
return chain.then(_ => {
Expand Down Expand Up @@ -242,17 +248,17 @@ class GatherRunner {
});
}

const status = 'Retrieving network records';
pass = pass.then(_ => {
passData.devtoolsLog = driver.devtoolsLog;
const status = 'Retrieving devtoolsLog and network records';
log.log('status', status);
return driver.endNetworkCollect();
}).then(networkRecords => {
const devtoolsLog = driver.endDevtoolsLog();
const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);
GatherRunner.assertPageLoaded(options.url, driver, networkRecords);
// expose devtoolsLog & networkRecords to gatherers
passData.devtoolsLog = driver.devtoolsLog;
passData.networkRecords = networkRecords;
log.verbose('statusEnd', status);

// Expose devtoolsLog and networkRecords to gatherers
passData.devtoolsLog = devtoolsLog;
passData.networkRecords = networkRecords;
});

// Disable throttling so the afterPass analysis isn't throttled
Expand Down Expand Up @@ -351,9 +357,12 @@ class GatherRunner {
.then(_ => GatherRunner.pass(runOptions, gathererResults))
.then(_ => GatherRunner.afterPass(runOptions, gathererResults))
.then(passData => {
// If requested by config, merge trace and network data for this
// pass into tracingData.
const passName = config.passName || Audit.DEFAULT_PASS;

// networkRecords are discarded and not added onto artifacts.
tracingData.devtoolsLogs[passName] = passData.devtoolsLog;

// If requested by config, add trace to pass's tracingData
if (config.recordTrace) {
tracingData.traces[passName] = passData.trace;
}
Expand Down
Loading

0 comments on commit d7e4d1b

Please sign in to comment.