Issue repaints before and after changing the squashing assignment of a RenderLayer.

Issue repaints before and after changing the squashing assignment of a RenderLayer.

Also, do not include squashed layers when computing repaint
rects including descendants. Squashed layers paint separately
with a different origin.

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

[chrishtr, 2014-06-02 20:16:24]

The compositing disabler in this CL is for the scenario Elliott brought up at
the meeting today.

https://codereview.chromium.org/309093002/diff/40002/LayoutTests/compositing/...
File LayoutTests/compositing/squashing/add-remove-squashed-layers-expected.txt
(right):

https://codereview.chromium.org/309093002/diff/40002/LayoutTests/compositing/...
LayoutTests/compositing/squashing/add-remove-squashed-layers-expected.txt:42:
(rect 300.00 300.00 100.00 100.00)
Repaint for overlap3 when getting added to the squashing layer.

https://codereview.chromium.org/309093002/diff/40002/LayoutTests/compositing/...
LayoutTests/compositing/squashing/add-remove-squashed-layers-expected.txt:59:
(rect 160.00 160.00 20.00 20.00)
The actual rect generated is (160 160 260 60), after assigning overlap3 to the
squashing layer.
This is clipped to the (180, 180) bounds of the  squashing graphics layer at the
time of the invalidation; the squashing layer is subsequently changed to bounds
of (260, 260).
It's an example of incorrect repaint rects due to trying to do this update
during
compositing update, as opposed to after it is done.

https://codereview.chromium.org/309093002/diff/40002/LayoutTests/compositing/...
LayoutTests/compositing/squashing/add-remove-squashed-layers-expected.txt:146:
(rect 220.00 220.00 100.00 100.00)
This and the ones below are the same situation as above.

https://codereview.chromium.org/309093002/diff/40002/LayoutTests/compositing/...
File LayoutTests/compositing/squashing/squash-above-fixed-1-expected.txt
(right):

https://codereview.chromium.org/309093002/diff/40002/LayoutTests/compositing/...
LayoutTests/compositing/squashing/squash-above-fixed-1-expected.txt:52: (rect
0.00 300.00 200.00 100.00)
These repaints are because the layer they were squashing into became no longer
composited (because it no longer overlaps the fixed pos element).

https://codereview.chromium.org/309093002/diff/40002/LayoutTests/compositing/...
LayoutTests/compositing/squashing/squash-above-fixed-1-expected.txt:131: (rect
0.00 400.00 200.00 100.00)
Similar situation to above.

[chrishtr, 2014-06-03 16:01:33]

ping

[Ian Vollick, 2014-06-03 17:05:15]

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

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:159:
DisableCompositingQueryAsserts disabler;
This seems reasonable to me, but I wonder if it would make sense to add a
possiblyStaleCompositingState accessor for use during repainting. We want to
invalidate our old rects in terms of the old composited layer tree, right? If
that's the case, maybe we should formally allow accessing the stale state?

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:162: if (curr->compositingState()
!= PaintsIntoOwnBacking && curr->compositingState() != PaintsIntoGroupedBacking)
Do we want to include HasOwnBackingButPaintsIntoAncestor here? It seems like we
shouldn't? Here and below.

[chrishtr, 2014-06-03 17:14:51]

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

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:159:
DisableCompositingQueryAsserts disabler;
On 2014/06/03 17:05:16, Ian Vollick wrote:
> This seems reasonable to me, but I wonder if it would make sense to add a
> possiblyStaleCompositingState accessor for use during repainting.

> We want to
> invalidate our old rects in terms of the old composited layer tree, right?

Yes.

> If that's the case, maybe we should formally allow accessing the stale state?

Separating old and new is what we need to do, I agree.

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:162: if (curr->compositingState()
!= PaintsIntoOwnBacking && curr->compositingState() != PaintsIntoGroupedBacking)
On 2014/06/03 17:05:16, Ian Vollick wrote:
> Do we want to include HasOwnBackingButPaintsIntoAncestor here? It seems like
we
> shouldn't? Here and below.

Don't know for sure. I don't really understand
HasOwnBackingButPaintsIntoAncestor.

[Ian Vollick, 2014-06-03 17:59:26]

On 2014/06/03 17:14:51, chrishtr wrote:
>
https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayerRepainter.cpp (right):
> 
>
https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayerRepainter.cpp:159:
> DisableCompositingQueryAsserts disabler;
> On 2014/06/03 17:05:16, Ian Vollick wrote:
> > This seems reasonable to me, but I wonder if it would make sense to add a
> > possiblyStaleCompositingState accessor for use during repainting.
> 
> > We want to
> > invalidate our old rects in terms of the old composited layer tree, right?
> 
> Yes.
> 
> > If that's the case, maybe we should formally allow accessing the stale
state?
> 
> Separating old and new is what we need to do, I agree.
> 
>
https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayerRepainter.cpp:162: if
(curr->compositingState()
> != PaintsIntoOwnBacking && curr->compositingState() !=
PaintsIntoGroupedBacking)
> On 2014/06/03 17:05:16, Ian Vollick wrote:
> > Do we want to include HasOwnBackingButPaintsIntoAncestor here? It seems like
> we
> > shouldn't? Here and below.
> 
> Don't know for sure. I don't really understand
> HasOwnBackingButPaintsIntoAncestor.

That compositing state is strange. What it really means is "I've got a backing,
but just so that it can house transforms and clips that will apply to my subtree
-- as for myself, I'm basically not composited". You get this, for example, if
you use -webkit-perspective.

It feels correct to include these layers here (I think we want to include
anything that doesn't paint into some other container), but it's certainly a
change in behavior.

[chrishtr, 2014-06-03 18:01:50]

By "include", do you mean issue repaints for HasOwnBackingButPaintsIntoAncestor?
I agree that it sounds correct to issue repaints for them.
This is the behavior before this CL.

[Ian Vollick, 2014-06-03 18:09:34]

On 2014/06/03 18:01:50, chrishtr wrote:
> By "include", do you mean issue repaints for
HasOwnBackingButPaintsIntoAncestor?
> I agree that it sounds correct to issue repaints for them.
> This is the behavior before this CL.

By "include", I mean repaint them on line 193:
   
curr->repainter().repaintIncludingNonCompositingDescendantsInternal(repaintContainer);

I don't think this was quite the behavior before this cl. If your compositing
state was HasOwnBackingButPaintsIntoAncestor, then hasCompositedLayerMapping
would be true. So to match the old behavior exactly, I think the conditional
would need to look like:
    if (curr->compositingState() != PaintsIntoOwnBacking &&
curr->compositingState() != PaintsIntoGroupedBacking && cur->compositingState !=
HasOwnBackingButPaintsIntoAncestor)
rather than:
    if (curr->compositingState() != PaintsIntoOwnBacking &&
curr->compositingState() != PaintsIntoGroupedBacking)

[chrishtr, 2014-06-03 18:14:32]

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

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:162: if (curr->compositingState()
!= PaintsIntoOwnBacking && curr->compositingState() != PaintsIntoGroupedBacking)
On 2014/06/03 17:14:52, chrishtr wrote:
> On 2014/06/03 17:05:16, Ian Vollick wrote:
> > Do we want to include HasOwnBackingButPaintsIntoAncestor here? It seems like
> we
> > shouldn't? Here and below.
> 
> Don't know for sure. I don't really understand
> HasOwnBackingButPaintsIntoAncestor. 

Ok will leave it. if it paints into the ancestor, repaints should be included
when computing
recursive repaint rects that paint into the same backing, which is the intent of
this method.

[Ian Vollick, 2014-06-03 18:24:00]

Now that I've wrapped my head around the minor behavior change, this lgtm, but
I'm not an expert in the repainting code. Please get a 2nd opinion before
landing.

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

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:162: if (curr->compositingState()
!= PaintsIntoOwnBacking && curr->compositingState() != PaintsIntoGroupedBacking)
On 2014/06/03 18:14:32, chrishtr wrote:
> On 2014/06/03 17:14:52, chrishtr wrote:
> > On 2014/06/03 17:05:16, Ian Vollick wrote:
> > > Do we want to include HasOwnBackingButPaintsIntoAncestor here? It seems
like
> > we
> > > shouldn't? Here and below.
> > 
> > Don't know for sure. I don't really understand
> > HasOwnBackingButPaintsIntoAncestor. 
> 
> Ok will leave it. if it paints into the ancestor, repaints should be included
> when computing
> recursive repaint rects that paint into the same backing, which is the intent
of
> this method.

Yeah, I agree. The behavior change seems good. I've got an idea that will make
this change a bit cleaner and remove the need for the disabler. Can we add a
method, which would return true if the compositing state was neither
PaintsIntoOwnBacking nor PaintsIntoGroupedBacking? If we named it something
invalidation-specific, we could access the stale state. Maybe
RenderLayer::repaintsIntoAncestorContainer? (RenderLayer::paintsIntoAncestor
would be nicer, but doesn't provide a hint that this is intended just for
invalidation).

[ojan, 2014-06-03 18:41:11]

lgtm

I'm a bit concerned about the boundsRectForRepaint thing, but that's not really
new in this patch and doesn't need to block this landed. Just thinking out loud.

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerRepainter.cpp (left):

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:155:
m_renderer.repaintUsingContainer(repaintContainer,
pixelSnappedIntRect(m_renderer.computeRepaintRect()), InvalidationLayer);
Are there cases where we call this without a repaintContainer? Presumably that's
why you change this function call?

boundsRectForRepaint concerns me somewhat as it makes it easy to accidentally
not pass a repaintContainer and get the slow, but correct result.

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

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:159:
DisableCompositingQueryAsserts disabler;
On 2014/06/03 17:14:52, chrishtr wrote:
> On 2014/06/03 17:05:16, Ian Vollick wrote:
> > This seems reasonable to me, but I wonder if it would make sense to add a
> > possiblyStaleCompositingState accessor for use during repainting.
> 
> > We want to
> > invalidate our old rects in terms of the old composited layer tree, right?
> 
> Yes.
> 
> > If that's the case, maybe we should formally allow accessing the stale
state?
> 
> Separating old and new is what we need to do, I agree.

This will be fixed when we stop doing repaints during style recalc, right?
Adding a staleCompositingState has appealed to me in the past, but everytime
I've thought through how it would actually work, it's led to a mess of spaghetti
code. I'm not opposed to the idea, but it's also not clearly good to me unless
we can find a clean way of writing the code. My hope is that we instead
structure all the code so that we don't need to query stale data outside of
InCompositingUpdate, where it's ok-ish as long as we're very careful.

Also, presumably you saw tests fail without this disabler? Please list one
failing test for reference in the comment. That way, if someone tries to remove
this in the future and no tests fail, they can have some confidence that the
disabler is actually no longer needed.

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:2186: }
Nit: you could write this as follows to avoid some minor code duplication:
if (updatedAssignment)
    compositor()->repaintOnCompositingChange(squashedLayer);

[chrishtr, 2014-06-03 20:47:08]

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerRepainter.cpp (left):

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:155:
m_renderer.repaintUsingContainer(repaintContainer,
pixelSnappedIntRect(m_renderer.computeRepaintRect()), InvalidationLayer);
On 2014/06/03 18:41:11, ojan wrote:
> Are there cases where we call this without a repaintContainer? Presumably
that's
> why you change this function call?

The computeRepaintRect() method is the wrong one to call. I'm in the process of
refactoring the
RenderObject code as well to make it harder/impossible to call the wrong method.

> 
> boundsRectForRepaint concerns me somewhat as it makes it easy to accidentally
> not pass a repaintContainer and get the slow, but correct result.

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

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:159:
DisableCompositingQueryAsserts disabler;
On 2014/06/03 18:41:11, ojan wrote:
> On 2014/06/03 17:14:52, chrishtr wrote:
> > On 2014/06/03 17:05:16, Ian Vollick wrote:
> > > This seems reasonable to me, but I wonder if it would make sense to add a
> > > possiblyStaleCompositingState accessor for use during repainting.
> > 
> > > We want to
> > > invalidate our old rects in terms of the old composited layer tree, right?
> > 
> > Yes.
> > 
> > > If that's the case, maybe we should formally allow accessing the stale
> state?
> > 
> > Separating old and new is what we need to do, I agree.
> 
> This will be fixed when we stop doing repaints during style recalc, right?

Yes, if that is actually the case. We would need to do:

1. Style recalc, set dirty bits on repaint for various things
2. Issue repaints for the dirty things with the existing compositing state
3. Update compositing state
4. Issue repaints for the dirty things with the new compositing state

With that kind of phasing, we can have more accurate asserts.

> Adding a staleCompositingState has appealed to me in the past, but everytime
> I've thought through how it would actually work, it's led to a mess of
spaghetti
> code. I'm not opposed to the idea, but it's also not clearly good to me unless
> we can find a clean way of writing the code. My hope is that we instead
> structure all the code so that we don't need to query stale data outside of
> InCompositingUpdate, where it's ok-ish as long as we're very careful.
> 
> Also, presumably you saw tests fail without this disabler? Please list one
> failing test for reference in the comment. That way, if someone tries to
remove
> this in the future and no tests fail, they can have some confidence that the
> disabler is actually no longer needed.

Yes. When loading Calendar. But for some reason does not happen any more. And no
layout tests crash.

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:162: if (curr->compositingState()
!= PaintsIntoOwnBacking && curr->compositingState() != PaintsIntoGroupedBacking)
On 2014/06/03 18:24:00, Ian Vollick wrote:
> On 2014/06/03 18:14:32, chrishtr wrote:
> > On 2014/06/03 17:14:52, chrishtr wrote:
> > > On 2014/06/03 17:05:16, Ian Vollick wrote:
> > > > Do we want to include HasOwnBackingButPaintsIntoAncestor here? It seems
> like
> > > we
> > > > shouldn't? Here and below.
> > > 
> > > Don't know for sure. I don't really understand
> > > HasOwnBackingButPaintsIntoAncestor. 
> > 
> > Ok will leave it. if it paints into the ancestor, repaints should be
included
> > when computing
> > recursive repaint rects that paint into the same backing, which is the
intent
> of
> > this method.
> 
> Yeah, I agree. The behavior change seems good. I've got an idea that will make
> this change a bit cleaner and remove the need for the disabler. Can we add a
> method, which would return true if the compositing state was neither
> PaintsIntoOwnBacking nor PaintsIntoGroupedBacking? If we named it something
> invalidation-specific, we could access the stale state. Maybe
> RenderLayer::repaintsIntoAncestorContainer? (RenderLayer::paintsIntoAncestor
> would be nicer, but doesn't provide a hint that this is intended just for
> invalidation).

Hmm.

The case here I agree does not actually violate any chicken-egg constraints. But
somehow we need
to encapsulate that fact in the interfaces. It's not ok for random code to call
your proposed paintsIntoAncestor() for example... need to think about this some
more.

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/309093002/diff/90001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:2186: }
On 2014/06/03 18:41:11, ojan wrote:
> Nit: you could write this as follows to avoid some minor code duplication:
> if (updatedAssignment)
>     compositor()->repaintOnCompositingChange(squashedLayer);

I'm doing it the current way because I want to have it happen before
m_squashedLayers is changed, because in principle that changes compositing
state.

[chrishtr, 2014-06-03 22:24:31]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-06-03 22:25:04]

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

[commit-bot: I haz the power, 2014-06-04 00:25:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6852)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/12509)

[commit-bot: I haz the power, 2014-06-04 01:09:08]

Change committed as 175417