Bug 126869

Summary: Do refactor in collectGradientAttributes() and renderStyleForLengthResolve()
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: SVGAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, d-r, fmalita, gyuyoung.kim, pdr, rniwa, schenney, zimmermann
Priority: P2 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch krit: review+

Description Gyuyoung Kim 2014-01-12 21:04:59 PST
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.
Comment 1 Gyuyoung Kim 2014-01-12 21:06:15 PST
Created attachment 221002 [details]
Patch
Comment 2 Gyuyoung Kim 2014-01-12 21:07:40 PST
Created attachment 221003 [details]
Patch
Comment 3 Build Bot 2014-01-12 22:45:22 PST
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
Comment 4 Build Bot 2014-01-12 22:45:24 PST
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
Comment 5 Build Bot 2014-01-13 00:03:30 PST
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
Comment 6 Build Bot 2014-01-13 00:03:33 PST
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
Comment 7 Gyuyoung Kim 2014-01-13 08:33:15 PST
Created attachment 221047 [details]
Patch
Comment 8 Gyuyoung Kim 2014-01-13 08:35:11 PST
First patch changed behavior. So, I modify this patch.
Comment 9 Gyuyoung Kim 2014-01-19 08:27:52 PST
Created attachment 221585 [details]
Patch
Comment 10 Sam Weinig 2014-01-19 11:57:48 PST
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&
Comment 11 Gyuyoung Kim 2014-01-19 20:21:18 PST
Created attachment 221607 [details]
Patch
Comment 12 Gyuyoung Kim 2014-01-19 20:21:58 PST
(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.
Comment 13 Dirk Schulze 2014-01-20 06:38:51 PST
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.
Comment 14 Gyuyoung Kim 2014-01-20 16:07:03 PST
Committed r162385: <http://trac.webkit.org/changeset/162385>
Comment 15 Gyuyoung Kim 2014-01-20 16:10:40 PST
(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.