Bug 97986 – SVG radialGradient should support 'fr' for focal radius (just like Canvas)

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

SVG radialGradient should support 'fr' for focal radius (just like Canvas). This is a new feature in SVG2.

[Dirk Schulze]

Created attachment 166495 [details]
Add focus radius for SVG radialGradient

The patch introduces focal radius for SVG radialGradient. It also removes the restriction that a focal point must be in the radius of the gradient. This is according to SVG2. I don't upload it for review now, since we need to figure out how much stuff is affected by this patch.

[WebKit Review Bot]

Attachment 166495 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:74:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Created attachment 167387 [details]
Patch

Hope to get a list of 'to rebaseline' tests on other builds.

[WebKit Review Bot]

Attachment 167387 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/mac/svg/W3C-SVG-1.1-S..." exit_code: 1
Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:74:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 167387 [details]
Patch

Attachment 167387 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14172904

New failing tests:
svg/text/non-bmp-positioning-lists.svg

[Dirk Schulze]

Created attachment 167413 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167413 [details]
Patch

Attachment 167413 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14171951

New failing tests:
svg/W3C-SVG-1.1/styling-inherit-01-b.svg
svg/W3C-SVG-1.1/pservers-grad-13-b.svg
svg/custom/relative-sized-content-with-resources.xhtml
svg/custom/stroked-pattern.svg
svg/custom/gradient-deep-referencing.svg
svg/custom/radial-gradient-with-outstanding-focalPoint.svg
svg/custom/radialGradient-focal-radius.svg
svg/custom/gradient-with-1d-boundingbox.svg
svg/custom/recursive-gradient.svg

[Dirk Schulze]

Created attachment 167426 [details]
Patch

[Dirk Schulze]

Created attachment 167428 [details]
Patch

[Daniel Bates]

Comment on attachment 167428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167428&action=review

> Source/WebCore/ChangeLog:33
> +            New setters ang getters for focal radius.

Nit: ang => and

> Source/WebCore/svg/RadialGradientAttributes.h:34
>          , m_fx()
>          , m_fy()
> +        , m_fr()

It's unnecessary to call the default constructor for these instance variables as the compiler will do this. I take it you feel that it improves the readability to do so?

> LayoutTests/svg/custom/radialGradient-focal-radius.svg:6
> +	<stop stop-color="blue" offset="0%"/>
> +	<stop stop-color="yellow" offset="50%"/>
> +	<stop stop-color="green" offset="100%"/>

Please remove the tab characters on these lines.

> LayoutTests/svg/custom/radialGradient-focal-radius.svg:25
> +</svg>

Nit: Missing a newline at the end of this file.

[Dirk Schulze]

Comment on attachment 167428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167428&action=review

>> Source/WebCore/ChangeLog:33
>> +            New setters ang getters for focal radius.
> 
> Nit: ang => and

Fixed, thanks.

>> Source/WebCore/svg/RadialGradientAttributes.h:34
>> +        , m_fr()
> 
> It's unnecessary to call the default constructor for these instance variables as the compiler will do this. I take it you feel that it improves the readability to do so?

Indeed, I would like to keep the calls. Do you think I should remove them?

>> LayoutTests/svg/custom/radialGradient-focal-radius.svg:6
>> +	<stop stop-color="green" offset="100%"/>
> 
> Please remove the tab characters on these lines.

Fixed. Good catch.

>> LayoutTests/svg/custom/radialGradient-focal-radius.svg:25
>> +</svg>
> 
> Nit: Missing a newline at the end of this file.

Not sure if that matters for test files, but did the change.

[Dirk Schulze]

Created attachment 167467 [details]
Patch

[Daniel Bates]

(In reply to comment #11)
> [...]
> 
> Indeed, I would like to keep the calls. Do you think I should remove them?

I would prefer to remove these calls hence the reason I pointed them out. Having said that, if you feel that it improves the readability of this code then feel free to keep them. I trust your judgement.

[Daniel Bates]

Comment on attachment 167467 [details]
Patch

Thanks Dirk for updating the patch. The patch looks sane to me.

[Dirk Schulze]

Committed r130599: <http://trac.webkit.org/changeset/130599>

[Daniel Bates]

Updated the expected results for the following tests (as they are passing on the respective bots):

EFL:
svg/filters/feDisplacementMap.svg
svg/filters/filterRes.svg

GTK:
svg/custom/gradient-attr-update.svg
svg/custom/gradient-rotated-bbox.svg
svg/custom/gradient-userSpaceOnUse-with-percentage.svg
svg/custom/large-bounding-box-percents.svg
svg/filters/feDisplacementMap.svg
svg/filters/filterRes.svg

Qt:
qt/svg/custom/gradient-attr-update.svg
qt/svg/custom/gradient-rotated-bbox.svg
qt/svg/custom/gradient-userSpaceOnUse-with-percentage.svg
qt/svg/custom/large-bounding-box-percents.svg

And committed this in <http://trac.webkit.org/changeset/130600>.

[Daniel Bates]

(In reply to comment #16)
> [...]
> Qt:
> qt/svg/custom/gradient-attr-update.svg
> qt/svg/custom/gradient-rotated-bbox.svg
> qt/svg/custom/gradient-userSpaceOnUse-with-percentage.svg
> qt/svg/custom/large-bounding-box-percents.svg
> 

This should read:

svg/custom/gradient-attr-update.svg
svg/custom/gradient-rotated-bbox.svg
svg/custom/gradient-userSpaceOnUse-with-percentage.svg
svg/custom/large-bounding-box-percents.svg

[Stephen Chenney]

Created attachment 183179 [details]
User reported failure

Dirk, could you please look at the attached file in Chromium (Canary, and maybe ToT Safari) and see if it is due to this bug. he regression range we have strongly suggests it it.

Is the issue a bug or the new expected behavior?

[Dirk Schulze]

(In reply to comment #18)
> Created an attachment (id=183179) [details]
> User reported failure
> 
> Dirk, could you please look at the attached file in Chromium (Canary, and maybe ToT Safari) and see if it is due to this bug. he regression range we have strongly suggests it it.
> 
> Is the issue a bug or the new expected behavior?

That is the new behavior of SVG radial gradients (following Canvas).

[Stephen Chenney]

Thanks Dirk. I have verified that one can convert the user's example to match the old definitions and it then gives the expected result. No further action is required.