Refactor unreliable hit test code to avoid owning_layers

Refactor unreliable hit test code to avoid owning_layers

In [1] we switched the unreliable hit test code from using ScrollNode's
owning_layer to using the ScrollTree's layer_id_to_scroll_node_index.
This patch improves on [1] to not use layer_id_to_scroll_node_index
either, which turned out to be owning_layer in disguise.

The existing IsClosestScrollAncestor code has been refactored into a
helper (IsInitialScrollHitTestReliable) that makes the main thread
fallback cases more obvious (see: [2,3]). A test has been added for
the failure case noticed by Ali Juma in [1] where a drawn scrollbar
is the child of a scrolling layer but itself does not scroll.

[1] https://codereview.chromium.org/2702143002
[2] https://codereview.chromium.org/238803005
[3] https://codereview.chromium.org/1878323002

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2718833002
Cr-Commit-Position: refs/heads/master@{#453462}
Committed: https://chromium.googlesource.com/chromium/src/+/47fcc62bde25cb7275e56c3b256c0d0dc75aefaa

[pdr., 2017-02-25 07:29:10]

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

[commit-bot: I haz the power, 2017-02-25 07:29:19]

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

[pdr., 2017-02-25 07:47:25]

Description was changed from

==========
Refactor unreliable hit test code to avoid owning_layers

In [1] we switched the unreliable hit test code from using ScrollNode's
owning_layer to using the ScrollTree's layer_id_to_scroll_node_index.
This patch improves on [1] to not use layer_id_to_scroll_node_index
either, which turned out to be owning_layer in disguise.

The existing IsClosestScrollAncestor code has been refactored into a
helper (IsInitialScrollHitTestReliable) that makes the main thread
fallback cases more obvious. A test has been added for the failure
case noticed by Ali Juma in https://codereview.chromium.org/2702143002.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Refactor unreliable hit test code to avoid owning_layers

In [1] we switched the unreliable hit test code from using ScrollNode's
owning_layer to using the ScrollTree's layer_id_to_scroll_node_index.
This patch improves on [1] to not use layer_id_to_scroll_node_index
either, which turned out to be owning_layer in disguise.

The existing IsClosestScrollAncestor code has been refactored into a
helper (IsInitialScrollHitTestReliable) that makes the main thread
fallback cases more obvious (see: [2,3]). A test has been added for
the failure case noticed by Ali Juma in
https://codereview.chromium.org/2702143002.

[2] https://codereview.chromium.org/238803005
[3] https://codereview.chromium.org/1878323002

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-02-25 08:05:59]

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

[commit-bot: I haz the power, 2017-02-25 08:06:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2017-02-27 21:14:47]

Description was changed from

==========
Refactor unreliable hit test code to avoid owning_layers

In [1] we switched the unreliable hit test code from using ScrollNode's
owning_layer to using the ScrollTree's layer_id_to_scroll_node_index.
This patch improves on [1] to not use layer_id_to_scroll_node_index
either, which turned out to be owning_layer in disguise.

The existing IsClosestScrollAncestor code has been refactored into a
helper (IsInitialScrollHitTestReliable) that makes the main thread
fallback cases more obvious (see: [2,3]). A test has been added for
the failure case noticed by Ali Juma in
https://codereview.chromium.org/2702143002.

[2] https://codereview.chromium.org/238803005
[3] https://codereview.chromium.org/1878323002

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Refactor unreliable hit test code to avoid owning_layers

In [1] we switched the unreliable hit test code from using ScrollNode's
owning_layer to using the ScrollTree's layer_id_to_scroll_node_index.
This patch improves on [1] to not use layer_id_to_scroll_node_index
either, which turned out to be owning_layer in disguise.

The existing IsClosestScrollAncestor code has been refactored into a
helper (IsInitialScrollHitTestReliable) that makes the main thread
fallback cases more obvious (see: [2,3]). A test has been added for
the failure case noticed by Ali Juma in [1] where a drawn scrollbar
is the child of a scrolling layer but itself does not scroll.

[1] https://codereview.chromium.org/2702143002
[2] https://codereview.chromium.org/238803005
[3] https://codereview.chromium.org/1878323002

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[pdr., 2017-02-27 21:14:47]

pdr@chromium.org changed reviewers:
+ ajuma@chromium.org

[pdr., 2017-02-27 21:15:53]

pdr@chromium.org changed reviewers:
+ bokan@chromium.org

[pdr., 2017-02-27 21:15:59]

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

[commit-bot: I haz the power, 2017-02-27 21:17:08]

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

[pdr., 2017-02-27 21:40:05]

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2648: if (scroll_node->scrollable)
The refactoring in this patch is possible because
FindFirstScrollingLayerOrScrollbarLayerThatIsHitByPoint could return a layer
that directly created a scrollable scroll node, or a layer that did not (aka a
non-scrolling drawn scrollbar). If the layer created was a scrollable scroll
node, we can just use the scroll tree to directly check that we're in the
expected scroll subtree. If the layer did not create a scrollable scroll node,
the existing code would always return false; this subtle point has been made
explicit in IsInitialScrollHitTestReliable by falling back to the main thread
for drawn scrollbars.

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

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1950: return layer->scrollable() ||
The original code was really checking the following:
if (layer->is_drawn_render_surface_layer_list_member()
  || layer->scrollable()
  || (layer->ToScrollbarLayer() &&
layer->is_drawn_render_surface_layer_list_member())
  || !layer->touch_event_handler_region().IsEmpty())

This checked is_drawn_render_surface_layer_list_member twice, and can be
refactored to only do it once.

[commit-bot: I haz the power, 2017-02-27 22:37:21]

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

[commit-bot: I haz the power, 2017-02-27 22:37:22]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-02-27 22:38:15]

I'll leave the finer points to Ali but the cleanup in general lgtm

[ajuma, 2017-02-28 00:38:58]

Generally looks great! Just have a couple questions:

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2737: first_scrolling_layer_or_drawn_scrollbar
== layer_impl)
If layer_impl is itself a non-scrollable drawn scrollbar, this returns true. I
think the old logic fell back to main thread scrolling in that case?

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1041:
std::unique_ptr<SolidColorScrollbarLayerImpl> drawnScrollbar =
drawn_scrollbar

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1065: // scrollbar layer is not an
ancestor of squash, we cannot scroll on the impl
Just to clarify, would things change at all if scrollbar layer were an ancestor,
or is it just that it's a non-scrollable scrollbar?

[pdr., 2017-02-28 01:01:36]

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2737: first_scrolling_layer_or_drawn_scrollbar
== layer_impl)
On 2017/02/28 at 00:38:58, ajuma wrote:
> If layer_impl is itself a non-scrollable drawn scrollbar, this returns true. I
think the old logic fell back to main thread scrolling in that case?

True, this should not check for equality. Updated and added a test.

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1041:
std::unique_ptr<SolidColorScrollbarLayerImpl> drawnScrollbar =
On 2017/02/28 at 00:38:58, ajuma wrote:
> drawn_scrollbar

third_party/WebKitten spotted! :D

Done

https://codereview.chromium.org/2718833002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1065: // scrollbar layer is not an
ancestor of squash, we cannot scroll on the impl
On 2017/02/28 at 00:38:58, ajuma wrote:
> Just to clarify, would things change at all if scrollbar layer were an
ancestor, or is it just that it's a non-scrollable scrollbar?

This comment was incorrectly cribbed from
ScrollWithOverlappingNonScrollableLayer. Updated with your suggestion.

[ajuma, 2017-02-28 01:05:10]

Thanks! lgtm

[pdr., 2017-02-28 01:06:06]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-02-28 01:06:07]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2718833002/#ps40001
(title: "Fix bug when directly hitting a drawn scrollbar")

[commit-bot: I haz the power, 2017-02-28 01:06:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 02:22:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488243966710250,
"parent_rev": "e1adcb83eede09036e7ca43abb0ea0e01e7d4f36", "commit_rev":
"47fcc62bde25cb7275e56c3b256c0d0dc75aefaa"}

[commit-bot: I haz the power, 2017-02-28 02:23:04]

Description was changed from

==========
Refactor unreliable hit test code to avoid owning_layers

In [1] we switched the unreliable hit test code from using ScrollNode's
owning_layer to using the ScrollTree's layer_id_to_scroll_node_index.
This patch improves on [1] to not use layer_id_to_scroll_node_index
either, which turned out to be owning_layer in disguise.

The existing IsClosestScrollAncestor code has been refactored into a
helper (IsInitialScrollHitTestReliable) that makes the main thread
fallback cases more obvious (see: [2,3]). A test has been added for
the failure case noticed by Ali Juma in [1] where a drawn scrollbar
is the child of a scrolling layer but itself does not scroll.

[1] https://codereview.chromium.org/2702143002
[2] https://codereview.chromium.org/238803005
[3] https://codereview.chromium.org/1878323002

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Refactor unreliable hit test code to avoid owning_layers

In [1] we switched the unreliable hit test code from using ScrollNode's
owning_layer to using the ScrollTree's layer_id_to_scroll_node_index.
This patch improves on [1] to not use layer_id_to_scroll_node_index
either, which turned out to be owning_layer in disguise.

The existing IsClosestScrollAncestor code has been refactored into a
helper (IsInitialScrollHitTestReliable) that makes the main thread
fallback cases more obvious (see: [2,3]). A test has been added for
the failure case noticed by Ali Juma in [1] where a drawn scrollbar
is the child of a scrolling layer but itself does not scroll.

[1] https://codereview.chromium.org/2702143002
[2] https://codereview.chromium.org/238803005
[3] https://codereview.chromium.org/1878323002

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2718833002
Cr-Commit-Position: refs/heads/master@{#453462}
Committed:
https://chromium.googlesource.com/chromium/src/+/47fcc62bde25cb7275e56c3b256c...
==========

[commit-bot: I haz the power, 2017-02-28 02:23:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/47fcc62bde25cb7275e56c3b256c...

[findit-for-me, 2017-02-28 05:38:13]

FYI: Findit identified this CL at revision 453462 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[pdr., 2017-02-28 05:59:53]

On 2017/02/28 at 05:38:13, findit-for-me wrote:
> FYI: Findit identified this CL at revision 453462 as the culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

It's not obvious to me that this change caused the failure in
RenderWidgetHostViewChildFrameTest.SwapCompositorFrame, but lets roll out to be
sure.

[pdr., 2017-02-28 06:00:20]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2723553003/ by pdr@chromium.org.

The reason for reverting is: May have caused
RenderWidgetHostViewChildFrameTest.SwapCompositorFrame to fail on windows..

[pdr., 2017-02-28 19:14:58]

On 2017/02/28 at 06:00:20, pdr. wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2723553003/ by pdr@chromium.org.
> 
> The reason for reverting is: May have caused
RenderWidgetHostViewChildFrameTest.SwapCompositorFrame to fail on windows..

Turned out to be unrelated to this patch. Relanding in
https://codereview.chromium.org/2720713004