CSS Motion Path: Support offset-rotate in addition to offset-rotation

CSS Motion Path: Support offset-rotate in addition to offset-rotation

offset-rotation is being renamed to offset-rotate, but we will already have shipped offset-rotation.

We 'alias' offset-rotate to offset-rotation, sharing storage in ComputedStyle.

Spec discussion:
https://github.com/w3c/fxtf-drafts/issues/70

Design Docs:
https://goo.gl/XFvGR8
https://docs.google.com/document/d/1AHSyz3_Qu8hZK5czcQdfucVanoxpsUkBbzY16ti4_NY/edit?usp=sharing

BUG=671333
Committed: https://crrev.com/af51f3582ab7798d8a58628c6975f7c08df8c8a2
Cr-Commit-Position: refs/heads/master@{#436835}

[Eric Willigers, 2016-12-05 20:09:04]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[Eric Willigers, 2016-12-05 20:09:21]

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

[commit-bot: I haz the power, 2016-12-05 20:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Willigers, 2016-12-05 20:11:47]

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1053: case
CSSPropertyOffsetRotate:
This CL is intended to be merged to M56. We don't add a use counter in this CL,
to keep the merge simple.

A follow-up CL will add a use counter.

[commit-bot: I haz the power, 2016-12-05 20:22:06]

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

[commit-bot: I haz the power, 2016-12-05 20:22:07]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eric Willigers, 2016-12-06 00:51:40]

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

[sashab, 2016-12-06 04:16:34]

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/composition/offset-rotate-composition.html
(right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/composition/offset-rotate-composition.html:5:
assertComposition({
Maybe add one-line comments above each of these explaining what they are
testing.

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/interpolation/offset-rotate-interpolation.html
(right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/interpolation/offset-rotate-interpolation.html:23:
assertInterpolation({
Ditto here.

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/responsive/offset-rotate-responsive.html
(right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/responsive/offset-rotate-responsive.html:4:
assertCSSResponsive({
and here :)

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/css3/motion-path/offset-rotate.html (right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/css3/motion-path/offset-rotate.html:7: #div2 {
Is there a reason you don't want to use inline styles? It would make it easier
to follow the logic :)

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/animation/CSSOffsetRotationInterpolationType.h
(left):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSOffsetRotationInterpolationType.h:16:
DCHECK_EQ(cssProperty(), CSSPropertyOffsetRotation);
Do you want DCHECK(property == offsetrotation || property == offsetrotate)?

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1053: case
CSSPropertyOffsetRotate:
On 2016/12/05 at 20:11:47, Eric Willigers wrote:
> This CL is intended to be merged to M56. We don't add a use counter in this
CL, to keep the merge simple.
> 
> A follow-up CL will add a use counter.

SGTM, please add a TODO.

[Eric Willigers, 2016-12-06 05:30:05]

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/composition/offset-rotate-composition.html
(right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/composition/offset-rotate-composition.html:5:
assertComposition({
On 2016/12/06 04:16:34, sashab wrote:
> Maybe add one-line comments above each of these explaining what they are
> testing.

Done.

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/interpolation/offset-rotate-interpolation.html
(right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/interpolation/offset-rotate-interpolation.html:23:
assertInterpolation({
On 2016/12/06 04:16:34, sashab wrote:
> Ditto here.

Done.

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/responsive/offset-rotate-responsive.html
(right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/responsive/offset-rotate-responsive.html:4:
assertCSSResponsive({
On 2016/12/06 04:16:34, sashab wrote:
> and here :)

Done.

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/css3/motion-path/offset-rotate.html (right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/css3/motion-path/offset-rotate.html:7: #div2 {
On 2016/12/06 04:16:34, sashab wrote:
> Is there a reason you don't want to use inline styles? It would make it easier
> to follow the logic :)

Done.

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/animation/CSSOffsetRotationInterpolationType.h
(left):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSOffsetRotationInterpolationType.h:16:
DCHECK_EQ(cssProperty(), CSSPropertyOffsetRotation);
On 2016/12/06 04:16:34, sashab wrote:
> Do you want DCHECK(property == offsetrotation || property == offsetrotate)?

Done.

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2555543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1053: case
CSSPropertyOffsetRotate:
On 2016/12/06 04:16:34, sashab wrote:
> On 2016/12/05 at 20:11:47, Eric Willigers wrote:
> > This CL is intended to be merged to M56. We don't add a use counter in this
> CL, to keep the merge simple.
> > 
> > A follow-up CL will add a use counter.
> 
> SGTM, please add a TODO.

Done.

[sashab, 2016-12-06 05:55:18]

Nice, looking much better!

Just one more thing... Have you looked at the way we do existing property
aliases? Do any of them suit your needs so you don't have to add all this code?
timloh@ might know more

lgtm from me assuming this is the best way to do things

[Eric Willigers, 2016-12-06 06:06:46]

On 2016/12/06 05:55:18, sashab wrote:
> Nice, looking much better!
> 
> Just one more thing... Have you looked at the way we do existing property
> aliases? Do any of them suit your needs so you don't have to add all this
code?
> timloh@ might know more
> 
> lgtm from me assuming this is the best way to do things

The existing approach for aliases can't be reused as we already using it for
motion-rotation.

Discussed in design doc
https://docs.google.com/document/d/1AHSyz3_Qu8hZK5czcQdfucVanoxpsUkBbzY16ti4_...

[Eric Willigers, 2016-12-06 06:13:20]

ericwilligers@chromium.org changed reviewers:
+ rbyers@chromium.org
- timloh@chromium.org

[Eric Willigers, 2016-12-06 06:13:21]

+rbyers@ for API owners

[Rick Byers, 2016-12-06 16:30:42]

webexposed LGTM
Should we send deprecation warnings for offset-rotation saying to use
offset-rotate instead now too?

https://codereview.chromium.org/2555543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2555543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1055: //
TODO(ericwilligers): Distinct use counter for CSSPropertyOffsetRotate.
Might as well do this now, it's trivial right?  Changing the meaning of UMA data
over time makes analysis more confusing...

[Eric Willigers, 2016-12-06 18:43:12]

https://codereview.chromium.org/2555543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2555543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1055: //
TODO(ericwilligers): Distinct use counter for CSSPropertyOffsetRotate.
On 2016/12/06 16:30:42, Rick Byers wrote:
> Might as well do this now, it's trivial right?  Changing the meaning of UMA
data
> over time makes analysis more confusing...

The intent is for this patch to be merged to M56 (design doc
https://goo.gl/XFvGR8 ).


We would need to assign use count 1692, and renumber CSSFlexibleBox ...

 
    CSSShadowPiercingDescendantCombinator = 1691,
    // The above items are available in M56 branch.

    CSSFlexibleBox = 1692,
    CSSGridLayout = 1693,


Thus I attempted to instead keep this CL as simple as possible. The design doc
proposes deprecation messages in M 57.

[Rick Byers, 2016-12-06 19:01:35]

On 2016/12/06 18:43:12, Eric Willigers wrote:
>
https://codereview.chromium.org/2555543002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):
> 
>
https://codereview.chromium.org/2555543002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/UseCounter.cpp:1055: //
> TODO(ericwilligers): Distinct use counter for CSSPropertyOffsetRotate.
> On 2016/12/06 16:30:42, Rick Byers wrote:
> > Might as well do this now, it's trivial right?  Changing the meaning of UMA
> data
> > over time makes analysis more confusing...
> 
> The intent is for this patch to be merged to M56 (design doc
> https://goo.gl/XFvGR8 ).
> 
> 
> We would need to assign use count 1692, and renumber CSSFlexibleBox ...
> 
>  
>     CSSShadowPiercingDescendantCombinator = 1691,
>     // The above items are available in M56 branch.
> 
>     CSSFlexibleBox = 1692,
>     CSSGridLayout = 1693,
> 
> 
> Thus I attempted to instead keep this CL as simple as possible. The design doc
> proposes deprecation messages in M 57.

Ok, sounds fine to me.  Thanks for the details.

[Eric Willigers, 2016-12-06 19:23:14]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2016-12-06 19:23:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 19:36:19]

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

[commit-bot: I haz the power, 2016-12-06 19:36:21]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eric Willigers, 2016-12-06 21:52:17]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 21:53:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Willigers, 2016-12-07 00:17:37]

The CQ bit was unchecked by ericwilligers@chromium.org

[Eric Willigers, 2016-12-07 00:17:42]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2016-12-07 00:17:43]

The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2555543002/#ps40001
(title: "reformat")

[commit-bot: I haz the power, 2016-12-07 00:18:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 02:09:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481069862651660,
"parent_rev": "ad49bf792dc2b794f156f0ce410c3a34388459ad", "commit_rev":
"b065d626844c9a61b8fdf1191a6e52c8db7f89cf"}

[commit-bot: I haz the power, 2016-12-07 02:09:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-07 02:13:15]

Description was changed from

==========
CSS Motion Path: Support offset-rotate in addition to offset-rotation

offset-rotation is being renamed to offset-rotate, but we will already have
shipped offset-rotation.

We 'alias' offset-rotate to offset-rotation, sharing storage in ComputedStyle.

Spec discussion:
https://github.com/w3c/fxtf-drafts/issues/70

Design Docs:
https://goo.gl/XFvGR8
https://docs.google.com/document/d/1AHSyz3_Qu8hZK5czcQdfucVanoxpsUkBbzY16ti4_...

BUG=671333
==========

to

==========
CSS Motion Path: Support offset-rotate in addition to offset-rotation

offset-rotation is being renamed to offset-rotate, but we will already have
shipped offset-rotation.

We 'alias' offset-rotate to offset-rotation, sharing storage in ComputedStyle.

Spec discussion:
https://github.com/w3c/fxtf-drafts/issues/70

Design Docs:
https://goo.gl/XFvGR8
https://docs.google.com/document/d/1AHSyz3_Qu8hZK5czcQdfucVanoxpsUkBbzY16ti4_...

BUG=671333

Committed: https://crrev.com/af51f3582ab7798d8a58628c6975f7c08df8c8a2
Cr-Commit-Position: refs/heads/master@{#436835}
==========

[commit-bot: I haz the power, 2016-12-07 02:13:16]

Patchset 3 (id:??) landed as
https://crrev.com/af51f3582ab7798d8a58628c6975f7c08df8c8a2
Cr-Commit-Position: refs/heads/master@{#436835}