1456394 - Remove *ReadOnly and AnimationEffectTiming animation interfaces

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P3)

Description

[Brian Birtles (:birtles)]

We need to update our effect API to match the spec changes here:

  https://github.com/w3c/csswg-drafts/pull/2432

The WPT tests for this have already been merged into mozilla-central (bug 1445877).

Comment 1

[Brian Birtles (:birtles)]

Here's a roughly complete patch queue (although most of the commits are hidden by treeherder):

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=344071a296fb9023eb33f3531c91fd9d961bb60b

Although this will bitrot quickly it's probably best to land this after the merge (and after getting back from PTO/Golden Week). I'll fix the inevitable test failures, and rebase on top of 1456688 tomorrow, but then I'll probably defer putting this up for review until May 7.

Comment 2

[Brian Birtles (:birtles)]

Updated try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53fa8c0f1e5c238592d18271c4f70828c4a043fc

I'll pick this up again after May 7. Hopefully it doesn't bitrot too badly by then.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Move KeyframeEffect attribute setters to KeyframeEffectReadOnly;

It might seem a bit odd to move the setters to the read-only class that we are
ultimately planning to drop but the reason for doing this is that
KeyframeEffectReadOnly.cpp has a *lot* more code than KeyframeEffect.cpp.

In order to simplify code archaeology we take the following approach:

  1. Move code from KeyframeEffect.{h,cpp} to KeyframeEffectReadOnly.{h,cpp}.
  2. Delete KeyframeEffect.{h,cpp}.
  3. Rename KeyframeEffectReadOnly.{h,cpp} to KeyframeEffect.{h,cpp}.

Note that at least steps 2 and 3 must be performed in separate patches as
mercurial does not successfully track renames when the target name already
exists.

Review commit: https://reviewboard.mozilla.org/r/241958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241958/

Comment 4

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Moving timing updates to KeyframeEffectReadOnly;

This might seem a bit odd but later in this patch series we will rename
KeyframeEffectReadOnly to KeyframeEffect.

Review commit: https://reviewboard.mozilla.org/r/241960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241960/

Comment 5

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Move KeyframeEffect constructors to KeyframeEffectReadOnly.{h,cpp};

By doing this we will have all the KeyframeEffect* related code in
KeyframeEffectReadOnly.{h,cpp} so we can rename them to KeyframeEffect.{h,cpp}
in the next patch and make it easier the history for the bulk of this code.

The added [HeaderFile] annotation will be removed in a subsequent patch in this
series.

Review commit: https://reviewboard.mozilla.org/r/241962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241962/

Comment 6

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Rename KeyframeEffectReadOnly.{h,cpp} to KeyframeEffect.{h,cpp};

Review commit: https://reviewboard.mozilla.org/r/241964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241964/

Comment 7

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Merge KeyframeEffectReadOnly and KeyframeEffect;

Review commit: https://reviewboard.mozilla.org/r/241966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241966/

Comment 8

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Rename AnimationEffectReadOnly to AnimationEffect;

Review commit: https://reviewboard.mozilla.org/r/241968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241968/

Comment 9

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Rename animation timing dictionaries;

As per the changes in:

  https://github.com/w3c/csswg-drafts/pull/2432/commits/d4519d9bba28422683809e74dcfc3f083d799aac#diff-4c9f5c055fb219a7fcad23a9a7a80b64R4206

Review commit: https://reviewboard.mozilla.org/r/241970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241970/

Comment 10

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Add AnimationEffect.getTiming();

Review commit: https://reviewboard.mozilla.org/r/241972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241972/

Comment 11

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Factor out TimingParams::FromEffectTiming;

Factoring out this method makes the code a little easier to read, particularly
when in the next patch in this series we add
a TimingParams::MergeOptionalEffectTiming method.

Review commit: https://reviewboard.mozilla.org/r/241974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241974/

Comment 12

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Add AnimationEffect.updateTiming();

Review commit: https://reviewboard.mozilla.org/r/241976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241976/

Comment 13

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Make TimingParameters::operator== compare end delay values;

Review commit: https://reviewboard.mozilla.org/r/241978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241978/

Comment 14

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Replace a few references to 'timing' in DevTools with 'getComputedTiming()' and 'updateTiming()';

Review commit: https://reviewboard.mozilla.org/r/241980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241980/

Comment 15

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Use updateTiming() in animation tests;

We will remove animation.effect.timing in the next patch.

Review commit: https://reviewboard.mozilla.org/r/241982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241982/

Comment 16

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Drop AnimationEffectTiming(ReadOnly) interfaces;

Review commit: https://reviewboard.mozilla.org/r/241984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241984/

Comment 17

[Brian Birtles (:birtles)]

Attachment: Bug 1456394 - Update reference to AnimationEffectTiming in wpt tests;

Review commit: https://reviewboard.mozilla.org/r/241986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241986/

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973604 [details]
Bug 1456394 - Update reference to AnimationEffectTiming in wpt tests;

https://reviewboard.mozilla.org/r/241986/#review247806

Comment 19

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8973601 [details]
Bug 1456394 - Replace a few references to 'timing' in DevTools with 'getComputedTiming()' and 'updateTiming()';

https://reviewboard.mozilla.org/r/241980/#review247808

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973590 [details]
Bug 1456394 - Move KeyframeEffect attribute setters to KeyframeEffectReadOnly;

https://reviewboard.mozilla.org/r/241958/#review247810

I assume that the test cases marked as FAIL will be resolved in a later patch in this series.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973591 [details]
Bug 1456394 - Moving timing updates to KeyframeEffectReadOnly;

https://reviewboard.mozilla.org/r/241960/#review247812

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973592 [details]
Bug 1456394 - Move KeyframeEffect constructors to KeyframeEffectReadOnly.{h,cpp};

https://reviewboard.mozilla.org/r/241962/#review247814

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973593 [details]
Bug 1456394 - Rename KeyframeEffectReadOnly.{h,cpp} to KeyframeEffect.{h,cpp};

https://reviewboard.mozilla.org/r/241964/#review247816

Looks fine, I haven't audited there is no other usage which didn't appear in this patch, though.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973594 [details]
Bug 1456394 - Merge KeyframeEffectReadOnly and KeyframeEffect;

https://reviewboard.mozilla.org/r/241966/#review247818

::: dom/animation/EffectSet.h:139
(Diff revision 1)
>        MOZ_ASSERT(!Done());
>        mHashIterator.Next();
>        return *this;
>      }
>  
> -    dom::KeyframeEffectReadOnly* operator* ()
> +    dom::KeyframeEffect* operator* ()

Very very minor nit.  Do we have a space between operator-blah and () in our style?

::: dom/animation/KeyframeEffect.cpp:664
(Diff revision 1)
>    //
>    // In all non-Xray cases, `aGlobal` matches the current Realm, so this
>    // matches the spec behavior.
>    //
>    // In Xray case, the new objects should be created using the document of
> -  // the target global, but KeyframeEffect and KeyframeEffectReadOnly
> +  // the target global, but the KeyframeEffect constructors is called in the

I am having trouble understanding this comment.  I think  'the KeyframeEffect constructors' means now the constructor which is exposed in script, doesn't mean KeyframeEffect constructors in C++, right?  Then it should be 'constructor', not prural. 

Oops. After reading this patch further, there is a copy constructor for KeyframeEffect.  So, 'constructors are called'?

::: dom/animation/KeyframeEffect.cpp:707
(Diff revision 1)
>      return nullptr;
>    }
>  
> -  // Create a new KeyframeEffectReadOnly object with aSource's target,
> +  // Create a new KeyframeEffect object with aSource's target,
>    // iteration composite operation, composite operation, and spacing mode.
> -  // The constructor creates a new AnimationEffect(ReadOnly) object by
> +  // The constructor creates a new AnimationEffectTiming object by

Oops. the comment was originally wrong, that was reviwed by me. :/

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973595 [details]
Bug 1456394 - Rename AnimationEffectReadOnly to AnimationEffect;

https://reviewboard.mozilla.org/r/241968/#review247822

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973597 [details]
Bug 1456394 - Add AnimationEffect.getTiming();

https://reviewboard.mozilla.org/r/241972/#review247826

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973598 [details]
Bug 1456394 - Factor out TimingParams::FromEffectTiming;

https://reviewboard.mozilla.org/r/241974/#review247830

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973597 [details]
Bug 1456394 - Add AnimationEffect.getTiming();

https://reviewboard.mozilla.org/r/241972/#review247834

::: commit-message-7b54b:1
(Diff revision 1)
> +Bug 1456394 - Add AnimationEffect.getTiming(); r?hiro,bz

We should put the link to the spec here (especially for bz).

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973600 [details]
Bug 1456394 - Make TimingParameters::operator== compare end delay values;

https://reviewboard.mozilla.org/r/241978/#review247840

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973602 [details]
Bug 1456394 - Use updateTiming() in animation tests;

https://reviewboard.mozilla.org/r/241982/#review247842

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973599 [details]
Bug 1456394 - Add AnimationEffect.updateTiming();

https://reviewboard.mozilla.org/r/241976/#review247832

I don't quite understand why we don't use mDocument for nsAutoAnimationMutationBatch but I believe bz will audit the part.

::: commit-message-cb9f9:1
(Diff revision 1)
> +Bug 1456394 - Add AnimationEffect.updateTiming(); r?hiro,bz

Should we put the link to the spec of updateTiming() here in this commit message?

::: dom/animation/AnimationEffect.cpp:85
(Diff revision 1)
> +    Maybe<nsAutoAnimationMutationBatch> mb;
> +    if (AsKeyframeEffect() && AsKeyframeEffect()->GetTarget()) {
> +      mb.emplace(AsKeyframeEffect()->GetTarget()->mElement->OwnerDoc());
> +    }

This part is completely out of my knowledge.  Can you please add a comment to explain why we don't use mDocument instead?  I guess it's related to bug 1414674 or some others, but I don't quite understand them.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8973603 [details]
Bug 1456394 - Drop AnimationEffectTiming(ReadOnly) interfaces;

https://reviewboard.mozilla.org/r/241984/#review247854

::: dom/animation/AnimationEffect.cpp:341
(Diff revision 1)
>    SetSpecifiedTiming(timing);
>  }
>  
>  AnimationEffect::~AnimationEffect()
>  {
> -  // mTiming is cycle collected, so we have to do null check first even though
> +  // This is just here so Animation can be an incomplete type in the header.

Just curious, where is/are the place(s) that requires Animation is an incomplete type?

Comment 33

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)
> Comment on attachment 8973590 [details]
> Bug 1456394 - Move KeyframeEffect attribute setters to
> KeyframeEffectReadOnly;
> 
> https://reviewboard.mozilla.org/r/241958/#review247810
> 
> I assume that the test cases marked as FAIL will be resolved in a later
> patch in this series.

Yes, that's right.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> Comment on attachment 8973594 [details]
> Bug 1456394 - Merge KeyframeEffectReadOnly and KeyframeEffect;
> 
> https://reviewboard.mozilla.org/r/241966/#review247818
> 
> ::: dom/animation/EffectSet.h:139
> (Diff revision 1)
> >        MOZ_ASSERT(!Done());
> >        mHashIterator.Next();
> >        return *this;
> >      }
> >  
> > -    dom::KeyframeEffectReadOnly* operator* ()
> > +    dom::KeyframeEffect* operator* ()
> 
> Very very minor nit.  Do we have a space between operator-blah and () in our
> style?

Looking at our codebase, it seems we don't normally. Fixed now in this patch.

> ::: dom/animation/KeyframeEffect.cpp:664
> (Diff revision 1)
> >    //
> >    // In all non-Xray cases, `aGlobal` matches the current Realm, so this
> >    // matches the spec behavior.
> >    //
> >    // In Xray case, the new objects should be created using the document of
> > -  // the target global, but KeyframeEffect and KeyframeEffectReadOnly
> > +  // the target global, but the KeyframeEffect constructors is called in the
> 
> I am having trouble understanding this comment.  I think  'the
> KeyframeEffect constructors' means now the constructor which is exposed in
> script, doesn't mean KeyframeEffect constructors in C++, right?  Then it
> should be 'constructor', not prural. 
> 
> Oops. After reading this patch further, there is a copy constructor for
> KeyframeEffect.  So, 'constructors are called'?

Fixed.

Comment 34

[Brian Birtles (:birtles)]

Comment on attachment 8973590 [details]
Bug 1456394 - Move KeyframeEffect attribute setters to KeyframeEffectReadOnly;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241958/diff/1-2/

Comment 35

[Brian Birtles (:birtles)]

Comment on attachment 8973591 [details]
Bug 1456394 - Moving timing updates to KeyframeEffectReadOnly;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241960/diff/1-2/

Comment 36

[Brian Birtles (:birtles)]

Comment on attachment 8973592 [details]
Bug 1456394 - Move KeyframeEffect constructors to KeyframeEffectReadOnly.{h,cpp};

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241962/diff/1-2/

Comment 37

[Brian Birtles (:birtles)]

Comment on attachment 8973593 [details]
Bug 1456394 - Rename KeyframeEffectReadOnly.{h,cpp} to KeyframeEffect.{h,cpp};

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241964/diff/1-2/

Comment 38

[Brian Birtles (:birtles)]

Comment on attachment 8973594 [details]
Bug 1456394 - Merge KeyframeEffectReadOnly and KeyframeEffect;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241966/diff/1-2/

Comment 39

[Brian Birtles (:birtles)]

Comment on attachment 8973595 [details]
Bug 1456394 - Rename AnimationEffectReadOnly to AnimationEffect;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241968/diff/1-2/

Comment 40

[Brian Birtles (:birtles)]

Comment on attachment 8973596 [details]
Bug 1456394 - Rename animation timing dictionaries;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241970/diff/1-2/

Comment 41

[Brian Birtles (:birtles)]

Comment on attachment 8973597 [details]
Bug 1456394 - Add AnimationEffect.getTiming();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241972/diff/1-2/

Comment 42

[Brian Birtles (:birtles)]

Comment on attachment 8973598 [details]
Bug 1456394 - Factor out TimingParams::FromEffectTiming;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241974/diff/1-2/

Comment 43

[Brian Birtles (:birtles)]

Comment on attachment 8973599 [details]
Bug 1456394 - Add AnimationEffect.updateTiming();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241976/diff/1-2/

Comment 44

[Brian Birtles (:birtles)]

Comment on attachment 8973600 [details]
Bug 1456394 - Make TimingParameters::operator== compare end delay values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241978/diff/1-2/

Comment 45

[Brian Birtles (:birtles)]

Comment on attachment 8973601 [details]
Bug 1456394 - Replace a few references to 'timing' in DevTools with 'getComputedTiming()' and 'updateTiming()';

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241980/diff/1-2/

Comment 46

[Brian Birtles (:birtles)]

Comment on attachment 8973602 [details]
Bug 1456394 - Use updateTiming() in animation tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241982/diff/1-2/

Comment 47

[Brian Birtles (:birtles)]

Comment on attachment 8973603 [details]
Bug 1456394 - Drop AnimationEffectTiming(ReadOnly) interfaces;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241984/diff/1-2/

Comment 48

[Brian Birtles (:birtles)]

Comment on attachment 8973604 [details]
Bug 1456394 - Update reference to AnimationEffectTiming in wpt tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241986/diff/1-2/

Comment 49

[Brian Birtles (:birtles)]

Rebased but mercurial handles rebasing with file renames so poorly the chance I broke something in the process is pretty high.

Comment 50

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)
> Comment on attachment 8973603 [details]
> Bug 1456394 - Drop AnimationEffectTiming(ReadOnly) interfaces;
> 
> https://reviewboard.mozilla.org/r/241984/#review247854
> 
> ::: dom/animation/AnimationEffect.cpp:341
> (Diff revision 1)
> >    SetSpecifiedTiming(timing);
> >  }
> >  
> >  AnimationEffect::~AnimationEffect()
> >  {
> > -  // mTiming is cycle collected, so we have to do null check first even though
> > +  // This is just here so Animation can be an incomplete type in the header.
> 
> Just curious, where is/are the place(s) that requires Animation is an
> incomplete type?

There aren't any anymore!

I wrote these patches a couple of months ago and there were at that time but it looks like they're gone now.

Comment 51

[Brian Birtles (:birtles)]

Comment on attachment 8973603 [details]
Bug 1456394 - Drop AnimationEffectTiming(ReadOnly) interfaces;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241984/diff/2-3/

Comment 52

[Brian Birtles (:birtles)]

Comment on attachment 8973604 [details]
Bug 1456394 - Update reference to AnimationEffectTiming in wpt tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241986/diff/2-3/

Comment 53

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> Comment on attachment 8973599 [details]
> Bug 1456394 - Add AnimationEffect.updateTiming();
> ...
> ::: dom/animation/AnimationEffect.cpp:85
> (Diff revision 1)
> > +    Maybe<nsAutoAnimationMutationBatch> mb;
> > +    if (AsKeyframeEffect() && AsKeyframeEffect()->GetTarget()) {
> > +      mb.emplace(AsKeyframeEffect()->GetTarget()->mElement->OwnerDoc());
> > +    }
> 
> This part is completely out of my knowledge.  Can you please add a comment
> to explain why we don't use mDocument instead?  I guess it's related to bug
> 1414674 or some others, but I don't quite understand them.

mDocument is the document from the current realm where the KeyframeEffect was constructed.

However, for animation observers we always use the owner document for the target element of the effect.

Comment 54

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #53)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> > Comment on attachment 8973599 [details]
> > Bug 1456394 - Add AnimationEffect.updateTiming();
> > ...
> > ::: dom/animation/AnimationEffect.cpp:85
> > (Diff revision 1)
> > > +    Maybe<nsAutoAnimationMutationBatch> mb;
> > > +    if (AsKeyframeEffect() && AsKeyframeEffect()->GetTarget()) {
> > > +      mb.emplace(AsKeyframeEffect()->GetTarget()->mElement->OwnerDoc());
> > > +    }
> > 
> > This part is completely out of my knowledge.  Can you please add a comment
> > to explain why we don't use mDocument instead?  I guess it's related to bug
> > 1414674 or some others, but I don't quite understand them.
> 
> mDocument is the document from the current realm where the KeyframeEffect
> was constructed.
> 
> However, for animation observers we always use the owner document for the
> target element of the effect.

Oh I see.  Thanks for the explanation.  It would be nice to have a test case for our animation observer tests at some point. :)

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973590 [details]
Bug 1456394 - Move KeyframeEffect attribute setters to KeyframeEffectReadOnly;

https://reviewboard.mozilla.org/r/241958/#review249084

r=me on the webidl bits

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973592 [details]
Bug 1456394 - Move KeyframeEffect constructors to KeyframeEffectReadOnly.{h,cpp};

https://reviewboard.mozilla.org/r/241962/#review249086

::: commit-message-930df:5
(Diff revision 2)
> +Bug 1456394 - Move KeyframeEffect constructors to KeyframeEffectReadOnly.{h,cpp}; r?hiro,bz
> +
> +By doing this we will have all the KeyframeEffect* related code in
> +KeyframeEffectReadOnly.{h,cpp} so we can rename them to KeyframeEffect.{h,cpp}
> +in the next patch and make it easier the history for the bulk of this code.

"make it easier to examine the history"?

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973593 [details]
Bug 1456394 - Rename KeyframeEffectReadOnly.{h,cpp} to KeyframeEffect.{h,cpp};

https://reviewboard.mozilla.org/r/241964/#review249088

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973594 [details]
Bug 1456394 - Merge KeyframeEffectReadOnly and KeyframeEffect;

https://reviewboard.mozilla.org/r/241966/#review249090

r=me on the webidl and test_interfaces changes.  I didn't read the rest very carefully, though I did skim it some.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973595 [details]
Bug 1456394 - Rename AnimationEffectReadOnly to AnimationEffect;

https://reviewboard.mozilla.org/r/241968/#review249092

r=me on the webidl and test_interfaces changes.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973596 [details]
Bug 1456394 - Rename animation timing dictionaries;

https://reviewboard.mozilla.org/r/241970/#review249094

r=me

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973597 [details]
Bug 1456394 - Add AnimationEffect.getTiming();

https://reviewboard.mozilla.org/r/241972/#review249096

r=me on the webidl bits

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973599 [details]
Bug 1456394 - Add AnimationEffect.updateTiming();

https://reviewboard.mozilla.org/r/241976/#review249112

r=me on the webidl bits.

::: dom/animation/TimingParams.h:82
(Diff revision 2)
>      nsIDocument* aDocument, ErrorResult& aRv);
>    static TimingParams FromEffectTiming(
>      const dom::EffectTiming& aEffectTiming,
>      nsIDocument* aDocument,
>      ErrorResult& aRv);
> +  static TimingParams MergeOptionalEffectTiming(

This should really be documented.  What does it do?

Comment 63

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8973603 [details]
Bug 1456394 - Drop AnimationEffectTiming(ReadOnly) interfaces;

https://reviewboard.mozilla.org/r/241984/#review249114

r=me on the webidl and test_interfaces changes.

Comment 64

[Brian Birtles (:birtles)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #62)
> Comment on attachment 8973599 [details]
> Bug 1456394 - Add AnimationEffect.updateTiming();
> 
> https://reviewboard.mozilla.org/r/241976/#review249112
> 
> r=me on the webidl bits.
> 
> ::: dom/animation/TimingParams.h:82
> (Diff revision 2)
> >      nsIDocument* aDocument, ErrorResult& aRv);
> >    static TimingParams FromEffectTiming(
> >      const dom::EffectTiming& aEffectTiming,
> >      nsIDocument* aDocument,
> >      ErrorResult& aRv);
> > +  static TimingParams MergeOptionalEffectTiming(
> 
> This should really be documented.  What does it do?

Updated with the following comment:

  // Returns a copy of |aSource| where each timing property in |aSource| that
  // is also specified in |aEffectTiming| is replaced with the value from
  // |aEffectTiming|.
  //
  // If any of the values in |aEffectTiming| are invalid, |aRv.Failed()| will be
  // true and an unmodified copy of |aSource| will be returned.

Comment 65

[Brian Birtles (:birtles)]

Thanks for the reviews!

Comment 66

[Brian Birtles (:birtles)]

Comment on attachment 8973590 [details]
Bug 1456394 - Move KeyframeEffect attribute setters to KeyframeEffectReadOnly;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241958/diff/2-3/

Comment 67

[Brian Birtles (:birtles)]

Comment on attachment 8973591 [details]
Bug 1456394 - Moving timing updates to KeyframeEffectReadOnly;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241960/diff/2-3/

Comment 68

[Brian Birtles (:birtles)]

Comment on attachment 8973592 [details]
Bug 1456394 - Move KeyframeEffect constructors to KeyframeEffectReadOnly.{h,cpp};

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241962/diff/2-3/

Comment 69

[Brian Birtles (:birtles)]

Comment on attachment 8973593 [details]
Bug 1456394 - Rename KeyframeEffectReadOnly.{h,cpp} to KeyframeEffect.{h,cpp};

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241964/diff/2-3/

Comment 70

[Brian Birtles (:birtles)]

Comment on attachment 8973594 [details]
Bug 1456394 - Merge KeyframeEffectReadOnly and KeyframeEffect;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241966/diff/2-3/

Comment 71

[Brian Birtles (:birtles)]

Comment on attachment 8973595 [details]
Bug 1456394 - Rename AnimationEffectReadOnly to AnimationEffect;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241968/diff/2-3/

Comment 72

[Brian Birtles (:birtles)]

Comment on attachment 8973596 [details]
Bug 1456394 - Rename animation timing dictionaries;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241970/diff/2-3/

Comment 73

[Brian Birtles (:birtles)]

Comment on attachment 8973597 [details]
Bug 1456394 - Add AnimationEffect.getTiming();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241972/diff/2-3/

Comment 74

[Brian Birtles (:birtles)]

Comment on attachment 8973598 [details]
Bug 1456394 - Factor out TimingParams::FromEffectTiming;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241974/diff/2-3/

Comment 75

[Brian Birtles (:birtles)]

Comment on attachment 8973599 [details]
Bug 1456394 - Add AnimationEffect.updateTiming();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241976/diff/2-3/

Comment 76

[Brian Birtles (:birtles)]

Comment on attachment 8973600 [details]
Bug 1456394 - Make TimingParameters::operator== compare end delay values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241978/diff/2-3/

Comment 77

[Brian Birtles (:birtles)]

Comment on attachment 8973601 [details]
Bug 1456394 - Replace a few references to 'timing' in DevTools with 'getComputedTiming()' and 'updateTiming()';

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241980/diff/2-3/

Comment 78

[Brian Birtles (:birtles)]

Comment on attachment 8973602 [details]
Bug 1456394 - Use updateTiming() in animation tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241982/diff/2-3/

Comment 79

[Brian Birtles (:birtles)]

Comment on attachment 8973603 [details]
Bug 1456394 - Drop AnimationEffectTiming(ReadOnly) interfaces;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241984/diff/3-4/

Comment 80

[Brian Birtles (:birtles)]

Comment on attachment 8973604 [details]
Bug 1456394 - Update reference to AnimationEffectTiming in wpt tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241986/diff/3-4/

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45e028b105e5
Move KeyframeEffect attribute setters to KeyframeEffectReadOnly; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/831f34bd841d
Moving timing updates to KeyframeEffectReadOnly; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5f8a3f4a435a
Move KeyframeEffect constructors to KeyframeEffectReadOnly.{h,cpp}; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/ede7f6c30b62
Rename KeyframeEffectReadOnly.{h,cpp} to KeyframeEffect.{h,cpp}; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/238d0908de38
Merge KeyframeEffectReadOnly and KeyframeEffect; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/7eae2c106d30
Rename AnimationEffectReadOnly to AnimationEffect; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/97256bd7ef99
Rename animation timing dictionaries; r=bz
https://hg.mozilla.org/integration/autoland/rev/995676902281
Add AnimationEffect.getTiming(); r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/2d36f43a3807
Factor out TimingParams::FromEffectTiming; r=hiro
https://hg.mozilla.org/integration/autoland/rev/ea71864d99b4
Add AnimationEffect.updateTiming(); r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/ab4634b0c0b7
Make TimingParameters::operator== compare end delay values; r=hiro
https://hg.mozilla.org/integration/autoland/rev/0ab592b16b04
Replace a few references to 'timing' in DevTools with 'getComputedTiming()' and 'updateTiming()'; r=daisuke
https://hg.mozilla.org/integration/autoland/rev/e85f004907be
Use updateTiming() in animation tests; r=hiro
https://hg.mozilla.org/integration/autoland/rev/99694c74c0f4
Drop AnimationEffectTiming(ReadOnly) interfaces; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/8e9a4a323f0c
Update reference to AnimationEffectTiming in wpt tests; r=hiro

Comment 82

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10961 for changes under testing/web-platform/tests

Comment 83

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 84

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/45e028b105e5
https://hg.mozilla.org/mozilla-central/rev/831f34bd841d
https://hg.mozilla.org/mozilla-central/rev/5f8a3f4a435a
https://hg.mozilla.org/mozilla-central/rev/ede7f6c30b62
https://hg.mozilla.org/mozilla-central/rev/238d0908de38
https://hg.mozilla.org/mozilla-central/rev/7eae2c106d30
https://hg.mozilla.org/mozilla-central/rev/97256bd7ef99
https://hg.mozilla.org/mozilla-central/rev/995676902281
https://hg.mozilla.org/mozilla-central/rev/2d36f43a3807
https://hg.mozilla.org/mozilla-central/rev/ea71864d99b4
https://hg.mozilla.org/mozilla-central/rev/ab4634b0c0b7
https://hg.mozilla.org/mozilla-central/rev/0ab592b16b04
https://hg.mozilla.org/mozilla-central/rev/e85f004907be
https://hg.mozilla.org/mozilla-central/rev/99694c74c0f4
https://hg.mozilla.org/mozilla-central/rev/8e9a4a323f0c

Comment 85

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 86

[Florian Scholz (Open Web Docs)]

Here's a great comment that summarizes these changes:
https://github.com/w3c/csswg-drafts/pull/2432#issue-174270202

For MDN, I did:

Compat data updates:
https://github.com/mdn/browser-compat-data/pull/2481
https://github.com/mdn/browser-compat-data/pull/2482
https://github.com/mdn/browser-compat-data/pull/2483
https://github.com/mdn/browser-compat-data/pull/2485

Navigation updates:
https://github.com/mdn/kumascript/pull/742

Compat table updates and navigation updates will be deployed to MDN soon, but it's not there yet as of now (will be later this week).
I went ahead and did update a bunch of MDN wiki pages for this already, though:

Updated MDN pages for AnimationEffectReadOnly -> AnimationEffect:
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffect
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffect/getTiming
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffect/getComputedTiming
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffect/updateTiming
(and AnimationEffectReadOnly pages now redirect to AnimationEffect)


AnimationEffectTiming(ReadOnly) are gone, so these pages are deleted now:
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTiming
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly/delay
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly/direction
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly/duration
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly/easing
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly/endDelay
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly/fill
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly/iterations
https://developer.mozilla.org/en-US/docs/Web/API/AnimationEffectTimingReadOnly/iterationStart

Updated MDN pages for AnimationEffectTimingProperties -> EffectTiming
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming/delay
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming/direction
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming/duration
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming/easing
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming/endDelay
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming/fill
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming/iterations
https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming/iterationStart
(and AnimationEffectTimingProperties pages now redirect to EffectTiming)

KeyframeEffectReadOnly is gone, so pages redirect to KeyframeEffect and these have been updated:
https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffect
https://developer.mozilla.org/docs/Web/API/KeyframeEffect/KeyframeEffect
https://developer.mozilla.org/docs/Web/API/KeyframeEffect/composite
https://developer.mozilla.org/docs/Web/API/KeyframeEffect/iterationComposite
https://developer.mozilla.org/docs/Web/API/KeyframeEffect/getKeyframes
https://developer.mozilla.org/docs/Web/API/KeyframeEffect/setKeyframes
https://developer.mozilla.org/docs/Web/API/KeyframeEffect/target

Reviews and fixes to these MDN wiki pages are welcome, but let me know if I got something terribly wrong and I will fix it up.

Comment 87

[Brian Birtles (:birtles)]

Looks perfect, thanks Florian!

Comment 88

[Florian Scholz (Open Web Docs)]

Thanks, the navigation and the compat tables are updated now as well.