Bug 108050 – SVGPathStringSource should not up convert 8-bit strings to UTF-16

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

SVGPathStringSource should not upconvert UTF-8 strings to UTF-16

[Sam Weinig]

Created attachment 184938 [details]
Patch

[Sam Weinig]

Some things I am not sure of in this patch:
- Should I move the FloatPoint parsing helpers elsewhere?
- Should I use a union for the character pointers?
- Should I be caching the m_is8BitSource bit?

[WebKit Review Bot]

Attachment 184938 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/svg/SVGParserUtilities.cpp', u'Source/WebCore/svg/SVGParserUtilities.h', u'Source/WebCore/svg/SVGPathSource.h', u'Source/WebCore/svg/SVGPathStringSource.cpp', u'Source/WebCore/svg/SVGPathStringSource.h']" exit_code: 1
Source/WebCore/svg/SVGPathStringSource.cpp:102:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:186:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:232:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:240:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:248:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:256:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:264:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:272:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:280:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:288:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/svg/SVGPathStringSource.cpp:313:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 12 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen Chenney]

Comment on attachment 184938 [details]
Patch

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

I am not a reviewer yet, so someone official will have to weigh in.

Regarding your open questions...
- I support moving the parseFloatPoint methods to the utilities class.
- I support a union for the 8-bit and 16-bit string fields
- It looks like is8bit is efficient to evaluate, so I don't think there is much need to cache it. This code is not in the fast or expensive part of SVG path rendering.

> Source/WebCore/svg/SVGPathStringSource.cpp:105
> +        return m_currentCharacter16 < m_endCharacter16;

return m_is8BitSource ? ( m_currentCharacter8 < m_endCharacter8 ) : ( m_currentCharacter16 < m_endCharacter16 );

> Source/WebCore/svg/SVGPathStringSource.cpp:114
>  }

Ditto.

> Source/WebCore/svg/SVGPathStringSource.cpp:189
> +        return parseSVGSegmentTypeHelper(m_currentCharacter16, pathSegType);

Ditto.

> Source/WebCore/svg/SVGPathStringSource.cpp:225
>      SVGPathSegType nextCommand;

Why not move this up to above the "if" and remove the extra declarations inside the if/else blocks?

> Source/WebCore/svg/SVGPathStringSource.cpp:235
> +        return WebCore::parseFloatPoint(m_currentCharacter16, m_endCharacter16, targetPoint);

Same ?: fix.

> Source/WebCore/svg/SVGPathStringSource.cpp:243
> +        return WebCore::parseFloatPoint(m_currentCharacter16, m_endCharacter16, targetPoint);

Ditto.

> Source/WebCore/svg/SVGPathStringSource.cpp:251
> +        return parseNumber(m_currentCharacter16, m_endCharacter16, x);

Ditto.

> Source/WebCore/svg/SVGPathStringSource.cpp:267
> +        return WebCore::parseFloatPoint3(m_currentCharacter16, m_endCharacter16, point1, point2, targetPoint);

Ditto.

> Source/WebCore/svg/SVGPathStringSource.cpp:283
> +        return WebCore::parseFloatPoint2(m_currentCharacter16, m_endCharacter16, point2, targetPoint);

Ditto.

> Source/WebCore/svg/SVGPathStringSource.cpp:291
> +        return WebCore::parseFloatPoint(m_currentCharacter16, m_endCharacter16, targetPoint);

Ditto.

[Anders Carlsson]

Comment on attachment 184938 [details]
Patch

I think you can remove the WebCore:: specifiers - patch looks fine otherwise.

[Stephen Chenney]

Please at least clean up the style issues before committing. The way that style checking works now, existing problems in the code (which this patch would introduce) will cause style errors in future changes in the code.

I would also like the other changes to make things simpler for future editing. This code is likely to change shortly due to SVG 2 spec changes.

[Sam Weinig]

Committed r140985: <http://trac.webkit.org/changeset/140985>

[Philip Rogers]

(In reply to comment #7)
> Committed r140985: <http://trac.webkit.org/changeset/140985>

Unsurprisingly this also led to a performance increase :) Around 5% on this test; nice work:
http://webkit-perf.appspot.com/graph.html#tests=[[10670997,2001,173262]]&sel=1359355423972.2766,1359447995400.8481,26.956521739130423,93.04347826086956&displayrange=30&datatype=running