Bug 23435 – Make SVG spreadMethod a member of Gradient rather than GraphicsContext

• Component: SVG
• Status: RESOLVED FIXED

[Evan Stade]

Both cairo and skia treat spreadMethod as an aspect of the platform gradient (rather than state).

[Evan Stade]

Created attachment 26864 [details]
gradient spreadMethod patch

[Nikolas Zimmermann]

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]

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]

Created attachment 26903 [details]
SVG gradient spreadMethod patch try2

updated according to reviewers' comments

[Evan Stade]

thanks for the comments. Updated.

[Nikolas Zimmermann]

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]

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]

(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]

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]

(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 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]

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]

Created attachment 26917 [details]
try5

fixed typo

[Evan Stade]

Created attachment 26918 [details]
try6

added bug url to changelog

[Nikolas Zimmermann]

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]

can you land this patch? Do you have commit rights?

[Dimitri Glazkov (Google)]

I'll land this.

[Dimitri Glazkov (Google)]

Committed in http://trac.webkit.org/changeset/40292