Refactor the frame's lifecycle event handling code #644
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ankur22
force-pushed
the
refactor/593-lifecycle
branch
3 times, most recently
from
5349147 to
80e94c4
Compare
inancgumus
reviewed
Nov 9, 2022
There was a problem hiding this comment.
I'm glad we got rid of all these 🥳 Thanks! Let's wait for #643 before merging this one.
ankur22
force-pushed
the
refactor/593-lifecycle
branch
from
5d5f08d to
7a3a290
Compare
ankur22
changed the base branch from
refactor/593-lifecycle-tests
to
refactor/593-lifecycle-tests-refactored
This timer is no longer needed since the fixes in PR #555 and #597. These PRs helped to ensure that we receive the CDP events in order and that we correctly handle them when we do receive them. The networkidle timer was in place for a situation where we might not receive the networkidle lifecycle event from the browser via CDP. This could still occur, which would mean that there's a bug outside of our direct control. When this happens we should find a better solution.
There's nothing listening to these events. We can add it back in when there is need for it, but at the moment it's just dead code.
When recalculateLifecycle is called from clearLifecycle, all it is doing is clearing the subtreeLifecycleEvents, so now we're just explicitly doing that and not calling recalculateLifecycle.
These internal lifecycle events aren't being listened to by any handler. Let's remove it for now and add it back in when there is a need for them.
Each frame (main and the subs) receive their own lifecycle events in the correct order (so the sub frames first, and then the root/main frame). The reason for having this code in the first place I believe is due to out of order CDP event, which has now been fixed in PR #555. After extensive stress testing I can confirm that we no longer need to perform the recalculateLifecycle in a recursive way. So each frame should be in charge of recalculating its own lifecycle. There maybe a need to do this in the future if the browser changes its behaviour on how it deals with lifecycle events, but at this point we should not consider adding such a recursive call.
We're only ever going to fire the newly received CDP lifecycle event so there's no need for the for loop to go over all the events that have already fired and ignore the ones that have already been emitted to internal handlers. This should lead us to refactoring out the subtreeLifecycleEvents as there's no need for it now, it's just a duplicate of lifecycleEvents.
subtreeLifecycleEvents is no longer needed. It's just a duplicate of lifecycleEvents.
There's no need for recalculateLifecycle since all it is now doing is emitting the event to internal handlers. So refactor/move it to onLifecycleEvent. Closes: #593
This is no longer needed since it's only just logging a debug log.
ankur22
force-pushed
the
refactor/593-lifecycle
branch
from
7a3a290 to
c199c80
Compare
Base automatically changed from
refactor/593-lifecycle-tests-refactored
to
main
November 30, 2022 14:54
inancgumus
added a commit
that referenced
this pull request
Jan 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #593
These were the prerequisite PRs to fix and add tests around lifecycle events:
Now that these are in place, I feel confident that the refactoring in this PR doesn't affect the
waitUntillifecycle behaviour.Each commit explains why the change was made and eventually we end up in a situation where we no longer need the networkIdle timer,
recalculateLifecycleandsubtreeLifecycleEvents.I believe the reasons why lifecycle event was so complicated in the first place was down to unordered CDP events, which has since been resolved in PR #555. Now we're seeing all CDP events come in order, starting with events coming in for the sub frame and ending on the root/main frame, from
DOMContentLoaded, toload, and ending onnetworkIdle. NOTE: there are other lifecycle events that the browser sends through, but we're not interested in those yet.