Ensure that render surface layer list is not dirty/empty before hit testing for touch events

Ensure that render surface layer list is not dirty/empty before hit testing for touch events



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

[jamesr, 2013-01-08 01:48:22]

What does "not dirty" mean?  I understand "not empty", but do not understand why
this would lead to assuming there are touch event handlers at a given point. 
The scrolling code assumes that if this list is empty there is nothing
scrollable at the given point and ignores (throws out) the input.

[jamesr, 2013-01-08 01:48:29]

Also, where's your test?

[jamesr, 2013-01-08 01:49:34]

Ah, perhaps the "dirty" bit is referring to not calling ensure...().  That's
definitely a bug.  I think the early out should be "return false", though, and
you need a unit test.

[Yusuf, 2013-01-08 02:00:02]

I was referring to the DCHECK we started to hit in layer_tree_impl.

  // If this assert triggers, then the list is dirty.
  DCHECK(!layer_tree_host_impl_->needsUpdateDrawProperties());

So I should be returning false and we will end up sending scrollBegin and trying
to scroll and that can be ignored if necessary I guess, right?

Uploaded a patch with that change. Will be uploading another one with a test.

[enne (OOO), 2013-01-08 02:02:05]

On 2013/01/08 01:49:34, jamesr wrote:
> Ah, perhaps the "dirty" bit is referring to not calling ensure...().  That's
> definitely a bug.  I think the early out should be "return false", though, and
> you need a unit test.

If you get a hit test between commit and updateDrawProperties, you will have a
bogus layer list.  If you just call updateDrawProperties, you could handle it.

[Yusuf, 2013-01-08 02:16:36]

On 2013/01/08 02:02:05, enne wrote:
> On 2013/01/08 01:49:34, jamesr wrote:
> > Ah, perhaps the "dirty" bit is referring to not calling ensure...().  That's
> > definitely a bug.  I think the early out should be "return false", though,
and
> > you need a unit test.
> 
> If you get a hit test between commit and updateDrawProperties, you will have a
> bogus layer list.  If you just call updateDrawProperties, you could handle it.

Added unit test and calling updateDrawProperties directly now

[enne (OOO), 2013-01-08 02:26:49]

https://codereview.chromium.org/11776033/diff/3002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11776033/diff/3002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:379: updateDrawProperties();
This is just a tiny request, but can you do what scrollBegin does so that we do
the same in both places? i.e.

     if (!ensureRenderSurfaceLayerList())
         return false;

It does mostly the same thing, but I think that more clearly says why it's doing
what it's doing, and it'll be easier to find them both in the future.

[Yusuf, 2013-01-08 02:30:34]

On 2013/01/08 02:26:49, enne wrote:
> https://codereview.chromium.org/11776033/diff/3002/cc/layer_tree_host_impl.cc
> File cc/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/11776033/diff/3002/cc/layer_tree_host_impl.cc...
> cc/layer_tree_host_impl.cc:379: updateDrawProperties();
> This is just a tiny request, but can you do what scrollBegin does so that we
do
> the same in both places? i.e.
> 
>      if (!ensureRenderSurfaceLayerList())
>          return false;
> 
> It does mostly the same thing, but I think that more clearly says why it's
doing
> what it's doing, and it'll be easier to find them both in the future.

Done! Sorry I think I misunderstood your previous comment. Thanks!

[jamesr, 2013-01-08 02:31:36]

lgtm

[enne (OOO), 2013-01-08 02:32:22]

On 2013/01/08 02:30:34, Yusuf wrote:

> Done! Sorry I think I misunderstood your previous comment. Thanks!

No, my fault.  I should have been more specific.  lgtm.

[commit-bot: I haz the power, 2013-01-08 02:33:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/11776033/11001

[commit-bot: I haz the power, 2013-01-08 06:13:55]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-08 06:28:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/11776033/11001

[commit-bot: I haz the power, 2013-01-08 08:00:36]

Change committed as 175500