[FedCM] Hook up FedCM network traffic to show in dev tools (devtools) [chromium/src : main]
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Thanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
Related details
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Yi Gu
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
Thanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
2) Do FedCM requests originate from the renderer?
They all originate from the browser process.
Alex Rudenko (Gerrit)
Alex Rudenko added 3 comments![]( "Open in Gerrit")
@ca...@chromium.org since you know more about the network and interception implementation could you please review as well?
/*loader_id=*/"", request.headers, *request_info,loader_id should be set unless the request is originating from a worker
/*initiator_devtools_request_id=*/"", /*frame_token=*/std::nullopt,frameId is important for attributing a request to the correct frame
Related details
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Yi Gu
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 3 comments![]( "Open in Gerrit")
Please add test coverage.
if (IsDevToolsEnabled(frame_tree_node_id_)) {This check is a little bit too coarse considering what is predicated upon it. The existence of a DevToolsAgentHost doesn't technically mean devtools are "enabled" for given target -- it may be somewhat incidental (e.g. someone doing a target discovery via chrome://inspect). The right thing to check is whether there's an enabled NetworkHandler. A potential problem with setting a request id the way it's done here is that we sometimes use presence of the network request id as an indication that the network domain has been enabled at the time of request creation, which, in this case, would not be a fair assumption.
So... how about this:
- introduce `NetworkHandler::MaybeAssignDevtoolsId(network::ResourceRequest& request)`, where L1348-1349 will go `if (enabled_)` and if not set yet (there may be multiple handlers)
- use the presence of id to gate further probes, including WillSendFedCmRequest() (or just do them unconditionally and expect devtools_instrumentation to bail out if there are no enabled agents).
auto it = url_devtools_request_id_map_.find(final_url);So the final url is generally not the same as the URL we originally created the request for. Any chance we could use a stable key?
Related details
Nicolás Peña (Gerrit)
Nicolás Peña added 3 comments![]( "Open in Gerrit")
Thanks for working on this! Any chance you could get a screencast and post it in the bug to see how it works?
/*frame_id=*/std::nullopt);Similar to Alex's comments above, I think this needs to be set
content::FrameTreeNodeId frame_tree_node_id = content::FrameTreeNodeId());Please don't add a default value, the frame tree node ID should always be set to something
Related details
- Christian Biesinger
suresh potti (Gerrit)
suresh potti added 7 comments![]( "Open in Gerrit")
Sam GotoThanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
2) Do FedCM requests originate from the renderer?
They all originate from the browser process.
Can someone respond to Point 3 ?
loader_id should be set unless the request is originating from a worker
Done
/*initiator_devtools_request_id=*/"", /*frame_token=*/std::nullopt,frameId is important for attributing a request to the correct frame
Done
Similar to Alex's comments above, I think this needs to be set
Done
content::FrameTreeNodeId frame_tree_node_id = content::FrameTreeNodeId());Please don't add a default value, the frame tree node ID should always be set to something
It gets set in IdpNetworkRequestManager cstr.
This check is a little bit too coarse considering what is predicated upon it. The existence of a DevToolsAgentHost doesn't technically mean devtools are "enabled" for given target -- it may be somewhat incidental (e.g. someone doing a target discovery via chrome://inspect). The right thing to check is whether there's an enabled NetworkHandler. A potential problem with setting a request id the way it's done here is that we sometimes use presence of the network request id as an indication that the network domain has been enabled at the time of request creation, which, in this case, would not be a fair assumption.
So... how about this:
- introduce `NetworkHandler::MaybeAssignDevtoolsId(network::ResourceRequest& request)`, where L1348-1349 will go `if (enabled_)` and if not set yet (there may be multiple handlers)
- use the presence of id to gate further probes, including WillSendFedCmRequest() (or just do them unconditionally and expect devtools_instrumentation to bail out if there are no enabled agents).
Done
auto it = url_devtools_request_id_map_.find(final_url);So the final url is generally not the same as the URL we originally created the request for. Any chance we could use a stable key?
I logged the url request at both place at request and reponse and it happens to be same.
Related details
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 3 comments![]( "Open in Gerrit")
Sam GotoThanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
suresh potti2) Do FedCM requests originate from the renderer?
They all originate from the browser process.
Can someone respond to Point 3 ?
Alex (or caseq), can you clarify what your concern is? Obviously this change will expose new requests to devtools (and so, presumably, to webdriver bidi -- I had no idea network requests were part of that).
Regarding network event lifecycle, do you have a document explaining it?
auto it = url_devtools_request_id_map_.find(final_url);suresh pottiSo the final url is generally not the same as the URL we originally created the request for. Any chance we could use a stable key?
I logged the url request at both place at request and reponse and it happens to be same.
They happened to be the same in that case, but for the well-known file they are often different. I think you need to pass the original URL to the callback (or the request ID, I guess)
Related details
- Alex Rudenko
- Andrey Kosyakov
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 4 comments![]( "Open in Gerrit")
if (IsDevToolsAttached(frame_tree_node_id_)) {We can safely drop this check now -- assigning request id will only happen if this is true and does not cost much.
if (IsDevToolsAttached(frame_tree_node_id_)) {ditto
auto it = url_devtools_request_id_map_.find(final_url);suresh pottiSo the final url is generally not the same as the URL we originally created the request for. Any chance we could use a stable key?
I logged the url request at both place at request and reponse and it happens to be same.
... in one particular case, not always. What if there are redirects? Let's just use url_loader pointer as a key.
await new Promise(resolve => setTimeout(resolve, 5000));Let's avoid timeouts in tests, this is a source of flakiness. Is there a deterministic point where a request would have been sent? I assume by the time the dialog promise resolves?
Related details
- Alex Rudenko
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
Sam GotoThanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
suresh potti2) Do FedCM requests originate from the renderer?
They all originate from the browser process.
Christian BiesingerCan someone respond to Point 3 ?
Alex (or caseq), can you clarify what your concern is? Obviously this change will expose new requests to devtools (and so, presumably, to webdriver bidi -- I had no idea network requests were part of that).
Regarding network event lifecycle, do you have a document explaining it?
I for one don't have concerns re (3) -- that is, in my view, if we *are* making a network request, why not expose it? An argument can be made that we generally expose the page-side view of the network, but we also have a precedent of exposing other types of network activity (signed exchange, keep alive etc). A somewhat unfortunate aspect is that we don't consistently apply network interception here, but this isn't new either. @alexr...@chromium.org, can you clarify your concerns?
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Sam GotoThanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
suresh potti2) Do FedCM requests originate from the renderer?
They all originate from the browser process.
Christian BiesingerCan someone respond to Point 3 ?
Andrey KosyakovAlex (or caseq), can you clarify what your concern is? Obviously this change will expose new requests to devtools (and so, presumably, to webdriver bidi -- I had no idea network requests were part of that).
Regarding network event lifecycle, do you have a document explaining it?
I for one don't have concerns re (3) -- that is, in my view, if we *are* making a network request, why not expose it? An argument can be made that we generally expose the page-side view of the network, but we also have a precedent of exposing other types of network activity (signed exchange, keep alive etc). A somewhat unfortunate aspect is that we don't consistently apply network interception here, but this isn't new either. @alexr...@chromium.org, can you clarify your concerns?
My main concern was the lack of test coverage for the change and the #3 is mostly questions about the plans for FedCM network instrumentation for WebDriver. The WebDriver BiDi specification for network events is here https://w3c.github.io/webdriver-bidi/#module-network It would be great if we could add test coverage for FedCM and WebDriver. If the instrumentation for FedCM in CDP is not consistently applied as caseq@ mentioned, it might break WebDriver for automation clients if they start using FedCM. If the instrumentation is partial (for example, not all events are emitted or interception is not possible, or dummy data is used for some events), perhaps we could use a flag to opt-in clients one by one (for example, adding a flag in Network.enable(emitFedCmEvents: bool)).
Related details
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
dp.Network.onRequestWillBeSent(event => {Let's also log other instrumented events and assert other metadata on the network events (at least frameId, type, extraInfo). Are FedCM requests expected to result in Network.requestWillBeSentExtraInfo and Network.responseReceivedExtraInfo events? (we could check that hasExtraInfo is set correctly and events are emitted/not emitted accordingly)
suresh potti (Gerrit)
suresh potti added 5 comments![]( "Open in Gerrit")
We can safely drop this check now -- assigning request id will only happen if this is true and does not cost much.
Done
if (IsDevToolsAttached(frame_tree_node_id_)) {suresh pottiditto
Done
auto it = url_devtools_request_id_map_.find(final_url);suresh pottiSo the final url is generally not the same as the URL we originally created the request for. Any chance we could use a stable key?
Andrey KosyakovI logged the url request at both place at request and reponse and it happens to be same.
... in one particular case, not always. What if there are redirects? Let's just use url_loader pointer as a key.
Done
Let's also log other instrumented events and assert other metadata on the network events (at least frameId, type, extraInfo). Are FedCM requests expected to result in Network.requestWillBeSentExtraInfo and Network.responseReceivedExtraInfo events? (we could check that hasExtraInfo is set correctly and events are emitted/not emitted accordingly)
Federated Credential Management (FedCM) requests initiated from the browser process do not set any ExtraInfo. These requests utilize the following events:
Network.onRequestWillBeSent
Network.onResponseReceived
Network.onLoadingFinished
Modifications were made in the web test to verify the URL and method from the request parameters, as well as the URL, status, and statusText from the response parameters and encodedDataLength from onLoadingFinished parameters.
await new Promise(resolve => setTimeout(resolve, 5000));Let's avoid timeouts in tests, this is a source of flakiness. Is there a deterministic point where a request would have been sent? I assume by the time the dialog promise resolves?
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolás Peña (Gerrit)
Nicolás Peña added 2 comments![]( "Open in Gerrit")
content::FrameTreeNodeId frame_tree_node_id = content::FrameTreeNodeId());suresh pottiPlease don't add a default value, the frame tree node ID should always be set to something
It gets set in IdpNetworkRequestManager cstr.
I mean not having a default value for the parameter.
content::BrowserTaskEnvironment::MainThreadType::UI};Why is this needed?
Related details
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
suresh potti (Gerrit)
suresh potti added 3 comments![]( "Open in Gerrit")
Sam GotoThanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
suresh potti2) Do FedCM requests originate from the renderer?
They all originate from the browser process.
Christian BiesingerCan someone respond to Point 3 ?
Andrey KosyakovAlex (or caseq), can you clarify what your concern is? Obviously this change will expose new requests to devtools (and so, presumably, to webdriver bidi -- I had no idea network requests were part of that).
Regarding network event lifecycle, do you have a document explaining it?
Alex RudenkoI for one don't have concerns re (3) -- that is, in my view, if we *are* making a network request, why not expose it? An argument can be made that we generally expose the page-side view of the network, but we also have a precedent of exposing other types of network activity (signed exchange, keep alive etc). A somewhat unfortunate aspect is that we don't consistently apply network interception here, but this isn't new either. @alexr...@chromium.org, can you clarify your concerns?
My main concern was the lack of test coverage for the change and the #3 is mostly questions about the plans for FedCM network instrumentation for WebDriver. The WebDriver BiDi specification for network events is here https://w3c.github.io/webdriver-bidi/#module-network It would be great if we could add test coverage for FedCM and WebDriver. If the instrumentation for FedCM in CDP is not consistently applied as caseq@ mentioned, it might break WebDriver for automation clients if they start using FedCM. If the instrumentation is partial (for example, not all events are emitted or interception is not possible, or dummy data is used for some events), perhaps we could use a flag to opt-in clients one by one (for example, adding a flag in Network.enable(emitFedCmEvents: bool)).
Given the addition of web_tests, is it still necessary to address Point 3?
content::FrameTreeNodeId frame_tree_node_id = content::FrameTreeNodeId());suresh pottiPlease don't add a default value, the frame tree node ID should always be set to something
Nicolás PeñaIt gets set in IdpNetworkRequestManager cstr.
I mean not having a default value for the parameter.
Done
content::BrowserTaskEnvironment::MainThreadType::UI};Why is this needed?
This change was needed as to address DCHECK failure caused by FrameTreeNode::GloballyFindByID(....) which expects to run in BrowserThread::UI thread.
This came as a surprise as i always thought TaskEnvironment::MainThreadType::UI is same as BrowserThread::UI but it isn't.
Related details
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 3 comments![]( "Open in Gerrit")
Sam GotoThanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
suresh potti2) Do FedCM requests originate from the renderer?
They all originate from the browser process.
Christian BiesingerCan someone respond to Point 3 ?
Andrey KosyakovAlex (or caseq), can you clarify what your concern is? Obviously this change will expose new requests to devtools (and so, presumably, to webdriver bidi -- I had no idea network requests were part of that).
Regarding network event lifecycle, do you have a document explaining it?
Alex RudenkoI for one don't have concerns re (3) -- that is, in my view, if we *are* making a network request, why not expose it? An argument can be made that we generally expose the page-side view of the network, but we also have a precedent of exposing other types of network activity (signed exchange, keep alive etc). A somewhat unfortunate aspect is that we don't consistently apply network interception here, but this isn't new either. @alexr...@chromium.org, can you clarify your concerns?
suresh pottiMy main concern was the lack of test coverage for the change and the #3 is mostly questions about the plans for FedCM network instrumentation for WebDriver. The WebDriver BiDi specification for network events is here https://w3c.github.io/webdriver-bidi/#module-network It would be great if we could add test coverage for FedCM and WebDriver. If the instrumentation for FedCM in CDP is not consistently applied as caseq@ mentioned, it might break WebDriver for automation clients if they start using FedCM. If the instrumentation is partial (for example, not all events are emitted or interception is not possible, or dummy data is used for some events), perhaps we could use a flag to opt-in clients one by one (for example, adding a flag in Network.enable(emitFedCmEvents: bool)).
Given the addition of web_tests, is it still necessary to address Point 3?
Yes, I think it would be great to eventually address #3 unrelated to the CL.
/*loader_id=*/request.devtools_request_id.value(), request.headers,loader_id should be the request ID of the request that loaded the document (which in turn triggered the current network request). Would it be possible to send the correct loader_id here?
pictureUrl : https://idp.example/profile/567I tested locally and I see that requests to https://idp.example/profile/567 and https://idp.example/profile/123 are also being issued but only the requestWillBeSent events are emitted. This could stall automation clients that wait for the loadingFailed or loadingComplete events (stalls Puppeteer when waiting for the idle network and in the frontend they also show up as forever pending). Could you please add instrumentation for the loading failed events as well (I assume that it fails due to inaccessible URL) and corresponding tests?
Related details
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 2 comments![]( "Open in Gerrit")
// For the metrics endpoint, we do not care about the result.we should move this entire if block below the devtools call -- we should still notify devtools that we finished the metrics endpoint call.
if (url_loader && response_info) {thinking about our discussion earlier, I suspect this is null if the request fails below the HTTP layer -- we should still notify devtools if that happens
Related details
- Andrey Kosyakov
suresh potti (Gerrit)
suresh potti added 4 comments![]( "Open in Gerrit")
/*loader_id=*/request.devtools_request_id.value(), request.headers,loader_id should be the request ID of the request that loaded the document (which in turn triggered the current network request). Would it be possible to send the correct loader_id here?
Done
// For the metrics endpoint, we do not care about the result.we should move this entire if block below the devtools call -- we should still notify devtools that we finished the metrics endpoint call.
made changes accordingly
if (url_loader && response_info) {thinking about our discussion earlier, I suspect this is null if the request fails below the HTTP layer -- we should still notify devtools if that happens
made cahnges accordingly
pictureUrl : https://idp.example/profile/567I tested locally and I see that requests to https://idp.example/profile/567 and https://idp.example/profile/123 are also being issued but only the requestWillBeSent events are emitted. This could stall automation clients that wait for the loadingFailed or loadingComplete events (stalls Puppeteer when waiting for the idle network and in the frontend they also show up as forever pending). Could you please add instrumentation for the loading failed events as well (I assume that it fails due to inaccessible URL) and corresponding tests?
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 4 comments![]( "Open in Gerrit")
pictureUrl : https://idp.example/profile/567suresh pottiI tested locally and I see that requests to https://idp.example/profile/567 and https://idp.example/profile/123 are also being issued but only the requestWillBeSent events are emitted. This could stall automation clients that wait for the loadingFailed or loadingComplete events (stalls Puppeteer when waiting for the idle network and in the frontend they also show up as forever pending). Could you please add instrumentation for the loading failed events as well (I assume that it fails due to inaccessible URL) and corresponding tests?
Captured in logs on loading success & failed events
I do see loading failed events but DevTools does not handle it right (still shows as pending): https://i.imgur.com/6TTDxj3.png I think frameId is missing and loaderId is not correct for some of the events. Potentially there are some other problems with instrumentation for those images.
Url: https://devtools.test:8443/inspector-protocol/fedcm/resources/dialog-shown-event.https.html, Status: 200::OKnit: we can probably subscribe to network events after we navigate to the page to avoid logging navigation related events.
dp.Network.onRequestWillBeSent(event => {Let's verify that all events have the same loader ID. It should be the one from the Page.frameNavigated event when we navigate to https://devtools.test:8443/inspector-protocol/fedcm/resources/dialog-shown-event.https.html I tested locally and it looks like it's correct for requestWillBeSent but incorrect for responseReceived events.
We can also verify frameId in the test (which seems to be correct for all events).
await dp.FedCm.enable({disableRejectionDelay: true});nit: should we enable FedCM before we trigger the dialog? to avoid potential flakiness
Related details
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
pictureUrl : https://idp.example/profile/567suresh pottiI tested locally and I see that requests to https://idp.example/profile/567 and https://idp.example/profile/123 are also being issued but only the requestWillBeSent events are emitted. This could stall automation clients that wait for the loadingFailed or loadingComplete events (stalls Puppeteer when waiting for the idle network and in the frontend they also show up as forever pending). Could you please add instrumentation for the loading failed events as well (I assume that it fails due to inaccessible URL) and corresponding tests?
Alex RudenkoCaptured in logs on loading success & failed events
I do see loading failed events but DevTools does not handle it right (still shows as pending): https://i.imgur.com/6TTDxj3.png I think frameId is missing and loaderId is not correct for some of the events. Potentially there are some other problems with instrumentation for those images.
it looks like the image request are not actually going through the instrumentation added in this CL? Could we add a failing test case for instrumentation added in this CL?
suresh potti (Gerrit)
suresh potti added 8 comments![]( "Open in Gerrit")
Sam GotoThanks for a CL, I have got a few high-level questions:
1) could you please add inspector-protocol tests to third_party/blink/web_tests/http/tests/inspector-protocol/network/ to make sure that network events are instrumented in a way that CDP clients expect (typical network events, ability to retrieve request/response body)? Network events have a somewhat complex lifecycle so perhaps a doc would be good to define how much of Network needs to be supported for the requests to work in DevTools?
2) Do FedCM requests originate from the renderer? can they be attributes to a particular navigable? Can FedCM requests run from worker?
3) Are there any compatibility concerns for non-DevTools clients like WebDriver (especially if instrumentation is not complete). I see that FedCM integrates with the Fetch standard: exposing these CDP events would affect WebDriver BiDi. Are there plans to add FedCM x WebDriver coverage in WPT (https://github.com/web-platform-tests/wpt/tree/master/webdriver/tests/bidi/network).
suresh potti2) Do FedCM requests originate from the renderer?
They all originate from the browser process.
Christian BiesingerCan someone respond to Point 3 ?
Andrey KosyakovAlex (or caseq), can you clarify what your concern is? Obviously this change will expose new requests to devtools (and so, presumably, to webdriver bidi -- I had no idea network requests were part of that).
Regarding network event lifecycle, do you have a document explaining it?
Alex RudenkoI for one don't have concerns re (3) -- that is, in my view, if we *are* making a network request, why not expose it? An argument can be made that we generally expose the page-side view of the network, but we also have a precedent of exposing other types of network activity (signed exchange, keep alive etc). A somewhat unfortunate aspect is that we don't consistently apply network interception here, but this isn't new either. @alexr...@chromium.org, can you clarify your concerns?
suresh pottiMy main concern was the lack of test coverage for the change and the #3 is mostly questions about the plans for FedCM network instrumentation for WebDriver. The WebDriver BiDi specification for network events is here https://w3c.github.io/webdriver-bidi/#module-network It would be great if we could add test coverage for FedCM and WebDriver. If the instrumentation for FedCM in CDP is not consistently applied as caseq@ mentioned, it might break WebDriver for automation clients if they start using FedCM. If the instrumentation is partial (for example, not all events are emitted or interception is not possible, or dummy data is used for some events), perhaps we could use a flag to opt-in clients one by one (for example, adding a flag in Network.enable(emitFedCmEvents: bool)).
Alex RudenkoGiven the addition of web_tests, is it still necessary to address Point 3?
Yes, I think it would be great to eventually address #3 unrelated to the CL.
Done
// For the metrics endpoint, we do not care about the result.suresh pottiwe should move this entire if block below the devtools call -- we should still notify devtools that we finished the metrics endpoint call.
made changes accordingly
Done
if (url_loader && response_info) {suresh pottithinking about our discussion earlier, I suspect this is null if the request fails below the HTTP layer -- we should still notify devtools if that happens
made cahnges accordingly
Done
content::BrowserTaskEnvironment::MainThreadType::UI};suresh pottiWhy is this needed?
This change was needed as to address DCHECK failure caused by FrameTreeNode::GloballyFindByID(....) which expects to run in BrowserThread::UI thread.
This came as a surprise as i always thought TaskEnvironment::MainThreadType::UI is same as BrowserThread::UI but it isn't.
Done
pictureUrl : https://idp.example/profile/567suresh pottiI tested locally and I see that requests to https://idp.example/profile/567 and https://idp.example/profile/123 are also being issued but only the requestWillBeSent events are emitted. This could stall automation clients that wait for the loadingFailed or loadingComplete events (stalls Puppeteer when waiting for the idle network and in the frontend they also show up as forever pending). Could you please add instrumentation for the loading failed events as well (I assume that it fails due to inaccessible URL) and corresponding tests?
Alex RudenkoCaptured in logs on loading success & failed events
Alex RudenkoI do see loading failed events but DevTools does not handle it right (still shows as pending): https://i.imgur.com/6TTDxj3.png I think frameId is missing and loaderId is not correct for some of the events. Potentially there are some other problems with instrumentation for those images.
it looks like the image request are not actually going through the instrumentation added in this CL? Could we add a failing test case for instrumentation added in this CL?
Corrected laoderId set on response.
Url: https://devtools.test:8443/inspector-protocol/fedcm/resources/dialog-shown-event.https.html, Status: 200::OKnit: we can probably subscribe to network events after we navigate to the page to avoid logging navigation related events.
Done
Let's verify that all events have the same loader ID. It should be the one from the Page.frameNavigated event when we navigate to https://devtools.test:8443/inspector-protocol/fedcm/resources/dialog-shown-event.https.html I tested locally and it looks like it's correct for requestWillBeSent but incorrect for responseReceived events.
We can also verify frameId in the test (which seems to be correct for all events).
Thanks for pointing it out. Seems i didnt update the logic for DidReceiveFedCmResponse on passing correct loader_id.
nit: should we enable FedCM before we trigger the dialog? to avoid potential flakiness
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolás Peña (Gerrit)
Nicolás Peña added 5 comments![]( "Open in Gerrit")
Low-Coverage-Reason: OTHER.Is this still needed?
urlloader_devtools_request_id_map_;nit: add a comment about what this is storing?
base::UnguessableToken frame_token_;nit: devtools_frame_token_?
if (!callback) {Do we need to notify devtools even if callback is not present? Based on the comment below this only occurs for metrics endpoint so I'm thinking yes? You could move your code to the beginning of the method
status.value_or(network::URLLoaderCompletionStatus(net::ERR_FAILED));Implementations of this usually use url_loader->NetError() instead
Related details
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
suresh potti (Gerrit)
suresh potti added 5 comments![]( "Open in Gerrit")
Low-Coverage-Reason: OTHER.suresh pottiIs this still needed?
Yes, to supppress the warnings on code coverage for newly added code.
nit: add a comment about what this is storing?
Done
base::UnguessableToken frame_token_;suresh pottinit: devtools_frame_token_?
Done
Do we need to notify devtools even if callback is not present? Based on the comment below this only occurs for metrics endpoint so I'm thinking yes? You could move your code to the beginning of the method
Done
status.value_or(network::URLLoaderCompletionStatus(net::ERR_FAILED));Implementations of this usually use url_loader->NetError() instead
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 4 comments![]( "Open in Gerrit")
bool UpdateLoaderIdIfExists(FrameTreeNodeId frame_tree_node_id,`Get` seems like a better name than `Update`
returning a `std::optional<string>` instead of bool + out param is maybe a bit easier to use?
return false;the previous code did the equivalent of `return true` here
// if we cannot get the loader_id from the parent, use an empty string.this does not seem to match the actual behavior. the behavior seems to be not to send notifications if we can't get a loader id...
protocol::Network::ResourceTypeEnum::Other, *response_info,would it make sense to add FedCm to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/devtools_protocol/browser_protocol.pdl;l=5953?q=protocol.pdl&ss=chromium as part of this CL?
Related details
- Alex Rudenko
- Andrey Kosyakov
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
suresh potti (Gerrit)
suresh potti added 4 comments![]( "Open in Gerrit")
bool UpdateLoaderIdIfExists(FrameTreeNodeId frame_tree_node_id,`Get` seems like a better name than `Update`
returning a `std::optional<string>` instead of bool + out param is maybe a bit easier to use?
Done
the previous code did the equivalent of `return true` here
Done
// if we cannot get the loader_id from the parent, use an empty string.this does not seem to match the actual behavior. the behavior seems to be not to send notifications if we can't get a loader id...
Done
protocol::Network::ResourceTypeEnum::Other, *response_info,would it make sense to add FedCm to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/devtools_protocol/browser_protocol.pdl;l=5953?q=protocol.pdl&ss=chromium as part of this CL?
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 12 comments![]( "Open in Gerrit")
const base::UnguessableToken& frame_token,ditto
const base::UnguessableToken& frame_token);You shouldn't need to pass both devtools frame token and `frame_tree_node_id`, the former is available from `RenderFrameHostImpl` on the impl site (i.e. in `devtools_instrumentation.cc` when you resolve `frame_tree_node_id`)
void WillSendFedCmRequest(const FrameTreeNodeId frame_tree_node_id,I just realized you're adding an overload for an identifier that already exists (L471). It's quite unfortunate to have overloads that do unrelated things. Let's rename either one (those you're adding could be `...NetworkResponse` for example).
Also, here and below, no need for `const` since you're passing by value.
std::string& loader_id) {So this optionally returns value through an output parameter and indicates whether it happened through return value? We generally [prefer](https://abseil.io/tips/176) returning just an `std::optional<std::string>` in such cases. Let's also rename this to something like `MaybeGetLoaderIdForFrame()`
if (!frame_tree_node_id) {We should be able to remove this, `GloballyFindByID()` would just return a nullptr.
// if we cannot get the loader_id from the parent, use an empty string.This is a bit misleading now, given we're not dispatching the instrumentation signal in case `UpdateLoderIdIfExists()` fails to obtain the id.
// if we cannot get the loader_id from the parent, use an empty string.As above.
base::UnguessableToken devtools_frame_token_;Just `frame_tree_node_id_` should be sufficient per comments above.
const frame_Id = event.params.frameId;Let's give these two some distinct names instead of using ones that are both not style-compliant and easy to mix up with those in outer scope.
if( (loaderId === loader_Id) && (framedId === frame_Id )){`if ((`
requestIdParams.set(params.requestId, params.request.url);so `requestIdParams` really is rather `urlByRequestId`?
Related details
- Alex Rudenko
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
suresh potti (Gerrit)
suresh potti added 12 comments![]( "Open in Gerrit")
const base::UnguessableToken& frame_token,suresh pottiditto
Done
You shouldn't need to pass both devtools frame token and `frame_tree_node_id`, the former is available from `RenderFrameHostImpl` on the impl site (i.e. in `devtools_instrumentation.cc` when you resolve `frame_tree_node_id`)
Done
void WillSendFedCmRequest(const FrameTreeNodeId frame_tree_node_id,I just realized you're adding an overload for an identifier that already exists (L471). It's quite unfortunate to have overloads that do unrelated things. Let's rename either one (those you're adding could be `...NetworkResponse` for example).
Also, here and below, no need for `const` since you're passing by value.
Done
std::string& loader_id) {So this optionally returns value through an output parameter and indicates whether it happened through return value? We generally [prefer](https://abseil.io/tips/176) returning just an `std::optional<std::string>` in such cases. Let's also rename this to something like `MaybeGetLoaderIdForFrame()`
Done
if (!frame_tree_node_id) {We should be able to remove this, `GloballyFindByID()` would just return a nullptr.
Done
// if we cannot get the loader_id from the parent, use an empty string.This is a bit misleading now, given we're not dispatching the instrumentation signal in case `UpdateLoderIdIfExists()` fails to obtain the id.
Done
// if we cannot get the loader_id from the parent, use an empty string.suresh pottiAs above.
Done
base::UnguessableToken devtools_frame_token_;Just `frame_tree_node_id_` should be sufficient per comments above.
Done
const frame_Id = event.params.frameId;Let's give these two some distinct names instead of using ones that are both not style-compliant and easy to mix up with those in outer scope.
Done
if(loaderId === "" && framedId === "" ){suresh potti`if (`
Done
if( (loaderId === loader_Id) && (framedId === frame_Id )){suresh potti`if ((`
Done
requestIdParams.set(params.requestId, params.request.url);so `requestIdParams` really is rather `urlByRequestId`?
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Nicolás Peña
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolás Peña (Gerrit)
Nicolás Peña voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nicolás Peña
| Code-Review | +1 |
1 comment
Related details
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
auto network_manager = std::make_unique<IdpNetworkRequestManager>(Isn't there a unit test here that we could write to make sure that this doesn't regress going forward?
Related details
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Yi Gu
- suresh potti
suresh potti (Gerrit)
suresh potti added 1 comment![]( "Open in Gerrit")
auto network_manager = std::make_unique<IdpNetworkRequestManager>(Isn't there a unit test here that we could write to make sure that this doesn't regress going forward?
Can be solved by having default value to FrameTreeNodeId param in IdpNetworkRequestManager cstr, where this unit test doesnt have to be touched.
Related details
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
Christian Biesinger (Gerrit)
Christian Biesinger added 5 comments![]( "Open in Gerrit")
protocol::Network::Initiator::TypeEnum::Other,can you not pass your new FedCM value here?
std::optional<network::URLLoaderCompletionStatus> status =I don't think storing this in a variable is useful, but if you want to keep the variable, please move it inside the if below.
auto response_body_str = response_body ? *response_body : std::string();AIUI, `auto` here will be `std::string`, so this will do a copy.
This extra copy is unfortunate. can you either change auto to `const std::string&` or inline this expression into the call to DidReceiveFedCmNetworkResponse?
response_info ? response_info->headers : nullptr, response_body_str,If you pass the response_info to DidReceiveFedCmNetworkResponse instead of passing the headers, it could call ExtractDevToolsInfo to create the URLResponseHeadDevToolsInfoPtr, which sets additional data than just the headers.
FedCMI would put this above Other, though this is up to the devtools owners
Related details
- Alex Rudenko
- Andrey Kosyakov
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 5 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
5 comments
lgtm, but please also get one from Alex.
return std::string();did you mean to return nullopt here?
if (ftn == nullptr) {In case the ftn lookup failed, we won't be able to deliver the event either. So how about we move the lookup to the call site, then bail out if ftn wasn't found and otherwise expect frame token to be just present.
You would also be able to [pass ftn to `DispatchToAgents()`](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_instrumentation.cc;l=93) reducing the number of redundant lookups.
network::mojom::URLRequestDevToolsInfoPtr request_info =Let's move this within the conditional just before `DispatchToAgents()` to reduce the ovehead.
if ((cachedLoaderId === "") && (cachedFramedId === "" )){style: ... `)) {`, please
Related details
- Alex Rudenko
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Code-Coverage
Code-Review
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
pictureUrl : https://idp.example/profile/567suresh pottiI tested locally and I see that requests to https://idp.example/profile/567 and https://idp.example/profile/123 are also being issued but only the requestWillBeSent events are emitted. This could stall automation clients that wait for the loadingFailed or loadingComplete events (stalls Puppeteer when waiting for the idle network and in the frontend they also show up as forever pending). Could you please add instrumentation for the loading failed events as well (I assume that it fails due to inaccessible URL) and corresponding tests?
Alex RudenkoCaptured in logs on loading success & failed events
Alex RudenkoI do see loading failed events but DevTools does not handle it right (still shows as pending): https://i.imgur.com/6TTDxj3.png I think frameId is missing and loaderId is not correct for some of the events. Potentially there are some other problems with instrumentation for those images.
suresh pottiit looks like the image request are not actually going through the instrumentation added in this CL? Could we add a failing test case for instrumentation added in this CL?
Corrected laoderId set on response.
To clarify on this thread:
1) I see that this PR does not add new instrumentation for loading failed events. At the same time, I see that loading failed events are emitted for picture URLs and those events are not handled correctly (always pending) by the DevTools frontend (presumably due to the missing metadata). I would like to understand where those failed events are being instrumented? I think it is important that DevTools shows correctly when requests fail and why (I think seeing requests if everything went well is not so critical). I would consider this part non-blocking as it appears that pictureUrl requests were already partially instrumented previously.
2) Let's add tests and instrumentation that reports failures with the newly instrumented requests (like .well-known/web-identity/accounts.php etc). And that those requests show up as failed correctly in DevTools.
Related details
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
pictureUrl : https://idp.example/profile/567suresh pottiI tested locally and I see that requests to https://idp.example/profile/567 and https://idp.example/profile/123 are also being issued but only the requestWillBeSent events are emitted. This could stall automation clients that wait for the loadingFailed or loadingComplete events (stalls Puppeteer when waiting for the idle network and in the frontend they also show up as forever pending). Could you please add instrumentation for the loading failed events as well (I assume that it fails due to inaccessible URL) and corresponding tests?
Alex RudenkoCaptured in logs on loading success & failed events
Alex RudenkoI do see loading failed events but DevTools does not handle it right (still shows as pending): https://i.imgur.com/6TTDxj3.png I think frameId is missing and loaderId is not correct for some of the events. Potentially there are some other problems with instrumentation for those images.
suresh pottiit looks like the image request are not actually going through the instrumentation added in this CL? Could we add a failing test case for instrumentation added in this CL?
Alex RudenkoCorrected laoderId set on response.
To clarify on this thread:
1) I see that this PR does not add new instrumentation for loading failed events. At the same time, I see that loading failed events are emitted for picture URLs and those events are not handled correctly (always pending) by the DevTools frontend (presumably due to the missing metadata). I would like to understand where those failed events are being instrumented? I think it is important that DevTools shows correctly when requests fail and why (I think seeing requests if everything went well is not so critical). I would consider this part non-blocking as it appears that pictureUrl requests were already partially instrumented previously.
2) Let's add tests and instrumentation that reports failures with the newly instrumented requests (like .well-known/web-identity/accounts.php etc). And that those requests show up as failed correctly in DevTools.
Discussed offline, indeed the instrumentation for loading failed already happens (I overlooked it).
Related details
- Andrey Kosyakov
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
FedCMI would put this above Other, though this is up to the devtools owners
I agree let's put it before Other
suresh potti (Gerrit)
suresh potti added 9 comments![]( "Open in Gerrit")
did you mean to return nullopt here?
removed the function
In case the ftn lookup failed, we won't be able to deliver the event either. So how about we move the lookup to the call site, then bail out if ftn wasn't found and otherwise expect frame token to be just present.
You would also be able to [pass ftn to `DispatchToAgents()`](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_instrumentation.cc;l=93) reducing the number of redundant lookups.
removed this function.
network::mojom::URLRequestDevToolsInfoPtr request_info =Let's move this within the conditional just before `DispatchToAgents()` to reduce the ovehead.
Done
protocol::Network::Initiator::TypeEnum::Other,can you not pass your new FedCM value here?
Passing FedCM here will fail to show networks activities in devtools front end, as plumbing for FedCM required in devtools front-end.
std::optional<network::URLLoaderCompletionStatus> status =I don't think storing this in a variable is useful, but if you want to keep the variable, please move it inside the if below.
status is used at line 1411 for cors error which is outside if block.
auto response_body_str = response_body ? *response_body : std::string();AIUI, `auto` here will be `std::string`, so this will do a copy.
This extra copy is unfortunate. can you either change auto to `const std::string&` or inline this expression into the call to DidReceiveFedCmNetworkResponse?
Done
response_info ? response_info->headers : nullptr, response_body_str,If you pass the response_info to DidReceiveFedCmNetworkResponse instead of passing the headers, it could call ExtractDevToolsInfo to create the URLResponseHeadDevToolsInfoPtr, which sets additional data than just the headers.
Done
FedCMAlex RudenkoI would put this above Other, though this is up to the devtools owners
I agree let's put it before Other
Done
if ((cachedLoaderId === "") && (cachedFramedId === "" )){style: ... `)) {`, please
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 2 comments![]( "Open in Gerrit")
if (loader_id.has_value()) {```suggestion
if (!loader_id.has_value()) {
return;
}
```
if (loader_id.has_value()) {```suggestion
if (!loader_id.has_value()) {
return;
}
```
Related details
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
suresh potti (Gerrit)
suresh potti added 2 comments![]( "Open in Gerrit")
if (loader_id.has_value()) {suresh potti```suggestion
if (!loader_id.has_value()) {
return;
}
```
Done
if (loader_id.has_value()) {suresh potti```suggestion
if (!loader_id.has_value()) {
return;
}
```
Done
Related details
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted Code-Review+1![]( "Open in Gerrit")
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +0 |
1 comment
protocol::Network::Initiator::TypeEnum::Other, status);This is actually a wrong enum, should be `protocol::Network::ResourceTypeEnum::Other`
suresh potti (Gerrit)
suresh potti added 1 comment![]( "Open in Gerrit")
protocol::Network::Initiator::TypeEnum::Other, status);This is actually a wrong enum, should be `protocol::Network::ResourceTypeEnum::Other`
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger added 4 comments![]( "Open in Gerrit")
protocol::Network::Initiator::TypeEnum::Other,suresh pottican you not pass your new FedCM value here?
Passing FedCM here will fail to show networks activities in devtools front end, as plumbing for FedCM required in devtools front-end.
Ah, that is unfortunate... it would be better IMO if the frontend treated unknown types as "other"
&protocol::NetworkHandler::ResponseReceived,just to confirm -- this does not have to be called for failed requests?
if (!response_body.empty()) {hmm... this looks a bit odd to me, if the request was successful but the response was empty it would be good to show that in devtools. is it not necessary to call this function in that situation?
response_info, response_body_str, completion_status);we should probably remove the entry from urlloader_devtools_request_id_map_ here, since we will never use it again.
Related details
- Alex Rudenko
- Andrey Kosyakov
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
suresh potti (Gerrit)
suresh potti added 3 comments![]( "Open in Gerrit")
just to confirm -- this does not have to be called for failed requests?
Yes. Only request & loadingfailed event should suffice.
if (!response_body.empty()) {hmm... this looks a bit odd to me, if the request was successful but the response was empty it would be good to show that in devtools. is it not necessary to call this function in that situation?
Not sure if it adds any value. alexr...@chromium.org can you comment on this ?
response_info, response_body_str, completion_status);we should probably remove the entry from urlloader_devtools_request_id_map_ here, since we will never use it again.
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
Christian Biesinger voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christian Biesinger
| Code-Review | +1 |
1 comment
lgtm pending a response from alex
Related details
- Alex Rudenko
- Andrey Kosyakov
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
if (!response_body.empty()) {suresh pottihmm... this looks a bit odd to me, if the request was successful but the response was empty it would be good to show that in devtools. is it not necessary to call this function in that situation?
Not sure if it adds any value. alexr...@chromium.org can you comment on this ?
Indeed, I think it would be better to check for `status.error_code == net::OK` to detect a successful request. Also, now that I paid a closer attention I think the BodyDataReceived handler should be invoked before LoadingComplete handler.
Related details
suresh potti (Gerrit)
suresh potti added 1 comment![]( "Open in Gerrit")
if (!response_body.empty()) {suresh pottihmm... this looks a bit odd to me, if the request was successful but the response was empty it would be good to show that in devtools. is it not necessary to call this function in that situation?
Alex RudenkoNot sure if it adds any value. alexr...@chromium.org can you comment on this ?
Indeed, I think it would be better to check for `status.error_code == net::OK` to detect a successful request. Also, now that I paid a closer attention I think the BodyDataReceived handler should be invoked before LoadingComplete handler.
- Alex Rudenko
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted Code-Review+1![]( "Open in Gerrit")
- Andrey Kosyakov
- Christian Biesinger
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Christian Biesinger (Gerrit)
Christian Biesinger voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andrey Kosyakov
- Sam Goto
- Sam Goto
- Yi Gu
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
1 comment
Related details
suresh potti (Gerrit)
suresh potti added 1 comment![]( "Open in Gerrit")
auto network_manager = std::make_unique<IdpNetworkRequestManager>(suresh pottiIsn't there a unit test here that we could write to make sure that this doesn't regress going forward?
Can be solved by having default value to FrameTreeNodeId param in IdpNetworkRequestManager cstr, where this unit test doesnt have to be touched.
Acknowledged
Related details
- Sam Goto
- Sam Goto
- Yi Gu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
suresh potti (Gerrit)
suresh potti voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[FedCM] Hook up FedCM network traffic to show in dev tools (devtools)
- M content/browser/devtools/devtools_instrumentation.cc
- M content/browser/devtools/devtools_instrumentation.h
- M content/browser/webid/idp_network_request_manager.cc
- M content/browser/webid/idp_network_request_manager.h
- M content/browser/webid/idp_network_request_manager_unittest.cc
- M content/browser/webid/test/mock_idp_network_request_manager.cc
- M third_party/blink/public/devtools_protocol/browser_protocol.pdl
- A third_party/blink/web_tests/http/tests/inspector-protocol/fedcm/fedcm-request-captured-in-devtools-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/fedcm/fedcm-request-captured-in-devtools.js
Code-Review: +1 by Andrey Kosyakov, +1 by Alex Rudenko, +1 by Christian Biesinger
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolás Peña (Gerrit)
Nicolás Peña added 1 comment![]( "Open in Gerrit")
- suresh potti
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
suresh potti (Gerrit)
suresh potti added 1 comment![]( "Open in Gerrit")
url_loader.reset();Why is this removed?
URLLoader ownership transfers from local variable to callback closure via std::move(). Callback keeps URLLoader alive during network operation. When callback executes, URLLoader becomes function parameter, automatically destroyed when function ends. No manual cleanup needed - RAII handles resource management automatically.