This cl removes "goto" and replaces it with local variables, and fixes poor coding style.
Created attachment 223945 [details] Patch
Created attachment 223946 [details] Patch
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
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
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
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
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
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
Comment on attachment 223946 [details] Patch EWS says this has bugs. Also, what are we fixing here?
(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.
Created attachment 224035 [details] Patch
(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.
(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?
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?
(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().
(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.
(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.
(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 ?
Created attachment 224185 [details] Patch
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?
Created attachment 224286 [details] Patch
(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
Created attachment 224287 [details] Patch
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
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
(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.
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
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
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
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
Created attachment 224338 [details] Patch
(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 ?
(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 ?
Comment on attachment 224338 [details] Patch LGTM. r=me.
Comment on attachment 224338 [details] Patch Thanks !
Comment on attachment 224338 [details] Patch Clearing flags on attachment: 224338 Committed r164276: <http://trac.webkit.org/changeset/164276>
All reviewed patches have been landed. Closing bug.