cc: Don't consider HUD layer for touches

cc: Don't consider HUD layer for touches

When paint rects are drawn, the HeadsUpDisplayLayerImpl is created
and drawn. However, this causes scrolling touches not to hit the
layer underneath, thus preventing the user from scrolling. The fix
is to classify the layer as a non-interactive overlay layer and
to not consider these layers as possible hit layers.

BUG=172572
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183371

[vmpstr, 2013-02-16 00:04:54]

Also, I'd appreciate a better name suggestion than "isNonInteractiveOverlay"

[jamesr, 2013-02-16 00:08:50]

Hmm, seems like a similar bugfix to
https://bugs.webkit.org/show_bug.cgi?id=108957 although that just punted all
input handling to the main thread when the overlay was up.

What about just setting the TouchHandlerEventRegion to the empty set?

[jamesr, 2013-02-16 00:08:50]

Hmm, seems like a similar bugfix to
https://bugs.webkit.org/show_bug.cgi?id=108957 although that just punted all
input handling to the main thread when the overlay was up.

What about just setting the TouchHandlerEventRegion to the empty set?

[jamesr, 2013-02-16 00:10:25]

On 2013/02/16 00:08:50, jamesr wrote:
> Hmm, seems like a similar bugfix to
> https://bugs.webkit.org/show_bug.cgi?id=108957 although that just punted all
> input handling to the main thread when the overlay was up.
> 
> What about just setting the TouchHandlerEventRegion to the empty set?

Hmm, that won't work.  You could make the hit testing logic in LayerTreeHostImpl
know to skip the hud_layer since that's stored on the LayerTreeImpl.  I'd prefer
not to add new layer properties if we can avoid it.

[nduca, 2013-02-16 00:28:40]

This smells like the skips continuous painting thing all over again.

[vmpstr, 2013-02-16 01:57:04]

On 2013/02/16 00:28:40, nduca wrote:
> This smells like the skips continuous painting thing all over again.

ptal. I had to move a couple more functions around. I moved one to math_util,
since it felt like that's where it belongs. The other one, I had to make a
static member, instead of a static nonmember, so that lthi can access it. Bulk
of the changes are in unittests now, it's mostly moving from one file to the
other. cc_unittests pass on my machine

[danakj, 2013-02-16 03:07:29]

MathUtil is the place where things live that are meant to migrate into ui/gfx -
so things should move out of MathUtil not into. I don't think this function
would belong in ui/gfx though, can it just be static where it is used still?

[vmpstr, 2013-02-17 19:33:44]

On 2013/02/16 03:07:29, danakj wrote:
> MathUtil is the place where things live that are meant to migrate into ui/gfx
-
> so things should move out of MathUtil not into. I don't think this function
> would belong in ui/gfx though, can it just be static where it is used still?

The problem is that this function is now used in both layer tree host impl and
layer tree host common, so it can't be a non member static (unless we copy it in
two places). Would you prefer it becomes static member in layer tree host
common?

[danakj, 2013-02-18 19:18:48]

I was hoping vollick's change to make tryScroll not scroll things with
maxScrollOffset = 0 would also fix this.

I guess it would not though, since that affects the ancestor walk, but would
affect which leaf node was chosen by the hit test.

I think this can be done far simpler though with a specialization function
LayerTreeHostCommon.

https://codereview.chromium.org/12280014/diff/2002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12280014/diff/2002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1247: if (currentLayer == activeTree()->hud_layer())
I don't understand why we would need to move around so much code to do this. We
do per-layer-type stuff in LayerTreeHostCommon all the time. See
updateLayerContentsScale for example.

You could just add a local bool IsHudLayer(LayerImpl*) { ... } method there to
deal with this.

That seems like it could be solved with 5 loc plus a test.

[danakj, 2013-02-18 19:19:17]

would not affect which leaf node*

[vmpstr, 2013-02-18 20:03:52]

On 2013/02/18 19:18:48, danakj wrote:
> I don't understand why we would need to move around so much code to do this.
We
> do per-layer-type stuff in LayerTreeHostCommon all the time. See
> updateLayerContentsScale for example.
> 
> You could just add a local bool IsHudLayer(LayerImpl*) { ... } method there to
> deal with this.
> 
> That seems like it could be solved with 5 loc plus a test.

I talked about patch set #1 (which changes a lot less code) with jamesr offline.
He is against adding virtuals to the layers if we can avoid it. So the two
solutions that emerged were either to pass a hud layer to this function in layer
tree host common (as an extra parameter that basically says "ignore this
layer"), or to move the function from common to lthi. This patch does the latter
(it doesn't actually change a lot of code, but it moves a lot of it around).
Adding an extra parameter just seems kind of iffy in that case, and since lthi
is the only thing that calls the function (aside form unittests) it felt like a
better solution.

I'm not sure how we'd implement IsHudLayer in layer tree host common, without
adding another function that sets a hud layer in there... It's entirely possible
that I'm missing something, so could you elaborate a bit on your proposed
solution?

[danakj, 2013-02-18 21:17:48]

On Mon, Feb 18, 2013 at 3:03 PM, <vmpstr@chromium.org> wrote:

> On 2013/02/18 19:18:48, danakj wrote:
>
>> I don't understand why we would need to move around so much code to do
>> this.
>>
> We
>
>> do per-layer-type stuff in LayerTreeHostCommon all the time. See
>> updateLayerContentsScale for example.
>>
>
>  You could just add a local bool IsHudLayer(LayerImpl*) { ... } method
>> there to
>> deal with this.
>>
>
>  That seems like it could be solved with 5 loc plus a test.
>>
>
> I talked about patch set #1 (which changes a lot less code) with jamesr
> offline.
> He is against adding virtuals to the layers if we can avoid it. So the two
> solutions that emerged were either to pass a hud layer to this function in
> layer
> tree host common (as an extra parameter that basically says "ignore this
> layer"), or to move the function from common to lthi. This patch does the
> latter
> (it doesn't actually change a lot of code, but it moves a lot of it
> around).
> Adding an extra parameter just seems kind of iffy in that case, and since
> lthi
> is the only thing that calls the function (aside form unittests) it felt
> like a
> better solution.
>
> I'm not sure how we'd implement IsHudLayer in layer tree host common,
> without
> adding another function that sets a hud layer in there... It's entirely
> possible
> that I'm missing something, so could you elaborate a bit on your proposed
> solution?
>

Sure, I was imagining this:

IsHudLayer(Layer* layer) { return false; }
IsHudLayer(LayerImpl* layer) {
  return layer == layer->layerTreeImpl()->hud_layer();
}


>
>
>
https://codereview.chromium.**org/12280014/<https://codereview.chromium.org/1...
>

[vmpstr, 2013-02-18 22:10:30]

> 
> Sure, I was imagining this:
> 
> IsHudLayer(Layer* layer) { return false; }
> IsHudLayer(LayerImpl* layer) {
>   return layer == layer->layerTreeImpl()->hud_layer();
> }
> 

Oh I didn't realize that I can access the hud layer through any LayerImpl! :)
Please see if the code is better now. Since that function is only implemented
on LayerImpl, I did something slightly different, but the general idea should
be the same.

(I haven't tested this yet... just compiled it, but I will when I'm in the
office)

[jamesr, 2013-02-18 23:10:04]

LayerTreeHostCommon is for code that's common to both the main and impl thread
layer tree types.  This is impl-thread specific, so I don't think it belongs in
*Common.

[danakj, 2013-02-19 16:01:32]

On Mon, Feb 18, 2013 at 6:10 PM, <jamesr@chromium.org> wrote:

> LayerTreeHostCommon is for code that's common to both the main and impl
> thread
> layer tree types.  This is impl-thread specific, so I don't think it
> belongs in
> *Common.
>

I guess it should move to LayerTreeImpl then (not LayerTreeHostImpl, as it
works with a single impl tree). I don't think there's anything really
specifically impl-y about that code, it could be used to hit test on either
tree, just isn't right now, but we could always move it back if there was a
reason to.


>
>
https://codereview.chromium.**org/12280014/<https://codereview.chromium.org/1...
>

[vmpstr, 2013-02-19 17:01:59]

I propose that we track the code location in this bug that tomhudson opened:

crbug.com/176917

That is to say, let's separate this fix from which class this function should
go. Does that sound reasonable?

[danakj, 2013-02-19 18:10:54]

This code is more impl-specific than I remembered. I think getting it out of
LTHCommon would definitely be the right move.

On 2013/02/19 17:01:59, vmpstr wrote:
> I propose that we track the code location in this bug that tomhudson opened:
> 
> crbug.com/176917
> 
> That is to say, let's separate this fix from which class this function should
> go. Does that sound reasonable?

I'm ok with that if James is, but it's definitely something we should do. Can
you add a test for this change?

[vmpstr, 2013-02-19 19:27:08]

> I'm ok with that if James is, but it's definitely something we should do. Can
> you add a test for this change?

Added a test. I've verified that the test fails without the patch, and passes
with the patch

[nduca, 2013-02-19 19:49:05]

@danakj, is there a path forward that isn't hideous but that could get something
landed today? Without this, turning on paint rects horribly cripples m25 and I'd
like to get this over tot here.

[danakj, 2013-02-19 20:08:12]

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:10: #include "cc/heads_up_display_layer_impl.h"
you don't need this header i don't think?

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:1142: hudLayer =
renderSurfaceLayerList.front()->layerTreeImpl()->hud_layer();
If this code is going to move to LTImpl anyways, I think this would be cleaner
if you just pass in the LayerTreeImpl* here.

You can grab the RenderSurfaceLayerList() off it, as well as the hud_layer()
then.

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
File cc/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common_unittest.cc:2975: gfx::PointF anchor(0, 0);
0.f

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common_unittest.cc:2976: gfx::PointF position(0, 0);
0.f

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common_unittest.cc:2983: hud->setForceRenderSurface(true);
why this? the hud doesn't own a surface in production.

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common_unittest.cc:2985: hud->setDrawsContent(true);
not needed for the hud. it always draws content.

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common_unittest.cc:2987:
hostImpl.activeTree()->set_hud_layer(hud.get());
I really dislike that this unit test is now needing to know about so many
classes and their APIs.

I think the best way to write this test would be to make a test in
layer_tree_host_unittest.cc, make a hud layer on the main thread, and do the hit
testing stuff in the drawLayersOnImplTread() function.

[jamesr, 2013-02-19 20:08:24]

On 2013/02/19 18:10:54, danakj wrote:
> This code is more impl-specific than I remembered. I think getting it out of
> LTHCommon would definitely be the right move.
> 
> On 2013/02/19 17:01:59, vmpstr wrote:
> > I propose that we track the code location in this bug that tomhudson opened:
> > 
> > crbug.com/176917
> > 
> > That is to say, let's separate this fix from which class this function
should
> > go. Does that sound reasonable?
> 
> I'm ok with that if James is, but it's definitely something we should do. Can
> you add a test for this change?

I'm ok with that plan.

[anagy4698, 2013-02-19 20:20:36]

Ejha, az összes fontos üzenetet elolvasta a beérkezett levelek közül!


2013/2/19 <jamesr@chromium.org>

> On 2013/02/19 18:10:54, danakj wrote:
>
>> This code is more impl-specific than I remembered. I think getting it out
>> of
>> LTHCommon would definitely be the right move.
>>
>
>  On 2013/02/19 17:01:59, vmpstr wrote:
>> > I propose that we track the code location in this bug that tomhudson
>> opened:
>> >
>> > crbug.com/176917
>> >
>> > That is to say, let's separate this fix from which class this function
>>
> should
>
>> > go. Does that sound reasonable?
>>
>
>  I'm ok with that if James is, but it's definitely something we should do.
>> Can
>> you add a test for this change?
>>
>
> I'm ok with that plan.
>
>
https://codereview.chromium.**org/12280014/<https://codereview.chromium.org/1...
>



-- 
anagy4698@gmail.com.

[danakj, 2013-02-19 22:52:58]

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:1142: hudLayer =
renderSurfaceLayerList.front()->layerTreeImpl()->hud_layer();
On 2013/02/19 20:08:12, danakj wrote:
> If this code is going to move to LTImpl anyways, I think this would be cleaner
> if you just pass in the LayerTreeImpl* here.
> 
> You can grab the RenderSurfaceLayerList() off it, as well as the hud_layer()
> then.

Nix that. It will require a lot of test changes.. let's leave that until the
code moves separately so the behaviour change is concise and clear.

This seems a little round-a-bout just to avoid a pointer deref, which will
disappear anyways when we move this into LayerTreeImpl. How about not caching
the hudLayer and just checking currentLayer == currentLayer->ltI()->hud_layer()
below?

https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:1154: if (currentLayer == hudLayer)
Since this is the least likely thing to exclude a layer, how about moving this
down to be the last continue?

[vmpstr, 2013-02-19 23:16:28]

> This seems a little round-a-bout just to avoid a pointer deref, which will
> disappear anyways when we move this into LayerTreeImpl. How about not caching
> the hudLayer and just checking currentLayer ==
currentLayer->ltI()->hud_layer()
> below?
> 
>
https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common...
> cc/layer_tree_host_common.cc:1154: if (currentLayer == hudLayer)
> Since this is the least likely thing to exclude a layer, how about moving this
> down to be the last continue?

Good point... Done.

- I need the includes, because the compiler complains when it compares
HeadsUpDisplayLayerImpl* to LayerImpl* without knowing that one is derived from
the other

- For whatever reason removing either setForceRenderSurface or setDrawsContent
calls in the unittests causes LayerTreeHostCommon::calculateDrawProperties to
come up with only one layer (the root). This makes the rest of the test moot,
since we're never even passing the hud layer to the hit test.

- I did some of the unittest changes... For the others, if the code is going to
move (along with all the relevant unittests), then I think it makes sense to
rethink that at that time... Right now it's more or less consistent with the
rest of the findLayerThatHits* tests. What do you think?

[danakj, 2013-02-19 23:24:06]

On Tue, Feb 19, 2013 at 6:16 PM, <vmpstr@chromium.org> wrote:

>  This seems a little round-a-bout just to avoid a pointer deref, which will
>> disappear anyways when we move this into LayerTreeImpl. How about not
>> caching
>> the hudLayer and just checking currentLayer ==
>>
> currentLayer->ltI()->hud_**layer()
>
>> below?
>>
>
>
> https://codereview.chromium.**org/12280014/diff/11002/cc/**
>
layer_tree_host_common.cc#**newcode1154<https://codereview.chromium.org/12280014/diff/11002/cc/layer_tree_host_common.cc#newcode1154>
>
>> cc/layer_tree_host_common.cc:**1154: if (currentLayer == hudLayer)
>> Since this is the least likely thing to exclude a layer, how about moving
>> this
>> down to be the last continue?
>>
>
> Good point... Done.
>
> - I need the includes, because the compiler complains when it compares
> HeadsUpDisplayLayerImpl* to LayerImpl* without knowing that one is derived
> from
> the other
>
> - For whatever reason removing either setForceRenderSurface or
> setDrawsContent
> calls in the unittests causes LayerTreeHostCommon::**calculateDrawProperties
> to
> come up with only one layer (the root). This makes the rest of the test
> moot,
> since we're never even passing the hud layer to the hit test.
>

Ah, so two things:

Because you went right to the impl side, and the main thread hud layer sets
the drawsContent bit, you'll need to set drawsContent to true to get this
to work.

forceRenderSurface is almost certainly not what you want as that does not
mimic the real hud. The RenderSurfaceLayerList will have size 1 (meaning 1
surface), but the surface's layerList() will have both layers in it (they
both draw to the surface).

- I did some of the unittest changes... For the others, if the code is
> going to
> move (along with all the relevant unittests), then I think it makes sense
> to
> rethink that at that time... Right now it's more or less consistent with
> the
> rest of the findLayerThatHits* tests. What do you think?
>

Yah, sounds fine, let's move on and move this code soon :)


>
https://codereview.chromium.**org/12280014/<https://codereview.chromium.org/1...
>

[vmpstr, 2013-02-19 23:38:14]

> Ah, so two things:
> 
> Because you went right to the impl side, and the main thread hud layer sets
> the drawsContent bit, you'll need to set drawsContent to true to get this
> to work.
> 
> forceRenderSurface is almost certainly not what you want as that does not
> mimic the real hud. The RenderSurfaceLayerList will have size 1 (meaning 1
> surface), but the surface's layerList() will have both layers in it (they
> both draw to the surface).
> 

Thanks for the explanation! It makes sense now: it was just my "sanity" check
was asserting a wrong thing... I removed the forceRenderSurface call, and
verified that the unittest is still good (fail without patch, pass with patch)

[danakj, 2013-02-19 23:40:50]

Cool, LGTM

[commit-bot: I haz the power, 2013-02-20 00:06:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/12280014/21001

[commit-bot: I haz the power, 2013-02-20 02:03:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/12280014/21001

[commit-bot: I haz the power, 2013-02-20 02:07:09]

Change committed as 183371