Bug 23435 - Make SVG spreadMethod a member of Gradient rather than GraphicsContext
Summary: Make SVG spreadMethod a member of Gradient rather than GraphicsContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-20 11:09 PST by Evan Stade
Modified: 2009-01-27 09:14 PST (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Stade 2009-01-20 11:09:54 PST
Both cairo and skia treat spreadMethod as an aspect of the platform gradient (rather than state).
Comment 1 Evan Stade 2009-01-20 11:10:44 PST
Created attachment 26864 [details]
gradient spreadMethod patch
Comment 2 Nikolas Zimmermann 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.
Comment 3 David Levin 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;


Comment 4 Evan Stade 2009-01-21 13:20:02 PST
Created attachment 26903 [details]
SVG gradient spreadMethod patch try2

updated according to reviewers' comments
Comment 5 Evan Stade 2009-01-21 13:21:00 PST
thanks for the comments. Updated.
Comment 6 Nikolas Zimmermann 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...
Comment 7 Evan Stade 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.
Comment 8 Nikolas Zimmermann 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.
Comment 9 Evan Stade 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.
Comment 10 Nikolas Zimmermann 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 :-)
Comment 11 Nikolas Zimmermann 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!
Comment 12 Evan Stade 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
Comment 13 Evan Stade 2009-01-21 16:47:29 PST
Created attachment 26917 [details]
try5

fixed typo
Comment 14 Evan Stade 2009-01-21 17:04:37 PST
Created attachment 26918 [details]
try6

added bug url to changelog
Comment 15 Nikolas Zimmermann 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.
Comment 16 Dirk Schulze 2009-01-27 00:06:25 PST
can you land this patch? Do you have commit rights?
Comment 17 Dimitri Glazkov (Google) 2009-01-27 08:47:30 PST
I'll land this.
Comment 18 Dimitri Glazkov (Google) 2009-01-27 09:14:35 PST
Committed in http://trac.webkit.org/changeset/40292