-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
inspector: initial support for Network.loadNetworkResource #58077
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
inspector: initial support for Network.loadNetworkResource #58077
Conversation
Review requested:
|
86ba453
to
435c33f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58077 +/- ##
==========================================
- Coverage 90.06% 90.06% -0.01%
==========================================
Files 641 645 +4
Lines 188846 188996 +150
Branches 37049 37063 +14
==========================================
+ Hits 170087 170211 +124
- Misses 11476 11488 +12
- Partials 7283 7297 +14
🚀 New features to boost your workflow:
|
5e20033
to
1f0c547
Compare
@legendecas cc: @joyeecheung |
8b582eb
to
ce745ca
Compare
The implementation approach was significantly changed in commit ce745ca. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work! The new approach looks more straightforward as a first step to me.
0e70772
to
688e313
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Some minor suggestions..
2a37101
to
160eedb
Compare
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - inspector: initial support for Network.loadNetworkResource ⚠ - format ⚠ - fix build error ⚠ - fix unusual chars ⚠ - remove unix.h ⚠ - fix namespace ⚠ - fix lint ⚠ - fetch network resource on worker thread ⚠ - format ⚠ - format ⚠ - fix build error ⚠ - put method api ⚠ - remove comment ⚠ - avoid making networkResourceManager a static class. ⚠ - format ⚠ - add mutex lock to NetworkResourceManager ⚠ - Update src/inspector/network_agent.cc ⚠ - use url as stream id ⚠ - fix test ⚠ - fix lint ⚠ - use std::optional ⚠ - add flag to manpage ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/16067992462 |
2688004
to
04d8eca
Compare
Landed in b7db89f |
Fixes: #57873
Adds initial support for Network.loadNetworkResource in the inspector.
https://chromedevtools.github.io/devtools-protocol/tot/Network/#method-loadNetworkResource
how to load network resource
Resource fetching is done via a spawned child process to keep the inspector's event loop clean and isolated.The JavaScript code for the fetch process is currently hardcoded, which may not be ideal, but I couldn't come up with a better approach at this point.
When --experimental-inspector-network-resource is set, a dedicated worker for loadNetworkResource is launched.
This worker fetches the resource and returns the result to the frontend.
https://github.com/nodejs/node/pull/58077/files#diff-5d3d58cbcfda008f49966a11d73863e67b78a237aa386f1fbdc5c9124136c1eb
check the behavior in chromium
To verify this change in Chromium, modify the section in the link as shown below and launch Chromium with the built DevTools frontend.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/sdk/Target.ts;l=87-88
# open Chromium with built devtools-frontend Chromium --custom-devtools-frontend=(file path)
other infomation
If this change is considered acceptable, I will send a follow-up change request to expand the Node.js DevTools target with IO support.
If it is necessary to wait until Network.loadNetworkResource becomes stable in CRDP, please feel free to keep this PR pending.