Issue 584053002: Handle semicolons at end of keyTimes and values

Issue 584053002: Handle semicolons at end of keyTimes and values (Closed)

Created:
6 years, 3 months ago by rwlbuis

Modified:
6 years, 2 months ago

Reviewers:
pdr., Erik Dahlström (inactive), fs

CC:
blink-reviews, ed+blinkwatch_opera.com, shans, rjwright, Mike Lawther (Google), blink-reviews-animation_chromium.org, kouhei+svg_chromium.org, dstockwell, Timothy Loh, krit, f(malita), gyuyoung.kim_webkit.org, darktears, Stephen Chennney, Steve Block, pdr+svgwatchlist_chromium.org, Eric Willigers

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

Handle semicolons at end of keyTimes and values After https://codereview.chromium.org/539383002/ , make the handling of keyTimes and values consistent with keySplines. Add two tests to verify this. New behavior matches FireFox. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183274

Patch Set 1 #

Total comments: 2

Patch Set 2 : More tests and refactor into function #

Patch Set 3 : Fix unintended behavior #

Total comments: 2

Patch Set 4 : Address ed's remark #

Created: 6 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+82 lines, -14 lines)			Patch
A +	LayoutTests/svg/custom/animation-values-parsing-error.html	View		1 chunk	+1 line, -1 line	0 comments	Download
A	LayoutTests/svg/custom/animation-values-parsing-error-expected.txt	View	1	1 chunk	+12 lines, -0 lines	0 comments	Download
A +	LayoutTests/svg/custom/keyTimes-parsing-error.html	View		1 chunk	+1 line, -1 line	0 comments	Download
A	LayoutTests/svg/custom/keyTimes-parsing-error-expected.txt	View		1 chunk	+11 lines, -0 lines	0 comments	Download
A	LayoutTests/svg/custom/script-tests/animation-values-parsing-error.js	View	1 2 3	1 chunk	+12 lines, -0 lines	0 comments	Download
A	LayoutTests/svg/custom/script-tests/keyTimes-parsing-error.js	View		1 chunk	+7 lines, -0 lines	0 comments	Download
M	Source/core/svg/SVGAnimationElement.cpp	View	1 2	4 chunks	+38 lines, -12 lines	0 comments	Download

Messages

Total messages: 19 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimationElement.cpp File Source/core/svg/SVGAnimationElement.cpp (right): https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimationElement.cpp#newcode169 Source/core/svg/SVGAnimationElement.cpp:169: Vector<String> parseList; Maybe move this lot into a function ...

6 years, 3 months ago (2014-09-22 10:26:29 UTC) #4

Erik Dahlström (inactive)

On 2014/09/22 10:26:29, fs wrote: > https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimationElement.cpp > File Source/core/svg/SVGAnimationElement.cpp (right): > > https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimationElement.cpp#newcode169 > ...

6 years, 3 months ago (2014-09-22 10:47:06 UTC) #5

rwlbuis

6 years, 3 months ago (2014-09-22 15:12:56 UTC) #6

rwlbuis

On 2014/09/22 10:47:06, Erik Dahlström wrote: > On 2014/09/22 10:26:29, fs wrote: > > > ...

6 years, 3 months ago (2014-09-22 15:15:13 UTC) #7

On 2014/09/22 15:15:13, rwlbuis wrote: > On 2014/09/22 10:47:06, Erik Dahlström wrote: > > On ...

6 years, 3 months ago (2014-09-22 15:40:10 UTC) #8

rwlbuis

On 2014/09/22 15:40:10, fs wrote: > On 2014/09/22 15:15:13, rwlbuis wrote: > > On 2014/09/22 ...

6 years, 2 months ago (2014-09-26 14:58:20 UTC) #9

On 2014/09/22 15:40:10, fs wrote:
> On 2014/09/22 15:15:13, rwlbuis wrote:
> > On 2014/09/22 10:47:06, Erik Dahlström wrote:
> > > On 2014/09/22 10:26:29, fs wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > File Source/core/svg/SVGAnimationElement.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > Source/core/svg/SVGAnimationElement.cpp:169: Vector<String> parseList;
> > > > Maybe move this lot into a function now that it's largeish?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > Source/core/svg/SVGAnimationElement.cpp:178:
> > > > reportAttributeParsingError(ParsingAttributeFailedError, name, value);
> > > > Please add a test for a 'values' for an attribute that accept strings
(is
> > > > SVGAnimatedString) and may accept the empty string (xlink:href, target,
> > > result,
> > > > in. etc.)
> > > 
> > > It would also be interesting with a test for something like
> > > values="50%;invalid-value;10%", since I know that this was handled
> differently
> > > in firefox (disregards the entire attribute if there's an error anywhere
in
> > the
> > > list). Also a test for values="50%;;;;10%" would be good.
> > 
> > I added tests for both. However right now the invalid-value case is hard to
> > detect as we
> > simply parse into Strings, and only know later if the strings should
actually
> be
> > interpreted as
> > floats/lengths (say with attributeName="width"). I think this may need a
> bigger
> > patch and
> > I propose to make that a separate bug.
> 
> Sounds reasonable given that it shouldn't be a change in behavior since
> previously |allowEmptyEntries| was 'false'. It seems there might still be
> "unintended" change in behavior for cases like "...; ;..." - which would still
> leave " " previously, and then be stripped to "".

Ok, so is there anything left to do for this patch?

On 2014/09/26 14:58:20, rwlbuis wrote: > On 2014/09/22 15:40:10, fs wrote: > > On 2014/09/22 ...

6 years, 2 months ago (2014-09-26 15:10:24 UTC) #10

On 2014/09/26 14:58:20, rwlbuis wrote:
> On 2014/09/22 15:40:10, fs wrote:
> > On 2014/09/22 15:15:13, rwlbuis wrote:
> > > On 2014/09/22 10:47:06, Erik Dahlström wrote:
> > > > On 2014/09/22 10:26:29, fs wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > > File Source/core/svg/SVGAnimationElement.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > > Source/core/svg/SVGAnimationElement.cpp:169: Vector<String> parseList;
> > > > > Maybe move this lot into a function now that it's largeish?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > > Source/core/svg/SVGAnimationElement.cpp:178:
> > > > > reportAttributeParsingError(ParsingAttributeFailedError, name, value);
> > > > > Please add a test for a 'values' for an attribute that accept strings
> (is
> > > > > SVGAnimatedString) and may accept the empty string (xlink:href,
target,
> > > > result,
> > > > > in. etc.)
> > > > 
> > > > It would also be interesting with a test for something like
> > > > values="50%;invalid-value;10%", since I know that this was handled
> > differently
> > > > in firefox (disregards the entire attribute if there's an error anywhere
> in
> > > the
> > > > list). Also a test for values="50%;;;;10%" would be good.
> > > 
> > > I added tests for both. However right now the invalid-value case is hard
to
> > > detect as we
> > > simply parse into Strings, and only know later if the strings should
> actually
> > be
> > > interpreted as
> > > floats/lengths (say with attributeName="width"). I think this may need a
> > bigger
> > > patch and
> > > I propose to make that a separate bug.
> > 
> > Sounds reasonable given that it shouldn't be a change in behavior since
> > previously |allowEmptyEntries| was 'false'. It seems there might still be
> > "unintended" change in behavior for cases like "...; ;..." - which would
still
> > leave " " previously, and then be stripped to "".
> 
> Ok, so is there anything left to do for this patch?

Restore the "unintended" bit mentioned above? (~= allow whitespace to mean empty
and be accepted.)

rwlbuis

On 2014/09/26 15:10:24, fs wrote: > On 2014/09/26 14:58:20, rwlbuis wrote: > > On 2014/09/22 ...

6 years, 2 months ago (2014-09-26 15:48:25 UTC) #11

On 2014/09/26 15:10:24, fs wrote:
> On 2014/09/26 14:58:20, rwlbuis wrote:
> > On 2014/09/22 15:40:10, fs wrote:
> > > On 2014/09/22 15:15:13, rwlbuis wrote:
> > > > On 2014/09/22 10:47:06, Erik Dahlström wrote:
> > > > > On 2014/09/22 10:26:29, fs wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > > > File Source/core/svg/SVGAnimationElement.cpp (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > > > Source/core/svg/SVGAnimationElement.cpp:169: Vector<String>
parseList;
> > > > > > Maybe move this lot into a function now that it's largeish?
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/584053002/diff/1/Source/core/svg/SVGAnimation...
> > > > > > Source/core/svg/SVGAnimationElement.cpp:178:
> > > > > > reportAttributeParsingError(ParsingAttributeFailedError, name,
value);
> > > > > > Please add a test for a 'values' for an attribute that accept
strings
> > (is
> > > > > > SVGAnimatedString) and may accept the empty string (xlink:href,
> target,
> > > > > result,
> > > > > > in. etc.)
> > > > > 
> > > > > It would also be interesting with a test for something like
> > > > > values="50%;invalid-value;10%", since I know that this was handled
> > > differently
> > > > > in firefox (disregards the entire attribute if there's an error
anywhere
> > in
> > > > the
> > > > > list). Also a test for values="50%;;;;10%" would be good.
> > > > 
> > > > I added tests for both. However right now the invalid-value case is hard
> to
> > > > detect as we
> > > > simply parse into Strings, and only know later if the strings should
> > actually
> > > be
> > > > interpreted as
> > > > floats/lengths (say with attributeName="width"). I think this may need a
> > > bigger
> > > > patch and
> > > > I propose to make that a separate bug.
> > > 
> > > Sounds reasonable given that it shouldn't be a change in behavior since
> > > previously |allowEmptyEntries| was 'false'. It seems there might still be
> > > "unintended" change in behavior for cases like "...; ;..." - which would
> still
> > > leave " " previously, and then be stripped to "".
> > 
> > Ok, so is there anything left to do for this patch?
> 
> Restore the "unintended" bit mentioned above? (~= allow whitespace to mean
empty
> and be accepted.)

Ah! I'll upload a new patch that should fix this, hopefully it will turn up
green...

Erik Dahlström (inactive)

https://codereview.chromium.org/584053002/diff/40001/LayoutTests/svg/custom/script-tests/animation-values-parsing-error.js File LayoutTests/svg/custom/script-tests/animation-values-parsing-error.js (right): https://codereview.chromium.org/584053002/diff/40001/LayoutTests/svg/custom/script-tests/animation-values-parsing-error.js#newcode9 LayoutTests/svg/custom/script-tests/animation-values-parsing-error.js:9: animate.setAttribute("animationName", "in"); should be "attributeName", not "animationName".

6 years, 2 months ago (2014-10-06 07:33:53 UTC) #13

LGTM w/ ed's issue fixed. https://codereview.chromium.org/584053002/diff/40001/Source/core/svg/SVGAnimationElement.cpp File Source/core/svg/SVGAnimationElement.cpp (right): https://codereview.chromium.org/584053002/diff/40001/Source/core/svg/SVGAnimationElement.cpp#newcode68 Source/core/svg/SVGAnimationElement.cpp:68: goto fail; I think ...

6 years, 2 months ago (2014-10-06 09:02:33 UTC) #14

rwlbuis

On 2014/10/06 09:02:33, fs wrote: > LGTM w/ ed's issue fixed. > > https://codereview.chromium.org/584053002/diff/40001/Source/core/svg/SVGAnimationElement.cpp > ...

6 years, 2 months ago (2014-10-06 14:38:31 UTC) #15

rwlbuis

On 2014/10/06 07:33:53, Erik Dahlström wrote: > https://codereview.chromium.org/584053002/diff/40001/LayoutTests/svg/custom/script-tests/animation-values-parsing-error.js > File LayoutTests/svg/custom/script-tests/animation-values-parsing-error.js > (right): > > ...

6 years, 2 months ago (2014-10-06 14:40:42 UTC) #16

commit-bot: I haz the power

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

6 years, 2 months ago (2014-10-06 14:40:59 UTC) #18

commit-bot: I haz the power

6 years, 2 months ago (2014-10-06 15:50:53 UTC) #19

Message was sent while issue was closed.

Committed patchset #4 (id:60001) as 183274

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages