Skip to content

core(fr): user triggered navigations#13496

Merged
adamraine merged 40 commits into
masterfrom
fr-navigation-requestor
Feb 10, 2022
Merged

core(fr): user triggered navigations#13496
adamraine merged 40 commits into
masterfrom
fr-navigation-requestor

Conversation

@adamraine

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor 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.

Comment thread lighthouse-core/runner.js Outdated
@adamraine adamraine changed the title WIP: user triggered navigations (callback) core(fr): user triggered navigations Jan 7, 2022
@adamraine

Copy link
Copy Markdown
Contributor Author

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

Comment thread lighthouse-core/gather/driver/navigation.js Outdated
Comment thread lighthouse-core/gather/driver/prepare.js
Comment thread lighthouse-core/test/fraggle-rock/scenarios/api-test-pptr.js Outdated

@connorjclark connorjclark left a comment

Copy link
Copy Markdown
Collaborator

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!

Comment thread lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js
Comment thread lighthouse-core/gather/driver/network-monitor.js
Comment thread docs/user-flows.md Outdated
Comment thread lighthouse-core/gather/driver/prepare.js Outdated
Comment thread lighthouse-core/test/fraggle-rock/scenarios/api-test-pptr.js Outdated
Comment thread lighthouse-core/test/fraggle-rock/scenarios/api-test-pptr.js Outdated
Comment thread lighthouse-core/test/fraggle-rock/scenarios/api-test-pptr.js Outdated
Comment thread lighthouse-core/test/gather/driver/network-monitor-test.js Outdated

@paulirish paulirish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

love it

Comment thread docs/user-flows.md Outdated
@adamraine

Copy link
Copy Markdown
Contributor 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