-
Notifications
You must be signed in to change notification settings - Fork 43
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
Network Module: responseStarted
#435
Comments
It's basically the same point in the request lifecycle as Having some event at that point seems necessary for interception use cases, and it seems better to avoid the CDP design of having an entirely different set of lifecycle events depending on whether you are intercepting the request or passively monitoring it. |
The way I read your comment is: Once the Interception feature is implemented, then this is unblocked. Otherwise, it's effectively not possible to do so at the moment (at least with CDP). Does this interpretation sound correct? |
I think there are two separate concerns here. One is: do we need an event at this point in the lifecycle? The answer to that appears to be "yes" if we want to use a consistent set of events for network request monitoring and interception; being able to modify the response headers seems like an important use case for interception, and this is the right point in the lifecycle for that. The second is: does this exactly map onto existing CDP events. The answer seems to be "no"; the Network domain events seem to happen at slightly different times, and the Fetch domain events are only emitted if you block the request; I assume that the BiDi mapper implementation intercepting any request/response where this event is enabled will be considered too high overhead. So this does indeed seem like it would require CDP changes to be efficiently implemented in Chromium. That seems broadly like it ought to be possible given that it corresponds closely to an existing lifecycle point both in the Fetch domain, and in the Web Extensions API. But it is indeed more work for you (by contrast we've implemented this in Gecko and I'm not aware that it caused any special difficulties). |
Not much to add on top of what @jgraham said, but regarding our implementation: this event was already mapped to internal lifecycle events we used in DevTools, so this was relatively easy to add. If there are specific fields that are problematic for you here, can you share the list? I can check what we are doing for them, but in general I think it's expected that the event will only contain partial information compared to responseCompleted (eg timings will not be all available). |
IIUC, a motivating use case for Mozilla for the Network module in its current state is being able to create a HAR file, discussed in #262. Is the Another thing I noticed is that there's no event for when the body response is coming in, similar to "progress" events in various web APIs. Is this an intentional omission from the model? |
Or perhaps #196 is the more direct statement of that use case: observe network activity without modifying (intercepting) it. |
I don't think HAR generation needs the responseStarted event. For instance, I wrote a module to build har files from bidi events with selenium which is only using beforeRequestSent and responseCompleted. I think the main motivation is what @jgraham mentioned at #435 (comment) : we want to have a consistent set of events for observing and intercepting network events. |
If this is only needed for network intercept, how about we drop it until we spec that feature? Implementing it before network intercept is a risk, if we find it's not the right model for intercept we'll need a new event, and the current one would be vestigial. |
There's already a PR open for network request intercept, so we can discuss whether it's a good fit directly: #429 As best I understand it this event matches what both web extensions and CDP already have for their network request interception features, so it would be surprising to find that it's the wrong fit for BiDi. So my guess is that dropping it would be quite a lot of work (to edit the spec and tests), only be temporary, and therefore represents a bad tradeoff. If Chrome is unable to implement the full event in the short term then some use cases (such as HAR generation) are still possible without this event. |
Ah, I didn't realize you'd created #429. Reviewing that is the best path forward then. |
As CDP does not have a corresponding event (w3c/webdriver-bidi#435), just mirror the `responseCompleted` event. Bug: #765 Spec: https://w3c.github.io/webdriver-bidi/#event-network-responseStarted
As CDP does not have a corresponding event (w3c/webdriver-bidi#435), just mirror the `responseCompleted` event. Bug: #765 Spec: https://w3c.github.io/webdriver-bidi/#event-network-responseStarted
As CDP does not have a corresponding event (w3c/webdriver-bidi#435), just mirror the `responseCompleted` event. Bug: #765 Spec: https://w3c.github.io/webdriver-bidi/#event-network-responseStarted
As CDP does not have a corresponding event (w3c/webdriver-bidi#435), just mirror the `responseCompleted` event. Bug: #765 Spec: https://w3c.github.io/webdriver-bidi/#event-network-responseStarted
As CDP does not have a corresponding event (w3c/webdriver-bidi#435), just mirror the `responseCompleted` event. Fixed: #765 Spec: https://w3c.github.io/webdriver-bidi/#event-network-responseStarted
In the Chromium side we have decided to just mirror the This is not ideal, as but as mentioned in the first comment, CDP doesn't have a corresponding network lifecycle event. If more pressing use cases for that event appear in the future, we can consider adding a new event to CDP. |
Originally tracked via #204
@thiagowfx:
network.responseStarted
does not map to CDP (c.f. https://chromedevtools.github.io/devtools-protocol/tot/Network/). There's no straightforward way to do it in CDP.The closest event is
Network.responseReceivedExtraInfo
, however:Network.responseReceived
:What is the motivation to include this in the spec? Can we revisit if it is absolutely necessary?
(cc @sadym-chromium as we discussed
Also cc @OrKoN to confirm the above statement from the CDP side.)
@OrKoN:
The text was updated successfully, but these errors were encountered: