Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(145)

Issue 2933493003: Add viz-host HitTestQuery. (Closed)

Created:
3 years, 6 months ago by riajiang
Modified:
3 years, 5 months ago
CC:
chromium-reviews, blundell+watchlist_chromium.org, cc-bugs_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+1022 lines, -9 lines) Patch
M components/viz/common/hit_test/aggregated_hit_test_region.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +2 lines, -2 lines 0 comments Download
M components/viz/host/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A components/viz/host/hit_test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +36 lines, -0 lines 0 comments Download
A + components/viz/host/hit_test/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -2 lines 0 comments Download
A components/viz/host/hit_test/OWNERS View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A components/viz/host/hit_test/hit_test_query.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +84 lines, -0 lines 0 comments Download
A components/viz/host/hit_test/hit_test_query.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +68 lines, -0 lines 0 comments Download
A components/viz/host/hit_test/hit_test_query_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +627 lines, -0 lines 0 comments Download
M components/viz/service/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M components/viz/service/hit_test/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -1 line 0 comments Download
M components/viz/service/hit_test/hit_test_aggregator_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 13 chunks +195 lines, -4 lines 4 comments Download

Messages

Total messages: 87 (53 generated)
riajiang
Hi all, this CL still has some TODOs related to https://codereview.chromium.org/2908783002/ and needs more test ...
3 years, 6 months ago (2017-06-09 21:23:31 UTC) #2
rjkroege
it's the right general idea. My primary concern is that you need to refine the ...
3 years, 6 months ago (2017-06-12 17:17:24 UTC) #3
riajiang
https://codereview.chromium.org/2933493003/diff/1/components/viz/frame_sinks/BUILD.gn File components/viz/frame_sinks/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/1/components/viz/frame_sinks/BUILD.gn#newcode38 components/viz/frame_sinks/BUILD.gn:38: "//components/viz/hit_test", On 2017/06/12 17:17:23, rjkroege wrote: > must establish ...
3 years, 6 months ago (2017-06-13 18:42:30 UTC) #4
riajiang
Updated the patch. Could you take another look? Thanks!
3 years, 6 months ago (2017-06-15 02:09:14 UTC) #5
gklassen
Some quick comments ( but not a comprehensive review ). https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit_test/hit_test_query.cc File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/host/hit_test/hit_test_query.cc#newcode31 ...
3 years, 6 months ago (2017-06-15 19:51:20 UTC) #6
rjkroege
https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/hit_test/DEPS File components/viz/common/hit_test/DEPS (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/hit_test/DEPS#newcode2 components/viz/common/hit_test/DEPS:2: "+cc/surfaces", This seems too big. There is a bunch ...
3 years, 6 months ago (2017-06-15 19:58:36 UTC) #7
riajiang
https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/hit_test/DEPS File components/viz/common/hit_test/DEPS (right): https://codereview.chromium.org/2933493003/diff/20001/components/viz/common/hit_test/DEPS#newcode2 components/viz/common/hit_test/DEPS:2: "+cc/surfaces", On 2017/06/15 19:58:35, rjkroege wrote: > This seems ...
3 years, 6 months ago (2017-06-16 02:56:55 UTC) #9
sadrul
https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit_test/BUILD.gn File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit_test/BUILD.gn#newcode10 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 ...
3 years, 6 months ago (2017-06-16 17:09:51 UTC) #10
sadrul
Also, make sure to update the CL description, and link to a bug (and the ...
3 years, 6 months ago (2017-06-16 17:10:57 UTC) #11
riajiang
Also updated tree walking to be recursive. https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit_test/BUILD.gn File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/60001/components/viz/host/hit_test/BUILD.gn#newcode10 components/viz/host/hit_test/BUILD.gn:10: "components/viz/common/display_hit_test_data.h", On ...
3 years, 6 months ago (2017-06-23 02:46:37 UTC) #14
varkha
Just a few nits, looking at the data structures. https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit_test/hit_test_query.h File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/host/hit_test/hit_test_query.h#newcode18 components/viz/host/hit_test/hit_test_query.h:18: ...
3 years, 5 months ago (2017-06-26 19:25:30 UTC) #16
rjkroege
https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/hit_test/display_hit_test_data.h File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/hit_test/display_hit_test_data.h#newcode5 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 ...
3 years, 5 months ago (2017-06-27 00:10:52 UTC) #17
sadrul
https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/hit_test/display_hit_test_data.h File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/hit_test/display_hit_test_data.h#newcode25 components/viz/common/hit_test/display_hit_test_data.h:25: using DisplayHitTestData = std::vector<DisplayHitTestDataRegion>; If you follow the general ...
3 years, 5 months ago (2017-06-27 03:37:55 UTC) #18
riajiang
https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/hit_test/display_hit_test_data.h File components/viz/common/hit_test/display_hit_test_data.h (right): https://codereview.chromium.org/2933493003/diff/80001/components/viz/common/hit_test/display_hit_test_data.h#newcode5 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 ...
3 years, 5 months ago (2017-06-27 15:51:45 UTC) #19
rjkroege
https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/BUILD.gn File components/viz/common/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/BUILD.gn#newcode7 components/viz/common/BUILD.gn:7: "display_hit_test_data.h", display_hit_test_region.h is the name name I thinks. And, ...
3 years, 5 months ago (2017-06-28 18:46:43 UTC) #20
riajiang
On 2017/06/28 18:46:43, rjkroege wrote: > https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/BUILD.gn > File components/viz/common/BUILD.gn (right): > > https://codereview.chromium.org/2933493003/diff/100001/components/viz/common/BUILD.gn#newcode7 > ...
3 years, 5 months ago (2017-06-29 13:54:46 UTC) #21
riajiang
(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 ...
3 years, 5 months ago (2017-06-29 23:53:48 UTC) #23
Fady Samuel
nit: A bunch of cc types have moved to viz so you'll need to rebase...
3 years, 5 months ago (2017-07-12 19:43:56 UTC) #25
riajiang
Rebased above https://codereview.chromium.org/2938953002/ (yay!). Could you take another look? Thanks! On 2017/07/12 19:43:56, Fady Samuel ...
3 years, 5 months ago (2017-07-14 20:42:41 UTC) #27
gklassen
One suggestion on the api but aside from that it lgtm https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.h File components/viz/host/hit_test/hit_test_query.h (right): ...
3 years, 5 months ago (2017-07-18 13:12:57 UTC) #48
rjkroege
Please add the TODOs and file bugs. https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc#newcode22 components/viz/host/hit_test/hit_test_query.cc:22: return target; ...
3 years, 5 months ago (2017-07-18 21:38:50 UTC) #49
sadrul
https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/BUILD.gn File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/BUILD.gn#newcode15 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/hit_test/hit_test_query.cc File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc#newcode30 ...
3 years, 5 months ago (2017-07-19 15:40:08 UTC) #50
riajiang
https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/BUILD.gn File components/viz/host/hit_test/BUILD.gn (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/BUILD.gn#newcode15 components/viz/host/hit_test/BUILD.gn:15: "//components/viz/common:common", On 2017/07/19 15:40:08, sadrul wrote: > Don't need ...
3 years, 5 months ago (2017-07-19 17:45:55 UTC) #51
rjkroege
https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc#newcode53 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: > ...
3 years, 5 months ago (2017-07-19 18:01:26 UTC) #52
riajiang
https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc File components/viz/host/hit_test/hit_test_query.cc (right): https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc#newcode30 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 ...
3 years, 5 months ago (2017-07-19 19:22:14 UTC) #53
rjkroege
On 2017/07/19 19:22:14, riajiang wrote: > https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc > File components/viz/host/hit_test/hit_test_query.cc (right): > > https://codereview.chromium.org/2933493003/diff/260001/components/viz/host/hit_test/hit_test_query.cc#newcode30 > ...
3 years, 5 months ago (2017-07-19 21:37:46 UTC) #54
riajiang
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/hit_test/hit_test_query.cc ...
3 years, 5 months ago (2017-07-20 00:14:42 UTC) #55
rjkroege
lgtm
3 years, 5 months ago (2017-07-21 15:26:00 UTC) #56
sadrul
lgtm https://codereview.chromium.org/2933493003/diff/300001/components/viz/host/hit_test/hit_test_query.h File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/300001/components/viz/host/hit_test/hit_test_query.h#newcode66 components/viz/host/hit_test/hit_test_query.h:66: Target FindTargetForLocation(const gfx::Point& location_in_root); This too can be ...
3 years, 5 months ago (2017-07-21 17:18:11 UTC) #63
riajiang
https://codereview.chromium.org/2933493003/diff/300001/components/viz/host/hit_test/hit_test_query.h File components/viz/host/hit_test/hit_test_query.h (right): https://codereview.chromium.org/2933493003/diff/300001/components/viz/host/hit_test/hit_test_query.h#newcode66 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: ...
3 years, 5 months ago (2017-07-22 04:23:05 UTC) #79
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2933493003/380001
3 years, 5 months ago (2017-07-22 04:23:18 UTC) #82
commit-bot: I haz the power
Committed patchset #17 (id:380001) as https://chromium.googlesource.com/chromium/src/+/537141c4ede41728e1cf026f2967f0838f1e51de
3 years, 5 months ago (2017-07-22 04:29:23 UTC) #85
varkha
lgtm with a couple small suggestions. https://codereview.chromium.org/2933493003/diff/380001/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/2933493003/diff/380001/components/viz/service/hit_test/hit_test_aggregator_unittest.cc#newcode107 components/viz/service/hit_test/hit_test_aggregator_unittest.cc:107: } I'd make ...
3 years, 5 months ago (2017-07-22 04:31:46 UTC) #86
riajiang
3 years, 5 months ago (2017-07-24 18:24:50 UTC) #87
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.

Powered by Google App Engine
This is Rietveld 408576698