Rework the SVGPathSource interface

Rework the SVGPathSource interface

This CL replaces the current per-command interface of SVGPathSource
with a per-segment interface.

A new struct PathSegmentData is added, and existing SVGPathSources are
modified to implement parse into this struct. For the most part this is
a trivial copy and paste operation. In SVGPathStringSource, some of the
helpers are refactored, and leading whitespace is consumed in the
constructor to avoid having something similar to moveToNextToken() in
the new interface. Number parsing is also made "latched", by setting a
flag on error, and only check that flag when all the input has been
considered. This makes the code a bit more streamlined while optimizing
for the common case (of no error.) This also renders the
parseFloatPoint* helpers unused, so they are removed.

peekSegmentType() is added to support checking if the first segment is
a move command without directly affecting the common 'transformation'
loop.

Users of the SVGPathSource interface (SVGPathParser and SVGPathBlender)
are refactored to make use of the new interface.
For SVGPathParser this means renaming the parse* methods to emit*,
while hoisting the input-handling out into the main "parsing" loop, and
passing the parsed PathSegmentData to the right emitter.
Similarly for SVGPathBlender, the input-handling is hoisted out of the
blend* methods, and the two inputs are passed as parameters. The
command equality check is changed to only be based on the command type.

BUG=467592
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192348

[fs, 2015-03-20 12:06:02]

fs@opera.com changed reviewers:
+ ed@opera.com, fmalita@chromium.org, kouhei@chromium.org, pdr@chromium.org

[fs, 2015-03-20 13:46:09]

So this is where the "meatier" parts start - still not too big a CL I hope.

[Stephen Chennney, 2015-03-20 19:40:55]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2015-03-20 19:40:55]

LGTM. Tricky to review, but the new code is indeed much better.

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathStri...
File Source/core/svg/SVGPathStringSource.cpp (right):

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathStri...
Source/core/svg/SVGPathStringSource.cpp:178: ASSERT(!m_seenError);
This concerned me because I think of a command that we can't parse as an error
we should somehow report. But I know we don't do this right now (I looked into
it at some point in the past) so I'm OK with moving forward as is.

[f(malita), 2015-03-22 14:18:48]

lgtm++

I agree, this looks much nicer.

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathBlen...
File Source/core/svg/SVGPathBlender.cpp (right):

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:300: case PathSegUnknown:
Let's make this 'default' to ensure full enum coverage.

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:308: if (!m_fromSource->hasMoreData() ||
!m_toSource->hasMoreData())
I believe this conditional can go: the one above established that
m_toSource->hasMoreData() == m_fromSource->hasMoreData(), and the loop condition
is m_toSource->hasMoreData() - so it should break naturally and return true.

[kouhei (in TOK), 2015-03-23 01:44:48]

lgtm

[fs, 2015-03-23 11:22:10]

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathBlen...
File Source/core/svg/SVGPathBlender.cpp (right):

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:300: case PathSegUnknown:
On 2015/03/22 14:18:48, f(malita) wrote:
> Let's make this 'default' to ensure full enum coverage.

Done.

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:308: if (!m_fromSource->hasMoreData() ||
!m_toSource->hasMoreData())
On 2015/03/22 14:18:48, f(malita) wrote:
> I believe this conditional can go: the one above established that
> m_toSource->hasMoreData() == m_fromSource->hasMoreData(), and the loop
condition
> is m_toSource->hasMoreData() - so it should break naturally and return true.

Make sense. Done.

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathStri...
File Source/core/svg/SVGPathStringSource.cpp (right):

https://codereview.chromium.org/1023993002/diff/1/Source/core/svg/SVGPathStri...
Source/core/svg/SVGPathStringSource.cpp:178: ASSERT(!m_seenError);
On 2015/03/20 19:40:55, Stephen Chenney wrote:
> This concerned me because I think of a command that we can't parse as an error
> we should somehow report. But I know we don't do this right now (I looked into
> it at some point in the past) so I'm OK with moving forward as is.

Yes, error reporting here is not great (or readable for large input). I have
some ideas for how that could be improved - now I just need to find that
mythical creature known as "time", so I can try it out =)

[fs, 2015-03-23 12:03:39]

The CQ bit was checked by fs@opera.com

[fs, 2015-03-23 12:03:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, schenney@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/1023993002/#ps20001
(title: "Blender fixups.")

[commit-bot: I haz the power, 2015-03-23 12:03:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023993002/20001

[commit-bot: I haz the power, 2015-03-23 12:07:23]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192348