Make all scrollable layers visible to hit testing.

Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226
Committed: https://crrev.com/eec54e30707389a2e9e0335f432ca2cd4396f869
Cr-Commit-Position: refs/heads/master@{#432056}

[skobes, 2016-11-12 00:44:15]

Description was changed from

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226
==========

to

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

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

[skobes, 2016-11-12 00:44:21]

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

[commit-bot: I haz the power, 2016-11-12 00:44:45]

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

[skobes, 2016-11-12 02:03:12]

skobes@chromium.org changed reviewers:
+ enne@chromium.org, majidvp@chromium.org

[commit-bot: I haz the power, 2016-11-12 02:45:33]

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

[commit-bot: I haz the power, 2016-11-12 02:45:34]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_TIMED_OUT, no build URL)

[majidvp, 2016-11-14 17:21:11]

This looks reasonable to me.


I tried once in the past to make a similar change but
didn't see it through as I was not sure about the 
potential implications. 

Some context:
 - https://bugs.chromium.org/p/chromium/issues/detail?id=455439#c24
 - https://codereview.chromium.org/868363004/#ps20001

https://codereview.chromium.org/2495123002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2495123002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:1891: return layer->scrollable() ||
Should we also check for LayerImpl::user_scrollable here?

[skobes, 2016-11-14 17:46:33]

On 2016/11/14 17:21:11, majidvp wrote:
> I tried once in the past to make a similar change but
> didn't see it through as I was not sure about the 
> potential implications. 
> 
> Some context:
>  - https://bugs.chromium.org/p/chromium/issues/detail?id=455439#c24
>  - https://codereview.chromium.org/868363004/#ps20001

I found that patch but I was not sure why you abandoned your initial approach. 
Were there any specific issues you ran into?

https://codereview.chromium.org/2495123002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2495123002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:1891: return layer->scrollable() ||
On 2016/11/14 17:21:11, majidvp wrote:
> Should we also check for LayerImpl::user_scrollable here?

ComputeLayerScrollsDrawnDescendants was not concerned with user_scrollable().  I
think that's handled by the scroll bubbling machinery
(LayerTreeHostImpl::ComputeScrollDelta etc.)

[majidvp, 2016-11-14 18:21:28]

lgtm.

On 2016/11/14 17:46:33, skobes wrote:
> On 2016/11/14 17:21:11, majidvp wrote:
> > I tried once in the past to make a similar change but
> > didn't see it through as I was not sure about the 
> > potential implications. 
> > 
> > Some context:
> >  - https://bugs.chromium.org/p/chromium/issues/detail?id=455439#c24
> >  - https://codereview.chromium.org/868363004/#ps20001
> 
> I found that patch but I was not sure why you abandoned your initial approach.

> Were there any specific issues you ran into?
> 
I don't recall an specific issue. I think I opted out for remaining 
close to the original logic for the fear of breaking things I was not
intimately familiar with. Perhaps this was unfounded fear.

[skobes, 2016-11-14 18:22:48]

Thanks.  Waiting on @enne for OWNERS.

[enne (OOO), 2016-11-14 18:33:11]

lgtm.  I don't remember all the details of why this was implemented this way
initially, but it clearly seems wrong for this case.

[skobes, 2016-11-14 18:52:42]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2016-11-14 18:53:25]

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, 2016-11-14 20:58:10]

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

[commit-bot: I haz the power, 2016-11-14 20:58:11]

Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_TIMED_OUT, no build URL)

[skobes, 2016-11-14 21:35:19]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2016-11-14 21:41:07]

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

[skobes, 2016-11-14 21:49:37]

The CQ bit was unchecked by skobes@chromium.org

[skobes, 2016-11-14 21:49:46]

Description was changed from

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

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

to

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226
==========

[skobes, 2016-11-14 21:49:53]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2016-11-14 21:51:27]

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, 2016-11-14 22:51:31]

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

[commit-bot: I haz the power, 2016-11-14 22:51:33]

Failed to apply patch for cc/trees/scroll_node.h:
While running git apply --index -p1;
  error: patch failed: cc/trees/scroll_node.h:61
  error: cc/trees/scroll_node.h: patch does not apply

Patch:       cc/trees/scroll_node.h
Index: cc/trees/scroll_node.h
diff --git a/cc/trees/scroll_node.h b/cc/trees/scroll_node.h
index
05154e43ea6013b55f24738637d232630a4b62c1..511b895ade6d5ec4ff5339b37838e276b9f59f88
100644
--- a/cc/trees/scroll_node.h
+++ b/cc/trees/scroll_node.h
@@ -61,8 +61,6 @@ struct CC_EXPORT ScrollNode {
   bool user_scrollable_vertical;
   ElementId element_id;
   int transform_id;
-  // Number of drawn layers pointing to this node or any of its descendants.
-  int num_drawn_descendants;
 
   bool operator==(const ScrollNode& other) const;

[skobes, 2016-11-14 23:00:55]

Description was changed from

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226
==========

to

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

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

[skobes, 2016-11-14 23:02:39]

The CQ bit was checked by skobes@chromium.org

[skobes, 2016-11-14 23:02:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
majidvp@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2495123002/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-14 23:04:42]

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, 2016-11-15 01:08:48]

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

[commit-bot: I haz the power, 2016-11-15 01:08:49]

Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_TIMED_OUT, no build URL)

[skobes, 2016-11-15 01:09:19]

Description was changed from

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

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

to

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226
==========

[skobes, 2016-11-15 01:09:24]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2016-11-15 01:10:06]

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, 2016-11-15 01:58:52]

Description was changed from

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226
==========

to

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226
==========

[commit-bot: I haz the power, 2016-11-15 01:58:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-15 03:00:48]

Description was changed from

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226
==========

to

==========
Make all scrollable layers visible to hit testing.

Scrollers should remain scrollable even if their descendants are all offscreen.
An offscreen descendant with a render surface was causing num_drawn_descendants
to be decremented to 0 in ClearIsDrawnRenderSurfaceLayerListMember.

This patch removes num_drawn_descendants and scrolls_drawn_descendant in favor
of simply checking LayerImpl::scrollable().

BUG=641226

Committed: https://crrev.com/eec54e30707389a2e9e0335f432ca2cd4396f869
Cr-Commit-Position: refs/heads/master@{#432056}
==========

[commit-bot: I haz the power, 2016-11-15 03:00:50]

Patchset 2 (id:??) landed as
https://crrev.com/eec54e30707389a2e9e0335f432ca2cd4396f869
Cr-Commit-Position: refs/heads/master@{#432056}