RESOLVED FIXED 23435
Make SVG spreadMethod a member of Gradient rather than GraphicsContext
https://bugs.webkit.org/show_bug.cgi?id=23435
Summary Make SVG spreadMethod a member of Gradient rather than GraphicsContext
Evan Stade
Reported 2009-01-20 11:09:54 PST
Both cairo and skia treat spreadMethod as an aspect of the platform gradient (rather than state).
Attachments
gradient spreadMethod patch (15.77 KB, patch)
2009-01-20 11:10 PST, Evan Stade
zimmermann: review-
SVG gradient spreadMethod patch try2 (16.55 KB, patch)
2009-01-21 13:20 PST, Evan Stade
no flags
move spreadMethod attribute to Gradient (from GraphicsContext) (17.36 KB, patch)
2009-01-21 15:59 PST, Evan Stade
zimmermann: review+
try4 (17.59 KB, patch)
2009-01-21 16:45 PST, Evan Stade
no flags
try5 (17.62 KB, patch)
2009-01-21 16:47 PST, Evan Stade
no flags
try6 (17.77 KB, patch)
2009-01-21 17:04 PST, Evan Stade
zimmermann: review+
Evan Stade
Comment 1 2009-01-20 11:10:44 PST
Created attachment 26864 [details] gradient spreadMethod patch
Nikolas Zimmermann
Comment 2 2009-01-21 08:41:46 PST
Comment on attachment 26864 [details] gradient spreadMethod patch This patch looks great! Though you should pass around the enum, instead of an integer. This could be achieved by moving GradientSpreadMethod from GraphicsContext.h to GraphicsTypes.h, and including GraphicsTypes.h in both GraphicsContext.h and Gradient.h. What do you think? r- because of that.
David Levin
Comment 3 2009-01-21 09:49:58 PST
Just happened to look at this and saw a few things: In WebCore/platform/graphics/cairo/GradientCairo.cpp, applySpreadMethod: * case statements shouldn't be indented (in other places too) * cairo_pattern_set_extend(pattern, CAIRO_EXTEND_PAD); is indented only 3 spaces. * It should "return pattern;" at the end of the function. In WebCore/platform/graphics/skia/GradientSkia.cpp, return m_gradient; got indented incorrectly. switch (spreadMethod) { ... default: The default is going to prevent compiler warning/errors about unhandled enums values (when this is an enum). If you want to ensure that SkShader::TileMode tile; is initialized (which is great), then do SkShader::TileMode tile = SkShader::kClamp_TileMode;
Evan Stade
Comment 4 2009-01-21 13:20:02 PST
Created attachment 26903 [details] SVG gradient spreadMethod patch try2 updated according to reviewers' comments
Evan Stade
Comment 5 2009-01-21 13:21:00 PST
thanks for the comments. Updated.
Nikolas Zimmermann
Comment 6 2009-01-21 13:37:21 PST
Comment on attachment 26903 [details] SVG gradient spreadMethod patch try2 Looks good. Though you need to obey some parameter names, as the mac build recently fails on 'unused variable' warnings. > Index: WebCore/platform/graphics/cg/GradientCG.cpp > =================================================================== > --- WebCore/platform/graphics/cg/GradientCG.cpp (revision 39831) > +++ WebCore/platform/graphics/cg/GradientCG.cpp (working copy) > @@ -50,7 +50,7 @@ static void gradientCallback(void* info, > out[3] = a; > } > > -CGShadingRef Gradient::platformGradient() > +CGShadingRef Gradient::platformGradient(GradientSpreadMethod spreadMethod) Obey the 'spreadMethod' name here. > Index: WebCore/platform/graphics/qt/GradientQt.cpp > =================================================================== > --- WebCore/platform/graphics/qt/GradientQt.cpp (revision 39831) > +++ WebCore/platform/graphics/qt/GradientQt.cpp (working copy) > @@ -40,7 +40,7 @@ void Gradient::platformDestroy() > m_gradient = 0; > } > > -QGradient* Gradient::platformGradient() > +QGradient* Gradient::platformGradient(GradientSpreadMethod spreadMethod) Same here, not sure if Linux builds fail as well, but better safe than sorry. > Index: WebCore/platform/graphics/wx/GradientWx.cpp > =================================================================== > --- WebCore/platform/graphics/wx/GradientWx.cpp (revision 39831) > +++ WebCore/platform/graphics/wx/GradientWx.cpp (working copy) > @@ -36,7 +36,7 @@ void Gradient::platformDestroy() > notImplemented(); > } > > -PlatformGradient Gradient::platformGradient() > +PlatformGradient Gradient::platformGradient(GradientSpreadMethod spreadMethod) Ditto. While I didn't comment on this before, I still like to raise the issue: Why is spread method living in GraphicsContext anyways? It seems to me it better fits in the 'Gradient' class anyway (as the gradient stops live there as well) - this way we wouldn't need to pass down the spreadMethod() in a lot of calls. What do you think? If you have strong reasons against, we might as well r+ the patch as it looks fine, though maybe it's unnecessary...
Evan Stade
Comment 7 2009-01-21 14:08:12 PST
Sorry, I'm not sure what you mean by "obey the parameter name". I thought compilers (even the ones that worry about unused local variables) typically don't complain about unused parameters. You might be right about moving the spread method to the gradient. I'll look into that.
Nikolas Zimmermann
Comment 8 2009-01-21 14:32:23 PST
(In reply to comment #7) > Sorry, I'm not sure what you mean by "obey the parameter name". I thought > compilers (even the ones that worry about unused local variables) typically > don't complain about unused parameters. See http://trac.webkit.org/changeset/39880. -Wno-unused-parameter is now firing in cases where you don't use a named function argument, as we fail the build on warnings, it would be broken. You either have to obey the parameter name, if you don't intend to use it, or use "UNUSED_PARAM(myParameter);" > You might be right about moving the spread method to the gradient. I'll look > into that. Excellent.
Evan Stade
Comment 9 2009-01-21 15:59:22 PST
Created attachment 26911 [details] move spreadMethod attribute to Gradient (from GraphicsContext) Ok, thanks for the explanation. I hadn't heard "obey" used in that context before. I've moved spreadMethod as you suggested. Note that you should not try to change a gradient's spreadMethod after m_gradient is created.
Nikolas Zimmermann
Comment 10 2009-01-21 16:22:15 PST
(In reply to comment #9) > Created an attachment (id=26911) [review] > move spreadMethod attribute to Gradient (from GraphicsContext) > > Ok, thanks for the explanation. I hadn't heard "obey" used in that context > before. Oh jesus, I meant to say "omit", not "obey". Sorry for the confusion, I should drink more coffee :-)
Nikolas Zimmermann
Comment 11 2009-01-21 16:28:33 PST
Comment on attachment 26911 [details] move spreadMethod attribute to Gradient (from GraphicsContext) Patch looks much better, r+, with one comments: > + void setSpreadMethod(GradientSpreadMethod spreadMethod) { > + ASSERT(m_gradient == NULL); > + m_spreadMethod = spreadMethod; > + } We use "0" instead of "NULL" in WebKit. But why does m_gradient have to be null? I'd rather implement the setSpreadMethod in Gradient.cpp, and add a FIXME to handle dynamic spread method changes (when m_gradient has already been created), instead of asserting Thanks for the nice work!
Evan Stade
Comment 12 2009-01-21 16:45:23 PST
Created attachment 26916 [details] try4 > We use "0" instead of "NULL" in WebKit. Ok. > But why does m_gradient have to be null? ... OK, but I left the ASSERT to avoid future head-banging since the current use pattern should never hit this ASSERT and it would only be hit if someone made changes. > Thanks for the nice work! thanks for the review
Evan Stade
Comment 13 2009-01-21 16:47:29 PST
Created attachment 26917 [details] try5 fixed typo
Evan Stade
Comment 14 2009-01-21 17:04:37 PST
Created attachment 26918 [details] try6 added bug url to changelog
Nikolas Zimmermann
Comment 15 2009-01-22 21:12:22 PST
Comment on attachment 26918 [details] try6 Most issues seem resolved, r=me - just one minor style issue: > + void setSpreadMethod(GradientSpreadMethod spreadMethod); > + > + GradientSpreadMethod spreadMethod() { return m_spreadMethod; } You can remove the newline between those methods, and omit the parameter name in setSpreadMethod, so it just reads 'void setSpreadMethod(GradientSpreadMethod);' You don't need to upload another patch, just fix that issue before landing.
Dirk Schulze
Comment 16 2009-01-27 00:06:25 PST
can you land this patch? Do you have commit rights?
Dimitri Glazkov (Google)
Comment 17 2009-01-27 08:47:30 PST
I'll land this.
Dimitri Glazkov (Google)
Comment 18 2009-01-27 09:14:35 PST
Note You need to log in before you can comment on or make changes to this bug.