Explicitly use SkPaint(s) in SVGShapePainter

Source/core/paint/SVGShapePainter.cpp

Index: Source/core/paint/SVGShapePainter.cpp
diff --git a/Source/core/paint/SVGShapePainter.cpp b/Source/core/paint/SVGShapePainter.cpp
index 6b59e3e52975893d6cadec115cf25806bcb25608..90eac1dacffa08eec756f437118faa0a18390a49 100644
--- a/Source/core/paint/SVGShapePainter.cpp
+++ b/Source/core/paint/SVGShapePainter.cpp
@@ -20,6 +20,7 @@
 #include "core/paint/SVGPaintContext.h"
 #include "core/paint/TransformRecorder.h"
 #include "platform/graphics/paint/DisplayItemListContextRecorder.h"
+#include "third_party/skia/include/core/SkPaint.h"
 #include "third_party/skia/include/core/SkPicture.h"
 
 namespace blink {
@@ -34,14 +35,39 @@ static bool setupNonScalingStrokeContext(AffineTransform& strokeTransform, Graph
     return true;
 }
 
-static void useStrokeStyleToFill(GraphicsContext* context)
+static bool paintForLayoutObject(const PaintInfo& paintInfo, const ComputedStyle& style, LayoutObject& layoutObject, LayoutSVGResourceMode resourceMode, SkPaint& paint, const AffineTransform* additionalPaintServerTransform = nullptr)
 {
-    if (Gradient* gradient = context->strokeGradient())
-        context->setFillGradient(gradient);
-    else if (Pattern* pattern = context->strokePattern())
-        context->setFillPattern(pattern);
-    else
-        context->setFillColor(context->strokeColor());
+    if (paintInfo.isRenderingClipPathAsMaskImage()) {
+        if (resourceMode == ApplyToStrokeMode)
+            return false;
+        paint.setColor(SVGComputedStyle::initialFillPaintColor().rgb());
+        paint.setShader(nullptr);
+        return true;
+    }
+
+    SVGPaintServer paintServer = SVGPaintServer::requestForLayoutObject(layoutObject, style, resourceMode);
+    if (!paintServer.isValid())
+        return false;
+
+    if (additionalPaintServerTransform && paintServer.isTransformDependent())
+        paintServer.prependTransform(*additionalPaintServerTransform);
+
+    const SVGComputedStyle& svgStyle = style.svgStyle();
+    float paintAlpha = resourceMode == ApplyToFillMode ? svgStyle.fillOpacity() : svgStyle.strokeOpacity();
+    paintServer.transferToSkPaint(paint, paintAlpha);

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

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.

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

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.

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

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.

+
+    paint.setFilterQuality(WebCoreInterpolationQualityToSkFilterQuality(InterpolationDefault));
+
+    if (resourceMode == ApplyToFillMode)
+        paintInfo.context->setFillRule(svgStyle.fillRule());
+    // TODO(fs): The color filter can set when generating a picture for a mask -
+    // due to color-interpolation. We could also just apply the
+    // color-interpolation property from the the shape itself (which could mean
+    // the paintserver if it has it specified), since that would be more in line
+    // with the spec for color-interpolation. For now, just steal it from the GC
+    // though.
+    paint.setColorFilter(paintInfo.context->colorFilter());

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

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]

Hmm, examples? It would boil down to ordering then I guess...
...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.

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

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

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

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

+    return true;
 }
 
 void SVGShapePainter::paint(const PaintInfo& paintInfo)
@@ -64,21 +90,15 @@ void SVGShapePainter::paint(const PaintInfo& paintInfo)
                 const SVGComputedStyle& svgStyle = m_renderSVGShape.style()->svgStyle();
 
                 bool shouldAntiAlias = svgStyle.shapeRendering() != SR_CRISPEDGES;
-                // We're munging GC paint attributes without saving first (and so does
-                // updateGraphicsContext below). This is in line with making GC less stateful,
-                // but we should keep an eye out for unintended side effects.
-                //
-                // FIXME: the mutation avoidance check should be in (all) the GC setters.
-                if (shouldAntiAlias != paintContext.paintInfo().context->shouldAntialias())
-                    paintContext.paintInfo().context->setShouldAntialias(shouldAntiAlias);
 
                 for (int i = 0; i < 3; i++) {
                     switch (svgStyle.paintOrderType(i)) {
                     case PT_FILL: {
-                        GraphicsContextStateSaver stateSaver(*paintContext.paintInfo().context, false);
-                        if (!SVGLayoutSupport::updateGraphicsContext(paintContext.paintInfo(), stateSaver, m_renderSVGShape.styleRef(), m_renderSVGShape, ApplyToFillMode))
+                        SkPaint fillPaint;
+                        if (!paintForLayoutObject(paintContext.paintInfo(), m_renderSVGShape.styleRef(), m_renderSVGShape, ApplyToFillMode, fillPaint))
                             break;
-                        fillShape(paintContext.paintInfo().context);
+                        fillPaint.setAntiAlias(shouldAntiAlias);
+                        fillShape(paintContext.paintInfo().context, fillPaint);
                         break;
                     }
                     case PT_STROKE:
@@ -96,9 +116,16 @@ void SVGShapePainter::paint(const PaintInfo& paintInfo)
                                 additionalPaintServerTransform = &nonScalingTransform;
                             }
 
-                            if (!SVGLayoutSupport::updateGraphicsContext(paintContext.paintInfo(), stateSaver, m_renderSVGShape.styleRef(), m_renderSVGShape, ApplyToStrokeMode, additionalPaintServerTransform))
+                            SkPaint strokePaint;
+                            if (!paintForLayoutObject(paintContext.paintInfo(), m_renderSVGShape.styleRef(), m_renderSVGShape, ApplyToStrokeMode, strokePaint, additionalPaintServerTransform))
                                 break;
-                            strokeShape(paintContext.paintInfo().context);
+                            strokePaint.setAntiAlias(shouldAntiAlias);
+
+                            StrokeData strokeData;
+                            SVGLayoutSupport::applyStrokeStyleToStrokeData(strokeData, m_renderSVGShape.styleRef(), m_renderSVGShape);
+                            strokeData.setupPaint(&strokePaint);
+
+                            strokeShape(paintContext.paintInfo().context, strokePaint);
                         }
                         break;
                     case PT_MARKERS:
@@ -121,39 +148,39 @@ void SVGShapePainter::paint(const PaintInfo& paintInfo)
     }
 }
 
-void SVGShapePainter::fillShape(GraphicsContext* context)
+void SVGShapePainter::fillShape(GraphicsContext* context, const SkPaint& paint)
 {
     switch (m_renderSVGShape.geometryCodePath()) {
     case RectGeometryFastPath:
-        context->fillRect(m_renderSVGShape.objectBoundingBox());
+        context->fillRect(m_renderSVGShape.objectBoundingBox(), &paint);
         break;
     case EllipseGeometryFastPath:
-        context->fillEllipse(m_renderSVGShape.objectBoundingBox());
+        context->fillEllipse(m_renderSVGShape.objectBoundingBox(), &paint);
         break;
     default:
-        context->fillPath(m_renderSVGShape.path());
+        context->fillPath(m_renderSVGShape.path(), &paint);
     }
 }
 
-void SVGShapePainter::strokeShape(GraphicsContext* context)
+void SVGShapePainter::strokeShape(GraphicsContext* context, const SkPaint& paint)
 {
     if (!m_renderSVGShape.style()->svgStyle().hasVisibleStroke())
         return;
 
     switch (m_renderSVGShape.geometryCodePath()) {
     case RectGeometryFastPath:
-        context->strokeRect(m_renderSVGShape.objectBoundingBox(), m_renderSVGShape.strokeWidth());
+        context->strokeRect(m_renderSVGShape.objectBoundingBox(), paint);
         break;
     case EllipseGeometryFastPath:
-        context->strokeEllipse(m_renderSVGShape.objectBoundingBox());
+        context->strokeEllipse(m_renderSVGShape.objectBoundingBox(), &paint);
         break;
     default:
         ASSERT(m_renderSVGShape.hasPath());
         Path* usePath = &m_renderSVGShape.path();
         if (m_renderSVGShape.hasNonScalingStroke())
             usePath = m_renderSVGShape.nonScalingStrokePath(usePath, m_renderSVGShape.nonScalingStrokeTransform());
-        context->strokePath(*usePath);
-        strokeZeroLengthLineCaps(context);
+        context->strokePath(*usePath, &paint);
+        strokeZeroLengthLineCaps(context, paint);
     }
 }
 
@@ -203,39 +230,35 @@ void SVGShapePainter::paintMarker(const PaintInfo& paintInfo, LayoutSVGResourceM
     }
 }
 
-void SVGShapePainter::strokeZeroLengthLineCaps(GraphicsContext* context)
+void SVGShapePainter::strokeZeroLengthLineCaps(GraphicsContext* context, const SkPaint& strokePaint)
 {
     const Vector<FloatPoint>* zeroLengthLineCaps = m_renderSVGShape.zeroLengthLineCaps();
     if (!zeroLengthLineCaps || zeroLengthLineCaps->isEmpty())
         return;
 
-    Path* usePath;
-    AffineTransform nonScalingTransform;
+    // We need a paint for filling.
+    SkPaint fillPaint = strokePaint;
+    fillPaint.setStyle(SkPaint::kFill_Style);
 
+    AffineTransform nonScalingTransform;
     if (m_renderSVGShape.hasNonScalingStroke())
         nonScalingTransform = m_renderSVGShape.nonScalingStrokeTransform();
 
-    GraphicsContextStateSaver stateSaver(*context, true);
-    useStrokeStyleToFill(context);
+    Path tempPath;
     for (size_t i = 0; i < zeroLengthLineCaps->size(); ++i) {
-        usePath = zeroLengthLinecapPath((*zeroLengthLineCaps)[i]);
+        FloatRect subpathRect = LayoutSVGPath::zeroLengthSubpathRect((*zeroLengthLineCaps)[i], m_renderSVGShape.strokeWidth());
+        tempPath.clear();
+        if (m_renderSVGShape.style()->svgStyle().capStyle() == SquareCap)
+            tempPath.addRect(subpathRect);
+        else
+            tempPath.addEllipse(subpathRect);
+        // This open-codes LayoutSVGShape::nonScalingStrokePath, because the
+        // requirements here differ (we have a temporary path that we can
+        // mutate.)
         if (m_renderSVGShape.hasNonScalingStroke())
-            usePath = m_renderSVGShape.nonScalingStrokePath(usePath, nonScalingTransform);
-        context->fillPath(*usePath);
+            tempPath.transform(nonScalingTransform);
+        context->fillPath(tempPath, &fillPaint);
     }
 }
 
-Path* SVGShapePainter::zeroLengthLinecapPath(const FloatPoint& linecapPosition) const
-{
-    DEFINE_STATIC_LOCAL(Path, tempPath, ());
-
-    tempPath.clear();
-    if (m_renderSVGShape.style()->svgStyle().capStyle() == SquareCap)
-        tempPath.addRect(LayoutSVGPath::zeroLengthSubpathRect(linecapPosition, m_renderSVGShape.strokeWidth()));
-    else
-        tempPath.addEllipse(LayoutSVGPath::zeroLengthSubpathRect(linecapPosition, m_renderSVGShape.strokeWidth()));
-
-    return &tempPath;
-}
-
 } // namespace blink