core(fr): extract driver preparation methods #12445
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
patrickhulce
commented
May 5, 2021
|
|
||
| // `Debugger.setSkipAllPauses` is reset after every navigation, so retrigger it on main frame navigations. | ||
| // See https://bugs.chromium.org/p/chromium/issues/detail?id=990945&q=setSkipAllPauses&can=2 | ||
| session.on('Page.frameNavigated', event => { |
There was a problem hiding this comment.
| * @return {Promise<void>} | ||
| */ | ||
| async function shimRequestIdleCallbackOnNewDocument(driver, settings) { | ||
| await driver.executionContext.evaluateOnNewDocument(pageFunctions.wrapRequestIdleCallback, { |
| * @return {Promise<void>} | ||
| */ | ||
| async function dismissJavaScriptDialogs(session) { | ||
| session.on('Page.javascriptDialogOpening', data => { |
There was a problem hiding this comment.
same exact impl
| const warning = await storage.getImportantStorageWarning(session, navigation.url); | ||
| if (warning) warnings.push(warning); | ||
| await storage.clearDataForOrigin(session, navigation.url); | ||
| await storage.clearBrowserCaches(session); |
There was a problem hiding this comment.
it's new that these two clear are colocated. I couldn't think of a compelling reason why you'd only want to clear the data but not the caches, and we previously only cleared the caches on the same passes as the ones we cleared data so from protocol message log perspective it should behave the same as before.
| * @param {LH.Config.Settings} settings | ||
| * @param {Pick<LH.Config.NavigationDefn, 'disableThrottling'|'disableStorageReset'|'blockedUrlPatterns'> & {url: string}} navigation | ||
| */ | ||
| async function prepareNetworkForNavigation(session, settings, navigation) { |
There was a problem hiding this comment.
this is effectively the same as setupPassNetwork previously
| * @param {LH.Gatherer.FRTransitionalDriver} driver | ||
| * @param {LH.Config.Settings} settings | ||
| */ | ||
| async function prepareTargetForNavigationMode(driver, settings) { |
There was a problem hiding this comment.
the same as setupDriver but with storage reset extracted to prepareTargetForIndividualNavigation
| /** @type {Array<LH.IcuMessage>} */ | ||
| const warnings = []; | ||
|
|
||
| const shouldResetStorage = !settings.disableStorageReset && !navigation.disableStorageReset; |
There was a problem hiding this comment.
storage reset was moved here for three main reasons:
- It requires a URL, so it's already specific to a navigation intent, doesn't make sense as a general target setup.
- If we're trying to clear storage, then we should clear before each navigation. We've come a long way in 5 years of Lighthouse and have clarity that
passes/navigationsare not going to be how we solve flows, we're solving flows with FR, so additional navigations should be reloading the same situation unless configured otherwise :) - It does not change the default config sequence anyway, only if you had custom config combinations of
recordTrace: false, useThrottling: truewill it matter.
| @@ -115,7 +115,7 @@ async function cleanBrowserCaches(session) { | |||
|
|
|||
| module.exports = { | |||
| clearDataForOrigin, | |||
| cleanBrowserCaches, | |||
| clearBrowserCaches, | |||
There was a problem hiding this comment.
just consistency I noticed
connorjclark
approved these changes
May 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Starts to extract pieces from gather-runner for shared FR usage.
This creates a
driver/prepare.jswhose goal is to prepare a target for gathering (setup global protocol domains, hung usage handling, throttling, emulation, etc). This one is a little murkier where it should belong. I can see an argument forrunner-helpers.jsinstead, and I can see an argument for a completely new place that previously didn't exist. I chosedriver/preparebecause it is aligned with this logic previously living in the driver monolith, felt weird to put it in thefraggle-rockfolder and have legacy gather-runner reaching into it. If there are strong opinions about another place, it's just a filename/path, so speak up :)Related Issues/PRs
ref #11313