Bug 128658

Summary: Refactor SVGPreserveAspectRatio::parse()
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, krit, pdr, rniwa, schenney, sergio, 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-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch none

Gyuyoung Kim
Reported 2014-02-11 23:59:53 PST
This cl removes "goto" and replaces it with local variables, and fixes poor coding style.
Attachments
Patch (6.21 KB, patch)
2014-02-12 00:02 PST, Gyuyoung Kim
no flags
Patch (6.21 KB, patch)
2014-02-12 00:04 PST, Gyuyoung Kim
no flags
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (1.05 MB, application/zip)
2014-02-12 01:10 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (1.07 MB, application/zip)
2014-02-12 01:29 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (1.13 MB, application/zip)
2014-02-12 01:39 PST, Build Bot
no flags
Patch (6.35 KB, patch)
2014-02-12 17:48 PST, Gyuyoung Kim
no flags
Patch (7.04 KB, patch)
2014-02-14 02:30 PST, Gyuyoung Kim
no flags
Patch (8.42 KB, patch)
2014-02-14 22:18 PST, Gyuyoung Kim
no flags
Patch (8.51 KB, patch)
2014-02-14 22:30 PST, Gyuyoung Kim
no flags
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (456.68 KB, application/zip)
2014-02-14 23:22 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (494.68 KB, application/zip)
2014-02-15 00:01 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (487.33 KB, application/zip)
2014-02-15 00:56 PST, Build Bot
no flags
Patch (8.10 KB, patch)
2014-02-16 23:50 PST, Gyuyoung Kim
no flags
Gyuyoung Kim
Comment 1 2014-02-12 00:02:06 PST
Gyuyoung Kim
Comment 2 2014-02-12 00:04:33 PST
Build Bot
Comment 3 2014-02-12 01:10:27 PST
Comment on attachment 223946 [details] Patch Attachment 223946 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6255577655148544 New failing tests: svg/animations/svgPreserveAspectRatio-animation-1.html svg/dom/preserve-aspect-ratio-parser.html http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm svg/W3C-SVG-1.1/coords-viewattr-01-b.svg svg/zoom/text/zoom-coords-viewattr-01-b.svg svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html svg/zoom/page/zoom-coords-viewattr-01-b.svg svg/dom/SVGViewSpec.html
Build Bot
Comment 4 2014-02-12 01:10:30 PST
Created attachment 223951 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Build Bot
Comment 5 2014-02-12 01:29:17 PST
Comment on attachment 223946 [details] Patch Attachment 223946 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5510968301846528 New failing tests: svg/animations/svgPreserveAspectRatio-animation-1.html svg/dom/preserve-aspect-ratio-parser.html http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm svg/W3C-SVG-1.1/coords-viewattr-01-b.svg svg/zoom/text/zoom-coords-viewattr-01-b.svg svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html svg/zoom/page/zoom-coords-viewattr-01-b.svg svg/dom/SVGViewSpec.html
Build Bot
Comment 6 2014-02-12 01:29:20 PST
Created attachment 223953 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Build Bot
Comment 7 2014-02-12 01:39:08 PST
Comment on attachment 223946 [details] Patch Attachment 223946 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5752005356158976 New failing tests: svg/animations/svgPreserveAspectRatio-animation-1.html svg/dom/preserve-aspect-ratio-parser.html http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm svg/W3C-SVG-1.1/coords-viewattr-01-b.svg svg/zoom/text/zoom-coords-viewattr-01-b.svg svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html svg/zoom/page/zoom-coords-viewattr-01-b.svg svg/dom/SVGViewSpec.html
Build Bot
Comment 8 2014-02-12 01:39:11 PST
Created attachment 223954 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Andreas Kling
Comment 9 2014-02-12 10:59:29 PST
Comment on attachment 223946 [details] Patch EWS says this has bugs. Also, what are we fixing here?
Gyuyoung Kim
Comment 10 2014-02-12 15:29:16 PST
(In reply to comment #9) > (From update of attachment 223946 [details]) > EWS says this has bugs. Also, what are we fixing here? Though this patch is trivial, I think it is better not to use "goto" in function. That's why I replace "goto" with local variable.
Gyuyoung Kim
Comment 11 2014-02-12 17:48:52 PST
Gyuyoung Kim
Comment 12 2014-02-12 17:49:38 PST
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 223946 [details] [details]) > > EWS says this has bugs. Also, what are we fixing here? > > Though this patch is trivial, I think it is better not to use "goto" in function. That's why I replace "goto" with local variable. Fixed the failing tests.
Dirk Schulze
Comment 13 2014-02-13 04:50:08 PST
(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #9) > > > (From update of attachment 223946 [details] [details] [details]) > > > EWS says this has bugs. Also, what are we fixing here? > > > > Though this patch is trivial, I think it is better not to use "goto" in function. That's why I replace "goto" with local variable. > > Fixed the failing tests. I am curious. You landed the patch in Blink first. Did you land a different patch there, or did the tests not trigger problems there?
Dirk Schulze
Comment 14 2014-02-13 04:55:20 PST
Comment on attachment 224035 [details] Patch Looks good in general. It is sad that we set align and meetOrSlice twice on successful parsing. Who is calling ::parse here? If it is an internal function, can we pass a reference to align and meetOrSlice into an inline function instead? Also, if we return flaw ein your code, do we give a warning on the console like for the other attributes?
Gyuyoung Kim
Comment 15 2014-02-13 20:45:42 PST
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > (From update of attachment 223946 [details] [details] [details] [details]) > > > > EWS says this has bugs. Also, what are we fixing here? > > > > > > Though this patch is trivial, I think it is better not to use "goto" in function. That's why I replace "goto" with local variable. > > > > Fixed the failing tests. > > I am curious. You landed the patch in Blink first. Did you land a different patch there, or did the tests not trigger problems there? No, almost is same. I just missed to use local variable in first patch. There is no difference between them except for checking the exception code in setAlign() and setMeetOrSlice().
Gyuyoung Kim
Comment 16 2014-02-13 21:03:28 PST
(In reply to comment #14) > (From update of attachment 224035 [details]) > Looks good in general. It is sad that we set align and meetOrSlice twice on successful parsing. Who is calling ::parse here? If it is an internal function, can we pass a reference to align and meetOrSlice into an inline function instead? > > Also, if we return flaw ein your code, do we give a warning on the console like for the other attributes? Let me try to adjust your suggestion to this patch.
Gyuyoung Kim
Comment 17 2014-02-14 01:58:54 PST
(In reply to comment #14) > (From update of attachment 224035 [details]) > Looks good in general. It is sad that we set align and meetOrSlice twice on successful parsing. Who is calling ::parse here? If it is an internal function, can we pass a reference to align and meetOrSlice into an inline function instead? It is a public function. And also, It is invoked by SVGViewSpec.cpp. https://trac.webkit.org/browser/trunk/Source/WebCore/svg/SVGViewSpec.cpp#L266 > Also, if we return flaw ein your code, do we give a warning on the console like for the other attributes? It looks we may print some warning messages using LOG_ERROR. Do you agree to use LOG_ERROR to print a warning ? Besides I think it would be good to use it only for key points.
Gyuyoung Kim
Comment 18 2014-02-14 02:06:23 PST
(In reply to comment #17) > > Also, if we return flaw ein your code, do we give a warning on the console like for the other attributes? > > It looks we may print some warning messages using LOG_ERROR. Do you agree to use LOG_ERROR to print a warning ? Besides I think it would be good to use it only for key points. Or, did you mean that we give a warning when the exception code value is not 0 ?
Gyuyoung Kim
Comment 19 2014-02-14 02:30:37 PST
Dirk Schulze
Comment 20 2014-02-14 03:45:43 PST
Comment on attachment 224185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224185&action=review > Source/WebCore/svg/SVGPreserveAspectRatio.cpp:72 > + setAlign(align, ec); > + setMeetOrSlice(meetOrSlice, ec); You set the initial values here and if the parsing is successful at the end again. I didn't check if the two values are pure enums, or if there is more that needs to be done (update DOM stack). A solutions would be to pass align and meetOrSlice into this function as reference. You say that parse is a public function. Who else is calling SVGPreserveAspectRatio::parse? Is it somewhere outside of SVGPreserveAspectRatio?
Gyuyoung Kim
Comment 21 2014-02-14 22:18:21 PST
Gyuyoung Kim
Comment 22 2014-02-14 22:21:16 PST
(In reply to comment #20) > (From update of attachment 224185 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224185&action=review > > > Source/WebCore/svg/SVGPreserveAspectRatio.cpp:72 > > + setAlign(align, ec); > > + setMeetOrSlice(meetOrSlice, ec); > > You set the initial values here and if the parsing is successful at the end again. I didn't check if the two values are pure enums, or if there is more that needs to be done (update DOM stack). A solutions would be to pass align and meetOrSlice into this function as reference. To hide concrete parse() function, I change the existing parse() with parseInternal(), and add parse() to support existing usage. How do you think about this suggestion ? > You say that parse is a public function. Who else is calling SVGPreserveAspectRatio::parse? Is it somewhere outside of SVGPreserveAspectRatio? As mentioned in comment #17, this is being used by SVGViewSpec::parseViewSpec(). https://trac.webkit.org/browser/trunk/Source/WebCore/svg/SVGViewSpec.cpp#L266
Gyuyoung Kim
Comment 23 2014-02-14 22:30:09 PST
Build Bot
Comment 24 2014-02-14 23:22:09 PST
Comment on attachment 224287 [details] Patch Attachment 224287 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5176014002978816 New failing tests: svg/dom/preserve-aspect-ratio-parser.html
Build Bot
Comment 25 2014-02-14 23:22:12 PST
Created attachment 224288 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Gyuyoung Kim
Comment 26 2014-02-14 23:45:51 PST
(In reply to comment #24) > (From update of attachment 224287 [details]) > Attachment 224287 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/5176014002978816 > > New failing tests: > svg/dom/preserve-aspect-ratio-parser.html Let me fix this fail.
Build Bot
Comment 27 2014-02-15 00:01:10 PST
Comment on attachment 224287 [details] Patch Attachment 224287 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5505548321554432 New failing tests: svg/dom/preserve-aspect-ratio-parser.html
Build Bot
Comment 28 2014-02-15 00:01:17 PST
Created attachment 224289 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Build Bot
Comment 29 2014-02-15 00:56:10 PST
Comment on attachment 224287 [details] Patch Attachment 224287 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5288826150846464 New failing tests: svg/dom/preserve-aspect-ratio-parser.html
Build Bot
Comment 30 2014-02-15 00:56:15 PST
Created attachment 224290 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Gyuyoung Kim
Comment 31 2014-02-16 23:50:13 PST
Gyuyoung Kim
Comment 32 2014-02-17 00:02:32 PST
(In reply to comment #31) > Created an attachment (id=224338) [details] > Patch >> A solutions would be to pass align and meetOrSlice into this function as reference. In new patch, I change the parse() with parseInternal(). And, existing parse() calls the parseInternal(). If we pass references of align and meetOrSlice, it looks caller needs to set the reference to m_align, m_meetOrSlice as below, void SVGPreserveAspectRatio::parse(const String& value) { SVGPreserveAspectRatioType align = SVG_PRESERVEASPECTRATIO_XMIDYMID; SVGMeetOrSliceType meetOrSlice = SVG_MEETORSLICE_MEET; const UChar* begin = value.deprecatedCharacters(); parse(begin, begin + value.length(), true); parseInternal(begin, begin + value.length(), true, align, meetOrSlice); m_align = align; m_meetOrSlice = meetOrSlice; } It seems the work is a little overhead for caller. I think it would be nicer to handle it inside parseInternal() itself. So, latest patch just sets m_align and m_meetOrSlice in beginning/end of parseInternal(). I think it is not big overhead. Dirk, How do you think ?
Gyuyoung Kim
Comment 33 2014-02-17 14:47:02 PST
(In reply to comment #32) > (In reply to comment #31) > > Created an attachment (id=224338) [details] [details] > > Patch > > >> A solutions would be to pass align and meetOrSlice into this function as reference. > > In new patch, I change the parse() with parseInternal(). And, existing parse() calls the parseInternal(). If we pass references of align and meetOrSlice, it looks caller needs to set the reference to m_align, m_meetOrSlice as below, > > void SVGPreserveAspectRatio::parse(const String& value) > { > SVGPreserveAspectRatioType align = SVG_PRESERVEASPECTRATIO_XMIDYMID; > SVGMeetOrSliceType meetOrSlice = SVG_MEETORSLICE_MEET; > > const UChar* begin = value.deprecatedCharacters(); > parse(begin, begin + value.length(), true); > parseInternal(begin, begin + value.length(), true, align, meetOrSlice); > m_align = align; > m_meetOrSlice = meetOrSlice; > } > > It seems the work is a little overhead for caller. I think it would be nicer to handle it inside parseInternal() itself. So, latest patch just sets m_align and m_meetOrSlice in beginning/end of parseInternal(). I think it is not big overhead. Dirk, How do you think ? Dirk, what do you think about latest patch ?
Dirk Schulze
Comment 34 2014-02-18 01:05:17 PST
Comment on attachment 224338 [details] Patch LGTM. r=me.
Gyuyoung Kim
Comment 35 2014-02-18 01:07:33 PST
Comment on attachment 224338 [details] Patch Thanks !
WebKit Commit Bot
Comment 36 2014-02-18 01:37:30 PST
Comment on attachment 224338 [details] Patch Clearing flags on attachment: 224338 Committed r164276: <http://trac.webkit.org/changeset/164276>
WebKit Commit Bot
Comment 37 2014-02-18 01:37:36 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.