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.

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(fr): user triggered navigations #13496

Merged
merged 40 commits into from
Feb 10, 2022
Merged

core(fr): user triggered navigations #13496

merged 40 commits into from
Feb 10, 2022

Conversation

adamraine
Copy link
Member

Doc

Interested in any feedback on the callback implementation.

Some details not mentioned in design doc:

  • Need to backfill requestedUrl in a lot of places because we don't have prior knowledge of the URL with a callback function.
  • Can't clear data for origin without prior knowledge of the URL, so this step is explicitly skipped when given a callback. It is already skipped for every flow step except the first, which should cover most usages of the callback.

Related
#13375
#11313

session.setNextProtocolTimeout(Infinity);
waitForPageNavigateCmd = session.sendCommand('Page.navigate', {url: requestor});
} else {
waitForPageNavigateCmd = requestor();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be useful to make a new status logging step here, and keep the original for this method the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will have to replace the URL in the original log message with something for user controlled navigations. I think that would make a log statement here redundant.

lighthouse-core/runner.js Outdated Show resolved Hide resolved
@adamraine adamraine changed the title WIP: user triggered navigations (callback) core(fr): user triggered navigations Jan 7, 2022
@adamraine
Copy link
Member Author

I'm gonna start working on test cases, but this is ready for review otherwise.

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

just add some docs about best practices for requestor function and LGTM!

docs/user-flows.md Outdated Show resolved Hide resolved
lighthouse-core/gather/driver/prepare.js Outdated Show resolved Hide resolved
lighthouse-core/test/gather/driver/network-monitor-test.js Outdated Show resolved Hide resolved
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

love it

docs/user-flows.md Outdated Show resolved Hide resolved
@adamraine
Copy link
Member Author

Welcome to the future 😎

@adamraine adamraine merged commit 8fa48dd into master Feb 10, 2022
@adamraine adamraine deleted the fr-navigation-requestor branch February 10, 2022 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants