Should not repaint if a CSS property change can be composited

Should not repaint if a CSS property change can be composited

This patch fixes a case that when a RenderObject has StyleDiffLayout,
its composited descendants shouldn't repaint if the style changes can be
composited (and don't change the layout of them).

The optimized CSS properties are clip, position, z-index,
opacity, transform, and filter.

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

[trchen, 2014-03-04 23:46:26]

Hello esprehn@, this patch reduces unnecessary repaints for CSS property changes
that can be composited.

Note: this patch is originally written by enne. I just added test for it. It is
why enne can't be the only reviewer. :)

Note2: We still have unnecessary repaints for non-composited children, which is
more difficult to fix because StyleDiffLayout shadows StyleDiffRepaint, layout
must imply repaint. I was reworking RenderStyle::diff to handle such situation.
That is the WIP you saw the other day.

[esprehn, 2014-03-05 00:09:49]

This needs a better description, you only change the behavior for CSS clip, so
the title and the description should explain that.

I'm pretty sure this optimization can apply for zIndex too though, and for
opacity we can just recomposite descendants instead of repainting. Perhaps for
future patches.

https://codereview.chromium.org/182383019/diff/1/LayoutTests/compositing/repa...
File LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html
(right):

https://codereview.chromium.org/182383019/diff/1/LayoutTests/compositing/repa...
LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html:60:
testRunner.display();
display() is going away, I think you need to use something different.

https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayerModelObject.cpp (right):

https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayerModelObject.cpp:114: if (!(oldStyle->clip() ==
newStyle->clip())
Use != instead

https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayerModelObject.cpp:117: // a parent's clip
changes.
Don't wrap the comment.

https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayerModelObject.cpp:119: } else if
(!layer()->hasCompositedLayerMapping()) {
How do we know this is safe? The other properties could have changed too, like
hasAutoZIndex or position.

It's not clear why when clip changes and lots of other things do too we can take
this special branch. Shouldn't we check the other stuff first?

https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayerModelObject.cpp:126: )
paren should be in the previous line

[trchen, 2014-03-05 00:21:15]

On 2014/03/05 00:09:49, esprehn wrote:
> This needs a better description, you only change the behavior for CSS clip, so
> the title and the description should explain that.
> 
> I'm pretty sure this optimization can apply for zIndex too though, and for
> opacity we can just recomposite descendants instead of repainting. Perhaps for
> future patches.

It was my first thought when I saw this patch. :)
Look at the line "} else if (!layer()->hasCompositedLayerMapping()) {",
it does apply optimization for position, z-index, opacity, transform, and
filter. See explanation below.

I just realize that I didn't test the case for the clip optimization, will add
that in next CL.

>
https://codereview.chromium.org/182383019/diff/1/LayoutTests/compositing/repa...
> File LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html
> (right):
> 
>
https://codereview.chromium.org/182383019/diff/1/LayoutTests/compositing/repa...
> LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html:60:
> testRunner.display();
> display() is going away, I think you need to use something different.

I think we can get away without display(). Let me try that first.

>
https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderLayerModelObject.cpp (right):
> 
>
https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderLayerModelObject.cpp:114: if (!(oldStyle->clip()
==
> newStyle->clip())
> Use != instead

Done

>
https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderLayerModelObject.cpp:117: // a parent's clip
> changes.
> Don't wrap the comment.

Done

>
https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderLayerModelObject.cpp:119: } else if
> (!layer()->hasCompositedLayerMapping()) {
> How do we know this is safe? The other properties could have changed too, like
> hasAutoZIndex or position.
> 
> It's not clear why when clip changes and lots of other things do too we can
take
> this special branch. Shouldn't we check the other stuff first?

It reads: originally we always issue repaint for these style changes. Now we
issue repaint only when the layer is not composited, as these changes can be
composited.

>
https://codereview.chromium.org/182383019/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderLayerModelObject.cpp:126: )
> paren should be in the previous line

Done

[enne (OOO), 2014-03-05 00:24:19]

https://codereview.chromium.org/182383019/diff/1/LayoutTests/compositing/repa...
File LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html
(right):

https://codereview.chromium.org/182383019/diff/1/LayoutTests/compositing/repa...
LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html:60:
testRunner.display();
On 2014/03/05 00:09:49, esprehn wrote:
> display() is going away, I think you need to use something different.

Yeah.  If you import LayoutTests/resources/run-after-display.js, you'll get a
global runAfterDisplay(callback) function which I think will do what you want
here to set up the state prior to toggling.

[trchen, 2014-03-05 04:50:08]

New CL uploaded.

On 2014/03/05 00:24:19, enne wrote:
>
https://codereview.chromium.org/182383019/diff/1/LayoutTests/compositing/repa...
> File LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html
> (right):
> 
>
https://codereview.chromium.org/182383019/diff/1/LayoutTests/compositing/repa...
> LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html:60:
> testRunner.display();
> On 2014/03/05 00:09:49, esprehn wrote:
> > display() is going away, I think you need to use something different.
> 
> Yeah.  If you import LayoutTests/resources/run-after-display.js, you'll get a
> global runAfterDisplay(callback) function which I think will do what you want
> here to set up the state prior to toggling.

That's exactly what I need. Thanks for the info!

[trchen, 2014-03-06 23:04:23]

Hi esprehn@,

This is a friendly reminder to please submit your feedback on this new CL in
Chromium Code Reviews (codereview.chromium.org).

..... Just joking. lol

[esprehn, 2014-03-06 23:26:01]

lgtm, one comment.

https://codereview.chromium.org/182383019/diff/20001/LayoutTests/compositing/...
File LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html
(right):

https://codereview.chromium.org/182383019/diff/20001/LayoutTests/compositing/...
LayoutTests/compositing/repaint/should-not-repaint-composited-styles.html:75:
<td></td><td>composited child</td><td>non-composited child</td>
It would be nice if you didn't use the table and just put an <h1> or <div> for
each header. The table adds a lot of noise to the test.

[trchen, 2014-03-07 00:47:26]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-03-07 00:47:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/182383019/60001

[commit-bot: I haz the power, 2014-03-07 00:55:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 00:55:09]

Try jobs failed on following builders: mac_blink_compile

[trchen, 2014-03-07 01:33:12]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-03-07 01:33:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/182383019/60001

[commit-bot: I haz the power, 2014-03-07 03:31:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 03:31:44]

Try jobs failed on following builders: linux_blink

[trchen, 2014-03-07 05:00:45]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-03-07 05:00:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/182383019/60001

[commit-bot: I haz the power, 2014-03-07 06:26:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 06:26:44]

Try jobs failed on following builders: linux_blink

[trchen, 2014-03-07 23:32:38]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-03-07 23:32:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/182383019/60001

[commit-bot: I haz the power, 2014-03-08 03:45:06]

Change committed as 168760