Closed Bug 1273784 Opened 4 years ago Closed 3 years ago

Implement keyframe effect copy constructors

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- affected
firefox52 --- fixed

People

(Reporter: birtles, Assigned: boris)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(6 files)

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.
Assignee: nobody → boris.chiou
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.
Assignee: boris.chiou → nobody
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
See Also: → 1314537
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
Attachment #8805473 - Flags: review?(bugs) → 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?
Flags: needinfo?(boris.chiou)
Comment on attachment 8807465 [details]
Bug 1273784 - Part 3: Implement KeyframeEffect(ReadOnly) copy constructor API.

https://reviewboard.mozilla.org/r/89778/#review90542
Attachment #8807465 - Flags: review?(hiikezoe) → 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?
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.
Attachment #8807467 - Flags: review?(hiikezoe)
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
Attachment #8807464 - Flags: review?(hiikezoe) → 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?
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?
Attachment #8807466 - Flags: review?(hiikezoe) → review+
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.
Flags: needinfo?(boris.chiou)
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?
Attachment #8807467 - Flags: review?(hiikezoe) → review+
Comment on attachment 8808086 [details]
Bug 1273784 - Part 6: Factor out BuildProperties.

https://reviewboard.mozilla.org/r/91014/#review90760
Attachment #8808086 - Flags: review?(hiikezoe) → 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

> 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
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?
Flags: needinfo?(cmills)
(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?
Flags: needinfo?(cmills)
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.
You need to log in before you can comment on or make changes to this bug.