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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

({dev-doc-complete})

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

This is going to involve a lot of renaming test files too, I expect.
MozReview-Commit-ID: GwLLY39l1KE
Attachment #8751578 - Flags: review?(hiikezoe)
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.
Attachment #8751578 - Flags: review?(bugs)
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
Attachment #8751578 - Flags: review?(hiikezoe) → review+
(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!
Attachment #8751578 - Attachment is obsolete: true
Attachment #8751578 - Flags: review?(bugs)
(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!
(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 on attachment 8751583 [details] [diff] [review]
Rename KeyframeEffectReadOnly.getFrames() and KeyframeEffect.setFrames() to getKeyframes()/setKeyframes()

r+ for the .webidl.
Attachment #8751583 - Flags: review?(bugs) → review+
I've renamed and updated the various MDN pages for this change.
https://hg.mozilla.org/mozilla-central/rev/45c71ce1c164
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.