Move common GC state update code out of paint-server application

Move common GC state update code out of paint-server application

All paint-server RenderSVGResources contain the same code to update the
GC state, transferring stroke properties, fill-rule etc. as required by
the resource mode. They only deviate in which set{Stroke,Fill}* variant
they end up calling.
Move the part that updates non-(direct-)paint-server state to a new
helper SVGRenderSupport::updateGraphicsContext, and then sink that out
of the various applyResource methods.
The special case handling of clipping masks in
RenderSVGResourceSolidColor is transformed into a single block, leaving
the generic path oblivious to this detail.

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

[fs, 2014-10-06 15:08:31]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, pdr@chromium.org, schenney@chromium.org

[pdr., 2014-10-07 02:57:48]

This code is so gross, thank you for cleaning it up.

I think you're taking the right approach here, just two minor nits.

https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.cpp:208:
SVGRenderSupport::updateGraphicsContext(context, style, *this, ApplyToFillMode);
Can we move this call into applyResource so it doesn't need to follow every
applyResource call?

https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGRenderSupport.h (right):

https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGRenderSupport.h:91: static void
updateGraphicsContext(GraphicsContext*, const RenderStyle*, const RenderObject&,
unsigned resourceModeFlags);
This seems like a better fit as a protected member on RenderSVGResource.

[fs, 2014-10-07 12:34:20]

Code shuffled as suggested, but please read responses below as well.

https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.cpp:208:
SVGRenderSupport::updateGraphicsContext(context, style, *this, ApplyToFillMode);
On 2014/10/07 02:57:48, pdr wrote:
> Can we move this call into applyResource so it doesn't need to follow every
> applyResource call?

We can - at least for now. In ~2 CLs applyResource will turn into
preparePaintServer, and at that point updateGraphicsContext needs to be out of
it. It'll probably look nicer together w/ that change though.

https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGRenderSupport.h (right):

https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGRenderSupport.h:91: static void
updateGraphicsContext(GraphicsContext*, const RenderStyle*, const RenderObject&,
unsigned resourceModeFlags);
On 2014/10/07 02:57:48, pdr wrote:
> This seems like a better fit as a protected member on RenderSVGResource.

I can move it there in the interim, but longer term I think a better resting
place will be somewhere in core/paint/. I had a hope that this function would
even disintegrate (in most callsites it's known what resourceMode is after all),
but am not there yet (RenderingClipPathAsMask is one wrench that complicates
things).

[pdr., 2014-10-07 13:12:55]

On 2014/10/07 at 12:34:20, fs wrote:
> Code shuffled as suggested, but please read responses below as well.
> 
>
https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/Re...
> File Source/core/rendering/svg/RenderSVGShape.cpp (right):
> 
>
https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGShape.cpp:208:
SVGRenderSupport::updateGraphicsContext(context, style, *this, ApplyToFillMode);
> On 2014/10/07 02:57:48, pdr wrote:
> > Can we move this call into applyResource so it doesn't need to follow every
> > applyResource call?
> 
> We can - at least for now. In ~2 CLs applyResource will turn into
preparePaintServer, and at that point updateGraphicsContext needs to be out of
it. It'll probably look nicer together w/ that change though.
> 
>
https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/SV...
> File Source/core/rendering/svg/SVGRenderSupport.h (right):
> 
>
https://codereview.chromium.org/630903003/diff/1/Source/core/rendering/svg/SV...
> Source/core/rendering/svg/SVGRenderSupport.h:91: static void
updateGraphicsContext(GraphicsContext*, const RenderStyle*, const RenderObject&,
unsigned resourceModeFlags);
> On 2014/10/07 02:57:48, pdr wrote:
> > This seems like a better fit as a protected member on RenderSVGResource.
> 
> I can move it there in the interim, but longer term I think a better resting
place will be somewhere in core/paint/. I had a hope that this function would
even disintegrate (in most callsites it's known what resourceMode is after all),
but am not there yet (RenderingClipPathAsMask is one wrench that complicates
things).

LGTM

[fs, 2014-10-07 13:46:48]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-10-07 13:48:33]

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

[commit-bot: I haz the power, 2014-10-07 13:53:47]

Committed patchset #2 (id:20001) as 183332