Bug 20543 - SVG should use the new Gradient support on GraphicsContext
Summary: SVG should use the new Gradient support on GraphicsContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 20373
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-27 05:43 PDT by Eric Seidel (no email)
Modified: 2008-12-03 12:32 PST (History)
2 users (show)

See Also:


Attachments
Update SVG to use new fill/stroke functions for gradients (25.36 KB, patch)
2008-08-27 05:45 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Attempt to clean up SVG gradient code (10.78 KB, patch)
2008-08-27 05:45 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Make more of the CG code cross-platform (11.97 KB, patch)
2008-08-27 05:46 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
SVG Gradient fixes to pass all but one test (9.24 KB, patch)
2008-08-27 05:46 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
SVGPaintServer cleanUp gradient (31.58 KB, patch)
2008-09-29 13:06 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Move gradient spread method to GraphicsContext (14.36 KB, patch)
2008-09-30 13:44 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
Move gradient spread method to GraphicsContext (19.40 KB, patch)
2008-10-02 11:58 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Just for testing (56.51 KB, patch)
2008-10-06 23:13 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
screenie of krit's testing patch on Mac (51.01 KB, image/png)
2008-10-07 02:19 PDT, Eric Seidel (no email)
no flags Details
makes SVGPaintServerGradient platform-independent (57.68 KB, patch)
2008-11-15 10:54 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
makes SVGPaintServerGradient platform-independent (57.75 KB, patch)
2008-11-17 14:37 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
makes SVGPaintServerGradient platform-independent (60.77 KB, patch)
2008-12-01 13:00 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
makes SVGPaintServerGradient platform-independent (59.14 KB, patch)
2008-12-03 08:06 PST, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-08-27 05:43:45 PDT
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.
Comment 1 Eric Seidel (no email) 2008-08-27 05:45:54 PDT
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(-)
Comment 2 Eric Seidel (no email) 2008-08-27 05:45:57 PDT
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(-)
Comment 3 Eric Seidel (no email) 2008-08-27 05:46:00 PDT
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(-)
Comment 4 Eric Seidel (no email) 2008-08-27 05:46:05 PDT
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(-)
Comment 5 Eric Seidel (no email) 2008-08-27 05:48:13 PDT
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.
Comment 6 Eric Seidel (no email) 2008-08-27 05:50:05 PDT
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.
Comment 7 Dirk Schulze 2008-09-29 13:06:53 PDT
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!!!
Comment 8 Dirk Schulze 2008-09-30 13:44:22 PDT
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.
Comment 9 Eric Seidel (no email) 2008-09-30 13:56:32 PDT
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.
Comment 10 Dirk Schulze 2008-10-02 11:58:06 PDT
Created attachment 24025 [details]
Move gradient spread method to GraphicsContext

Renaming SPREADMETHOD_* to SpreadMethod* forced me to change a bit more code :)
Comment 11 Eric Seidel (no email) 2008-10-02 12:29:45 PDT
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.
Comment 12 Dirk Schulze 2008-10-06 23:13:44 PDT
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 .
Comment 13 Eric Seidel (no email) 2008-10-07 02:19:23 PDT
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. :(
Comment 14 Darin Adler 2008-10-12 17:59:33 PDT
If the patch fails the gradient test, then shouldn't it be review- instead of review+?
Comment 15 Dirk Schulze 2008-10-12 22:54:14 PDT
(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.
Comment 16 Darin Adler 2008-10-15 09:44:34 PDT
I'm working on landing the reviewed patch.
Comment 17 Darin Adler 2008-10-15 10:02:36 PDT
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.
Comment 18 Darin Adler 2008-10-15 10:03:04 PDT
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
Comment 19 Dirk Schulze 2008-11-15 10:54:12 PST
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.
Comment 20 Eric Seidel (no email) 2008-11-17 13:35:55 PST
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.
Comment 21 Dirk Schulze 2008-11-17 14:37:34 PST
Created attachment 25229 [details]
makes SVGPaintServerGradient platform-independent

readded:

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

and the needed calls.
Comment 22 Eric Seidel (no email) 2008-12-01 11:27:04 PST
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.
Comment 23 Dirk Schulze 2008-12-01 13:00:18 PST
Created attachment 25635 [details]
makes SVGPaintServerGradient platform-independent

this should fix the Mac build and make the Layout tests work for gradients again.
Comment 24 Dirk Schulze 2008-12-03 08:06:48 PST
Created attachment 25711 [details]
makes SVGPaintServerGradient platform-independent

Fixes all layouttests. 2 pixel tests have to be updated.
Comment 25 Nikolas Zimmermann 2008-12-03 08:28:55 PST
Comment on attachment 25711 [details]
makes SVGPaintServerGradient platform-independent

Looks great, no more comments from my side after the xxxth review :-)
Comment 26 Dirk Schulze 2008-12-03 12:32:09 PST
landed in r38952.