Bug 126869 – Do refactor in collectGradientAttributes() and renderStyleForLengthResolve()

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Gyuyoung Kim]

In SVGRadialGradientElement::collectGradientAttributes(), though *current* element may not SVGRadialGradientElement, it always goes into if (isRadial) { ... } block on a first iteration because *isRadial* is initialized with true. This cl fix it by moving it to a proper place. Now, we don't need to keep *isRadial* flag. Remove it.

[Gyuyoung Kim]

Created attachment 221002 [details]
Patch

[Gyuyoung Kim]

Created attachment 221003 [details]
Patch

[Build Bot]

Comment on attachment 221003 [details]
Patch

Attachment 221003 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5940320948715520

New failing tests:
svg/W3C-SVG-1.1/pservers-grad-13-b.svg
svg/custom/gradient-rotated-bbox.svg
svg/custom/relative-sized-content-with-resources.xhtml
svg/W3C-SVG-1.1/coords-units-01-b.svg
svg/custom/gradient-attr-update.svg
svg/custom/gradient-userSpaceOnUse-with-percentage.svg
svg/W3C-SVG-1.1/pservers-grad-12-b.svg
svg/W3C-SVG-1.1/pservers-grad-02-b.svg
svg/custom/gradient-with-1d-boundingbox.svg
svg/W3C-SVG-1.1/struct-use-05-b.svg
svg/W3C-SVG-1.1/pservers-grad-11-b.svg
svg/custom/radial-gradient-with-outstanding-focalPoint.svg
svg/W3C-SVG-1.1/pservers-grad-05-b.svg
svg/W3C-SVG-1.1/pservers-grad-06-b.svg
svg/W3C-SVG-1.1/pservers-grad-14-b.svg
svg/W3C-SVG-1.1/pservers-grad-04-b.svg
svg/custom/large-bounding-box-percents.svg
svg/custom/radialGradient-focal-radius.svg

[Build Bot]

Created attachment 221009 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221003 [details]
Patch

Attachment 221003 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6423760454287360

New failing tests:
svg/W3C-SVG-1.1/pservers-grad-13-b.svg
svg/custom/gradient-rotated-bbox.svg
svg/custom/large-bounding-box-percents.svg
svg/custom/relative-sized-content-with-resources.xhtml
svg/W3C-SVG-1.1/coords-units-01-b.svg
svg/custom/gradient-attr-update.svg
svg/custom/gradient-userSpaceOnUse-with-percentage.svg
svg/W3C-SVG-1.1/pservers-grad-12-b.svg
svg/W3C-SVG-1.1/pservers-grad-11-b.svg
svg/W3C-SVG-1.1/pservers-grad-02-b.svg
svg/W3C-SVG-1.1/struct-use-05-b.svg
svg/custom/radial-gradient-with-outstanding-focalPoint.svg
svg/W3C-SVG-1.1/pservers-grad-05-b.svg
svg/W3C-SVG-1.1/pservers-grad-06-b.svg
svg/W3C-SVG-1.1/pservers-grad-14-b.svg
svg/W3C-SVG-1.1/pservers-grad-04-b.svg
svg/custom/gradient-with-1d-boundingbox.svg
svg/custom/radialGradient-focal-radius.svg

[Build Bot]

Created attachment 221012 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Gyuyoung Kim]

Created attachment 221047 [details]
Patch

[Gyuyoung Kim]

First patch changed behavior. So, I modify this patch.

[Gyuyoung Kim]

Created attachment 221585 [details]
Patch

[Sam Weinig]

Comment on attachment 221585 [details]
Patch

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

> Source/WebCore/svg/SVGLengthContext.cpp:207
>          return 0;

Change this 0 to a nullptr while you are here.

> Source/WebCore/svg/SVGLinearGradientElement.cpp:126
> +static void setGradientAttributes(SVGGradientElement* element, LinearGradientAttributes& attributes, bool isLinear = true)

If SVGGradientElement* is never null, please convert it to a SVGGradientElement^

> Source/WebCore/svg/SVGRadialGradientElement.cpp:138
> +static void setGradientAttributes(SVGGradientElement* element, RadialGradientAttributes& attributes, bool isRadial = true)

SVGGradientElement&

[Gyuyoung Kim]

Created attachment 221607 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 221585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221585&action=review
> 
> > Source/WebCore/svg/SVGLengthContext.cpp:207
> >          return 0;
> 
> Change this 0 to a nullptr while you are here.
> 
> > Source/WebCore/svg/SVGLinearGradientElement.cpp:126
> > +static void setGradientAttributes(SVGGradientElement* element, LinearGradientAttributes& attributes, bool isLinear = true)
> 
> If SVGGradientElement* is never null, please convert it to a SVGGradientElement^
> 
> > Source/WebCore/svg/SVGRadialGradientElement.cpp:138
> > +static void setGradientAttributes(SVGGradientElement* element, RadialGradientAttributes& attributes, bool isRadial = true)
> 
> SVGGradientElement&

Done. Thanks.

[Dirk Schulze]

Comment on attachment 221607 [details]
Patch

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

I do not see a huge improvement of the code in comparison to the previous code. Adapting to the new style rules is still a fine. r=me

> Source/WebCore/ChangeLog:9
> +        one of poor implementations. This cl fix it by extracting a logic into a new method.

cl may be replaced.

[Gyuyoung Kim]

Committed r162385: <http://trac.webkit.org/changeset/162385>

[Gyuyoung Kim]

(In reply to comment #13)
> (From update of attachment 221607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221607&action=review
> 
> I do not see a huge improvement of the code in comparison to the previous code. Adapting to the new style rules is still a fine. r=me

Yes, this one is just small refactor patch. However, I believe this patch is more make sense from code readability perspective. Thank you for your review.
 
> > Source/WebCore/ChangeLog:9
> > +        one of poor implementations. This cl fix it by extracting a logic into a new method.
> 
> cl may be replaced.

Fixed.