WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
SVG gradient spreadMethod patch try2
(16.55 KB, patch)
2009-01-21 13:20 PST
,
Evan Stade
no flags
Details
Formatted Diff
Diff
move spreadMethod attribute to Gradient (from GraphicsContext)
(17.36 KB, patch)
2009-01-21 15:59 PST
,
Evan Stade
zimmermann
: review+
Details
Formatted Diff
Diff
try4
(17.59 KB, patch)
2009-01-21 16:45 PST
,
Evan Stade
no flags
Details
Formatted Diff
Diff
try5
(17.62 KB, patch)
2009-01-21 16:47 PST
,
Evan Stade
no flags
Details
Formatted Diff
Diff
try6
(17.77 KB, patch)
2009-01-21 17:04 PST
,
Evan Stade
zimmermann
: review+
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed in
http://trac.webkit.org/changeset/40292
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug