Closed
Bug 1179627
Opened 9 years ago
Closed 8 years ago
Implement Animation.id
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: birtles, Assigned: chikoski)
References
()
Details
Attachments
(2 files, 14 obsolete files)
4.59 KB,
patch
|
chikoski
:
review+
|
Details | Diff | Splinter Review |
2.88 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
This is really just for authors, it doesn't have any functionality. To that extent it seems like we could just rely on expando properties but, as currently specced, it should return an empty string as opposed to undefined when not set. It should also, I suppose, do error-checking on setting to something other than a string. I wonder if there's a way to do that without adding a string member to every instance of Animation.
Reporter | ||
Comment 1•8 years ago
|
||
I spoke to heycam about this on IRC and we think it's probably not worth trying to avoid the extra string member. I was curious if there was a means of using the storage of [Cached] attributes to store the string but it seems like the wrapper won't be generated in time for this when constructing Animation objects using the constructor. If this does prove to be something we want to optimize we could have a static hashmap for associating Animations with ids.
Comment 2•8 years ago
|
||
I wrote test file. See the attachment files.
Comment 3•8 years ago
|
||
Comment 4•8 years ago
|
||
Thank you, white-hawk-taka! Next time you should upload files as diff files. See https://developer.mozilla.org/ja/docs/Creating_a_patch
Comment 5•8 years ago
|
||
Attachment #8695368 -
Attachment is obsolete: true
Comment 6•8 years ago
|
||
Attachment #8695635 -
Attachment is obsolete: true
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8695636 [details] file_animation-identify.html Thanks for looking into this! Here are a few suggestions for the test. >test(function(t) { > var div = addDive(t); addDiv > div.style.animation = 'anim 100s'; 'abc 100s' (The animation-name part, 'anim' here, should refer to the name of the @keyframes rule) > var animation = div.getAnimation()[0]; getAnimations (with an 's' at the end) > animation.id = 'anim' > > assert_equals(animation.id, 'anim', > 'animation.id is the show it'); "animation.id reflects the value set" We should also add a test for the initial value.
Comment 8•8 years ago
|
||
I have fix and make the patch.
Reporter | ||
Comment 9•8 years ago
|
||
(In reply to white-hawk-taka from comment #8) > Created attachment 8695731 [details] [diff] [review] > bug1179627.patch > > I have fix and make the patch. Looks good! It would be good to also add a test that animation.id is initially ''. Once you have done that, please request review on the patch. You can do that when you attach the patch or from the "Details" page later. Set the 'review' section to '?' and then choose either Hiro (type ':hiro' and it will autocomplete his email address) or me (type ':birtles').
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to white-hawk-taka from comment #8) > Created attachment 8695731 [details] [diff] [review] > bug1179627.patch > > I have fix and make the patch. I have created a patch to implement Animation.id. By the way, your patch causes a build error. Could you fix it and finish your code? Or can I do that?
Assignee | ||
Comment 11•8 years ago
|
||
This patch implemented Animation.id
Attachment #8697304 -
Flags: review?(bbirtles)
Reporter | ||
Comment 12•8 years ago
|
||
Comment on attachment 8697304 [details] [diff] [review] animationIdImpl.patch Review of attachment 8697304 [details] [diff] [review]: ----------------------------------------------------------------- This looks good but I'd like to have another look after adding the mutation observer notifications. ::: dom/animation/Animation.cpp @@ +48,5 @@ > // > // --------------------------------------------------------------------------- > > void > +Animation::GetId(nsAString& aResult) I think we could just write GetId these as an inline method in the header file. It would make maintaining this code a little easier. @@ +56,5 @@ > + > +void > +Animation::SetId(const nsAString& aId) > +{ > + mId = aId; I think this needs to notify mutation observers (and we need a test for this too). i.e. nsNodeUtils::AnimationChanged(this) We should probably compare if mId and aId are equal as well, in order to prevent redundant notifications. @@ +57,5 @@ > +void > +Animation::SetId(const nsAString& aId) > +{ > + mId = aId; > +} This line adds trailing whitespace. ::: dom/animation/Animation.h @@ +59,5 @@ > , mPendingState(PendingState::NotPending) > , mAnimationIndex(sNextAnimationIndex++) > , mFinishedAtLastComposeStyle(false) > , mIsRelevant(false) > + , mFinishedIsResolved(false) This line adds trailing white whitespace. Please fix. @@ +88,5 @@ > Continue > }; > > // Animation interface methods > + void GetId(nsAString& aResult); I think this can be a const method. ::: dom/webidl/Animation.webidl @@ +14,5 @@ > > [Func="nsDocument::IsWebAnimationsEnabled"] > interface Animation : EventTarget { > + attribute DOMString id; > + // Bug 1049975: Make 'effect' writeable This line adds extra trailing whitespace. Please fix. This also needs review from a DOM peer (e.g. smaug).
Attachment #8697304 -
Flags: review?(bbirtles)
Assignee | ||
Comment 13•8 years ago
|
||
Thank you for reviewing. This new patch includes: - Animation::SetId modification to call nsNodeUtils::AnimationChanged when Animation.id is changed - Test code for mutation observer call in Animation::SetId - removal several trail spaces
Attachment #8697304 -
Attachment is obsolete: true
Attachment #8697354 -
Flags: review?(bbirtles)
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8697354 [details] [diff] [review] animationIdImpl.patch (updated with review comment) Review of attachment 8697354 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand how the tests pass without generating an added record. ::: dom/animation/Animation.cpp @@ +54,5 @@ > + return; > + } > + mId = aId; > + nsNodeUtils::AnimationChanged(this); > +} Trailing space here. ::: dom/animation/test/chrome/test_animation_observers.html @@ +802,5 @@ > childA.remove(); > childB.remove(); > }); > > +addAsyncAnimTest("chaging id causes mutation observer notification", {}, function*() { This line is over 80 chars long. @@ +804,5 @@ > }); > > +addAsyncAnimTest("chaging id causes mutation observer notification", {}, function*() { > + // Start an animation that should already be finished. > + div.style = "animation: anim 1s;"; The comment doesn't match the code here. Also, why not just have: div.style.animation = "anim 1s"; @@ +807,5 @@ > + // Start an animation that should already be finished. > + div.style = "animation: anim 1s;"; > + > + // The animation should cause no Animations to be created. > + var animation = div.getAnimations()[0]; The comment doesn't match the code here. @@ +810,5 @@ > + // The animation should cause no Animations to be created. > + var animation = div.getAnimations()[0]; > + animations.id = "new id"; > + assert_records([{ added: [], changed: [animation], removed: []}], > + "records after id is changed"); Shouldn't this create an added record? @@ +815,5 @@ > + > + div.style = ""; > +}); > + > +addAsyncAnimTest("asigning same value with id does not cause mutation observer notification", {}, function*() { This line is longer than 80 characters. @@ +817,5 @@ > +}); > + > +addAsyncAnimTest("asigning same value with id does not cause mutation observer notification", {}, function*() { > + // Start an animation that should already be finished. > + div.style = "animation: anim 1s;"; Again, the comment doesn't match the code. @@ +826,5 @@ > + assert_records([{ added: [], changed: [animation], removed: []}], > + "records after id is changed"); > + animations.id = "new id"; > + assert_records([{ added: [], changed: [], removed: []}], > + "records after assigning same value with id"); I think we may as well just combine this with the previous test. @@ +829,5 @@ > + assert_records([{ added: [], changed: [], removed: []}], > + "records after assigning same value with id"); > + div.style = ""; > +}); > + A lot of trailing space in this file.
Attachment #8697354 -
Flags: review?(bbirtles)
Assignee | ||
Comment 15•8 years ago
|
||
Thank you for reviewing. As your comment, my test did not work well. I updated my patch to fix broken test cases.
Attachment #8697643 -
Flags: review?(bbirtles)
Reporter | ||
Comment 16•8 years ago
|
||
Comment on attachment 8697643 [details] [diff] [review] animationIdImpl.patch Review of attachment 8697643 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. adding r?smaug for WebIDL changes ::: dom/animation/test/chrome/test_animation_observers.html @@ +802,5 @@ > childA.remove(); > childB.remove(); > }); > > +addAsyncAnimTest("chaging id", { observe: div, subtree: true }, function*() { "change_id" We should move this test inside the big loop that tests observing the div or its parent (starting on line 168). Perhaps right before, "change_currentTime_while_pause_pending", at about line 765. @@ +814,5 @@ > + yield await_frame(); > + assert_records([{ added: [], changed: [animation], removed: []}], > + "records after id is changed"); > + > + div.style.animation = ""; We should wait for the removal record too. @@ +817,5 @@ > + > + div.style.animation = ""; > +}); > + > +addAsyncAnimTest("asigning same value with id", { observe: div, subtree: true }, function*() { "assigning" This line is over 80 characters. @@ +833,5 @@ > + yield await_frame(); > + assert_records([], > + "records after assigning same value with id"); > + div.style.animation = ""; > +}); I think we can combine these two tests into one since the first half is identical.
Attachment #8697643 -
Flags: review?(bugs)
Attachment #8697643 -
Flags: review?(bbirtles)
Attachment #8697643 -
Flags: review+
Comment 17•8 years ago
|
||
Comment on attachment 8697643 [details] [diff] [review] animationIdImpl.patch + attribute DOMString id; We seem to not follow the specs whitespace handling anyway here, so just don't add those extra spaces.
Attachment #8697643 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 18•8 years ago
|
||
r=bbirtles@mozilla.com Thank you for reviewing. I updated my patch. > We should move this test inside the big loop that tests observing the div or > its parent (starting on line 168). Perhaps right before, > "change_currentTime_while_pause_pending", at about line 765. I could not find the test you mentioned, so I moved "change_id" to the end of the big loop (L739).
Attachment #8697354 -
Attachment is obsolete: true
Attachment #8697643 -
Attachment is obsolete: true
Attachment #8698357 -
Flags: review?(bugs)
Comment 19•8 years ago
|
||
chiko, you don't need to ask to review anymore because your patch has got review+ by Brian and Olli. (In reply to chikoski from comment #18) > > We should move this test inside the big loop that tests observing the div or > its parent (starting on line 168). Perhaps right before, > > "change_currentTime_while_pause_pending", at about line 765. > > I could not find the test you mentioned, so I moved "change_id" to the end > of the big loop (L739). Your local tree must be older than the current tip. The position is here. https://dxr.mozilla.org/mozilla-central/source/dom/animation/test/chrome/test_animation_observers.html#765
Comment 20•8 years ago
|
||
white-hawk-taka, can you please finish attachment 8695731 [details] [diff] [review] up?
Flags: needinfo?(white-hawk-taka)
Comment 21•8 years ago
|
||
Comment on attachment 8698357 [details] [diff] [review] animationIdImpl.patch r+ for the .webidl
Attachment #8698357 -
Flags: review?(bugs) → review+
Comment 22•8 years ago
|
||
This patch has id init test. Please review it.
Attachment #8695731 -
Attachment is obsolete: true
Flags: needinfo?(white-hawk-taka)
Attachment #8699845 -
Flags: review?(bbirtles)
Comment 23•8 years ago
|
||
Comment on attachment 8699845 [details] [diff] [review] bug1179627.patch Review of attachment 8699845 [details] [diff] [review]: ----------------------------------------------------------------- Please include corresponding bug number in commit message like this: Bug 1179627 - Part 2: Automation tests for animation.id. ::: dom/animation/test/css-animations/file_animation-identify.html @@ +15,5 @@ > + var animation = div.getAnimations()[0]; > + > + //initial > + assert_equals(animation.id, '', > + 'animation.id has initial value'); Please remove trailing space. @@ +20,5 @@ > + > + animation.id = 'anim' > + > + assert_equals(animation.id, 'anim', > + 'animation.id relects the value set'); reflects. ::: dom/animation/test/mochitest.ini @@ +40,5 @@ > skip-if = buildapp == 'mulet' > support-files = css-animations/file_element-get-animations.html > [css-animations/test_timeline-get-animations.html] > +[css-animations/test_animation-identify.html] > +support-files = css-animations/file_animation-identify.html These two lines should not be between test_timeline-get-animations.html and file_timeline-get-animations.html.
Comment 24•8 years ago
|
||
Thanks review. I fix it. Please review again.
Attachment #8700264 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 25•8 years ago
|
||
r=bbirtles@mozilla.com r=bugs@pettay.fi Thank you all. I updated my patch to - move added test case("change_id") to appropriate position in test_animation_observer.html - remove bug fix of mochitest.ini because it will be contained in bug1179627.patch
Attachment #8698357 -
Attachment is obsolete: true
Comment 26•8 years ago
|
||
Comment on attachment 8700264 [details] [diff] [review] bug1179627.patch r=me with the commit log change as I commented in comment#23. I'd actually prefer that file name matches to API name, say, file_animation-id.html, but I'd like to hear Brian's opinion.
Attachment #8700264 -
Flags: review?(hiikezoe)
Attachment #8700264 -
Flags: review?(bbirtles)
Attachment #8700264 -
Flags: review+
Updated•8 years ago
|
Attachment #8699845 -
Attachment is obsolete: true
Attachment #8699845 -
Flags: review?(bbirtles)
Comment 27•8 years ago
|
||
Comment on attachment 8700283 [details] [diff] [review] animationIdImpl.patch Review of attachment 8700283 [details] [diff] [review]: ----------------------------------------------------------------- chiko, once you got r+, you can set r+ flag by yourself when you attach revised patches. ::: dom/animation/test/chrome/test_animation_observers.html @@ +1392,1 @@ > }); Here is an extra line. You should remove it.
Reporter | ||
Comment 28•8 years ago
|
||
Comment on attachment 8700264 [details] [diff] [review] bug1179627.patch Review of attachment 8700264 [details] [diff] [review]: ----------------------------------------------------------------- As Hiro suggests, please rename this to test_animation-id.html and file_animation-id.html. r=birtles with comments addressed and the files renamed. ::: dom/animation/test/css-animations/file_animation-identify.html @@ +15,5 @@ > + var animation = div.getAnimations()[0]; > + > + //initial > + assert_equals(animation.id, '', > + 'animation.id has initial value'); No need for the comment, and I think this fits on one line within 80 characters so you can replace these 3 lines with just: assert_equals(animation.id, '', 'id for CSS Animation is initially empty'); @@ +21,5 @@ > + animation.id = 'anim' > + > + assert_equals(animation.id, 'anim', > + 'animation.id reflects the value set'); > + }, 'Returned CSS animations have the correct Animation.id'); 'animation.id for CSS Animations'
Attachment #8700264 -
Flags: review?(bbirtles) → review+
Comment 29•8 years ago
|
||
Thanks review. File name has changed, exclude comment and fix lines under 80 characters. Please review again.
Attachment #8700264 -
Attachment is obsolete: true
Attachment #8700447 -
Flags: review?(hiikezoe)
Attachment #8700447 -
Flags: review?(bbirtles)
Reporter | ||
Comment 30•8 years ago
|
||
Comment on attachment 8700447 [details] [diff] [review] bug1179627.patch Looks perfect! Thanks! By the way, when you receive a comment like "r=me with comments addressed", you can just make the changes and then mark the patch as review+ by yourself. You don't need to request review again unless it has changed a lot or there is something you are uncertain about.
Attachment #8700447 -
Flags: review?(hiikezoe)
Attachment #8700447 -
Flags: review?(bbirtles)
Attachment #8700447 -
Flags: review+
Comment 31•8 years ago
|
||
Thank you Shirotaka_Pro and chiko! I just pushed a try with a bug number in part 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd8a1e831ee5
Comment 32•8 years ago
|
||
Thanks for the test! test_animation_observers.html failed on the try. https://treeherder.mozilla.org/logviewer.html#?job_id=14769756&repo=try I think we should use longer duration instead of 1s. chiko, please change the duration and update the patch?
Assignee | ||
Comment 33•8 years ago
|
||
Thanks, Hiro. I updated my patch with your comment.
Attachment #8700283 -
Attachment is obsolete: true
Attachment #8701355 -
Flags: review+
Comment 34•8 years ago
|
||
Comment on attachment 8701355 [details] [diff] [review] animationIdImpl.patch Review of attachment 8701355 [details] [diff] [review]: ----------------------------------------------------------------- Thanks chiko! I just push a new try with 100s duration. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0ade35449e0 Please update your patch again! ::: dom/animation/test/chrome/test_animation_observers.html @@ +762,5 @@ > "records after animation end"); > }); > }); > > + addAsyncAnimTest("chage_id", aOptions, function*() { chage -> change. @@ +763,5 @@ > }); > }); > > + addAsyncAnimTest("chage_id", aOptions, function*() { > + e.style.animation = "anim 1s"; You still need to change the duration to 100s to avoid intermittent failures on slower platforms.
Assignee | ||
Comment 35•8 years ago
|
||
Thanks Hiro. I have just updated my patch.
Attachment #8701355 -
Attachment is obsolete: true
Attachment #8705602 -
Flags: review+
Comment 36•8 years ago
|
||
Added bug number.
Attachment #8695362 -
Attachment is obsolete: true
Attachment #8695636 -
Attachment is obsolete: true
Attachment #8700447 -
Attachment is obsolete: true
Attachment #8705846 -
Flags: review+
Updated•8 years ago
|
Attachment #8705602 -
Attachment description: animationIdImpl.patch → part 1 - Implement Animation.id
Comment 37•8 years ago
|
||
Thank you, everyone! https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f40f36d220
Assignee: nobody → chikoski
Keywords: checkin-needed
Comment 38•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef66db4c4e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/be55d584f00a
Keywords: checkin-needed
Comment 39•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7ef66db4c4e9 https://hg.mozilla.org/mozilla-central/rev/be55d584f00a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•