Disable clipping on root scroller's ancestors.

Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller. This means parent boxes with hidden overflow will not clip the
root scroller but this is acceptable as a property of the root scroller.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/469bcf04ced0868049864010a367b0d56a73ffa7
Cr-Commit-Position: refs/heads/master@{#418647}

[bokan, 2016-08-29 16:03:33]

Description was changed from

==========
Disable clipping on root scroller's ancestors.

Today, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller.

BUG=505516
==========

to

==========
Disable clipping on root scroller's ancestors.

Today, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2016-08-29 16:38:09]

Description was changed from

==========
Disable clipping on root scroller's ancestors.

Today, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2016-08-29 16:54:36]

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

[bokan, 2016-08-29 16:55:19]

Description was changed from

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2016-08-29 16:55:19]

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

[bokan, 2016-08-29 16:56:53]

Hi Chris,

Are you the right person to review this? If not, could you please suggest
someone?

I'm not sure if making PaintLayerCompositor aware of RootScrollerController is
the right approach but this seemed preferable to twiddling PaintLayers from
RootScrollerController. I'm happy to try an alternate approach if there's a
better way.

Thanks!

[commit-bot: I haz the power, 2016-08-29 16:57:11]

Dry run: 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-08-30 03:42:29]

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

[commit-bot: I haz the power, 2016-08-30 03:42:30]

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

[chrishtr, 2016-08-30 16:44:59]

Yes I'm a good reviewer. I'll review it today hopefully.

[chrishtr, 2016-08-31 01:23:58]

Doesn't removing this masking have a user-visible effect in some cases? If so,
what are the
cases and is losing them an acceptable side-effect of custom root scroller?

[bokan, 2016-08-31 12:23:40]

Description was changed from

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller. This doesn't have any user-visible difference since the
RootScroller is guaranteed to fill the viewport.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2016-08-31 13:32:42]

On 2016/08/31 01:23:58, chrishtr wrote:
> Doesn't removing this masking have a user-visible effect in some cases? If so,
> what are the
> cases and is losing them an acceptable side-effect of custom root scroller?

Yes, if the root scroller is inside a parent box that's overflow: hidden or
scroll, it won't get clipped. I think this is fine because:

1. This use case is unsupported for the <html> "root scroller" today so we're
not losing anything
2. We require that the root scroller is exactly viewport filling so having a
clipping box breaks that visually if not technically. Even if you position it
within the box such that it exactly fills the viewport, the top control hiding
behavior will look odd
3. We already disallow scrolling to chain past the root scroller so the parent
box is already isolated from scrolls

It allowing a parent to clip the root scroller might be a nice way to accomplish
a "border" around a scrollable page, but there's alternatives such as using
sibling elements or position-fixed within the scroller and those should stick to
the viewport edges when top controls show/hide.

The general use case for this feature is to let authors keep encapsulate
separate "views" in non-root elements but still use them as if they were the
root. I've chosen several restrictions to make this straightforward in terms of
behavior and implementation. We may want to loosen some of these restrictions in
the future but for the initial version keeping it restrictive is preferred.

[bokan, 2016-08-31 13:33:53]

Description was changed from

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller. This doesn't have any user-visible difference since the
RootScroller is guaranteed to fill the viewport.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller. This means parent boxes with hidden overflow will not clip the
root scroller but this is acceptable as a property of the root scroller.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2016-09-01 23:42:57]

This change also needs some tests.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:604:
if (m_scrollingLayer)
Could you try moving these to the sites where we call setMasksToBounds on each
of these layers, and
generalize the logic there?

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:606:
if (m_ancestorClippingLayer)
Why have the m_ancestorClippingLayer layer at all then in this case?

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:608:
if (m_childContainmentLayer)
ditto

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:345:
void setClippingOnRootScrollerAncestors();
Name it disableClippingOnRootScrollerAncestors instead?

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:485:
m_containerLayer->setMasksToBounds(shouldClip);
Could you try moving these to the sites where we call setMasksToBounds on each
of these layers, and
generalize the logic there?

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h:140:
void setClippingOnRootScrollerAncestors();
Name it disableClippingOnRootScrollerAncestors instead?

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:12:
#include "core/layout/compositing/CompositedLayerMapping.h"
Do you need this include?

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (left):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:599: 
spurious

[bokan, 2016-09-09 00:19:03]

Sorry for the long delay.

I think I've addressed all the feedback, ptal. I'll also add a few more tests if
these ones are along the lines you were thinking. Thanks.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:604:
if (m_scrollingLayer)
On 2016/09/01 23:42:56, chrishtr wrote:
> Could you try moving these to the sites where we call setMasksToBounds on each
> of these layers, and
> generalize the logic there?

Moved the m_scrollingLayer->setMasksBounds to happen in
CLM::updateScrollingLayers.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:606:
if (m_ancestorClippingLayer)
On 2016/09/01 23:42:56, chrishtr wrote:
> Why have the m_ancestorClippingLayer layer at all then in this case?

Good point, I've moved these to the arguments that get passed to
updateClippingLayers to prevent/remove them in this case.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:608:
if (m_childContainmentLayer)
On 2016/09/01 23:42:56, chrishtr wrote:
> ditto

Done.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:345:
void setClippingOnRootScrollerAncestors();
On 2016/09/01 23:42:56, chrishtr wrote:
> Name it disableClippingOnRootScrollerAncestors instead?

This method is gone now.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:485:
m_containerLayer->setMasksToBounds(shouldClip);
On 2016/09/01 23:42:57, chrishtr wrote:
> Could you try moving these to the sites where we call setMasksToBounds on each
> of these layers, and
> generalize the logic there?

Currently, setMasksToBounds gets called only when these layers get created but
this needs to get called from compositing updates, so I don't think I can move
this logic there. On the flip side, I think I can just remove the
setMasksToBounds calls in the layer creation code (in ensureRootLayer) and rely
on this code. The latest patch does this.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h:140:
void setClippingOnRootScrollerAncestors();
On 2016/09/01 23:42:57, chrishtr wrote:
> Name it disableClippingOnRootScrollerAncestors instead?

It can be used to enable clipping as well if the root scroller is moved. It's
also now used to set clipping to the default values instead of in layer creation
(see reply to your comment in this function's implementation). I've renamed to
`updateClippingOnCompositorLayers` which I think describes it better.

On a side note, do we make a distinction between `masking` and `clipping`? I've
been using the term interchangeably but don't know if that's true.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:12:
#include "core/layout/compositing/CompositedLayerMapping.h"
On 2016/09/01 23:42:57, chrishtr wrote:
> Do you need this include?

Nope, removed.

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (left):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:599: 
On 2016/09/01 23:42:57, chrishtr wrote:
> spurious

Done.

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/root-scroller/clipping-ancestor-is-composited-sibling.html
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/root-scroller/clipping-ancestor-is-composited-sibling.html:44:
testRunner.dumpAsTextWithPixelResults();
Do you know of a way to dump _just_ the pixel results? Having empty .txt files
seems like a drag.

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:98:
layer->setNeedsCompositingInputsUpdate();
I set this on all ancestor layers since we need a GraphicsLayerUpdate on all the
ancestor CompositedLayerMappings which only happens if the associated layer has
needsCompositingInputsUpdate during CompositingInputsUpdater 
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...)
but I couldn't find a way to make just setting the current layer break in
practice? Is this right or is it overkill?

[chrishtr, 2016-09-09 18:21:16]

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
(right):

https://codereview.chromium.org/2289833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h:140:
void setClippingOnRootScrollerAncestors();
On 2016/09/09 at 00:19:02, bokan wrote:
> On 2016/09/01 23:42:57, chrishtr wrote:
> > Name it disableClippingOnRootScrollerAncestors instead?
> 
> It can be used to enable clipping as well if the root scroller is moved. It's
also now used to set clipping to the default values instead of in layer creation
(see reply to your comment in this function's implementation). I've renamed to
`updateClippingOnCompositorLayers` which I think describes it better.
> 
> On a side note, do we make a distinction between `masking` and `clipping`?
I've been using the term interchangeably but don't know if that's true.

Masking and clipping are kind of the same thing. Most people say "mask" when
they mean something other than a rectangular clip due to overflow:<hidden or
scroll> or their equivalent for frames. In the cases in this CL it's mostly the
latter...

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/root-scroller/clipping-ancestor-is-composited-sibling.html
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/root-scroller/clipping-ancestor-is-composited-sibling.html:44:
testRunner.dumpAsTextWithPixelResults();
On 2016/09/09 at 00:19:03, bokan wrote:
> Do you know of a way to dump _just_ the pixel results? Having empty .txt files
seems like a drag.

I don't know of a way. Agree it's weird.

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/root-scroller/clipping-ancestor-is-composited-sibling.html:52:
<iframe id="child"
src="resources/clipping-ancestor-is-composited-sibling-iframe.html"></iframe>
Why do all your tests uses iframes?

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:456:
needsDescendantsClippingLayer = false;
Could you add some unittest examples to CompositedLayerMappingTest.cpp that
explictly checks the masking parameters?
There are examples there already that should make it easy.

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:468:
void PaintLayerCompositor::updateClippingOnCompositorLayers()
Ditto for unittesting of this method.

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:98:
layer->setNeedsCompositingInputsUpdate();
On 2016/09/09 at 00:19:03, bokan wrote:
> I set this on all ancestor layers since we need a GraphicsLayerUpdate on all
the ancestor CompositedLayerMappings which only happens if the associated layer
has needsCompositingInputsUpdate during CompositingInputsUpdater 
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...)
but I couldn't find a way to make just setting the current layer break in
practice? Is this right or is it overkill?

I think it's overkill. PaintLayer::setNeedsCompositingInputsUpdate() already
updates all ancestor PaintLayers. Does it not work? See:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...

[bokan, 2016-09-10 00:05:54]

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/root-scroller/clipping-ancestor-is-composited-sibling.html
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/root-scroller/clipping-ancestor-is-composited-sibling.html:52:
<iframe id="child"
src="resources/clipping-ancestor-is-composited-sibling-iframe.html"></iframe>
On 2016/09/09 18:21:15, chrishtr wrote:
> Why do all your tests uses iframes?

Iframes are probably the more important case for this feature (AMP specifically
mentioned it's only useful via iframes) so I want to make sure that's well
tested. I've also added some basic non-iframe tests now too.

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:456:
needsDescendantsClippingLayer = false;
On 2016/09/09 18:21:15, chrishtr wrote:
> Could you add some unittest examples to CompositedLayerMappingTest.cpp that
> explictly checks the masking parameters?
> There are examples there already that should make it easy.

Done.

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:468:
void PaintLayerCompositor::updateClippingOnCompositorLayers()
On 2016/09/09 18:21:16, chrishtr wrote:
> Ditto for unittesting of this method.

I couldn't find unit tests for PaintLayer or PaintLayerCompositor so I added it
to my existing RootScrollerTest unit tests.

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:98:
layer->setNeedsCompositingInputsUpdate();
On 2016/09/09 18:21:16, chrishtr wrote:
> On 2016/09/09 at 00:19:03, bokan wrote:
> > I set this on all ancestor layers since we need a GraphicsLayerUpdate on all
> the ancestor CompositedLayerMappings which only happens if the associated
layer
> has needsCompositingInputsUpdate during CompositingInputsUpdater 
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...)
> but I couldn't find a way to make just setting the current layer break in
> practice? Is this right or is it overkill?
> 
> I think it's overkill. PaintLayer::setNeedsCompositingInputsUpdate() already
> updates all ancestor PaintLayers. Does it not work? See:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...

My read of that is that it ensures we recursively descend into
CompositingInputsUpdater::updateRecursive but it doesn't ensure we
setNeedsGraphicsLayerUpdate at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...

It seems to be the difference between m_childNeedsCompositingInputsUpdate and
m_needsDescendantDependentCompositingInputsUpdate.

This isn't needed for any of my tests nor could I find an example where it
breaks but I'm worried that might just be because of simple cases. In the
examples I stepped through we always ended up calling
setNeedsGraphicsLayerUpdate with the Subtree scope because the root layer needed
a compositing inputs update.

I'm happy to remove the loop and see if we find any cases through usage. OTOH,
changing root scrollers should be exceedingly rare so it shouldn't be a perf hit
either. Let me know which you'd prefer.

[chrishtr, 2016-09-12 21:01:28]

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:98:
layer->setNeedsCompositingInputsUpdate();
On 2016/09/10 at 00:05:54, bokan wrote:
> On 2016/09/09 18:21:16, chrishtr wrote:
> > On 2016/09/09 at 00:19:03, bokan wrote:
> > > I set this on all ancestor layers since we need a GraphicsLayerUpdate on
all
> > the ancestor CompositedLayerMappings which only happens if the associated
layer
> > has needsCompositingInputsUpdate during CompositingInputsUpdater 
> >
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...)
> > but I couldn't find a way to make just setting the current layer break in
> > practice? Is this right or is it overkill?
> > 
> > I think it's overkill. PaintLayer::setNeedsCompositingInputsUpdate() already
> > updates all ancestor PaintLayers. Does it not work? See:
> > 
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
> 
> My read of that is that it ensures we recursively descend into
CompositingInputsUpdater::updateRecursive but it doesn't ensure we
setNeedsGraphicsLayerUpdate at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> 
> It seems to be the difference between m_childNeedsCompositingInputsUpdate and
m_needsDescendantDependentCompositingInputsUpdate.
> 
> This isn't needed for any of my tests nor could I find an example where it
breaks but I'm worried that might just be because of simple cases. In the
examples I stepped through we always ended up calling
setNeedsGraphicsLayerUpdate with the Subtree scope because the root layer needed
a compositing inputs update.
> 
> I'm happy to remove the loop and see if we find any cases through usage. OTOH,
changing root scrollers should be exceedingly rare so it shouldn't be a perf hit
either. Let me know which you'd prefer.

Oh in that case you just need
setNeedsGraphicsLayerUpdate(GraphicsLayerUpdateSubtree) on the root. or
setNeedsCompositingUpdate(CompositingUpdateRebuildTree).

[bokan, 2016-09-13 20:40:02]

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

[commit-bot: I haz the power, 2016-09-13 20:40:23]

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

[bokan, 2016-09-13 20:40:52]

ptal

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:98:
layer->setNeedsCompositingInputsUpdate();
On 2016/09/12 21:01:28, chrishtr wrote:
> On 2016/09/10 at 00:05:54, bokan wrote:
> > On 2016/09/09 18:21:16, chrishtr wrote:
> > > On 2016/09/09 at 00:19:03, bokan wrote:
> > > > I set this on all ancestor layers since we need a GraphicsLayerUpdate on
> all
> > > the ancestor CompositedLayerMappings which only happens if the associated
> layer
> > > has needsCompositingInputsUpdate during CompositingInputsUpdater 
> > >
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...)
> > > but I couldn't find a way to make just setting the current layer break in
> > > practice? Is this right or is it overkill?
> > > 
> > > I think it's overkill. PaintLayer::setNeedsCompositingInputsUpdate()
already
> > > updates all ancestor PaintLayers. Does it not work? See:
> > > 
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
> > 
> > My read of that is that it ensures we recursively descend into
> CompositingInputsUpdater::updateRecursive but it doesn't ensure we
> setNeedsGraphicsLayerUpdate at
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> > 
> > It seems to be the difference between m_childNeedsCompositingInputsUpdate
and
> m_needsDescendantDependentCompositingInputsUpdate.
> > 
> > This isn't needed for any of my tests nor could I find an example where it
> breaks but I'm worried that might just be because of simple cases. In the
> examples I stepped through we always ended up calling
> setNeedsGraphicsLayerUpdate with the Subtree scope because the root layer
needed
> a compositing inputs update.
> > 
> > I'm happy to remove the loop and see if we find any cases through usage.
OTOH,
> changing root scrollers should be exceedingly rare so it shouldn't be a perf
hit
> either. Let me know which you'd prefer.
> 
> Oh in that case you just need
> setNeedsGraphicsLayerUpdate(GraphicsLayerUpdateSubtree) on the root. or
> setNeedsCompositingUpdate(CompositingUpdateRebuildTree).

Ok, I used setNeedsCompositingUpdate since I can't touch compositing state from
here.

[commit-bot: I haz the power, 2016-09-13 21:18:25]

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

[commit-bot: I haz the power, 2016-09-13 21:18:26]

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

[bokan, 2016-09-13 22:40:05]

My newly added tests fail on the slimming_paint_v2 bot but pass on the regular
ones. I'm guessing it runs with an explicitly set group of RuntimeEnabledFlags?
Should I add the root scroller flag to that bot or disable my tests there?

[bokan, 2016-09-13 22:43:19]

On 2016/09/13 22:40:05, bokan wrote:
> My newly added tests fail on the slimming_paint_v2 bot but pass on the regular
> ones. I'm guessing it runs with an explicitly set group of
RuntimeEnabledFlags?
> Should I add the root scroller flag to that bot or disable my tests there?

Actually, given that this all deals with compositing, it's probably *actually*
failing under SPv2. Is it far enough along that I could implement this under
SPv2 as well?

[chrishtr, 2016-09-13 22:48:23]

On 2016/09/13 at 22:43:19, bokan wrote:
> On 2016/09/13 22:40:05, bokan wrote:
> > My newly added tests fail on the slimming_paint_v2 bot but pass on the
regular
> > ones. I'm guessing it runs with an explicitly set group of
RuntimeEnabledFlags?
> > Should I add the root scroller flag to that bot or disable my tests there?
> 
> Actually, given that this all deals with compositing, it's probably *actually*
failing under SPv2. Is it far enough along that I could implement this under
SPv2 as well?

No. Just add your tests to FlagExpectations/enable-slimming-paint-v2. We're not
far enough along with SPv2 to support your feature yet.

[bokan, 2016-09-13 23:00:55]

On 2016/09/13 22:48:23, chrishtr wrote:
> On 2016/09/13 at 22:43:19, bokan wrote:
> > On 2016/09/13 22:40:05, bokan wrote:
> > > My newly added tests fail on the slimming_paint_v2 bot but pass on the
> regular
> > > ones. I'm guessing it runs with an explicitly set group of
> RuntimeEnabledFlags?
> > > Should I add the root scroller flag to that bot or disable my tests there?
> > 
> > Actually, given that this all deals with compositing, it's probably
*actually*
> failing under SPv2. Is it far enough along that I could implement this under
> SPv2 as well?
> 
> No. Just add your tests to FlagExpectations/enable-slimming-paint-v2. We're
not
> far enough along with SPv2 to support your feature yet.

Done.

[chrishtr, 2016-09-14 17:16:42]

https://codereview.chromium.org/2289833002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:117:
layer->setNeedsCompositingInputsUpdate();
I think you're going to need

compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree);

because the masking etc needs to be updated, right?

[bokan, 2016-09-14 17:25:46]

https://codereview.chromium.org/2289833002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp
(right):

https://codereview.chromium.org/2289833002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:117:
layer->setNeedsCompositingInputsUpdate();
On 2016/09/14 17:16:41, chrishtr wrote:
> I think you're going to need
> 
> compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree);
> 
> because the masking etc needs to be updated, right?

Ah, yes, you're right, thanks for catching that. Done.

[chrishtr, 2016-09-14 17:26:15]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-09-14 17:26:16]

lgtm

[commit-bot: I haz the power, 2016-09-14 17:26:31]

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-09-14 19:58:09]

Description was changed from

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller. This means parent boxes with hidden overflow will not clip the
root scroller but this is acceptable as a property of the root scroller.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller. This means parent boxes with hidden overflow will not clip the
root scroller but this is acceptable as a property of the root scroller.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-09-14 19:58:12]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-09-14 20:00:12]

Description was changed from

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller. This means parent boxes with hidden overflow will not clip the
root scroller but this is acceptable as a property of the root scroller.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable clipping on root scroller's ancestors.

Currently, while the user is hiding the top controls, the compositor expands the
main frame's clipping layer so that we reveal more content. This has to happen
because we don't actually resize the anything in the Layer/Layout trees in
Blink until after the top controls are fully shown or hidden.

When an element is set as the root scroller, we want to provide the same
effect. However, since there's still masking layers above the root scroller, we
can't see the expanded clip.  Instead of expanding all the layers above the
root scroller, this CL removes the masksToBounds bit on all layers above the
root scroller. This means parent boxes with hidden overflow will not clip the
root scroller but this is acceptable as a property of the root scroller.

Also, GraphicsLayer setMasksToBoundaries kept a bit on GraphicsLayer but this
was
never read so I removed it.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/469bcf04ced0868049864010a367b0d56a73ffa7
Cr-Commit-Position: refs/heads/master@{#418647}
==========

[commit-bot: I haz the power, 2016-09-14 20:00:14]

Patchset 16 (id:??) landed as
https://crrev.com/469bcf04ced0868049864010a367b0d56a73ffa7
Cr-Commit-Position: refs/heads/master@{#418647}