Closed
Bug 1273784
Opened 8 years ago
Closed 8 years ago
Implement keyframe effect copy constructors
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: birtles, Assigned: boris)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(6 files)
58 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Cool. I can take this.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → boris.chiou
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.
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee: boris.chiou → nobody
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → boris.chiou
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
Attachment #8805473 -
Flags: review?(bugs) → review+
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?
Updated•8 years ago
|
Flags: needinfo?(boris.chiou)
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
Attachment #8807465 -
Flags: review?(hiikezoe) → review+
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?
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8807467 -
Flags: review?(hiikezoe)
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.)
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.
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
Attachment #8807464 -
Flags: review?(hiikezoe) → review+
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?
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.
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?
Attachment #8807466 -
Flags: review?(hiikezoe) → review+
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.
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(boris.chiou)
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?
Attachment #8807467 -
Flags: review?(hiikezoe) → review+
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
Attachment #8808086 -
Flags: review?(hiikezoe) → review+
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.
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
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 43•8 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!
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Comment 44•8 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?
Flags: needinfo?(cmills)
Comment 45•8 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?
Flags: needinfo?(cmills)
Comment 46•8 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
Reporter | ||
Comment 47•8 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.
Comment 48•8 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
Comment 49•8 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•