Bug 20543 – SVG should use the new Gradient support on GraphicsContext

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

SVG should use the new Gradient support on GraphicsContext

I have an incomplete patch (well, set of patches) to add support for using Gradient instead of having SVGPaintServerGradient do lots of platform-specific junk.  It still needs to be cleaned up (and one regressed test fixed), but I'm posting it here for now.  I'm not sure I'll get a chance to work on this more in the near future, we'll see.

[Eric Seidel (no email)]

Created attachment 23025 [details]
Update SVG to use new fill/stroke functions for gradients

 .../JavaScriptCore.xcodeproj/project.pbxproj       |    3 -
 WebCore/rendering/SVGRenderTreeAsText.cpp          |    1 -
 WebCore/svg/GradientAttributes.h                   |   14 ++-
 WebCore/svg/LinearGradientAttributes.h             |   15 ++-
 WebCore/svg/RadialGradientAttributes.h             |   15 ++-
 WebCore/svg/SVGGradientElement.cpp                 |    2 +-
 WebCore/svg/SVGLinearGradientElement.cpp           |    9 +-
 WebCore/svg/SVGLinearGradientElement.h             |    2 -
 WebCore/svg/SVGRadialGradientElement.cpp           |    5 +-
 WebCore/svg/graphics/SVGPaintServer.h              |    9 +-
 WebCore/svg/graphics/SVGPaintServerGradient.cpp    |   17 +--
 WebCore/svg/graphics/SVGPaintServerGradient.h      |   39 +----
 WebCore/svg/graphics/cg/SVGPaintServerCg.cpp       |   32 +----
 .../svg/graphics/cg/SVGPaintServerGradientCg.cpp   |  177 ++------------------
 14 files changed, 75 insertions(+), 265 deletions(-)

[Eric Seidel (no email)]

Created attachment 23026 [details]
Attempt to clean up SVG gradient code

 WebCore/platform/graphics/FloatSize.h              |    6 +
 WebCore/svg/graphics/SVGPaintServerGradient.h      |    3 -
 .../svg/graphics/cg/SVGPaintServerGradientCg.cpp   |  164 ++++++++++----------
 3 files changed, 90 insertions(+), 83 deletions(-)

[Eric Seidel (no email)]

Created attachment 23027 [details]
Make more of the CG code cross-platform

 WebCore/svg/graphics/cg/CgSupport.cpp              |   93 +++++++++++---------
 WebCore/svg/graphics/cg/CgSupport.h                |   16 ++--
 WebCore/svg/graphics/cg/RenderPathCg.cpp           |   23 ++---
 .../svg/graphics/cg/SVGPaintServerGradientCg.cpp   |    5 +-
 WebCore/svg/graphics/cg/SVGResourceClipperCg.cpp   |   49 +++++------
 5 files changed, 90 insertions(+), 96 deletions(-)

[Eric Seidel (no email)]

Created attachment 23028 [details]
SVG Gradient fixes to pass all but one test

 WebCore/platform/graphics/FloatSize.h              |    1 +
 WebCore/svg/GradientAttributes.h                   |   13 +++++++---
 WebCore/svg/LinearGradientAttributes.h             |    1 +
 WebCore/svg/RadialGradientAttributes.h             |   24 +++++++++++++++----
 WebCore/svg/SVGLinearGradientElement.cpp           |    3 +-
 WebCore/svg/SVGRadialGradientElement.cpp           |    4 +-
 WebCore/svg/graphics/cg/CgSupport.cpp              |    1 +
 WebCore/svg/graphics/cg/CgSupport.h                |    1 +
 WebCore/svg/graphics/cg/SVGPaintServerCg.cpp       |    3 +-
 .../svg/graphics/cg/SVGPaintServerGradientCg.cpp   |   23 +++++++++++-------
 10 files changed, 50 insertions(+), 24 deletions(-)

[Eric Seidel (no email)]

Two of the above patches could probably be reviewed and landed separately.  It wasn't super-simple to break them apart, and so eventually I gave up and finished preparing the patch for bug 20373.  My primary motivator here was to make it simple to merge in Skia support, as GraphicsContextSkia has a similar pattern/gradient refactor to what I did as part of bug 20373.  This SVG cleanup is a very secondary goal, and so I've stopped working on it for now.

[Eric Seidel (no email)]

Oh, and btw, there are a couple hacks in this patch which would need to be cleaned up for this to be done for real:

1.  Serialization of gradient stops was disabled.  That will need to be fixed or all SVG test cases which use gradients will fail.
2.  GradientAtrributes*.h files really shouldn't have such large function implementations in them.  I think that the createGradient and addStopsToGradient logic should be pushed down into either corresponding .cpp files or the SVGGradientElement*.cpp files.

[Dirk Schulze]

Created attachment 23915 [details]
SVGPaintServer cleanUp gradient

I updated the patches above to match current trunk and to go on. I made only small changes on the code itself, but moved out changes on CgSupport and RenderStyle. This should be part of another patch. It breaks builds for Qt and Cairo currently.

It needs the same fixes like discribed above!!!

[Dirk Schulze]

Created attachment 23953 [details]
Move gradient spread method to GraphicsContext

Move gradient spread method from SVG to GraphicsContext. The ports will use this code later, when gradients are platform independent.

[Eric Seidel (no email)]

Comment on attachment 23953 [details]
Move gradient spread method to GraphicsContext

I have mixed feelings about this change.  CG has no concept of "Spread Method", so if/when we ever decide to implement that obscure part of SVG (which I've yet to see a single SVG depend on), we'll need to write a "spread method" implementation on top of GraphicsContext functions or on top of CG functions.

So if we have to write one generic implementation (for the currently most-popular port of WebKit) then it makes me wonder if we'll want to use that implementation for all other ports.

enum GradientSpreadMethod {
 129         SPREADMETHOD_PAD = 1,
 130         SPREADMETHOD_REFLECT = 2,
 131         SPREADMETHOD_REPEAT = 3
 132     };

This should follow the CamelCase naming convention of all other enums in that file.

SpreadMethodPad
SpreadMethodRepeat
SpreadMethodReflect

 74         GradientSpreadMethod spreadMethod;

That needs to be initialized to something.

 459         if (spreadMethod())
That should never be false!  Since spreadMethod() == 0 is not a valid value.

No default is needed here:
167         default:
 168             gradient.setSpread(QGradient::PadSpread);
 169             break;

I think this change is overall OK.  I think we'll end up crossing the "how to write a CG implementation" bridge when we come to it.  And when we do write a CG implementation of this we'll think about generalizing said implementation.

[Dirk Schulze]

Created attachment 24025 [details]
Move gradient spread method to GraphicsContext

Renaming SPREADMETHOD_* to SpreadMethod* forced me to change a bit more code :)

[Eric Seidel (no email)]

Comment on attachment 24025 [details]
Move gradient spread method to GraphicsContext

 108             attributes.setSpreadMethod((GradientSpreadMethod) current->spreadMethod());

Should be changed to use static_cast.

Otherwise looks fine.

[Dirk Schulze]

Created attachment 24140 [details]
Just for testing

Just for testing http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-pservers-grad-17-b.html .

[Eric Seidel (no email)]

Created attachment 24142 [details]
screenie of krit's testing patch on Mac

I had to fix the extra variable "isFilled" to make the mac build compile with this patch.  The patch still fails this gradient test. :(

[Darin Adler]

If the patch fails the gradient test, then shouldn't it be review- instead of review+?

[Dirk Schulze]

(In reply to comment #14)
The r+ patch is adding spreadMethod to GraphicsContext. Cg doesn't support spreadMethod. So it doesn't fail any test.
But it isn't used by Cairo or Qt as well. Bbut it will be used, when we get the whole clean up patch to work.

[Darin Adler]

I'm working on landing the reviewed patch.

[Darin Adler]

Comment on attachment 24025 [details]
Move gradient spread method to GraphicsContext

I think it was a mistake to remove the spread method constants from the SVG DOM. Don't we want people to be able to get at those from JavaScript?

I also think it's wrong to just cast from the SVG DOM type to the GraphicsContext type; the types need to be independent.

[Darin Adler]

Comment on attachment 24025 [details]
Move gradient spread method to GraphicsContext

Cleared review flag since I landed this as http://trac.webkit.org/changeset/37605

[Dirk Schulze]

Created attachment 25187 [details]
makes SVGPaintServerGradient platform-independent

This patch makes SVG gradients platform independent (some Cg-code left for text drawing).
It fixes a regressen of one LayoutTest on Cg: pservers-grad-17-b-expected,
will make linear gradients work for Qt (after a path-transformation-patch, independent from this patch),
makes it possible to implement stroke and fill gradients for texts on Qt and Cairo.

[Eric Seidel (no email)]

Comment on attachment 25187 [details]
makes SVGPaintServerGradient platform-independent

Does this pass the layout tests?

I would think removing this line:
130      ts  << "[stops=" << gradientStops() << "]";

would cause them to fail.  I think we want to find a way to print out what gradient stops a gradient has when dumping text results for SVG files.

[Dirk Schulze]

Created attachment 25229 [details]
makes SVGPaintServerGradient platform-independent

readded:

130      ts  << "[stops=" << gradientStops() << "]";

and the needed calls.

[Eric Seidel (no email)]

Comment on attachment 25229 [details]
makes SVGPaintServerGradient platform-independent

Testing this on CG now, since I don't think you have a mac available to you.

[Dirk Schulze]

Created attachment 25635 [details]
makes SVGPaintServerGradient platform-independent

this should fix the Mac build and make the Layout tests work for gradients again.

[Dirk Schulze]

Created attachment 25711 [details]
makes SVGPaintServerGradient platform-independent

Fixes all layouttests. 2 pixel tests have to be updated.

[Nikolas Zimmermann]

Comment on attachment 25711 [details]
makes SVGPaintServerGradient platform-independent

Looks great, no more comments from my side after the xxxth review :-)

[Dirk Schulze]

landed in r38952.