Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Description • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 1 • 8 years ago

Cool. I can take this.

	Boris Chiou [:boris] Assignee
	Comment 2 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 8 years ago
mozreview-review

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

	Hiroyuki Ikezoe (:hiro)
	Comment 12 • 8 years ago
mozreview-review

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?

	Hiroyuki Ikezoe (:hiro)
	Comment 13 • 8 years ago
mozreview-review

Comment on attachment 8807465 [details]
Bug 1273784 - Part 3: Implement KeyframeEffect(ReadOnly) copy constructor API.

https://reviewboard.mozilla.org/r/89778/#review90542

	Hiroyuki Ikezoe (:hiro)
	Comment 14 • 8 years ago
mozreview-review

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?

	Boris Chiou [:boris] Assignee
	Comment 15 • 8 years ago
mozreview-review-reply

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.

	Boris Chiou [:boris] Assignee
	Comment 16 • 8 years ago
mozreview-review

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.)

	Boris Chiou [:boris] Assignee
	Comment 17 • 8 years ago
mozreview-review-reply

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.

	Hiroyuki Ikezoe (:hiro)
	Comment 18 • 8 years ago
mozreview-review

Comment on attachment 8807464 [details]
Bug 1273784 - Part 2: Overload ConstructKeyframeEffect for copy constructor.

https://reviewboard.mozilla.org/r/89776/#review90706

	Hiroyuki Ikezoe (:hiro)
	Comment 19 • 8 years ago
mozreview-review

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?

	Boris Chiou [:boris] Assignee
	Comment 20 • 8 years ago
mozreview-review-reply

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.

	Hiroyuki Ikezoe (:hiro)
	Comment 21 • 8 years ago
mozreview-review

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?

	Boris Chiou [:boris] Assignee
	Comment 22 • 8 years ago
mozreview-review-reply

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.

	Boris Chiou [:boris] Assignee
	Comment 23 • 8 years ago
mozreview-review-reply

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.

	Hiroyuki Ikezoe (:hiro)
	Comment 38 • 8 years ago
mozreview-review

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?

	Hiroyuki Ikezoe (:hiro)
	Comment 39 • 8 years ago
mozreview-review

Comment on attachment 8808086 [details]
Bug 1273784 - Part 6: Factor out BuildProperties.

https://reviewboard.mozilla.org/r/91014/#review90760

	Boris Chiou [:boris] Assignee
	Comment 40 • 8 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 41 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 42 • 8 years ago
bugherder

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

	Chris Mills [:cmills]
	Comment 43 • 7 years ago

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!

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 44 • 7 years ago

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?

	Chris Mills [:cmills]
	Comment 45 • 7 years ago

(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?

	Sebastian Zartner [:sebo]
	Comment 46 • 7 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 47 • 7 years ago

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.

	Sebastian Zartner [:sebo]
	Comment 48 • 7 years ago

> 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

	Chris Mills [:cmills]
	Comment 49 • 7 years ago

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.