1271904 - Rename KeyframeEffectReadOnly.getFrames() and KeyframeEffect.setFrames() to getKeyframes()/setKeyframes()

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Spec change: https://github.com/w3c/web-animations/commit/be0227af30a9e013ed2f99f6824f9e849af9d262

This is going to involve a lot of renaming test files too, I expect.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Rename KeyframeEffectReadOnly.getFrames() and KeyframeEffect.setFrames() to getKeyframes()/setKeyframes()

MozReview-Commit-ID: GwLLY39l1KE

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8751578 [details] [diff] [review]
Rename KeyframeEffectReadOnly.getFrames() and KeyframeEffect.setFrames() to getKeyframes()/setKeyframes()

Hi Olli, can you please review the webidl changes here. Thanks.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8751578 [details] [diff] [review]
Rename KeyframeEffectReadOnly.getFrames() and KeyframeEffect.setFrames() to getKeyframes()/setKeyframes()

Review of attachment 8751578 [details] [diff] [review]:
-----------------------------------------------------------------

Are you going to rename below files in a subsequent patch?  I think we should rename them in this patch.

dom/animation/test/css-transitions/test_keyframeeffect-getframes.html
dom/animation/test/css-transitions/file_keyframeeffect-getframes.html
dom/animation/test/css-animations/test_keyframeeffect-getframes.html
dom/animation/test/css-animations/file_keyframeeffect-getframes.html

There are two remainings in comment.  Yay! This reminds me that I have to rewrite it!
https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/dom/animation/test/chrome/test_animation_performance_warning.html#176
https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/dom/animation/test/chrome/test_animation_performance_warning.html#352

::: testing/web-platform/meta/MANIFEST.json
@@ +35222,1 @@
>      ],

I guess this part is not intentional, but it's OK to me.

::: testing/web-platform/mozilla/meta/MANIFEST.json
@@ +7,5 @@
>      "wdspec": []
>    },
>    "local_changes": {
>      "deleted": [],
> +    "deleted_reftests": {},

I am not familiar with our test framework for web platform test. Where did this change come from?

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
@@ +69,4 @@
>     "KeyframeEffectReadOnly constructor in KeyframeTimingOptions");
>  
>  test(function(t) {
>    var getFrame = function(composite) {

nit: here is a survivor.

::: testing/web-platform/tests/web-animations/keyframe-effect/setKeyframes.html
@@ +4,1 @@
>  <link rel="help" href="https://w3c.github.io/web-animations/#dom-keyframeeffect-setframes">

nit: #dom-keyframeeffect-setkeyframes

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8751578 [details] [diff] [review]
> Rename KeyframeEffectReadOnly.getFrames() and KeyframeEffect.setFrames() to
> getKeyframes()/setKeyframes()
> 
> Review of attachment 8751578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are you going to rename below files in a subsequent patch?  I think we
> should rename them in this patch.
> 
> dom/animation/test/css-transitions/test_keyframeeffect-getframes.html
> dom/animation/test/css-transitions/file_keyframeeffect-getframes.html
> dom/animation/test/css-animations/test_keyframeeffect-getframes.html
> dom/animation/test/css-animations/file_keyframeeffect-getframes.html

Good idea, will do.

> There are two remainings in comment.  Yay! This reminds me that I have to
> rewrite it!
> https://dxr.mozilla.org/mozilla-central/rev/
> 3461f3cae78495f100a0f7d3d2e0b89292d3ec02/dom/animation/test/chrome/
> test_animation_performance_warning.html#176
> https://dxr.mozilla.org/mozilla-central/rev/
> 3461f3cae78495f100a0f7d3d2e0b89292d3ec02/dom/animation/test/chrome/
> test_animation_performance_warning.html#352

Yes, I deliberately left them and filed a bug for removing that comment (bug 1272204).

> ::: testing/web-platform/meta/MANIFEST.json
> @@ +35222,1 @@
> >      ],
> 
> I guess this part is not intentional, but it's OK to me.
> 
> ::: testing/web-platform/mozilla/meta/MANIFEST.json
> @@ +7,5 @@
> >      "wdspec": []
> >    },
> >    "local_changes": {
> >      "deleted": [],
> > +    "deleted_reftests": {},
> 
> I am not familiar with our test framework for web platform test. Where did
> this change come from?

These changes come from running ./mach web-platform-tests --manifest-update

> :::
> testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
> @@ +69,4 @@
> >     "KeyframeEffectReadOnly constructor in KeyframeTimingOptions");
> >  
> >  test(function(t) {
> >    var getFrame = function(composite) {
> 
> nit: here is a survivor.

Thanks!

> :::
> testing/web-platform/tests/web-animations/keyframe-effect/setKeyframes.html
> @@ +4,1 @@
> >  <link rel="help" href="https://w3c.github.io/web-animations/#dom-keyframeeffect-setframes">
> 
> nit: #dom-keyframeeffect-setkeyframes

Thanks!

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Rename KeyframeEffectReadOnly.getFrames() and KeyframeEffect.setFrames() to getKeyframes()/setKeyframes()

MozReview-Commit-ID: GwLLY39l1KE

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #4)
> > ::: testing/web-platform/mozilla/meta/MANIFEST.json
> > @@ +7,5 @@
> > >      "wdspec": []
> > >    },
> > >    "local_changes": {
> > >      "deleted": [],
> > > +    "deleted_reftests": {},
> > 
> > I am not familiar with our test framework for web platform test. Where did
> > this change come from?
> 
> These changes come from running ./mach web-platform-tests --manifest-update

Yes, I guessed so.  What I wanted to know is which change caused this.  I did not know web platform tests include reftests!

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > > ::: testing/web-platform/mozilla/meta/MANIFEST.json
> > > @@ +7,5 @@
> > > >      "wdspec": []
> > > >    },
> > > >    "local_changes": {
> > > >      "deleted": [],
> > > > +    "deleted_reftests": {},
> > > 
> > > I am not familiar with our test framework for web platform test. Where did
> > > this change come from?
> > 
> > These changes come from running ./mach web-platform-tests --manifest-update
> 
> Yes, I guessed so.  What I wanted to know is which change caused this.  I
> did not know web platform tests include reftests!

Yes, it does. I just presumed that the update script had been updated and started adding this. Maybe I should just manually drop this line.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8751583 [details] [diff] [review]
Rename KeyframeEffectReadOnly.getFrames() and KeyframeEffect.setFrames() to getKeyframes()/setKeyframes()

r+ for the .webidl.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45c71ce1c164

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I've renamed and updated the various MDN pages for this change.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45c71ce1c164