Refactor parsing in SVGTransformList

Refactor parsing in SVGTransformList

In preparation for extended error reporting.

Split SVGTransform creation out of parseTransformOfType, and then fold
the remains into parseTransformParamList - naming the result
parseTransformArgumentsForType. Use a Vector with suitable
initial-capacity rather than a float[]. Change the handling trailing
commas so that it is not triggered when the maximum number of arguments
are reached. (This will allow for better errors to be reported in some
cases.)
Change parseAndSkipTransformType to return the parsed type via the
return value rather than an out parameter.
Reduce the number of calls to skipOptionalSVGSpaces in parseInternal
and parseTransformArgumentsForType, and make better use of the return
value from it.

Also make SVGTransformList::consolidate() and add() use initialize(...)
rather than open-coding it.

BUG=231612
Committed: https://crrev.com/440669dd8170823c9643d40060fd666297c73313
Cr-Commit-Position: refs/heads/master@{#372656}

[fs, 2016-01-29 18:43:18]

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

[f(malita), 2016-01-29 20:51:14]

LGTM + discretionary nits

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTransformList.cpp (right):

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:100: // These should be
kept in sync with enum SVGTransformType
Nit: in cases like this, I normally add static asserts for all the enum values;
feel free to ignore if it seems like overkill.

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:103: const unsigned
MaxTransformArguments = 6;
Nit: kMaxTransformArguments? Or am I getting them codestyles all mixed up in my
head? :)

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:115: const size_t
maxPossibleParams = required + optional;
Nit: just in case these end up changing -

ASSERT(maxPossibleParams <= MaxTransformArguments);

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:128: if
(arguments.size() == maxPossibleParams)
Aren't we breaking too early here?  It looks like we might be missing a
potential trailing delimiter - e.g. translate(1,1,) would parse with the new
version but not prev one?

(later)
never mind, I see the caller catches this case.

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:205: return false;
Nit:

ASSERT(valueCount >= requiredValuesForType[transformType]);

[fs, 2016-01-29 21:48:13]

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTransformList.cpp (right):

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:100: // These should be
kept in sync with enum SVGTransformType
On 2016/01/29 at 20:51:14, f(malita) wrote:
> Nit: in cases like this, I normally add static asserts for all the enum
values; feel free to ignore if it seems like overkill.

Since these (enums) are web exposed API (and unlikely to be added to...) I don't
think it's very important (and already checked by bindings IIRC.)

That being said, having the "knowledge" locally might be "illustrative" I
suppose. I assume you mean asserts like:

static_assert(SVG_TRANSFORM_UNKNOWN == 0...);
...
static_assert(SVG_TRANSFORM_SKEWY == 6...);
static_assert(WTF_ARRAY_LENGTH(...) == SVG_...);

?

Not sure if there's anything else we could usefully assert here.

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:103: const unsigned
MaxTransformArguments = 6;
On 2016/01/29 at 20:51:14, f(malita) wrote:
> Nit: kMaxTransformArguments? Or am I getting them codestyles all mixed up in
my head? :)

kFoo is "where we're going" (Chromium Style). I suspect using the k-form now
would be ok... (Not 100% if current Blink style says anything specificly in this
case, but this is consistent with enum naming at least...)

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:128: if
(arguments.size() == maxPossibleParams)
On 2016/01/29 at 20:51:14, f(malita) wrote:
> Aren't we breaking too early here?  It looks like we might be missing a
potential trailing delimiter - e.g. translate(1,1,) would parse with the new
version but not prev one?
> 
> (later)
> never mind, I see the caller catches this case.

This was born out of a wish to get better error reporting - without doing too
much surgery... (yes, this is just "a little" =)). There might a little change
in behavior for the other caller though I guess - I think that caller was more
lax already though. (Compare: "1 2 foo" and "1,2,foo" for instance - in the
<animateTransform> caller.) I think I can tighten that up a bit though (require
the trailer to be WS-only.)

[fs, 2016-02-01 13:36:20]

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTransformList.cpp (right):

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:100: // These should be
kept in sync with enum SVGTransformType
On 2016/01/29 at 21:48:13, fs wrote:
> On 2016/01/29 at 20:51:14, f(malita) wrote:
> > Nit: in cases like this, I normally add static asserts for all the enum
values; feel free to ignore if it seems like overkill.
> 
> Since these (enums) are web exposed API (and unlikely to be added to...) I
don't think it's very important (and already checked by bindings IIRC.)
> 
> That being said, having the "knowledge" locally might be "illustrative" I
suppose. I assume you mean asserts like:
> 
> static_assert(SVG_TRANSFORM_UNKNOWN == 0...);
> ...
> static_assert(SVG_TRANSFORM_SKEWY == 6...);
> static_assert(WTF_ARRAY_LENGTH(...) == SVG_...);
> 
> ?
> 
> Not sure if there's anything else we could usefully assert here.

Asserts added.

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:103: const unsigned
MaxTransformArguments = 6;
On 2016/01/29 at 21:48:13, fs wrote:
> On 2016/01/29 at 20:51:14, f(malita) wrote:
> > Nit: kMaxTransformArguments? Or am I getting them codestyles all mixed up in
my head? :)
> 
> kFoo is "where we're going" (Chromium Style). I suspect using the k-form now
would be ok... (Not 100% if current Blink style says anything specificly in this
case, but this is consistent with enum naming at least...)

Changed to k-prefixed.

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:115: const size_t
maxPossibleParams = required + optional;
On 2016/01/29 at 20:51:14, f(malita) wrote:
> Nit: just in case these end up changing -
> 
> ASSERT(maxPossibleParams <= MaxTransformArguments);

Done.

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:128: if
(arguments.size() == maxPossibleParams)
On 2016/01/29 at 21:48:13, fs wrote:
> On 2016/01/29 at 20:51:14, f(malita) wrote:
> > Aren't we breaking too early here?  It looks like we might be missing a
potential trailing delimiter - e.g. translate(1,1,) would parse with the new
version but not prev one?
> > 
> > (later)
> > never mind, I see the caller catches this case.
> 
> This was born out of a wish to get better error reporting - without doing too
much surgery... (yes, this is just "a little" =)). There might a little change
in behavior for the other caller though I guess - I think that caller was more
lax already though. (Compare: "1 2 foo" and "1,2,foo" for instance - in the
<animateTransform> caller.) I think I can tighten that up a bit though (require
the trailer to be WS-only.)

Tightened up SVGTransformList::create(SVGTransformType, const String&) a bit to
address this.

https://codereview.chromium.org/1643243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:205: return false;
On 2016/01/29 at 20:51:14, f(malita) wrote:
> Nit:
> 
> ASSERT(valueCount >= requiredValuesForType[transformType]);

Done.

[f(malita), 2016-02-01 13:39:15]

Thanks, still LGTM :)

[fs, 2016-02-01 13:58:14]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-02-01 13:58:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1643243002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1643243002/60001

[commit-bot: I haz the power, 2016-02-01 14:03:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-01 14:05:11]

Description was changed from

==========
Refactor parsing in SVGTransformList

In preparation for extended error reporting.

Split SVGTransform creation out of parseTransformOfType, and then fold
the remains into parseTransformParamList - naming the result
parseTransformArgumentsForType. Use a Vector with suitable
initial-capacity rather than a float[]. Change the handling trailing
commas so that it is not triggered when the maximum number of arguments
are reached. (This will allow for better errors to be reported in some
cases.)
Change parseAndSkipTransformType to return the parsed type via the
return value rather than an out parameter.
Reduce the number of calls to skipOptionalSVGSpaces in parseInternal
and parseTransformArgumentsForType, and make better use of the return
value from it.

Also make SVGTransformList::consolidate() and add() use initialize(...)
rather than open-coding it.

BUG=231612
==========

to

==========
Refactor parsing in SVGTransformList

In preparation for extended error reporting.

Split SVGTransform creation out of parseTransformOfType, and then fold
the remains into parseTransformParamList - naming the result
parseTransformArgumentsForType. Use a Vector with suitable
initial-capacity rather than a float[]. Change the handling trailing
commas so that it is not triggered when the maximum number of arguments
are reached. (This will allow for better errors to be reported in some
cases.)
Change parseAndSkipTransformType to return the parsed type via the
return value rather than an out parameter.
Reduce the number of calls to skipOptionalSVGSpaces in parseInternal
and parseTransformArgumentsForType, and make better use of the return
value from it.

Also make SVGTransformList::consolidate() and add() use initialize(...)
rather than open-coding it.

BUG=231612

Committed: https://crrev.com/440669dd8170823c9643d40060fd666297c73313
Cr-Commit-Position: refs/heads/master@{#372656}
==========

[commit-bot: I haz the power, 2016-02-01 14:05:12]

Patchset 4 (id:??) landed as
https://crrev.com/440669dd8170823c9643d40060fd666297c73313
Cr-Commit-Position: refs/heads/master@{#372656}