Web Animations has removed the clone() method in favor of copy constructors for KeyframeEffectReadOnly and KeyframeEffect.[1] [1] https://github.com/w3c/web-animations/commit/565bc7c2103e8dd4192a75b744f01e7c44ebc9ad I think Boris was interested in looking into this.

Cool. I can take this.

I'm not planning to do this bug in this quarter, so untake this now. Please feel free to take this if you are interested in this bug.

Comment on attachment 8805473 [details] Bug 1273784 - Part 1: Add the copy constructor of KeyframeEffect(ReadOnly) in webidl. https://reviewboard.mozilla.org/r/89218/#review90348

Comment on attachment 8807464 [details] Bug 1273784 - Part 2: Overload ConstructKeyframeEffect for copy constructor. https://reviewboard.mozilla.org/r/89776/#review90540 I think this is basically fine but one thing I am concerned is that we need to call SetKeyframes() instead of copying some member variables of KeyframeEffectReadOnly. Boris?

Comment on attachment 8807465 [details] Bug 1273784 - Part 3: Implement KeyframeEffect(ReadOnly) copy constructor API. https://reviewboard.mozilla.org/r/89778/#review90542

Comment on attachment 8805473 [details] Bug 1273784 - Part 1: Add the copy constructor of KeyframeEffect(ReadOnly) in webidl. https://reviewboard.mozilla.org/r/89218/#review90546 ::: dom/webidl/KeyframeEffect.webidl:41 (Diff revision 2) > readonly attribute IterationCompositeOperation iterationComposite; > readonly attribute CompositeOperation composite; > readonly attribute DOMString spacing; > > // Not yet implemented: > // KeyframeEffect clone(); Can't we drop this clone() in this bug?

Comment on attachment 8807464 [details] Bug 1273784 - Part 2: Overload ConstructKeyframeEffect for copy constructor. https://reviewboard.mozilla.org/r/89776/#review90540 SetKeyframes() will re-compute the spacing and re-build the animation properties, so there are some redundant computations. I also have this concern, so I add part 5 which only copies the member variables to avoid the redundant calculation. I will also send you a review request for part 5.

Comment on attachment 8807467 [details] Bug 1273784 - Part 5: Avoid re-building the animation properties and re-calculating computed offsets. https://reviewboard.mozilla.org/r/90358/#review90686 The idea is simple: I split UpdateProperties() into (a) BuildProperties, and (b) SetProperties. Therefore, I can re-use SetProperties() for copy-constructor. (p.s. I don't copy mProperties directly because I think we have to reset the status of mIsRunningOnCompositor and SetProperties() can do it.)

Comment on attachment 8805473 [details] Bug 1273784 - Part 1: Add the copy constructor of KeyframeEffect(ReadOnly) in webidl. https://reviewboard.mozilla.org/r/89218/#review90546 > Can't we drop this clone() in this bug? Yes. I didn't notice this. I will drop it. Thanks.

Comment on attachment 8807464 [details] Bug 1273784 - Part 2: Overload ConstructKeyframeEffect for copy constructor. https://reviewboard.mozilla.org/r/89776/#review90706

Comment on attachment 8807467 [details] Bug 1273784 - Part 5: Avoid re-building the animation properties and re-calculating computed offsets. https://reviewboard.mozilla.org/r/90358/#review90708 ::: dom/animation/KeyframeEffectReadOnly.cpp:596 (Diff revision 2) > + nsTArray<AnimationProperty> properties(aSource.mProperties); > + effect->SetProperties(Move(properties)); > return effect.forget(); > } I think we can/should copy mCumulativeChangeHint too. mCumulativeChangeHint should be the same as the source one because both of targets are the same. ::: dom/animation/KeyframeEffectReadOnly.cpp:646 (Diff revision 2) > +KeyframeEffectReadOnly::SetProperties(nsTArray<AnimationProperty>&& aProperties) > +{ > + if (mProperties == aProperties) { > + return; > + } > + > + // Preserve the state of the mIsRunningOnCompositor flag. > + nsCSSPropertyIDSet runningOnCompositorProperties; > + > + for (const AnimationProperty& property : mProperties) { > + if (property.mIsRunningOnCompositor) { > + runningOnCompositorProperties.AddProperty(property.mProperty); > + } > + } > + > + mProperties = Move(aProperties); > + > + for (AnimationProperty& property : mProperties) { > + property.mIsRunningOnCompositor = > + runningOnCompositorProperties.HasProperty(property.mProperty); > + } > +} For the copy constructor, I think we can avoid iteration mProperties twice. We can just copy mProperty and mSegments. mIsRunningOnCompositor and mPerformanceWarning can be left as default values of AnimationProperty struct. So how about adding a copy constructor in AnimationProperty?

Comment on attachment 8807467 [details] Bug 1273784 - Part 5: Avoid re-building the animation properties and re-calculating computed offsets. https://reviewboard.mozilla.org/r/90358/#review90708 > For the copy constructor, I think we can avoid iteration mProperties twice. We can just copy mProperty and mSegments. mIsRunningOnCompositor and mPerformanceWarning can be left as default values of AnimationProperty struct. > So how about adding a copy constructor in AnimationProperty? Sounds great. I will add a copy constructor for AnimationProperty.

Comment on attachment 8807466 [details] Bug 1273784 - Part 4: Test. https://reviewboard.mozilla.org/r/90020/#review90544 ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/copy-contructor.html:19 (Diff revision 2) > +test(function(t) { > + createStyle(t, { '@keyframes anim': '' }); > + var target = createDiv(t); > + target.style.animation = 'anim 2s'; > + > + var anim = target.getAnimations().find(anim => anim.animationName == 'anim'); I think now we can use arrow function, but let's not use it for now. Also we've been using getAnimations()[0] to get an animation object if it is guaranteed that there is only one animation. Is there any reason to use Array.find? I just talked with Brian about this test case, he said, we just need a KeyframeReadOnly object here, just like you did in another test files. So something like this; var effect = new KeyframeEffectReadOnly(...) var copiedEffect = new KeyframeEffect(effect); ... ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffectReadOnly/copy-contructor.html:35 (Diff revision 2) > + for (var i = 0; i < KeyframesA.length; ++i) { > + assert_equals(KeyframesA[i].offset, KeyframesB[i].offset, > + 'Keyframe ' + i + ' has the same offset'); > + assert_equals(KeyframesA[i].computedOffset, KeyframesB[i].computedOffset, > + 'keyframe ' + i + ' has the same computedOffset'); > + assert_equals(KeyframesA[i].easing, KeyframesB[i].easing, > + 'keyframe ' + i + ' has the same easing'); > + assert_equals(KeyframesA[i].composite, KeyframesB[i].composite, > + 'keyframe ' + i + ' has the same composite'); > + } Shouldn't we check property member too?

Comment on attachment 8807466 [details] Bug 1273784 - Part 4: Test. https://reviewboard.mozilla.org/r/90020/#review90544 > I think now we can use arrow function, but let's not use it for now. > Also we've been using getAnimations()[0] to get an animation object if it is guaranteed that there is only one animation. Is there any reason to use Array.find? > > I just talked with Brian about this test case, he said, we just need a KeyframeReadOnly object here, just like you did in another test files. So something like this; > > var effect = new KeyframeEffectReadOnly(...) > var copiedEffect = new KeyframeEffect(effect); > ... Sure. I can just create a KeyframeEffectReadOnly object for this test case. Thanks. Not reason to use find. Actually, I just copied the example from the spec. :) > Shouldn't we check property member too? I will also check it. Thanks.

Comment on attachment 8807467 [details] Bug 1273784 - Part 5: Avoid re-building the animation properties and re-calculating computed offsets. https://reviewboard.mozilla.org/r/90358/#review90708 > I think we can/should copy mCumulativeChangeHint too. mCumulativeChangeHint should be the same as the source one because both of targets are the same. OK. I would add this to part 2.

Comment on attachment 8807467 [details] Bug 1273784 - Part 5: Avoid re-building the animation properties and re-calculating computed offsets. https://reviewboard.mozilla.org/r/90358/#review90758 ::: dom/animation/KeyframeEffectReadOnly.h:160 (Diff revision 5) > + AnimationProperty(const AnimationProperty& aOther) > + : mProperty(aOther.mProperty), mSegments(aOther.mSegments) { } > + AnimationProperty& operator=(const AnimationProperty& aOther) > + { > + mProperty = aOther.mProperty; > + mSegments = aOther.mSegments; > + return *this; > + } Oops! My last comment seems wrong. Is this copy constructor really necessary? As you wrote, we just need the copy assignment?

Comment on attachment 8808086 [details] Bug 1273784 - Part 6: Factor out BuildProperties. https://reviewboard.mozilla.org/r/91014/#review90760

Comment on attachment 8807467 [details] Bug 1273784 - Part 5: Avoid re-building the animation properties and re-calculating computed offsets. https://reviewboard.mozilla.org/r/90358/#review90758 > Oops! My last comment seems wrong. Is this copy constructor really necessary? As you wrote, we just need the copy assignment? Yes, but I think it's better to write both copy constructor and assignment to let others know we only copy/assign mProperty and mSegments to avoid unnecessary errors in the future. Therefore, I think this is OK.

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/27ab2738bb84 Part 1: Add the copy constructor of KeyframeEffect(ReadOnly) in webidl. r=smaug https://hg.mozilla.org/integration/autoland/rev/90787e3bbb8d Part 2: Overload ConstructKeyframeEffect for copy constructor. r=hiro https://hg.mozilla.org/integration/autoland/rev/f71551aae1c6 Part 3: Implement KeyframeEffect(ReadOnly) copy constructor API. r=hiro https://hg.mozilla.org/integration/autoland/rev/c543f2472f40 Part 4: Test. r=hiro https://hg.mozilla.org/integration/autoland/rev/2d93758d64e9 Part 5: Avoid re-building the animation properties and re-calculating computed offsets. r=hiro https://hg.mozilla.org/integration/autoland/rev/48ee0a796817 Part 6: Factor out BuildProperties. r=hiro

https://hg.mozilla.org/mozilla-central/rev/27ab2738bb84 https://hg.mozilla.org/mozilla-central/rev/90787e3bbb8d https://hg.mozilla.org/mozilla-central/rev/f71551aae1c6 https://hg.mozilla.org/mozilla-central/rev/c543f2472f40 https://hg.mozilla.org/mozilla-central/rev/2d93758d64e9 https://hg.mozilla.org/mozilla-central/rev/48ee0a796817

I have documented the new constructors, adding descriptions and updating browser support info on: https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly/KeyframeEffectReadOnly https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffect https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffect/KeyframeEffect And also updating browser support info other related pages: https://developer.mozilla.org/en-US/docs/Web/API/Web_Animations_API I've also added a note to the Fx52 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM Let me know if this looks OK. Thanks!

Hi Chris, thanks for updating this. This interface and its constructor, however, is not shipping to beta/release at this time so perhaps it doesn't belong in the release notes? I think we also decided not to add features we aren't shipping to the browser support info so perhaps we should not mention it there yet?

(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #44) > Hi Chris, thanks for updating this. This interface and its constructor, > however, is not shipping to beta/release at this time so perhaps it doesn't > belong in the release notes? > > I think we also decided not to add features we aren't shipping to the > browser support info so perhaps we should not mention it there yet? I think that some people will still be interested in knowing this stuff exists, even just in Nightly/Dev edition. So how about this: 1. I remove the release note mention; I agree that this isn't really appropriate for now. 2. I change the browser support info notes to say that it is currently available in 52+ Nightly/Dev edition only ? Would that work for you?

Chris, for experimental features not shipping in the release yet we now have this page: https://developer.mozilla.org/en-US/Firefox/Experimental_features Regarding the support info, I was told lately (might have been Jean-Yves) that we should list it as not supported, i.e. {{CompatNo}}, and add a footnote referring to the experimental implementation. Sebastian

We originally started marking non-shipping features in the compat table but it was difficult to know what version to include since they have been implemented incrementally and the spec has changed (and in some areas may yet change again). The same is true for the Chrome implementation since they started earlier and the spec changed since. So we agreed with them to just leave non-shipping features out for now.

> Regarding the support info, I was told lately (might have been Jean-Yves) > that we should list it as not supported, i.e. {{CompatNo}}, and add a > footnote referring to the experimental implementation. Now I remembered where the discussion happened. See https://bugzilla.mozilla.org/show_bug.cgi?id=1176968#c22 and https://groups.google.com/forum/#!topic/mozilla.dev.mdc/B--xY8My3_4. Sebastian

Ok, no problem. As suggested by Sebastian, I've made an entry for it in the Experimental features page, and deleted the entry from the release notes. I've also updated the support information to "No support", but with a footnote saying it is available in 52 Nightly and Dev edition, as seems to be what we've agreed for such cases in the docs team.