Use Length for the stroke-dasharray property in SVGLayoutStyle

Use Length for the stroke-dasharray property in SVGLayoutStyle

This replaces the SVGLengthList used for strokeDashArray in
SVGLayoutStyle with a (reference-counted) Vector of Lengths.
SVGStrokeDasharrayStyleInterpolation is updated to treat a dash array in
a similar fashion, by having a list of LengthStyleInterpolations.
The storage of types in SVGStrokeDasharrayStyleInterpolation is dropped
for the same reason. AnimatableStrokeDasharrayList is updated to use
AnimatableLengths instead of AnimatableSVGLengths.

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

[fs, 2015-03-03 16:28:34]

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

[fs, 2015-03-03 19:57:17]

With this, SVGStrokeDasharrayStyleInterpolation almost fits into the
ListStyleInterpolation<...> mold.

PTAL

[pdr., 2015-03-03 21:08:22]

https://codereview.chromium.org/975733002/diff/1/LayoutTests/animations/inter...
File
LayoutTests/animations/interpolation/svg-stroke-dasharray-interpolation-expected.txt
(right):

https://codereview.chromium.org/975733002/diff/1/LayoutTests/animations/inter...
LayoutTests/animations/interpolation/svg-stroke-dasharray-interpolation-expected.txt:101:
FAIL: stroke-dasharray from [0px 20px] to [40px 0px 60em] was [0px, 20px] at 0,
expected [0px 20px 0px 20px 0px 20px] (parsed as [0px, 20px, 0px, 20px, 0px,
20px])
Could you update this to not be failing?

https://codereview.chromium.org/975733002/diff/1/Source/core/animation/SVGStr...
File Source/core/animation/SVGStrokeDasharrayStyleInterpolation.cpp (left):

https://codereview.chromium.org/975733002/diff/1/Source/core/animation/SVGStr...
Source/core/animation/SVGStrokeDasharrayStyleInterpolation.cpp:76: if (types[i]
== CSSPrimitiveValue::CSS_UNKNOWN)
Where did this check go?

[Eric Willigers, 2015-03-04 02:21:13]

lgtm

Add a test
LayoutTests/web-animations-api/animations-responsive-strokeDasharray.html to
show that stroke-dasharray animations are responsive to font size changes.

https://codereview.chromium.org/975733002/diff/1/Source/core/animation/SVGStr...
File Source/core/animation/SVGStrokeDasharrayStyleInterpolation.cpp (left):

https://codereview.chromium.org/975733002/diff/1/Source/core/animation/SVGStr...
Source/core/animation/SVGStrokeDasharrayStyleInterpolation.cpp:76: if (types[i]
== CSSPrimitiveValue::CSS_UNKNOWN)
On 2015/03/03 21:08:22, pdr wrote:
> Where did this check go?

canCreateFrom calls have already ensured we have two lists of lengths.

LengthStyleInterpolation essentially interpolates calc expressions, we no longer
need to ensure the lengths have a common unit type.

[fs, 2015-03-04 09:49:31]

On 2015/03/04 02:21:13, Eric Willigers wrote:
> Add a test
> LayoutTests/web-animations-api/animations-responsive-strokeDasharray.html to
> show that stroke-dasharray animations are responsive to font size changes.

Done.

https://codereview.chromium.org/975733002/diff/1/LayoutTests/animations/inter...
File
LayoutTests/animations/interpolation/svg-stroke-dasharray-interpolation-expected.txt
(right):

https://codereview.chromium.org/975733002/diff/1/LayoutTests/animations/inter...
LayoutTests/animations/interpolation/svg-stroke-dasharray-interpolation-expected.txt:101:
FAIL: stroke-dasharray from [0px 20px] to [40px 0px 60em] was [0px, 20px] at 0,
expected [0px 20px 0px 20px 0px 20px] (parsed as [0px, 20px, 0px, 20px, 0px,
20px])
On 2015/03/03 21:08:21, pdr wrote:
> Could you update this to not be failing?

Not without getting a fail in the test that uses the Web Animations API (see
below). IIUC this sort of a shortcoming of the framework since there's two cases
but only one expectation. FWIW this is technically a pass (the value produces an
equivalent result - i.e. the rendering will be correct), it's just that the
animation code will just use the value as is for 0 and 1, so in this case when
the lengths differ you get this result (no other list-type has this "smallest
common multiple" behavior).

[fs, 2015-03-05 16:17:15]

pdr?

[pdr., 2015-03-05 21:36:00]

LGTM

https://codereview.chromium.org/975733002/diff/20001/Source/core/html/forms/I...
File Source/core/html/forms/InputType.cpp (right):

https://codereview.chromium.org/975733002/diff/20001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:35: #include "core/dom/ExceptionCode.h"
Nit: were these ExceptionCode.h's needed?

https://codereview.chromium.org/975733002/diff/20001/Source/core/layout/svg/S...
File Source/core/layout/svg/SVGLayoutSupport.cpp (right):

https://codereview.chromium.org/975733002/diff/20001/Source/core/layout/svg/S...
Source/core/layout/svg/SVGLayoutSupport.cpp:372: DashArray dashArray =
resolveSVGDashArray(*svgStyle.strokeDashArray(), style, lengthContext);
Can we save an unnecessary copy by using a const ref here?

[fs, 2015-03-06 11:27:46]

https://codereview.chromium.org/975733002/diff/20001/Source/core/html/forms/I...
File Source/core/html/forms/InputType.cpp (right):

https://codereview.chromium.org/975733002/diff/20001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:35: #include "core/dom/ExceptionCode.h"
On 2015/03/05 21:35:59, pdr wrote:
> Nit: were these ExceptionCode.h's needed?

Yes, there are various ExceptionState::throwDOMException calls here which
apparently got this included via SVGLengthList.h [1] in SVGLayoutStyleDefs.h
(ouch!?). But - like you - I was a bit surprised (amazed even) to see compile
errors pop up from that removal... (my favorite was the Worker.cpp one)

[1] which includes SVGListPropertyHelper.h which in turn includes
ExceptionCode.h

https://codereview.chromium.org/975733002/diff/20001/Source/core/layout/svg/S...
File Source/core/layout/svg/SVGLayoutSupport.cpp (right):

https://codereview.chromium.org/975733002/diff/20001/Source/core/layout/svg/S...
Source/core/layout/svg/SVGLayoutSupport.cpp:372: DashArray dashArray =
resolveSVGDashArray(*svgStyle.strokeDashArray(), style, lengthContext);
On 2015/03/05 21:35:59, pdr wrote:
> Can we save an unnecessary copy by using a const ref here?

There's no unnecessary copy here - for the instances in this compilation unit it
appears to just be inlined, and in the other callsite it's RVO'd.

[fs, 2015-03-06 11:27:55]

The CQ bit was checked by fs@opera.com

[fs, 2015-03-06 11:27:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
ericwilligers@chromium.org
Link to the patchset: https://codereview.chromium.org/975733002/#ps20001 (title:
"Add test for style-change responsive-ness.")

[commit-bot: I haz the power, 2015-03-06 11:28:27]

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

[commit-bot: I haz the power, 2015-03-06 12:49:56]

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