Bug 74370 – Hardware-backed renderLayer could avoid repainting during a positioned movement layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

The current logic always repaints the old and the new position of the positioned object as part of RenderLayer::updateLayerPositions. However if we have a backing, we could be able to just recomposite and avoid the repaint invalidation.

This was seen as part of http://jsbin.com/ulofav where several repaints can be seen on the animated div to be related to the RenderLayer logic after layouting the div.

[Julien Chaffraix]

Created attachment 118929 [details]
Proposed optimization: Pass the did-a-positioned-movement-layout to RenderLayer and avoid repainting in this case if we have a backing.

[Simon Fraser (smfr)]

Comment on attachment 118929 [details]
Proposed optimization: Pass the did-a-positioned-movement-layout to RenderLayer and avoid repainting in this case if we have a backing.

Does this do the right thing for a positioned-movement-only of a non-composited abs pos inside a compositing layer?

[Julien Chaffraix]

(In reply to comment #2)
> (From update of attachment 118929 [details])
> Does this do the right thing for a positioned-movement-only of a non-composited abs pos inside a compositing layer?

I would say so: it would do an invalidation as isComposited() will return false. Do you want the test coverage to be broadened and include that case or do you think this is already covered?

[Julien Chaffraix]

Created attachment 119222 [details]
Added more testing following Simon's cue, a passing test is worth 1000 words.

[Simon Fraser (smfr)]

Comment on attachment 119222 [details]
Added more testing following Simon's cue, a passing test is worth 1000 words.

View in context: https://bugs.webkit.org/attachment.cgi?id=119222&action=review

> Source/WebCore/rendering/RenderLayer.h:79
> +    RepaintAfterLayout = 0,
> +    NeedsFullRepaint = 1,
> +    NeedsFullRepaintForPositionedMovementLayout = 1 << 1,

These values are slightly confusing. Maybe it's not used, but "no repaint needed" would be a sensible 0 value, with the other values all non-zero.

I also find 1 << 0, 1, << 1 a bit more consistent.

> Source/WebCore/rendering/RenderLayer.h:751
> +    RepaintStatus m_repaintStatus : 2;

I think we normally stuff enums into unsigned bitfield fields, for Windows?

[Julien Chaffraix]

Comment on attachment 119222 [details]
Added more testing following Simon's cue, a passing test is worth 1000 words.

View in context: https://bugs.webkit.org/attachment.cgi?id=119222&action=review

>> Source/WebCore/rendering/RenderLayer.h:79
>> +    NeedsFullRepaintForPositionedMovementLayout = 1 << 1,
> 
> These values are slightly confusing. Maybe it's not used, but "no repaint needed" would be a sensible 0 value, with the other values all non-zero.
> 
> I also find 1 << 0, 1, << 1 a bit more consistent.

The NoRepaintNeeded is indeed never uses as we rely always on the RenderLayers for properly repainting after layout. That's why the default value is RepaintAfterLayout.

I am fine with adding this value in case we want to support it at some point (I will put a comment that it is unused though).

>> Source/WebCore/rendering/RenderLayer.h:751
>> +    RepaintStatus m_repaintStatus : 2;
> 
> I think we normally stuff enums into unsigned bitfield fields, for Windows?

You're right, I will switch to unsigned to avoid the issue with padding on Windows.

[Julien Chaffraix]

Comment on attachment 119222 [details]
Added more testing following Simon's cue, a passing test is worth 1000 words.

View in context: https://bugs.webkit.org/attachment.cgi?id=119222&action=review

>>> Source/WebCore/rendering/RenderLayer.h:79
>>> +    NeedsFullRepaintForPositionedMovementLayout = 1 << 1,
>> 
>> These values are slightly confusing. Maybe it's not used, but "no repaint needed" would be a sensible 0 value, with the other values all non-zero.
>> 
>> I also find 1 << 0, 1, << 1 a bit more consistent.
> 
> The NoRepaintNeeded is indeed never uses as we rely always on the RenderLayers for properly repainting after layout. That's why the default value is RepaintAfterLayout.
> 
> I am fine with adding this value in case we want to support it at some point (I will put a comment that it is unused though).

I have to amend this: adding NoRepaintNeeded adds one extra (unneeded) bit to m_repaintStatus. Although I don't think this will grow RenderLayer by another byte, it is still using a bit that could be used for better optimizations. I would rather not add NoRepaintNeeded in this case and will post a patch in this direction.

[Julien Chaffraix]

Created attachment 119407 [details]
Updated change: RepaintAfterLayout -> NeedsNormalRepaint (should make the enum values less confusing) + addressed Simon's comments.

[Simon Fraser (smfr)]

Comment on attachment 119407 [details]
Updated change: RepaintAfterLayout -> NeedsNormalRepaint (should make the enum values less confusing) + addressed Simon's comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=119407&action=review

> Source/WebCore/rendering/RenderLayer.cpp:891
> +    if (m_repaintStatus == NeedsNormalRepaint)

I'm surprised you don't need casts here from unsigned to the enum.

[Julien Chaffraix]

Comment on attachment 119407 [details]
Updated change: RepaintAfterLayout -> NeedsNormalRepaint (should make the enum values less confusing) + addressed Simon's comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=119407&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:891
>> +    if (m_repaintStatus == NeedsNormalRepaint)
> 
> I'm surprised you don't need casts here from unsigned to the enum.

Interestingly, it seems to be compiling on all platforms (I tested the patch on mac before submitting).

[WebKit Review Bot]

Comment on attachment 119407 [details]
Updated change: RepaintAfterLayout -> NeedsNormalRepaint (should make the enum values less confusing) + addressed Simon's comments.

Clearing flags on attachment: 119407

Committed r102952: <http://trac.webkit.org/changeset/102952>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

I think this caused bug 116319.