SVG animateMotion paths that only cause offsets are no longer ignored

SVG animateMotion paths that only cause offsets are no longer ignored

Chrome was not applying animateMotion paths that do not actually express
motion.

Similarly, motion-path paths like "M 100 100 L 100 100" were not being
applied.

The SkPathMeasure constructed from such paths is empty, but we can
directly read the starting point from the path.

BUG=321022
Committed: https://crrev.com/4e4d176ad83910be4002bd740caeb83a904371ed
Cr-Commit-Position: refs/heads/master@{#361061}

[Eric Willigers, 2015-10-29 11:34:53]

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

[fs, 2015-10-29 11:48:13]

LGTM, but you'll need someone to approve the platform/ change as well.

[Eric Willigers, 2015-10-29 20:20:32]

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

[Eric Willigers, 2015-10-29 20:20:32]

+pdr for platform

[pdr., 2015-10-30 00:52:05]

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/css3/motion-path/path-establishes-stacking-context.html
(right):

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/css3/motion-path/path-establishes-stacking-context.html:6:
div {
Was this needed?

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1007:
motionPath.path().pointAndNormalAtLength(computedDistance, point, angle);
There are only a few callers of these functions and with this change they pretty
much all ignore the "ok" param. Can we just remove "ok" entirely and update the
callsites?

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGAnimateMotionElement.cpp (right):

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGAnimateMotionElement.cpp:261: FloatPoint
positionAtEndOfDuration =
m_animationPath.pointAtLength(m_animationPath.length(), ok);
pointAtLength just calls pointAndNormalAtLength internally. Does this need to be
updated to handle the "M 100 100 L 100 100" case too?

[Eric Willigers, 2015-11-20 07:49:53]

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/css3/motion-path/path-establishes-stacking-context.html
(right):

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/css3/motion-path/path-establishes-stacking-context.html:6:
div {
On 2015/10/30 00:52:04, pdr wrote:
> Was this needed?

With parent width 800px, the default transform-origin 50% is 400px. So 'M400,0'
maintains position.

This is a consequence of
https://lists.w3.org/Archives/Public/public-fx/2015AprJun/0077.html
https://codereview.chromium.org/1220793011

Without the present change, the path 'M0,0' was being ignored. Now that it is
not being ignored, 'M0,0' causes a translate unless the parent width is 0px (and
it isn't, so the test starts failing unless we make some change).

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1007:
motionPath.path().pointAndNormalAtLength(computedDistance, point, angle);
On 2015/10/30 00:52:05, pdr wrote:
> There are only a few callers of these functions and with this change they
pretty
> much all ignore the "ok" param. Can we just remove "ok" entirely and update
the
> callsites?

Done.

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGAnimateMotionElement.cpp (right):

https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGAnimateMotionElement.cpp:261: FloatPoint
positionAtEndOfDuration =
m_animationPath.pointAtLength(m_animationPath.length(), ok);
On 2015/10/30 00:52:05, pdr wrote:
> pointAtLength just calls pointAndNormalAtLength internally. Does this need to
be
> updated to handle the "M 100 100 L 100 100" case too?

Updated.

animateMotion-still.html now tests this case when time is 2s,3s
(repeatCount==1).

[pdr., 2015-11-20 22:38:00]

On 2015/11/20 at 07:49:53, ericwilligers wrote:
>
https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/css3/motion-path/path-establishes-stacking-context.html
(right):
> 
>
https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/css3/motion-path/path-establishes-stacking-context.html:6:
div {
> On 2015/10/30 00:52:04, pdr wrote:
> > Was this needed?
> 
> With parent width 800px, the default transform-origin 50% is 400px. So
'M400,0' maintains position.
> 
> This is a consequence of
> https://lists.w3.org/Archives/Public/public-fx/2015AprJun/0077.html
> https://codereview.chromium.org/1220793011
> 
> Without the present change, the path 'M0,0' was being ignored. Now that it is
not being ignored, 'M0,0' causes a translate unless the parent width is 0px (and
it isn't, so the test starts failing unless we make some change).
> 
>
https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):
> 
>
https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/style/ComputedStyle.cpp:1007:
motionPath.path().pointAndNormalAtLength(computedDistance, point, angle);
> On 2015/10/30 00:52:05, pdr wrote:
> > There are only a few callers of these functions and with this change they
pretty
> > much all ignore the "ok" param. Can we just remove "ok" entirely and update
the
> > callsites?
> 
> Done.
> 
>
https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGAnimateMotionElement.cpp (right):
> 
>
https://codereview.chromium.org/1418663004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGAnimateMotionElement.cpp:261: FloatPoint
positionAtEndOfDuration =
m_animationPath.pointAtLength(m_animationPath.length(), ok);
> On 2015/10/30 00:52:05, pdr wrote:
> > pointAtLength just calls pointAndNormalAtLength internally. Does this need
to be
> > updated to handle the "M 100 100 L 100 100" case too?
> 
> Updated.
> 
> animateMotion-still.html now tests this case when time is 2s,3s
(repeatCount==1).

LGTM!

[Eric Willigers, 2015-11-22 15:05:16]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2015-11-22 15:05:17]

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/1418663004/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-11-22 15:05:23]

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

[commit-bot: I haz the power, 2015-11-22 16:16:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-22 16:16:41]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Eric Willigers, 2015-11-22 23:08:53]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2015-11-22 23:08:58]

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

[commit-bot: I haz the power, 2015-11-23 00:30:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-23 00:31:44]

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