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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: don't load blank page twice in first pass #6369

Merged
merged 4 commits into from
Oct 25, 2018
Merged

core: don't load blank page twice in first pass #6369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f28ea7
core: don't load blank page twice in first pass
connorjclark Oct 23, 2018
bfb3ac1
restore original loadBlank
connorjclark Oct 23, 2018
772754e
remove extra call to load blank
connorjclark Oct 24, 2018
c8b2c13
edit mega comment
connorjclark Oct 24, 2018
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
45 changes: 24 additions & 21 deletions lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,22 @@ const Driver = require('../gather/driver.js'); // eslint-disable-line no-unused-
* Execution sequence when GatherRunner.run() is called:
*
* 1. Setup
* A. navigate to about:blank
* B. driver.connect()
Copy link
Member

Choose a reason for hiding this comment

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

I think this is now

A. driver.connect()
B. navigate to about:blank

* C. GatherRunner.setupDriver()
* i. assertNoSameOriginServiceWorkerClients
* ii. beginEmulation
* iii. enableRuntimeEvents
* iv. evaluateScriptOnLoad rescue native Promise from potential polyfill
* v. register a performance observer
* vi. register dialog dismisser
* vii. clearDataForOrigin
* i. navigate to a blank page
Copy link
Member

Choose a reason for hiding this comment

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

remove line

* ii. assertNoSameOriginServiceWorkerClients
* iii. retrieve and save userAgent
* iv. beginEmulation
* v. enableRuntimeEvents
* vi. evaluateScriptOnLoad rescue native Promise from potential polyfill
* vii. register a performance observer
* viii. register dialog dismisser
* iv. clearDataForOrigin
*
* 2. For each pass in the config:
* A. GatherRunner.beforePass()
* i. navigate to about:blank
* ii. Enable network request blocking for specified patterns
* iii. all gatherers' beforePass()
* i. Enable network request blocking for specified patterns
Copy link
Member

Choose a reason for hiding this comment

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

still need i. navigate to about:blank from master (we could maybe add a parenthetical about the first pass? but I don't think it's needed for just the overview up here)

* ii. all gatherers' beforePass()
* B. GatherRunner.pass()
* i. cleanBrowserCaches() (if it's a perf run)
* ii. beginDevtoolsLog()
Expand Down Expand Up @@ -105,7 +105,9 @@ class GatherRunner {
static async setupDriver(driver, options) {
log.log('status', 'Initializing…');
const resetStorage = !options.settings.disableStorageReset;
// Enable emulation based on settings
// In the devtools/extension case, we can't still be on the site while trying to clear state
// So we first navigate to a blank page, then apply our emulation & setup
await GatherRunner.loadBlank(driver);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should this use the config for the first pass (for blank page and duration)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah, I think @paulirish was even borderline on the way to removing the blankPage and blankDuration params entirely from the passes anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused, aren't we still loading blank twice now? Seems like this line should go away and the comments moved into the loadBlank call in run

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry if I confused you with my "two blank calls" comment I meant for the pass and for before setup driver, not for setup driver + before setup driver :)

await driver.assertNoSameOriginServiceWorkerClients(options.requestedUrl);
await driver.beginEmulation(options.settings);
await driver.enableRuntimeEvents();
Expand Down Expand Up @@ -176,7 +178,7 @@ class GatherRunner {
}

/**
* Navigates to about:blank and calls beforePass() on gatherers before tracing
* Calls beforePass() on gatherers before tracing
* has started and before navigation to the target page.
* @param {LH.Gatherer.PassContext} passContext
* @param {Partial<GathererResults>} gathererResults
Expand All @@ -185,9 +187,7 @@ class GatherRunner {
static async beforePass(passContext, gathererResults) {
const blockedUrls = (passContext.passConfig.blockedUrlPatterns || [])
.concat(passContext.settings.blockedUrlPatterns || []);
const blankPage = passContext.passConfig.blankPage;
const blankDuration = passContext.passConfig.blankDuration;
await GatherRunner.loadBlank(passContext.driver, blankPage, blankDuration);

// Set request blocking before any network activity
// No "clearing" is done at the end of the pass since blockUrlPatterns([]) will unset all if
// neccessary at the beginning of the next pass.
Expand Down Expand Up @@ -394,12 +394,11 @@ class GatherRunner {
try {
await driver.connect();
const baseArtifacts = await GatherRunner.getBaseArtifacts(options);
await GatherRunner.loadBlank(driver);
baseArtifacts.BenchmarkIndex = await options.driver.getBenchmarkIndex();
await GatherRunner.setupDriver(driver, options);
baseArtifacts.BenchmarkIndex = await options.driver.getBenchmarkIndex();
Copy link
Collaborator

@patrickhulce patrickhulce Oct 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

wait I take it back this won't work sorry!! :)

It needs to go before we setup emulation since its purpose is establishing the right multiplier (see #6162)

it doesn't seem like it'd be the end of the world to have the two loadBlank calls in this function, was there another reason I'm forgetting that it's inside setupDriver now?

Copy link
Member

Choose a reason for hiding this comment

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

+1 to restoring the original loadBlank line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


// Run each pass
let firstPass = true;
let isFirstPass = true;
for (const passConfig of passes) {
const passContext = {
driver: options.driver,
Expand All @@ -412,6 +411,10 @@ class GatherRunner {
};

await driver.setThrottling(options.settings, passConfig);
if (!isFirstPass) {
// Already on blank page if driver was just set up.
await GatherRunner.loadBlank(driver, passConfig.blankPage, passConfig.blankDuration);
}
await GatherRunner.beforePass(passContext, gathererResults);
await GatherRunner.pass(passContext, gathererResults);
const passData = await GatherRunner.afterPass(passContext, gathererResults);
Expand All @@ -434,10 +437,10 @@ class GatherRunner {
baseArtifacts.traces[passConfig.passName] = passData.trace;
}

if (firstPass) {
if (isFirstPass) {
// Copy redirected URL to artifact in the first pass only.
baseArtifacts.URL.finalUrl = passContext.url;
firstPass = false;
isFirstPass = false;
}
}
const resetStorage = !options.settings.disableStorageReset;
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ describe('GatherRunner', function() {
dismissJavaScriptDialogs: asyncFunc,
enableRuntimeEvents: asyncFunc,
cacheNatives: asyncFunc,
gotoURL: asyncFunc,
registerPerformanceObserver: asyncFunc,
cleanBrowserCaches: createCheck('calledCleanBrowserCaches'),
clearDataForOrigin: createCheck('calledClearStorage'),
Expand Down Expand Up @@ -375,6 +376,7 @@ describe('GatherRunner', function() {
dismissJavaScriptDialogs: asyncFunc,
enableRuntimeEvents: asyncFunc,
cacheNatives: asyncFunc,
gotoURL: asyncFunc,
registerPerformanceObserver: asyncFunc,
cleanBrowserCaches: createCheck('calledCleanBrowserCaches'),
clearDataForOrigin: createCheck('calledClearStorage'),
Expand Down