Bug 108050 - SVGPathStringSource should not up convert 8-bit strings to UTF-16
Summary: SVGPathStringSource should not up convert 8-bit strings to UTF-16
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-27 21:58 PST by Sam Weinig
Modified: 2013-01-30 15:56 PST (History)
5 users (show)

See Also:


Attachments
Patch (17.38 KB, patch)
2013-01-27 22:01 PST, Sam Weinig
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2013-01-27 21:58:31 PST
SVGPathStringSource should not upconvert UTF-8 strings to UTF-16
Comment 1 Sam Weinig 2013-01-27 22:01:56 PST
Created attachment 184938 [details]
Patch
Comment 2 Sam Weinig 2013-01-27 22:03:41 PST
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?
Comment 3 WebKit Review Bot 2013-01-27 22:05:40 PST
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.
Comment 4 Stephen Chenney 2013-01-28 08:29:21 PST
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.
Comment 5 Anders Carlsson 2013-01-28 10:09:13 PST
Comment on attachment 184938 [details]
Patch

I think you can remove the WebCore:: specifiers - patch looks fine otherwise.
Comment 6 Stephen Chenney 2013-01-28 10:23:15 PST
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.
Comment 7 Sam Weinig 2013-01-28 11:29:27 PST
Committed r140985: <http://trac.webkit.org/changeset/140985>
Comment 8 Philip Rogers 2013-01-30 15:56:50 PST
(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