Explicitly use SkPaint(s) in SVGShapePainter

Explicitly use SkPaint(s) in SVGShapePainter

This replaces use of SVGLayoutSupport::updateGraphicsContext to update
the GC-state with code that fills in and uses SkPaints explicitly, and
hence avoids saving GC-state (except for CTM updates).
As a side-effect, the code to handle stroke of zero-length is simplified
significantly.

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

[fs, 2015-04-02 14:45:29]

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

[fs, 2015-04-02 15:01:47]

Easter special!

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.h:199: void fillPath(const Path&, const
SkPaint* = nullptr);
This is a bit ugly, and only really to avoid adding additional entrypoints.

void fillPath(const Path&, const SkPaint&);

would probably have been nicer. Let me know what you prefer.

[f(malita), 2015-04-02 15:23:01]

w00t!

Initial GC API comments.

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.h:199: void fillPath(const Path&, const
SkPaint* = nullptr);
On 2015/04/02 15:01:47, fs wrote:
> This is a bit ugly, and only really to avoid adding additional entrypoints.
> 
> void fillPath(const Path&, const SkPaint&);
> 
> would probably have been nicer. Let me know what you prefer.

Since the stroke/fill trait is specified on the SkPaint itself, this approach
seems a bit confusing (you can now call fillPath with a paint that draws a
stroke instead).

I think what we want is a drawPath(const Path&, const SkPaint&).

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.h:240: void drawPath(const SkPath&,
const SkPaint&);
... except we already have an SkPath version! :)

Could we just use this one instead?

  drawPath(myPath.skPath(), myPaint);

[f(malita), 2015-04-02 15:31:58]

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.h:214: void strokeRect(const
FloatRect&, const SkPaint&);
Same comment for

fill/strokeRect -> drawRect
fill/strokeEllipse -> drawOval

[f(malita), 2015-04-02 15:51:12]

moar piecemeal review

https://codereview.chromium.org/1052873003/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp (right):

https://codereview.chromium.org/1052873003/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp:52: void
SVGPaintServer::transferToSkPaint(SkPaint& paint, float paintAlpha)
Naming nit: applyToSkPaint maybe? ("transfer" sounds like it may invalidate the
current object).

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
File Source/core/paint/SVGShapePainter.cpp (right):

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
Source/core/paint/SVGShapePainter.cpp:57: paintServer.transferToSkPaint(paint,
paintAlpha);
Would it make sense to compute the paintAlpha in
SVGPaintServer::requestForLayoutObject and store it in the paint server itself?
Then it could be applied transparently and we could drop the paintAlpha arg
here.

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
Source/core/paint/SVGShapePainter.cpp:69:
paint.setColorFilter(paintInfo.context->colorFilter());
This is one of the problematic GC states: we can sometimes avoid a layer by
munging all subtree paints' color filter.

But it's not really safe/guaranteed to be correct I think, as something down the
paint pipe may want to farther tweak the color filter => the results will not
reflect a correct composition.

I believe we should bite the bullet and always apply color interpolation filters
locally using saveLayers,  instead of passing state down the line.

One of these days I'll get to cleaning it up, but maybe you can mention it in
your comment here.

[fs, 2015-04-03 18:19:44]

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
File Source/core/paint/SVGShapePainter.cpp (right):

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
Source/core/paint/SVGShapePainter.cpp:57: paintServer.transferToSkPaint(paint,
paintAlpha);
On 2015/04/02 15:51:12, f(malita) wrote:
> Would it make sense to compute the paintAlpha in
> SVGPaintServer::requestForLayoutObject and store it in the paint server
itself?
> Then it could be applied transparently and we could drop the paintAlpha arg
> here.

I believe I had it there a while when working on the SVGPaintServer
wrapper/helper. Don't recall the reason for moving it/not including it in the
end, but I can certainly revisit it.

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
Source/core/paint/SVGShapePainter.cpp:69:
paint.setColorFilter(paintInfo.context->colorFilter());
On 2015/04/02 15:51:12, f(malita) wrote:
> This is one of the problematic GC states: we can sometimes avoid a layer by
> munging all subtree paints' color filter.
> 
> But it's not really safe/guaranteed to be correct I think, as something down
the
> paint pipe may want to farther tweak the color filter => the results will not
> reflect a correct composition.

Hmm, examples? It would boil down to ordering then I guess...

> I believe we should bite the bullet and always apply color interpolation
filters
> locally using saveLayers,  instead of passing state down the line.

...and this would guarantee correct ordering. Probably not too bad considering
this a (very) rare property AFAIK. Presumably "layer hoisting" would "fix" it in
most cases too.

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.h:240: void drawPath(const SkPath&,
const SkPaint&);
On 2015/04/02 15:23:01, f(malita) wrote:
> ... except we already have an SkPath version! :)
> 
> Could we just use this one instead?
> 
>   drawPath(myPath.skPath(), myPaint);

So many methods...

[fs, 2015-04-07 12:03:56]

https://codereview.chromium.org/1052873003/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp (right):

https://codereview.chromium.org/1052873003/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp:52: void
SVGPaintServer::transferToSkPaint(SkPaint& paint, float paintAlpha)
On 2015/04/02 15:51:12, f(malita) wrote:
> Naming nit: applyToSkPaint maybe? ("transfer" sounds like it may invalidate
the
> current object).

Done.

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
File Source/core/paint/SVGShapePainter.cpp (right):

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
Source/core/paint/SVGShapePainter.cpp:57: paintServer.transferToSkPaint(paint,
paintAlpha);
On 2015/04/03 18:19:44, fs wrote:
> On 2015/04/02 15:51:12, f(malita) wrote:
> > Would it make sense to compute the paintAlpha in
> > SVGPaintServer::requestForLayoutObject and store it in the paint server
> itself?
> > Then it could be applied transparently and we could drop the paintAlpha arg
> > here.
> 
> I believe I had it there a while when working on the SVGPaintServer
> wrapper/helper. Don't recall the reason for moving it/not including it in the
> end, but I can certainly revisit it.

I revisited this, and while it probably would be a good idea right now it just
ends up hanging on the fence. In part because of the complicated structure of
requestPaint and co. Within an SkPaint in place though it could make sense to
use that as a logistical type too and then perhaps peel some of that complexity.

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
Source/core/paint/SVGShapePainter.cpp:69:
paint.setColorFilter(paintInfo.context->colorFilter());
On 2015/04/02 15:51:12, f(malita) wrote:
> This is one of the problematic GC states: we can sometimes avoid a layer by
> munging all subtree paints' color filter.
> 
> But it's not really safe/guaranteed to be correct I think, as something down
the
> paint pipe may want to farther tweak the color filter => the results will not
> reflect a correct composition.
> 
> I believe we should bite the bullet and always apply color interpolation
filters
> locally using saveLayers,  instead of passing state down the line.
> 
> One of these days I'll get to cleaning it up, but maybe you can mention it in
> your comment here.

I've extended the comment, borrowing some of the above.

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.h:199: void fillPath(const Path&, const
SkPaint* = nullptr);
On 2015/04/02 15:23:00, f(malita) wrote:
> On 2015/04/02 15:01:47, fs wrote:
> > This is a bit ugly, and only really to avoid adding additional entrypoints.
> > 
> > void fillPath(const Path&, const SkPaint&);
> > 
> > would probably have been nicer. Let me know what you prefer.
> 
> Since the stroke/fill trait is specified on the SkPaint itself, this approach
> seems a bit confusing (you can now call fillPath with a paint that draws a
> stroke instead).
> 
> I think what we want is a drawPath(const Path&, const SkPaint&).

Using the suggestion below.

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.h:240: void drawPath(const SkPath&,
const SkPaint&);
On 2015/04/02 15:23:01, f(malita) wrote:
> ... except we already have an SkPath version! :)
> 
> Could we just use this one instead?
> 
>   drawPath(myPath.skPath(), myPaint);

Using these three.

[f(malita), 2015-04-07 12:44:30]

LGTM

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
File Source/core/paint/SVGShapePainter.cpp (right):

https://codereview.chromium.org/1052873003/diff/1/Source/core/paint/SVGShapeP...
Source/core/paint/SVGShapePainter.cpp:69:
paint.setColorFilter(paintInfo.context->colorFilter());
On 2015/04/03 18:19:44, fs wrote:
> On 2015/04/02 15:51:12, f(malita) wrote:
> > This is one of the problematic GC states: we can sometimes avoid a layer by
> > munging all subtree paints' color filter.
> > 
> > But it's not really safe/guaranteed to be correct I think, as something down
> the
> > paint pipe may want to farther tweak the color filter => the results will
not
> > reflect a correct composition.
> 
> Hmm, examples? It would boil down to ordering then I guess...

I was thinking mainly of nesting (masks vs. filters vs. shadows) which is not
currently supported, but yeah, it just seems fragile in general.

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1052873003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.h:240: void drawPath(const SkPath&,
const SkPaint&);
On 2015/04/03 18:19:44, fs wrote:
> On 2015/04/02 15:23:01, f(malita) wrote:
> > ... except we already have an SkPath version! :)
> > 
> > Could we just use this one instead?
> > 
> >   drawPath(myPath.skPath(), myPaint);
> 
> So many methods...

Hopefully we get rid of all the fill/stroke variants at some point.

[fs, 2015-04-07 14:54:56]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-04-07 14:55:07]

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

[commit-bot: I haz the power, 2015-04-07 14:59:04]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193272