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

browser(firefox): use browsingContextID for frame IDs #3999

Merged

Conversation

aslushnikov
Copy link
Collaborator

BrowsingContextIDs are consistent across the processes, so we can use
them to target frames in both browser and content processes. This will
aid browser-side navigation.

As a nice side-effect, we can drop a round-trip to the content process
for every requestWillBeSent event since we almost always can
attribute all network events to the proper parent frames.

I say "almost", because we in fact fail to correctly attribute requests
from workers that are instantiated by subframes. This, however, is
not working in Chromium ATM, so I consider this to be a minor regression
that is worth the simplification.

BrowsingContextIDs are consistent across the processes, so we can use
them to target frames in both browser and content processes. This will
aid browser-side navigation.

As a nice side-effect, we can drop a round-trip to the content process
for every `requestWillBeSent` event since we *almost* always can
attribute all network events to the proper parent frames.

I say "almost", because we in fact **fail** to correctly attribute requests
from workers that are instantiated by subframes. This, however, is
not working in Chromium ATM, so I consider this to be a minor regression
that is worth the simplification.
@aslushnikov aslushnikov requested a review from dgozman September 29, 2020 09:13
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

This looks great! Do we have tests for network in popups? I am a bit worried that browsingContext might not be there in time, otherwise everything looks perfect.

@aslushnikov
Copy link
Collaborator Author

This looks great! Do we have tests for network in popups? I am a bit worried that browsingContext might not be there in time, otherwise everything looks perfect.

There's a test for request interception for the popup document request - and it works. There's no route.request().frame() though since there's no frame tree to report until the popup is starting loading - but this is consistent with current behavior.

@aslushnikov aslushnikov merged commit 2631e1a into microsoft:master Sep 29, 2020
@aslushnikov aslushnikov deleted the ffff-cross-process-frame-ids branch September 29, 2020 18:22
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.

2 participants