Factor painting code for filters out of FilterEffectRenderer into FilterPainter.

Factor painting code for filters out of FilterEffectRenderer into FilterPainter.

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

[chrishtr, 2014-11-11 01:01:00]

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

[chrishtr, 2014-11-11 01:01:00]

A very simple refactor, to follow the pattern in the referenced bug.

Next I'm trying to figure out how to make the code in LayerPainter cleaner, this
CL doesn't really achieve that yet.

[Stephen White, 2014-11-11 03:53:24]

LGTM

https://codereview.chromium.org/713823005/diff/1/Source/core/paint/FilterPain...
File Source/core/paint/FilterPainter.h (right):

https://codereview.chromium.org/713823005/diff/1/Source/core/paint/FilterPain...
Source/core/paint/FilterPainter.h:21: RenderLayer& m_renderLayer;
Since all we need out of the RenderLayer is
m_renderLayer.filterRenderer()->lastEffect(), I wonder if we should just pass
that to the FilterPainter instead in beginFilterEffect(). It would reduce
dependencies (and avoid a cyclic one between this class and
FilterEffectRenderer). Just a thought.

[mstensho (USE GERRIT), 2014-11-11 08:15:29]

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

[mstensho (USE GERRIT), 2014-11-11 08:15:29]

https://codereview.chromium.org/713823005/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/713823005/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:352: filterPainter.endFilterEffect(context);
Can't you just have ~FilterPainter() do this job, to achieve a half-RAII
experience? :)

You could store |context| as a member when calling beginFilterEffect().

[chrishtr, 2014-11-11 21:18:55]

On 2014/11/11 at 08:15:29, mstensho wrote:
>
https://codereview.chromium.org/713823005/diff/1/Source/core/paint/LayerPaint...
> File Source/core/paint/LayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/713823005/diff/1/Source/core/paint/LayerPaint...
> Source/core/paint/LayerPainter.cpp:352:
filterPainter.endFilterEffect(context);
> Can't you just have ~FilterPainter() do this job, to achieve a half-RAII
experience? :)
> 
> You could store |context| as a member when calling beginFilterEffect().

Great idea. I achieved full RAII as it turns out. Please take another look.

[Stephen White, 2014-11-11 21:29:17]

I guess I'm an old fuddy-duddy, but I'm not fond of this particular use of RAII.
When the destructor does something non-trivial, the ordering of destruction is
unclear at the callsite (it's just a scope that you have to trace back to find
out what guard was created). I find it much clearer to have explicit
begin()/end() pairs. If you want to ensure correctness, use a guard but assert
that end() has been called in the destructor.

I realize I may be in the minority on this, given how many projects have started
to use this idiom.

[chrishtr, 2014-11-11 21:32:14]

On 2014/11/11 at 21:29:17, senorblanco wrote:
> I guess I'm an old fuddy-duddy, but I'm not fond of this particular use of
RAII. When the destructor does something non-trivial, the ordering of
destruction is unclear at the callsite (it's just a scope that you have to trace
back to find out what guard was created). I find it much clearer to have
explicit begin()/end() pairs. If you want to ensure correctness, use a guard but
assert that end() has been called in the destructor.
> 
> I realize I may be in the minority on this, given how many projects have
started to use this idiom.

You're referring to order of the clip and filter destruction? I agree that part
is the weak point of this CL.

On the other hand, what I really like about this CL is:

1. No wasted clip create/abort for filters that can't be begun
2. People who don't care about filters aren't distracted by some of the code
idioms in LayerPainter relating to it.
3. All of the LayerPainter clip code is now RAII :)

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

Only a couple of nits.

Bonus point for getting rid of that
uh-oh-I-shouldn't-have-set-that-clip-rectangle-after-all thing. :)

https://codereview.chromium.org/713823005/diff/40001/Source/core/paint/Filter...
File Source/core/paint/FilterPainter.cpp (right):

https://codereview.chromium.org/713823005/diff/40001/Source/core/paint/Filter...
Source/core/paint/FilterPainter.cpp:21: 
Extraneous blank line.

https://codereview.chromium.org/713823005/diff/40001/Source/core/paint/Filter...
File Source/core/paint/FilterPainter.h (right):

https://codereview.chromium.org/713823005/diff/40001/Source/core/paint/Filter...
Source/core/paint/FilterPainter.h:21: FilterPainter(RenderLayer&,
GraphicsContext*, const FloatRect& filterBoxRect, const ClipRect&,
LayerPaintingInfo, PaintLayerFlags paintFlags);
Surely you meant const LayerPaintingInfo& ?

Then you could also forward-declare that struct instead of including the header
file. Not that it matters a lot in this case, though.

We could also consider (in a future CL) to pass the exact information we need
(have to do the same for applyRoundedRectClips() as well then), instead of
passing the whole structure. It seems to be limited to a paint rectangle and a
root layer. Not sure how you'd like that, but I, for one, am not a big fan of
LayerPaintingInfo. :)

[Stephen White, 2014-11-11 22:04:05]

On 2014/11/11 21:32:14, chrishtr wrote:
> On 2014/11/11 at 21:29:17, senorblanco wrote:
> > I guess I'm an old fuddy-duddy, but I'm not fond of this particular use of
> RAII. When the destructor does something non-trivial, the ordering of
> destruction is unclear at the callsite (it's just a scope that you have to
trace
> back to find out what guard was created). I find it much clearer to have
> explicit begin()/end() pairs. If you want to ensure correctness, use a guard
but
> assert that end() has been called in the destructor.
> > 
> > I realize I may be in the minority on this, given how many projects have
> started to use this idiom.
> 
> You're referring to order of the clip and filter destruction? I agree that
part
> is the weak point of this CL.
> 
> On the other hand, what I really like about this CL is:
> 
> 1. No wasted clip create/abort for filters that can't be begun
> 2. People who don't care about filters aren't distracted by some of the code
> idioms in LayerPainter relating to it.
> 3. All of the LayerPainter clip code is now RAII :)

No, my fuddy-duddyness is actually worse than that: I'm objecting to the use of
RAII for 
anything other than resource acquisition and initialization. :) I.e., when the
destructor
 does anything non-trivial, it's not obvious at the callsite what's going on
(ordering, behaviour). 
But that idiom is already used here (by the ClipRecorder), so it's not like this
change is introducing something new.

Here's another idea (unrelated to RAII) to remove that early-out from the
constructor and the m_filterInProgress: since the only way a filter can actually
fail to start now is if SkiaImageFilterBuilder returns NULL, you could put the
first 4 lines of the constructor (everything up to the early-return) at the call
site in LayerPainter, and only create the FilterPainter if
SkiaImageFilterBuilder returns a non-NULL filter. Then #1 and #3 are done, and
most of #2 (there's a little more filter-related code in LayerPainter).

WDYT?

[mstensho (USE GERRIT), 2014-11-11 22:05:48]

On 2014/11/11 21:18:55, chrishtr wrote:
> On 2014/11/11 at 08:15:29, mstensho wrote:
> >
>
https://codereview.chromium.org/713823005/diff/1/Source/core/paint/LayerPaint...
> > File Source/core/paint/LayerPainter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/713823005/diff/1/Source/core/paint/LayerPaint...
> > Source/core/paint/LayerPainter.cpp:352:
> filterPainter.endFilterEffect(context);
> > Can't you just have ~FilterPainter() do this job, to achieve a half-RAII
> experience? :)
> > 
> > You could store |context| as a member when calling beginFilterEffect().
> 
> Great idea. I achieved full RAII as it turns out. Please take another look.

Nice!

As you must have realized already, this CL now collides spectacularly with my
https://codereview.chromium.org/705303004 , so you have quite a few conflicts to
resolve. :/

[chrishtr, 2014-11-11 22:14:04]

On 2014/11/11 at 22:04:05, senorblanco wrote:
> On 2014/11/11 21:32:14, chrishtr wrote:
> > On 2014/11/11 at 21:29:17, senorblanco wrote:
> > > I guess I'm an old fuddy-duddy, but I'm not fond of this particular use of
> > RAII. When the destructor does something non-trivial, the ordering of
> > destruction is unclear at the callsite (it's just a scope that you have to
trace
> > back to find out what guard was created). I find it much clearer to have
> > explicit begin()/end() pairs. If you want to ensure correctness, use a guard
but
> > assert that end() has been called in the destructor.
> > > 
> > > I realize I may be in the minority on this, given how many projects have
> > started to use this idiom.
> > 
> > You're referring to order of the clip and filter destruction? I agree that
part
> > is the weak point of this CL.
> > 
> > On the other hand, what I really like about this CL is:
> > 
> > 1. No wasted clip create/abort for filters that can't be begun
> > 2. People who don't care about filters aren't distracted by some of the code
> > idioms in LayerPainter relating to it.
> > 3. All of the LayerPainter clip code is now RAII :)
> 
> No, my fuddy-duddyness is actually worse than that: I'm objecting to the use
of RAII for 
> anything other than resource acquisition and initialization. :) I.e., when the
destructor
>  does anything non-trivial, it's not obvious at the callsite what's going on
(ordering, behaviour). 
> But that idiom is already used here (by the ClipRecorder), so it's not like
this change is introducing something new.
> 
> Here's another idea (unrelated to RAII) to remove that early-out from the
constructor and the m_filterInProgress: since the only way a filter can actually
fail to start now is if SkiaImageFilterBuilder returns NULL, you could put the
first 4 lines of the constructor (everything up to the early-return) at the call
site in LayerPainter, and only create the FilterPainter if
SkiaImageFilterBuilder returns a non-NULL filter. Then #1 and #3 are done, and
most of #2 (there's a little more filter-related code in LayerPainter).
> 
> WDYT?

That would be possible but leak filter internals like you mentioned.

I should point out the full thinking behind the RAII that we started using in
ClipRecorder, maybe that will convince you sufficiently it's ok.
There are two additional reasons to use it that we came up with, other than the
standard "make it impossible to forget to clear the state at block boundaries":

1. It avoids duplicate logic about if (needsClip) in each case. Filters also
benefits from the analogous situation in this CL.
2. The original reason I pursued ClipRecorder is in order to put Slimming Paint
display lists in place, behind a flag. I tried to get the flag in place without
the current pattern, and found it was pretty ugly.
Here the reader of LayerPainter doesn't have to worry about Slimming Paint/not
Slimming Paint.

[Stephen White, 2014-11-11 22:20:55]

https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Filte...
File Source/core/paint/FilterPainter.h (right):

https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Filte...
Source/core/paint/FilterPainter.h:24: void endFilterEffect(GraphicsContext*);
This doesn't seem to be defined anywhere.

https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Layer...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:353: } // Filter clip block
Nit: maybe we should change this to FilterPainter block?

[chrishtr, 2014-11-11 22:21:37]

https://codereview.chromium.org/713823005/diff/1/Source/core/paint/FilterPain...
File Source/core/paint/FilterPainter.h (right):

https://codereview.chromium.org/713823005/diff/1/Source/core/paint/FilterPain...
Source/core/paint/FilterPainter.h:21: RenderLayer& m_renderLayer;
On 2014/11/11 at 03:53:24, Stephen White wrote:
> Since all we need out of the RenderLayer is
m_renderLayer.filterRenderer()->lastEffect(), I wonder if we should just pass
that to the FilterPainter instead in beginFilterEffect(). It would reduce
dependencies (and avoid a cyclic one between this class and
FilterEffectRenderer). Just a thought.

This is the standard pattern for these painting classes, so I'd prefer to stick
with it unless we see a perf regression. I'm quite confident we will see none
for filters, since they are not a hotspot in this way.

https://codereview.chromium.org/713823005/diff/1/Source/core/paint/FilterPain...
Source/core/paint/FilterPainter.h:21: RenderLayer& m_renderLayer;
On 2014/11/11 at 03:53:24, Stephen White wrote:
> Since all we need out of the RenderLayer is
m_renderLayer.filterRenderer()->lastEffect(), I wonder if we should just pass
that to the FilterPainter instead in beginFilterEffect(). It would reduce
dependencies (and avoid a cyclic one between this class and
FilterEffectRenderer). Just a thought.

This is the standard pattern for these painting classes, so I'd prefer to stick
with it unless we see a perf regression. I'm quite confident we will see none
for filters, since they are not a hotspot in this way.

https://codereview.chromium.org/713823005/diff/40001/Source/core/paint/Filter...
File Source/core/paint/FilterPainter.h (right):

https://codereview.chromium.org/713823005/diff/40001/Source/core/paint/Filter...
Source/core/paint/FilterPainter.h:21: FilterPainter(RenderLayer&,
GraphicsContext*, const FloatRect& filterBoxRect, const ClipRect&,
LayerPaintingInfo, PaintLayerFlags paintFlags);
On 2014/11/11 at 22:03:19, mstensho wrote:
> Surely you meant const LayerPaintingInfo& ?
> 
> Then you could also forward-declare that struct instead of including the
header file. Not that it matters a lot in this case, though.

Done.

> 
> We could also consider (in a future CL) to pass the exact information we need
(have to do the same for applyRoundedRectClips() as well then), instead of
passing the whole structure. It seems to be limited to a paint rectangle and a
root layer. Not sure how you'd like that, but I, for one, am not a big fan of
LayerPaintingInfo. :)

[chrishtr, 2014-11-11 22:22:42]

https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Filte...
File Source/core/paint/FilterPainter.h (right):

https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Filte...
Source/core/paint/FilterPainter.h:24: void endFilterEffect(GraphicsContext*);
On 2014/11/11 22:20:55, Stephen White wrote:
> This doesn't seem to be defined anywhere.

Done.

https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Layer...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:353: } // Filter clip block
On 2014/11/11 22:20:55, Stephen White wrote:
> Nit: maybe we should change this to FilterPainter block?

Done.

[Stephen White, 2014-11-11 22:41:51]

On 2014/11/11 22:22:42, chrishtr wrote:
>
https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Filte...
> File Source/core/paint/FilterPainter.h (right):
> 
>
https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Filte...
> Source/core/paint/FilterPainter.h:24: void endFilterEffect(GraphicsContext*);
> On 2014/11/11 22:20:55, Stephen White wrote:
> > This doesn't seem to be defined anywhere.
> 
> Done.
> 
>
https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Layer...
> File Source/core/paint/LayerPainter.cpp (left):
> 
>
https://codereview.chromium.org/713823005/diff/100001/Source/core/paint/Layer...
> Source/core/paint/LayerPainter.cpp:353: } // Filter clip block
> On 2014/11/11 22:20:55, Stephen White wrote:
> > Nit: maybe we should change this to FilterPainter block?
> 
> Done.

LGTM for the upcoming version that compiles :)

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

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-11-12 00:03:14]

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

[commit-bot: I haz the power, 2014-11-12 01:47:03]

Committed patchset #9 (id:160001) as 185163