core(fr): extract driver preparation methods #12445
Conversation
|
|
||
| // `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.
this little 4 lines is new based on https://bugs.chromium.org/p/chromium/issues/detail?id=990945&q=setSkipAllPauses&can=2
| * @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
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