Handle semicolons at end of keySplines value

Handle semicolons at end of keySplines value

Firefox has more lenient behavior than we have:

https://bugzilla.mozilla.org/show_bug.cgi?id=705236

Discussion on the w3c list indicates we want said behavior:

http://lists.w3.org/Archives/Public/www-svg/2014Sep/0016.html

Adopting this behavior for a single trailing semicolon
fixes the testcase and makes the parsing code a bit simpler.
animate-keySplines.html tests this behavior.

When using more than one semicolon at the end of keySplines
ignore parsing of the value and report an error on the console.
keySplines-parsing-error.html tests this behavior.

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

[rwlbuis, 2014-09-04 23:50:04]

rob.buis@samsung.com changed reviewers:
+ pdr@chromium.org

[rwlbuis, 2014-09-04 23:50:21]

PTAL

[pdr., 2014-09-05 02:59:27]

On 2014/09/04 23:50:21, rwlbuis wrote:
> PTAL

More lenient parsing sounds good to me, as interop is important in this area. I
need a little help understanding the Gecko change though... it looks like Gecko
was changing their implementation to match ours!

[fs, 2014-09-05 08:59:03]

On 2014/09/05 02:59:27, pdr wrote:
> On 2014/09/04 23:50:21, rwlbuis wrote:
> > PTAL
> 
> More lenient parsing sounds good to me, as interop is important in this area.
I
> need a little help understanding the Gecko change though... it looks like
Gecko
> was changing their implementation to match ours!

Might be worth nothing that this make parsing of 'keySplines' consistent with
the parsing of 'values' and 'keyTimes' (which use, and hence depend on, the
behavior of the String::split version it uses - which ignores/drops empty
trails.) The Gecko change seems to primarily be about 'values' [1] - which might
be why they want to match Blink/WebKit.

[1] Quote from description: 'In our parsing of the SMIL "values" attribute...'

[rwlbuis, 2014-09-05 15:10:31]

On 2014/09/05 08:59:03, fs wrote:
> On 2014/09/05 02:59:27, pdr wrote:
> > On 2014/09/04 23:50:21, rwlbuis wrote:
> > > PTAL
> > 
> > More lenient parsing sounds good to me, as interop is important in this
area.
> I
> > need a little help understanding the Gecko change though... it looks like
> Gecko
> > was changing their implementation to match ours!
> 
> Might be worth nothing that this make parsing of 'keySplines' consistent with

I would not say 'nothing' ;) But yes, I had not seen 'values' and 'keyTimes'
accept
';' at the end, it all follows from using split.

> the parsing of 'values' and 'keyTimes' (which use, and hence depend on, the
> behavior of the String::split version it uses - which ignores/drops empty
> trails.) The Gecko change seems to primarily be about 'values' [1] - which
might
> be why they want to match Blink/WebKit.
> 
> [1] Quote from description: 'In our parsing of the SMIL "values" attribute...'

Thanks for pointing this out!

[rwlbuis, 2014-09-05 15:12:49]

On 2014/09/05 02:59:27, pdr wrote:
> On 2014/09/04 23:50:21, rwlbuis wrote:
> > PTAL
> 
> More lenient parsing sounds good to me, as interop is important in this area.
I
> need a little help understanding the Gecko change though... it looks like
Gecko
> was changing their implementation to match ours!

I think with fs' explanation it became clear. Let me know if the test needs
work, I tried
to change the test so it would use two animations, one with the delimiter at the
end, but
could not make that work. Simply copying the test just for this seemed a waste
to me. Maybe
the current solution, though lazy, is good enough? After all keySplines without
delimiter
at the end is already tested in a few other tests.

[kouhei (in TOK), 2014-09-05 16:48:09]

On 2014/09/05 15:12:49, rwlbuis wrote:
> On 2014/09/05 02:59:27, pdr wrote:
> > On 2014/09/04 23:50:21, rwlbuis wrote:
> > > PTAL
> > 
> > More lenient parsing sounds good to me, as interop is important in this
area.
> I
> > need a little help understanding the Gecko change though... it looks like
> Gecko
> > was changing their implementation to match ours!
> 
> I think with fs' explanation it became clear. Let me know if the test needs
> work, I tried
> to change the test so it would use two animations, one with the delimiter at
the
> end, but
> could not make that work. Simply copying the test just for this seemed a waste
> to me. Maybe
> the current solution, though lazy, is good enough? After all keySplines
without
> delimiter
> at the end is already tested in a few other tests.

There are many "parse" functions in core/svg and they aren't really consistent
on accepting trailing delimiters.
SVG spec doesn't seem to allow trailing delimiters, but if we are going this
way, should we align all parse functions to allow this?
https://svgwg.org/svg2-draft/single-page.html#types-DataTypeList
One difficulty would be list of strings, where we should be able to parse a zero
length string "".

[Erik Dahlström (inactive), 2014-09-08 07:32:19]

On 2014/09/05 16:48:09, kouhei wrote:
> On 2014/09/05 15:12:49, rwlbuis wrote:
> > On 2014/09/05 02:59:27, pdr wrote:
> > > On 2014/09/04 23:50:21, rwlbuis wrote:
> > > > PTAL
> > > 
> > > More lenient parsing sounds good to me, as interop is important in this
> area.
> > I
> > > need a little help understanding the Gecko change though... it looks like
> > Gecko
> > > was changing their implementation to match ours!
> > 
> > I think with fs' explanation it became clear. Let me know if the test needs
> > work, I tried
> > to change the test so it would use two animations, one with the delimiter at
> the
> > end, but
> > could not make that work. Simply copying the test just for this seemed a
waste
> > to me. Maybe
> > the current solution, though lazy, is good enough? After all keySplines
> without
> > delimiter
> > at the end is already tested in a few other tests.
> 
> There are many "parse" functions in core/svg and they aren't really consistent
> on accepting trailing delimiters.
> SVG spec doesn't seem to allow trailing delimiters, but if we are going this
> way, should we align all parse functions to allow this?
> https://svgwg.org/svg2-draft/single-page.html#types-DataTypeList
> One difficulty would be list of strings, where we should be able to parse a
zero
> length string "".

I think it would be good to make sure that all of the semicolon-delimited lists
have the same behavior, attributes: values, keyTimes, keySplines, keyPoints,
begin and end.

I've raised this issue on SVG2, but I don't think it's a big burden to have to
specify an additional semicolon if one really wanted the empty string, e.g
values="foo;;" (to be interpreted as two values, not three). Also note that the
spec requires ignoring leading and trailing whitespace in the values attribute.

[rwlbuis, 2014-09-17 15:17:44]

On 2014/09/08 07:32:19, Erik Dahlström wrote:
> On 2014/09/05 16:48:09, kouhei wrote:
> > On 2014/09/05 15:12:49, rwlbuis wrote:
> > > On 2014/09/05 02:59:27, pdr wrote:
> > > > On 2014/09/04 23:50:21, rwlbuis wrote:
> > > > > PTAL
> > > > 
> > > > More lenient parsing sounds good to me, as interop is important in this
> > area.
> > > I
> > > > need a little help understanding the Gecko change though... it looks
like
> > > Gecko
> > > > was changing their implementation to match ours!
> > > 
> > > I think with fs' explanation it became clear. Let me know if the test
needs
> > > work, I tried
> > > to change the test so it would use two animations, one with the delimiter
at
> > the
> > > end, but
> > > could not make that work. Simply copying the test just for this seemed a
> waste
> > > to me. Maybe
> > > the current solution, though lazy, is good enough? After all keySplines
> > without
> > > delimiter
> > > at the end is already tested in a few other tests.
> > 
> > There are many "parse" functions in core/svg and they aren't really
consistent
> > on accepting trailing delimiters.
> > SVG spec doesn't seem to allow trailing delimiters, but if we are going this
> > way, should we align all parse functions to allow this?
> > https://svgwg.org/svg2-draft/single-page.html#types-DataTypeList
> > One difficulty would be list of strings, where we should be able to parse a
> zero
> > length string "".
> 
> I think it would be good to make sure that all of the semicolon-delimited
lists
> have the same behavior, attributes: values, keyTimes, keySplines, keyPoints,
> begin and end.
> 
> I've raised this issue on SVG2, but I don't think it's a big burden to have to
> specify an additional semicolon if one really wanted the empty string, e.g
> values="foo;;" (to be interpreted as two values, not three). Also note that
the
> spec requires ignoring leading and trailing whitespace in the values
attribute.

IIRC this was discussed last week, any effect on this patch? I am wondering what
to add to the CL, more tests? Deal with the ";;" issue?

[Erik Dahlström (inactive), 2014-09-18 15:56:06]

On 2014/09/17 15:17:44, rwlbuis wrote:
> On 2014/09/08 07:32:19, Erik Dahlström wrote:
> > On 2014/09/05 16:48:09, kouhei wrote:
> > > On 2014/09/05 15:12:49, rwlbuis wrote:
> > > > On 2014/09/05 02:59:27, pdr wrote:
> > > > > On 2014/09/04 23:50:21, rwlbuis wrote:
> > > > > > PTAL
> > > > > 
> > > > > More lenient parsing sounds good to me, as interop is important in
this
> > > area.
> > > > I
> > > > > need a little help understanding the Gecko change though... it looks
> like
> > > > Gecko
> > > > > was changing their implementation to match ours!
> > > > 
> > > > I think with fs' explanation it became clear. Let me know if the test
> needs
> > > > work, I tried
> > > > to change the test so it would use two animations, one with the
delimiter
> at
> > > the
> > > > end, but
> > > > could not make that work. Simply copying the test just for this seemed a
> > waste
> > > > to me. Maybe
> > > > the current solution, though lazy, is good enough? After all keySplines
> > > without
> > > > delimiter
> > > > at the end is already tested in a few other tests.
> > > 
> > > There are many "parse" functions in core/svg and they aren't really
> consistent
> > > on accepting trailing delimiters.
> > > SVG spec doesn't seem to allow trailing delimiters, but if we are going
this
> > > way, should we align all parse functions to allow this?
> > > https://svgwg.org/svg2-draft/single-page.html#types-DataTypeList
> > > One difficulty would be list of strings, where we should be able to parse
a
> > zero
> > > length string "".
> > 
> > I think it would be good to make sure that all of the semicolon-delimited
> lists
> > have the same behavior, attributes: values, keyTimes, keySplines, keyPoints,
> > begin and end.
> > 
> > I've raised this issue on SVG2, but I don't think it's a big burden to have
to
> > specify an additional semicolon if one really wanted the empty string, e.g
> > values="foo;;" (to be interpreted as two values, not three). Also note that
> the
> > spec requires ignoring leading and trailing whitespace in the values
> attribute.
> 
> IIRC this was discussed last week, any effect on this patch? I am wondering
what
> to add to the CL, more tests? Deal with the ";;" issue?

Result of that discussion: a single trailing semicolon should be
allowed/ignored.

Please add a test for the ";;" issue.

For keySplines empty values such as would be the result from ";;" should mean
that the whole attribute is invalid (according to spec), should be treated as if
it the keySplines attribute wasn't specified.

[rwlbuis, 2014-09-18 18:34:11]

On 2014/09/18 15:56:06, Erik Dahlström wrote:
> On 2014/09/17 15:17:44, rwlbuis wrote:
> > On 2014/09/08 07:32:19, Erik Dahlström wrote:
> > > On 2014/09/05 16:48:09, kouhei wrote:
> > > > On 2014/09/05 15:12:49, rwlbuis wrote:
> > > > > On 2014/09/05 02:59:27, pdr wrote:
> > > > > > On 2014/09/04 23:50:21, rwlbuis wrote:
> > > > > > > PTAL
> > > > > > 
> > > > > > More lenient parsing sounds good to me, as interop is important in
> this
> > > > area.
> > > > > I
> > > > > > need a little help understanding the Gecko change though... it looks
> > like
> > > > > Gecko
> > > > > > was changing their implementation to match ours!
> > > > > 
> > > > > I think with fs' explanation it became clear. Let me know if the test
> > needs
> > > > > work, I tried
> > > > > to change the test so it would use two animations, one with the
> delimiter
> > at
> > > > the
> > > > > end, but
> > > > > could not make that work. Simply copying the test just for this seemed
a
> > > waste
> > > > > to me. Maybe
> > > > > the current solution, though lazy, is good enough? After all
keySplines
> > > > without
> > > > > delimiter
> > > > > at the end is already tested in a few other tests.
> > > > 
> > > > There are many "parse" functions in core/svg and they aren't really
> > consistent
> > > > on accepting trailing delimiters.
> > > > SVG spec doesn't seem to allow trailing delimiters, but if we are going
> this
> > > > way, should we align all parse functions to allow this?
> > > > https://svgwg.org/svg2-draft/single-page.html#types-DataTypeList
> > > > One difficulty would be list of strings, where we should be able to
parse
> a
> > > zero
> > > > length string "".
> > > 
> > > I think it would be good to make sure that all of the semicolon-delimited
> > lists
> > > have the same behavior, attributes: values, keyTimes, keySplines,
keyPoints,
> > > begin and end.
> > > 
> > > I've raised this issue on SVG2, but I don't think it's a big burden to
have
> to
> > > specify an additional semicolon if one really wanted the empty string, e.g
> > > values="foo;;" (to be interpreted as two values, not three). Also note
that
> > the
> > > spec requires ignoring leading and trailing whitespace in the values
> > attribute.
> > 
> > IIRC this was discussed last week, any effect on this patch? I am wondering
> what
> > to add to the CL, more tests? Deal with the ";;" issue?
> 
> Result of that discussion: a single trailing semicolon should be
> allowed/ignored.
> 
> Please add a test for the ";;" issue.
> 
> For keySplines empty values such as would be the result from ";;" should mean
> that the whole attribute is invalid (according to spec), should be treated as
if
> it the keySplines attribute wasn't specified.

Thanks for the summary, my latest patch set adds a test for both scenarios and
also reports an
error on the console in case of parsing error in keySplines attribute.

[rwlbuis, 2014-09-18 18:59:32]

@pdr PTAL

[pdr., 2014-09-18 19:19:05]

LGTM with a nit. Please add a link to the spec discussion in the change
description.

https://codereview.chromium.org/539383002/diff/20001/Source/core/svg/SVGAnima...
File Source/core/svg/SVGAnimationElement.cpp (right):

https://codereview.chromium.org/539383002/diff/20001/Source/core/svg/SVGAnima...
Source/core/svg/SVGAnimationElement.cpp:180: m_keySplines.clear();
Can you move the "m_keySplines.clear()" call into parseKeySplines? It will more
closely match the other parse* functions.

[rwlbuis, 2014-09-18 20:32:02]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-09-18 20:32:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/539383002/40001

[commit-bot: I haz the power, 2014-09-18 21:39:18]

Committed patchset #3 (id:40001) as 182272