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

Jump to bottom

chore: emulate frameStartedNavigating event #2879

Merged
merged 6 commits into from
Dec 11, 2024
Merged

chore: emulate frameStartedNavigating event #2879

merged 6 commits into from
Dec 11, 2024

Conversation

sadym-chromium
Copy link
Collaborator

@sadym-chromium sadym-chromium commented Dec 10, 2024 •
edited
Loading

Implementation of Alternative 3. Rely on “Network.requestWillBeSent” CDP event. Required for #2856.

Emit frameStartedNavigating event on CdpTarget before Network.requestWillBeSent.

@sadym-chromium sadym-chromium requested a review from OrKoN December 10, 2024 12:37
@OrKoN OrKoN added the puppeteer Run Puppeteer test when added to PR label Dec 10, 2024
OrKoN
OrKoN reviewed Dec 10, 2024
View reviewed changes
src/cdp/CdpConnection.ts Outdated Show resolved Hide resolved
OrKoN
OrKoN reviewed Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

I see that 'Network.requestWillBeSent' is used once in chromium-bidi? what was the race-condition that you mentioned?

@OrKoN
Copy link
Collaborator

OrKoN commented Dec 10, 2024

WDYT if we implement it this way instead #2880 (plus network domain enablement changes )? Note that TargetEvents.FrameStartedNavigating should always fire before the network event since it is subscribed to the network event first.

@sadym-chromium sadym-chromium mentioned this pull request Dec 11, 2024
Closed
@sadym-chromium
Copy link
Collaborator Author

WDYT if we implement it this way instead #2880 (plus network domain enablement changes )? Note that TargetEvents.FrameStartedNavigating should always fire before the network event since it is subscribed to the network event first.

done

@sadym-chromium sadym-chromium requested a review from OrKoN December 11, 2024 10:33
@sadym-chromium
Copy link
Collaborator Author

There is no e2e tests though

@sadym-chromium sadym-chromium changed the title chore: emulate Page.frameStartedNavigating event chore: emulate frameStartedNavigating event Dec 11, 2024
OrKoN
OrKoN requested changes Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

EmulatedCdpMapping is not needed anymore?

@sadym-chromium
Copy link
Collaborator Author

EmulatedCdpMapping is not needed anymore?

right, my bad, artefact after merging. Fixed

@sadym-chromium sadym-chromium requested a review from OrKoN December 11, 2024 12:05
@OrKoN
Copy link
Collaborator

OrKoN commented Dec 11, 2024

(No new Puppeteer failures)

@sadym-chromium sadym-chromium merged commit 43ad1d0 into main Dec 11, 2024
48 of 49 checks passed
@sadym-chromium sadym-chromium deleted the sadym/emulate-page-frameStartedNavigating branch December 11, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrKoN OrKoN OrKoN approved these changes

Assignees
No one assigned
Labels
puppeteer Run Puppeteer test when added to PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sadym-chromium @OrKoN