Bug 128658 – Refactor SVGPreserveAspectRatio::parse()

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

[Gyuyoung Kim]

This cl removes "goto" and replaces it with local variables, and fixes poor coding style.

[Gyuyoung Kim]

Created attachment 223945 [details]
Patch

[Gyuyoung Kim]

Created attachment 223946 [details]
Patch

[Build Bot]

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]

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 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]

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 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]

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 on attachment 223946 [details]
Patch

EWS says this has bugs. Also, what are we fixing here?

[Gyuyoung Kim]

(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]

Created attachment 224035 [details]
Patch

[Gyuyoung Kim]

(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]

(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 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]

(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]

(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]

(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]

(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]

Created attachment 224185 [details]
Patch

[Dirk Schulze]

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]

Created attachment 224286 [details]
Patch

[Gyuyoung Kim]

(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]

Created attachment 224287 [details]
Patch

[Build Bot]

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]

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]

(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 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]

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 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]

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]

Created attachment 224338 [details]
Patch

[Gyuyoung Kim]

(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]

(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 on attachment 224338 [details]
Patch

LGTM. r=me.

[Gyuyoung Kim]

Comment on attachment 224338 [details]
Patch

Thanks !

[WebKit Commit Bot]

Comment on attachment 224338 [details]
Patch

Clearing flags on attachment: 224338

Committed r164276: <http://trac.webkit.org/changeset/164276>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.