Closed Bug 1179627 Opened 9 years ago Closed 8 years ago

Implement Animation.id

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox42 --- affected
firefox46 --- fixed

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.
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.
Attached file test_animation-identify.html (obsolete) —
I wrote test file.
See the attachment files.
Attached file file_animation-identify.html (obsolete) —
Thank you, white-hawk-taka!
Next time you should upload files as diff files.
See https://developer.mozilla.org/ja/docs/Creating_a_patch
Attached file file_animation-identify.html (obsolete) —
Attachment #8695368 - Attachment is obsolete: true
Attached file file_animation-identify.html (obsolete) —
Attachment #8695635 - Attachment is obsolete: true
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.
Attached patch bug1179627.patch (obsolete) — Splinter Review
I have fix and make the patch.
(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').
(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?
Attached patch animationIdImpl.patch (obsolete) — Splinter Review
This patch implemented Animation.id
Attachment #8697304 - Flags: review?(bbirtles)
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)
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)
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)
Blocks: 1231945
Attached patch animationIdImpl.patch (obsolete) — Splinter Review
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)
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 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+
Attached patch animationIdImpl.patch (obsolete) — Splinter Review
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)
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
white-hawk-taka, can you please finish attachment 8695731 [details] [diff] [review] up?
Flags: needinfo?(white-hawk-taka)
Comment on attachment 8698357 [details] [diff] [review]
animationIdImpl.patch

r+ for the .webidl
Attachment #8698357 - Flags: review?(bugs) → review+
Attached patch bug1179627.patch (obsolete) — Splinter Review
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 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.
Attached patch bug1179627.patch (obsolete) — Splinter Review
Thanks review. I fix it. Please review again.
Attachment #8700264 - Flags: review?(hiikezoe)
Attached patch animationIdImpl.patch (obsolete) — Splinter Review
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 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+
Attachment #8699845 - Attachment is obsolete: true
Attachment #8699845 - Flags: review?(bbirtles)
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.
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+
Attached patch bug1179627.patch (obsolete) — Splinter Review
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)
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+
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
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?
Attached patch animationIdImpl.patch (obsolete) — Splinter Review
Thanks, Hiro.

I updated my patch with your comment.
Attachment #8700283 - Attachment is obsolete: true
Attachment #8701355 - Flags: review+
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.
Thanks Hiro.

I have just updated my patch.
Attachment #8701355 - Attachment is obsolete: true
Attachment #8705602 - Flags: review+
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+
Attachment #8705602 - Attachment description: animationIdImpl.patch → part 1 - Implement Animation.id
Thank you, everyone!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f40f36d220
Assignee: nobody → chikoski
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ef66db4c4e9
https://hg.mozilla.org/mozilla-central/rev/be55d584f00a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.