[WIP] Decode collected Durable Messages on retrieval [chromium/src : main]
0 views
Skip to first unread message
Andrey Kosyakov (Gerrit)
Aug 6, 2025, 1:38:53 PMAug 6
to Alex N. Jose, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Alex N. Jose
Andrey Kosyakov added 11 comments![]( "Open in Gerrit")
File services/network/devtools_durable_msg.h
Line 49, Patchset 2 (Latest):
const base::span<const uint8_t> as_span() const { return bytes_; }Andrey Kosyakov . unresolved
Thought you would want to avoid this per Adam's suggestions?
Line 42, Patchset 2 (Latest):
bool CopyTo(scoped_refptr<net::GrowableIOBuffer> decoded_buffer) const;Andrey Kosyakov . unresolved
Maybe DecodeTo()?
File services/network/devtools_durable_msg.cc
Line 41, Patchset 2 (Latest):
if (base::checked_cast<size_t>(decoded_buffer->capacity()) <Andrey Kosyakov . unresolved
So I wound expect the only reason to use a GrowableIOBuffer in the API (as opposed to a span<>) would be to actually grow it as needed? Otherwise span<> would be a better type to use in the interface.
Line 68, Patchset 2 (Latest):
if (decoding_stream->MayHaveMoreBytes()) {Andrey Kosyakov . unresolved
Ditto re growing the buffer on demand.
File services/network/devtools_durable_msg_collector.cc
Line 40, Patchset 2 (Latest):
CHECK(message->second->CopyTo(decoded_buffer));Andrey Kosyakov . unresolved
Looking at the code, it doesn't look like we can CHECK() here.
File services/network/devtools_durable_msg_encoded_source_stream.h
Line 20, Patchset 2 (Latest):
int Read(net::IOBuffer* dest_buffer,Andrey Kosyakov . unresolved
Move overrides to private?
File-level comment, Patchset 2 (Latest):
Andrey Kosyakov . unresolved
nit: there's no need in putting it into a separate file as long there's just one client. Unless you'd like to write a unit test for it that is ;-)
File services/network/devtools_durable_msg_encoded_source_stream.cc
Line 13, Patchset 2 (Latest):
auto source_span = msg_->as_span();Andrey Kosyakov . unresolved
Looks like you don't really need the entire message? Perhaps pass just the span?
Line 22, Patchset 2 (Latest):
bytes_read_ += consume;Andrey Kosyakov . unresolved
Once you start storing and not the message (`span<const uint8_t> remaining_bytes_`), you can just do `remaining_bytes_.remove_prefix(consume);` here to simplify all the math (and ditch `bytes_read_`)
File services/network/url_loader.cc
Line 1320, Patchset 2 (Latest):
!response_->client_side_content_decoding_types.empty()) {Andrey Kosyakov . unresolved
Ditch this check perhaps for simplicity?
File third_party/blink/web_tests/http/tests/inspector-protocol/network/get-durable-http-response-gzip-body-expected.txt
Line 4, Patchset 2 (Latest):
KioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKio=Andrey Kosyakov . unresolved
Maybe to atob() on the output so it's more evident it's correct?
Related details
Attention is currently required from:
- Alex N. Jose
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Aug 7, 2025, 10:34:14 PMAug 7
to Andrey Kosyakov, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Andrey Kosyakov
Alex N. Jose added 11 comments![]( "Open in Gerrit")
File services/network/devtools_durable_msg.h
Line 49, Patchset 2:
const base::span<const uint8_t> as_span() const { return bytes_; }Andrey Kosyakov . resolved
Thought you would want to avoid this per Adam's suggestions?
Alex N. Jose
Done
Line 42, Patchset 2:
bool CopyTo(scoped_refptr<net::GrowableIOBuffer> decoded_buffer) const;Andrey Kosyakov . resolved
Alex N. JoseMaybe DecodeTo()?
Marked as resolved, renamed back to `Retrieve`
File services/network/devtools_durable_msg.cc
Line 41, Patchset 2:
if (base::checked_cast<size_t>(decoded_buffer->capacity()) <Andrey Kosyakov . resolved
So I wound expect the only reason to use a GrowableIOBuffer in the API (as opposed to a span<>) would be to actually grow it as needed? Otherwise span<> would be a better type to use in the interface.
Alex N. Jose
Done
Ditto re growing the buffer on demand.
Alex N. Jose
Done
File services/network/devtools_durable_msg_collector.cc
Looking at the code, it doesn't look like we can CHECK() here.
Alex N. Jose
Done
File services/network/devtools_durable_msg_encoded_source_stream.h
Line 20, Patchset 2:
int Read(net::IOBuffer* dest_buffer,Andrey Kosyakov . unresolved
Move overrides to private?
Alex N. Jose
They seem to be public methods per net/filter/source_stream.h
nit: there's no need in putting it into a separate file as long there's just one client. Unless you'd like to write a unit test for it that is ;-)
Alex N. Jose
Done
File services/network/devtools_durable_msg_encoded_source_stream.cc
Looks like you don't really need the entire message? Perhaps pass just the span?
Alex N. Jose
Done
Line 22, Patchset 2:
bytes_read_ += consume;Andrey Kosyakov . unresolved
Once you start storing and not the message (`span<const uint8_t> remaining_bytes_`), you can just do `remaining_bytes_.remove_prefix(consume);` here to simplify all the math (and ditch `bytes_read_`)
Alex N. Jose
That API isn't available in base::span.
File services/network/url_loader.cc
Line 1320, Patchset 2:
!response_->client_side_content_decoding_types.empty()) {Andrey Kosyakov . resolved
Ditch this check perhaps for simplicity?
Alex N. Jose
Done
File third_party/blink/web_tests/http/tests/inspector-protocol/network/get-durable-http-response-gzip-body-expected.txt
Line 4, Patchset 2:
KioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKio=Andrey Kosyakov . resolved
Maybe to atob() on the output so it's more evident it's correct?
Attention is currently required from:
- Andrey Kosyakov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Aug 20, 2025, 9:24:47 PMAug 20
to Chromium LUCI CQ, Andrey Kosyakov, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Alex N. Jose abandoned this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Aug 27, 2025, 4:21:12 PM (12 days ago) Aug 27
to Adam Rice, Andrey Kosyakov, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Andrey Kosyakov
Alex N. Jose added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 10 (Latest):
Alex N. Jose . resolved
@ca...@chromium.org, @ri...@chromium.org,
This CL follows up on the base implementation, to add decoding on retrieval. Could I get your review of this CL?
Thanks!
Related details
Attention is currently required from:
- Adam Rice
- Andrey Kosyakov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Aug 27, 2025, 4:45:50 PM (12 days ago) Aug 27
to Alex N. Jose, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Alex N. Jose
Andrey Kosyakov added 3 comments![]( "Open in Gerrit")
Patchset-level comments
Andrey Kosyakov . resolved
Looks mostly good to me!
File services/network/devtools_durable_msg.h
Line 34, Patchset 10 (Latest):
size_t byte_size() const { return bytes_.size(); }Andrey Kosyakov . unresolved
Does this one still make sense?
File services/network/devtools_durable_msg.cc
Line 47, Patchset 10 (Latest):
size_t bytes_read_ = 0;Andrey Kosyakov . unresolved
Can we ditch this and just do `encoded_bytes_ = encoded_bytes_.subspan(bytes_read)` as we go? Basically, at any moment the encoded_bytes_ keeps the remaining bytes to be read.
Related details
Attention is currently required from:
- Adam Rice
- Alex N. Jose
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Aug 27, 2025, 7:48:13 PM (12 days ago) Aug 27
to Adam Rice, Andrey Kosyakov, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Andrey Kosyakov
Alex N. Jose added 2 comments![]( "Open in Gerrit")
File services/network/devtools_durable_msg.h
Line 34, Patchset 10:
size_t byte_size() const { return bytes_.size(); }Andrey Kosyakov . unresolved
Does this one still make sense?
Alex N. Jose
It's used in tests only. Perhaps make this `_for_testing`?
File services/network/devtools_durable_msg.cc
Can we ditch this and just do `encoded_bytes_ = encoded_bytes_.subspan(bytes_read)` as we go? Basically, at any moment the encoded_bytes_ keeps the remaining bytes to be read.
Attention is currently required from:
- Adam Rice
- Andrey Kosyakov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Aug 27, 2025, 7:54:33 PM (12 days ago) Aug 27
to Alex N. Jose, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Alex N. Jose
Andrey Kosyakov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
3 comments
Patchset-level comments
File services/network/devtools_durable_msg.h
Line 34, Patchset 10:
size_t byte_size() const { return bytes_.size(); }Andrey Kosyakov . unresolved
Alex N. JoseDoes this one still make sense?
It's used in tests only. Perhaps make this `_for_testing`?
Andrey Kosyakov
sgtm!
File services/network/devtools_durable_msg.cc
Line 32, Patchset 11 (Latest):
encoded_bytes_ = encoded_bytes_.subspan(consume);Andrey Kosyakov . unresolved
nit: [`base::span::split_at()`](https://source.chromium.org/chromium/chromium/src/+/main:base/containers/span.h;l=763) may be useful here.
Related details
Attention is currently required from:
- Adam Rice
- Alex N. Jose
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Aug 27, 2025, 8:07:49 PM (12 days ago) Aug 27
to Andrey Kosyakov, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice
Alex N. Jose added 2 comments![]( "Open in Gerrit")
File services/network/devtools_durable_msg.h
Line 34, Patchset 10:
size_t byte_size() const { return bytes_.size(); }Andrey Kosyakov . resolved
Alex N. JoseDoes this one still make sense?
It's used in tests only. Perhaps make this `_for_testing`?
Alex N. Jose
Done
File services/network/devtools_durable_msg.cc
nit: [`base::span::split_at()`](https://source.chromium.org/chromium/chromium/src/+/main:base/containers/span.h;l=763) may be useful here.
Alex N. Jose
Done
Related details
Attention is currently required from:
- Adam Rice
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Sep 2, 2025, 1:53:38 PM (6 days ago) Sep 2
to Reilly Grant, Code Review Nudger, Andrey Kosyakov, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Reilly Grant
Alex N. Jose added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Alex N. Jose . resolved
Adding an additional network service owner to spread the review load.
reillyg@, could I get your review of the network service changes in this CL?
Related details
Attention is currently required from:
- Adam Rice
- Reilly Grant
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Sep 2, 2025, 6:36:21 PM (6 days ago) Sep 2
to Alex N. Jose, Reilly Grant, Code Review Nudger, Andrey Kosyakov, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Alex N. Jose
Reilly Grant added 3 comments![]( "Open in Gerrit")
File services/network/devtools_durable_msg.h
Line 49, Patchset 15 (Latest):
std::vector<net::SourceStreamType> GetClientDecodingTypesForTesting() {Reilly Grant . unresolved
```suggestion
const std::vector<net::SourceStreamType>& GetClientDecodingTypesForTesting() {
```
File services/network/url_loader_unittest.cc
Line 3118, Patchset 15 (Latest):
int compressed_size = compressed.size();
ASSERT_EQ(accounting_delegate.size(), compressed_size);
ASSERT_EQ(durable_message.encoded_byte_size(),
static_cast<size_t>(compressed_size));
ASSERT_EQ(durable_message.byte_size_for_testing(),
static_cast<size_t>(compressed_size));Reilly Grant . unresolved
Why are conversions necessary here? `compressed.size()` is already `size_t`.
```suggestion
size_t compressed_size = compressed.size();
ASSERT_EQ(accounting_delegate.size(), compressed_size);
ASSERT_EQ(durable_message.encoded_byte_size(), compressed_size);
ASSERT_EQ(durable_message.byte_size_for_testing(), compressed_size);
```
Line 3144, Patchset 15 (Latest):
ASSERT_EQ(accounting_delegate.size(),
static_cast<int64_t>(kGzippedBodyLength));Reilly Grant . unresolved
Why are conversions necessary here? Both values are already `size_t`.
```suggestion
ASSERT_EQ(accounting_delegate.size(), kGzippedBodyLength);
```
Related details
Attention is currently required from:
- Adam Rice
- Alex N. Jose
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Sep 2, 2025, 7:34:22 PM (6 days ago) Sep 2
to Reilly Grant, Code Review Nudger, Andrey Kosyakov, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Reilly Grant
Alex N. Jose added 3 comments![]( "Open in Gerrit")
File services/network/devtools_durable_msg.h
Line 49, Patchset 15:
std::vector<net::SourceStreamType> GetClientDecodingTypesForTesting() {Reilly Grant . resolved
```suggestion
const std::vector<net::SourceStreamType>& GetClientDecodingTypesForTesting() {
```
Alex N. Jose
Done
File services/network/url_loader_unittest.cc
Line 3118, Patchset 15:
int compressed_size = compressed.size();
ASSERT_EQ(accounting_delegate.size(), compressed_size);
ASSERT_EQ(durable_message.encoded_byte_size(),
static_cast<size_t>(compressed_size));
ASSERT_EQ(durable_message.byte_size_for_testing(),
static_cast<size_t>(compressed_size));Reilly Grant . resolved
Why are conversions necessary here? `compressed.size()` is already `size_t`.
```suggestion
size_t compressed_size = compressed.size();
ASSERT_EQ(accounting_delegate.size(), compressed_size);
ASSERT_EQ(durable_message.encoded_byte_size(), compressed_size);
ASSERT_EQ(durable_message.byte_size_for_testing(), compressed_size);
```
Alex N. Jose
I've switched the cast to `accounting_delegate.size()` given the majority of comparisons operate over `size_t`.
Line 3144, Patchset 15:
ASSERT_EQ(accounting_delegate.size(),
static_cast<int64_t>(kGzippedBodyLength));Reilly Grant . unresolved
Why are conversions necessary here? Both values are already `size_t`.
```suggestion
ASSERT_EQ(accounting_delegate.size(), kGzippedBodyLength);
```
Attention is currently required from:
- Adam Rice
- Reilly Grant
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Sep 2, 2025, 7:59:17 PM (6 days ago) Sep 2
to Reilly Grant, Code Review Nudger, Andrey Kosyakov, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Reilly Grant
Alex N. Jose added 1 comment![]( "Open in Gerrit")
File services/network/url_loader_unittest.cc
Line 3144, Patchset 15:
ASSERT_EQ(accounting_delegate.size(),
static_cast<int64_t>(kGzippedBodyLength));Reilly Grant . unresolved
Alex N. JoseWhy are conversions necessary here? Both values are already `size_t`.
```suggestion
ASSERT_EQ(accounting_delegate.size(), kGzippedBodyLength);
```
`accounting_delegate.size()` is `int64_t`.
Alex N. Jose
For additional context, `MockDurableMessageAccountingDelegate` keeps the size counter variable as `int64_t`, so it can effectively catch potential accounting errors.
Reilly Grant (Gerrit)
Sep 2, 2025, 8:02:54 PM (6 days ago) Sep 2
to Alex N. Jose, Reilly Grant, Code Review Nudger, Andrey Kosyakov, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice and Alex N. Jose
Reilly Grant voted and added 2 comments![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +1 |
2 comments
Patchset-level comments
File services/network/url_loader_unittest.cc
Line 3144, Patchset 15:
ASSERT_EQ(accounting_delegate.size(),
static_cast<int64_t>(kGzippedBodyLength));Reilly Grant . resolved
Alex N. JoseWhy are conversions necessary here? Both values are already `size_t`.
```suggestion
ASSERT_EQ(accounting_delegate.size(), kGzippedBodyLength);
```
`accounting_delegate.size()` is `int64_t`.
Attention is currently required from:
- Adam Rice
- Alex N. Jose
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Sep 3, 2025, 5:36:14 PM (5 days ago) Sep 3
to Reilly Grant, Code Review Nudger, Andrey Kosyakov, Adam Rice, Chromium LUCI CQ, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Adam Rice
Alex N. Jose voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Adam Rice
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Sep 3, 2025, 5:40:56 PM (5 days ago) Sep 3
to Alex N. Jose, Reilly Grant, Code Review Nudger, Andrey Kosyakov, Adam Rice, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
16 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/http/tests/inspector-protocol/network/get-durable-response-gzip-body.js
Insertions: 1, Deletions: 3.
@@ -9,7 +9,7 @@
session.evaluate(`fetch("${resourceUrl}").then(r => r.text());`);
const requestWillBeSent = (await dp.Network.onceRequestWillBeSent()).params;
testRunner.log(`Request for ${requestWillBeSent.request.url}`);
- await dp.Network.onceResponseReceived();
+ await dp.Network.onceLoadingFinished();
const resourceRequestId = requestWillBeSent.requestId;
testRunner.log('-- Test Page.navigate() to a cross origin URL --');
@@ -20,5 +20,3 @@
testRunner.completeTest();
})
-
-
```
Change information
Commit message:
Decode collected Durable Messages on retrieval
This is a follow up CL to crrev.com/c/6876126.
This CL implements DurableMessageEncodedSourceStream to decode response
bodies on retrieval, if they were marked for client side decoding by
the network service.
Bug: 414864477, 440170590
Change-Id: Ib370e6ccad9d715ac6863deaa7cdc3dd61b0f81d
Reviewed-by: Andrey Kosyakov <ca...@chromium.org>
Commit-Queue: Alex N. Jose <ale...@chromium.org>
Reviewed-by: Reilly Grant <rei...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1510547}
Files:
- M services/network/devtools_durable_msg.cc
- M services/network/devtools_durable_msg.h
- M services/network/devtools_durable_msg_collector.cc
- M services/network/devtools_durable_msg_collector_unittest.cc
- M services/network/url_loader.cc
- M services/network/url_loader_unittest.cc
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/get-durable-response-gzip-body-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/get-durable-response-gzip-body.js
Change size: M
Delta: 8 files changed, 189 insertions(+), 37 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Andrey Kosyakov, +1 by Reilly Grant
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Sep 5, 2025, 3:27:46 AM (3 days ago) Sep 5
to Chromium LUCI CQ, Alex N. Jose, Reilly Grant, Code Review Nudger, Andrey Kosyakov, AyeAye Python Dispatcher, chromium...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, network-ser...@chromium.org
Attention needed from Alex N. Jose
Adam Rice voted and added 7 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
7 comments
Patchset-level comments
File services/network/devtools_durable_msg.h
Line 44, Patchset 18 (Latest):
void SetClientDecodingTypes(std::vector<net::SourceStreamType> types) {Adam Rice . unresolved
Nit: `set_client_decoding_types` would be a more natural name for this, as it is trivial and inline.
Line 16, Patchset 18 (Latest):
#include "mojo/public/cpp/base/big_buffer.h"Adam Rice . unresolved
Nit: you could predeclare `mojo_base::BigBuffer` as it is only used as a return type.
File services/network/devtools_durable_msg.cc
Line 27, Patchset 18 (Latest):
if (consume <= 0) {Adam Rice . unresolved
Nit: `consume` is a `size_t`. It cannot be < 0, so this condition should be `(consume == 0)`.
Line 31, Patchset 18 (Latest):
auto split_buffer = encoded_bytes_.split_at(consume);
dest_buffer->span().copy_prefix_from(split_buffer.first);
encoded_bytes_ = split_buffer.second;Adam Rice . unresolved
I think it's always good to use structured binding with `split_at`.
```suggestion
auto [used_portion, unused_portion] = encoded_bytes_.split_at(consume);
dest_buffer->span().copy_prefix_from(used_portion);
encoded_bytes_ = unused_portion;
```
Line 98, Patchset 18 (Latest):
break;Adam Rice . unresolved
You should add
```
CHECK_NE(result, ERR_IO_PENDING);
```
so that if someone makes a `FilterSourceStream` that decompresses asynchronously (on another thread, for example), then it will crash noisily.
File services/network/url_loader_unittest.cc
Line 3131, Patchset 18 (Latest):
size_t kGzippedBodyLength = 60;Adam Rice . unresolved
Constants should be `static constexpr`.
Related details
Attention is currently required from:
- Alex N. Jose
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages