Both cairo and skia treat spreadMethod as an aspect of the platform gradient (rather than state).
Created attachment 26864 [details] gradient spreadMethod patch
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.
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;
Created attachment 26903 [details] SVG gradient spreadMethod patch try2 updated according to reviewers' comments
thanks for the comments. Updated.
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...
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.
(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.
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.
(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 :-)
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!
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
Created attachment 26917 [details] try5 fixed typo
Created attachment 26918 [details] try6 added bug url to changelog
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.
can you land this patch? Do you have commit rights?
I'll land this.
Committed in http://trac.webkit.org/changeset/40292