Scroll on main if impl-hit testing isn't guaranteed to be correct

Scroll on main if impl-hit testing isn't guaranteed to be correct

It often happens that we have composited layers with
holes. Sometimes we'll try and interact through the
hole (we might, say, want to scroll something back
there). The cc machinery isn't currently smart enough to
figure out exactly what got hit, so when we detect that
our hit testing result might be inaccurate we fall back
to main thread scrolling.

Here are the specifics on how I determine if me might
have hit a "holey" occluder.

I find two starting layers:
 1) The first layer below the viewport point.
 2) The first scrolling layer below the viewport point.

If we haven't hit an interfering occluder, the first
scrolling layer we should reach when we walk up from 1)
is 2). If we find some other layer, things have likely
gone wonky, so we bail.

I've also added stats so that we can track how often we
fall back to main thread scrolling because we couldn't
figure out what was going on.

R=enne@chromium.org
BUG=364090
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264306

[enne (OOO), 2014-04-15 21:25:54]

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

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2440: LayerImpl*
LayerTreeHostCommon::FindFirstScrollingLayerThatIsHitByPoint(
Ew, code duplication.  You're going to want something like this for raycasting
touch regions too.

How about a base::Callback or or templatize this with some class that has a bool
Query(LayerImpl*, gfx::PointF) function?

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2469: // Skip the HUD layer.
Sadness.

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2474: found_layer = current_layer;
readability/debuggability nit: just return current_layer here, and return NULL
instead of break and at the end?

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2478: if (current_layer->contents_opaque())
I think for correctness, it is always ok to return NULL from this function
(since it will punt to the main thread).  However, what case is this solving?

A (scrollable)
-B (not scrollable child, scrolls with A, opaque)

For a point that hits A+B, you would return NULL from this function and scroll
on the main thread.  That seems inefficient.  Why not just return A from this
function and not have this case?

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2148: LayerImpl* ancestor = child;
style nit: don't put this out of scope just to avoid a line break.

[Ian Vollick, 2014-04-15 23:27:27]

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

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2440: LayerImpl*
LayerTreeHostCommon::FindFirstScrollingLayerThatIsHitByPoint(
On 2014/04/15 21:25:54, enne wrote:
> Ew, code duplication.  You're going to want something like this for raycasting
> touch regions too.
> 
> How about a base::Callback or or templatize this with some class that has a
bool
> Query(LayerImpl*, gfx::PointF) function?
Yeah, it's pretty gross. Haste makes.. this cl. :)

It's not so cute, but I refactored the common bits in the loop to cut down on
duplication. WDYT?

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2469: // Skip the HUD layer.
On 2014/04/15 21:25:54, enne wrote:
> Sadness.
yeh.

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2474: found_layer = current_layer;
On 2014/04/15 21:25:54, enne wrote:
> readability/debuggability nit: just return current_layer here, and return NULL
> instead of break and at the end?

Done.

https://codereview.chromium.org/238803005/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:2478: if (current_layer->contents_opaque())
On 2014/04/15 21:25:54, enne wrote:
> I think for correctness, it is always ok to return NULL from this function
> (since it will punt to the main thread).  However, what case is this solving?
> 
> A (scrollable)
> -B (not scrollable child, scrolls with A, opaque)
> 
> For a point that hits A+B, you would return NULL from this function and scroll
> on the main thread.  That seems inefficient.  Why not just return A from this
> function and not have this case?
Good point. This was a stale idea from the algorithm Rick and I had been
discussing, but it's not actually helpful here. Removed.

[aelias_OOO_until_Jul13, 2014-04-16 00:48:27]

I assume this was discussed offline already, but this patch caught my eye and
I'd like to hear what the background is.  Could you explain what scenarios this
happens in and what makes it impossible to handle this correctly on the impl
thread (maybe in the patch description and/or in a linked bug)?

I'm allergic to unnecessary main-thread scrolls, since on hostile content I've
seen it cause jank catastrophes, so it would be nice to avoid it if possible.

[enne (OOO), 2014-04-16 00:49:37]

Thanks.  Looks good % tests.

[aelias_OOO_until_Jul13, 2014-04-16 03:20:29]

I chatted with Ian offline and I agreed this can land with a UMA stat tracking
the number of main-thread scrolls caused by this, since this is blocking layer
squashing, is expected to be rare, and there's a long-term plan to fix this
properly.  (If it turns out not to be rare though, we might have to revisit
sooner than expected.)

Still would like a better patch description laying out the scenario where this
can happen, for anyone else who comes across this.

[Ian Vollick, 2014-04-16 13:16:37]

On 2014/04/16 03:20:29, aelias wrote:
> I chatted with Ian offline and I agreed this can land with a UMA stat tracking
> the number of main-thread scrolls caused by this, since this is blocking layer
> squashing, is expected to be rare, and there's a long-term plan to fix this
> properly.  (If it turns out not to be rare though, we might have to revisit
> sooner than expected.)
> 
> Still would like a better patch description laying out the scenario where this
> can happen, for anyone else who comes across this.

Added a stat and test. PTAL!

[Ian Vollick, 2014-04-16 13:28:22]

On 2014/04/16 13:16:37, Ian Vollick wrote:
> On 2014/04/16 03:20:29, aelias wrote:
> > I chatted with Ian offline and I agreed this can land with a UMA stat
tracking
> > the number of main-thread scrolls caused by this, since this is blocking
layer
> > squashing, is expected to be rare, and there's a long-term plan to fix this
> > properly.  (If it turns out not to be rare though, we might have to revisit
> > sooner than expected.)
> > 
> > Still would like a better patch description laying out the scenario where
this
> > can happen, for anyone else who comes across this.
> 
> Added a stat and test. PTAL!

Wait, hold off on that, please. I want to add another test..

[Ian Vollick, 2014-04-16 13:54:13]

On 2014/04/16 13:28:22, Ian Vollick wrote:
> On 2014/04/16 13:16:37, Ian Vollick wrote:
> > On 2014/04/16 03:20:29, aelias wrote:
> > > I chatted with Ian offline and I agreed this can land with a UMA stat
> tracking
> > > the number of main-thread scrolls caused by this, since this is blocking
> layer
> > > squashing, is expected to be rare, and there's a long-term plan to fix
this
> > > properly.  (If it turns out not to be rare though, we might have to
revisit
> > > sooner than expected.)
> > > 
> > > Still would like a better patch description laying out the scenario where
> this
> > > can happen, for anyone else who comes across this.
> > 
> > Added a stat and test. PTAL!
> 
> Wait, hold off on that, please. I want to add another test..

Sorry about that -- I think it's ready for review now.

[Ian Vollick, 2014-04-16 13:58:45]

asvitkine@chromium.org: Please review changes in 

histograms/

[Alexei Svitkine (slow), 2014-04-16 14:24:11]

histograms lgtm with nit

https://codereview.chromium.org/238803005/diff/80001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/238803005/diff/80001/cc/input/input_handler.h...
cc/input/input_handler.h:54: enum ScrollStatus {
Nit: Add a comment mentioning these are used in UMA, so they shouldn't be
reordered or existing entries removed.

[Ian Vollick, 2014-04-16 14:38:13]

On 2014/04/16 14:24:11, Alexei Svitkine wrote:
> histograms lgtm with nit
> 
> https://codereview.chromium.org/238803005/diff/80001/cc/input/input_handler.h
> File cc/input/input_handler.h (right):
> 
>
https://codereview.chromium.org/238803005/diff/80001/cc/input/input_handler.h...
> cc/input/input_handler.h:54: enum ScrollStatus {
> Nit: Add a comment mentioning these are used in UMA, so they shouldn't be
> reordered or existing entries removed.

Ah, good point. Done.

[enne (OOO), 2014-04-16 16:43:13]

There's a bug for incorrect hit testing, right? Can you fill that out in the
description?

Thanks for the tests, lgtm.

[Ian Vollick, 2014-04-16 16:48:51]

On 2014/04/16 16:43:13, enne wrote:
> There's a bug for incorrect hit testing, right? Can you fill that out in the
> description?
> 
> Thanks for the tests, lgtm.

Embarrassingly there wasn't, despite all our talking about it. :/ 

I've created the bug and linked to it.

Thanks for the quick reviews.

[Ian Vollick, 2014-04-16 16:49:00]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-04-16 16:53:23]

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

[commit-bot: I haz the power, 2014-04-16 17:41:21]

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

[commit-bot: I haz the power, 2014-04-16 17:41:21]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jdduke (slow), 2014-04-16 17:41:53]

content/renderer/input lgtm with one question.

https://codereview.chromium.org/238803005/diff/100001/content/renderer/input/...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/238803005/diff/100001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:167: NOTREACHED();
Would it be at all useful to break here (and below)?

[Ian Vollick, 2014-04-16 17:57:23]

On 2014/04/16 17:41:53, jdduke wrote:
> content/renderer/input lgtm with one question.
> 
>
https://codereview.chromium.org/238803005/diff/100001/content/renderer/input/...
> File content/renderer/input/input_handler_proxy.cc (right):
> 
>
https://codereview.chromium.org/238803005/diff/100001/content/renderer/input/...
> content/renderer/input/input_handler_proxy.cc:167: NOTREACHED();
> Would it be at all useful to break here (and below)?

Yep! Done.

[Ian Vollick, 2014-04-16 17:57:51]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-04-16 18:00:03]

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

[commit-bot: I haz the power, 2014-04-16 21:22:53]

Change committed as 264306