Land layer squashing behind a flag

Land layer squashing behind a flag

This patch adds appropriate ASSERTs and flag
checks to ensure that the feature does not affect
tip of tree when the flag is disabled.

Tests and bugs will be added in upcoming patches.

BUG=261605
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163777

[shawnsingh, 2013-11-27 22:24:23]

Ian, PTAL thanks!

the html layout tests are usable interactively, but not really written as layout
tests yet, so they will fail the trybots.

[enne (OOO), 2013-11-27 23:20:26]

Exciting!

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:2089: if
(m_nextSquashedLayerIndex == 0x0fff0000) {
There's just something about temporary and "only valid during this phase" sorts
of member variables that have a code smell for me.

I think this "next squash layer index" concept would be much more elegant if it
lived in SquashingState rather than in CompositedLayerMapping.  SquashingState
also already has its own internal state (m_hasMostRecentMapping) that says
whether it's between begin/finish and so you wouldn't need this sentinel value.

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:357: CompositedLayerMapping*
groupedMapping() const { return m_groupedMapping; }
vollick: I wonder if this "groupedMapping" could be used for your "paints into
an ancestor".  One option you mentioned to me offline earlier today was the idea
of having multiple CompositedLayerMappings in a RenderLayer.  Well, here you go!

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:448: // FIXME: this last
vestigal if-statement is depressing, see if we can remove it cleanly somehow
You could put it in the squashing state destructor?  Alternatively, make a top
level assignLayersToBackings(updateRoot, layersChanged) function that calls
assignLayersToBackingsRecursive(updateRoot, squashingState, layersChange) that
wraps this cleanup?

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1069: if
(layer->compositingState() == PaintsIntoOwnBacking || layer->compositingState()
== HasOwnBackingButPaintsIntoAncestor) {
If a layer paints into an ancestor, is the offset below correct?  Should layers
that squash into such a ancestor-painting layer squash into the ancestor's space
but the layer's mapping?

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1075: if
(squashingState.hasMostRecentMapping)
I think these next few lines could be cleaner if SquashingState had a helper
"setNewMapping(...)" function that cleaned up the old mapping if needed, set
flags, etc.

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerRepainter.cpp (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerRepainter.cpp:187:
offsetRect.move(-m_renderer->layer()->offsetFromSquashingLayerOrigin());
Should you be storing all of these offsets as LayoutUnits and not IntRects? It
seems like you're losing precision all the way through this squashing pipeline.

Related: https://codereview.chromium.org/20723003/

[shawnsingh, 2013-12-02 01:55:02]

PTAL, thanks!   Responses to previous feedback inline below

About the layout test expected result:  the 3 redundent repaint rects seems to
be a separate issue not caused by this patch.  I filed
https://code.google.com/p/chromium/issues/detail?id=324672 for it.

Also, i've noticed that css3/filters/huge-region-composited.html has been
failing due to this patch - but when I tried the test manually, I notice that
that test crashes on stable and canary, too... even without my patch.

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:2089: if
(m_nextSquashedLayerIndex == 0x0fff0000) {
On 2013/11/27 23:20:26, enne wrote:
> There's just something about temporary and "only valid during this phase"
sorts
> of member variables that have a code smell for me.
> 
> I think this "next squash layer index" concept would be much more elegant if
it
> lived in SquashingState rather than in CompositedLayerMapping.  SquashingState
> also already has its own internal state (m_hasMostRecentMapping) that says
> whether it's between begin/finish and so you wouldn't need this sentinel
value.

Done - new patch is indeed much more elegant about this.

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:357: CompositedLayerMapping*
groupedMapping() const { return m_groupedMapping; }
On 2013/11/27 23:20:26, enne wrote:
> vollick: I wonder if this "groupedMapping" could be used for your "paints into
> an ancestor".  One option you mentioned to me offline earlier today was the
idea
> of having multiple CompositedLayerMappings in a RenderLayer.  Well, here you
go!

Not entirely sure what the offline discussion was, but my vision is to evolve
towards code where we assign individual _paint phases_ of RenderLayers to
compositing layers in a very flexible manner.  The only benefit of a "mapping"
would be to help us understand how things changed between updates so we don't
re-allocate and repaint things unnecessarily.  I'm not clear what multiple
CompositedLayerMappings would accomplish per RenderLayer?

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:448: // FIXME: this last
vestigal if-statement is depressing, see if we can remove it cleanly somehow
On 2013/11/27 23:20:26, enne wrote:
> You could put it in the squashing state destructor?  Alternatively, make a top
> level assignLayersToBackings(updateRoot, layersChanged) function that calls
> assignLayersToBackingsRecursive(updateRoot, squashingState, layersChange) that
> wraps this cleanup?

Done - used a toplevel wrapper function.

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1069: if
(layer->compositingState() == PaintsIntoOwnBacking || layer->compositingState()
== HasOwnBackingButPaintsIntoAncestor) {
On 2013/11/27 23:20:26, enne wrote:
> If a layer paints into an ancestor, is the offset below correct?  Should
layers
> that squash into such a ancestor-painting layer squash into the ancestor's
space
> but the layer's mapping?

Sorry, I'm not understanding what the question is.  My confusion is that
painting into ancestor is a different state than painting into a grouped
mapping... so how would both happen at the same time?

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1075: if
(squashingState.hasMostRecentMapping)
On 2013/11/27 23:20:26, enne wrote:
> I think these next few lines could be cleaner if SquashingState had a helper
> "setNewMapping(...)" function that cleaned up the old mapping if needed, set
> flags, etc.

done

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerRepainter.cpp (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerRepainter.cpp:187:
offsetRect.move(-m_renderer->layer()->offsetFromSquashingLayerOrigin());
On 2013/11/27 23:20:26, enne wrote:
> Should you be storing all of these offsets as LayoutUnits and not IntRects? It
> seems like you're losing precision all the way through this squashing
pipeline.
> 
> Related: https://codereview.chromium.org/20723003/

Would you be OK with us moving forward with this version of the patch, and I can
create a crbug report to address the subpixel issue?

For what it's worth, I believe I didn't do any additional rounding in this
patch.  So if there is any subpixel bugs, it would be that i'm using an
already-rounded value that shouldn't be rounded before I use it.

[enne (OOO), 2013-12-02 18:48:59]

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:357: CompositedLayerMapping*
groupedMapping() const { return m_groupedMapping; }
On 2013/12/02 01:55:03, shawnsingh wrote:

> I'm not clear what multiple
> CompositedLayerMappings would accomplish per RenderLayer?

Maybe I misunderstand, but this looks like multiple mappings per layer here:
m_compositedLayerMapping and m_groupedMapping.

[shawnsingh, 2013-12-02 19:46:25]

On 2013/12/02 18:48:59, enne wrote:
>
https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayer.h (right):
> 
>
https://codereview.chromium.org/88863002/diff/90001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.h:357: CompositedLayerMapping*
> groupedMapping() const { return m_groupedMapping; }
> On 2013/12/02 01:55:03, shawnsingh wrote:
> 
> > I'm not clear what multiple
> > CompositedLayerMappings would accomplish per RenderLayer?
> 
> Maybe I misunderstand, but this looks like multiple mappings per layer here:
> m_compositedLayerMapping and m_groupedMapping.

Ah... so m_groupedMapping only exists for the RenderLayer to access it's
squashingLayer when needed (AFAIK the only use case where it's needed is for
sending repaint invalidations).  For now, it is much, much simpler to place the
squashing GraphicsLayer onto the most recent CompositedLayerMapping that occurs
immediately previous in paint order, hence the m_groupedMapping pointer.

A cleaner concept would be that the RenderLayer has a reference to the exact
GraphicsLayer it paints into (well, more correctly, a GraphicsLayer reference
for each paint phase of the RenderLayer).  Perhaps we won't even need the
CompositingState enum at that time, since CompositingState primarily indicates
which backing a layer is designated to paint into.  I'm hoping we can evolve the
code towards this idea over time.

WDYT?

[Ian Vollick, 2013-12-02 21:17:04]

I'm also really excited about this. I've just got lots of what are probably very
dumb questions. :)

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:179: // when things change as
well as things getting destroyed.
I wonder if it can be driven from RLC::rebuildCompLayerTree?

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:580: // FIXME: is it correct to
fix the ASSERT by adding the squashing condition here, too?
Yeah, I think so. It should be valid for compAncestor to be squashed.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:795: if (m_squashingLayer) {
IIUC, we ultimately want to postpone geometry computation for squashing layers
until all squashed layers have finished doing their geometry stuff, right? What
would you think of breaking this out into a separate fn. We could update
geometry as if there were no squashing first, and then do a pass over the
squashing layers to update stuff for squashing? Perhaps that would be too slow?
Or wrong? Both?

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:803: // FIXME: do we need to?
is it better for some reason?
We'll always be able to move it back, if necessary since we know
offsetFromBackingRoot, right? If so, then I don't think it matters too much.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:811: if
(m_ancestorClippingLayer) {
Ancestor transform layer's gonna have to be dealt with here, too, I think. Dunno
if that warrants a FIXME, though. That patch may still fail.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:820:
m_squashingLayer->setSize(totalSquashBounds.size());
It occurs to me that perspective depends on layer size (it has an effect on the
perspective origin). I don't think we should squash layers w/
-webkit-perspective. If this is true, is it already on your radar?

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:823: // FIXME: find a cleaner
way to compute renderer offsets, hopefully that supports transforms, too?
I kinda think we should punt on squashing layers w/ 3d transforms forever.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:908: if (m_squashingLayer) {
Usually there's a bit here where we remove all children from a layer before
adding new children in. Is that not necessary for the squashing containment
layer?

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1245: // FIXME: Either the
asserts or the code in his function are broken for squashing. (I think it's the
asserts?)
It's broken. There are three layers that might be the 'top' of the CLM's
hierarchy. The ancestor clip layer, graphics layer, now squashing containment
layer, and eventually ancestor transform layer. We need to do
scrollingCoordinator->updateScrollParentForGraphicsLayer(notTheTopOfTheHierarchy,
0) for all non-NULL layers that aren't at the top, and
sc->uSPFGL->(topOfTheHierarchy, scrollParent) for the top. This fn is brittle
and badly written (sorry).

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1286: bool preserves3D =
renderer()->style()->transformStyle3D() == TransformStyle3DPreserve3D &&
!renderer()->hasReflection();
Can we write a CLM::preserves3D() helper fn? I find we're rewriting this snippet
often in a couple of places.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1886:
doPaintTask(m_squashedLayers[i], &context, clip);
Cool.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:2095: // FIXME: Perhaps we can
find a tighter more clever mechanism later.
Makes sense, but for now, this seems ok to me, too.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:4006: // FIXME: double-check that
updateGraphicsLayerGeometry is not necessary (well the squashing portion is what
might be needed? but probably not)...
Hmm. It'd be handy if we had up-to-date geometry for the to-be-squashed layer
that we could use when computing size and pos for the squashed layer, but maybe
that's wasted work.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerClipper.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerClipper.cpp:57: // FIXME: these asserts trigger
for squashing. Need to update this code to support squashing as appropriate.
It might be as simple as clearing the clip rect cache when the set of squashed
layers changes.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:527: // If the layer has overlap
or assumed overlap, but no other reasons, then it should be squashed.
Can we also allow CompositingReasonOverflowScrollingParent?

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:570: // and what happens when
the groupedMapping may have changed under our feet based on previou CLMs being
allocated/de-allocated?
We've got ptrs going in both directions right? We should be able to notify the
appropriate parties when either the squashing or squashed layers get updated,
right?

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1072: // FIXME: does this need
to be true here? Do we need more logic to decide when it should be true?
I think we'll need to plumb a boolean out of addRenderLayerTo.. We want
layersChanged = true whenever we need to rebuild the composited layer tree. If
there are any squashing changes, the tree topology will change, so I think we
need to do that rebuild.

[trchen, 2013-12-02 22:56:55]

Unsolicited feedback. :>

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1282: if
(!m_squashingContainmentLayer) {
This can merge with the if-block above, with
ASSERT(!m_squashingContainmentLayer).

Having two ifs may seem more robust, but gives reader the wrong impression that
either layer can ever exist singly.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1286: bool preserves3D =
renderer()->style()->transformStyle3D() == TransformStyle3DPreserve3D &&
!renderer()->hasReflection();
On 2013/12/02 21:17:05, Ian Vollick wrote:
> Can we write a CLM::preserves3D() helper fn? I find we're rewriting this
snippet
> often in a couple of places.

Couldn't we setPreserves3D(true) always? Squashing containment layer is supposed
to be a no-op layer. If the element wants to flatten stuffs, the stuffs will be
flattened in m_graphicsLayer already.
(Actually I feel the default value for GraphicsLayer::m_preserves3D should be
true, which is closer to no-op.)

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1294:
m_squashingLayer->removeFromParent();
No need to explicitly removeFromParent. The destructor guarantees it.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1301:
m_squashingContainmentLayer->removeFromParent();
Ditto

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1882: doPaintTask(paintInfo,
&context, clip);
Do we have to skip painting here if we're squashed into someone else's layer?

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:2099:
m_squashingLayer->setNeedsDisplay();
Why null checking is needed below but not here?

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1067: absBounds.y() +
squashingState.absToLocalOffset.height());
I don't quite understand this, but doing a linear translation to convert
absolute to local coordinate looks very fishy to me.

[shawnsingh, 2013-12-05 17:55:24]

new patch uploaded - PTAL!

Thanks for all the comments.  The patch is much improved:
 - removed a lot of cruft and unnecessary variables and bogus FIXMEs
 - fixed squashing with transformed elements
 - fixed double-painting issue
 - addressed reviewer feedback thus far

known issues:
 - clipRects seem to be wrong.
 - there still seem to be a few cases where squashing layers get positioned
incorrectly

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:179: // when things change as
well as things getting destroyed.
On 2013/12/02 21:17:05, Ian Vollick wrote:
> I wonder if it can be driven from RLC::rebuildCompLayerTree?


This FIXME should be removed.  I've convinced myself that it's appropriate to
remove references of this CLM on renderLayers when the CLM is getting destroyed,
and any other changes should rely on the full compositing update to recompute
squashing assignments.  So this code belongs either here or in
assignLayersToBackings where the CLM is de-allocated.

Also, the condition 2 lines below this is sligtly incorrect.  In the new patch I
fixed it it so that we only setGroupedMapping(0) if the renderLayer referred to
this particular CLM that's being destroyed.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:580: // FIXME: is it correct to
fix the ASSERT by adding the squashing condition here, too?
On 2013/12/02 21:17:05, Ian Vollick wrote:
> Yeah, I think so. It should be valid for compAncestor to be squashed.

Actually I've convinced myself this FIXME is bogus, too.  compAncestor should
definitely assert that it has it's own compositing mapping here.  Another way to
see this - a squashing layer should never be a compositedAncestor for other
composited content.  It would always be a leaf node in the composited layer
tree.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:795: if (m_squashingLayer) {
On 2013/12/02 21:17:05, Ian Vollick wrote:
> IIUC, we ultimately want to postpone geometry computation for squashing layers
> until all squashed layers have finished doing their geometry stuff, right?
What
> would you think of breaking this out into a separate fn. We could update
> geometry as if there were no squashing first, and then do a pass over the
> squashing layers to update stuff for squashing? Perhaps that would be too
slow?
> Or wrong? Both?

Can you please elaborate what geometry stuff is unfinished by this point?  I'm
not seeing it.  This code gets run after all layers have been assigned to the
appropriate backings (i.e. after the CLM m_squashingLayer knows all
squashedLayers that will paint into it).

I do agree we should eventually put this code outside of
updateGraphicsLayerGeometry, but it seems the appropriate time to pull this out
would be when we pull squashingLayer out entirely.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:820:
m_squashingLayer->setSize(totalSquashBounds.size());
On 2013/12/02 21:17:05, Ian Vollick wrote:
> It occurs to me that perspective depends on layer size (it has an effect on
the
> perspective origin). I don't think we should squash layers w/
> -webkit-perspective. If this is true, is it already on your radar?

Yeah, I am planning to add a canBeSquashed() function that returns false in the
various conditions we want to skip squashing.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:823: // FIXME: find a cleaner
way to compute renderer offsets, hopefully that supports transforms, too?
On 2013/12/02 21:17:05, Ian Vollick wrote:
> I kinda think we should punt on squashing layers w/ 3d transforms forever.

3d transforms would always be composited... ?
This FIXME refers to 2d transforms that do need to be supported.  There are just
2-3 known bugs that should be straightforward to fix to get 2d transforms
squashed properly, I already have the test cases for it, too.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:908: if (m_squashingLayer) {
On 2013/12/02 21:17:05, Ian Vollick wrote:
> Usually there's a bit here where we remove all children from a layer before
> adding new children in. Is that not necessary for the squashing containment
> layer?

I think I just missed it.  Added to new patch, thanks for catching!

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1245: // FIXME: Either the
asserts or the code in his function are broken for squashing. (I think it's the
asserts?)
On 2013/12/02 21:17:05, Ian Vollick wrote:
> It's broken. There are three layers that might be the 'top' of the CLM's
> hierarchy. The ancestor clip layer, graphics layer, now squashing containment
> layer, and eventually ancestor transform layer. We need to do
>
scrollingCoordinator->updateScrollParentForGraphicsLayer(notTheTopOfTheHierarchy,
> 0) for all non-NULL layers that aren't at the top, and
> sc->uSPFGL->(topOfTheHierarchy, scrollParent) for the top. This fn is brittle
> and badly written (sorry).

Please let me know if new patch does it properly =)

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1286: bool preserves3D =
renderer()->style()->transformStyle3D() == TransformStyle3DPreserve3D &&
!renderer()->hasReflection();
On 2013/12/02 22:56:55, trchen wrote:
> On 2013/12/02 21:17:05, Ian Vollick wrote:
> > Can we write a CLM::preserves3D() helper fn? I find we're rewriting this
> snippet
> > often in a couple of places.
> 
> Couldn't we setPreserves3D(true) always? Squashing containment layer is
supposed
> to be a no-op layer. If the element wants to flatten stuffs, the stuffs will
be
> flattened in m_graphicsLayer already.
> (Actually I feel the default value for GraphicsLayer::m_preserves3D should be
> true, which is closer to no-op.)

I would prefer to call it CLM::owningLayerPreserves3D()

we cannot just setPreserves3D(true).  This would be creating a random 3d
rendering context where one should not exist, and layers would incorrectly sort
against each other.  The example of this is shown in the attached test case here
- crbug.com/322628

I'm going to wait until Ian's work about this is done, and rebase correctly on
top of that.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1294:
m_squashingLayer->removeFromParent();
On 2013/12/02 22:56:55, trchen wrote:
> No need to explicitly removeFromParent. The destructor guarantees it.

The style in the rest of this file does explicitly use removeFromParent() - I
would prefer to keep it uniform with the rest of the file, and we could discuss
this cleanup separately...

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1882: doPaintTask(paintInfo,
&context, clip);
On 2013/12/02 22:56:55, trchen wrote:
> Do we have to skip painting here if we're squashed into someone else's layer?

We will guarantee not to reach this code when the squashing GraphicsLayer is
asking to paint.  And we are also guaranteed that a squashed RenderLayer will
never own it's own OwnPtr compositedLayerMapping.  (This is asserted in
compositingState() accessor, and possibly other places)  

So that means we would never ask a squashed RenderLayer to paint in this chunk
of code.

That said, there is a repaint bug that should be straightforward for me to solve
where i'm accidentally double-painting squashed contents.  I believe that's a
bug in RenderLayer.cpp paint() entry points.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:2099:
m_squashingLayer->setNeedsDisplay();
On 2013/12/02 22:56:55, trchen wrote:
> Why null checking is needed below but not here?

Thanks for catching this - null check is needed in both places.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerClipper.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerClipper.cpp:57: // FIXME: these asserts trigger
for squashing. Need to update this code to support squashing as appropriate.
On 2013/12/02 21:17:05, Ian Vollick wrote:
> It might be as simple as clearing the clip rect cache when the set of squashed
> layers changes.

will look into it soon =)

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:527: // If the layer has overlap
or assumed overlap, but no other reasons, then it should be squashed.
On 2013/12/02 21:17:05, Ian Vollick wrote:
> Can we also allow CompositingReasonOverflowScrollingParent?

I don't think we can yet.  If you remember we had discussed a while ago - we
would need to support the ability to create multiple squashed layers for
squashed content due to overlap and squashed content due to overflow scrolling.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:570: // and what happens when
the groupedMapping may have changed under our feet based on previou CLMs being
allocated/de-allocated?
On 2013/12/02 21:17:05, Ian Vollick wrote:
> We've got ptrs going in both directions right? We should be able to notify the
> appropriate parties when either the squashing or squashed layers get updated,
> right?

Short answer - yes.  longer answer - I want to brainstorm a taxonomy of various
transitions a layer can have among compositingStates, and make sure that all
plumbing exists to invalidate in each scenario of the taxonomy - if you don't
mind, it'd be helpful to do this offline together.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1067: absBounds.y() +
squashingState.absToLocalOffset.height());
On 2013/12/02 22:56:55, trchen wrote:
> I don't quite understand this, but doing a linear translation to convert
> absolute to local coordinate looks very fishy to me.

New patch works for transforms =)  (I have the tests locally, just need to make
them into proper layout tests)

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1072: // FIXME: does this need
to be true here? Do we need more logic to decide when it should be true?
On 2013/12/02 21:17:05, Ian Vollick wrote:
> I think we'll need to plumb a boolean out of addRenderLayerTo.. We want
> layersChanged = true whenever we need to rebuild the composited layer tree. If
> there are any squashing changes, the tree topology will change, so I think we
> need to do that rebuild.

Agreed - but not yet done in latest patch =/

[Ian Vollick, 2013-12-06 21:11:29]

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:795: if (m_squashingLayer) {
On 2013/12/05 17:55:24, shawnsingh wrote:
> On 2013/12/02 21:17:05, Ian Vollick wrote:
> > IIUC, we ultimately want to postpone geometry computation for squashing
layers
> > until all squashed layers have finished doing their geometry stuff, right?
> What
> > would you think of breaking this out into a separate fn. We could update
> > geometry as if there were no squashing first, and then do a pass over the
> > squashing layers to update stuff for squashing? Perhaps that would be too
> slow?
> > Or wrong? Both?
> 
> Can you please elaborate what geometry stuff is unfinished by this point?  I'm
> not seeing it.  This code gets run after all layers have been assigned to the
> appropriate backings (i.e. after the CLM m_squashingLayer knows all
> squashedLayers that will paint into it).
> 
> I do agree we should eventually put this code outside of
> updateGraphicsLayerGeometry, but it seems the appropriate time to pull this
out
> would be when we pull squashingLayer out entirely.
I was incorrectly worried that calculateCompositedLayerBounds would return the
wrong thing until the updateGraphicsLayerGeometry was done.

https://codereview.chromium.org/88863002/diff/130001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:823: // FIXME: find a cleaner
way to compute renderer offsets, hopefully that supports transforms, too?
On 2013/12/05 17:55:24, shawnsingh wrote:
> On 2013/12/02 21:17:05, Ian Vollick wrote:
> > I kinda think we should punt on squashing layers w/ 3d transforms forever.
> 
> 3d transforms would always be composited... ?
> This FIXME refers to 2d transforms that do need to be supported.  There are
just
> 2-3 known bugs that should be straightforward to fix to get 2d transforms
> squashed properly, I already have the test cases for it, too.
(facepalm). Of course. Sorry, carry on..

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1263: // FIXME: not 100%
certain if this scrollParent code adjustment for squashing is correct?.
Looks correct, but would you mind doing a minor cleanup for me?
RL* topLayer = childForSuperlayers();
updateScrollParent(m_squashingContainmentLayer.get(), topLayer, scrollParent,
sc);
updateScrollParent(m_ancestorClippingLayer.get(), ...
...

void updateScrollParent(layer, topLayer, scrollParent, sc)
{
  if (!layer) return;
  sc->updateScrollParentForGraphicsLayer(layer, layer == topLayer ? scrollParent
: 0);
}

I think this will scale better if we add more layers to the tree.

[Ian Vollick, 2013-12-07 02:44:42]

This is looking nice. I've just got a few minor comments.

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1263: // FIXME: not 100%
certain if this scrollParent code adjustment for squashing is correct?.
On 2013/12/06 21:11:30, Ian Vollick wrote:
> Looks correct, but would you mind doing a minor cleanup for me?
> RL* topLayer = childForSuperlayers();
> updateScrollParent(m_squashingContainmentLayer.get(), topLayer, scrollParent,
> sc);
> updateScrollParent(m_ancestorClippingLayer.get(), ...
> ...
> 
> void updateScrollParent(layer, topLayer, scrollParent, sc)
> {
>   if (!layer) return;
>   sc->updateScrollParentForGraphicsLayer(layer, layer == topLayer ?
scrollParent
> : 0);
> }
> 
> I think this will scale better if we add more layers to the tree.

On second thought, I'm pretty confident this is correct. Let's just go with this
for now and I'll land a cleanup later.

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:4021: // FIXME: double-check that
updateGraphicsLayerGeometry is not necessary (well the squashing portion is what
might be needed? but probably not)...
I think we do want to update the geometry here. The position of the squashed
layer depends on the position of m_graphicsLayer and m_ancestorClippingLayer.
Those could updated due to this style change, couldn't they?

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:490: 
Unnecessary ws change?

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1610: bool
needsOwnBackingDueToOverlap = !isLayerSquashingEnabled() &&
requiresSquashing(layer->compositingReasons());
Does this not imply that the only reason we'd ever squash is overlap? Will that
always be true?

[Ian Vollick, 2013-12-11 16:45:17]

I see you've posted a new patch set, but I don't see another comment on this
here. In my previous comments, I've raised a few questions, -- when you have a
moment, could you please respond to them?

https://codereview.chromium.org/88863002/diff/160001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/160001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1035: static IntPoint
computeOffsetFromAbsolute(RenderLayer* layer)
why was this moved?

[shawnsingh, 2013-12-11 18:50:33]

PTAL - I've addressed your comments now.  And rebased to closer to tip of tree. 
Should be able to run trybots in just a few hours, dependency is running through
the cq now.

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1263: // FIXME: not 100%
certain if this scrollParent code adjustment for squashing is correct?.
> On second thought, I'm pretty confident this is correct. Let's just go with
this
> for now and I'll land a cleanup later.

Your cleanup landed anyway, and the latest patch is rebased on top of it, so
that's that. =)

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:4021: // FIXME: double-check that
updateGraphicsLayerGeometry is not necessary (well the squashing portion is what
might be needed? but probably not)...
On 2013/12/07 02:44:42, Ian Vollick wrote:
> I think we do want to update the geometry here. The position of the squashed
> layer depends on the position of m_graphicsLayer and m_ancestorClippingLayer.
> Those could updated due to this style change, couldn't they?

(a) if the squashing layer is affected by m_graphicsLayer and
m_ancestorClippingLayer, they will be updated when updateGraphicsLayerGeometry()
is called by those layers.
(b) if the squashing layer is affected by layers of it's own, it won't affect
m_graphicsLayer and m_ancestorClippingLayer.

However, the idea that the squashingLayer itself may change it's own
size/position due to a style change.... means that we do probably need to
updateGraphicsLayerGeometry here.  New patch adds it, but keeps a variation of
the FIXME to follow-up later.

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:490: 
On 2013/12/07 02:44:42, Ian Vollick wrote:
> Unnecessary ws change?

removed

https://codereview.chromium.org/88863002/diff/150001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1610: bool
needsOwnBackingDueToOverlap = !isLayerSquashingEnabled() &&
requiresSquashing(layer->compositingReasons());
On 2013/12/07 02:44:42, Ian Vollick wrote:
> Does this not imply that the only reason we'd ever squash is overlap? Will
that
> always be true?

No, it may not always be true... re-named variable to avoid this confusion.

https://codereview.chromium.org/88863002/diff/160001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/160001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1035: static IntPoint
computeOffsetFromAbsolute(RenderLayer* layer)
On 2013/12/11 16:45:18, Ian Vollick wrote:
> why was this moved?

Nothing was moved?  Or maybe i'm misunderstanding what you're asking.   The diff
itself is a little unclean, but it is what I intended...  This is a new helper
function to make it smoother to get the correct painting offsets plumbed into
the paint task for squashing layers.   assignLayersToBackings still exists as it
did before, but it's in the diff only because of the new SquashingState arg.

[Ian Vollick, 2013-12-11 19:47:22]

lgtm

https://codereview.chromium.org/88863002/diff/160001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/88863002/diff/160001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1035: static IntPoint
computeOffsetFromAbsolute(RenderLayer* layer)
On 2013/12/11 18:50:34, shawnsingh wrote:
> On 2013/12/11 16:45:18, Ian Vollick wrote:
> > why was this moved?
> 
> Nothing was moved?  Or maybe i'm misunderstanding what you're asking.   The
diff
> itself is a little unclean, but it is what I intended...  This is a new helper
> function to make it smoother to get the correct painting offsets plumbed into
> the paint task for squashing layers.   assignLayersToBackings still exists as
it
> did before, but it's in the diff only because of the new SquashingState arg.

This was my mistake. I misinterpreted the patch to patch diff :)

[commit-bot: I haz the power, 2013-12-11 21:54:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/88863002/190001

[commit-bot: I haz the power, 2013-12-12 00:41:08]

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

[Ian Vollick, 2013-12-12 05:46:08]

On 2013/12/12 00:41:08, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

slgtm

[Ian Vollick, 2013-12-12 05:46:12]

On 2013/12/12 00:41:08, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

slgtm

[commit-bot: I haz the power, 2013-12-12 05:46:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/88863002/210001

[commit-bot: I haz the power, 2013-12-12 07:34:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/88863002/230001

[commit-bot: I haz the power, 2013-12-12 12:18:37]

Change committed as 163777