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

refactor: simplify the Lifecycle Watcher #12295

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Lightning00Blade
Copy link
Collaborator

@Lightning00Blade Lightning00Blade commented Apr 19, 2024

We can expect the correct order of event for DOMContentLoaded and load events,
that the main frame's will come last upstream CL.

@Lightning00Blade Lightning00Blade force-pushed the simplify-the-watcher branch 4 times, most recently from ab924b9 to d040223 Compare April 23, 2024 13:08
@Lightning00Blade Lightning00Blade marked this pull request as ready for review April 23, 2024 13:08
// CDP provided the correct order for Loading Events
// And NetworkIdle is a global state
// Consider removing
for (const child of frame.childFrames()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so was it here only for the empty frames? isn't this code also checks for networkidle events which could come in any order of the frames/subframes? Is it definitely not possible that the load or domcontent loader events arrive after the main frame load/domcontentloaded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So for the Lifecycle events they now (after the linked CL) come in order, that's only true for parsing so anything coming from the HTML or Scripts (evaluateOnNewDocument included) will be correct.
As for the NetworkIdle I have to double check but I think the state for every frame is track on the same NetworkIdle detector so it's global for the page (comment also suggest that).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure it can be tracked globally with OOPIFs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah you may be correct. I don't think we have such test. I will write one here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants