Remove optimization to lazily allocate transparency layers in LayerPainter.

Remove optimization to lazily allocate transparency layers in LayerPainter.

Before this CL, LayerPainter would try to avoid starting a transparency layer
until it encountered actual drawable content within the RenderLayer that has transparency, or its child.

This optimization makes for code complexity and speed issues, makes it hard to
implement clean display lists, and is most likely not a useful optimization.
It is likely not useful for two reasons:

* It only has effect if the webpage has opacity but no actual content. That does not seem common.
* Skia already has optimizations to remove unnecessary layers if there is no content within them:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/src/core/SkRecordOpts.cpp&q=SkRecordNoopSaveRestores&sq=package:chromium&type=cs&l=21

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

[chrishtr, 2014-11-13 02:00:43]

chrishtr@chromium.org changed reviewers:
+ leviw@chromium.org

[leviw_travelin_and_unemployed, 2014-11-13 02:04:06]

LGTM+++++!
http://www.wallpaperup.com/uploads/wallpapers/2013/05/22/91382/big_thumb_ebf7...

[mstensho (USE GERRIT), 2014-11-13 14:03:32]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2014-11-13 14:03:32]

Note that the tests added to TestExpectations do not fail because of multicol.

With this change, a 0x0 block with opacity<1.0 no longer affects its child
layers, and that's a regression.

[mstensho (USE GERRIT), 2014-11-13 14:28:36]

So beginTransparencyLayers() is called lazily (in paintBackgroundForFragments()
and paintForegroundForFragments()), so that if you get shouldPaintContent =
false in paintLayerContents(), you won't begin transparency for that layer. But
you still have to descend into child layers, and if there's a child layer that
needs to paint, it would (prior to this change) check the ancestry and begin
transparency on those layers where it had not already been begun. (uh... pardon
my English)

So I guess the idea behind the recursion in beginTransparencyLayers() was that
transparency is expensive and if we do it lazily, we can avoid it in a few
cases.

Minimal TC:

<div style="opacity:0.5; position:relative; height:0;">
    <div style="position:relative; background:green;">Background should be pale
green</div>
</div>

[chrishtr, 2014-11-13 17:29:57]

On 2014/11/13 at 14:28:36, mstensho wrote:
> So beginTransparencyLayers() is called lazily (in
paintBackgroundForFragments() and paintForegroundForFragments()), so that if you
get shouldPaintContent = false in paintLayerContents(), you won't begin
transparency for that layer. But you still have to descend into child layers,
and if there's a child layer that needs to paint, it would (prior to this
change) check the ancestry and begin transparency on those layers where it had
not already been begun. (uh... pardon my English)
> 
> So I guess the idea behind the recursion in beginTransparencyLayers() was that
transparency is expensive and if we do it lazily, we can avoid it in a few
cases.
> 
> Minimal TC:
> 
> <div style="opacity:0.5; position:relative; height:0;">
>     <div style="position:relative; background:green;">Background should be
pale green</div>
> </div>

This is amazing, thanks for the minimal test case. I'll look further at a better
solution. Maybe we can get rid of the lazy allocation.

[chrishtr, 2014-11-13 18:40:32]

Updated the CL to fully remove the lazy allocation of transparency layers.
Please take another look.

[mstensho (USE GERRIT), 2014-11-13 19:06:21]

Awesome!

https://codereview.chromium.org/725433002/diff/120001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/725433002/diff/120001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1: 
Please back out this.

https://codereview.chromium.org/725433002/diff/120001/LayoutTests/paint/trans...
File
LayoutTests/paint/transparency/transparency-that-paints-content-only-in-child-expected.html
(right):

https://codereview.chromium.org/725433002/diff/120001/LayoutTests/paint/trans...
LayoutTests/paint/transparency/transparency-that-paints-content-only-in-child-expected.html:2:
<div style="opacity:0.5; position:relative; height:0;">
might as well remove the height:0 declaration. Even position:relative, but
that's less important.

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:81: // FIXME: can we get rid of this? It
would mean transparency starts after clipping in these cases.
Did you try to just remove it? :)

I think this is just part of the lazy transparency thing (it just sets
transparency on the ancestry, not the layer itself), and since you always begin
transparency on the layers that need it now upon entering them, I don't
understand why it still would be necessary.

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:132: // FIXME: refactor to remove this.
+1

If you can get rid of the transparency layer begun in the transforms code (see
my issue above), it should be really easy to get rid of this one. But sure, it
can wait, if you prefer to do it in a separate CL.

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:706: const LayoutRect&
transparencyPaintDirtyRect, bool haveTransparency, const LayerPaintingInfo&
localPaintingInfo, PaintBehavior paintBehavior,
haveTransparency parameter no longer needed.

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:715: const LayoutRect&
transparencyPaintDirtyRect, bool haveTransparency, const LayerPaintingInfo&
localPaintingInfo, PaintBehavior paintBehavior,
haveTransparency parameter no longer needed.

[chrishtr, 2014-11-13 19:40:57]

https://codereview.chromium.org/725433002/diff/120001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/725433002/diff/120001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1: 
On 2014/11/13 19:06:21, mstensho wrote:
> Please back out this.

Done.

https://codereview.chromium.org/725433002/diff/120001/LayoutTests/paint/trans...
File
LayoutTests/paint/transparency/transparency-that-paints-content-only-in-child-expected.html
(right):

https://codereview.chromium.org/725433002/diff/120001/LayoutTests/paint/trans...
LayoutTests/paint/transparency/transparency-that-paints-content-only-in-child-expected.html:2:
<div style="opacity:0.5; position:relative; height:0;">
On 2014/11/13 at 19:06:21, mstensho wrote:
> might as well remove the height:0 declaration. Even position:relative, but
that's less important.

Done.

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:81: // FIXME: can we get rid of this? It
would mean transparency starts after clipping in these cases.
On 2014/11/13 at 19:06:21, mstensho wrote:
> Did you try to just remove it? :)
> 
> I think this is just part of the lazy transparency thing (it just sets
transparency on the ancestry, not the layer itself), and since you always begin
transparency on the layers that need it now upon entering them, I don't
understand why it still would be necessary.

No I didn't. I'll try in a followup, wanted to make this patch a little simpler.

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:132: // FIXME: refactor to remove this.
On 2014/11/13 at 19:06:21, mstensho wrote:
> +1
> 
> If you can get rid of the transparency layer begun in the transforms code (see
my issue above), it should be really easy to get rid of this one. But sure, it
can wait, if you prefer to do it in a separate CL.

Exactly.

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:715: const LayoutRect&
transparencyPaintDirtyRect, bool haveTransparency, const LayerPaintingInfo&
localPaintingInfo, PaintBehavior paintBehavior,
On 2014/11/13 at 19:06:21, mstensho wrote:
> haveTransparency parameter no longer needed.

Done.

[mstensho (USE GERRIT), 2014-11-13 19:49:28]

lgtm, provided that you fix the compilation error. ;)

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/725433002/diff/120001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:81: // FIXME: can we get rid of this? It
would mean transparency starts after clipping in these cases.
On 2014/11/13 19:40:57, chrishtr wrote:
> On 2014/11/13 at 19:06:21, mstensho wrote:
> > Did you try to just remove it? :)
> > 
> > I think this is just part of the lazy transparency thing (it just sets
> transparency on the ancestry, not the layer itself), and since you always
begin
> transparency on the layers that need it now upon entering them, I don't
> understand why it still would be necessary.
> 
> No I didn't. I'll try in a followup, wanted to make this patch a little
simpler.

Acknowledged.

[chrishtr, 2014-11-13 20:56:58]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-11-13 20:58:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/725433002/160001

[commit-bot: I haz the power, 2014-11-13 22:03:25]

Committed patchset #9 (id:160001) as 185324