Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2937233002:
Use HitTestAggregator in GpuRootCompositorFrameSink (Closed)
DescriptionUse HitTestAggregator in GpuRootCompositorFrameSink
GpuRootCompositorFrameSink creates an instance of HitTestAggregator
and provides the notifications required when surfaces are added to the
next CompositorFrame, destroyed and when the next frame begins.
BUG=732399
Review-Url: https://codereview.chromium.org/2937233002
Cr-Commit-Position: refs/heads/master@{#487495}
Committed: https://chromium.googlesource.com/chromium/src/+/4a4a734896cc777dd9fb69675e21dbd7c2886f30
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase and move switches from base to common #
Total comments: 4
Patch Set 3 : rebase #Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : remove conditional use of HitTestAggregator #
Total comments: 4
Patch Set 6 : remove unnecessary define and bracing #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== Use HitTestAggregator in GpuRootCompositorFrameSink GpuRootCompositorFrameSink creates and instance of HitTestAggregator and provide the notifications required when surfaces are added to the next CompositorFrame, destroyed and when the next frame begins. A command line flag --use-viz-hit-test is used to ensure this component is not used until the full feature is complete. BUG=732399 ========== to ========== Use HitTestAggregator in GpuRootCompositorFrameSink GpuRootCompositorFrameSink creates and instance of HitTestAggregator and provide the notifications required when surfaces are added to the next CompositorFrame, destroyed and when the next frame begins. A command line flag --use-viz-hit-test is used to ensure this component is not used until the full feature is complete. BUG=732399 ==========
gklassen@chromium.org changed reviewers: + fsamuel@google.com, rjkroege@chromium.org
gklassen@chromium.org changed reviewers: - fsamuel@google.com
This is the third of three CLs from the original WIP CL. Should Fady and Sadrul be added as reviewers for this one?
rjkroege@chromium.org changed reviewers: + fsamuel@chromium.org
lgtm
I think you'll need to rebase. https://codereview.chromium.org/2937233002/diff/1/components/viz/base/switches.h File components/viz/base/switches.h (right): https://codereview.chromium.org/2937233002/diff/1/components/viz/base/switche... components/viz/base/switches.h:15: VIZ_BASE_EXPORT extern const char kUseVizHitTest[]; We kind of decided to move switches to common... https://codereview.chromium.org/2937233002/diff/1/components/viz/frame_sinks/... File components/viz/frame_sinks/BUILD.gn (left): https://codereview.chromium.org/2937233002/diff/1/components/viz/frame_sinks/... components/viz/frame_sinks/BUILD.gn:37: "//components/viz/display_compositor:display_compositor_sources", Please rebase. https://codereview.chromium.org/2937233002/diff/1/components/viz/frame_sinks/... File components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc (right): https://codereview.chromium.org/2937233002/diff/1/components/viz/frame_sinks/... components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc:53: if (base::CommandLine::ForCurrentProcess()->HasSwitch( We are about to introduce a bunch of flags. I suggest we create a FrameSinkManagerSettings object that is populated prior to creation.
new patch uploaded with improvements based on reviewer comments. Can you send me a pointer to how to setup switches and I will happily implement. https://codereview.chromium.org/2937233002/diff/1/components/viz/base/switches.h File components/viz/base/switches.h (right): https://codereview.chromium.org/2937233002/diff/1/components/viz/base/switche... components/viz/base/switches.h:15: VIZ_BASE_EXPORT extern const char kUseVizHitTest[]; On 2017/06/15 15:02:40, Fady Samuel wrote: > We kind of decided to move switches to common... Done. https://codereview.chromium.org/2937233002/diff/1/components/viz/frame_sinks/... File components/viz/frame_sinks/BUILD.gn (left): https://codereview.chromium.org/2937233002/diff/1/components/viz/frame_sinks/... components/viz/frame_sinks/BUILD.gn:37: "//components/viz/display_compositor:display_compositor_sources", On 2017/06/15 15:02:40, Fady Samuel wrote: > Please rebase. Done. https://codereview.chromium.org/2937233002/diff/1/components/viz/frame_sinks/... File components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc (right): https://codereview.chromium.org/2937233002/diff/1/components/viz/frame_sinks/... components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc:53: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/06/15 15:02:40, Fady Samuel wrote: > We are about to introduce a bunch of flags. I suggest we create a > FrameSinkManagerSettings object that is populated prior to creation. Ok. Can you send me a pointer to a pattern to follow and I will happily set this up.
https://codereview.chromium.org/2937233002/diff/20001/components/viz/frame_si... File components/viz/frame_sinks/BUILD.gn (right): https://codereview.chromium.org/2937233002/diff/20001/components/viz/frame_si... components/viz/frame_sinks/BUILD.gn:7: public_deps = [ This change is unnecessary? https://codereview.chromium.org/2937233002/diff/20001/components/viz/service/... File components/viz/service/BUILD.gn (right): https://codereview.chromium.org/2937233002/diff/20001/components/viz/service/... components/viz/service/BUILD.gn:53: "//components/viz/hit_test", What's components/viz/hit_test?
The most recent patch includes rebase for recent changes. Can you please re-review? I haven't added a settings class mostly because this change doesn't seem like it could make use of a settings class well and so perhaps its better to introduce a settings class as part of another CL or a CL dedicated for that purpose. https://codereview.chromium.org/2937233002/diff/20001/components/viz/frame_si... File components/viz/frame_sinks/BUILD.gn (right): https://codereview.chromium.org/2937233002/diff/20001/components/viz/frame_si... components/viz/frame_sinks/BUILD.gn:7: public_deps = [ On 2017/06/20 17:18:39, Fady Samuel wrote: > This change is unnecessary? Done. https://codereview.chromium.org/2937233002/diff/20001/components/viz/service/... File components/viz/service/BUILD.gn (right): https://codereview.chromium.org/2937233002/diff/20001/components/viz/service/... components/viz/service/BUILD.gn:53: "//components/viz/hit_test", On 2017/06/20 17:18:39, Fady Samuel wrote: > What's components/viz/hit_test? Done.
https://codereview.chromium.org/2937233002/diff/60001/components/viz/service/... File components/viz/service/frame_sinks/gpu_root_compositor_frame_sink.cc (right): https://codereview.chromium.org/2937233002/diff/60001/components/viz/service/... components/viz/service/frame_sinks/gpu_root_compositor_frame_sink.cc:56: hit_test_aggregator_ = base::MakeUnique<HitTestAggregator>(); Do we need a switch at all? Any reason not to always have this created?
Ok. After discussion with Rob and Fady the switch has been removed. https://codereview.chromium.org/2937233002/diff/60001/components/viz/service/... File components/viz/service/frame_sinks/gpu_root_compositor_frame_sink.cc (right): https://codereview.chromium.org/2937233002/diff/60001/components/viz/service/... components/viz/service/frame_sinks/gpu_root_compositor_frame_sink.cc:56: hit_test_aggregator_ = base::MakeUnique<HitTestAggregator>(); On 2017/07/17 17:08:24, Fady Samuel wrote: > Do we need a switch at all? Any reason not to always have this created? It was included to keep runtime cost minimal when this feature is not in use and to provide a single place to control it's use - but after discussion with you and with Rob the conclusion seems to be that it's ok to use directly here and an appropriate switch will be added elsewhere when required. Done.
https://codereview.chromium.org/2937233002/diff/80001/components/viz/common/B... File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2937233002/diff/80001/components/viz/common/B... components/viz/common/BUILD.gn:9: defines = [ "VIZ_COMMON_IMPLEMENTATION" ] Undo? https://codereview.chromium.org/2937233002/diff/80001/components/viz/service/... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2937233002/diff/80001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.cc:136: if (search == active_.end()) { nit: no need for braces.
Thanks. Both comments should be fixed in the latest patch. https://codereview.chromium.org/2937233002/diff/80001/components/viz/common/B... File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2937233002/diff/80001/components/viz/common/B... components/viz/common/BUILD.gn:9: defines = [ "VIZ_COMMON_IMPLEMENTATION" ] On 2017/07/18 13:48:59, Fady Samuel wrote: > Undo? ( Thanks, I had undone this but I guess I hadn't uploaded yet. ) Done. https://codereview.chromium.org/2937233002/diff/80001/components/viz/service/... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2937233002/diff/80001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.cc:136: if (search == active_.end()) { On 2017/07/18 13:48:59, Fady Samuel wrote: > nit: no need for braces. Done.
lgtm + update the description. Thanks!
Description was changed from ========== Use HitTestAggregator in GpuRootCompositorFrameSink GpuRootCompositorFrameSink creates and instance of HitTestAggregator and provide the notifications required when surfaces are added to the next CompositorFrame, destroyed and when the next frame begins. A command line flag --use-viz-hit-test is used to ensure this component is not used until the full feature is complete. BUG=732399 ========== to ========== Use HitTestAggregator in GpuRootCompositorFrameSink GpuRootCompositorFrameSink creates an instance of HitTestAggregator and provides the notifications required when surfaces are added to the next CompositorFrame, destroyed and when the next frame begins. BUG=732399 ==========
The CQ bit was checked by gklassen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rjkroege@chromium.org Link to the patchset: https://codereview.chromium.org/2937233002/#ps100001 (title: "remove unnecessary define and bracing")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500390428375580,
"parent_rev": "1342f6d5c7b7d92942a4c88dac8c9d81a9fcc07c", "commit_rev":
"4a4a734896cc777dd9fb69675e21dbd7c2886f30"}
Message was sent while issue was closed.
Description was changed from ========== Use HitTestAggregator in GpuRootCompositorFrameSink GpuRootCompositorFrameSink creates an instance of HitTestAggregator and provides the notifications required when surfaces are added to the next CompositorFrame, destroyed and when the next frame begins. BUG=732399 ========== to ========== Use HitTestAggregator in GpuRootCompositorFrameSink GpuRootCompositorFrameSink creates an instance of HitTestAggregator and provides the notifications required when surfaces are added to the next CompositorFrame, destroyed and when the next frame begins. BUG=732399 Review-Url: https://codereview.chromium.org/2937233002 Cr-Commit-Position: refs/heads/master@{#487495} Committed: https://chromium.googlesource.com/chromium/src/+/4a4a734896cc777dd9fb69675e21... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4a4a734896cc777dd9fb69675e21... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
