Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2938953002:
Implement HitTestAggregator (Closed)
|
Created:
3 years, 6 months ago by gklassen Modified:
3 years, 5 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, droger+watchlist_chromium.org, viettrungluu+watch_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, cc-bugs_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement HitTestAggregator
Implements a HitTestAggregator component to facilitate cross process
hit testing with minimal process hops.
The HitTestAggregator maintains a list of HitTestData objects that will
be created and sent with each CompositorFrame. These are aggregated
into a single DisplayHitTest data structure that can be made available
in shared memory to facilitate hit testing with minimal process hops
for any Display.
HitTestAggregator listens as surfaces are added to the current
CompositorFrame to ensure that the list only includes information that
matches what has been rendered in the current frame.
Aggregation occurs on the BeginFrame notification.
This change includes the implementaiton of the HitTestAggregator and
unit test cases to verify the intended behaviour.
Next steps will include completion of the creation and transport of
HitTestData objects, implementation of shared memory and using this
component to target incoming events.
BUG=732399
Review-Url: https://codereview.chromium.org/2938953002
Cr-Commit-Position: refs/heads/master@{#486852}
Committed: https://chromium.googlesource.com/chromium/src/+/57cb4a50cf2c5e3a2c7015a4d335aa894c982338
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 40
Patch Set 3 : reorganize files to conform to agreed structure #Patch Set 4 : improvents based on reviewer comments ( partial ) #
Total comments: 60
Patch Set 5 : Re-implement bounds to clean up tree nicely #Patch Set 6 : improvements based on reviewer comments #
Total comments: 40
Patch Set 7 : Use two distinct buffers instead of one and remove in-place synchronization. #
Total comments: 21
Patch Set 8 : Use Shared Memory handles and mappings to manage buffers & other improvements based on reviewer com… #
Total comments: 31
Patch Set 9 : flatten directory structure & other improvements based on reviewer comments #Patch Set 10 : Keep a reference to mapped buffers & add a unit test for missing child #
Total comments: 8
Patch Set 11 : Rename HitTestData to HitTestRegionList and improve array check #
Total comments: 2
Patch Set 12 : Remove non-committers from OWNERS and sharpend includes in DEPS #
Total comments: 14
Patch Set 13 : address nits #Patch Set 14 : remove spaces after brackets in coments #
Total comments: 14
Patch Set 15 : rebase & improvements based on comments #
Total comments: 2
Patch Set 16 : correct active count logic and add unit test to protect against similar errors #
Total comments: 17
Patch Set 17 : sharpen dependencies and improve comments based on review #Patch Set 18 : rename DisplayHitTestRegion to AggregatedHitTestRegion #
Total comments: 1
Patch Set 19 : rebase #Patch Set 20 : remove DEPS made unnecessary by rebase #Patch Set 21 : make destructor default #Patch Set 22 : add transform include to resolve build bot failure #
Total comments: 51
Patch Set 23 : improvements based on reviewer comments #Patch Set 24 : move common hit_test code into hit_test folder #
Total comments: 8
Patch Set 25 : rebase and improvements based on reviewer comments #Patch Set 26 : remove unnecessary include added at rebase #
Total comments: 4
Patch Set 27 : correct mojom include directory #Patch Set 28 : correct mojom include directory and improvements based on reviewer comments #Messages
Total messages: 88 (36 generated)
Description was changed from ========== Implement HitTestAggregator Implements a HitTestAggregator component to facilitate cross process hit testing with minimal process hops. The HitTestAggregator maintains a list of HitTestData objects that will be created and sent with each CompositorFrame. These are aggregated into a single DisplayHitTest data structure that can be made available in shared memory to facilitate hit testing with minimal process hops for any Display. HitTestAggregator listens as surfaces are added to the current CompositorFrame to ensure that the list only includes information that matches what has been rendered in the current frame. Aggregation occurs on the BeginFrame notification. This change includes the implementaiton of the HitTestAggregator and unit test cases to verify the intended behaviour. Next steps will include completion of the creation and transport of HitTestData objects, implementation of shared memory and using this component to target incoming events. Bug:732399 ========== to ========== Implement HitTestAggregator Implements a HitTestAggregator component to facilitate cross process hit testing with minimal process hops. The HitTestAggregator maintains a list of HitTestData objects that will be created and sent with each CompositorFrame. These are aggregated into a single DisplayHitTest data structure that can be made available in shared memory to facilitate hit testing with minimal process hops for any Display. HitTestAggregator listens as surfaces are added to the current CompositorFrame to ensure that the list only includes information that matches what has been rendered in the current frame. Aggregation occurs on the BeginFrame notification. This change includes the implementaiton of the HitTestAggregator and unit test cases to verify the intended behaviour. Next steps will include completion of the creation and transport of HitTestData objects, implementation of shared memory and using this component to target incoming events. Bug:732399 ==========
gklassen@chromium.org changed reviewers: + rjkroege@chromium.org
Description was changed from ========== Implement HitTestAggregator Implements a HitTestAggregator component to facilitate cross process hit testing with minimal process hops. The HitTestAggregator maintains a list of HitTestData objects that will be created and sent with each CompositorFrame. These are aggregated into a single DisplayHitTest data structure that can be made available in shared memory to facilitate hit testing with minimal process hops for any Display. HitTestAggregator listens as surfaces are added to the current CompositorFrame to ensure that the list only includes information that matches what has been rendered in the current frame. Aggregation occurs on the BeginFrame notification. This change includes the implementaiton of the HitTestAggregator and unit test cases to verify the intended behaviour. Next steps will include completion of the creation and transport of HitTestData objects, implementation of shared memory and using this component to target incoming events. Bug:732399 ========== to ========== Implement HitTestAggregator Implements a HitTestAggregator component to facilitate cross process hit testing with minimal process hops. The HitTestAggregator maintains a list of HitTestData objects that will be created and sent with each CompositorFrame. These are aggregated into a single DisplayHitTest data structure that can be made available in shared memory to facilitate hit testing with minimal process hops for any Display. HitTestAggregator listens as surfaces are added to the current CompositorFrame to ensure that the list only includes information that matches what has been rendered in the current frame. Aggregation occurs on the BeginFrame notification. This change includes the implementaiton of the HitTestAggregator and unit test cases to verify the intended behaviour. Next steps will include completion of the creation and transport of HitTestData objects, implementation of shared memory and using this component to target incoming events. BUG=732399 ==========
( is sending this email required to kick start the review? )
Excellent progress -- some more stuff to sort out per discussion: transformations, ordering and compatibility with the walker? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data.h:10: #include <map> why? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data.h:11: #include <memory> ditto? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data.h:28: // the other. read_offset_ is used to swap buffers atomically. I think you need two buffers. I am not convinced that the read_offset can be made secure. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/display_hit_test_data_factory.h (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data_factory.h:21: virtual ~DisplayHitTestDataFactory() = default; you'll need to override it anyway so = 0? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data_factory.h:23: // Creates the memory required for a DisplayHitTestData structure. The same pattern of abstract interface permits not worrying about how messages will be sent to the host to coordinate the buffer swap. Either a separate class or this one renamed. "HitTestHostClient" or something like that. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/display_hit_test_data_factory_local.h (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data_factory_local.h:8: #include <stdint.h> why? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data_factory_local.h:10: #include <map> why? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.cc:14: // telemetry? ). UMA? We can come up with an estimate based on what I did for the hittest doc https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.cc:50: // TODO(gklassen): We need comprehensive run-time validation of hit test three separate bugs here: validation, telling the FrameSink and the FrameSink telling the host (need a general mechanism to inform the host of a difficult renderer.) https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:5: #ifndef COMPONENTS_VIZ_HIT_TEST_HIT_TEST_AGGREGATOR_H_ structuring convention: components/viz/service/hit_test for this code https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:8: #include <stdint.h> check for unnecessary includes? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:18: #include "components/viz/hit_test/public/interfaces/hit_test_data.mojom.h" Please move stuff around per discussion in person. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:51: void Aggregate(cc::SurfaceId display_surface_id); Aggregate can have a separate description? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:53: // Called at BeginFrame to swap buffers in shared memory. see my previous blither about other way to do this. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:56: // SurfaceObserver overrides can be private. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:70: DisplayHitTestData* GetDisplayHitTestData(); do we need? why? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:91: // Resize the memory for the DispalyHitTestData Structure. DisplayHitTestData https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator_unittest.cc:31: class HitTestAggregatorTest : public testing::Test { more tests will be needed to address web contents with 3d. I'm going to suggest handle that in a subsequent CL. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator_unittest.cc:329: EXPECT_EQ(region->flags, kHitTestChildSurface); afaik, there are two levels of transform. Do you want to deal with this now or in a subsequent patch? https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator_unittest.cc:609: EXPECT_EQ(region->rect, gfx::Rect(0, 0, 600, 600)); Non-actionable suggestion for future work: squares can hide errors with transposing numbers. It's happened to me at least once
New patch coming soon ( after I convert squares to rects in test cases ). I will plan to - convert to using an augmented BeginFrame message for synchronization - convert to dual buffers instead of a single memory area, and - reconstruct the tree to bring bounds back and will let you know when this patch is ready. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data.h:10: #include <map> On 2017/06/15 21:58:43, rjkroege wrote: > why? Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data.h:11: #include <memory> On 2017/06/15 21:58:43, rjkroege wrote: > ditto? Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data.h:28: // the other. read_offset_ is used to swap buffers atomically. On 2017/06/15 21:58:43, rjkroege wrote: > I think you need two buffers. I am not convinced that the read_offset can be > made secure. It needs to be made secure regardless of which approach. I will investigate the alternative of sending the synchronization with the BeginFrame message. Any pointers to where this message exists would be helpful. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/display_hit_test_data_factory.h (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data_factory.h:21: virtual ~DisplayHitTestDataFactory() = default; On 2017/06/15 21:58:44, rjkroege wrote: > you'll need to override it anyway so = 0? Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data_factory.h:23: // Creates the memory required for a DisplayHitTestData structure. On 2017/06/15 21:58:43, rjkroege wrote: > The same pattern of abstract interface permits not worrying about how messages > will be sent to the host to coordinate the buffer swap. Either a separate class > or this one renamed. "HitTestHostClient" or something like that. Not sure what this is referring to? ( sorry ). https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/display_hit_test_data_factory_local.h (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data_factory_local.h:8: #include <stdint.h> On 2017/06/15 21:58:44, rjkroege wrote: > why? Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/display_hit_test_data_factory_local.h:10: #include <map> On 2017/06/15 21:58:44, rjkroege wrote: > why? Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.cc:14: // telemetry? ). On 2017/06/15 21:58:44, rjkroege wrote: > UMA? We can come up with an estimate based on what I did for the hittest doc Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.cc:50: // TODO(gklassen): We need comprehensive run-time validation of hit test On 2017/06/15 21:58:44, rjkroege wrote: > three separate bugs here: validation, telling the FrameSink and the FrameSink > telling the host (need a general mechanism to inform the host of a difficult > renderer.) Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:5: #ifndef COMPONENTS_VIZ_HIT_TEST_HIT_TEST_AGGREGATOR_H_ On 2017/06/15 21:58:44, rjkroege wrote: > structuring convention: components/viz/service/hit_test for this code Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:8: #include <stdint.h> On 2017/06/15 21:58:44, rjkroege wrote: > check for unnecessary includes? Done. ( is there a script or other tool to make this easy? ) https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:18: #include "components/viz/hit_test/public/interfaces/hit_test_data.mojom.h" On 2017/06/15 21:58:44, rjkroege wrote: > Please move stuff around per discussion in person. Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:51: void Aggregate(cc::SurfaceId display_surface_id); On 2017/06/15 21:58:44, rjkroege wrote: > Aggregate can have a separate description? Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:53: // Called at BeginFrame to swap buffers in shared memory. On 2017/06/15 21:58:44, rjkroege wrote: > see my previous blither about other way to do this. Thank you. I will investigate. Any pointers to the existing path / message would be appreciated. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:56: // SurfaceObserver On 2017/06/15 21:58:44, rjkroege wrote: > overrides can be private. Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:70: DisplayHitTestData* GetDisplayHitTestData(); On 2017/06/15 21:58:44, rjkroege wrote: > do we need? why? Done. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator.h:91: // Resize the memory for the DispalyHitTestData Structure. On 2017/06/15 21:58:44, rjkroege wrote: > DisplayHitTestData Done. Good eye. Thank you. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... File components/viz/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator_unittest.cc:31: class HitTestAggregatorTest : public testing::Test { On 2017/06/15 21:58:44, rjkroege wrote: > more tests will be needed to address web contents with 3d. I'm going to suggest > handle that in a subsequent CL. Done. TODO added. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator_unittest.cc:329: EXPECT_EQ(region->flags, kHitTestChildSurface); On 2017/06/15 21:58:45, rjkroege wrote: > afaik, there are two levels of transform. Do you want to deal with this now or > in a subsequent patch? I would prefer to deal with this in a later patch when we know the structure and context to ensure the implementation matches the use case. https://codereview.chromium.org/2938953002/diff/20001/components/viz/hit_test... components/viz/hit_test/hit_test_aggregator_unittest.cc:609: EXPECT_EQ(region->rect, gfx::Rect(0, 0, 600, 600)); On 2017/06/15 21:58:45, rjkroege wrote: > Non-actionable suggestion for future work: squares can hide errors with > transposing numbers. It's happened to me at least once Done. Converted all squares to rects.
varkha@chromium.org changed reviewers: + varkha@chromium.org
Just learning about this and driving by so posting a few nits. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... File components/viz/common/hit_test/OWNERS (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/OWNERS:2: gklassen@chromium.org I vaguely remember a recent discussion that we should only have Chromium committers in OWNERS files. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:11: #include "cc/surfaces/surface_observer.h" Is this header necessary? https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:12: #include "services/viz/hit_test/public/interfaces/hit_test_data.mojom.h" Ditto. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:16: // DiplayHitTestData contains the hit_test data for the Display. nit: introduce hit_test? Or did you mean hit-test in a general sense? https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:24: // the other. read_offset_ is used to swap buffers atomically. nit: s/read_offset_/|read_offset| https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:30: // A read_offset_ set to kOldPleaseReAcquire indicates that the buffer has been nit: s/read_offset_/|read_offset| https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:35: cc::FrameSinkId frame_sink_id; Needs cc/surfaces/frame_sink_id.h ? https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:37: gfx::Rect rect; does this need a header? https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:38: gfx::Transform transform; Ditto. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:44: base::subtle::Atomic32 read_offset; Needs base/atomicops.h ? https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:50: #endif // COMPONENTS_VIZ_COMMON_HIT_TEST_DISPLAY_HIT_TEST_DATA_H_ nit: remove extra space after // https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/OWNERS (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/OWNERS:2: gklassen@chromium.org ditto https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/display_hit_test_data_factory.h (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/display_hit_test_data_factory.h:33: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_DISPLAY_HIT_TEST_DATA_FACTORY_H_ nit: Remove extra space after // https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/display_hit_test_data_factory_local.cc (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/display_hit_test_data_factory_local.cc:5: #include "display_hit_test_data_factory_local.h" nit: ws after this line (but consider the net nit. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/display_hit_test_data_factory_local.cc:6: #include "base/logging.h" Is this necessary? https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.cc:6: #include "base/logging.h" order alphabetically (git cl format will do that for you). https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.cc:20: if (hit_test_region->flags == hit_test::mojom::kHitTestChildSurface) { nit: Can inverse this and return early. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.cc:21: if (!hit_test_region->surface_id.is_valid()) { nit: no {} for one liner conditional. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:22: // HitTest maintains maping between display regions and associated surfaces HitTestAggregator? https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:51: // Called at BeginFrame to swap buffers in shared memory. nit: Maybe "Called at BeginFrame. Swaps buffers in shared memory." https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:54: DisplayHitTestRegion* GetCurrentRegions(); Comment? https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:60: HitTestDataMap active_; nit: move after methods? https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:62: // Keep track of the number of regions in the active list nit: Keeps. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:74: // Resize the memory for the DisplayHitTestData Structure. nit: Resizes. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:75: // Copies current data and marks the structure nit. Sentence ends in . https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:83: // SurfaceObserver nit: cc::SurfaceObserver: https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:90: const cc::BeginFrameArgs& args) override {} nit: ws after this line. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:94: // to active_ in this method. nit: pending_ and active_ refer to variables so wrap in ||. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:107: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_ nit: remove extra space after //
New patch uploaded that includes - refactoring of the tree to add bounds ( it seems to clean things up nicely - thank you all for the help ), - improvements based on varkha's comments ( most addressed although I would appreciate advice on OWNERS ). Key remaining task is to change the memory management and synchronization to use two distinct buffers and to rely on message passing for synchronization. I will let you know when this is done but in the meantime feedback and review on these improvements and the approach in general is greatly appreciated. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... File components/viz/common/hit_test/OWNERS (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/OWNERS:2: gklassen@chromium.org On 2017/06/19 21:15:03, varkha (OOO till June 13) wrote: > I vaguely remember a recent discussion that we should only have Chromium > committers in OWNERS files. I found a discussion here: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/WjvaT2B9-fs which seems to answer that Yes it is ok to have an OWNER that is not a committer and then quotes and references a document that clearly states it is not ok ( Owners must "Be a Chromium project member with full commit access of at least 6 months tenure." ). I am fine either way. Rob will likely need to review if I am un-listed and so I'm happy to follow his preference. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:11: #include "cc/surfaces/surface_observer.h" On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > Is this header necessary? No, you're right, it isn't. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:12: #include "services/viz/hit_test/public/interfaces/hit_test_data.mojom.h" On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > Ditto. This one is necessary. It pulls in the constants that define valid values for flags and it also pulls in the appropriate definitions for the types used in the structure. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:16: // DiplayHitTestData contains the hit_test data for the Display. On 2017/06/19 21:15:03, varkha (OOO till June 13) wrote: > nit: introduce hit_test? Or did you mean hit-test in a general sense? Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:24: // the other. read_offset_ is used to swap buffers atomically. On 2017/06/19 21:15:03, varkha (OOO till June 13) wrote: > nit: s/read_offset_/|read_offset| Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:30: // A read_offset_ set to kOldPleaseReAcquire indicates that the buffer has been On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > nit: s/read_offset_/|read_offset| Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:35: cc::FrameSinkId frame_sink_id; On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > Needs cc/surfaces/frame_sink_id.h ? These are pulled in with the mojom reference and definition. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:44: base::subtle::Atomic32 read_offset; On 2017/06/19 21:15:03, varkha (OOO till June 13) wrote: > Needs base/atomicops.h ? This will need to change anyways - we will be changing the synchronization strategy. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:50: #endif // COMPONENTS_VIZ_COMMON_HIT_TEST_DISPLAY_HIT_TEST_DATA_H_ On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > nit: remove extra space after // Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/display_hit_test_data_factory_local.cc (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/display_hit_test_data_factory_local.cc:5: #include "display_hit_test_data_factory_local.h" On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > nit: ws after this line (but consider the net nit. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/display_hit_test_data_factory_local.cc:6: #include "base/logging.h" On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > Is this necessary? Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.cc:6: #include "base/logging.h" On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > order alphabetically (git cl format will do that for you). Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.cc:20: if (hit_test_region->flags == hit_test::mojom::kHitTestChildSurface) { On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > nit: Can inverse this and return early. I anticipate that this method can and will expand to include other tests for other types so I'd like to keep this structure for now - and I don't *think* it costs much additional runtime at all right now - sound ok? https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.cc:21: if (!hit_test_region->surface_id.is_valid()) { On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > nit: no {} for one liner conditional. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:22: // HitTest maintains maping between display regions and associated surfaces On 2017/06/19 21:15:05, varkha (OOO till June 13) wrote: > HitTestAggregator? Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:51: // Called at BeginFrame to swap buffers in shared memory. On 2017/06/19 21:15:05, varkha (OOO till June 13) wrote: > nit: Maybe "Called at BeginFrame. Swaps buffers in shared memory." Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:54: DisplayHitTestRegion* GetCurrentRegions(); On 2017/06/19 21:15:05, varkha (OOO till June 13) wrote: > Comment? Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:60: HitTestDataMap active_; On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > nit: move after methods? Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:62: // Keep track of the number of regions in the active list On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > nit: Keeps. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:74: // Resize the memory for the DisplayHitTestData Structure. On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > nit: Resizes. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:75: // Copies current data and marks the structure On 2017/06/19 21:15:05, varkha (OOO till June 13) wrote: > nit. Sentence ends in . Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:83: // SurfaceObserver On 2017/06/19 21:15:05, varkha (OOO till June 13) wrote: > nit: cc::SurfaceObserver: Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:90: const cc::BeginFrameArgs& args) override {} On 2017/06/19 21:15:05, varkha (OOO till June 13) wrote: > nit: ws after this line. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:94: // to active_ in this method. On 2017/06/19 21:15:05, varkha (OOO till June 13) wrote: > nit: pending_ and active_ refer to variables so wrap in ||. Done.
Thanks, no need to address the nits before taking on deeper work but I was recording them as I read the code so see below. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:12: #include "services/viz/hit_test/public/interfaces/hit_test_data.mojom.h" On 2017/06/20 16:09:37, gklassen wrote: > On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > > Ditto. > > This one is necessary. It pulls in the constants that define valid values for > flags and it also pulls in the appropriate definitions for the types used in the > structure. Not sure about the style with mojo. Do we usually do that? I am used to bringing in only the strictly required includes in the header files and leaving the rest to be included / imported in .cc files. https://codereview.chromium.org/2938953002/diff/100001/components/viz/common/... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:34: // The frame_sink_id corresponding to this region. Events that match nit: s/frame_sink_id/cc::FrameSinkId https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/display_hit_test_data_factory.h (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/display_hit_test_data_factory.h:11: #include <memory> Are 3 headers above necessary? https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/display_hit_test_data_factory.h:14: #include "components/viz/service/viz_service_export.h" Is this necessary? Should the class be VIZ_SERVICE_EXPORT? https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/display_hit_test_data_factory.h:33: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_DISPLAY_HIT_TEST_DATA_FACTORY_H_ nit: extra space after // https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:27: if (!ValidateHitTestRegion(region)) { nit: no need for {} https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:130: while (length < max_size) { nit: no need for {} https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:159: for (auto& region : hit_test_data->regions) { nit ditto. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:198: for (auto& child_region : hit_test_data->regions) { nit: ditto. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:28: using HitTestDataMap = std::map<cc::SurfaceId, hit_test::mojom::HitTestDataPtr>; Does this need to be visible publicly? https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:45: void Aggregate(cc::SurfaceId display_surface_id); Q: Do both Aggregate and PostTaskAggregate need to be in public API? https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:69: int GetBackIndex(); nit: doc. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:78: // cc:SurfaceObserver nit: cc::SurfaceObserver: https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:120: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_ nit: extra space after // https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:120: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_ nit: extra space after // https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:37: nit: // testing::Test: https://codereview.chromium.org/2938953002/diff/100001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2938953002/diff/100001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:28: // Flags to indicate the type of HitTestRegion. nit: tabulation. https://codereview.chromium.org/2938953002/diff/100001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:58: //TODO(gklassen): invert order if its easier to write them back to front. nit: space after //, its -> it's
riajiang@chromium.org changed reviewers: + riajiang@chromium.org
https://codereview.chromium.org/2938953002/diff/100001/components/viz/common/... File components/viz/common/DEPS (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/common/... components/viz/common/DEPS:3: "+cc/surfaces", "+cc/..." before "+services/..."? Also Rob commented about this in my CL before - too much stuff in cc/surfaces - so I changed it to only include "+cc/surfaces/frame_sink_id.h". https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:335: EXPECT_EQ(region->flags, kHitTestChildSurface); Was thinking to make sure that I understand this test case: so the difference between this one and OneEmbedderTwoRegions is whether they are regions or children? Does that mean in this case c1 and c2 can't receive events (i.e. all events should go to e) since they both only have kHitTestChildSurface flag but not kHitTestMine (as opposed to kHitTestChildSurface | kHitTestMine in OccludedChildFrame for c)? https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:697: EXPECT_EQ(region->child_count, 0); Can we maybe add a test case where e has two children, c1 and d where d is under c1 to confirm which z-order (back-to-front or front-to-back) we are using?
The latest patch contains improvements based on reviewer comments and discussion inlcluding - re-arrangements of structure, - tree restructure so that there is always a root, - using two buffers instead of one, - using shared memory handles to manage memory and lifetime, - other comments and nits addressed. It's much better with all the comments and review. Thank you all. With this patch I am open and ready for another round of review and additional comments. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:12: #include "services/viz/hit_test/public/interfaces/hit_test_data.mojom.h" On 2017/06/20 19:56:58, varkha wrote: > On 2017/06/20 16:09:37, gklassen wrote: > > On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > > > Ditto. > > > > This one is necessary. It pulls in the constants that define valid values for > > flags and it also pulls in the appropriate definitions for the types used in > the > > structure. > > Not sure about the style with mojo. Do we usually do that? I am used to bringing > in only the strictly required includes in the header files and leaving the rest > to be included / imported in .cc files. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:35: cc::FrameSinkId frame_sink_id; On 2017/06/20 16:09:37, gklassen wrote: > On 2017/06/19 21:15:04, varkha (OOO till June 13) wrote: > > Needs cc/surfaces/frame_sink_id.h ? > > These are pulled in with the mojom reference and definition. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:37: gfx::Rect rect; On 2017/06/19 21:15:04, varkha wrote: > does this need a header? Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:38: gfx::Transform transform; On 2017/06/19 21:15:03, varkha wrote: > Ditto. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:44: base::subtle::Atomic32 read_offset; On 2017/06/20 16:09:37, gklassen wrote: > On 2017/06/19 21:15:03, varkha (OOO till June 13) wrote: > > Needs base/atomicops.h ? > > This will need to change anyways - we will be changing the synchronization > strategy. Done. https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/60001/components/viz/service/... components/viz/service/hit_test/hit_test_aggregator.h:107: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_ On 2017/06/19 21:15:04, varkha wrote: > nit: remove extra space after // Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/common/... File components/viz/common/DEPS (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/common/... components/viz/common/DEPS:3: "+cc/surfaces", On 2017/06/20 23:54:59, riajiang wrote: > "+cc/..." before "+services/..."? > > Also Rob commented about this in my CL before - too much stuff in cc/surfaces - > so I changed it to only include "+cc/surfaces/frame_sink_id.h". Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/common/... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:34: // The frame_sink_id corresponding to this region. Events that match On 2017/06/20 19:56:58, varkha wrote: > nit: s/frame_sink_id/cc::FrameSinkId Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/display_hit_test_data_factory.h (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/display_hit_test_data_factory.h:11: #include <memory> On 2017/06/20 19:56:58, varkha wrote: > Are 3 headers above necessary? Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/display_hit_test_data_factory.h:14: #include "components/viz/service/viz_service_export.h" On 2017/06/20 19:56:58, varkha wrote: > Is this necessary? Should the class be VIZ_SERVICE_EXPORT? Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/display_hit_test_data_factory.h:33: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_DISPLAY_HIT_TEST_DATA_FACTORY_H_ On 2017/06/20 19:56:58, varkha wrote: > nit: extra space after // Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:27: if (!ValidateHitTestRegion(region)) { On 2017/06/20 19:56:59, varkha wrote: > nit: no need for {} Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:130: while (length < max_size) { On 2017/06/20 19:56:59, varkha wrote: > nit: no need for {} Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:159: for (auto& region : hit_test_data->regions) { On 2017/06/20 19:56:59, varkha wrote: > nit ditto. Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:198: for (auto& child_region : hit_test_data->regions) { On 2017/06/20 19:56:59, varkha wrote: > nit: ditto. Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:28: using HitTestDataMap = std::map<cc::SurfaceId, hit_test::mojom::HitTestDataPtr>; On 2017/06/20 19:56:59, varkha wrote: > Does this need to be visible publicly? Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:45: void Aggregate(cc::SurfaceId display_surface_id); On 2017/06/20 19:56:59, varkha wrote: > Q: Do both Aggregate and PostTaskAggregate need to be in public API? Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:69: int GetBackIndex(); On 2017/06/20 19:56:59, varkha wrote: > nit: doc. Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:78: // cc:SurfaceObserver On 2017/06/20 19:56:59, varkha wrote: > nit: cc::SurfaceObserver: Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:120: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_ On 2017/06/20 19:56:59, varkha wrote: > nit: extra space after // Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:120: #endif // COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_ On 2017/06/20 19:56:59, varkha wrote: > nit: extra space after // Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:37: On 2017/06/20 19:56:59, varkha wrote: > nit: // testing::Test: Done. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:335: EXPECT_EQ(region->flags, kHitTestChildSurface); On 2017/06/20 23:54:59, riajiang wrote: > Was thinking to make sure that I understand this test case: so the difference > between this one and OneEmbedderTwoRegions is whether they are regions or > children? Does that mean in this case c1 and c2 can't receive events (i.e. all > events should go to e) since they both only have kHitTestChildSurface flag but > not kHitTestMine (as opposed to kHitTestChildSurface | kHitTestMine in > OccludedChildFrame for c)? Yes. https://codereview.chromium.org/2938953002/diff/100001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:697: EXPECT_EQ(region->child_count, 0); On 2017/06/20 23:54:59, riajiang wrote: > Can we maybe add a test case where e has two children, c1 and d where d is under > c1 to confirm which z-order (back-to-front or front-to-back) we are using? Good call, thank you. Test Added. Done. https://codereview.chromium.org/2938953002/diff/100001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2938953002/diff/100001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:28: // Flags to indicate the type of HitTestRegion. On 2017/06/20 19:56:59, varkha wrote: > nit: tabulation. Done. https://codereview.chromium.org/2938953002/diff/100001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:58: //TODO(gklassen): invert order if its easier to write them back to front. On 2017/06/20 19:56:59, varkha wrote: > nit: space after //, its -> it's Done.
assorted comments. major issue here is the revised naming or stuff... sorry. https://codereview.chromium.org/2938953002/diff/120001/components/viz/common/... File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/120001/components/viz/common/... components/viz/common/BUILD.gn:16: "//services/viz/hit_test/public/interfaces", sorry for churn... but services/viz/public/interfaces for the mojom. https://codereview.chromium.org/2938953002/diff/120001/components/viz/common/... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/120001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. recent discussion suggests that common should be flattened: components/viz/hit_test_* until the number of files grows larger. https://codereview.chromium.org/2938953002/diff/120001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:44: // The number of children ( including their children ) below this entry. nit: I think it's weird to have a space after/before the (). You can ignore me in this. https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... File components/viz/service/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/BUILD.gn:101: test("hit_test_unit_tests") { currently the opinion is that hit_test_unit_tests should be part of unit_tests -- there will be one unit test target for components/* Use filter flags to pull out a subset for faster iteration. https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:12: // TODO(gklassen): Review and select appropriate sizes ( based on same nit about spaces and ( https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:115: while (size < max_size) it is conceivable that a client could try to DoS us. There should be a check here to detect a client that has attempted to create too many regions? https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:125: if (search == active_.end()) { how this can happen? https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:159: if (region->flags == hit_test::mojom::kHitTestChildSurface) { why is this possible? You are only activating surfaces that are in the Display CompositorFrame right? https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/BUILD.gn:7: mojom("interfaces") { flattened https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/OWNERS (right): https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/OWNERS:1: per-file *.mojom=set noparent see blither about flattening https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:43: // SurfaceId corresponding to this HitTestData. I've observed before: all of this info could just be in the 0-th HitTestRegion. I'm not sure if that's a good idea or not. But it has (at least to me) a pleasant regularity.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2938953002/diff/140001/components/viz/common/... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/140001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:39: // The rectangle that defines the region. In the parent region's coordinate space, I guess? https://codereview.chromium.org/2938953002/diff/140001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:42: // The transform applied to the rect. ditto https://codereview.chromium.org/2938953002/diff/140001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:45: // The number of children ( including their children ) below this entry. no space after ( or before ) (applies to other places) https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... File components/viz/service/hit_test/OWNERS (right): https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/OWNERS:3: riajiang@chromium.org Use file:/// to refer to the other OWNERS file, so you need to update the list in just one place. https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:89: size_t num_bytes = size * sizeof(DisplayHitTestRegion); Since |size| here depends on the input coming in from untrusted clients, should this use base::CheckedNumeric<> to check for overflow? https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:111: size += kIncrementalSize; We probably don't need the while loop? https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:126: mojo::ScopedSharedBufferMapping mapping = MapWriteBuffer(); Is there a reason we need to map/unmap? Can we just map and keep the mapped memory around? https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:136: index = AppendRegion(regions, region, index); Can we DCHECK/CHECK that |index| remains within bounds at each step? https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/OWNERS (right): https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/OWNERS:5: riajiang@chromium.org I actually don't think we want non-security folks in here [yet, anyway]. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:24: const uint32 kHitTestNoTouchEventHandler = 0x10; As mentioned in https://codereview.chromium.org/2908783002/#msg62, instead of saying kHitTestNoTouchEventHandler, this should be kHitTestTouchEventHandler. We should also have a kHitTestMouseEventHandler. For aura windows, the window borders can have different hit-test regions for mouse vs. for touch. This means aura will send some HitTest* which should be used only for mouse events, and some HitTest* which should be used only for touch events. It would be simplest to have explicit kHitTestForTouch, kHitTestForMouse (and potentially kHitTestForStylus etc. in the future if so desired) flags. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:27: no blank line. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:37: // The transform applied to the child region. What is 'child region'? (https://codereview.chromium.org/2908783002/diff/480001/components/viz/hit_tes...) https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:42: ditto https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:59: array<HitTestRegion> regions; As mentioned in Ria's CL, I think I would go with HitTestData and HitTestDataList (or HitTestRegion and HitTestRegionList). Having both HitTestData and HitTestRegion is confusing, because neither is clearly a list of the other thing.
Patch uploaded with flattened hierarchy and addressing most nits. https://codereview.chromium.org/2938953002/diff/120001/components/viz/common/... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/120001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/06/27 00:00:07, rjkroege wrote: > recent discussion suggests that common should be flattened: > components/viz/hit_test_* until the number of files grows larger. Done. https://codereview.chromium.org/2938953002/diff/120001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:44: // The number of children ( including their children ) below this entry. On 2017/06/27 00:00:07, rjkroege wrote: > nit: I think it's weird to have a space after/before the (). You can ignore me > in this. Done. https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... File components/viz/service/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/BUILD.gn:101: test("hit_test_unit_tests") { On 2017/06/27 00:00:07, rjkroege wrote: > currently the opinion is that hit_test_unit_tests should be part of unit_tests > -- there will be one unit test target for components/* Use filter flags to pull > out a subset for faster iteration. Done. https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:12: // TODO(gklassen): Review and select appropriate sizes ( based on On 2017/06/27 00:00:07, rjkroege wrote: > same nit about spaces and ( Done. https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:115: while (size < max_size) On 2017/06/27 00:00:07, rjkroege wrote: > it is conceivable that a client could try to DoS us. There should be a check > here to detect a client that has attempted to create too many regions? Done. https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:125: if (search == active_.end()) { On 2017/06/27 00:00:07, rjkroege wrote: > how this can happen? Done. https://codereview.chromium.org/2938953002/diff/120001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:159: if (region->flags == hit_test::mojom::kHitTestChildSurface) { On 2017/06/27 00:00:07, rjkroege wrote: > why is this possible? You are only activating surfaces that are in the Display > CompositorFrame right? Yes, we are only activating surfaces that are in the Display Compositor Frame. It is possible that a surface references a child frame and that child's Compositor Frame is late and not included in the Display Compositor Frame and this check is to handle that case (assuming it's possible). https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/BUILD.gn:7: mojom("interfaces") { On 2017/06/27 00:00:07, rjkroege wrote: > flattened Done. https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/OWNERS (right): https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/OWNERS:1: per-file *.mojom=set noparent On 2017/06/27 00:00:07, rjkroege wrote: > see blither about flattening Done. https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2938953002/diff/120001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:43: // SurfaceId corresponding to this HitTestData. On 2017/06/27 00:00:07, rjkroege wrote: > I've observed before: all of this info could just be in the 0-th HitTestRegion. > I'm not sure if that's a good idea or not. But it has (at least to me) a > pleasant regularity. I agree and my initial attempt was to build it this way - but the code that builds the tree got a little too strange for my comfort because it had to inspect the 0-th element and handle it differently. Writing it this way seemed to keep the implementation cleaner because all regions are handled the same way and it makes requirement for flags, bounds and transform in the hit_test_data structure explicit and required. https://codereview.chromium.org/2938953002/diff/140001/components/viz/common/... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/140001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:39: // The rectangle that defines the region. On 2017/06/27 04:23:34, sadrul wrote: > In the parent region's coordinate space, I guess? Done. https://codereview.chromium.org/2938953002/diff/140001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:42: // The transform applied to the rect. On 2017/06/27 04:23:34, sadrul wrote: > ditto Done. https://codereview.chromium.org/2938953002/diff/140001/components/viz/common/... components/viz/common/hit_test/display_hit_test_data.h:45: // The number of children ( including their children ) below this entry. On 2017/06/27 04:23:34, sadrul wrote: > no space after ( or before ) (applies to other places) Done. https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... File components/viz/service/hit_test/OWNERS (right): https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/OWNERS:3: riajiang@chromium.org On 2017/06/27 04:23:34, sadrul wrote: > Use file:/// to refer to the other OWNERS file, so you need to update the list > in just one place. Acknowledged. This is now the only unique OWNERS file for the hit test component after flattening structure. https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:89: size_t num_bytes = size * sizeof(DisplayHitTestRegion); On 2017/06/27 04:23:35, sadrul wrote: > Since |size| here depends on the input coming in from untrusted clients, should > this use base::CheckedNumeric<> to check for overflow? Good call. A check has been added in the code that checks the size before allocation. Done. https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:111: size += kIncrementalSize; On 2017/06/27 04:23:35, sadrul wrote: > We probably don't need the while loop? Done. https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:126: mojo::ScopedSharedBufferMapping mapping = MapWriteBuffer(); On 2017/06/27 04:23:34, sadrul wrote: > Is there a reason we need to map/unmap? Can we just map and keep the mapped > memory around? It needs to be mapped here to initialize the list. I had considered keeping the mapped reference, but we really only need it at initialization and when we aggregate the list so this approach seemed to work well and avoids the needs to manage ( and swap ) another pointer. https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:136: index = AppendRegion(regions, region, index); On 2017/06/27 04:23:35, sadrul wrote: > Can we DCHECK/CHECK that |index| remains within bounds at each step? Done. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/OWNERS (right): https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/OWNERS:5: riajiang@chromium.org On 2017/06/27 04:23:35, sadrul wrote: > I actually don't think we want non-security folks in here [yet, anyway]. Done. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:24: const uint32 kHitTestNoTouchEventHandler = 0x10; On 2017/06/27 04:23:35, sadrul wrote: > As mentioned in https://codereview.chromium.org/2908783002/#msg62, instead of > saying kHitTestNoTouchEventHandler, this should be kHitTestTouchEventHandler. We > should also have a kHitTestMouseEventHandler. > > For aura windows, the window borders can have different hit-test regions for > mouse vs. for touch. This means aura will send some HitTest* which should be > used only for mouse events, and some HitTest* which should be used only for > touch events. It would be simplest to have explicit kHitTestForTouch, > kHitTestForMouse (and potentially kHitTestForStylus etc. in the future if so > desired) flags. Hmmm... I reviewed this implementation in detail, but I had thought the intention was to indicate regions that did not have a touch event handler and that this was the exception. I agree that we will have to review details for exactly what flags are required to handle this well. Ok to revisit after we have this working? https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:27: On 2017/06/27 04:23:35, sadrul wrote: > no blank line. Done. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:37: // The transform applied to the child region. On 2017/06/27 04:23:35, sadrul wrote: > What is 'child region'? > (https://codereview.chromium.org/2908783002/diff/480001/components/viz/hit_tes...) Done. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:42: On 2017/06/27 04:23:35, sadrul wrote: > ditto Done. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:59: array<HitTestRegion> regions; On 2017/06/27 04:23:35, sadrul wrote: > As mentioned in Ria's CL, I think I would go with HitTestData and > HitTestDataList (or HitTestRegion and HitTestRegionList). Having both > HitTestData and HitTestRegion is confusing, because neither is clearly a list of > the other thing. Agreed that neither is clearly a list of the other thing - but perhaps that's ok? Currently HitTestData includes all the hit-test information required for any surface and that includes a list of regions - that *seems* to match the structure has defined quite well - is there a better way to capture that?
One more modification to keep a reference to mapped buffers as Sadrul suggested and added another unit test for missing children. I'd value input on whether this is close to something we could land and then move onto next steps to flesh out the use of this component in context. https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/140001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:126: mojo::ScopedSharedBufferMapping mapping = MapWriteBuffer(); On 2017/06/27 21:46:33, gklassen wrote: > On 2017/06/27 04:23:34, sadrul wrote: > > Is there a reason we need to map/unmap? Can we just map and keep the mapped > > memory around? > > It needs to be mapped here to initialize the list. I had considered keeping the > mapped reference, but we really only need it at initialization and when we > aggregate the list so this approach seemed to work well and avoids the needs to > manage ( and swap ) another pointer. On second thought this is a good suggestion. Done.
> I'd value input on whether this is close to something we could land and then > move onto next steps to flesh out the use of this component in context. I think we should be able to land at least parts of this CL (e.g the data types, mojoms), if not all, soon. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:59: array<HitTestRegion> regions; On 2017/06/27 21:46:33, gklassen wrote: > On 2017/06/27 04:23:35, sadrul wrote: > > As mentioned in Ria's CL, I think I would go with HitTestData and > > HitTestDataList (or HitTestRegion and HitTestRegionList). Having both > > HitTestData and HitTestRegion is confusing, because neither is clearly a list > of > > the other thing. > > Agreed that neither is clearly a list of the other thing - but perhaps that's > ok? Currently HitTestData includes all the hit-test information required for > any surface and that includes a list of regions - that *seems* to match the > structure has defined quite well - is there a better way to capture that? I think the distinction between the two that matters most, when reading the code, is which one includes all data (or I guess the 'root data' or the 'toplevel data'), vs. which one includes data about one specific target+event-source. I am OK with any pair of names that makes that readily obvious. I don't think (HitTestRegion, HitTestData) pair meets that bar. In several offline discussions, I myself have been confused between the two, which to me means more folks would be similarly confused. https://codereview.chromium.org/2938953002/diff/180001/components/viz/common/... File components/viz/common/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/180001/components/viz/common/... components/viz/common/display_hit_test_data.h:30: struct DisplayHitTestRegion { Don't mean to bikeshed, but ... :) The file name is display_hit_test_data.h, and the documentation above at the top refers to Di[s]playHitTestData. So either this should be called DisplayHitTestData, or the file should be renamed + documentation above updated. https://codereview.chromium.org/2938953002/diff/180001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/180001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:26: return true; This can be just: return hit_test_region->flags != hit_test::mojom::kHitTestChildSurface || hit_test_region->surface_id.is_valid(); https://codereview.chromium.org/2938953002/diff/180001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:161: element->child_count = 0; I think |child_count| gets set to kEndOfList in AppendRoot() after returning? So maybe just skip this, and add a comment instead? https://codereview.chromium.org/2938953002/diff/180001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:183: index = AppendRegion(regions, child_region, index); Should we break if |index| reaches |write_size_ - 1| at any iteration in this loop?
New patch uploaded that addresses Sadrul's comments. https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2938953002/diff/140001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/hit_test_data.mojom:59: array<HitTestRegion> regions; On 2017/06/28 05:27:40, sadrul wrote: > On 2017/06/27 21:46:33, gklassen wrote: > > On 2017/06/27 04:23:35, sadrul wrote: > > > As mentioned in Ria's CL, I think I would go with HitTestData and > > > HitTestDataList (or HitTestRegion and HitTestRegionList). Having both > > > HitTestData and HitTestRegion is confusing, because neither is clearly a > list > > of > > > the other thing. > > > > Agreed that neither is clearly a list of the other thing - but perhaps that's > > ok? Currently HitTestData includes all the hit-test information required for > > any surface and that includes a list of regions - that *seems* to match the > > structure has defined quite well - is there a better way to capture that? > > I think the distinction between the two that matters most, when reading the > code, is which one includes all data (or I guess the 'root data' or the > 'toplevel data'), vs. which one includes data about one specific > target+event-source. I am OK with any pair of names that makes that readily > obvious. I don't think (HitTestRegion, HitTestData) pair meets that bar. In > several offline discussions, I myself have been confused between the two, which > to me means more folks would be similarly confused. Done. https://codereview.chromium.org/2938953002/diff/180001/components/viz/common/... File components/viz/common/display_hit_test_data.h (right): https://codereview.chromium.org/2938953002/diff/180001/components/viz/common/... components/viz/common/display_hit_test_data.h:30: struct DisplayHitTestRegion { On 2017/06/28 05:27:40, sadrul wrote: > Don't mean to bikeshed, but ... :) > > The file name is display_hit_test_data.h, and the documentation above at the top > refers to Di[s]playHitTestData. So either this should be called > DisplayHitTestData, or the file should be renamed + documentation above updated. Done. https://codereview.chromium.org/2938953002/diff/180001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/180001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:26: return true; On 2017/06/28 05:27:40, sadrul wrote: > This can be just: > > return hit_test_region->flags != hit_test::mojom::kHitTestChildSurface || > hit_test_region->surface_id.is_valid() As we understand better how these will be constructed, I hope and expect that we will add additional checks for other fields and so I'd like to preserve the existing structure for readability and extension. Is that ok? https://codereview.chromium.org/2938953002/diff/180001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:161: element->child_count = 0; On 2017/06/28 05:27:40, sadrul wrote: > I think |child_count| gets set to kEndOfList in AppendRoot() after returning? So > maybe just skip this, and add a comment instead? Done. https://codereview.chromium.org/2938953002/diff/180001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:183: index = AppendRegion(regions, child_region, index); On 2017/06/28 05:27:40, sadrul wrote: > Should we break if |index| reaches |write_size_ - 1| at any iteration in this > loop? Done.
lgtm. https://codereview.chromium.org/2938953002/diff/200001/components/viz/common/... File components/viz/common/DEPS (right): https://codereview.chromium.org/2938953002/diff/200001/components/viz/common/... components/viz/common/DEPS:2: "+cc/surfaces", this is overly-broad. It would be nicer to make sure that you explicitly list the deps you need: transitive expansion of surface_id.h right? https://codereview.chromium.org/2938953002/diff/200001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/200001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:12: // TODO(gklassen): Review and select appropriate sizes based on Please file bugs for the TODOs at your convenience.
https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:27: } nit: ws https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:44: } ditto https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:114: // Size check. nit: comments need to be complete sentences. https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:45: while (end->child_count != kEndOfList) { nit: no {} https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:143: // Expect 1 entry routing all events to the one surface ( display root ). nit: spacing https://codereview.chromium.org/2938953002/diff/220001/services/viz/public/in... File services/viz/public/interfaces/hit_test_region_list.mojom (right): https://codereview.chromium.org/2938953002/diff/220001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:48: // kHitTestIgnore keeps previous match in the parent ( transparent ). nit: spacing (here and elsewhere in this file).
https://codereview.chromium.org/2938953002/diff/220001/components/viz/common/... File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/220001/components/viz/common/... components/viz/common/BUILD.gn:7: "hit_test/display_hit_test_region.h", No hit_test/? Also there's no BUILD.gn and DEPS in common/ anymore so need to rebase?
lgtm with a couple small nits but you probably need to address riajiang@'s build concerns. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:116: // Check to ensure there is enough memory has been allocated. suggestion: "there is" -> "that" https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:132: CHECK(search != active_.end()); nit: use CHECK_NE for better formatting of output in failing case. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:68: // cc:SurfaceObserver: nit: add ':' in "cc::SurfaceObserver:"
lgtm (you will get some merge conflicts when you merge with tot before landing) https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:65: active_.erase(surface_id); Should this also update |active_region_count_|? https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:186: index = AppendRegion(regions, child_region, index); for (const auto& child_region : ...) { index = AppendRegion(...); if (index >= write_size_ - 1) break; } (we can probably do this in the loop in AppendRoot() too. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:102: int write_size_; Please document the size is the number of elements in the array, not the number of bytes.
https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:102: int write_size_; On 2017/06/28 20:30:12, sadrul wrote: > Please document the size is the number of elements in the array, not the number > of bytes. Maybe consider a self-descriptive variable name as well although not sure what would work the best. Maybe read_elements_count_ / write_elements_count_
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, varkha@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2938953002/#ps280001 (title: "rebase & improvements based on comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2938953002/diff/280001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/280001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:68: auto active_search = active_.find(surface_id); This will not work since you have already erased(), right? Should there be a unit-test to make sure this works correctly?
The CQ bit was unchecked by sadrul@chromium.org
Thanks for the catch Sadrul. https://codereview.chromium.org/2938953002/diff/220001/components/viz/common/... File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/220001/components/viz/common/... components/viz/common/BUILD.gn:7: "hit_test/display_hit_test_region.h", On 2017/06/28 19:23:50, riajiang wrote: > No hit_test/? > > Also there's no BUILD.gn and DEPS in common/ anymore so need to rebase? Done. https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:27: } On 2017/06/28 19:20:38, varkha wrote: > nit: ws Done. https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:44: } On 2017/06/28 19:20:38, varkha wrote: > ditto Done. https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:114: // Size check. On 2017/06/28 19:20:38, varkha wrote: > nit: comments need to be complete sentences. Done. https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:45: while (end->child_count != kEndOfList) { On 2017/06/28 19:20:38, varkha wrote: > nit: no {} Done. https://codereview.chromium.org/2938953002/diff/220001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:143: // Expect 1 entry routing all events to the one surface ( display root ). On 2017/06/28 19:20:38, varkha wrote: > nit: spacing Done. https://codereview.chromium.org/2938953002/diff/220001/services/viz/public/in... File services/viz/public/interfaces/hit_test_region_list.mojom (right): https://codereview.chromium.org/2938953002/diff/220001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:48: // kHitTestIgnore keeps previous match in the parent ( transparent ). On 2017/06/28 19:20:38, varkha wrote: > nit: spacing (here and elsewhere in this file). Done. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:65: active_.erase(surface_id); On 2017/06/28 20:30:12, sadrul wrote: > Should this also update |active_region_count_|? Done. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:116: // Check to ensure there is enough memory has been allocated. On 2017/06/28 19:47:05, varkha wrote: > suggestion: "there is" -> "that" Done. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:132: CHECK(search != active_.end()); On 2017/06/28 19:47:05, varkha wrote: > nit: use CHECK_NE for better formatting of output in failing case. For some reasons CHECK_NE doesn't compile? https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:186: index = AppendRegion(regions, child_region, index); On 2017/06/28 20:30:12, sadrul wrote: > for (const auto& child_region : ...) { > index = AppendRegion(...); > if (index >= write_size_ - 1) > break; > } > > (we can probably do this in the loop in AppendRoot() too. Done. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:68: // cc:SurfaceObserver: On 2017/06/28 19:47:05, varkha wrote: > nit: add ':' in "cc::SurfaceObserver:" Done. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:102: int write_size_; On 2017/06/28 20:30:12, sadrul wrote: > Please document the size is the number of elements in the array, not the number > of bytes. Done. https://codereview.chromium.org/2938953002/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:102: int write_size_; On 2017/06/28 21:22:00, varkha wrote: > On 2017/06/28 20:30:12, sadrul wrote: > > Please document the size is the number of elements in the array, not the > number > > of bytes. > > Maybe consider a self-descriptive variable name as well although not sure what > would > work the best. Maybe read_elements_count_ / write_elements_count_ Acknowledged. I've commented but kept the use of the term size because it seems to follow the convention used in vector and other std collections. Sound ok? https://codereview.chromium.org/2938953002/diff/280001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/280001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:68: auto active_search = active_.find(surface_id); On 2017/06/28 23:36:37, sadrul wrote: > This will not work since you have already erased(), right? > > Should there be a unit-test to make sure this works correctly? Thank you for the catch. Done.
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, varkha@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2938953002/#ps300001 (title: "correct active count logic and add unit test to protect against similar errors")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
I think you also need LGTM from an owner for changes in DEPS (presubmit suggested jbauman@chromium.org) and a security owner (tsepez@google.com) for mojom to pass the chromium_presubmit trybot.
gklassen@chromium.org changed reviewers: + jbauman@chromium.org, tsepez@chromium.org
+ tsepez for security review of mojom and jbauman for review of includes dependencies on surfaces. Can you please review?
gklassen@chromium.org changed reviewers: + danakj@chromium.org
+ danakj for DEPS review.
lgtm
https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... File components/viz/common/DEPS (right): https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/DEPS:2: "+cc/surfaces", I think this is too broad. Can you replace with specific .h files?
https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... File components/viz/common/DEPS (right): https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/DEPS:2: "+cc/surfaces", On 2017/06/30 20:10:36, rjkroege wrote: > I think this is too broad. Can you replace with specific .h files? Ya I am wondering what this will be. Most of surfaces will be going into viz/service which this can't do. Some will be going in common tho. See https://docs.google.com/spreadsheets/d/1buIcuS-lgM1eIYChOrBEAo0BdlIkb7p1TF58L... So it would be nice to specify here. https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/DEPS:4: "+services/viz/public/interfaces", It seems a maybe bit off to me that viz/common would include viz interfaces. The common code for use by both the client and service implementations, it feels like a level below these interfaces. But I also don't see this being used so, is it needed? https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... File components/viz/common/display_hit_test_region.h (right): https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/display_hit_test_region.h:16: // An array of DisplayHitTestRegion elements is used to define the put this comment directly above the struct definition, so it is anchored to it. https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/display_hit_test_region.h:19: // It is designed to be in shared memory so that the viz process can viz service can write https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/display_hit_test_region.h:20: // write the hit_test data, and the browser / ws process can read without the viz host can read https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/display_hit_test_region.h:27: struct DisplayHitTestRegion { I suggest AggregatedHitTestRegion, because HitTestRegion is also somehow related to Displays, so I found it hard to understand what is the difference. https://codereview.chromium.org/2938953002/diff/300001/services/viz/public/in... File services/viz/public/interfaces/hit_test_region_list.mojom (right): https://codereview.chromium.org/2938953002/diff/300001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:12: const uint32 kHitTestMine = 0x01; I think these should be defined in the C++ and then mirrored/included into the mojom. These to me are like data types for instance gfx::Rect. We have a C++ type, which we can refer to, and a mojom which represents it. This is simpler as its constants but same idea I think. That would put them in viz/common, which is where there is C++ referring to them also, and resolves my feelings about viz/common->services/viz/public/interfaces. WDYT Rob? Put another way, since C++ types are referring to these (abstractly atm) in viz/common/, we should put these in the C++ in viz/common/?
Thank you Dana! Sharpening the viz/common DEPS worked well and resolved the viz/common->services/viz/public/interfaces dependency. The most recent patch now includes your comment improvemetns and DisplayHitTestRegion has been renamed to AggregatedHitTestRegion ( I agree that it's a more descriptive name, Thank you ). I've kept the constants definitions in the mojom - mostly because it keeps them in one place - given that the viz/common->services/viz/public/interfaces depenency is resolved will this be ok? https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... File components/viz/common/DEPS (right): https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/DEPS:2: "+cc/surfaces", On 2017/06/30 20:39:38, danakj wrote: > On 2017/06/30 20:10:36, rjkroege wrote: > > I think this is too broad. Can you replace with specific .h files? > > Ya I am wondering what this will be. Most of surfaces will be going into > viz/service which this can't do. Some will be going in common tho. See > https://docs.google.com/spreadsheets/d/1buIcuS-lgM1eIYChOrBEAo0BdlIkb7p1TF58L... > > So it would be nice to specify here. Done. https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/DEPS:4: "+services/viz/public/interfaces", On 2017/06/30 20:39:38, danakj wrote: > It seems a maybe bit off to me that viz/common would include viz interfaces. The > common code for use by both the client and service implementations, it feels > like a level below these interfaces. But I also don't see this being used so, is > it needed? Done. https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... File components/viz/common/display_hit_test_region.h (right): https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/display_hit_test_region.h:16: // An array of DisplayHitTestRegion elements is used to define the On 2017/06/30 20:39:38, danakj wrote: > put this comment directly above the struct definition, so it is anchored to it. Done. https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/display_hit_test_region.h:19: // It is designed to be in shared memory so that the viz process can On 2017/06/30 20:39:38, danakj wrote: > viz service can write Done. https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/display_hit_test_region.h:20: // write the hit_test data, and the browser / ws process can read without On 2017/06/30 20:39:38, danakj wrote: > the viz host can read Done. https://codereview.chromium.org/2938953002/diff/300001/components/viz/common/... components/viz/common/display_hit_test_region.h:27: struct DisplayHitTestRegion { On 2017/06/30 20:39:38, danakj wrote: > I suggest AggregatedHitTestRegion, because HitTestRegion is also somehow related > to Displays, so I found it hard to understand what is the difference. Done. https://codereview.chromium.org/2938953002/diff/300001/services/viz/public/in... File services/viz/public/interfaces/hit_test_region_list.mojom (right): https://codereview.chromium.org/2938953002/diff/300001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:12: const uint32 kHitTestMine = 0x01; On 2017/06/30 20:39:38, danakj wrote: > I think these should be defined in the C++ and then mirrored/included into the > mojom. These to me are like data types for instance gfx::Rect. We have a C++ > type, which we can refer to, and a mojom which represents it. This is simpler as > its constants but same idea I think. That would put them in viz/common, which is > where there is C++ referring to them also, and resolves my feelings about > viz/common->services/viz/public/interfaces. WDYT Rob? > > Put another way, since C++ types are referring to these (abstractly atm) in > viz/common/, we should put these in the C++ in viz/common/? Acknowledged.
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2938953002/diff/300001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/300001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:47: HitTestAggregator::~HitTestAggregator() {} nit: = default https://codereview.chromium.org/2938953002/diff/340001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/340001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:58: bool HitTestAggregator::OnSurfaceDamaged(const cc::SurfaceId& surface_id, nit: You'll need to rebase, I moved SurfaceId, FrameSinkId and LocalSurfaceId to viz.
The CQ bit was checked by gklassen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/07/12 14:43:52, gklassen wrote: > Thank you Dana! > > Sharpening the viz/common DEPS worked well and resolved the > viz/common->services/viz/public/interfaces dependency. > > The most recent patch now includes your comment improvemetns and > DisplayHitTestRegion has been renamed to AggregatedHitTestRegion ( I agree that > it's a more descriptive name, Thank you ). > > I've kept the constants definitions in the mojom - mostly because it keeps them > in one place - given that the viz/common->services/viz/public/interfaces > depenency is resolved will this be ok? The recommendation [1], at least for enums (and I think it applies to constants too), seems to be that we should be defining in mojom, and use them in cpp code, instead of introducing mirror cpp enums, to avoid duplication. So this should be OK. [1] https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/hqA-ORdBRUk
latest patch includes rebase and related changes. Dana and Fady can you please review dependencies?
The CQ bit was checked by gklassen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
( latest patch also addresses making HitTestAggregator destructor default ) https://codereview.chromium.org/2938953002/diff/300001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/300001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:47: HitTestAggregator::~HitTestAggregator() {} On 2017/07/12 14:46:38, Fady Samuel wrote: > nit: = default Done.
The CQ bit was checked by gklassen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2938953002/diff/420001/services/viz/public/in... File services/viz/public/interfaces/hit_test_region_list.mojom (right): https://codereview.chromium.org/2938953002/diff/420001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:5: module viz.hit_test.mojom; nit: per offline discussion with danakj, viz.mojom.
I looked at the C++ more this time and have a number of comments https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... components/viz/common/BUILD.gn:9: "aggregated_hit_test_region.h", Can you put this in common/hit_test? We're trying to avoid situations where there is code in a directory that isn't a leaf in the directory tree. https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... File components/viz/common/aggregated_hit_test_region.h (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... components/viz/common/aggregated_hit_test_region.h:26: nit: remove whitespace between comment and the thing its commenting https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... components/viz/common/aggregated_hit_test_region.h:33: // services/viz/public/interfaces/hit_test_region_list.mojom.h. i would specify the namespace+typename rather than the file path. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/OWNERS (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/OWNERS:1: rjkroege@chromium.org a single owner is a lonely place, is there anyone else that'd fit here? https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:43: : active_region_count_(0), weak_ptr_factory_(this) { you can assign the 0 in the .h file where var is declared. this makes it easier to remember to init things. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:142: CHECK(search != active_.end()); do you mean DCHECK? See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:148: (AggregatedHitTestRegion*)write_buffer_.get(); Use c++ casts. https://google.github.io/styleguide/cppguide.html#Casting https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:214: std::swap(read_handle_, write_handle_); use ADL lookup for swap, with using std::swap: https://stackoverflow.com/questions/28130671/how-does-using-stdswap-enable-adl https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:15: namespace hit_test { just use viz namespace here. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:18: class HitTestAggregatorTest; tests are found in the same namespace as the class usually, so just viz. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:27: nit: remove whitespace https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:53: friend class test::HitTestAggregatorTest; There are nicer ways to expose things to tests than a friend. A friend has 2 disadvantages: - It loses all control of the class internals, you can't decide what to expose and what not - You have to name all the tests here, so when you add a test later sometimes you have to add knowledge about it here. You could instead put things you want to expose to tests in a protected block, then in the test file make a subclass that exposes those things with "using". https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:66: int index); nit: i'd group index and regions together since it is an index into regions, and maybe name it to say that as well https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:83: using HitTestRegionListMap = nit: Now that we have auto, usually I think it's better to just write out the type on the variables instead of using a type alias like this. It leaves less guessing/indirection about performance characteristics with moves or inserts etc. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:84: std::map<SurfaceId, hit_test::mojom::HitTestRegionListPtr>; How big is this going to be, what is the usage pattern going to be like? See base/containers/README.md for some options. std::map isn't always the best choice. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:5: #include "hit_test_aggregator.h" full path from src/ the include for the file being tested gets a blank line after it. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:11: namespace viz { just viz namespace here https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:27: using namespace hit_test::mojom; do not using namespace: https://google.github.io/styleguide/cppguide.html#Namespaces https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:66: (AggregatedHitTestRegion*)aggregator_.write_buffer_.get(); c++ casts https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:194: EXPECT_EQ(GetPendingCount(), 0); It looks like all the tests here are inverted, you want this to get the right output on failure: EXPECT(expected value, actual value) https://codereview.chromium.org/2938953002/diff/420001/services/viz/public/in... File services/viz/public/interfaces/hit_test_region_list.mojom (right): https://codereview.chromium.org/2938953002/diff/420001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. These services files should all be under services/viz/hit_test/public/interfaces as per recent discussion on services-dev. https://codereview.chromium.org/2938953002/diff/420001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:5: module viz.hit_test.mojom; I think this should be just viz.mojom
new patch uploaded that should address all comments except the suggestion to eliminate the use of the friend class. Dana could you please re-review and if the change to the friend class is important please let me know and I will undertake the change. https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... components/viz/common/BUILD.gn:9: "aggregated_hit_test_region.h", On 2017/07/13 15:39:38, danakj wrote: > Can you put this in common/hit_test? > > We're trying to avoid situations where there is code in a directory that isn't a > leaf in the directory tree. It was originally in common/hit_test, but I am happy to move it back assuming this has been agreed by others. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... File components/viz/common/aggregated_hit_test_region.h (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... components/viz/common/aggregated_hit_test_region.h:26: On 2017/07/13 15:39:38, danakj wrote: > nit: remove whitespace between comment and the thing its commenting Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/common/... components/viz/common/aggregated_hit_test_region.h:33: // services/viz/public/interfaces/hit_test_region_list.mojom.h. On 2017/07/13 15:39:38, danakj wrote: > i would specify the namespace+typename rather than the file path. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/OWNERS (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/OWNERS:1: rjkroege@chromium.org On 2017/07/13 15:39:38, danakj wrote: > a single owner is a lonely place, is there anyone else that'd fit here? There were more, but other suitable candidates are not yet contributors and are not qualified to be OWNERS ( like me :). https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:43: : active_region_count_(0), weak_ptr_factory_(this) { On 2017/07/13 15:39:38, danakj wrote: > you can assign the 0 in the .h file where var is declared. this makes it easier > to remember to init things. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:142: CHECK(search != active_.end()); On 2017/07/13 15:39:38, danakj wrote: > do you mean DCHECK? > > See > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... The use of CHECK was intentional and based on conversation with others - but perhaps without alot of thought - is DCHECK more appropriate? The root surface should be in the list when this is called. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:148: (AggregatedHitTestRegion*)write_buffer_.get(); On 2017/07/13 15:39:38, danakj wrote: > Use c++ casts. https://google.github.io/styleguide/cppguide.html#Casting Good call. Thanks. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:214: std::swap(read_handle_, write_handle_); On 2017/07/13 15:39:38, danakj wrote: > use ADL lookup for swap, with using std::swap: > https://stackoverflow.com/questions/28130671/how-does-using-stdswap-enable-adl Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:15: namespace hit_test { On 2017/07/13 15:39:38, danakj wrote: > just use viz namespace here. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:18: class HitTestAggregatorTest; On 2017/07/13 15:39:38, danakj wrote: > tests are found in the same namespace as the class usually, so just viz. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:27: On 2017/07/13 15:39:39, danakj wrote: > nit: remove whitespace Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:53: friend class test::HitTestAggregatorTest; On 2017/07/13 15:39:39, danakj wrote: > There are nicer ways to expose things to tests than a friend. A friend has 2 > disadvantages: > - It loses all control of the class internals, you can't decide what to expose > and what not > - You have to name all the tests here, so when you add a test later sometimes > you have to add knowledge about it here. > > You could instead put things you want to expose to tests in a protected block, > then in the test file make a subclass that exposes those things with "using". Thank you. I've looked at this a few times and there are many things that the tests currently inspect that I would need to add to the protected block. Given the limited scope would it be ok to use the friend class here? https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:66: int index); On 2017/07/13 15:39:38, danakj wrote: > nit: i'd group index and regions together since it is an index into regions, and > maybe name it to say that as well Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:83: using HitTestRegionListMap = On 2017/07/13 15:39:39, danakj wrote: > nit: Now that we have auto, usually I think it's better to just write out the > type on the variables instead of using a type alias like this. It leaves less > guessing/indirection about performance characteristics with moves or inserts > etc. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:84: std::map<SurfaceId, hit_test::mojom::HitTestRegionListPtr>; On 2017/07/13 15:39:38, danakj wrote: > How big is this going to be, what is the usage pattern going to be like? > > See base/containers/README.md for some options. std::map isn't always the best > choice. Thank you. After review std::map still seems like a suitable choice - at least until we can review size and usage patterns in context at which time base::flat\_map and base::small\_map may offer some improvement. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:5: #include "hit_test_aggregator.h" On 2017/07/13 15:39:39, danakj wrote: > full path from src/ > > the include for the file being tested gets a blank line after it. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:11: namespace viz { On 2017/07/13 15:39:39, danakj wrote: > just viz namespace here Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:27: using namespace hit_test::mojom; On 2017/07/13 15:39:39, danakj wrote: > do not using namespace: > https://google.github.io/styleguide/cppguide.html#Namespaces Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:66: (AggregatedHitTestRegion*)aggregator_.write_buffer_.get(); On 2017/07/13 15:39:39, danakj wrote: > c++ casts Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:194: EXPECT_EQ(GetPendingCount(), 0); On 2017/07/13 15:39:39, danakj wrote: > It looks like all the tests here are inverted, you want this to get the right > output on failure: > > EXPECT(expected value, actual value) > It did seem to get the right output - currently on failure the output reads Expected: GetPendingCount() Which is: 1 To be equal to: 0 Which makes sense during debug. But I see now that the convention seems to be to use the inverse and it's not really important to me, so I am happy to conform. Done. https://codereview.chromium.org/2938953002/diff/420001/services/viz/public/in... File services/viz/public/interfaces/hit_test_region_list.mojom (right): https://codereview.chromium.org/2938953002/diff/420001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/07/13 15:39:39, danakj wrote: > These services files should all be under services/viz/hit_test/public/interfaces > as per recent discussion on services-dev. Done. https://codereview.chromium.org/2938953002/diff/420001/services/viz/public/in... services/viz/public/interfaces/hit_test_region_list.mojom:5: module viz.hit_test.mojom; On 2017/07/13 15:15:31, Fady Samuel wrote: > nit: per offline discussion with danakj, viz.mojom. Done.
https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:194: EXPECT_EQ(GetPendingCount(), 0); On 2017/07/13 19:59:14, gklassen wrote: > On 2017/07/13 15:39:39, danakj wrote: > > It looks like all the tests here are inverted, you want this to get the right > > output on failure: > > > > EXPECT(expected value, actual value) > > > > It did seem to get the right output - currently on failure the output reads > > Expected: GetPendingCount() > Which is: 1 > To be equal to: 0 > > Which makes sense during debug. But I see now that the convention seems to be > to use the inverse and it's not really important to me, so I am happy to > conform. > > Done. Maybe gtest has changed... O_o It used to say blah failure blah line 20: Expected: 1 Actual: 0 If you ordered it this way. I guess you should do it the way that gets the right output, and all our older tests do it wrong now :/
https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:194: EXPECT_EQ(GetPendingCount(), 0); On 2017/07/13 20:10:30, danakj wrote: > On 2017/07/13 19:59:14, gklassen wrote: > > On 2017/07/13 15:39:39, danakj wrote: > > > It looks like all the tests here are inverted, you want this to get the > right > > > output on failure: > > > > > > EXPECT(expected value, actual value) > > > > > > > It did seem to get the right output - currently on failure the output reads > > > > Expected: GetPendingCount() > > Which is: 1 > > To be equal to: 0 > > > > Which makes sense during debug. But I see now that the convention seems to be > > to use the inverse and it's not really important to me, so I am happy to > > conform. > > > > Done. > > Maybe gtest has changed... O_o It used to say > > blah failure blah line 20: > Expected: 1 > Actual: 0 > > If you ordered it this way. I guess you should do it the way that gets the right > output, and all our older tests do it wrong now :/ Is it this (was news to me as well): https://chromium.googlesource.com/external/github.com/google/googletest.git/+... ?
The CQ bit was checked by gklassen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On Thu, Jul 13, 2017 at 4:29 PM, <varkha@chromium.org> wrote: > > https://codereview.chromium.org/2938953002/diff/420001/ > components/viz/service/hit_test/hit_test_aggregator_unittest.cc > File components/viz/service/hit_test/hit_test_aggregator_unittest.cc > (right): > > https://codereview.chromium.org/2938953002/diff/420001/ > components/viz/service/hit_test/hit_test_aggregator_unittest.cc#newcode194 > components/viz/service/hit_test/hit_test_aggregator_unittest.cc:194: > EXPECT_EQ(GetPendingCount(), 0); > On 2017/07/13 20:10:30, danakj wrote: > > On 2017/07/13 19:59:14, gklassen wrote: > > > On 2017/07/13 15:39:39, danakj wrote: > > > > It looks like all the tests here are inverted, you want this to > get the > > right > > > > output on failure: > > > > > > > > EXPECT(expected value, actual value) > > > > > > > > > > It did seem to get the right output - currently on failure the > output reads > > > > > > Expected: GetPendingCount() > > > Which is: 1 > > > To be equal to: 0 > > > > > > Which makes sense during debug. But I see now that the convention > seems to be > > > to use the inverse and it's not really important to me, so I am > happy to > > > conform. > > > > > > Done. > > > > Maybe gtest has changed... O_o It used to say > > > > blah failure blah line 20: > > Expected: 1 > > Actual: 0 > > > > If you ordered it this way. I guess you should do it the way that gets > the right > > output, and all our older tests do it wrong now :/ > > Is it this (was news to me as well): > https://chromium.googlesource.com/external/github.com/ > google/googletest.git/+/f364e188372e489230ef4e44e1aec6bcb08f3acf > ? > Interesting, looks like it! > > https://codereview.chromium.org/2938953002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:142: CHECK(search != active_.end()); On 2017/07/13 19:59:14, gklassen wrote: > On 2017/07/13 15:39:38, danakj wrote: > > do you mean DCHECK? > > > > See > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > The use of CHECK was intentional and based on conversation with others - but > perhaps without alot of thought - is DCHECK more appropriate? The root surface > should be in the list when this is called. Unless code outside your control is going to sometimes trigger it, or you're tracking down a known bug, or protecting against a security problem in difficult-to-control code, I don't think CHECK is right. I don't think this code matches any of thsoe. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:53: friend class test::HitTestAggregatorTest; On 2017/07/13 19:59:14, gklassen wrote: > On 2017/07/13 15:39:39, danakj wrote: > > There are nicer ways to expose things to tests than a friend. A friend has 2 > > disadvantages: > > - It loses all control of the class internals, you can't decide what to expose > > and what not > > - You have to name all the tests here, so when you add a test later sometimes > > you have to add knowledge about it here. > > > > You could instead put things you want to expose to tests in a protected block, > > then in the test file make a subclass that exposes those things with "using". > > Thank you. I've looked at this a few times and there are many things that the > tests currently inspect that I would need to add to the protected block. Given > the limited scope would it be ok to use the friend class here? I wouldn't block you from this, but I just don't recommend it. FWIW Another pattern instead of protected would be to friend a HitTestAggregatorTestApi or something, and it exposes by pass-thru instead of inheritance the things you want to expose. This again doesn't require updating the production code when you add a new test suite. But it does lose control of what the tests access, so it's a bit of a middle solution. https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:95: int read_size_; Can you init these? https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:5: #include "components/viz/service/hit_test/hit_test_aggregator.h" add empty line below this https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:65: regions = (AggregatedHitTestRegion*)aggregator_.read_buffer_.get(); Please look for more c-style casts https://codereview.chromium.org/2938953002/diff/460001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/OWNERS (right): https://codereview.chromium.org/2938953002/diff/460001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/OWNERS:1: per-file *.mojom=set noparent Do you wanna add rob as owner here (not per-file rule) so people can find who to ask about the code here even if he can't give final lgtms
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, sadrul@chromium.org, riajiang@chromium.org, tsepez@chromium.org, varkha@chromium.org, fsamuel@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2938953002/#ps500001 (title: "remove unnecessary include added at rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you again Dana, and everyone - this change is much better because of your input. The latest patch includes rebase changes and improvements based on these comments. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.cc:142: CHECK(search != active_.end()); On 2017/07/14 15:27:37, danakj wrote: > On 2017/07/13 19:59:14, gklassen wrote: > > On 2017/07/13 15:39:38, danakj wrote: > > > do you mean DCHECK? > > > > > > See > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > The use of CHECK was intentional and based on conversation with others - but > > perhaps without alot of thought - is DCHECK more appropriate? The root > surface > > should be in the list when this is called. > > Unless code outside your control is going to sometimes trigger it, or you're > tracking down a known bug, or protecting against a security problem in > difficult-to-control code, I don't think CHECK is right. I don't think this code > matches any of thsoe. Thank you for the explanation and detail - it's very helpful. Done. https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/420001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:53: friend class test::HitTestAggregatorTest; On 2017/07/14 15:27:37, danakj wrote: > On 2017/07/13 19:59:14, gklassen wrote: > > On 2017/07/13 15:39:39, danakj wrote: > > > There are nicer ways to expose things to tests than a friend. A friend has 2 > > > disadvantages: > > > - It loses all control of the class internals, you can't decide what to > expose > > > and what not > > > - You have to name all the tests here, so when you add a test later > sometimes > > > you have to add knowledge about it here. > > > > > > You could instead put things you want to expose to tests in a protected > block, > > > then in the test file make a subclass that exposes those things with > "using". > > > > Thank you. I've looked at this a few times and there are many things that the > > tests currently inspect that I would need to add to the protected block. > Given > > the limited scope would it be ok to use the friend class here? > > I wouldn't block you from this, but I just don't recommend it. > > FWIW Another pattern instead of protected would be to friend a > HitTestAggregatorTestApi or something, and it exposes by pass-thru instead of > inheritance the things you want to expose. This again doesn't require updating > the production code when you add a new test suite. But it does lose control of > what the tests access, so it's a bit of a middle solution. Thank you. Using a friend class bothered Rob as well and so I've made the change as you originally suggested and it is cleaner. Done. https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator.h (right): https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator.h:95: int read_size_; On 2017/07/14 15:27:37, danakj wrote: > Can you init these? Done. https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:5: #include "components/viz/service/hit_test/hit_test_aggregator.h" On 2017/07/14 15:27:37, danakj wrote: > add empty line below this Done. https://codereview.chromium.org/2938953002/diff/460001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:65: regions = (AggregatedHitTestRegion*)aggregator_.read_buffer_.get(); On 2017/07/14 15:27:37, danakj wrote: > Please look for more c-style casts ( thanks, and sorry - I should have already done that after your last comment ) Done. https://codereview.chromium.org/2938953002/diff/460001/services/viz/hit_test/... File services/viz/hit_test/public/interfaces/OWNERS (right): https://codereview.chromium.org/2938953002/diff/460001/services/viz/hit_test/... services/viz/hit_test/public/interfaces/OWNERS:1: per-file *.mojom=set noparent On 2017/07/14 15:27:37, danakj wrote: > Do you wanna add rob as owner here (not per-file rule) so people can find who to > ask about the code here even if he can't give final lgtms Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks, that looks good https://codereview.chromium.org/2938953002/diff/500001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/500001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:28: TestHitTestAggregator() {} nit: = default instead if {} when possible https://codereview.chromium.org/2938953002/diff/500001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:71: HitTestAggregatorTest() {} same
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, sadrul@chromium.org, riajiang@chromium.org, danakj@chromium.org, tsepez@chromium.org, fsamuel@chromium.org, varkha@chromium.org Link to the patchset: https://codereview.chromium.org/2938953002/#ps540001 (title: "correct mojom include directory and improvements based on reviewer comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks again. I had to correct the mojom include directory to fix the build ( my local builds were using the old files I guess ) so it gave me a chance to address these as well. https://codereview.chromium.org/2938953002/diff/500001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2938953002/diff/500001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:28: TestHitTestAggregator() {} On 2017/07/14 17:15:14, danakj wrote: > nit: = default instead if {} when possible Done. https://codereview.chromium.org/2938953002/diff/500001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:71: HitTestAggregatorTest() {} On 2017/07/14 17:15:14, danakj wrote: > same Done.
CQ is committing da patch.
Bot data: {"patchset_id": 540001, "attempt_start_ts": 1500053620150430,
"parent_rev": "1db86a05f5f29e5703136d9b46a5e60443c5897c", "commit_rev":
"57cb4a50cf2c5e3a2c7015a4d335aa894c982338"}
Message was sent while issue was closed.
Description was changed from ========== Implement HitTestAggregator Implements a HitTestAggregator component to facilitate cross process hit testing with minimal process hops. The HitTestAggregator maintains a list of HitTestData objects that will be created and sent with each CompositorFrame. These are aggregated into a single DisplayHitTest data structure that can be made available in shared memory to facilitate hit testing with minimal process hops for any Display. HitTestAggregator listens as surfaces are added to the current CompositorFrame to ensure that the list only includes information that matches what has been rendered in the current frame. Aggregation occurs on the BeginFrame notification. This change includes the implementaiton of the HitTestAggregator and unit test cases to verify the intended behaviour. Next steps will include completion of the creation and transport of HitTestData objects, implementation of shared memory and using this component to target incoming events. BUG=732399 ========== to ========== Implement HitTestAggregator Implements a HitTestAggregator component to facilitate cross process hit testing with minimal process hops. The HitTestAggregator maintains a list of HitTestData objects that will be created and sent with each CompositorFrame. These are aggregated into a single DisplayHitTest data structure that can be made available in shared memory to facilitate hit testing with minimal process hops for any Display. HitTestAggregator listens as surfaces are added to the current CompositorFrame to ensure that the list only includes information that matches what has been rendered in the current frame. Aggregation occurs on the BeginFrame notification. This change includes the implementaiton of the HitTestAggregator and unit test cases to verify the intended behaviour. Next steps will include completion of the creation and transport of HitTestData objects, implementation of shared memory and using this component to target incoming events. BUG=732399 Review-Url: https://codereview.chromium.org/2938953002 Cr-Commit-Position: refs/heads/master@{#486852} Committed: https://chromium.googlesource.com/chromium/src/+/57cb4a50cf2c5e3a2c7015a4d335... ==========
Message was sent while issue was closed.
Committed patchset #28 (id:540001) as https://chromium.googlesource.com/chromium/src/+/57cb4a50cf2c5e3a2c7015a4d335... |
