Hit test on the layer tree rather than the RSLL

Hit test on the layer tree rather than the RSLL

R=danakj@chromium.org,enne@chromium.org
BUG=369757, 368935
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269835

[enne (OOO), 2014-05-06 20:30:58]

https://codereview.chromium.org/266913021/diff/1/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/266913021/diff/1/cc/base/math_util.cc#newcode474
cc/base/math_util.cc:474: return h.CartesianPoint3d();
What if h is clipped?

https://codereview.chromium.org/266913021/diff/1/cc/layers/draw_properties.h
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/266913021/diff/1/cc/layers/draw_properties.h#...
cc/layers/draw_properties.h:117: bool is_in_layer_list;
vmpstr is doing something like this as well in
https://codereview.chromium.org/265883013/

https://codereview.chromium.org/266913021/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/266913021/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2554: return
layer->draw_properties().is_in_layer_list ||
What is the "is_in_layer_list" function standing in for here?

[Ian Vollick, 2014-05-06 21:06:20]

Added a test to make sure that I didn't break hit testing stuff that's 3D
sorted. I'm going to have to rebase this patch on vmpstr's
(https://codereview.chromium.org/265883013/), but % that, this should be ready
for review now.

https://codereview.chromium.org/266913021/diff/1/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/266913021/diff/1/cc/base/math_util.cc#newcode474
cc/base/math_util.cc:474: return h.CartesianPoint3d();
On 2014/05/06 20:30:58, enne wrote:
> What if h is clipped?

In that case, the answer isn't valid, but won't div-by-zero. This mimics the
behavior of ProjectPoint call above; the only exception is that I don't discard
z here. (I omitted the comment because the fn's are similar -- should I put it
back?)

https://codereview.chromium.org/266913021/diff/1/cc/layers/draw_properties.h
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/266913021/diff/1/cc/layers/draw_properties.h#...
cc/layers/draw_properties.h:117: bool is_in_layer_list;
On 2014/05/06 20:30:58, enne wrote:
> vmpstr is doing something like this as well in
> https://codereview.chromium.org/265883013/

As we discussed offline, this is subsumed by vmpstr's patch, so I'll wait for
his to land. I'll upload a rebase against that patch as soon as I can.

[Ian Vollick, 2014-05-09 14:09:43]

On 2014/05/06 21:06:20, Ian Vollick wrote:
> Added a test to make sure that I didn't break hit testing stuff that's 3D
> sorted. I'm going to have to rebase this patch on vmpstr's
> (https://codereview.chromium.org/265883013/), but % that, this should be ready
> for review now.
> 
> https://codereview.chromium.org/266913021/diff/1/cc/base/math_util.cc
> File cc/base/math_util.cc (right):
> 
>
https://codereview.chromium.org/266913021/diff/1/cc/base/math_util.cc#newcode474
> cc/base/math_util.cc:474: return h.CartesianPoint3d();
> On 2014/05/06 20:30:58, enne wrote:
> > What if h is clipped?
> 
> In that case, the answer isn't valid, but won't div-by-zero. This mimics the
> behavior of ProjectPoint call above; the only exception is that I don't
discard
> z here. (I omitted the comment because the fn's are similar -- should I put it
> back?)
> 
> https://codereview.chromium.org/266913021/diff/1/cc/layers/draw_properties.h
> File cc/layers/draw_properties.h (right):
> 
>
https://codereview.chromium.org/266913021/diff/1/cc/layers/draw_properties.h#...
> cc/layers/draw_properties.h:117: bool is_in_layer_list;
> On 2014/05/06 20:30:58, enne wrote:
> > vmpstr is doing something like this as well in
> > https://codereview.chromium.org/265883013/
> 
> As we discussed offline, this is subsumed by vmpstr's patch, so I'll wait for
> his to land. I'll upload a rebase against that patch as soon as I can.

I've rebased on top of vmpstr's work, and this should be ready for review.

Dana asked me to try a couple of things offline. a) Try and use
UpdateDrawProperties rather than CalculateDrawProperties, and b) move the hit
testing functions into LayerTreeImpl.

Task b) was successful. These really are tree operations and the code looks much
clearer this way, but I couldn't in good conscience leave the tests in
layer_tree_host_common_unittest.cc since these are no longer LTHC-related. So I
kinda refactored the universe a little bit :/ I've hoisted some of the
share-able LTHC_unittest.cc machinery into a header created
layer_tree_impl_unittest.cc

Sadly a) was more difficult. Not only does UpdateDrawProperties not give you the
ability to pass testing inputs, you're also required to have a real renderer and
output surface (and these are only the first problems I encountered, there may
be more). It'd be nice if we could use UDP and I think we should revisit this,
but I'd prefer to do that in a future CL since this patch is already bananas.

[danakj, 2014-05-09 18:43:48]

Looks like PS3 is your functional changes, and the rest is code moving around.
The changes in PS3 look good, maybe enne can look over them too.

Thanks for moving things out to LayerTreeImpl that's been something waiting to
happen for a long time.

The set_current_rsll_id() thing is very bad code smell, but I understand why
it's there for now. Let's follow up and see if we can't remove that.

And there's some code in LTHCommon to add non-visible layers to the RSLL for hit
testing, and then code elsewhere that ensures we don't try to draw non-visible
layers that we should follow up and remove.

A couple small requests and LGTM

https://codereview.chromium.org/266913021/diff/120001/cc/test/layer_tree_host...
File cc/test/layer_tree_host_common_test.cc (right):

https://codereview.chromium.org/266913021/diff/120001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.cc:116: return
render_surface_layer_list_.get();
move to header

https://codereview.chromium.org/266913021/diff/120001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.cc:121: return
render_surface_layer_list_impl_.get();
move to header

https://codereview.chromium.org/266913021/diff/120001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.cc:125: return
render_surface_layer_list_count_;
move to header

https://codereview.chromium.org/266913021/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/266913021/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.h:223: 1) {
can these stay 0 ie invalid to be overridden by tests that set/use them?

[enne (OOO), 2014-05-09 19:26:04]

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1138: : closest_match(NULL), closest_distance(0.f),
found_match(false) {}
nit: Maybe initialize closest_distance to negative infinity?

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1143: bool found_match;
found_match seems redundant with closest_match != NULL

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1182: struct HitTestVisibleFunctor {
HitTestVisibleOrTouchableFunctor?

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:42:
host_impl.active_tree()->set_current_render_surface_list_id_for_testing(1);
Please, no.  Why do you need this? Aren't you calling CalculateDrawProperties
here for real?

[enne (OOO), 2014-05-09 19:26:20]

Also, moving things to LayerTreeImpl seems great to me.  Thanks for that
cleanup.

[danakj, 2014-05-09 19:36:34]

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:42:
host_impl.active_tree()->set_current_render_surface_list_id_for_testing(1);
On 2014/05/09 19:26:04, enne wrote:
> Please, no.  Why do you need this? Aren't you calling CalculateDrawProperties
> here for real?

This is needed because it's calling LTHCommon::CalcDrawProps instead of
LTI::UpdateDrawProps. The latter sets the RSLL id on the LayerTreeImpl, which
the layers compare against. Without setting the id, they compare against 0 and
say they aren't in the RSLL/aren't in the current frame.

The real answer is to call LTI::UpdateDrawProps instead, but this is a bit
complicated since it needs an OutputSurface instead. So I am feeling like that
makes a good followup.

[enne (OOO), 2014-05-09 19:42:43]

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:42:
host_impl.active_tree()->set_current_render_surface_list_id_for_testing(1);
On 2014/05/09 19:36:34, danakj wrote:
> On 2014/05/09 19:26:04, enne wrote:
> > Please, no.  Why do you need this? Aren't you calling
CalculateDrawProperties
> > here for real?
> 
> This is needed because it's calling LTHCommon::CalcDrawProps instead of
> LTI::UpdateDrawProps. The latter sets the RSLL id on the LayerTreeImpl, which
> the layers compare against. Without setting the id, they compare against 0 and
> say they aren't in the RSLL/aren't in the current frame.
> 
> The real answer is to call LTI::UpdateDrawProps instead, but this is a bit
> complicated since it needs an OutputSurface instead. So I am feeling like that
> makes a good followup.

It seems like "inputs.current_render_surface_layer_list_id =
host_impl.active_tree()->current_render_surface_list_id();" would still be
closer to reality and not involve poking state on LTI.

[danakj, 2014-05-09 19:45:11]

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:42:
host_impl.active_tree()->set_current_render_surface_list_id_for_testing(1);
On 2014/05/09 19:42:43, enne wrote:
> On 2014/05/09 19:36:34, danakj wrote:
> > On 2014/05/09 19:26:04, enne wrote:
> > > Please, no.  Why do you need this? Aren't you calling
> CalculateDrawProperties
> > > here for real?
> > 
> > This is needed because it's calling LTHCommon::CalcDrawProps instead of
> > LTI::UpdateDrawProps. The latter sets the RSLL id on the LayerTreeImpl,
which
> > the layers compare against. Without setting the id, they compare against 0
and
> > say they aren't in the RSLL/aren't in the current frame.
> > 
> > The real answer is to call LTI::UpdateDrawProps instead, but this is a bit
> > complicated since it needs an OutputSurface instead. So I am feeling like
that
> > makes a good followup.
> 
> It seems like "inputs.current_render_surface_layer_list_id =
> host_impl.active_tree()->current_render_surface_list_id();" would still be
> closer to reality and not involve poking state on LTI.

Hmmm, ya ok that'd work too. It'd break if we called UpdateDrawProps, cuz the
value would increment in the LTI, but then this would go away at that point.

[danakj, 2014-05-09 19:46:21]

On 2014/05/09 19:45:11, danakj wrote:
>
https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
> File cc/trees/layer_tree_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
> cc/trees/layer_tree_impl_unittest.cc:42:
> host_impl.active_tree()->set_current_render_surface_list_id_for_testing(1);
> On 2014/05/09 19:42:43, enne wrote:
> > On 2014/05/09 19:36:34, danakj wrote:
> > > On 2014/05/09 19:26:04, enne wrote:
> > > > Please, no.  Why do you need this? Aren't you calling
> > CalculateDrawProperties
> > > > here for real?
> > > 
> > > This is needed because it's calling LTHCommon::CalcDrawProps instead of
> > > LTI::UpdateDrawProps. The latter sets the RSLL id on the LayerTreeImpl,
> which
> > > the layers compare against. Without setting the id, they compare against 0
> and
> > > say they aren't in the RSLL/aren't in the current frame.
> > > 
> > > The real answer is to call LTI::UpdateDrawProps instead, but this is a bit
> > > complicated since it needs an OutputSurface instead. So I am feeling like
> that
> > > makes a good followup.
> > 
> > It seems like "inputs.current_render_surface_layer_list_id =
> > host_impl.active_tree()->current_render_surface_list_id();" would still be
> > closer to reality and not involve poking state on LTI.
> 
> Hmmm, ya ok that'd work too. It'd break if we called UpdateDrawProps, cuz the
> value would increment in the LTI, but then this would go away at that point.

The weird thing about that is that the LTI value is 0, so we'd use 0. But 0 is
the thing used to mark layers are *not* in the RSLL. So I think that might break
tests.

[Ian Vollick, 2014-05-09 20:11:05]

On 2014/05/09 19:46:21, danakj wrote:
> On 2014/05/09 19:45:11, danakj wrote:
> >
>
https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
> > File cc/trees/layer_tree_impl_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
> > cc/trees/layer_tree_impl_unittest.cc:42:
> > host_impl.active_tree()->set_current_render_surface_list_id_for_testing(1);
> > On 2014/05/09 19:42:43, enne wrote:
> > > On 2014/05/09 19:36:34, danakj wrote:
> > > > On 2014/05/09 19:26:04, enne wrote:
> > > > > Please, no.  Why do you need this? Aren't you calling
> > > CalculateDrawProperties
> > > > > here for real?
> > > > 
> > > > This is needed because it's calling LTHCommon::CalcDrawProps instead of
> > > > LTI::UpdateDrawProps. The latter sets the RSLL id on the LayerTreeImpl,
> > which
> > > > the layers compare against. Without setting the id, they compare against
0
> > and
> > > > say they aren't in the RSLL/aren't in the current frame.
> > > > 
> > > > The real answer is to call LTI::UpdateDrawProps instead, but this is a
bit
> > > > complicated since it needs an OutputSurface instead. So I am feeling
like
> > that
> > > > makes a good followup.
> > > 
> > > It seems like "inputs.current_render_surface_layer_list_id =
> > > host_impl.active_tree()->current_render_surface_list_id();" would still be
> > > closer to reality and not involve poking state on LTI.
> > 
> > Hmmm, ya ok that'd work too. It'd break if we called UpdateDrawProps, cuz
the
> > value would increment in the LTI, but then this would go away at that point.
> 
> The weird thing about that is that the LTI value is 0, so we'd use 0. But 0 is
> the thing used to mark layers are *not* in the RSLL. So I think that might
break
> tests.

Yes, this last point is the tricky one. Without UpdateDrawProps or manual
intervention, current_render_surface_list_id() will return zero, which is
useless in tests.

[Ian Vollick, 2014-05-09 20:16:16]

On 2014/05/09 20:11:05, Ian Vollick wrote:
> On 2014/05/09 19:46:21, danakj wrote:
> > On 2014/05/09 19:45:11, danakj wrote:
> > >
> >
>
https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
> > > File cc/trees/layer_tree_impl_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
> > > cc/trees/layer_tree_impl_unittest.cc:42:
> > >
host_impl.active_tree()->set_current_render_surface_list_id_for_testing(1);
> > > On 2014/05/09 19:42:43, enne wrote:
> > > > On 2014/05/09 19:36:34, danakj wrote:
> > > > > On 2014/05/09 19:26:04, enne wrote:
> > > > > > Please, no.  Why do you need this? Aren't you calling
> > > > CalculateDrawProperties
> > > > > > here for real?
> > > > > 
> > > > > This is needed because it's calling LTHCommon::CalcDrawProps instead
of
> > > > > LTI::UpdateDrawProps. The latter sets the RSLL id on the
LayerTreeImpl,
> > > which
> > > > > the layers compare against. Without setting the id, they compare
against
> 0
> > > and
> > > > > say they aren't in the RSLL/aren't in the current frame.
> > > > > 
> > > > > The real answer is to call LTI::UpdateDrawProps instead, but this is a
> bit
> > > > > complicated since it needs an OutputSurface instead. So I am feeling
> like
> > > that
> > > > > makes a good followup.
> > > > 
> > > > It seems like "inputs.current_render_surface_layer_list_id =
> > > > host_impl.active_tree()->current_render_surface_list_id();" would still
be
> > > > closer to reality and not involve poking state on LTI.
> > > 
> > > Hmmm, ya ok that'd work too. It'd break if we called UpdateDrawProps, cuz
> the
> > > value would increment in the LTI, but then this would go away at that
point.
> > 
> > The weird thing about that is that the LTI value is 0, so we'd use 0. But 0
is
> > the thing used to mark layers are *not* in the RSLL. So I think that might
> break
> > tests.
> 
> Yes, this last point is the tricky one. Without UpdateDrawProps or manual
> intervention, current_render_surface_list_id() will return zero, which is
> useless in tests.

I think that the right way to do this is to use UpdateDrawProps since that will
both match reality and it will not require setting this value in tests. This bug
is a release blocker and will need to be back merged into 36 (since it will
almost certainly miss the branch point). It's already pretty hairy with all the
refactoring going on. Adding code to rewrite the test fixture to make
UpdateDrawProps work in the tests will make it a tough sell to merge, so I'd
like to do that separately.

[enne (OOO), 2014-05-09 20:18:55]

Ah, ok ok.  If it's gonna get cleaned up, then this is fine.

[Ian Vollick, 2014-05-09 20:27:35]

On 2014/05/09 20:18:55, enne wrote:
> Ah, ok ok.  If it's gonna get cleaned up, then this is fine.

I'll be sure to have the follow on patch up for review before landing this.

[Ian Vollick, 2014-05-10 02:57:16]

Comments addressed. PTAL.

(Also, I've done the refactoring necessary to switch from CalcDrawProps to
UpdateDrawProperties and included them here. If it's too big, I can split it out
into another patch.)

https://codereview.chromium.org/266913021/diff/120001/cc/test/layer_tree_host...
File cc/test/layer_tree_host_common_test.cc (right):

https://codereview.chromium.org/266913021/diff/120001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.cc:116: return
render_surface_layer_list_.get();
On 2014/05/09 18:43:48, danakj wrote:
> move to header

Done.

https://codereview.chromium.org/266913021/diff/120001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.cc:121: return
render_surface_layer_list_impl_.get();
On 2014/05/09 18:43:48, danakj wrote:
> move to header

Done.

https://codereview.chromium.org/266913021/diff/120001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.cc:125: return
render_surface_layer_list_count_;
On 2014/05/09 18:43:48, danakj wrote:
> move to header

Done.

https://codereview.chromium.org/266913021/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/266913021/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.h:223: 1) {
On 2014/05/09 18:43:48, danakj wrote:
> can these stay 0 ie invalid to be overridden by tests that set/use them?

Done.

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1138: : closest_match(NULL), closest_distance(0.f),
found_match(false) {}
On 2014/05/09 19:26:04, enne wrote:
> nit: Maybe initialize closest_distance to negative infinity?

Done.

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1143: bool found_match;
On 2014/05/09 19:26:04, enne wrote:
> found_match seems redundant with closest_match != NULL

Whoops, that was dumb :) Removed.

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1182: struct HitTestVisibleFunctor {
On 2014/05/09 19:26:04, enne wrote:
> HitTestVisibleOrTouchableFunctor?

Done.

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/266913021/diff/140001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:42:
host_impl.active_tree()->set_current_render_surface_list_id_for_testing(1);
On 2014/05/09 19:45:11, danakj wrote:
> On 2014/05/09 19:42:43, enne wrote:
> > On 2014/05/09 19:36:34, danakj wrote:
> > > On 2014/05/09 19:26:04, enne wrote:
> > > > Please, no.  Why do you need this? Aren't you calling
> > CalculateDrawProperties
> > > > here for real?
> > > 
> > > This is needed because it's calling LTHCommon::CalcDrawProps instead of
> > > LTI::UpdateDrawProps. The latter sets the RSLL id on the LayerTreeImpl,
> which
> > > the layers compare against. Without setting the id, they compare against 0
> and
> > > say they aren't in the RSLL/aren't in the current frame.
> > > 
> > > The real answer is to call LTI::UpdateDrawProps instead, but this is a bit
> > > complicated since it needs an OutputSurface instead. So I am feeling like
> that
> > > makes a good followup.
> > 
> > It seems like "inputs.current_render_surface_layer_list_id =
> > host_impl.active_tree()->current_render_surface_list_id();" would still be
> > closer to reality and not involve poking state on LTI.
> 
> Hmmm, ya ok that'd work too. It'd break if we called UpdateDrawProps, cuz the
> value would increment in the LTI, but then this would go away at that point.

I've got the refactor baked into the last patch set. wdyt, should it be included
here or landed separately?

[danakj, 2014-05-10 06:13:24]

Test changes LGTM and thanks

[danakj, 2014-05-10 06:17:42]

Test changes LGTM and thanks

[Ian Vollick, 2014-05-10 10:15:22]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-05-10 10:16:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vollick@chromium.org/266913021/180001

[Ian Vollick, 2014-05-10 11:34:43]

The CQ bit was unchecked by vollick@chromium.org

[Ian Vollick, 2014-05-10 11:34:57]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2014-05-10 11:35:02]

The CQ bit was unchecked by vollick@chromium.org

[Ian Vollick, 2014-05-10 12:23:04]

On 2014/05/10 10:16:34, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/vollick@chromium.org/266913021/180001

While this matches the current behavior and fixes the bug, I realized that I'm
kinda missing the point with this CL.

What this CL (currently) corrects is finding the right scrolling layer _behind_
the first layer you hit (I used to miss a scrolling layer if it wasn't a drawn
RSLL member). When you get this wrong you can punt to the main thread when you
shouldn't have to.

The problem that I realized I haven't yet solved has to do with finding the
first layer. In the previous patch set, I still required that the first layer
you hit be a drawn RSLL member. But if you stick your finger down in an "empty"
region of a scrollable area and try to scroll, you might not hit any drawn RSLL
member. You'll hit the scroll parent which won't be in a layer list if it
doesn't draw anything.

So when finding the first layer, I should also include scrollable layers that
affect the movement of any layer that's a drawn RSLL member. (I.e., we do want
to include a scrollable layer if it doesn't draw content, but has scroll
children that do. We _don't_ want to include a scrollable layer that's backface
invisible and has been rotated away from the camera).

This appears to be a straightforward fix, but I'll have to update the hit
testing logic and add a couple tests (to check both cases in my "i.e." blurb
above).

Will post an update soon.

[Ian Vollick, 2014-05-10 17:05:21]

On 2014/05/10 12:23:04, Ian Vollick wrote:
> On 2014/05/10 10:16:34, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> > 
https://chromium-status.appspot.com/cq/vollick@chromium.org/266913021/180001
> 
> While this matches the current behavior and fixes the bug, I realized that I'm
> kinda missing the point with this CL.
> 
> What this CL (currently) corrects is finding the right scrolling layer
_behind_
> the first layer you hit (I used to miss a scrolling layer if it wasn't a drawn
> RSLL member). When you get this wrong you can punt to the main thread when you
> shouldn't have to.
> 
> The problem that I realized I haven't yet solved has to do with finding the
> first layer. In the previous patch set, I still required that the first layer
> you hit be a drawn RSLL member. But if you stick your finger down in an
"empty"
> region of a scrollable area and try to scroll, you might not hit any drawn
RSLL
> member. You'll hit the scroll parent which won't be in a layer list if it
> doesn't draw anything.
> 
> So when finding the first layer, I should also include scrollable layers that
> affect the movement of any layer that's a drawn RSLL member. (I.e., we do want
> to include a scrollable layer if it doesn't draw content, but has scroll
> children that do. We _don't_ want to include a scrollable layer that's
backface
> invisible and has been rotated away from the camera).
> 
> This appears to be a straightforward fix, but I'll have to update the hit
> testing logic and add a couple tests (to check both cases in my "i.e." blurb
> above).
> 
> Will post an update soon.

I've addressed the issue I mentioned in my previous post (see the inter-patch
diff for layer_tree_host_impl_unittest.cc and layer_tree_impl.cc for the
functional and test changes).

PTAL.

[enne (OOO), 2014-05-12 18:12:17]

lgtm

[Ian Vollick, 2014-05-12 18:16:55]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-05-12 18:17:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vollick@chromium.org/266913021/220001

[commit-bot: I haz the power, 2014-05-12 19:07:31]

Change committed as 269835