|
|
DescriptionAdd viz-host HitTestQuery.
HitTestQuery finds the hit test target, for a given location, in the
AggregatedHitTestRegion list received from HitTestAggregator.
BUG=732400
TEST=viz_unittests
Review-Url: https://codereview.chromium.org/2933493003
Cr-Commit-Position: refs/heads/master@{#488854}
Committed: https://chromium.googlesource.com/chromium/src/+/537141c4ede41728e1cf026f2967f0838f1e51de
Patch Set 1 #
Total comments: 18
Patch Set 2 : update #
Total comments: 38
Patch Set 3 : comment #6 #7 #
Total comments: 10
Patch Set 4 : recursion; comment#10 #
Total comments: 29
Patch Set 5 : comments # 16 17 18 #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : tests #Patch Set 8 : rebase and small changes #Patch Set 9 : fix build error #Patch Set 10 : init flags value #Patch Set 11 : build file #
Total comments: 28
Patch Set 12 : comments 48-50 #
Total comments: 2
Patch Set 13 : more check for |child_count| #
Total comments: 2
Patch Set 14 : rebase #Patch Set 15 : const; std int min #Patch Set 16 : int32 #Patch Set 17 : don't combine uint and int; check for overflow #
Total comments: 4
Messages
Total messages: 87 (53 generated)
riajiang@chromium.org changed reviewers: + gklassen@chromium.org, rjkroege@chromium.org, sadrul@chromium.org
Hi all, this CL still has some TODOs related to https://codereview.chromium.org/2908783002/ and needs more test cases, but I thought I would just upload it first so you guys can take a look in case I'm misunderstanding something. Thanks!
it's the right general idea. My primary concern is that you need to refine the matrix updating. https://codereview.chromium.org/2933493003/diff/1/components/viz/frame_sinks/... File components/viz/frame_sinks/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/frame_sinks/... components/viz/frame_sinks/BUILD.gn:38: "//components/viz/hit_test", must establish layering conventions I think. https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/BUI... File components/viz/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/BUI... components/viz/hit_test/BUILD.gn:7: # TODO(riajiang): Integrate with https://codereview.chromium.org/2908783002/. One of you needs to land first. :-) https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/com... File components/viz/hit_test/common/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/com... components/viz/hit_test/common/display_hit_test_data.h:15: // TODO(riajiang): Integrate with https://codereview.chromium.org/2908783002/. you need to rebase https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... File components/viz/hit_test/query/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.cc:19: for (uint32_t i = 0; i < display_hit_test_data_list_.size(); ++i) { there's a way to make a vector using a pre-defined buffer. might be worth doing. https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.cc:23: DCHECK(data.transform.TransformPointReverse( the statement inside the DCHECK is going to not execute in release. Please define and document your expectations about the matrices. Also, where is the code that multiplies the matrices together? https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... File components/viz/hit_test/query/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.h:28: class HIT_TEST_EXPORT HitTestQuery { this needs eventually to dangle off of GpuHost. And you will multiple of them. One per display. And they can come and go as displays are added and removed. https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.h:35: void set_display_hit_test_data_list( this is fakery that will go away? https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.h:43: Target FindTargetForLocation(const gfx::Point& location_in_root); I may quibble at you about passing Target instances by value. Do they need to mutable? One might return a const& if only one thread is accessing this class at a time. https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... File components/viz/hit_test/query/hit_test_query_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query_unittest.cc:50: {e_id, e_bounds_in_e, transform_e_to_e, 1 /* HIT_TEST_MINE */, 3}, // e realistic transforms? https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query_unittest.cc:57: // All points are in display coordinate systems. need to be in their respective coordinate systes.
https://codereview.chromium.org/2933493003/diff/1/components/viz/frame_sinks/... File components/viz/frame_sinks/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/frame_sinks/... components/viz/frame_sinks/BUILD.gn:38: "//components/viz/hit_test", On 2017/06/12 17:17:23, rjkroege wrote: > must establish layering conventions I think. This is only to run the HitTestQuery code, I think once it's actually being used by other code we don't need this anymore? https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/com... File components/viz/hit_test/common/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/com... components/viz/hit_test/common/display_hit_test_data.h:15: // TODO(riajiang): Integrate with https://codereview.chromium.org/2908783002/. On 2017/06/12 17:17:24, rjkroege wrote: > you need to rebase Do you mean with the new client/host dir? Should HitTestQuery go to components/viz/host/hit_test? https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... File components/viz/hit_test/query/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.cc:23: DCHECK(data.transform.TransformPointReverse( On 2017/06/12 17:17:24, rjkroege wrote: > the statement inside the DCHECK is going to not execute in release. Please > define and document your expectations about the matrices. Also, where is the > code that multiplies the matrices together? Ah I see! I'll move the transform out of DCHECK and just DCHECK the result. The point would be transformed to the correct coordinate system after every walk? So we don't really need to keep a stack of all the matrices? Or do you mean we need the matrices for something else? https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... File components/viz/hit_test/query/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.h:28: class HIT_TEST_EXPORT HitTestQuery { On 2017/06/12 17:17:24, rjkroege wrote: > this needs eventually to dangle off of GpuHost. And you will multiple of them. > One per display. And they can come and go as displays are added and removed. Should GpuHost be the one that creates both the shmem and HitTestQuery? https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.h:35: void set_display_hit_test_data_list( On 2017/06/12 17:17:24, rjkroege wrote: > this is fakery that will go away? Yes. Once shmem is set up, we'll just read from there. https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query.h:43: Target FindTargetForLocation(const gfx::Point& location_in_root); On 2017/06/12 17:17:24, rjkroege wrote: > I may quibble at you about passing Target instances by value. Do they need to > mutable? One might return a const& if only one thread is accessing this class at > a time. True! They are not going to be mutable, changing to return a const&. https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... File components/viz/hit_test/query/hit_test_query_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query_unittest.cc:50: {e_id, e_bounds_in_e, transform_e_to_e, 1 /* HIT_TEST_MINE */, 3}, // e On 2017/06/12 17:17:24, rjkroege wrote: > realistic transforms? In this case, bounds of e and c are in the coordinate system of e, and bounds of a and b are in the coordinate system of c? This means transform_c_to_e would be the only one that's not identity? https://codereview.chromium.org/2933493003/diff/1/components/viz/hit_test/que... components/viz/hit_test/query/hit_test_query_unittest.cc:57: // All points are in display coordinate systems. On 2017/06/12 17:17:24, rjkroege wrote: > need to be in their respective coordinate systes. I thought that when the point first comes in, they are in display coordinates, and after we find the frame sink the point is going to, point would only then be updated to be in their own coordinate systems? Did I misunderstand something?
Updated the patch. Could you take another look? Thanks!
Some quick comments ( but not a comprehensive review ). https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:31: gfx::Point location(tentative_target_so_far.location_in_target); What happens if we transform a point for a child region that doesn't match? Eg. If A embeds B which embeds C and we test a point that overlaps all three but does not match A or B - then the target is A and we need to return the point as transformed to the coordinate space of A. We may need to use a recursive call or stack to handle this? https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:23: int flags; flags are now a uint32 instead of HitTesRegionFlags in 2908783002 as well ( it seemed to be necessary to enable or'ing of multiple flags ) - so this can likely stay but you may want to use a uint32 to match.
https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/h... File components/viz/common/hit_test/DEPS (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/h... components/viz/common/hit_test/DEPS:2: "+cc/surfaces", This seems too big. There is a bunch of stuff in cc/surfaces that should be deemed an internal detail. Can you add only surface_id.h? https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:16: struct DisplayHitTestData { I'd call this a DisplayHitTestDataRegion where the DisplayHitTestData is (effectively) a vector them. You should presume a class that provides an array of DisplayHitTestDataRegions and is a work-alike for a vector (but would be backed by a shared memory buffer.) https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:16: // 1. The list is in descending z-order. so: larger indicies mean closer to the eyepoint? https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:17: // 2. Children entries have their own FrameSinkId, but those that are just entries here mean "region"? https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:20: // 4. After applying transform to the incoming point, point is in the same you need ascii art. Or an associated .md file with pictures. These need more specificity. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:26: Target confirmed_target_so_far; You should use Target* instead of copying whole Target objects. Target can be a class. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:28: tentative_target_so_far.location_in_target = location_in_root; I'd put a blank line here. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:29: for (uint32_t i = 0; i < display_hit_test_data_list_.size(); ++i) { use an iterator https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:36: tentative_target_so_far.location_in_target = location; I think it's safe to begin by thinking about this in a purely 2d context but this needs additional thought in a 3d context. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:40: // HIT_TEST_MINE is set to be true, which means it can receive events. if you encounter a region with hit_test_mine set, you should exit? https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:20: gfx::Point location_in_target; the coordinate system of the target FrameSinkId... https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:43: Target FindTargetForLocation(const gfx::Point& location_in_root); You have written the code that does the event dispatching right? Where will the code that asks blink for help? Is this part of the public API? You need to explain this to me https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:28: // One surface. this is coming very well. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:39: {e_id, gfx::Rect(0, 0, 600, 600), gfx::Transform(), 1 /* HIT_TEST_MINE */, I would find it easier to read if you made this one line like some of the entries below. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:44: // All points are in e's coordinate system when we reach this case. is 0,0 in the region? https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:50: EXPECT_EQ(point1.ToString(), target1.location_in_target.ToString()); Doesn't Point's == operator mean that you don't need the .ToString() https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:178: transform_e_to_c1.Translate(-100, -100); we need unit tests that handle 3space transformations. It is conceivable that this extension should go in a subsequent CL. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:220: // | +c---------| Point maps to so... say we have a a d region inside area "3". below a and b...
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/h... File components/viz/common/hit_test/DEPS (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/h... components/viz/common/hit_test/DEPS:2: "+cc/surfaces", On 2017/06/15 19:58:35, rjkroege wrote: > This seems too big. There is a bunch of stuff in cc/surfaces that should be > deemed an internal detail. Can you add only surface_id.h? Changed to add "cc/surfaces/frame_sink_id.h". https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:16: struct DisplayHitTestData { On 2017/06/15 19:58:35, rjkroege wrote: > I'd call this a DisplayHitTestDataRegion where the DisplayHitTestData is > (effectively) a vector them. You should presume a class that provides an array > of DisplayHitTestDataRegions and is a work-alike for a vector (but would be > backed by a shared memory buffer.) Done. Changed DisplayHitTestData to be a vector of DisplayHitTestDataRegion. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:16: // 1. The list is in descending z-order. On 2017/06/15 19:58:35, rjkroege wrote: > so: larger indicies mean closer to the eyepoint? Yes, I'm assuming that for now for the correct transform and child count. Once that's updated for ascending z-order I'll update the code. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:17: // 2. Children entries have their own FrameSinkId, but those that are just On 2017/06/15 19:58:35, rjkroege wrote: > entries here mean "region"? Yes. Updated. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:20: // 4. After applying transform to the incoming point, point is in the same On 2017/06/15 19:58:35, rjkroege wrote: > you need ascii art. Or an associated .md file with pictures. These need more > specificity. Added ascii art example for now. Maybe will move all these assumptions to a doc later? https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:26: Target confirmed_target_so_far; On 2017/06/15 19:58:35, rjkroege wrote: > You should use Target* instead of copying whole Target objects. > > Target can be a class. Do you mean confirmed_target_so_far is copying the value of tentative_target_so_far? We do need to copy the value in this case? If they are pointers then they would point to the same object but we don't want to update confirmed_target_so_far every time? https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:28: tentative_target_so_far.location_in_target = location_in_root; On 2017/06/15 19:58:35, rjkroege wrote: > I'd put a blank line here. Done. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:29: for (uint32_t i = 0; i < display_hit_test_data_list_.size(); ++i) { On 2017/06/15 19:58:35, rjkroege wrote: > use an iterator Done. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:31: gfx::Point location(tentative_target_so_far.location_in_target); On 2017/06/15 19:51:20, gklassen wrote: > What happens if we transform a point for a child region that doesn't match? Eg. > If A embeds B which embeds C and we test a point that overlaps all three but > does not match A or B - then the target is A and we need to return the point as > transformed to the coordinate space of A. > > We may need to use a recursive call or stack to handle this? Do you maybe mean that the point P does not match *C* or B and the target should be A? This location conversion is for comparing the point with bounds (so they are in the same coordinate system), but we only update the location if it's actually inside the bounds (line 36)? I think I'll need to update this once we change it to deal with front-to-back z-order. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:40: // HIT_TEST_MINE is set to be true, which means it can receive events. On 2017/06/15 19:58:35, rjkroege wrote: > if you encounter a region with hit_test_mine set, you should exit? We should also check child_count? If a region has hit_test_mine but still has children left then we want to keep walking? https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:20: gfx::Point location_in_target; On 2017/06/15 19:58:36, rjkroege wrote: > the coordinate system of the target FrameSinkId... Done. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:23: int flags; On 2017/06/15 19:51:20, gklassen wrote: > flags are now a uint32 instead of HitTesRegionFlags in 2908783002 as well ( it > seemed to be necessary to enable or'ing of multiple flags ) - so this can likely > stay but you may want to use a uint32 to match. Ah I see, changed to uint32. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:43: Target FindTargetForLocation(const gfx::Point& location_in_root); On 2017/06/15 19:58:36, rjkroege wrote: > You have written the code that does the event dispatching right? > > Where will the code that asks blink for help? Is this part of the public API? > You need to explain this to me https://cs.chromium.org/chromium/src/services/ui/ws/event_targeter.cc?type=cs... is where I think we can put all these. It would first try to get the FrameSinkId from HitTestQuery and if not successful, do an async call to ask Blink for help. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:28: // One surface. On 2017/06/15 19:58:36, rjkroege wrote: > this is coming very well. :) https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:39: {e_id, gfx::Rect(0, 0, 600, 600), gfx::Transform(), 1 /* HIT_TEST_MINE */, On 2017/06/15 19:58:36, rjkroege wrote: > I would find it easier to read if you made this one line like some of the > entries below. Done. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:44: // All points are in e's coordinate system when we reach this case. On 2017/06/15 19:58:36, rjkroege wrote: > is 0,0 in the region? Ha I thought it's like 600,600 but it is in the region! It's on the bounds but because of the way Rect::Contains (https://cs.chromium.org/chromium/src/ui/gfx/geometry/rect.cc?l=163) works, we are counting it as in the region. Added in the tests. https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:50: EXPECT_EQ(point1.ToString(), target1.location_in_target.ToString()); On 2017/06/15 19:58:36, rjkroege wrote: > Doesn't Point's == operator mean that you don't need the .ToString() Yes! And it's not needed for FrameSinkId as well so deleted all ToString(). https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query_unittest.cc:220: // | +c---------| Point maps to On 2017/06/15 19:58:36, rjkroege wrote: > so... say we have a a d region inside area "3". below a and b... If it's back-to-front (what I'm assuming for now), then it would be e,d,c,a,b which should just work. If it's front-to-back, then as discussed today each DisplayHitTestRegion entry should have the correct transform already so we can just apply that transform each time without keeping a stack? Added this test case.
https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/BUILD.gn:10: "components/viz/common/display_hit_test_data.h", I assume you meant //components...? (it's weird that you are not getting errors from this). However, you should not include sources from other directories in here. Instead, some target in //components/viz/common/BUILD.gn should include this file as a source, and this target should have a dep on that target. https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:46: it != display_hit_test_data_list_.end(); ++it) { Use for (const auto& region : display_hit_test_data_list_) { ... } https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:56: // HIT_TEST_MINE is set to be true, which means it can receive events. I would be OK with starting with a different set of flags in this CL. We can merge once both CLs are landed. But we don't need to block on the other CL in my opinion. https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:58: confirmed_target_so_far = tentative_target_so_far; It's not clear to me yet how kHitTestIgnore flag would be used here. https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:21: uint32_t flags; Add a comment to point to where the flags are defined. https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:35: display_hit_test_data_list_ = display_hit_test_data_list; Use std::move
Also, make sure to update the CL description, and link to a bug (and the bug should then link back to the design doc).
Description was changed from ========== Add HitTestQuery to query through a list of DisplayHitTestData. BUG= ========== to ========== Add HitTestQuery to go through DisplayHitTestData to get Target for points. BUG=732400 TEST=viz_hit_test_unittests ==========
Description was changed from ========== Add HitTestQuery to go through DisplayHitTestData to get Target for points. BUG=732400 TEST=viz_hit_test_unittests ========== to ========== Add HitTestQuery to go through DisplayHitTestData to get Target for points. BUG=732400 TEST=viz_hit_test_host_unittests ==========
Also updated tree walking to be recursive. https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/BUILD.gn:10: "components/viz/common/display_hit_test_data.h", On 2017/06/16 17:09:51, sadrul wrote: > I assume you meant //components...? (it's weird that you are not getting errors > from this). > > However, you should not include sources from other directories in here. Instead, > some target in //components/viz/common/BUILD.gn should include this file as a > source, and this target should have a dep on that target. Yea I meant //components. Added BUILD.gn to components/viz/common/hit_test and dep on that from here. https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:58: confirmed_target_so_far = tentative_target_so_far; On 2017/06/16 17:09:51, sadrul wrote: > It's not clear to me yet how kHitTestIgnore flag would be used here. If it's kHitTestIgnore, then that child cannot receive any events (just a bounding box). https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:21: uint32_t flags; On 2017/06/16 17:09:51, sadrul wrote: > Add a comment to point to where the flags are defined. Done. https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:35: display_hit_test_data_list_ = display_hit_test_data_list; On 2017/06/16 17:09:51, sadrul wrote: > Use std::move Done.
varkha@chromium.org changed reviewers: + varkha@chromium.org
Just a few nits, looking at the data structures. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:18: cc::FrameSinkId id; nit: whitespace after. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:19: // |location_in_target| is in the coordinate system of the target FrameSinkId. nit: maybe just "Coordinates in the coordinate system ...", no need to repeat the member name. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:20: gfx::Point location_in_target; nit: whitespace after. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:22: // hit_test_data.mojom. Is this in services/viz/hit_test/public/interfaces/hit_test_data.mojom in [1]? [1] = https://codereview.chromium.org/2938953002 https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:41: // Find Target for |location_in_root|, including the FrameSinkId of the nit: Finds
https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:5: #ifndef COMPONENTS_VIZ_COMMON_HIT_TEST_DISPLAY_HIT_TEST_DATA_H_ flatten out the subdir. i.e. this goes in components/viz/common/* afaik. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/BUILD.gn:21: test("viz_hit_test_host_unittests") { afaik, needs to be a source set as is part of components unittest
https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:25: using DisplayHitTestData = std::vector<DisplayHitTestDataRegion>; If you follow the general convention in most chrome code, then I think this should be DisplayHitTestDataRegionList. i.e. using DisplayHitTestRegionList = std::vector<DisplayHitTestDataRegion>; (see, for example, cc::LayerList, cc::RenderPassList, ui::EventHandlerList etc.) Although, I would go with DisplayHitTestData, and DisplayHitTestDataList. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/OWNERS (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/OWNERS:3: riajiang@chromium.org Instead of using same list of OWNERS, use file:/// to refer to the other OWNERS https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:48: gfx::Point location_transformed(location_in_region); Can you document in the header file what the coordinate space of |location_in_region| is? The name |location_in_region| makes me think it is already in |region|'s coordinate space. If it is, then why do we need to transform using region's transform? https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:50: region->transform.TransformPoint(&location_transformed); TransformPoint() already early-outs for identity transforms. No need to check it first here. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:56: DisplayHitTestDataRegion* child_region_end = region + region->child_count; This code makes me nervous. We are reading data generated by a potentially compromised viz process. We need to check the bounds before we read. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:20: gfx::Point location_in_target; Might make sense to make this a gfx::PointF. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:26: // Finds the target for a given location based on DisplayHitTestData aggreated *aggregated https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:51: Target* target); Should this be a const method?
https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/h... File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:5: #ifndef COMPONENTS_VIZ_COMMON_HIT_TEST_DISPLAY_HIT_TEST_DATA_H_ On 2017/06/27 00:10:52, rjkroege wrote: > flatten out the subdir. i.e. this goes in components/viz/common/* afaik. Done. https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/h... components/viz/common/hit_test/display_hit_test_data.h:25: using DisplayHitTestData = std::vector<DisplayHitTestDataRegion>; On 2017/06/27 03:37:54, sadrul wrote: > If you follow the general convention in most chrome code, then I think this > should be DisplayHitTestDataRegionList. i.e. > > using DisplayHitTestRegionList = std::vector<DisplayHitTestDataRegion>; > > (see, for example, cc::LayerList, cc::RenderPassList, ui::EventHandlerList etc.) > > Although, I would go with DisplayHitTestData, and DisplayHitTestDataList. I see, changed to DisplayHitTestData and DisplayHitTestDataList https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/BUILD.gn:21: test("viz_hit_test_host_unittests") { On 2017/06/27 00:10:52, rjkroege wrote: > afaik, needs to be a source set as is part of components unittest I see, changed to be part of components_unittests https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/OWNERS (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/OWNERS:3: riajiang@chromium.org On 2017/06/27 03:37:54, sadrul wrote: > Instead of using same list of OWNERS, use file:/// to refer to the other OWNERS Removed OWNERS in common. I'll use file:/// to refer for this OWNERS file after Gary's CL is landed. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:48: gfx::Point location_transformed(location_in_region); On 2017/06/27 03:37:54, sadrul wrote: > Can you document in the header file what the coordinate space of > |location_in_region| is? > > The name |location_in_region| makes me think it is already in |region|'s > coordinate space. If it is, then why do we need to transform using region's > transform? Sry it is indeed confusing. Changed FindTargetForLocationInRegion->FindTargetInRegionForLocation and location_in_region->location_in_parent; documented in the header file. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:50: region->transform.TransformPoint(&location_transformed); On 2017/06/27 03:37:54, sadrul wrote: > TransformPoint() already early-outs for identity transforms. No need to check it > first here. I see, removed. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.cc:56: DisplayHitTestDataRegion* child_region_end = region + region->child_count; On 2017/06/27 03:37:54, sadrul wrote: > This code makes me nervous. We are reading data generated by a potentially > compromised viz process. We need to check the bounds before we read. Added check for |child_region_end|. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:18: cc::FrameSinkId id; On 2017/06/26 19:25:30, varkha wrote: > nit: whitespace after. Hmm I tried adding whitespace but git cl format would remove that? https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:19: // |location_in_target| is in the coordinate system of the target FrameSinkId. On 2017/06/26 19:25:30, varkha wrote: > nit: maybe just "Coordinates in the coordinate system ...", no need to repeat > the member name. Done. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:20: gfx::Point location_in_target; On 2017/06/27 03:37:54, sadrul wrote: > Might make sense to make this a gfx::PointF. Transform::TransformPoint only supports Point and Point3F? How about changing to Point3F when we add support for 3d space cases? https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:22: // hit_test_data.mojom. On 2017/06/26 19:25:30, varkha wrote: > Is this in services/viz/hit_test/public/interfaces/hit_test_data.mojom in [1]? > > [1] = https://codereview.chromium.org/2938953002 Yes, it's in that CL. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:26: // Finds the target for a given location based on DisplayHitTestData aggreated On 2017/06/27 03:37:55, sadrul wrote: > *aggregated Done. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:41: // Find Target for |location_in_root|, including the FrameSinkId of the On 2017/06/26 19:25:30, varkha wrote: > nit: Finds Done. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit... components/viz/host/hit_test/hit_test_query.h:51: Target* target); On 2017/06/27 03:37:55, sadrul wrote: > Should this be a const method? Done.
https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/... File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/... components/viz/common/BUILD.gn:7: "display_hit_test_data.h", display_hit_test_region.h is the name name I thinks. And, do you want to rebase above gary's? https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/... File components/viz/common/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/... components/viz/common/display_hit_test_data.h:17: struct DisplayHitTestData { this is a placeholder class right? You will remove this?
On 2017/06/28 18:46:43, rjkroege wrote: > https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/... > File components/viz/common/BUILD.gn (right): > > https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/... > components/viz/common/BUILD.gn:7: "display_hit_test_data.h", > display_hit_test_region.h is the name name I thinks. And, do you want to rebase > above gary's? > > https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/... > File components/viz/common/display_hit_test_data.h (right): > > https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/... > components/viz/common/display_hit_test_data.h:17: struct DisplayHitTestData { > this is a placeholder class right? You will remove this? Rebased above https://codereview.chromium.org/2938953002/ (my changes are only under components/viz/host, will rebase again once Gary's CL is actually landed to get rid of other changes). Looking into integration tests.
Patchset #7 (id:140001) has been deleted
(My changes are only in components/viz/host and components/viz/service/hit_test/hit_test_aggregator_unittest.cc. Will rebase above https://codereview.chromium.org/2938953002/ once it's landed to get rid of other changes.) Added testing for HitTestQuery in some tests in HitTestAggregatorTest to make sure DisplayHitTestRegion list match in HitTestAggregator and HitTestQuery. Kept other unittests in HitTestAggregatorTest and HitTestQueryTest the same - HitTestAggregatorTest contains some tests about active count/ list limit and HitTestQueryTest contains some other cases and also includes transform for point (x,y for rect) in the transform of the DisplayHitTestRegion to make sure both (including transform for point or not) work. Please take another look, thanks!
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
nit: A bunch of cc types have moved to viz so you'll need to rebase...
Patchset #8 (id:180001) has been deleted
Rebased above https://codereview.chromium.org/2938953002/ (yay!). Could you take another look? Thanks! On 2017/07/12 19:43:56, Fady Samuel wrote: > nit: A bunch of cc types have moved to viz so you'll need to rebase... Done.
The CQ bit was checked by riajiang@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by riajiang@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by riajiang@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by riajiang@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by riajiang@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: This issue passed the CQ dry run.
One suggestion on the api but aside from that it lgtm https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.h:35: void set_aggregated_hit_test_region_list( A recommendation: One method to set both the list and the size eg. set_aggregated_hit_test_region_list(AggregatedHitTestRegion*, uint32_t size) would ensure that one is never set without the other. https://codereview.chromium.org/2933493003/diff/260001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:184: EXPECT_EQ(mojom::kHitTestMine, target3.flags); Nice. Thank you for adding these to the test cases.
Please add the TODOs and file bugs. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:22: return target; I am generally opposed to return-by-value here. This is fine for now but there is an opportunity for optimization here. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:30: region->transform.TransformPoint(&location_transformed); region data cannot be trusted. Please file a bug for this and add an appropriate TODO. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:49: child_region = child_region + child_region->child_count + 1; if any child_count > child_region_count, you need to consider viz as hostile and report a security problem. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:53: target->frame_sink_id = region->frame_sink_id; the viz/host has a window <-> framesink map. there needs to be an injected validator that the reported frame_sink_id is in this map. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:55: target->flags = region->flags; invalid flags should be reported as a security fault. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query_unittest.cc:5: #include "components/viz/host/hit_test/hit_test_query.h" tests needed to be written for malformed AggregatedHitTestRegion data
https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/BUILD.gn:15: "//components/viz/common:common", Don't need :common https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:30: region->transform.TransformPoint(&location_transformed); On 2017/07/18 21:38:49, rjkroege wrote: > region data cannot be trusted. Please file a bug for this and add an appropriate > TODO. The TODO can probably go in the SetAggregatedHitTestRegionList(list, size) function, because that's where we would ideally do validation as required. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:30: region->transform.TransformPoint(&location_transformed); Can you explain why you are doing the transform here? |location_in_parent| should already be appropriately transformed to be in the parent's coordinate space. Why would you need to transform it again? https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:36: if (child_region_end > (aggregated_hit_test_region_list_ + Should be >= here, right? https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:43: while (child_region <= child_region_end) { Should be < For the regular iterators, we exclude the 'end'. We should be doing the same here. (if we need to change how |child_count| is set for that, then we should do that) https://codereview.chromium.org/2933493003/diff/260001/components/viz/service... File components/viz/service/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/service... components/viz/service/BUILD.gn:156: "//components/viz/host/hit_test:hit_test", Don't need the bits after :
https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/BUILD.gn:15: "//components/viz/common:common", On 2017/07/19 15:40:08, sadrul wrote: > Don't need :common Done. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:30: region->transform.TransformPoint(&location_transformed); On 2017/07/19 15:40:08, sadrul wrote: > Can you explain why you are doing the transform here? |location_in_parent| > should already be appropriately transformed to be in the parent's coordinate > space. Why would you need to transform it again? Because we need to transform from parent's coordinate space to this child |region|'s coordinate space? |region| can have a rotation etc.? https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:30: region->transform.TransformPoint(&location_transformed); On 2017/07/19 15:40:08, sadrul wrote: > On 2017/07/18 21:38:49, rjkroege wrote: > > region data cannot be trusted. Please file a bug for this and add an > appropriate > > TODO. > > The TODO can probably go in the SetAggregatedHitTestRegionList(list, size) > function, because that's where we would ideally do validation as required. Done. http://crbug.com/746470 https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:36: if (child_region_end > (aggregated_hit_test_region_list_ + On 2017/07/19 15:40:08, sadrul wrote: > Should be >= here, right? This is > now since I changed |child_region_end| based on the comment below? https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:43: while (child_region <= child_region_end) { On 2017/07/19 15:40:08, sadrul wrote: > Should be < > > For the regular iterators, we exclude the 'end'. We should be doing the same > here. (if we need to change how |child_count| is set for that, then we should do > that) Okay I see. Changed |child_region_end| to be |child_region| + |child_count| so this is < now. WDYT? https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:49: child_region = child_region + child_region->child_count + 1; On 2017/07/18 21:38:49, rjkroege wrote: > if any child_count > child_region_count, you need to consider viz as hostile and > report a security problem. Added in the bug. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:53: target->frame_sink_id = region->frame_sink_id; On 2017/07/18 21:38:49, rjkroege wrote: > the viz/host has a window <-> framesink map. there needs to be an injected > validator that the reported frame_sink_id is in this map. I didn't find it in viz/host - is it existing code or will be in viz/host? I thought this map was in mus-ws? Added in the bug. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:55: target->flags = region->flags; On 2017/07/18 21:38:49, rjkroege wrote: > invalid flags should be reported as a security fault. Added in the bug. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.h:35: void set_aggregated_hit_test_region_list( On 2017/07/18 13:12:57, gklassen wrote: > A recommendation: One method to set both the list and the size > > eg. set_aggregated_hit_test_region_list(AggregatedHitTestRegion*, uint32_t size) > > would ensure that one is never set without the other. Good point! Done. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query_unittest.cc:5: #include "components/viz/host/hit_test/hit_test_query.h" On 2017/07/18 21:38:49, rjkroege wrote: > tests needed to be written for malformed AggregatedHitTestRegion data Added one for invalid |child_count|. Will add more robust ones after properly handing all security faults. https://codereview.chromium.org/2933493003/diff/260001/components/viz/service... File components/viz/service/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/service... components/viz/service/BUILD.gn:156: "//components/viz/host/hit_test:hit_test", On 2017/07/19 15:40:08, sadrul wrote: > Don't need the bits after : Done.
https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:53: target->frame_sink_id = region->frame_sink_id; On 2017/07/19 17:45:55, riajiang wrote: > On 2017/07/18 21:38:49, rjkroege wrote: > > the viz/host has a window <-> framesink map. there needs to be an injected > > validator that the reported frame_sink_id is in this map. > > I didn't find it in viz/host - is it existing code or will be in viz/host? I > thought this map was in mus-ws? > > Added in the bug. so: viz-host is code in the mus-ws process. we need the map as part of both chrome and ws so makes sense to keep in viz-host code. https://codereview.chromium.org/2933493003/diff/280001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/280001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query_unittest.cc:575: // |child_count| is invalid, which is a security fault. For now, check to see which one is invalid? I was imagining a child count like csdtint::INT64_MIN
https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:30: region->transform.TransformPoint(&location_transformed); On 2017/07/19 17:45:55, riajiang wrote: > On 2017/07/19 15:40:08, sadrul wrote: > > Can you explain why you are doing the transform here? |location_in_parent| > > should already be appropriately transformed to be in the parent's coordinate > > space. Why would you need to transform it again? > > Because we need to transform from parent's coordinate space to this child > |region|'s coordinate space? |region| can have a rotation etc.? As discussed offline, transformation needed here. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.cc:53: target->frame_sink_id = region->frame_sink_id; On 2017/07/19 18:01:25, rjkroege wrote: > On 2017/07/19 17:45:55, riajiang wrote: > > On 2017/07/18 21:38:49, rjkroege wrote: > > > the viz/host has a window <-> framesink map. there needs to be an injected > > > validator that the reported frame_sink_id is in this map. > > > > I didn't find it in viz/host - is it existing code or will be in viz/host? I > > thought this map was in mus-ws? > > > > Added in the bug. > > so: viz-host is code in the mus-ws process. we need the map as part of both > chrome and ws so makes sense to keep in viz-host code. I see, will do when adding the map. https://codereview.chromium.org/2933493003/diff/280001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/280001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query_unittest.cc:575: // |child_count| is invalid, which is a security fault. For now, check to see On 2017/07/19 18:01:26, rjkroege wrote: > which one is invalid? I was imagining a child count like csdtint::INT64_MIN e's child_count is wrong since it only has three children but its child_count says 4?
On 2017/07/19 19:22:14, riajiang wrote: > https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... > File components/viz/host/hit_test/hit_test_query.cc (right): > > https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... > components/viz/host/hit_test/hit_test_query.cc:30: > region->transform.TransformPoint(&location_transformed); > On 2017/07/19 17:45:55, riajiang wrote: > > On 2017/07/19 15:40:08, sadrul wrote: > > > Can you explain why you are doing the transform here? |location_in_parent| > > > should already be appropriately transformed to be in the parent's coordinate > > > space. Why would you need to transform it again? > > > > Because we need to transform from parent's coordinate space to this child > > |region|'s coordinate space? |region| can have a rotation etc.? > > As discussed offline, transformation needed here. > > https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... > components/viz/host/hit_test/hit_test_query.cc:53: target->frame_sink_id = > region->frame_sink_id; > On 2017/07/19 18:01:25, rjkroege wrote: > > On 2017/07/19 17:45:55, riajiang wrote: > > > On 2017/07/18 21:38:49, rjkroege wrote: > > > > the viz/host has a window <-> framesink map. there needs to be an injected > > > > validator that the reported frame_sink_id is in this map. > > > > > > I didn't find it in viz/host - is it existing code or will be in viz/host? I > > > thought this map was in mus-ws? > > > > > > Added in the bug. > > > > so: viz-host is code in the mus-ws process. we need the map as part of both > > chrome and ws so makes sense to keep in viz-host code. > > I see, will do when adding the map. > > https://codereview.chromium.org/2933493003/diff/280001/components/viz/host/hi... > File components/viz/host/hit_test/hit_test_query_unittest.cc (right): > > https://codereview.chromium.org/2933493003/diff/280001/components/viz/host/hi... > components/viz/host/hit_test/hit_test_query_unittest.cc:575: // |child_count| is > invalid, which is a security fault. For now, check to see > On 2017/07/19 18:01:26, rjkroege wrote: > > which one is invalid? I was imagining a child count like csdtint::INT64_MIN > > e's child_count is wrong since it only has three children but its child_count > says 4? it needs to be a big nasty number
On 2017/07/19 21:37:46, rjkroege wrote: > On 2017/07/19 19:22:14, riajiang wrote: > > > https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... > > File components/viz/host/hit_test/hit_test_query.cc (right): > > > > > https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... > > components/viz/host/hit_test/hit_test_query.cc:30: > > region->transform.TransformPoint(&location_transformed); > > On 2017/07/19 17:45:55, riajiang wrote: > > > On 2017/07/19 15:40:08, sadrul wrote: > > > > Can you explain why you are doing the transform here? |location_in_parent| > > > > should already be appropriately transformed to be in the parent's > coordinate > > > > space. Why would you need to transform it again? > > > > > > Because we need to transform from parent's coordinate space to this child > > > |region|'s coordinate space? |region| can have a rotation etc.? > > > > As discussed offline, transformation needed here. > > > > > https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hi... > > components/viz/host/hit_test/hit_test_query.cc:53: target->frame_sink_id = > > region->frame_sink_id; > > On 2017/07/19 18:01:25, rjkroege wrote: > > > On 2017/07/19 17:45:55, riajiang wrote: > > > > On 2017/07/18 21:38:49, rjkroege wrote: > > > > > the viz/host has a window <-> framesink map. there needs to be an > injected > > > > > validator that the reported frame_sink_id is in this map. > > > > > > > > I didn't find it in viz/host - is it existing code or will be in viz/host? > I > > > > thought this map was in mus-ws? > > > > > > > > Added in the bug. > > > > > > so: viz-host is code in the mus-ws process. we need the map as part of both > > > chrome and ws so makes sense to keep in viz-host code. > > > > I see, will do when adding the map. > > > > > https://codereview.chromium.org/2933493003/diff/280001/components/viz/host/hi... > > File components/viz/host/hit_test/hit_test_query_unittest.cc (right): > > > > > https://codereview.chromium.org/2933493003/diff/280001/components/viz/host/hi... > > components/viz/host/hit_test/hit_test_query_unittest.cc:575: // |child_count| > is > > invalid, which is a security fault. For now, check to see > > On 2017/07/19 18:01:26, rjkroege wrote: > > > which one is invalid? I was imagining a child count like csdtint::INT64_MIN > > > > > e's child_count is wrong since it only has three children but its child_count > > says 4? > > it needs to be a big nasty number As discussed offline, added check for negative |child_count| and updated test case.
lgtm
The CQ bit was checked by riajiang@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by riajiang@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 https://codereview.chromium.org/2933493003/diff/300001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/300001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.h:66: Target FindTargetForLocation(const gfx::Point& location_in_root); This too can be a const method, I think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by riajiang@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by riajiang@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by riajiang@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: This issue passed the CQ dry run.
Description was changed from ========== Add HitTestQuery to go through DisplayHitTestData to get Target for points. BUG=732400 TEST=viz_hit_test_host_unittests ========== to ========== Add viz-host HitTestQuery. HitTestQuery finds the hit test target, for a given location, in the AggregatedHitTestRegion list received from HitTestAggregator. BUG=732400 TEST=viz_unittests ==========
https://codereview.chromium.org/2933493003/diff/300001/components/viz/host/hi... File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/300001/components/viz/host/hi... components/viz/host/hit_test/hit_test_query.h:66: Target FindTargetForLocation(const gfx::Point& location_in_root); On 2017/07/21 17:18:11, sadrul wrote: > This too can be a const method, I think? Done.
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gklassen@chromium.org, rjkroege@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2933493003/#ps380001 (title: "don't combine uint and int; check for overflow")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 380001, "attempt_start_ts": 1500697394900510, "parent_rev": "5f8fd9e4d2185d6c48f592f4b9f05de1ca04ac17", "commit_rev": "537141c4ede41728e1cf026f2967f0838f1e51de"}
Message was sent while issue was closed.
Description was changed from ========== Add viz-host HitTestQuery. HitTestQuery finds the hit test target, for a given location, in the AggregatedHitTestRegion list received from HitTestAggregator. BUG=732400 TEST=viz_unittests ========== to ========== Add viz-host HitTestQuery. HitTestQuery finds the hit test target, for a given location, in the AggregatedHitTestRegion list received from HitTestAggregator. BUG=732400 TEST=viz_unittests Review-Url: https://codereview.chromium.org/2933493003 Cr-Commit-Position: refs/heads/master@{#488854} Committed: https://chromium.googlesource.com/chromium/src/+/537141c4ede41728e1cf026f2967... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:380001) as https://chromium.googlesource.com/chromium/src/+/537141c4ede41728e1cf026f2967...
Message was sent while issue was closed.
lgtm with a couple small suggestions. https://codereview.chromium.org/2933493003/diff/380001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/380001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:107: } I'd make the class data private with protected getters and DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2933493003/diff/380001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:169: EXPECT_EQ(display_surface_id.frame_sink_id(), target1.frame_sink_id); It looks like the order has changed. Would it be good thing to change that to match what the macro expects now to have a better formatted output for failing case?
Message was sent while issue was closed.
https://codereview.chromium.org/2933493003/diff/380001/components/viz/service... File components/viz/service/hit_test/hit_test_aggregator_unittest.cc (right): https://codereview.chromium.org/2933493003/diff/380001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:107: } On 2017/07/22 04:31:45, varkha wrote: > I'd make the class data private with protected getters and > DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/2933493003/diff/380001/components/viz/service... components/viz/service/hit_test/hit_test_aggregator_unittest.cc:169: EXPECT_EQ(display_surface_id.frame_sink_id(), target1.frame_sink_id); On 2017/07/22 04:31:46, varkha wrote: > It looks like the order has changed. Would it be good thing to change that to > match what the macro expects now to have a better formatted output for failing > case? Done. |