core(driver): wait for Page.frameNavigated for about:blank #6446
Conversation
There was a problem hiding this comment.
I'll remove references to blankDuration elsewhere
love this, let's do it :)
I guess we'll want to test this over a variety of types of pages (similar to what caught the data URL issue) to make sure we're good here? If only we had a runner in the works for that :)
No idea what problems we could possibly run into, though...maybe things like evaluateScriptOnNewDocument might have difficulty?
| * @return {Promise<void>} | ||
| */ | ||
| static async loadBlank( | ||
| driver, | ||
| url = constants.defaultPassConfig.blankPage, | ||
| duration = constants.defaultPassConfig.blankDuration | ||
| url = constants.defaultPassConfig.blankPage |
There was a problem hiding this comment.
can this go back to being one line now :)
| @@ -843,6 +854,10 @@ class Driver { | |||
| this.setNextProtocolTimeout(30 * 1000); | |||
| this.sendCommand('Page.navigate', {url}); | |||
|
|
|||
| if (waitForNavigated) { | |||
| await this._waitForFrameNavigated(); | |||
There was a problem hiding this comment.
any concerns about racing here? i.e. should we get the promise before Page.navigate and then await it here?
There was a problem hiding this comment.
also, any concerns about delaying _waitForFullyLoaded until after Page.frameNavigated fires?
There was a problem hiding this comment.
dgozman convinced me that so long as the listener is attached synchronously (which it is inside this._waitForFrameNavigated), it is impossible for the event to be received too early.
Longer explanation was that the earliest possible resolution of the promise would be the next microtask and because the listener is installed before that microtask is executed there's no way to miss the event
There was a problem hiding this comment.
also, any concerns about delaying _waitForFullyLoaded until after Page.frameNavigated fires?
definitely, they should not be used together I'll add a throw
There was a problem hiding this comment.
dgozman convinced me that so long as the listener is attached synchronously (which it is inside this._waitForFrameNavigated), it is impossible for the event to be received too early.
yeahhhh, good point. As long as no one sticks an await in front of that sendCommand, I guess?
| * @return {Promise<void>} | ||
| */ | ||
| static async loadBlank( | ||
| driver, | ||
| url = constants.defaultPassConfig.blankPage, | ||
| duration = constants.defaultPassConfig.blankDuration |
There was a problem hiding this comment.
I forget when we've actually used a different blankDuration. Was it just for tests?
There was a problem hiding this comment.
tests and once upon a time WPT used it for video recording, but for past year or so he's just run LH straight outta the box
| @@ -843,6 +843,10 @@ class Driver { | |||
| const passContext = /** @type {Partial<LH.Gatherer.PassContext>} */ (options.passContext || {}); | |||
| const disableJS = passContext.disableJavaScript || false; | |||
|
|
|||
| if (waitForNavigated && waitForLoad) { | |||
| throw new Error('Cannot use both waitForNavigated and waitForLoad, pick just one'); | |||
| * Used for detecting that our about:blank reset has been completed. | ||
| */ | ||
| _waitForFrameNavigated() { | ||
| return new Promise(resolve => { |
There was a problem hiding this comment.
at some point we should add a driver method that wraps once with a promise. Seems like a lot of our uses of it end up like this
Summary
I'll want to put this through the DZL wringer and maybe someone sanity check this with the DevTools folks, but it seems like we should be able to just wait for the frameNavigated event instead of 300ms all the time.
On my machine it drives down about:blank times from...
300ms -> 200ms
300ms -> 16ms
300ms -> 14ms
If this gets cursory 👍 I'll remove references to blankDuration elsewhere, or we can keep it but just update the minimum to be 0
Related Issues/PRs
#6442