[ComputePressure] Make ComputePressure as a service. [chromium/src : main]
Wei4 Wang (Gerrit)
Attention is currently required from: Joshua Bell.
Wei4 Wang removed James Maclean from this change.
[ComputePressure] Make ComputePressure as a service.
The patch makes ComputePressure as a service and the major
changes are:
1. Move ComputePressureManager and related classes from
content/browser to services/device. Rename ComputePressureManager
to ComputePressureManagerImpl.
2. Rename ComputePressureHost to ComputePressureServiceImpl.
3. ComputePressureSample data are collected in service now and then
send to ComputePressureServiceImpl. If the data changes and meets
frequency requirement, ComputePressureServiceImpl will send the
data to Blink.
4. CpuProbe collects data only when there is active
ComputePressureObserver. It stops collecting when all
ComputePressureObservers become inactive.
The compute_pressure_different_quantizations_across_iframes test
fails in this patch. However, there is no concept of the quantization
in the newest spec any more and we will remove this test in the
future.
Bug: 1205695, 1311945
Change-Id: I388ce2ea3d7be2e717c080d92c77b8d0b3570f03
---
M content/browser/BUILD.gn
M content/browser/browser_interface_binders.cc
M content/browser/compute_pressure/OWNERS
M content/browser/compute_pressure/README.md
D content/browser/compute_pressure/compute_pressure_host.cc
D content/browser/compute_pressure/compute_pressure_host.h
D content/browser/compute_pressure/compute_pressure_host_unittest.cc
D content/browser/compute_pressure/compute_pressure_manager.cc
D content/browser/compute_pressure/compute_pressure_manager.h
D content/browser/compute_pressure/compute_pressure_manager_unittest.cc
M content/browser/compute_pressure/compute_pressure_quantizer.cc
M content/browser/compute_pressure/compute_pressure_quantizer.h
M content/browser/compute_pressure/compute_pressure_quantizer_unittest.cc
A content/browser/compute_pressure/compute_pressure_service_impl.cc
A content/browser/compute_pressure/compute_pressure_service_impl.h
A content/browser/compute_pressure/compute_pressure_service_impl_unittest.cc
D content/browser/compute_pressure/compute_pressure_test_support.cc
D content/browser/compute_pressure/compute_pressure_test_support.h
M content/browser/renderer_host/render_frame_host_impl.cc
M content/browser/renderer_host/render_frame_host_impl.h
M content/browser/storage_partition_impl.cc
M content/browser/storage_partition_impl.h
M content/test/BUILD.gn
M services/device/BUILD.gn
M services/device/binder_overrides.cc
M services/device/binder_overrides.h
A services/device/compute_pressure/BUILD.gn
A services/device/compute_pressure/COMMON_METADATA
A services/device/compute_pressure/DEPS
A services/device/compute_pressure/DIR_METADATA
A services/device/compute_pressure/OWNERS
A services/device/compute_pressure/compute_pressure_manager_impl.cc
A services/device/compute_pressure/compute_pressure_manager_impl.h
A services/device/compute_pressure/compute_pressure_manager_impl_unittest.cc
R services/device/compute_pressure/compute_pressure_sample.h
R services/device/compute_pressure/compute_pressure_sampler.cc
R services/device/compute_pressure/compute_pressure_sampler.h
R services/device/compute_pressure/compute_pressure_sampler_unittest.cc
A services/device/compute_pressure/compute_pressure_test_support.cc
A services/device/compute_pressure/compute_pressure_test_support.h
R services/device/compute_pressure/cpu_core_speed_info.cc
R services/device/compute_pressure/cpu_core_speed_info.h
R services/device/compute_pressure/cpu_core_speed_info_unittest.cc
R services/device/compute_pressure/cpu_probe.cc
R services/device/compute_pressure/cpu_probe.h
R services/device/compute_pressure/cpu_probe_linux.cc
R services/device/compute_pressure/cpu_probe_linux.h
R services/device/compute_pressure/cpu_probe_linux_unittest.cc
R services/device/compute_pressure/cpuid_base_frequency_parser.cc
R services/device/compute_pressure/cpuid_base_frequency_parser.h
R services/device/compute_pressure/cpuid_base_frequency_parser_unittest.cc
R services/device/compute_pressure/procfs_stat_cpu_parser.cc
R services/device/compute_pressure/procfs_stat_cpu_parser.h
R services/device/compute_pressure/procfs_stat_cpu_parser_unittest.cc
R services/device/compute_pressure/sysfs_cpufreq_core_parser.cc
R services/device/compute_pressure/sysfs_cpufreq_core_parser.h
R services/device/compute_pressure/sysfs_cpufreq_core_parser_unittest.cc
M services/device/device_service.cc
M services/device/device_service.h
M services/device/public/cpp/BUILD.gn
A services/device/public/cpp/test/scoped_compute_pressure_manager_overrider.cc
A services/device/public/cpp/test/scoped_compute_pressure_manager_overrider.h
M services/device/public/mojom/BUILD.gn
A services/device/public/mojom/compute_pressure_manager.mojom
A services/device/public/mojom/compute_pressure_state.mojom
M services/device/public/mojom/device_service.mojom
M third_party/blink/public/mojom/compute_pressure/compute_pressure.mojom
M third_party/blink/renderer/modules/compute_pressure/DIR_METADATA
M third_party/blink/renderer/modules/compute_pressure/compute_pressure_observer.cc
M third_party/blink/renderer/modules/compute_pressure/compute_pressure_observer.h
M third_party/blink/web_tests/external/wpt/compute-pressure/compute_pressure_different_quantizations.tentative.https.window.js
A third_party/blink/web_tests/platform/generic/external/wpt/compute-pressure/compute_pressure_different_quantizations_across_iframes.tentative.https.window-expected.txt
72 files changed, 1,903 insertions(+), 1,803 deletions(-)
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Joshua Bell.
Joshua Bell (Gerrit)
Attention is currently required from: Reilly Grant, Wei4 Wang.
Joshua Bell would like Reilly Grant to review this change authored by Wei4 Wang.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Bell (Gerrit)
Attention is currently required from: Reilly Grant, Wei4 Wang.
4 comments:
Patchset:
reillyg@ is probably a better reviewer for the servicification than me. I'll continue taking a look, time permitting. And sorry for the delay, I was OOO but apparently failed to set a message. I noticed a couple things...
File content/browser/compute_pressure/compute_pressure_service_impl.h:
Patch Set #23, Line 63:
double timestamp) override;Why was a double used for the timestamp instead of the previous base::Time?
This appears to make all of the callers do the conversion, and makes the API less precise about what the parameter means.
Patch Set #23, Line 103:
double last_reported_timestamp_ GUARDED_BY_CONTEXT(sequence_checker_);Again, why double instead of base::Time?
File content/browser/compute_pressure/compute_pressure_service_impl.cc:
Patch Set #23, Line 131:
// TODO(pwnall): Rate-limit observers in non-visible frames instead ofpwnall is no longer actively committing to chromium - update with a better owner?
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Joshua Bell, Reilly Grant.
2 comments:
File content/browser/compute_pressure/compute_pressure_service_impl.h:
Patch Set #23, Line 63:
double timestamp) override;
Why was a double used for the timestamp instead of the previous base::Time? […]
Thanks, becasue the data collecting is moved to service, the timestampe is also generated in service. We can't pass the base:Time value in mojom, so we use double here.
File content/browser/compute_pressure/compute_pressure_service_impl.cc:
Patch Set #23, Line 131:
// TODO(pwnall): Rate-limit observers in non-visible frames instead ofpwnall is no longer actively committing to chromium - update with a better owner?
Ack
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Reilly Grant (Gerrit)
Attention is currently required from: Joshua Bell, Wei4 Wang.
5 comments:
File content/browser/compute_pressure/compute_pressure_service_impl.h:
Patch Set #23, Line 63:
double timestamp) override;
Thanks, becasue the data collecting is moved to service, the timestampe is also generated in service […]
`mojo/public/mojom/base/time.mojom` provides Mojo types for `base::Time` and `base::TimeDelta`.
File services/device/compute_pressure/BUILD.gn:
2022
File third_party/blink/public/mojom/compute_pressure/compute_pressure.mojom:
Patch Set #23, Line 79:
// Origin-scoped access to the browser-side Compute Pressure API implementation.Why remove this comment?
File third_party/blink/renderer/modules/compute_pressure/compute_pressure_observer.cc:
Patch Set #23, Line 142:
ComputePressureServiceImplThis is leaking unnecessary implementation details. A "NotSupportedError" with a message like "Compute pressure is not available." is more appropriate.
File third_party/blink/web_tests/external/wpt/compute-pressure/compute_pressure_different_quantizations.tentative.https.window.js:
These changes to the tests seem unrelated.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Joshua Bell, Reilly Grant.
5 comments:
File content/browser/compute_pressure/compute_pressure_service_impl.h:
Patch Set #23, Line 63:
double timestamp) override;
`mojo/public/mojom/base/time.mojom` provides Mojo types for `base::Time` and `base::TimeDelta`.
Thanks, I will check it.
File services/device/compute_pressure/BUILD.gn:
2022
Ack
File third_party/blink/public/mojom/compute_pressure/compute_pressure.mojom:
Patch Set #23, Line 79:
// Origin-scoped access to the browser-side Compute Pressure API implementation.Why remove this comment?
Before this change, there is one ComputePressureHost per Origin, so it is origin-scoped. But in this patch, RenderFrameHostImpl owns an instance of this class and it is not Origin-scoped. So I remove the comment. However, this is a transition patch and we will have a class named ComputePressureObserverManager in blink to deal with the origin problem later.
File third_party/blink/renderer/modules/compute_pressure/compute_pressure_observer.cc:
Patch Set #23, Line 142:
ComputePressureServiceImpl
This is leaking unnecessary implementation details. […]
Ack
File third_party/blink/web_tests/external/wpt/compute-pressure/compute_pressure_different_quantizations.tentative.https.window.js:
These changes to the tests seem unrelated.
The quantization scheme is per frame now, not per origin.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Patchset:
Comments are resolved, PTAL, thanks.
File content/browser/compute_pressure/compute_pressure_service_impl.h:
Patch Set #23, Line 63:
double timestamp) override;
Thanks, I will check it.
Done
Patch Set #23, Line 103:
double last_reported_timestamp_ GUARDED_BY_CONTEXT(sequence_checker_);Again, why double instead of base::Time?
Done
File content/browser/compute_pressure/compute_pressure_service_impl.cc:
Patch Set #18, Line 49:
remote_.set_disconnect_handler(I move the binding of remote_ from constructor to AddObserver(), so it supports reconnection now. […]
Done
File content/browser/compute_pressure/compute_pressure_service_impl.cc:
Patch Set #19, Line 173:
receivers_.Clear();Sorry, I am careless. I meant to clear observers_.
Done
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Reilly Grant (Gerrit)
Attention is currently required from: Joshua Bell, Wei4 Wang.
Patch set 25:Code-Review +1
2 comments:
Patchset:
LGTM
File services/device/compute_pressure/compute_pressure_manager_impl_unittest.cc:
bool result;
base::RunLoop run_loop;
manager_.AddClient(std::move(client),
base::BindLambdaForTesting([&](bool success) {
result = success;
run_loop.Quit();
}));
run_loop.Run();```
base::test::TestFuture<bool> future;
manager_.AddClient(std::move(client), future.GetCallback());
return future.Get();
```
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Joshua Bell, Mustafa Emre Acer, Robert Kroeger.
Wei4 Wang would like Mustafa Emre Acer and Robert Kroeger to review this change.
M third_party/blink/web_tests/external/wpt/compute-pressure/compute_pressure_detached_iframe.tentative.https.html
M third_party/blink/web_tests/external/wpt/compute-pressure/compute_pressure_different_quantizations.tentative.https.window.js
A third_party/blink/web_tests/platform/generic/external/wpt/compute-pressure/compute_pressure_different_quantizations_across_iframes.tentative.https.window-expected.txt
73 files changed, 1,876 insertions(+), 1,804 deletions(-)
Wei4 Wang (Gerrit)
Attention is currently required from: Joshua Bell, Mustafa Emre Acer, Robert Kroeger.
1 comment:
Patchset:
Please help review the patch, thanks.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Bell (Gerrit)
Attention is currently required from: Mustafa Emre Acer, Robert Kroeger, Wei4 Wang.
1 comment:
Patchset:
apologies for the delay - I'll have cycles to review this tomorrow.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Bell (Gerrit)
Attention is currently required from: Mustafa Emre Acer, Robert Kroeger, Wei4 Wang.
Patch set 26:Code-Review +1
5 comments:
Patchset:
LGTM with some minor things...
File content/browser/compute_pressure/README.md:
Patch Set #26, Line 10:
`blink::mojom::ComputePressureService`, defined in Blink, is the interface betweenWord wrap to 80 columns?
File content/browser/compute_pressure/compute_pressure_service_impl.cc:
Patch Set #26, Line 202:
last_reported_state_ = {/* cpu_utilization */ -1, /* cpu_speed */ -1};nit: preferred comment style is now: /*cpu_utilization=*/
File services/device/compute_pressure/OWNERS:
Patch Set #26, Line 5:
jsb...@chromium.orgWDYT about adding reillyg@? He's gonna be a much better reviewer than I am going forward.
File services/device/compute_pressure/cpu_probe_linux.cc:
Patch Set #26, Line 30:
#include "third_party/re2/src/re2/re2.h"Is this include actually needed in this file?
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Mustafa Emre Acer, Robert Kroeger.
8 comments:
Patchset:
Thanks, jsbell@. Comments are resolved, PTAL.
Can you help find reviewers for the following files:
content/browser/browser_interface_binders.cc
content/browser/renderer_host/render_frame_host_impl.h
content/browser/renderer_host/render_frame_host_impl.cc
services/device/public/mojom/compute_pressure_manager.mojom
services/device/public/mojom/compute_pressure_state.mojom
services/device/public/mojom/device_service.mojom
third_party/blink/public/mojom/compute_pressure/compute_pressure.mojomIt seems rjkroege@ and meacer@ has no response on this.
File content/browser/compute_pressure/README.md:
Patch Set #26, Line 10:
`blink::mojom::ComputePressureService`, defined in Blink, is the interface betweenWord wrap to 80 columns?
Done
File content/browser/compute_pressure/compute_pressure_service_impl.cc:
Patch Set #26, Line 202:
last_reported_state_ = {/* cpu_utilization */ -1, /* cpu_speed */ -1};nit: preferred comment style is now: /*cpu_utilization=*/
Done
File services/device/compute_pressure/OWNERS:
Patch Set #26, Line 5:
jsb...@chromium.orgWDYT about adding reillyg@? He's gonna be a much better reviewer than I am going forward.
Done
File services/device/compute_pressure/compute_pressure_manager_impl_unittest.cc:
bool result;
base::RunLoop run_loop;
manager_.AddClient(std::move(client),
base::BindLambdaForTesting([&](bool success) {
result = success;
run_loop.Quit();
}));
run_loop.Run();
``` […]
Done
File services/device/compute_pressure/cpu_probe_linux.cc:
Patch Set #26, Line 30:
#include "third_party/re2/src/re2/re2.h"Is this include actually needed in this file?
Done
File third_party/blink/public/mojom/compute_pressure/compute_pressure.mojom:
Patch Set #23, Line 79:
// Origin-scoped access to the browser-side Compute Pressure API implementation.
Before this change, there is one ComputePressureHost per Origin, so it is origin-scoped. […]
Done
File third_party/blink/web_tests/external/wpt/compute-pressure/compute_pressure_different_quantizations.tentative.https.window.js:
The quantization scheme is per frame now, not per origin.
Done
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Raphael Kubo Da Costa (Gerrit)
Attention is currently required from: Alexander Timin, Chromium IPC Reviews, Mustafa Emre Acer, Robert Kroeger, Wei4 Wang.
Raphael Kubo Da Costa would like Alexander Timin and Chromium IPC Reviews to review this change authored by Wei4 Wang.
73 files changed, 1,886 insertions(+), 1,811 deletions(-)
Raphael Kubo Da Costa (Gerrit)
Attention is currently required from: Alexander Timin, Chromium IPC Reviews, Mustafa Emre Acer, Robert Kroeger, Wei4 Wang.
Patch set 27:Code-Review +1
1 comment:
Patchset:
I can't remove people from the Reviewers list, but I've added new people to review the pending files:
- chrome-ip...@google.com for the .mojom files
- altimin@ for content/browser/browser_interface_binders.cc and content/browser/renderer_host/*
I'm not sure if you used the "suggest owners" feature in Gerrit, but //ipc/SECURITY_OWNERS says chrome-ipc-reviews@ should be used for .mojom reviews (instead of e.g. meacer@ directly) and rjkroege@ reviews input and gesture code, which is not the case for this CL.
(also remember it's a good idea to mention which files or directories you'd like people to review when adding them to the reviewers list)
gwsq (Gerrit)
Attention is currently required from: Alexander Timin, Wei4 Wang.
Wei4 Wang has uploaded this change for review.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Alexander Timin, Wei4 Wang.
WARNING: gwsq was unable to find a reviewer who is not on vacation. As a fallback, gwsq is ignoring vacations and assigning mea...@chromium.org.
Reviewer source(s):
mea...@chromium.org is from context
Alexander Timin (Gerrit)
Attention is currently required from: Wei4 Wang.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #27, Line 3644:
std::unique_ptr<ComputePressureServiceImpl> compute_pressure_service_;Could we use DocumentService [1] instead of storing it on RenderFrameHostImpl?
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Alexander Timin.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #27, Line 3644:
std::unique_ptr<ComputePressureServiceImpl> compute_pressure_service_;
Could we use DocumentService [1] instead of storing it on RenderFrameHostImpl? […]
Thanks, There can be more than one ComputePressureObserver in one frame and they share the same ComputePressureServiceImpl. we just need to construct the ComputePressureServiceImpl for the first ComputePressureObserver and directly connect to the ComputePressureServiceImpl for the other ComputePressureObservers. So I think DocumentService is not suitable here.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Raphael Kubo Da Costa (Gerrit)
Attention is currently required from: Alexander Timin, Wei4 Wang.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #27, Line 3644:
std::unique_ptr<ComputePressureServiceImpl> compute_pressure_service_;
Thanks, There can be more than one ComputePressureObserver in one frame and they share the same Comp […]
Looking at
https://docs.google.com/document/d/1oopBPfX88thk6Ax79h_UbaoNQt416t7iNjrF38veuSg/edit?disco=AAAAPUPral4&resourcekey=0-A4afoyISzgiyd667or1Xmw maybe this is a good fit for DocumentUserData like SerialService?Tangential question: is the plan to reduce the amount of std::unique_ptr's here and migrate to Document{UserData,Service} where possible?
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Timin (Gerrit)
Attention is currently required from: Raphael Kubo Da Costa, Wei4 Wang.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #27, Line 3644:
std::unique_ptr<ComputePressureServiceImpl> compute_pressure_service_;
Looking at […]
Yeah, DocumentUserData is the right choice here, then.
We definitely want to avoid adding new members to RFHI if we can avoid it using DocumentUserData / DocumentService. Converting existing members is an aspiration rather something we are actively pursuing.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Alexander Timin, Chromium IPC Reviews, Raphael Kubo Da Costa.
Wei4 Wang would like Chromium IPC Reviews to review this change.
73 files changed, 1,860 insertions(+), 1,811 deletions(-)
Wei4 Wang (Gerrit)
Attention is currently required from: Alexander Timin, Chromium IPC Reviews, Raphael Kubo Da Costa.
2 comments:
Patchset:
PTAL, thanks.
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #27, Line 3644:
std::unique_ptr<ComputePressureServiceImpl> compute_pressure_service_;
Yeah, DocumentUserData is the right choice here, then. […]
Thanks, I have migrated the ComputePressureServiceImpl to DocumentUserData, PTAL.
gwsq (Gerrit)
Attention is currently required from: Alexander Timin, Raphael Kubo Da Costa.
Wei4 Wang has uploaded this change for review.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Alexander Timin, Raphael Kubo Da Costa.
Reviewer source(s):
mea...@chromium.org is from context
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Alexander Timin, Chromium IPC Reviews, Raphael Kubo Da Costa.
Wei4 Wang would like Chromium IPC Reviews to review this change.
Wei4 Wang removed Mustafa Emre Acer and Robert Kroeger from this change.
73 files changed, 1,854 insertions(+), 1,811 deletions(-)
Wei4 Wang (Gerrit)
Attention is currently required from: Alexander Timin, Chromium IPC Reviews, Raphael Kubo Da Costa.
1 comment:
Patchset:
PTAL, thanks.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Alexander Timin, Camille Lamy, Chromium IPC Reviews, Raphael Kubo Da Costa, Wei4 Wang.
gwsq would like Camille Lamy to review this change authored by Wei4 Wang.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Alexander Timin, Camille Lamy, Chromium IPC Reviews, Matthew Denton, Raphael Kubo Da Costa, Wei4 Wang.
gwsq would like Matthew Denton to review this change authored by Wei4 Wang.
gwsq (Gerrit)
Attention is currently required from: Alexander Timin, Camille Lamy, Matthew Denton, Raphael Kubo Da Costa, Wei4 Wang.
Wei4 Wang has uploaded this change for review.
gwsq (Gerrit)
Attention is currently required from: Alexander Timin, Camille Lamy, Matthew Denton, Raphael Kubo Da Costa, Wei4 Wang.
Shadowed: cl...@chromium.org
Reviewer source(s):
cl...@chromium.org, mpde...@chromium.org is from context
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Denton (Gerrit)
Attention is currently required from: Alexander Timin, Camille Lamy, Raphael Kubo Da Costa, Wei4 Wang.
Patch set 29:Code-Review +1
6 comments:
Patchset:
Looks pretty good!
File content/browser/compute_pressure/compute_pressure_service_impl.h:
Patch Set #29, Line 107:
weak_factory_You might not need this
File content/browser/compute_pressure/compute_pressure_service_impl.cc:
Patch Set #29, Line 32:
weak_factory_.GetWeakPtr()I think you can safely use base::Unretained(this) here if you want, see e.g. https://source.chromium.org/chromium/chromium/src/+/main:content/browser/compute_pressure/compute_pressure_host.cc;drc=917850e016efe08e27c61eaa03e6fc27200e5be1;l=42
There are a bunch of other usages here that can be base::Unretained() since Mojo guarantees it doesn't call any callbacks after Remote and RemoteSet and ReceiverSet (etc.) are deleted. |observers_| and |remote_| and |client_| are all owned by |this| so there's no danger in passing callbacks that use base::Unretained(this) to them.
Patch Set #29, Line 69:
weak_factory_.GetWeakPtr()Unretained
Patch Set #29, Line 82:
weak_factory_.GetWeakPtr()Unretained
Patch Set #29, Line 87:
weak_factory_.GetWeakPtr()Unretained
Alexander Timin (Gerrit)
Attention is currently required from: Camille Lamy, Raphael Kubo Da Costa, Wei4 Wang.
2 comments:
File content/browser/renderer_host/render_frame_host_impl.cc:
Patch Set #29, Line 10514:
void RenderFrameHostImpl::BindComputePressureService(Does it need to be a method on RFHI now? Could we turn this into a static Create/Bind method on ComputePressureServiceImpl itself?
Patch Set #29, Line 10516:
if (!base::FeatureList::IsEnabled(blink::features::kComputePressure)) {We should gating the interface into the map by the feature rather than the bind call itself.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Alexander Timin, Matthew Denton.
9 comments:
Patchset:
Comments are resolved, PTAL, thanks.
File content/browser/compute_pressure/compute_pressure_service_impl.h:
Patch Set #29, Line 107:
weak_factory_You might not need this
Done
File content/browser/compute_pressure/compute_pressure_service_impl.cc:
Patch Set #29, Line 32:
weak_factory_.GetWeakPtr()
I think you can safely use base::Unretained(this) here if you want, see e.g. https://source. […]
Done
Patch Set #29, Line 69:
weak_factory_.GetWeakPtr()Unretained
Done
Patch Set #29, Line 82:
weak_factory_.GetWeakPtr()Unretained
Done
Patch Set #29, Line 87:
weak_factory_.GetWeakPtr()Unretained
Done
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #27, Line 3644:
std::unique_ptr<ComputePressureServiceImpl> compute_pressure_service_;
Thanks, I have migrated the ComputePressureServiceImpl to DocumentUserData, PTAL.
Done
File content/browser/renderer_host/render_frame_host_impl.cc:
Patch Set #29, Line 10514:
void RenderFrameHostImpl::BindComputePressureService(
Does it need to be a method on RFHI now? Could we turn this into a static Create/Bind method on Comp […]
Done
Patch Set #29, Line 10516:
if (!base::FeatureList::IsEnabled(blink::features::kComputePressure)) {We should gating the interface into the map by the feature rather than the bind call itself.
Done
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Raphael Kubo Da Costa (Gerrit)
Attention is currently required from: Alexander Timin.
Patch set 30:Code-Review +1
1 comment:
Patchset:
altimin@, could you take another look at the changes here? They look good to go, and we've managed to reduce the amount of code in `render_frame_host_impl.*`
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Timin (Gerrit)
Attention is currently required from: Wei4 Wang.
Patchset:
render_frame_host_impl.cc lgtm, thanks!
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Wei4 Wang.
Patch set 30:Commit-Queue +2
Wei4 Wang (Gerrit)
Attention is currently required from: Nico Weber.
Wei4 Wang would like Nico Weber to review this change.
73 files changed, 1,851 insertions(+), 1,812 deletions(-)
Wei4 Wang (Gerrit)
Attention is currently required from: Nico Weber.
1 comment:
Patchset:
Hi, @thakis. I meet an error when I try to submit the CL:
You need LGTM from owners of depends-on paths in DEPS that were modified in this CL:'+third_party/re2'
Suggested missing target path OWNERS: tha...@chromium.orgCan you help add Code-Review for this CL? Thanks a lot.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Patchset:
Friendly ping. thakis@
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Patchset:
Hi, thakis@. Please help add "Code-Review +1" for '+third_party/re2'. Thanks a lot.
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Nico Weber (Gerrit)
Attention is currently required from: Wei4 Wang.
Patch set 33:Code-Review +1
1 comment:
Patchset:
re2 deps lg
To view, visit change 3661519. To unsubscribe, or for help writing mail filters, visit settings.
Wei4 Wang (Gerrit)
Attention is currently required from: Wei4 Wang.
Patch set 33:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[ComputePressure] Make ComputePressure as a service.
The patch makes ComputePressure as a service and the major
changes are:
1. Move ComputePressureManager and related classes from
content/browser to services/device. Rename ComputePressureManager
to ComputePressureManagerImpl.
2. Rename ComputePressureHost to ComputePressureServiceImpl.
3. ComputePressureSample data are collected in service now and then
send to ComputePressureServiceImpl. If the data changes and meets
frequency requirement, ComputePressureServiceImpl will send the
data to Blink.
4. CpuProbe collects data only when there is active
ComputePressureObserver. It stops collecting when all
ComputePressureObservers become inactive.
The compute_pressure_different_quantizations_across_iframes test
fails in this patch. However, there is no concept of the quantization
in the newest spec any more and we will remove this test in the
future.
Bug: 1205695, 1311945
Change-Id: I388ce2ea3d7be2e717c080d92c77b8d0b3570f03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3661519
Reviewed-by: Alexander Timin <alt...@chromium.org>
Reviewed-by: Matthew Denton <mpde...@chromium.org>
Reviewed-by: Reilly Grant <rei...@chromium.org>
Reviewed-by: Nico Weber <tha...@chromium.org>
Commit-Queue: Wei4 Wang <wei4...@intel.com>
Reviewed-by: Raphael Kubo Da Costa <raphael.ku...@intel.com>
Reviewed-by: Joshua Bell <jsb...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1025483}
73 files changed, 1,860 insertions(+), 1,812 deletions(-)
Blink W3C Test Autoroller (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/34387