Get rid of the one remaining extra call to LayerPainter::beginTransparencyLayers.

Get rid of the one remaining extra call to LayerPainter::beginTransparencyLayers.

This allows us to get rid of a related state bit on RenderLayer as well.

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

[chrishtr, 2014-11-14 01:21:24]

chrishtr@chromium.org changed reviewers:
+ mstensho@opera.com

[chrishtr, 2014-11-14 06:17:09]

Looks like exactly one test is failing:
fast/reflections/reflection-masks-opacity.html.

It's because I removed the reflection conditional in the closing of the
transparency layer. The image diff does not look significant though. I'm
strongly tempted to ignore it, since reflections are not a top priority feature.

[mstensho (USE GERRIT), 2014-11-14 12:20:52]

This is great, but could you address my issues (that should reduce the diff
quite a lot), so that I can have another look?

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:83:
LayerPainter(*m_renderLayer.parent()).beginTransparencyLayers(context,
paintingInfo.rootLayer, paintingInfo.paintDirtyRect,
paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);
I think this change is actually what causes the invisible (to the naked eye)
rendering difference in fast/reflections/reflection-masks-opacity.html . We get
here when rendering the reflection (because RenderReplica uses transforms to do
its job). The RenderReplica is a child of what it replicates, i.e. the
RenderImage in this case. So we end up beginning transparency here for the
image.

Before your changes:
Set image transparency (0.5) {
    Set replica transparency (1.0) {
        Paint reflection.
    }
    Paint image.
}

After your changes:
Set replica transparency (1.0) {
    Set image transparency (0.5) {
        Paint reflection.
    }
}
Set image transparency (0.5) {
    Paint image.
}

So the order has changed. It used to be 0.5 followed by 1.0, but now it's 1.0
followed by 0.5. Not that it should matter in theory, but maybe it does in
practice.

Yeah, I think it's fine to just rebaseline for that test.

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:79: // FIXME: unify this one-off path with
the code below.
I'm on it. :)

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:110: bool
shouldUseTransparencyLayer(RenderLayer& renderLayer, PaintBehavior
paintBehavior)
static? Static inline, even?

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:116: return
renderLayer.paintsWithTransparency(paintBehavior) ||
shouldUseTransparencyLayerForBlendMode;
Might as well flip the expressions here. No need to call
renderLayer.paintsWithTransparency(paintBehavior) if you already know that
shouldUseTransparencyLayerForBlendMode is true. Then again, the common return
value from shouldUseTransparencyLayer() is false, so you'll end up having to
evaluate both in most cases anyway.

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:147: if (m_transparencyLayerInProgress) {
Couldn't you just NULL m_context if there's no transparency, so that we won't
need this flag? Ugly?

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:157: void
LayerPainter::beginTransparencyLayers(GraphicsContext* context, const
RenderLayer* rootLayer, const LayoutRect& paintDirtyRect, const LayoutSize&
subPixelAccumulation, PaintBehavior paintBehavior)
Bye-bye to this then. :)

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:300: {
No need for this scope.

[chrishtr, 2014-11-14 20:11:37]

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:83:
LayerPainter(*m_renderLayer.parent()).beginTransparencyLayers(context,
paintingInfo.rootLayer, paintingInfo.paintDirtyRect,
paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);
On 2014/11/14 at 12:20:51, mstensho wrote:
> I think this change is actually what causes the invisible (to the naked eye)
rendering difference in fast/reflections/reflection-masks-opacity.html . We get
here when rendering the reflection (because RenderReplica uses transforms to do
its job). The RenderReplica is a child of what it replicates, i.e. the
RenderImage in this case. So we end up beginning transparency here for the
image.
> 
> Before your changes:
> Set image transparency (0.5) {
>     Set replica transparency (1.0) {
>         Paint reflection.
>     }
>     Paint image.
> }
> 
> After your changes:
> Set replica transparency (1.0) {
>     Set image transparency (0.5) {
>         Paint reflection.
>     }
> }
> Set image transparency (0.5) {
>     Paint image.
> }
> 
> So the order has changed. It used to be 0.5 followed by 1.0, but now it's 1.0
followed by 0.5. Not that it should matter in theory, but maybe it does in
practice.
> 
> Yeah, I think it's fine to just rebaseline for that test.

Done.

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:79: // FIXME: unify this one-off path with
the code below.
On 2014/11/14 at 12:20:52, mstensho wrote:
> I'm on it. :)

ack!

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:110: bool
shouldUseTransparencyLayer(RenderLayer& renderLayer, PaintBehavior
paintBehavior)
On 2014/11/14 at 12:20:51, mstensho wrote:
> static? Static inline, even?

Inlined.

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:116: return
renderLayer.paintsWithTransparency(paintBehavior) ||
shouldUseTransparencyLayerForBlendMode;
On 2014/11/14 at 12:20:51, mstensho wrote:
> Might as well flip the expressions here. No need to call
renderLayer.paintsWithTransparency(paintBehavior) if you already know that
shouldUseTransparencyLayerForBlendMode is true. Then again, the common return
value from shouldUseTransparencyLayer() is false, so you'll end up having to
evaluate both in most cases anyway.

Done.

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:147: if (m_transparencyLayerInProgress) {
On 2014/11/14 at 12:20:51, mstensho wrote:
> Couldn't you just NULL m_context if there's no transparency, so that we won't
need this flag? Ugly?

Feels ugly to me.

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:157: void
LayerPainter::beginTransparencyLayers(GraphicsContext* context, const
RenderLayer* rootLayer, const LayoutRect& paintDirtyRect, const LayoutSize&
subPixelAccumulation, PaintBehavior paintBehavior)
On 2014/11/14 at 12:20:51, mstensho wrote:
> Bye-bye to this then. :)

Yeah, forgot to delete. Sorry for the sloppy work here, I was too eager to send
this late last night. :)

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:300: {
On 2014/11/14 at 12:20:51, mstensho wrote:
> No need for this scope.

The reason I have a scope is to distinguish it from the lifetime of the
ClipPathHelper. Otherwise I'd be relying on whatever the order of destruction is
for stack-allocated variables.

[mstensho (USE GERRIT), 2014-11-14 23:03:22]

Note: the previous patch looks like it contains your changes squashed together
with a master rebase.

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:147: if (m_transparencyLayerInProgress) {
On 2014/11/14 20:11:36, chrishtr wrote:
> On 2014/11/14 at 12:20:51, mstensho wrote:
> > Couldn't you just NULL m_context if there's no transparency, so that we
won't
> need this flag? Ugly?
> 
> Feels ugly to me.

Acknowledged.

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:157: void
LayerPainter::beginTransparencyLayers(GraphicsContext* context, const
RenderLayer* rootLayer, const LayoutRect& paintDirtyRect, const LayoutSize&
subPixelAccumulation, PaintBehavior paintBehavior)
On 2014/11/14 20:11:36, chrishtr wrote:
> On 2014/11/14 at 12:20:51, mstensho wrote:
> > Bye-bye to this then. :)
> 
> Yeah, forgot to delete. Sorry for the sloppy work here, I was too eager to
send
> this late last night. :)

No worries. Didn't keep me from seeing the inner beauty of the patch. :)

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:300: {
On 2014/11/14 20:11:36, chrishtr wrote:
> On 2014/11/14 at 12:20:51, mstensho wrote:
> > No need for this scope.
> 
> The reason I have a scope is to distinguish it from the lifetime of the
> ClipPathHelper. Otherwise I'd be relying on whatever the order of destruction
is
> for stack-allocated variables.

Aren't the objects guaranteed to be destroyed in reverse declaration order?

[chrishtr, 2014-11-14 23:05:46]

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:300: {
On 2014/11/14 23:03:22, mstensho wrote:
> On 2014/11/14 20:11:36, chrishtr wrote:
> > On 2014/11/14 at 12:20:51, mstensho wrote:
> > > No need for this scope.
> > 
> > The reason I have a scope is to distinguish it from the lifetime of the
> > ClipPathHelper. Otherwise I'd be relying on whatever the order of
destruction
> is
> > for stack-allocated variables.
> 
> Aren't the objects guaranteed to be destroyed in reverse declaration order?

Probably, but I couldn't say that for sure without checking the spec. I think
it's worth it to make it obvious.

[mstensho (USE GERRIT), 2014-11-14 23:24:13]

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:300: {
On 2014/11/14 23:05:45, chrishtr wrote:
> On 2014/11/14 23:03:22, mstensho wrote:
> > On 2014/11/14 20:11:36, chrishtr wrote:
> > > On 2014/11/14 at 12:20:51, mstensho wrote:
> > > > No need for this scope.
> > > 
> > > The reason I have a scope is to distinguish it from the lifetime of the
> > > ClipPathHelper. Otherwise I'd be relying on whatever the order of
> destruction
> > is
> > > for stack-allocated variables.
> > 
> > Aren't the objects guaranteed to be destroyed in reverse declaration order?
> 
> Probably, but I couldn't say that for sure without checking the spec. I think
> it's worth it to make it obvious.

https://isocpp.org/wiki/faq/dtors#order-dtors-for-locals (anything else would be
insane anyway)

I find those extra scopes ugly, which is why I'm pushing so hard here. :)

[chrishtr, 2014-11-14 23:29:08]

On 2014/11/14 at 23:24:13, mstensho wrote:
>
https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
> File Source/core/paint/LayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/727633002/diff/60001/Source/core/paint/LayerP...
> Source/core/paint/LayerPainter.cpp:300: {
> On 2014/11/14 23:05:45, chrishtr wrote:
> > On 2014/11/14 23:03:22, mstensho wrote:
> > > On 2014/11/14 20:11:36, chrishtr wrote:
> > > > On 2014/11/14 at 12:20:51, mstensho wrote:
> > > > > No need for this scope.
> > > > 
> > > > The reason I have a scope is to distinguish it from the lifetime of the
> > > > ClipPathHelper. Otherwise I'd be relying on whatever the order of
> > destruction
> > > is
> > > > for stack-allocated variables.
> > > 
> > > Aren't the objects guaranteed to be destroyed in reverse declaration
order?
> > 
> > Probably, but I couldn't say that for sure without checking the spec. I
think
> > it's worth it to make it obvious.
> 
> https://isocpp.org/wiki/faq/dtors#order-dtors-for-locals (anything else would
be insane anyway)
> 
> I find those extra scopes ugly, which is why I'm pushing so hard here. :)

Ok. Done.

[mstensho (USE GERRIT), 2014-11-14 23:40:15]

lgtm!

[chrishtr, 2014-11-14 23:43:37]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-11-14 23:44:45]

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

[commit-bot: I haz the power, 2014-11-15 01:43:50]

Committed patchset #6 (id:100001) as 185402