Don't paint below image's baseline unless it is selected.

Don't paint below image's baseline unless it is selected.

We only need to paint below an inline image's baseline if it is selected,
otherwise we can just paint the image itself.

When an image's selection state changes we need to update the cached
paint rect if it has a layer, i.e. is positioned, so that the correct
area gets painted next time around. This was the 'real' fix for
http://webkit.org/b/22472 - note that the reduction there only ever failed because the image was relatively positioned.

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

[Julien - ping for review, 2014-05-02 00:23:35]

https://codereview.chromium.org/251783004/diff/60001/LayoutTests/fast/images/...
File LayoutTests/fast/images/do-not-paint-below-image-baseline.html (right):

https://codereview.chromium.org/251783004/diff/60001/LayoutTests/fast/images/...
LayoutTests/fast/images/do-not-paint-below-image-baseline.html:26: <img
class="before" src="resources/59.jpg">
You know that I like description and expected results in tests. That extends to
repaint test too!

https://codereview.chromium.org/251783004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderReplaced.cpp (right):

https://codereview.chromium.org/251783004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.cpp:584:
layer()->repainter().computeRepaintRects(containerForRepaint());
I would expect this to fail with repaint-after-layout as we disable
RenderLayerRepainter.

In repaint-after-layout, you need to update the previous repaint rectangle (and
the previous position from the repaint container).

https://codereview.chromium.org/251783004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderReplaced.h (right):

https://codereview.chromium.org/251783004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.h:72: virtual LayoutRect
overflowRectForRepaint() const OVERRIDE FINAL { return isSelected() ?
localSelectionRect() : visualOverflowRect(); }
I wonder if it would make sense for the visualOverflowRect to just include the
selection rect. The downside of that would be that you would need to recompute
it whenever selection changes but I think it's an OK trade off.

Also is this change really required as it seems orthogonal to the fix which
would be the computeRepaintRect when changing the selection state?

[rhogan, 2014-05-02 15:36:27]

https://codereview.chromium.org/251783004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderReplaced.cpp (right):

https://codereview.chromium.org/251783004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.cpp:584:
layer()->repainter().computeRepaintRects(containerForRepaint());
On 2014/05/02 00:23:35, Julien Chaffraix - PST wrote:
> I would expect this to fail with repaint-after-layout as we disable
> RenderLayerRepainter.
> 
> In repaint-after-layout, you need to update the previous repaint rectangle
(and
> the previous position from the repaint container).

If the layer-based repainter goes away then it will just get the correct rect
from clippedOverflowRectForRepaint() in
RenderObject::repaintAfterLayoutIfNeeded().

If I've misunderstood you could you point me in the right direction though?

https://codereview.chromium.org/251783004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderReplaced.h (right):

https://codereview.chromium.org/251783004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.h:72: virtual LayoutRect
overflowRectForRepaint() const OVERRIDE FINAL { return isSelected() ?
localSelectionRect() : visualOverflowRect(); }
On 2014/05/02 00:23:35, Julien Chaffraix - PST wrote:
> I wonder if it would make sense for the visualOverflowRect to just include the
> selection rect. The downside of that would be that you would need to recompute
> it whenever selection changes but I think it's an OK trade off.

That's what we do currently for replaced objects - paint the union of the
selection rect and the overflow rect. That's unnecessary invalidation unless the
image is selected. If it isn't selected we just want to invalidate the
visualOverflowRect() - which is the motivation of this change.

> 
> Also is this change really required as it seems orthogonal to the fix which
> would be the computeRepaintRect when changing the selection state?

The call to computeRepaintRects() when changing selection state is required in
order to keep the cached repaint rect up to date for images with a layer. Images
without a layer get it when they need it from overflowRectForRepaint().

[Julien - ping for review, 2014-05-16 12:41:46]

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

https://codereview.chromium.org/251783004/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.cpp:584:
layer()->repainter().computeRepaintRects(containerForRepaint());
Whenever RenderLayer needs to recompute its repaint rects, repaint-after-layout
requires a similar update: both RenderLayer and repaint-after-layout updates
their rectangles after layout, which isn't scheduled to happen here.

For repaint-after-layout, it's only a matter of calling
setPreviousRepaintRectangle(clippedOverflowRectForRepaint(containerForRepaint())
(gated by RuntimeEnabledFeatures::repaintAfterLayoutEnabled).

Also I am concerned about the call to containerForRepaint(), are we sure that
the compositing information is up-to-date? Else this is introducing a new
chicken-and-egg issue that repaint-after-layout wanted to solve.

[rhogan, 2014-05-16 21:05:47]

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

https://codereview.chromium.org/251783004/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.cpp:584:
layer()->repainter().computeRepaintRects(containerForRepaint());
On 2014/05/16 12:41:46, Julien Chaffraix - CET wrote:
> Whenever RenderLayer needs to recompute its repaint rects,
repaint-after-layout
> requires a similar update: both RenderLayer and repaint-after-layout updates
> their rectangles after layout, which isn't scheduled to happen here.

The RAL call to setPreviousRepaintRectangle happens in computeRepaintRects()
already, so I get it by calling that function, no?

> 
> For repaint-after-layout, it's only a matter of calling
>
setPreviousRepaintRectangle(clippedOverflowRectForRepaint(containerForRepaint())
> (gated by RuntimeEnabledFeatures::repaintAfterLayoutEnabled).
> 
> Also I am concerned about the call to containerForRepaint(), are we sure that
> the compositing information is up-to-date? Else this is introducing a new
> chicken-and-egg issue that repaint-after-layout wanted to solve.

I'm not clear on what could happen to change the repaintContainer between
layouts. It looks like the main potential culprit is a change in compositing
state. Can you give me some pointers?

What is the worst case scenario here? Is it an unnecessary invalidation or a
painting artefact?

[Julien - ping for review, 2014-05-19 09:31:36]

Assuming no extra chicken-and-egg issue, LGTM

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

https://codereview.chromium.org/251783004/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.cpp:584:
layer()->repainter().computeRepaintRects(containerForRepaint());
On 2014/05/16 21:05:47, rhogan wrote:
> On 2014/05/16 12:41:46, Julien Chaffraix - CET wrote:
> > Whenever RenderLayer needs to recompute its repaint rects,
> repaint-after-layout
> > requires a similar update: both RenderLayer and repaint-after-layout updates
> > their rectangles after layout, which isn't scheduled to happen here.
> 
> The RAL call to setPreviousRepaintRectangle happens in computeRepaintRects()
> already, so I get it by calling that function, no?

Good point, you don't need to call it twice indeed.

> > For repaint-after-layout, it's only a matter of calling
> >
>
setPreviousRepaintRectangle(clippedOverflowRectForRepaint(containerForRepaint())
> > (gated by RuntimeEnabledFeatures::repaintAfterLayoutEnabled).
> > 
> > Also I am concerned about the call to containerForRepaint(), are we sure
that
> > the compositing information is up-to-date? Else this is introducing a new
> > chicken-and-egg issue that repaint-after-layout wanted to solve.
> 
> I'm not clear on what could happen to change the repaintContainer between
> layouts. It looks like the main potential culprit is a change in compositing
> state. Can you give me some pointers?
>
> What is the worst case scenario here? Is it an unnecessary invalidation or a
> painting artefact?

Those are great questions. I don't actually know the exact worst case (it's
possible someone looked at it though but I haven't heard about any such
results). My hunch would be an unneeded invalidation (assuming the GPU does an
invalidation to the appropriate GraphicsLayer when it loses some child backing).

My point was more conceptual: we want to remove chicken and egg issues so if we
end up adding a new one, that's bad.

[rhogan, 2014-05-19 17:57:43]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-05-19 17:57:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/robhogan@gmail.com/251783004/90001

[commit-bot: I haz the power, 2014-05-19 18:10:13]

Change committed as 174301