DevTools: Map Fetch and EventSource to XHR for interception [chromium/src : main]
Wang Neden (Gerrit)
Wang Neden voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang Neden (Gerrit)
Wang Neden added 2 comments![]( "Open in Gerrit")
Clients wishing to intercept `fetch()` API calls must now explicitlyWang Nedento fix the reported bug: let's change the mapping to map XHR, Fetch, Prefetch and EventSource to XHR for the interception? (per https://g-issues.chromium.org/issues/40256663#comment10)
Thanks for the clear direction! I've updated the CL to implement the core of this suggestion.
The code now maps `Fetch` and `EventSource` to `kXhr` for interception, which resolves the main issue reported in the bug. The web tests have been updated to confirm this fix.
I have intentionally **excluded `Prefetch`** from this mapping for now, based directly on your initial, very valid concern about breaking changes. Currently, `Fetch.enable({ resourceType: 'Prefetch' })` fails with a clear error. Mapping it to `kXhr` would change this explicit failure to a silent one, which feels like a more subtle and problematic breaking change.
I believe the right way to handle `Prefetch` is part of a larger, dedicated effort, which I've detailed in my reply to your other comment about the reverse mapping.
if (resource_type ==Wang Nedenthere is a reverse of this mapping later in the file https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/protocol/network_handler.cc;l=1420;drc=6297ba28da79fc75d2f2d58bf79cfca949beef86 where blink::mojom::ResourceType::kPrefetch it re-mapped to Fetch.
That is an excellent catch, and you are absolutely right. Thank you for pointing this out. It highlights a much more fundamental inconsistency, and my current change would indeed make the meaning of the `Fetch` type even more ambiguous across the protocol.
I believe this requires a proper, architectural solution rather than a simple mapping fix.
Here is the two-phased engineering plan I propose:
1. **This CL**: Lands the immediate fix for the user-reported issue.
2. **A Follow-up CL**: This will be a dedicated effort to **implement true interception for Prefetch requests**. This will likely involve investigating the `SubresourceProxyingURLLORLaderService` path. As part of that correct implementation, I will also fix this reverse mapping, ensuring `blink::mojom::ResourceType::kPrefetch` consistently and correctly maps to the `'Prefetch'` protocol type everywhere.
This plan ensures we solve the deeper inconsistency correctly in the long term.
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
triggerAction: nulldoes it make sense to include options that do not have a trigger action?
Related details
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang Neden (Gerrit)
Wang Neden voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wang Neden
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
does it make sense to include options that do not have a trigger action?
You're right, the original test file was confusing because it mixed tests with two different purposes.
To make the testing intent crystal clear, I've adopted your feedback and taken it a step further: I've split the tests into two separate files:
1. `...-interception.js`: This file now exclusively tests successful interception scenarios, and every test case in it has a `triggerAction`.
2. `...-validation.js`: This file is now dedicated to testing the input validation of `Fetch.enable` for unsupported types.
This structure completely separates the two concerns and makes each test file's purpose immediately obvious from its name. I've just pushed the update.
Thanks for pushing for better clarity here!
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
intercepted_resource_types->insert(blink::mojom::ResourceType::kPrefetch);I think we should still keep this right? (by moving this condition to line 1369).
Related details
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang Neden (Gerrit)
Wang Neden voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wang Neden
| Auto-Submit | +1 |
1 comment
intercepted_resource_types->insert(blink::mojom::ResourceType::kPrefetch);I think we should still keep this right? (by moving this condition to line 1369).
Thanks for the quick feedback! I've just pushed an update to address your comment.
I've implemented it so that the original `Fetch` -> `kPrefetch` mapping is preserved, in addition to the new `kXhr` mapping. This is now handled within the unified `if` block, but the `kPrefetch` logic is applied **only** when `resourceType` is `Fetch`, to avoid any side effects on `XHR` and `EventSource`.
The code now reflects the logic we discussed. Please let me know if this updated implementation matches what you had in mind.
Thanks!
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
intercepted_resource_types->insert(blink::mojom::ResourceType::kPrefetch);Wang NedenI think we should still keep this right? (by moving this condition to line 1369).
Thanks for the quick feedback! I've just pushed an update to address your comment.
I've implemented it so that the original `Fetch` -> `kPrefetch` mapping is preserved, in addition to the new `kXhr` mapping. This is now handled within the unified `if` block, but the `kPrefetch` logic is applied **only** when `resourceType` is `Fetch`, to avoid any side effects on `XHR` and `EventSource`.
The code now reflects the logic we discussed. Please let me know if this updated implementation matches what you had in mind.
Thanks!
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |