-
Notifications
You must be signed in to change notification settings - Fork 4.6k
chore(bidi): add support for fetching response bodies #37244
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
Conversation
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
async updateNetworkDataCollection() { | ||
await this._networkManager.setDataCollection(true); |
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.
nit: just inline this call as there is only one call site of the method.
if (enable) { | ||
if (!this._dataCollectorId) { | ||
const dataTypes: [bidi.Network.DataType] = [bidi.Network.DataType.Response]; | ||
const maxEncodedDataSize = maxSize ?? 10_000_000; |
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.
Let's make it 200Mb to match Chromium constant.
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.
I've set it to 20_000_000
as per this comment.
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.
Let's add a comment pointing at https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_network_agent.cc;l=134;drc=4128411589187a396829a827f59a655bed876aa7 as the source of the number.
this._dataCollectorId = collector; | ||
} | ||
} else { | ||
if (this._dataCollectorId) { |
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.
This branch is never reached, let's drop it.
await this._updateProtocolRequestInterception(); | ||
} | ||
|
||
async setDataCollection(enable: boolean, maxSize?: number) { |
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.
Let's rename the method into initialize()
and remove all the if checks, we want the resource collection to be be enabled and done so during the setup phase.
Also if we don't have to pass collector
to network.getData
, there is no need to store in the network manager. Let's keep only the code/data that is actually used.
return; | ||
const getResponseBody = async () => { | ||
throw new Error(`Response body is not available for requests in Bidi`); | ||
const { bytes } = await this._session.send('network.getData', { request: params.request.request, dataType: bidi.Network.DataType.Response }); |
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.
Do we need to also pass collector
parameter?
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.
Heads-up, #37321 is adding the same functionality.
aab8eb0
to
d1a031a
Compare
This comment has been minimized.
This comment has been minimized.
const dataTypes: [bidi.Network.DataType] = [bidi.Network.DataType.Response]; | ||
const maxEncodedDataSize = 20_000_000; | ||
const contexts: [string] = [this._session.sessionId]; | ||
await this._session.send('network.addDataCollector', { dataTypes, maxEncodedDataSize, contexts }); |
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.
Is there a reason we do it per page, rather than initialize once per browser session as in https://github.com/microsoft/playwright/pull/37321/files#diff-8753d59ba95abdc3367549f4caf85cbe4fa7b702d4f94427301bc9eadf4e79e1 ?
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.
No particular reason, I've updated the PR to do it once per session.
d1a031a
to
a6992d1
Compare
Test results for "tests 1"5 flaky45357 passed, 816 skipped Merge workflow run. |
Fixes #37218 and (at least) the following tests in Firefox:
tests/library/browsercontext-base-url.spec.ts
:tests/library/browsercontext-credentials.spec.ts
:tests/library/browsercontext-har.spec.ts
:tests/library/browsertype-connect.spec.ts
:tests/library/har.spec.ts
:tests/page/frame-goto.spec.ts
:tests/page/page-event-request.spec.ts
:tests/page/page-history.spec.ts
:tests/page/page-network-response.spec.ts
:tests/page/page-request-fallback.spec.ts
:tests/page/page-request-fulfill.spec.ts
:tests/page/page-request-intercept.spec.ts
:tests/page/workers.spec.ts
: