Store repaint rects in the coordinate space of their backing GraphicsLayer.

Store repaint rects in the coordinate space of their backing
GraphicsLayer.

In particular, do so for squashing layers. This allows us
to track repaints for squashed layers where the layer itself
does not change but the layer moves with respect to the
layer it squashes into.

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

[leviw_travelin_and_unemployed, 2014-05-29 18:27:07]

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

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:733: LayoutStateDisabler
layoutStateDisabler(*transformedAncestor);
:(

This should go away with RAL.

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:355: void computeRectForRepaint(const
RenderLayerModelObject* repaintContainer, LayoutRect&) const;
These 2 functions are really confusing... Can we come up with names that don't
force me to read the comments to know what they do?

It doesn't seem like this function is really computing the rect for repaint.
It's mapping a provided rect to a coordinate space. Is the rect passed in
already clipped?

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1416: return
clippedOverflowRectForRepaint(repaintContainer ? repaintContainer:
containerForRepaint());
I don't really want this ternary to exist, but you're missing a space after the
colon if we keep it.

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1483: LayoutRect repaintRect =
boundsRectForRepaint(repaintContainer);
Why add the local?

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:811: LayoutRect computeRepaintRect(const
RenderLayerModelObject* repaintContainer = 0) const;
How about a computeRepaintRect that takes no parameters that's inline and calls
the one that takes a parameter with containerForRepaint? I only suggest this
because we want to be very clear that you should never be in this code without a
repaint container.

[chrishtr, 2014-05-29 18:34:10]

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

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:733: LayoutStateDisabler
layoutStateDisabler(*transformedAncestor);
On 2014/05/29 18:27:08, leviw wrote:
> :(
> 
> This should go away with RAL.

OK. FTR I just copied the code from its previous location.

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:355: void computeRectForRepaint(const
RenderLayerModelObject* repaintContainer, LayoutRect&) const;
On 2014/05/29 18:27:08, leviw wrote:
> These 2 functions are really confusing... Can we come up with names that don't
> force me to read the comments to know what they do?
> 
> It doesn't seem like this function is really computing the rect for repaint.
> It's mapping a provided rect to a coordinate space. Is the rect passed in
> already clipped?

The function is agnostic to the rect's purpose. It's just a dirty rect. Agree
about naming. Changed.
Seems better?

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1416: return
clippedOverflowRectForRepaint(repaintContainer ? repaintContainer:
containerForRepaint());
On 2014/05/29 18:27:08, leviw wrote:
> I don't really want this ternary to exist, but you're missing a space after
the
> colon if we keep it.

Done.

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/301843002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:811: LayoutRect computeRepaintRect(const
RenderLayerModelObject* repaintContainer = 0) const;
On 2014/05/29 18:27:08, leviw wrote:
> How about a computeRepaintRect that takes no parameters that's inline and
calls
> the one that takes a parameter with containerForRepaint? I only suggest this
> because we want to be very clear that you should never be in this code without
a
> repaint container.

Never be in this code without a repaint container? What do you mean?

[chrishtr, 2014-05-29 18:40:06]

Added inline method.

[chrishtr, 2014-05-29 20:33:32]

Fixed compile, please re-review.

[leviw_travelin_and_unemployed, 2014-05-29 20:49:00]

lgtm with nits and naming

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

https://codereview.chromium.org/301843002/diff/250001/LayoutTests/compositing...
LayoutTests/compositing/squashing/add-remove-squashed-layers-expected.txt:60:
(rect 0.00 0.00 100.00 100.00)
I assume you've validated these are, indeed, correct?

https://codereview.chromium.org/301843002/diff/250001/Source/core/rendering/L...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/301843002/diff/250001/Source/core/rendering/L...
Source/core/rendering/LayoutRepainter.cpp:55: fprintf(stderr,
"LayoutRepainter::repaintAfterLayout\n");
fprintf to ALL the stderrs!!

https://codereview.chromium.org/301843002/diff/250001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/301843002/diff/250001/Source/core/rendering/R...
Source/core/rendering/RenderBox.h:411: virtual void
adjustRectToRepaintBackingCoords(const RenderLayerModelObject* repaintContainer,
LayoutRect&, bool fixed = false) const OVERRIDE;
We use "map" in other parts of blink to signify transforming between coordinate
systems. How about mapRectToRepaintBacking?

[chrishtr, 2014-05-29 21:06:11]

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

https://codereview.chromium.org/301843002/diff/250001/LayoutTests/compositing...
LayoutTests/compositing/squashing/add-remove-squashed-layers-expected.txt:60:
(rect 0.00 0.00 100.00 100.00)
On 2014/05/29 20:49:00, leviw wrote:
> I assume you've validated these are, indeed, correct?

Yes, checked them.

https://codereview.chromium.org/301843002/diff/250001/Source/core/rendering/L...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/301843002/diff/250001/Source/core/rendering/L...
Source/core/rendering/LayoutRepainter.cpp:55: fprintf(stderr,
"LayoutRepainter::repaintAfterLayout\n");
On 2014/05/29 20:49:00, leviw wrote:
> fprintf to ALL the stderrs!!

Done.

https://codereview.chromium.org/301843002/diff/250001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/301843002/diff/250001/Source/core/rendering/R...
Source/core/rendering/RenderBox.h:411: virtual void
adjustRectToRepaintBackingCoords(const RenderLayerModelObject* repaintContainer,
LayoutRect&, bool fixed = false) const OVERRIDE;
On 2014/05/29 20:49:00, leviw wrote:
> We use "map" in other parts of blink to signify transforming between
coordinate
> systems. How about mapRectToRepaintBacking?

Done.

[chrishtr, 2014-05-29 21:06:21]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-05-29 21:07:21]

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

[commit-bot: I haz the power, 2014-05-30 00:59:14]

Change committed as 175090