Issue 2908783002: WIP Hittest Component.

gklassen

Description was changed from ========== git format fixes preliminary hittest API and HittestData mojo structures ...

3 years, 7 months ago (2017-05-26 17:45:29 UTC) #1

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 17:46:18 UTC) #2

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/1

3 years, 7 months ago (2017-05-26 17:46:54 UTC) #3

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 17:49:30 UTC) #4

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xcode-clang/builds/108743) mac_chromium_compile_dbg_ng on ...

3 years, 7 months ago (2017-05-26 17:49:31 UTC) #5

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 18:06:04 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/20001

3 years, 7 months ago (2017-05-26 18:06:54 UTC) #7

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 18:10:36 UTC) #9

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/220054) ios-device-xcode-clang on ...

3 years, 7 months ago (2017-05-26 18:10:37 UTC) #10

gklassen

Description was changed from ========== Initial and Preliminary Hittest Component. preliminary hittest API and HittestData ...

3 years, 7 months ago (2017-05-26 18:20:19 UTC) #11

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 18:31:42 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/40001

3 years, 7 months ago (2017-05-26 18:32:49 UTC) #14

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 18:36:52 UTC) #15

commit-bot: I haz the power

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/builds/224694) mac_chromium_rel_ng on ...

3 years, 7 months ago (2017-05-26 18:36:53 UTC) #16

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 18:53:18 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/60001

3 years, 7 months ago (2017-05-26 18:53:57 UTC) #18

rjkroege

rjkroege@chromium.org changed reviewers: + rjkroege@chromium.org

3 years, 7 months ago (2017-05-26 18:55:09 UTC) #20

rjkroege

you should begin with the HittestData mojo and struct traits. https://codereview.chromium.org/2908783002/diff/60001/components/viz/frame_sinks/BUILD.gn File components/viz/frame_sinks/BUILD.gn (right): https://codereview.chromium.org/2908783002/diff/60001/components/viz/frame_sinks/BUILD.gn#newcode31 ...

3 years, 7 months ago (2017-05-26 18:55:09 UTC) #21

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 18:57:52 UTC) #22

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/394263) mac_chromium_compile_dbg_ng on ...

3 years, 7 months ago (2017-05-26 18:57:53 UTC) #23

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 19:47:32 UTC) #24

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/80001

3 years, 7 months ago (2017-05-26 19:48:03 UTC) #25

gklassen

On 2017/05/26 18:55:09, rjkroege wrote: > you should begin with the HittestData mojo and struct ...

3 years, 7 months ago (2017-05-26 20:00:22 UTC) #26

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 20:03:04 UTC) #27

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/100001

3 years, 7 months ago (2017-05-26 20:03:38 UTC) #28

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 20:11:28 UTC) #29

commit-bot: I haz the power

Dry run: 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_presubmit/builds/449094)

3 years, 7 months ago (2017-05-26 20:11:29 UTC) #30

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 20:24:36 UTC) #31

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/120001

3 years, 7 months ago (2017-05-26 20:25:16 UTC) #32

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 21:07:50 UTC) #33

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/278253)

3 years, 7 months ago (2017-05-26 21:07:52 UTC) #34

rjkroege

https://codereview.chromium.org/2908783002/diff/100001/components/viz/frame_sinks/BUILD.gn File components/viz/frame_sinks/BUILD.gn (right): https://codereview.chromium.org/2908783002/diff/100001/components/viz/frame_sinks/BUILD.gn#newcode29 components/viz/frame_sinks/BUILD.gn:29: "//components/viz/hittest", not necessary yet? https://codereview.chromium.org/2908783002/diff/100001/components/viz/hittest/BUILD.gn File components/viz/hittest/BUILD.gn (right): https://codereview.chromium.org/2908783002/diff/100001/components/viz/hittest/BUILD.gn#newcode13 ...

3 years, 6 months ago (2017-05-29 15:58:03 UTC) #35

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-05-29 17:28:00 UTC) #36

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/140001

3 years, 6 months ago (2017-05-29 17:28:17 UTC) #37

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-05-29 18:31:51 UTC) #38

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-05-29 18:31:52 UTC) #39

gklassen

The CQ bit was checked by gklassen@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-05-29 19:03:00 UTC) #40

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2908783002/160001

3 years, 6 months ago (2017-05-29 19:03:05 UTC) #41

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-05-29 20:20:42 UTC) #42

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/422179)

3 years, 6 months ago (2017-05-29 20:20:43 UTC) #43

gklassen

Description was changed from ========== WIP Hittest Component. First step towards a Hittest component to ...

3 years, 6 months ago (2017-06-02 21:59:48 UTC) #44

rjkroege

Awesome! Please continue. https://codereview.chromium.org/2908783002/diff/180001/cc/surfaces/surface_observer.h File cc/surfaces/surface_observer.h (right): https://codereview.chromium.org/2908783002/diff/180001/cc/surfaces/surface_observer.h#newcode42 cc/surfaces/surface_observer.h:42: // Runs when the Surface Aggregator ...

3 years, 6 months ago (2017-06-02 22:45:46 UTC) #45

gklassen

Many comments integrated into next patch. Some questions below. I have uploaded another patch - ...

3 years, 6 months ago (2017-06-05 21:32:14 UTC) #47

rjkroege

https://codereview.chromium.org/2908783002/diff/200001/cc/surfaces/surface_aggregator.cc File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2908783002/diff/200001/cc/surfaces/surface_aggregator.cc#newcode763 cc/surfaces/surface_aggregator.cc:763: if (!damage_rect.IsEmpty()) { Would we ever want to adjust ...

3 years, 6 months ago (2017-06-07 17:16:46 UTC) #48

gklassen

Thanks again. Most comments integrated or made irrelevant by next patch. Some questions and comments ...

3 years, 6 months ago (2017-06-07 20:04:45 UTC) #49

rjkroege

https://codereview.chromium.org/2908783002/diff/200001/cc/surfaces/surface_aggregator.cc File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2908783002/diff/200001/cc/surfaces/surface_aggregator.cc#newcode763 cc/surfaces/surface_aggregator.cc:763: if (!damage_rect.IsEmpty()) { On 2017/06/07 20:04:43, gklassen wrote: > ...

3 years, 6 months ago (2017-06-07 20:22:57 UTC) #50

rjkroege

this is looking very reasonable. https://codereview.chromium.org/2908783002/diff/320001/cc/surfaces/surface_observer.h File cc/surfaces/surface_observer.h (right): https://codereview.chromium.org/2908783002/diff/320001/cc/surfaces/surface_observer.h#newcode41 cc/surfaces/surface_observer.h:41: has added https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_test/display_hit_test_data.h File ...

3 years, 6 months ago (2017-06-07 21:19:07 UTC) #51

gklassen

I've integrated many improvements from these replies. Thank you. I will review the next reviewer ...

3 years, 6 months ago (2017-06-08 17:09:22 UTC) #52

I've integrated many improvements from these replies.  Thank you.  I will review
the next reviewer set before uploading another patch - stay tuned.

https://codereview.chromium.org/2908783002/diff/200001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2908783002/diff/200001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:763: if (!damage_rect.IsEmpty()) {
On 2017/06/07 20:22:55, rjkroege wrote:
> On 2017/06/07 20:04:43, gklassen wrote:
> > On 2017/06/07 17:16:44, rjkroege wrote:
> > > Would we ever want to adjust the HitTestData without altering the display?
> > 
> > not sure what you mean?
> 
> It was idle speculation: would we want to alter the HTD without also altering
> the CF?
> 
> Also: I don't really understand SurfaceAggregator well enough to know if this
is
> the right place to invoke the observers.
> 
> Given that this is on the critical path, we want to make sure that the
observers
> are efficient.

OIC.  Currently the only mechanism we have contemplated to update hittest data
is tied to SubmitCompositorFrame - we could make changes to support updates at
any time, but currently I'm unaware of possible causes ( although I will have a
much better perspective after we are generating real hit test data and we can
keep this in mind ). 

I selected this entry with some over the shoulder help from some in the group. 
Is there a good set of reviewers who understand SurfaceAggregator and could help
review if this is a good place to invoke observers? ( We will also know much
more after we try it in context with real hittestdata ).

https://codereview.chromium.org/2908783002/diff/200001/cc/surfaces/surface_ma...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/2908783002/diff/200001/cc/surfaces/surface_ma...
cc/surfaces/surface_manager.h:77: void SurfaceWillDraw(const SurfaceId&
surface_id);
On 2017/06/07 20:22:56, rjkroege wrote:
> Add method comment.

Done.

https://codereview.chromium.org/2908783002/diff/200001/components/viz/frame_s...
File components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2908783002/diff/200001/components/viz/frame_s...
components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc:118:
hittest_aggregator_.Aggregate(display_->CurrentSurfaceId());
On 2017/06/07 20:22:56, rjkroege wrote:
> I'd like aggregate to run after DirectRenderer has handed over the work to the
> GPU so that the GPU is working while the GPU is working. Perhaps PostTask this
> to yourself as a closure?

Good Call. Done.

https://codereview.chromium.org/2908783002/diff/200001/components/viz/frame_s...
File components/viz/frame_sinks/gpu_root_compositor_frame_sink.h (right):

https://codereview.chromium.org/2908783002/diff/200001/components/viz/frame_s...
components/viz/frame_sinks/gpu_root_compositor_frame_sink.h:106:
HittestAggregator hittest_aggregator_;
On 2017/06/07 20:22:56, rjkroege wrote:
> If you write it this way, you are using space even before the code is
finished.
> Consider unique_ptr<> and only creating if the feature is enabled. (Feature
> needs to behind a flag before landing)

Good call. Done.

https://codereview.chromium.org/2908783002/diff/200001/components/viz/hittest...
File components/viz/hittest/hittest_aggregator.h (right):

https://codereview.chromium.org/2908783002/diff/200001/components/viz/hittest...
components/viz/hittest/hittest_aggregator.h:33: typedef std::map<cc::SurfaceId,
hittest::mojom::HittestDataPtr> HittestDataMap;
On 2017/06/07 20:22:56, rjkroege wrote:
> On 2017/06/07 20:04:44, gklassen wrote:
> > On 2017/06/07 17:16:45, rjkroege wrote:
> > > use using
> > 
> > the compiler reports "error: using declaration cannot refer to a template
> > specialization".  Is there another way to change it to use using?
> 
> This doesn't compile?
> 
> using HittestDataMap = std::map<cc::SurfaceId,
hittest::mojom::HittestDataPtr>;
> 
> 
> That's weird. What's strange about Mojo types?

( gklassen@ would like to recall this statement on account of he was using
incorrect syntax :$. )

Done.

https://codereview.chromium.org/2908783002/diff/200001/components/viz/hittest...
components/viz/hittest/hittest_aggregator.h:36: friend class
test::HittestAggregatorTest;
On 2017/06/07 20:22:56, rjkroege wrote:
> On 2017/06/07 20:04:44, gklassen wrote:
> > On 2017/06/07 17:16:45, rjkroege wrote:
> > > grumble.
> > 
> > Suggestions welcome.  The test code requires access to pending_ and active_
> > members to verify contents and I am loath to make those public.  Is there an
> > alternative?
> 
>
https://cs.chromium.org/chromium/src/base/gtest_prod_util.h?q=FRIEND_TEST_ALL...

This seems to require a MACRO per method and a friend declaration for the test
class still seems necessary?  

Here is an example
https://cs.chromium.org/chromium/src/base/json/json_parser.h?type=cs&q=FRIEND...

gklassen

new patch coming... https://codereview.chromium.org/2908783002/diff/320001/cc/surfaces/surface_observer.h File cc/surfaces/surface_observer.h (right): https://codereview.chromium.org/2908783002/diff/320001/cc/surfaces/surface_observer.h#newcode41 cc/surfaces/surface_observer.h:41: On 2017/06/07 21:19:04, rjkroege wrote: > ...

3 years, 6 months ago (2017-06-08 20:33:22 UTC) #53

new patch coming...

https://codereview.chromium.org/2908783002/diff/320001/cc/surfaces/surface_ob...
File cc/surfaces/surface_observer.h (right):

https://codereview.chromium.org/2908783002/diff/320001/cc/surfaces/surface_ob...
cc/surfaces/surface_observer.h:41: 
On 2017/06/07 21:19:04, rjkroege wrote:
> has added

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
File components/viz/hit_test/display_hit_test_data.h (right):

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/display_hit_test_data.h:26: // - other processes can
access without process hops or mojo calls
On 2017/06/07 21:19:04, rjkroege wrote:
> sentences need periods

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/display_hit_test_data.h:46: DisplayHitTestRegion
regions_[];
On 2017/06/07 21:19:04, rjkroege wrote:
> Some compilers don't like 0-length arrays. You might want to make this a
> DisplayHitTestRegion*

if it's a pointer then the struct won't be flat - the pointer will need to be
different in each process ( right? ).  It's possible that we could treat
DisplayHitTestData as a header and expect a following array but that seems odd
as well.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator.cc (right):

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:12: constexpr int kInitialSize =
1024;
On 2017/06/07 21:19:05, rjkroege wrote:
> need a TODO() to figure out what a good initial size is
> 
> need a TODO() to figure out what to do about changing size.

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:15: using namespace
hit_test::mojom;
On 2017/06/07 21:19:05, rjkroege wrote:
> I'm not keen on this.

Ok.  Removed.  Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:31: active_[surface_id] =
std::move(pending_[surface_id]);
On 2017/06/07 21:19:05, rjkroege wrote:
> so...  when you get another OnSurfaceWillDraw re-using a specific SurfaceId in
a
> subsequent frame, you will find the surface_id missing from pending_? And the
> code will be sad?

Thanks.  Code added to explicitly check and handle this case. Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:42: display_hit_test_data_->size_
= size;
On 2017/06/07 21:19:04, rjkroege wrote:
> DisplayHitTestData could have a constructor?

It's now a struct ( based on earlier discussion and recommendation ).

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:45:
display_hit_test_data_->regions_[size << 2].child_count_ = kEndOfList;
On 2017/06/07 21:19:05, rjkroege wrote:
> Why do you init it?

It's using child_count == kEndOfList to indicate the end of the list.  It's a
struct so this seemed like the best place to init - but better suggestions are
welcome and appreciated.  I'm not sure if I like it either.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:54: int
HitTestAggregator::Append(cc::SurfaceId surface_id, int index) {
On 2017/06/07 21:19:05, rjkroege wrote:
> completely private methods could be in an anonymous name space inside of this
> class.

Thanks.  This method currently accesses some member variables so I would need to
refactor.  Is it worth it?

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:57: // referenced surface not
found!
On 2017/06/07 21:19:05, rjkroege wrote:
> this indicates a bug in SurfaceAggregator yes? They should all exist.

Based on earlier conversations I believe it's possible for a surface to call
SubmitCompositorFrame late - if so then this would happen and be expected.  The
hit_test_data for this surface would be empty as would the contents of the
frame.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:62: if
(hit_test_data->regions_.size() == 0) {
On 2017/06/07 21:19:04, rjkroege wrote:
> we must have at least 1 region for valid data yes? DCHECK?

A surface with no children may have no sub-regions no?

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:72: element->flags_ =
HitTestRegionFlags::HIT_TEST_BOUNDS;
On 2017/06/07 21:19:04, rjkroege wrote:
> You have a separate region for the bounds? Why?

After review and discussion this morning I think you are right that this isn't
necessary.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:79: element->child_count_ = index
- parent_index - 1;
On 2017/06/07 21:19:05, rjkroege wrote:
> Assumptions here should be wrapped in DCHECK

Agreed.  Do you have suggestions for assumptions to check?

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:106: return
display_hit_test_data_->front_offset_ == 0
On 2017/06/07 21:19:05, rjkroege wrote:
> if is easier to read?

I wrote it both ways.  I think I agree with you.  Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator.h (right):

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:33: 
On 2017/06/07 21:19:06, rjkroege wrote:
> nit: remove blank?
> 
> And: since when can you preserve HitTestDataPtr? It's a unique_ptr under the
> hood yes? You will need a ref_ptr wrapper so that you can have a reference to
it
> in both the pending_ and active_ maps?

Yes - although I believe it may be possible to move objects from pending to
active lists rather than keep copies.  I have made changes to reflect this.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:46: void
SubmitHitTestData(hit_test::mojom::HitTestDataPtr hit_test_data);
On 2017/06/07 21:19:06, rjkroege wrote:
> Need a TODO for what to do if the submitted data is invalid.

Good call. TODO added to the implementaiton. Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:65: 
On 2017/06/07 21:19:06, rjkroege wrote:
> remove blank

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:72: DisplayHitTestData*
display_hit_test_data_;
On 2017/06/07 21:19:05, rjkroege wrote:
> Does it need to be public? Have an accessor?

Good call. Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:80: void
AllocateDisplayHitTestData();
On 2017/06/07 21:19:05, rjkroege wrote:
> method comments would be convenient for people unfamiliar with your code.

Agreed. Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator_unittest.cc (right):

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:69: // tests brainstorm
& todo
On 2017/06/07 21:19:06, rjkroege wrote:
> you would remove this before landing

ok.  Agreed. Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:75: // one surface
On 2017/06/07 21:19:06, rjkroege wrote:
> Love it! Wonderful!
> 
> And... comments need to be sentences.
> 
> It would be nice to add the ascii art to the display_hit_test_data .h
explaining
> how the data structure works and reference it here

Agreed.  Or should this be added to an .md?

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:90:
hit_test_data->bounds_.SetRect(0, 0, 1024, 768);
On 2017/06/07 21:19:06, rjkroege wrote:
> So: in this case, the example receives no events yes? Because it has no
region?
> 
> Or am I mis-understanding the representation.

I will update based on our conversation this morning.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:107: // no sub-routing
means an empty tree - all events to root
On 2017/06/07 21:19:06, rjkroege wrote:
> Ah. I am not misunderstanding.  I hope.
> 
> Then you need a case with the same picture but with a region equivalent to the
> bounds.

I will update based on our conversation this morning.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:136:
r2_hit_test_region->rect_.SetRect(0, 0, 512, 512);
On 2017/06/07 21:19:06, rjkroege wrote:
> the coordinates don't match your picture?

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:205: auto
c1_hit_test_region = HitTestRegion::New();
On 2017/06/07 21:19:06, rjkroege wrote:
> this needs a better naming convention: it's the hit test region where c1 goes
> but in the parent? as opposed to c1's region

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:213:
c2_hit_test_region->rect_.SetRect(0, 0, 512, 512);
On 2017/06/07 21:19:06, rjkroege wrote:
> don't match picture. :-)

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:218: auto
c1_hit_test_data = HitTestData::New();
On 2017/06/07 21:19:06, rjkroege wrote:
> the children don't have bounds?  i.e.: it's a bare HitTestData?

Will update based on conversation this morning.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:259: // we expect 5
entries in the tree - one for one for each child
On 2017/06/07 21:19:06, rjkroege wrote:
> Why? The children have no bounds region? or "HIT_TEST_MINE" region?

Will update based on conversation this morning.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:500: region =
RegionAtIndex(1);
On 2017/06/07 21:19:06, rjkroege wrote:
> I would expect an additional region typically yes? The region for the parent
> itself?

Will update based on conversation this morning.

rjkroege

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_test/hit_test_aggregator.cc File components/viz/hit_test/hit_test_aggregator.cc (right): https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_test/hit_test_aggregator.cc#newcode45 components/viz/hit_test/hit_test_aggregator.cc:45: display_hit_test_data_->regions_[size << 2].child_count_ = kEndOfList; On 2017/06/08 20:33:21, gklassen ...

3 years, 6 months ago (2017-06-09 15:47:08 UTC) #55

gklassen

many improvements implemented with the following major exceptions: A. I haven't abstracted memory allocation yet, ...

3 years, 6 months ago (2017-06-12 16:30:42 UTC) #56

many improvements implemented with the following major exceptions:

A. I haven't abstracted memory allocation yet, I plan to after lunch ( hoping to
come up with a good name for the base class ).
B. Bounds checking.  This may require a discussion and review.  I am happy to
re-add if we agree it's still necessary.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator.cc (right):

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:62: if
(hit_test_data->regions_.size() == 0) {
On 2017/06/09 15:47:07, rjkroege wrote:
> On 2017/06/08 20:33:21, gklassen wrote:
> > On 2017/06/07 21:19:04, rjkroege wrote:
> > > we must have at least 1 region for valid data yes? DCHECK?
> > 
> > A surface with no children may have no sub-regions no?
> 
> A HTD can have HTRs without any of them having the  HTR_CHILD_SURFACE

Right.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:79: element->child_count_ = index
- parent_index - 1;
On 2017/06/09 15:47:07, rjkroege wrote:
> On 2017/06/08 20:33:21, gklassen wrote:
> > On 2017/06/07 21:19:05, rjkroege wrote:
> > > Assumptions here should be wrapped in DCHECK
> > 
> > Agreed.  Do you have suggestions for assumptions to check?
> 
> this one. I'll note all the others that occur to me

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator_unittest.cc (right):

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:75: // one surface
On 2017/06/09 15:47:07, rjkroege wrote:
> On 2017/06/08 20:33:21, gklassen wrote:
> > On 2017/06/07 21:19:06, rjkroege wrote:
> > > Love it! Wonderful!
> > > 
> > > And... comments need to be sentences.
> > > 
> > > It would be nice to add the ascii art to the display_hit_test_data .h
> > explaining
> > > how the data structure works and reference it here
> > 
> > Agreed.  Or should this be added to an .md?
> 
> I like it here.

Acknowledged.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator_unittest.cc:500: region =
RegionAtIndex(1);
On 2017/06/09 15:47:07, rjkroege wrote:
> On 2017/06/08 20:33:22, gklassen wrote:
> > On 2017/06/07 21:19:06, rjkroege wrote:
> > > I would expect an additional region typically yes? The region for the
parent
> > > itself?
> > 
> > Will update based on conversation this morning. 
> 
> could type "done" using the handy button to mean that I should see it adjusted
> in the next CL

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
File components/viz/hit_test/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/public/interfaces/BUILD.gn:14:
"//skia/public/interfaces",
On 2017/06/07 21:19:06, rjkroege wrote:
> ?

Done.

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
File components/viz/hit_test/public/interfaces/hit_test_data.mojom (right):

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/public/interfaces/hit_test_data.mojom:16: // Region maps
to this surface ( me ).
On 2017/06/07 21:19:06, rjkroege wrote:
> It would make sense to me that HIT_TEST_MINE | HIT_TEST_BOUNDS to be set when
we
> want the smallest HTD whose bounds equal its region? This is elegant.
> 
> Perhaps you could write a helper to build HitTestData objects?

The current implementation is using HIT_TEST_MINE only.  Do we need
HIT_TEST_BOUNDS?

And yes, I would like to consider a helper to write these objects.  First I plan
to write them using the auto generated classes and then re-evaluate.  Sound ok?

https://codereview.chromium.org/2908783002/diff/320001/components/viz/hit_tes...
components/viz/hit_test/public/interfaces/hit_test_data.mojom:37: // SurfaceId
for the HIT_TEST_CHILD_SURFACE
On 2017/06/07 21:19:07, rjkroege wrote:
> If HIT_TEST_CHILD_SURFACE is set, this is not empty? Must DCHECK that.

Good call.  I have added Validation methods to DCHECK this on submit. Done.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/frame_s...
File components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2908783002/diff/360001/components/viz/frame_s...
components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc:50: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/06/09 15:47:07, rjkroege wrote:
> We must have a layering discussion with Fady. I am not sure if this
reasonable.
> Seems fine to me.

Thank you.  I have moved the switch to viz on Fady's counsel. 
Done.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
File components/viz/hit_test/display_hit_test_data.h (right):

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/display_hit_test_data.h:25: // write the hit_test data,
and other processes can read without
On 2017/06/09 15:47:08, rjkroege wrote:
> nit: only one process. The browser / ws.
> 

A very welcome nit.  Thank you. Done.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/display_hit_test_data.h:28: // One instance of this
class contains a double buffer of entries -
On 2017/06/09 15:47:08, rjkroege wrote:
> there is a need to add a TODO about setting up the shared memory scheme?
> 
> because mus is currently one process, we can add that later.

Added by abstracting allocation on your suggestion. Done.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/display_hit_test_data.h:45: base::subtle::Atomic32
read_offset_;
On 2017/06/09 15:47:07, rjkroege wrote:
> this is communication from a less-trusted process to a more trusted one. We
need
> a security review discussion early next week. I think that's an AI for me.

Acknowledged.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator.cc (right):

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:13: // TODO: Review and select
appropriate initial size ( based on telemetry? ).
On 2017/06/09 15:47:08, rjkroege wrote:
> convention is TODO(gklassen): 
> 
> i.e.: every TODO is suppose to have an owner

Done.  Thank you.  I like that.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:32: void
HitTestAggregator::OnSurfaceWillDraw(const cc::SurfaceId& surface_id) {
On 2017/06/09 15:47:08, rjkroege wrote:
> I like a blank line between defns

Me too.  Thanks for the catch. Done.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:38: active_[surface_id] =
std::move(pending_[surface_id]);
On 2017/06/09 15:47:08, rjkroege wrote:
> DCHECK that active_[surface_id] exists
> 

It may not exist though?  pending_ must exist but we runtime check that above.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:48: display_hit_test_data_ =
(DisplayHitTestData*)::operator new(cb);
On 2017/06/09 15:47:08, rjkroege wrote:
> i don't understand this line. And assuming that I do, c-style casts strongly
> undesired in Chrome

Agreed.  As discussed I've updated this code moderately to hopefully make it
clearer.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:87: element->frame_sink_id_ =
region->surface_id_.frame_sink_id();
On 2017/06/09 15:47:08, rjkroege wrote:
> Observation: not very C++-ish. Some might complain later.

Suggestions welcome - how can I make this more C++?

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:94: if (region->flags_ ==
On 2017/06/09 15:47:08, rjkroege wrote:
> have to &.
> 
> Multiple flags can be simultaneously defined

Acknowledged.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator.h (right):

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:42: // surfaces are aggregated and
put on screen.
On 2017/06/09 15:47:08, rjkroege wrote:
> nit: screen means display. This is important because... you'll have one of
these
> per Display and they can come and go as displays are added/removed. 
> 
> We can worry about this later. i.e. after this CL

Done.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:88:
base::WeakPtrFactory<HitTestAggregator> weak_ptr_factory_;
On 2017/06/09 15:47:08, rjkroege wrote:
> compiler will want this at the very bottom befpre the DISALLOW_

Done.

https://codereview.chromium.org/2908783002/diff/360001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:90: friend class
viz::hit_test::test::HitTestAggregatorTest;
On 2017/06/09 15:47:08, rjkroege wrote:
> Goes immediately below the private

Done.

rjkroege

note the renaming necessary per email to team. https://codereview.chromium.org/2908783002/diff/420001/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2908783002/diff/420001/cc/surfaces/surface_manager.h#newcode78 cc/surfaces/surface_manager.h:78: // ...

3 years, 6 months ago (2017-06-12 17:45:22 UTC) #57

rjkroege

let's chop this up into its components, land that and iterate around the transport. I ...

3 years, 6 months ago (2017-06-12 18:13:01 UTC) #58

gklassen

Another patch uploaded with improvements based on comments. Thank you again. The latest patch has ...

3 years, 6 months ago (2017-06-13 18:54:24 UTC) #59

Another patch uploaded with improvements based on comments.  Thank you again. 

The latest patch has the implementation of the abstracted memory allocation (
DisplayHitTestDataFactory ).  I would value your input and review - is this
close to what you had envisioned or can it be improved?

https://codereview.chromium.org/2908783002/diff/420001/components/viz/base/BU...
File components/viz/base/BUILD.gn (right):

https://codereview.chromium.org/2908783002/diff/420001/components/viz/base/BU...
components/viz/base/BUILD.gn:5: import("//cc/cc.gni")
On 2017/06/12 18:13:00, rjkroege wrote:
> this is most certainly wrong.

Agreed.  Thanks and Done.

https://codereview.chromium.org/2908783002/diff/420001/components/viz/base/BU...
components/viz/base/BUILD.gn:15: defines = [ "VIZ_BASE_IMPLEMENTATION=1" ]
On 2017/06/12 18:13:00, rjkroege wrote:
> ?

Thanks. Done.

https://codereview.chromium.org/2908783002/diff/420001/components/viz/base/sw...
File components/viz/base/switches.cc (right):

https://codereview.chromium.org/2908783002/diff/420001/components/viz/base/sw...
components/viz/base/switches.cc:5: #include "components/viz/base/switches.h"
On 2017/06/12 18:13:00, rjkroege wrote:
> I don't like where we've put this.  I think that it needs to be
> components/viz/service/hit_test/base

I will plan to leave it where it is based on our conversation with Fady, but I
will be happy to move later.

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator.cc (right):

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:16: constexpr int
kIncrementalSize = 1024;
On 2017/06/12 18:13:01, rjkroege wrote:
> You cannot tear this up and replace it later.

It copies over the portion in-use before switching buffers, so it should be ok? 
( Although this may evolve as we implement shared memory and if we decide to use
two buffers and add data to the BeginFrame for synchronization instead of using
the read_offset_ to synchronize swap ).

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:49:
DCHECK(ValidateHitTestData(hit_test_data));
On 2017/06/12 18:13:00, rjkroege wrote:
> We will need a comprehensive policy to deal with this that's smarter. You need
> to leave a TODO here. In particular, if a renderer is submitting invalid HTD
> instances, viz needs to tattle on it to the host and the host has to take
> appropriate action. 

Good call and thank you. Done.

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:88: // Copy over the current data
and then mark the old structure
On 2017/06/12 18:13:01, rjkroege wrote:
> Hm. We need to talk about this.

Based on the discussion yesterday is this ok for now - until we can complete the
shared memory implementation and review whether to add values to BeginFrame to
facilitate swap?

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:106: namespace {
On 2017/06/12 18:13:01, rjkroege wrote:
> one anonymous namespace at the top is conventional

Conventionalized & Done.

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:112: size +=
hit_test_data->regions_.size();
On 2017/06/12 18:13:01, rjkroege wrote:
> the regions are the same size? This is O(n). I am opposed. Track the length in
a
> member?

Agreed.  I couldn't find a convenient place to track the size - but after review
we can keep track as they are added to the active_ map.  

Done.

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.cc:127:
ResizeDisplayHitTestData(length);
On 2017/06/12 18:13:00, rjkroege wrote:
> This means that we can have a large variance in the running time of Aggregate?
> This is not necessarily a good idea. 

Agreed, but I don't know of a better place to do this work ( yet ).  Any
suggestions?

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
File components/viz/hit_test/hit_test_aggregator.h (right):

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:80: // Allocates memory for the
DisplayHitTestData Structure.
On 2017/06/12 18:13:01, rjkroege wrote:
> we discussed having some kind of injected factory thing. You chose not to do
> this?

No, I just wasn't quite done.  The most recent patch has the injected factory
thing ( I would highly value your review to see if this matches expectations or
could be improved ).

https://codereview.chromium.org/2908783002/diff/420001/components/viz/hit_tes...
components/viz/hit_test/hit_test_aggregator.h:85: // Copies current data and
marks the structure
On 2017/06/12 18:13:01, rjkroege wrote:
> I think that we need to talk about how the shared memory implementation will
> work

Agreed.  The implementation currently seems to have the minimal necessary to
facilitate testing and in-process memory allocation and this will likely evolve
as we complete the shared memory implementation.

gklassen

Another patch uploaded with improvements based on comments. Thank you again. The latest patch has ...

3 years, 6 months ago (2017-06-13 18:54:26 UTC) #60

sadrul

sadrul@chromium.org changed reviewers: + sadrul@chromium.org

3 years, 6 months ago (2017-06-16 17:07:53 UTC) #61

sadrul

https://codereview.chromium.org/2908783002/diff/480001/components/viz/hit_test/public/interfaces/hit_test_data.mojom File components/viz/hit_test/public/interfaces/hit_test_data.mojom (right): https://codereview.chromium.org/2908783002/diff/480001/components/viz/hit_test/public/interfaces/hit_test_data.mojom#newcode24 components/viz/hit_test/public/interfaces/hit_test_data.mojom:24: const uint32 kHitTestNoTouchEventHandler = 0x10; Instead of saying 'no ...

3 years, 6 months ago (2017-06-16 17:07:56 UTC) #62

gklassen

3 years, 5 months ago (2017-06-27 19:29:02 UTC) #63

New patch uploaded with flattened directory structure & improvements based on
reviewer comments.

https://codereview.chromium.org/2908783002/diff/480001/components/viz/hit_tes...
File components/viz/hit_test/public/interfaces/hit_test_data.mojom (right):

https://codereview.chromium.org/2908783002/diff/480001/components/viz/hit_tes...
components/viz/hit_test/public/interfaces/hit_test_data.mojom:24: const uint32
kHitTestNoTouchEventHandler = 0x10;
On 2017/06/16 17:07:56, sadrul wrote:
> Instead of saying 'no touch handler', I would rather go with a 'touch handler'
> enum (i.e. the default is the client does not receive touch events, and would
> need to explicitly turn the flag on to receive touch events).

Done.

https://codereview.chromium.org/2908783002/diff/480001/components/viz/hit_tes...
components/viz/hit_test/public/interfaces/hit_test_data.mojom:37: // The
transform applied to the child region.
On 2017/06/16 17:07:56, sadrul wrote:
> Do you mean the transform of the region with respect to the embedder? Because
> it's not clear what 'child region' is referring to.

Done.

Issue 2908783002: WIP Hittest Component.

Description

Patch Set 1 : rebase #

Patch Set 2 : update dependencies and owners #

Patch Set 3 : format #

Patch Set 4 : resolve presubmit checks #

Patch Set 5 : improvements driven by comments #

Patch Set 6 : Rename to HittestAggregator and create instance from GpuRootCompositorFrameSink #

Patch Set 7 : surface observer and test setup #

Patch Set 8 : improvements from reviewer comments #

Patch Set 9 : change DisplayHitTestData to POD, change case to HitTest, hook Begin Frame, first unit test works #

Patch Set 10 : hittest -> hit_test #

Patch Set 11 : 2 more tests and the tree works for at least the basic cases #

Patch Set 12 : use frame_sink_id and keep only child definitions in DisplayHitTestData tree #

Patch Set 13 : re-introduce a parent element in the tree for bounds checking #

Patch Set 14 : reviewer comments #

Patch Set 15 : updates from reviewer comments and discussion - bounds replaced with explicit regions #

Patch Set 16 : fix z-order in use case tests ( background rects at the back ) #

Patch Set 17 : Add unit test for a clipped child with a tab and transparent background #

Patch Set 18 : add transform to tab window test case #

Patch Set 19 : reviewer comments and handling of memory re-allocation #

Patch Set 20 : abstract DisplayHitTestData allocation #

Patch Set 21 : improvements based on reviewer comments #

Patch Set 22 : change HitTestFlags to constants so we can mix and match and convert to struct naming convention #

Messages