Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(oopif): autoattach to all descendants #7517

Closed
wants to merge 7 commits into from
Closed

core(oopif): autoattach to all descendants #7517

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7f2f90c
core(oopif): prefer autodiscover over autoattach
patrickhulce Mar 14, 2019
ddd8ee5
revert old attempt
patrickhulce Mar 15, 2019
552f042
thread session ID all the way down
patrickhulce Mar 15, 2019
b33d132
update smokehouse tests
patrickhulce Mar 15, 2019
e6f2d43
feedback
patrickhulce Mar 15, 2019
5545c08
fix driver tests
patrickhulce Mar 15, 2019
79932bd
fix gather runner test
patrickhulce Mar 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lighthouse-cli/test/fixtures/oopif.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
</head>
<body>
<h1>Hello frames</h1>
<iframe name="oopif" src="https://airhorner.com"></iframe>
<iframe name="oopif" src="https://paulirish.com/page/2"></iframe>
</body>
</html>
9 changes: 5 additions & 4 deletions lighthouse-cli/test/smokehouse/oopif-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ module.exports = [
'network-requests': {
details: {
items: {
// The page itself only makes a few requests.
// We want to make sure we are finding the iframe's requests (airhorner) while being flexible enough
// to allow changes to the live site.
length: '>10',
// We want to make sure we are finding the iframe's requests (paulirish.com) *AND*
// the iframe's iframe's iframe's requests (youtube.com/doubleclick/etc).
// - paulirish.com ~50-60 requests
// - paulirish.com + all descendant iframes ~80-90 requests
length: '>70',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling this is going to have a big effect on TTI as network idle is likely going to be pushed out in a bunch of cases.

},
},
},
Expand Down
8 changes: 4 additions & 4 deletions lighthouse-core/gather/connections/connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,18 +52,18 @@ class Connection {
* Call protocol methods
* @template {keyof LH.CrdpCommands} C
* @param {C} method
* @param {string|undefined} sessionId
* @param {LH.CrdpCommands[C]['paramsType']} paramArgs,
* @return {Promise<LH.CrdpCommands[C]['returnType']>}
*/
sendCommand(method, ...paramArgs) {
sendCommand(method, sessionId, ...paramArgs) {
// Reify params since we need it as a property so can't just spread again.
const params = paramArgs.length ? paramArgs[0] : undefined;

log.formatProtocol('method => browser', {method, params}, 'verbose');
log.formatProtocol('method => browser', {method, params, sessionId}, 'verbose');
const id = ++this._lastCommandId;
const message = JSON.stringify({id, method, params});
const message = JSON.stringify({id, method, params, sessionId});
this.sendRawMessage(message);

return new Promise(resolve => {
this._callbacks.set(id, {method, resolve});
});
Expand Down
30 changes: 18 additions & 12 deletions lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,17 @@ class Driver {
*/
this._monitoredUrl = null;

let targetProxyMessageId = 0;
this.on('Target.attachedToTarget', event => {
targetProxyMessageId++;
// We're only interested in network requests from iframes for now as those are "part of the page".
if (event.targetInfo.type !== 'iframe') return;

// We want to receive information about network requests from iframes, so enable the Network domain.
// Network events from subtargets will be stringified and sent back on `Target.receivedMessageFromTarget`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still get back these messages over the target domain?

In https://github.com/GoogleChrome/puppeteer/pull/3827/files#diff-0c1fe9dd78cc7f29823c09b251c8aae0R109 I saw the sessionId being received and handled in the handleRawMessage method (basically).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops no, I need to update the comment 馃憤

this.sendCommand('Target.sendMessageToTarget', {
message: JSON.stringify({id: targetProxyMessageId, method: 'Network.enable'}),
sessionId: event.sessionId,
this.sendCommand({method: 'Network.enable', sessionId: event.sessionId});
this.sendCommand({method: 'Target.setAutoAttach', sessionId: event.sessionId}, {
autoAttach: true,
waitForDebuggerOnStart: false,
flatten: true,
});
});

Expand Down Expand Up @@ -279,11 +279,15 @@ class Driver {
* Call protocol methods, with a timeout.
* To configure the timeout for the next call, use 'setNextProtocolTimeout'.
* @template {keyof LH.CrdpCommands} C
* @param {C} method
* @param {C|{method: C, sessionId: string}} methodObj
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah I dig this solution.

A followup PR can move this._nextProtocolTimeout into this methodObj. cc @hoten

Copy link
Member

@brendankenny brendankenny Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the same way about this as I did for #6347 :) We're trying to generalize a simple thing by making it more complicated, just for the two calls that need it.

Since they're both inside driver, would it be possible for now to make those two calls be calls of _innerSendCommand instead, leave sendCommand the same, and we set up a time to figure out what driver should look like in a multi target world (which is also going to help us do better SW work)? I think long term that can also help with complexity when we start doing more opportunities with iframe resources, as ideally we can minimize the manual sessionId management that has to be done in gatherers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they're both inside driver, would it be possible for now to make those two calls be calls of _innerSendCommand instead, leave sendCommand the same, and we set up a time to figure out what driver should look like in a multi target world (which is also going to help us do better SW work)?

sure

* @param {LH.CrdpCommands[C]['paramsType']} params
* @return {Promise<LH.CrdpCommands[C]['returnType']>}
*/
sendCommand(method, ...params) {
sendCommand(methodObj, ...params) {
/** @type {C} */
const method = typeof methodObj === 'string' ? methodObj : methodObj.method;
const sessionId = typeof methodObj === 'string' ? undefined : methodObj.sessionId;

const timeout = this._nextProtocolTimeout;
this._nextProtocolTimeout = DEFAULT_PROTOCOL_TIMEOUT;
return new Promise(async (resolve, reject) => {
Expand All @@ -295,7 +299,7 @@ class Driver {
reject(err);
}), timeout);
try {
const result = await this._innerSendCommand(method, ...params);
const result = await this._innerSendCommand(method, sessionId, ...params);
resolve(result);
} catch (err) {
reject(err);
Expand All @@ -310,18 +314,19 @@ class Driver {
* @private
* @template {keyof LH.CrdpCommands} C
* @param {C} method
* @param {string|undefined} sessionId
* @param {LH.CrdpCommands[C]['paramsType']} params
* @return {Promise<LH.CrdpCommands[C]['returnType']>}
*/
_innerSendCommand(method, ...params) {
_innerSendCommand(method, sessionId, ...params) {
const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method);
if (domainCommand) {
if (domainCommand && !sessionId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're going to expand to other targets, we should probably track enabled domains there as well...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll use your point back atcha for now ;)

It's only the two messages that are never disabled, so we can cross that bridge when we get to it IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only the two messages that are never disabled, so we can cross that bridge when we get to it IMO

ha, I am 馃憣馃憣 with crossing that bridge later, but maybe leave a comment about not bothering to check other targets?

const enable = domainCommand[2] === 'enable';
if (!this._shouldToggleDomain(domainCommand[1], enable)) {
return Promise.resolve();
}
}
return this._connection.sendCommand(method, ...params);
return this._connection.sendCommand(method, sessionId, ...params);
}

/**
Expand Down Expand Up @@ -1045,13 +1050,14 @@ class Driver {
await this.sendCommand('Target.setAutoAttach', {
autoAttach: true,
waitForDebuggerOnStart: false,
flatten: true,
});

await this.sendCommand('Page.enable');
await this.sendCommand('Page.setLifecycleEventsEnabled', {enabled: true});
await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
// No timeout needed for Page.navigate. See https://github.com/GoogleChrome/lighthouse/pull/6413.
const waitforPageNavigateCmd = this._innerSendCommand('Page.navigate', {url});
const waitforPageNavigateCmd = this._innerSendCommand('Page.navigate', undefined, {url});

if (waitForNavigated) {
await this._waitForFrameNavigated();
Expand Down
16 changes: 1 addition & 15 deletions lighthouse-core/lib/network-recorder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,26 +101,12 @@ class NetworkRecorder extends EventEmitter {
return !!(isQUIC && receivedHeaders && record.endTime);
}

/**
* frame root network requests don't always "finish" even when they're done loading data, use responseReceived instead
* @see https://github.com/GoogleChrome/lighthouse/issues/6067#issuecomment-423211201
* @param {LH.Artifacts.NetworkRequest} record
* @return {boolean}
*/
static _isFrameRootRequestAndFinished(record) {
const isFrameRootRequest = record.url === record.documentURL;
const responseReceived = record.responseReceivedTime > 0;
return !!(isFrameRootRequest && responseReceived && record.endTime);
}

/**
* @param {LH.Artifacts.NetworkRequest} record
* @return {boolean}
*/
static isNetworkRecordFinished(record) {
return record.finished ||
NetworkRecorder._isQUICAndFinished(record) ||
NetworkRecorder._isFrameRootRequestAndFinished(record);
return record.finished || NetworkRecorder._isQUICAndFinished(record);
}

/**
Expand Down
4 changes: 1 addition & 3 deletions lighthouse-core/test/lib/network-recorder-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,7 @@ describe('network recorder', function() {
];

const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0);
assert.deepStrictEqual(periods, [
{start: 1200, end: Infinity},
]);
assert.deepStrictEqual(periods, []);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what changed with the quiet period?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was the intentional removal of our hack for frame roots, now if an iframe never fires loadingFinished it shouldn't be seen as finished

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, got it, I wasn't thinking about the network-recorder.js changes

});

it('should handle QUIC requests', () => {
Expand Down