CSS animation for SVG presentation attribute 'd'

CSS animation for SVG presentation attribute 'd'

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821

BUG=535429
Committed: https://crrev.com/5994f089fd585f4ac9629b3f52db05044db55592
Cr-Commit-Position: refs/heads/master@{#365459}

[Eric Willigers, 2015-11-26 05:28:59]

ericwilligers@chromium.org changed reviewers:
+ alancutter@chromium.org, pdr@chromium.org

[Eric Willigers, 2015-11-26 05:28:59]

Builds on https://codereview.chromium.org/1439793003/

[dstockwell, 2015-11-26 08:38:39]

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

[dstockwell, 2015-11-26 08:38:40]

https://codereview.chromium.org/1473963004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp
(right):

https://codereview.chromium.org/1473963004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp:55:
SVGPathByteStreamSource toSource(*toAnimatablePath(value)->m_byteStream);
Is there a representation of paths which is not encoded as a byte stream? This
sounds expensive.

Are we planning to migrate to the InterpolationType in
https://codereview.chromium.org/1441853002/ ?

[fs, 2015-11-26 11:21:38]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2015-11-26 11:21:38]

https://codereview.chromium.org/1473963004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp
(right):

https://codereview.chromium.org/1473963004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp:55:
SVGPathByteStreamSource toSource(*toAnimatablePath(value)->m_byteStream);
On 2015/11/26 at 08:38:40, dstockwell wrote:
> Is there a representation of paths which is not encoded as a byte stream? This
sounds expensive.

I believe it's the cheapest representation that retains full semantics of the
path (the other option is 'String', but that has a higher cost since it needs to
string-to-float a lot.) It's not terribly expensive though since it's a fairly
straight memcpy.
 
> Are we planning to migrate to the InterpolationType in
https://codereview.chromium.org/1441853002/ ?

That'd be good. Needs to untie from SVGPath (SVGPropertyBAse) though for that.

[Eric Willigers, 2015-11-26 21:16:33]

https://codereview.chromium.org/1473963004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp
(right):

https://codereview.chromium.org/1473963004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp:55:
SVGPathByteStreamSource toSource(*toAnimatablePath(value)->m_byteStream);
On 2015/11/26 08:38:40, dstockwell wrote:
> 
> Are we planning to migrate to the InterpolationType in
> https://codereview.chromium.org/1441853002/ ?

We will have an InterpolationType. It will fix this CL's failing expectations in
d-responsive-expected.txt
But I think we always have distinct InterpolationType classes for CSS and SVG.
Alan? 

I understand we still need this AnimatableValue for transitions.

[alancutter (OOO until 2018), 2015-11-26 22:42:52]

On 2015/11/26 at 21:16:33, ericwilligers wrote:
>
https://codereview.chromium.org/1473963004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp
(right):
> 
>
https://codereview.chromium.org/1473963004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp:55:
SVGPathByteStreamSource toSource(*toAnimatablePath(value)->m_byteStream);
> On 2015/11/26 08:38:40, dstockwell wrote:
> > 
> > Are we planning to migrate to the InterpolationType in
> > https://codereview.chromium.org/1441853002/ ?
> 
> We will have an InterpolationType. It will fix this CL's failing expectations
in d-responsive-expected.txt
> But I think we always have distinct InterpolationType classes for CSS and SVG.
Alan? 

Correct, CSS will need its own InterpolationType class though we should aim to
share the core interpolation logic between them and only differ in binding with
SVG/CSSValues.

> I understand we still need this AnimatableValue for transitions.

AnimatablePath will still be required for transitions though it can probably be
built upon the CSSPathInterpolationType to avoid tons of logic duplication,
additionally I suspect this would start paving the way for removing
AnimatableValues altogether.

[Eric Willigers, 2015-12-15 08:24:04]

Patchset #2 (id:20001) has been deleted

[Eric Willigers, 2015-12-15 08:47:32]

> > I understand we still need this AnimatableValue for transitions.
> 
> AnimatablePath will still be required for transitions though it can probably
be
> built upon the CSSPathInterpolationType to avoid tons of logic duplication,
> additionally I suspect this would start paving the way for removing
> AnimatableValues altogether.

This part of the discussion (moving transitions functionality out of
AnimatableValue) is outside of scope for 'd'.

PTAL

[fs, 2015-12-15 11:52:53]

LGTM w/ nits

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp
(right):

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp:29: if
(fromSeg.command == PathSegUnknown || toSeg.command == PathSegUnknown)
'PathSegUnknown' should never be stored in an SVGPathByteStream, so this is dead
code. (This would hit an ASSERT_NOT_REACHED in
SVGPathByteStreamSource::parseSegment, so that bit should be covered too.)

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp:45:
byteStream = SVGPathByteStream::create();
These two lines could be merged.

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/animatable/AnimatablePath.h
(right):

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.h:17: static
PassRefPtr<AnimatablePath> create(PassRefPtr<CSSPathValue> pathValue)
PassRefPtrWillBeRawPtr I think. Same for all the other cases below.

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
(right):

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp:61:
#include "core/svg/SVGPathUtilities.h"
This doesn't appear to be used here?

[Eric Willigers, 2015-12-15 22:33:55]

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp
(right):

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp:29: if
(fromSeg.command == PathSegUnknown || toSeg.command == PathSegUnknown)
On 2015/12/15 11:52:52, fs wrote:
> 'PathSegUnknown' should never be stored in an SVGPathByteStream, so this is
dead
> code. (This would hit an ASSERT_NOT_REACHED in
> SVGPathByteStreamSource::parseSegment, so that bit should be covered too.)

Replaced with assert.

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp:45:
byteStream = SVGPathByteStream::create();
On 2015/12/15 11:52:52, fs wrote:
> These two lines could be merged.

Done.

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/animatable/AnimatablePath.h
(right):

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/animatable/AnimatablePath.h:17: static
PassRefPtr<AnimatablePath> create(PassRefPtr<CSSPathValue> pathValue)
On 2015/12/15 11:52:53, fs wrote:
> PassRefPtrWillBeRawPtr I think. Same for all the other cases below.

Done.

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
(right):

https://codereview.chromium.org/1473963004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp:61:
#include "core/svg/SVGPathUtilities.h"
On 2015/12/15 11:52:53, fs wrote:
> This doesn't appear to be used here?

Removed.

[Eric Willigers, 2015-12-16 02:50:38]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2015-12-16 02:50:39]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1473963004/#ps80001
(title: "Oilpan")

[commit-bot: I haz the power, 2015-12-16 02:53:29]

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

[commit-bot: I haz the power, 2015-12-16 05:13:35]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-12-16 05:14:38]

Description was changed from

==========
CSS animation for SVG presentation attribute 'd'

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821

BUG=535429
==========

to

==========
CSS animation for SVG presentation attribute 'd'

See SVG Working Group Action
http://www.w3.org/Graphics/SVG/WG/track/actions/3821

BUG=535429

Committed: https://crrev.com/5994f089fd585f4ac9629b3f52db05044db55592
Cr-Commit-Position: refs/heads/master@{#365459}
==========

[commit-bot: I haz the power, 2015-12-16 05:14:39]

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