Łukasz Anforowicz (Gerrit)

unread,

Jun 10, 2024, 8:25:57 PMJun 10

to Daniel Cheng, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, Nate Chapin, Hiroki Nakagawa, prerendering-reviews, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org, gavinp...@chromium.org, gavin...@chromium.org, hayato...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, lingqi...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, network-ser...@chromium.org, pdf-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, spang...@chromium.org, tburkar...@chromium.org, tracing...@chromium.org, twifka...@chromium.org, wfh+...@chromium.org

Attention needed from Daniel Cheng

Łukasz Anforowicz added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Łukasz Anforowicz . resolved

@dcheng, can you PTAL?

This is the CL that has motivated `span`-ification of `DataPipeConsumerHandle::BeginReadData` - see https://docs.google.com/document/d/1c4NKpXwpQ9MKK1SbJ4C6MvhXI8-KJZ4jq7N4VHTHJoI/edit?usp=sharing.

FWIW, I am hoping to land the CL with OO+2. WDYT about this plan? (Unlike the `BeginReadData` change, it seems slightly more difficult to incrementally `span`-ify although I guess it may be possible if the API contains 2 methods: the span-based one that forwards to the ptr+size-based one, and the ptr-sized one with `CHECK(false)` (poor man's `= 0`).

Open in Gerrit

Related details

Attention is currently required from:

Daniel Cheng

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniel Cheng (Gerrit)

unread,

Jun 11, 2024, 2:19:41 AMJun 11

to Łukasz Anforowicz, Daniel Cheng, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, Nate Chapin, Hiroki Nakagawa, prerendering-reviews, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org, gavinp...@chromium.org, gavin...@chromium.org, hayato...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, lingqi...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, network-ser...@chromium.org, pdf-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, spang...@chromium.org, tburkar...@chromium.org, tracing...@chromium.org, twifka...@chromium.org, wfh+...@chromium.org

Attention needed from Łukasz Anforowicz

Daniel Cheng voted and added 2 comments

Votes added by Daniel Cheng

Code-Review	+1
Owners-Override	+1

2 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Daniel Cheng . resolved

LGTM w/nit

File third_party/blink/renderer/platform/loader/fetch/url_loader/mojo_url_loader_client.cc

Line 171, Patchset 1 (Latest): buffered_body_.emplace(std::vector<char>(chars.begin(), chars.end()));

Daniel Cheng . unresolved

IMO, may as well use insert() then.

Open in Gerrit

Related details

Attention is currently required from:

Łukasz Anforowicz

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Łukasz Anforowicz (Gerrit)

unread,

Jun 11, 2024, 12:05:44 PMJun 11

to Daniel Cheng, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, Nate Chapin, Hiroki Nakagawa, prerendering-reviews, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org, gavinp...@chromium.org, gavin...@chromium.org, hayato...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, lingqi...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, network-ser...@chromium.org, pdf-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, spang...@chromium.org, tburkar...@chromium.org, tracing...@chromium.org, twifka...@chromium.org, wfh+...@chromium.org

Attention needed from Łukasz Anforowicz

Łukasz Anforowicz added 2 comments

Patchset-level comments

File-level comment, Patchset 1:

Łukasz Anforowicz . resolved

Thanks for the review! FYI: before proceeding with this CL, I need to first investigate b/346442782 and reland https://crrev.com/c/5598233.

File third_party/blink/renderer/platform/loader/fetch/url_loader/mojo_url_loader_client.cc

Line 171, Patchset 1: buffered_body_.emplace(std::vector<char>(chars.begin(), chars.end()));

Daniel Cheng . resolved

IMO, may as well use insert() then.

Łukasz Anforowicz

Done

Open in Gerrit

Related details

Attention is currently required from:

Łukasz Anforowicz

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Łukasz Anforowicz (Gerrit)

unread,

Jun 13, 2024, 10:00:22 AMJun 13

to Daniel Cheng, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, Nate Chapin, Hiroki Nakagawa, prerendering-reviews, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org, gavinp...@chromium.org, gavin...@chromium.org, hayato...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, lingqi...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, network-ser...@chromium.org, pdf-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, spang...@chromium.org, tburkar...@chromium.org, tracing...@chromium.org, twifka...@chromium.org, wfh+...@chromium.org

Łukasz Anforowicz voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 13, 2024, 11:35:10 AMJun 13

to Łukasz Anforowicz, Daniel Cheng, chromium...@chromium.org, devtools...@chromium.org, Nate Chapin, Hiroki Nakagawa, prerendering-reviews, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, dtraino...@chromium.org, extension...@chromium.org, gavinp...@chromium.org, gavin...@chromium.org, hayato...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, lingqi...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, network-ser...@chromium.org, pdf-r...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, spang...@chromium.org, tburkar...@chromium.org, tracing...@chromium.org, twifka...@chromium.org, wfh+...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

1 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/renderer/platform/loader/fetch/url_loader/mojo_url_loader_client.cc
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:

`span`-ification: `mojo::DataPipeDrainer::Client::OnDataAvailable`.

Bug: 40284755

Change-Id: I33d5c9123b543988589f3853f8a18548d67ee653

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5618873

Owners-Override: Daniel Cheng <dch...@chromium.org>

Reviewed-by: Daniel Cheng <dch...@chromium.org>

Commit-Queue: Łukasz Anforowicz <luk...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1314599}

Files:

M chrome/browser/preloading/prefetch/search_prefetch/search_preload_unified_browsertest.cc
M chrome/browser/preloading/prefetch/search_prefetch/streaming_search_prefetch_url_loader.cc
M chrome/browser/preloading/prefetch/search_prefetch/streaming_search_prefetch_url_loader.h
M components/pdf/browser/plugin_response_writer_unittest.cc
M components/ui_devtools/tracing_agent.cc
M content/browser/devtools/devtools_url_loader_interceptor.cc
M content/browser/devtools/request_body_collector.cc
M content/browser/download/data_url_blob_reader.cc
M content/browser/download/data_url_blob_reader.h
M content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
M content/browser/loader/navigation_early_hints_manager.cc
M content/browser/loader/object_navigation_fallback_body_loader.cc
M content/browser/loader/object_navigation_fallback_body_loader.h
M content/browser/loader/prefetch_url_loader.h
M content/browser/preloading/prefetch/prefetch_test_util_internal.cc
M content/browser/preloading/prefetch/prefetch_test_util_internal.h
M content/browser/service_worker/service_worker_installed_script_loader.h
M content/browser/tracing/tracing_controller_impl.cc
M content/browser/tracing/tracing_controller_impl.h
M content/common/service_worker/race_network_request_url_loader_client.h
M extensions/browser/blob_reader.cc
M extensions/browser/blob_reader.h
M extensions/renderer/extension_localization_throttle.cc
M mojo/public/cpp/system/data_pipe_drainer.cc
M mojo/public/cpp/system/data_pipe_drainer.h
M mojo/public/cpp/system/tests/data_pipe_drainer_unittest.cc
M services/network/public/cpp/empty_url_loader_client.cc
M services/network/public/cpp/empty_url_loader_client.h
M services/network/web_bundle/web_bundle_url_loader_factory.cc
M services/tracing/perfetto/consumer_host_unittest.cc
M services/tracing/public/cpp/perfetto/perfetto_tracing_backend.cc
M storage/browser/blob/blob_impl_unittest.cc
M storage/browser/blob/blob_storage_context_mojo_unittest.cc
M storage/browser/test/blob_test_utils.cc
M third_party/blink/renderer/core/frame/web_frame_test.cc
M third_party/blink/renderer/core/loader/resource/font_resource.cc
M third_party/blink/renderer/platform/blob/testing/fake_blob_registry.cc
M third_party/blink/renderer/platform/loader/fetch/url_loader/mojo_url_loader_client.cc

Change size: M

Delta: 38 files changed, 90 insertions(+), 82 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Daniel Cheng

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

`span`-ification: `mojo::DataPipeDrainer::Client::OnDataAvailable`. [chromium/src : main]

Łukasz Anforowicz (Gerrit)

Łukasz Anforowicz added 1 comment

Related details

Daniel Cheng (Gerrit)

Daniel Cheng voted and added 2 comments

Votes added by Daniel Cheng

2 comments

Related details

Łukasz Anforowicz (Gerrit)

Łukasz Anforowicz added 2 comments

Related details

Łukasz Anforowicz (Gerrit)

Łukasz Anforowicz voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information