Write a test for mutation observer observes records when iterationComposite is changed

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: hiro, Assigned: gabrielle.singhcadieux, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

3 years ago
In bug 1216843, I did totally forget about Mutation observers, I am going to notify the animation whose iterationComposite is changed to Mutation observers in that bug, but I'd leave the test case intentionally.  That's because it can be a candidate for a hackathon that we will hold in this August in Japan.
(Reporter)

Updated

3 years ago
Summary: Write a test test for mutation observer observes records when iterationComposite is changed → Write a test for mutation observer observes records when iterationComposite is changed
Priority: -- → P3
(Reporter)

Comment 1

3 years ago
Set spacing test case in test_animation_observers.html will be a reference.

http://hg.mozilla.org/mozilla-central/file/82d0a583a9a3/dom/animation/test/chrome/test_animation_observers.html#l1970
(Reporter)

Updated

2 years ago
Mentor: boris.chiou
Keywords: good-first-bug
(Assignee)

Comment 2

2 years ago
Hi,
I would like to write this test as part of a university assignment.
(Reporter)

Comment 3

2 years ago
Thank you!  The university is really nice. ;-)

You will need to add a test case in test_animation_observers.html, and as I commented #1, the test case for spacing will be used as a reference.  You will need to modify iterationComposite[1] instead of spacing.

[1] https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly/iterationComposite

Thank you!
Assignee: nobody → gabrielle.singhcadieux
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
I just wanted to confirm that test_animation_observers.html has now been split into test_animation_observers_sync.html and test_animation_observers_async.html, and that the spacing test is now
https://hg.mozilla.org/mozilla-central/file/tip/dom/animation/test/chrome/test_animation_observers_sync.html#l455
(Reporter)

Comment 5

2 years ago
That's right.  You can add a test for iterationComposite in test_animation_observers_sync.html.
(Assignee)

Comment 6

2 years ago
Posted patch v1 of patch (obsolete) — Splinter Review
(Assignee)

Comment 7

2 years ago
Posted patch v1 of patch, resubmitted (obsolete) — Splinter Review
I am resubmitting this patch because I don't think I specifically requested review when I first submitted it.
Attachment #8854705 - Attachment is obsolete: true
Attachment #8863153 - Flags: review?(boris.chiou)
Comment on attachment 8863153 [details] [diff] [review]
v1 of patch, resubmitted

Review of attachment 8863153 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: dom/animation/test/chrome/test_animation_observers_sync.html
@@ +949,5 @@
> +  var anim = new Animation();
> +  anim.effect = new KeyframeEffect(div,
> +                                   { opacity: [ 0, 1 ] },
> +                                   { duration: 100 * MS_PER_SEC,
> +                                     iterationComposite: replace });

nit:
I think you can write this as:

var anim = div.animate({ opacity: [ 0, 1 ]},
                       { duration: 100 * MS_PER_SEC, 
                         iterationComposite: replace });

This is much simpler. However, your current implementation is also fine.
Attachment #8863153 - Flags: review?(boris.chiou) → review+
Comment on attachment 8863153 [details] [diff] [review]
v1 of patch, resubmitted

Review of attachment 8863153 [details] [diff] [review]:
-----------------------------------------------------------------

BTW, before marking "check-in needed", you have to revise the commit message as something like this:

Bug 1293495 - Write a test for mutation observer when iterationComposite is chenaged. r=boris
(In reply to Boris Chiou [:boris] from comment #8)
> Comment on attachment 8863153 [details] [diff] [review]
> v1 of patch, resubmitted
> 
> Review of attachment 8863153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: dom/animation/test/chrome/test_animation_observers_sync.html
> @@ +949,5 @@
> > +  var anim = new Animation();
> > +  anim.effect = new KeyframeEffect(div,
> > +                                   { opacity: [ 0, 1 ] },
> > +                                   { duration: 100 * MS_PER_SEC,
> > +                                     iterationComposite: replace });
> 
> nit:
> I think you can write this as:
> 
> var anim = div.animate({ opacity: [ 0, 1 ]},
>                        { duration: 100 * MS_PER_SEC, 
>                          iterationComposite: replace });
> 
> This is much simpler. However, your current implementation is also fine.

Should that be iterationComposite: 'replace' (i.e. with quotes)?

I'm actually surprised this doesn't throw.
(In reply to Brian Birtles (:birtles) from comment #10)
> > I think you can write this as:
> > 
> > var anim = div.animate({ opacity: [ 0, 1 ]},
> >                        { duration: 100 * MS_PER_SEC, 
> >                          iterationComposite: replace });
> > 
> > This is much simpler. However, your current implementation is also fine.
> 
> Should that be iterationComposite: 'replace' (i.e. with quotes)?

Oh yes. Gabrielle, could you please add quotes for 'replace'? Thanks.

(PS. We use both single and double quotes in this file, so we may need to make them consistent in the future.)

> 
> I'm actually surprised this doesn't throw.

Looks like webidl binding code doesn't throw for this, and treat this a default value, so we get this enum directly without any error handling.
Flags: needinfo?(gabrielle.singhcadieux)
(Assignee)

Comment 12

2 years ago
(In reply to Boris Chiou [:boris] from comment #11)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > > I think you can write this as:
> > > 
> > > var anim = div.animate({ opacity: [ 0, 1 ]},
> > >                        { duration: 100 * MS_PER_SEC, 
> > >                          iterationComposite: replace });
> > > 
> > > This is much simpler. However, your current implementation is also fine.
> > 
> > Should that be iterationComposite: 'replace' (i.e. with quotes)?
> 
> Oh yes. Gabrielle, could you please add quotes for 'replace'? Thanks.
> 
> (PS. We use both single and double quotes in this file, so we may need to
> make them consistent in the future.)
> 
> > 
> > I'm actually surprised this doesn't throw.
> 
> Looks like webidl binding code doesn't throw for this, and treat this a
> default value, so we get this enum directly without any error handling.

Will do! I was simply following the example in the API:
https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly/iterationComposite
Flags: needinfo?(gabrielle.singhcadieux)
(Assignee)

Comment 13

2 years ago
Posted patch v2 of patch (obsolete) — Splinter Review
Attachment #8863153 - Attachment is obsolete: true
Attachment #8863775 - Flags: review?(boris.chiou)
(Assignee)

Comment 14

2 years ago
Attachment #8863775 - Attachment is obsolete: true
Attachment #8863775 - Flags: review?(boris.chiou)
Attachment #8863777 - Flags: review?(boris.chiou)
(Assignee)

Comment 15

2 years ago
Sorry to submit this multiple times - I realized that I didn't include all the changes in my previously submitted patch file!
Attachment #8863782 - Flags: review?(boris.chiou)
(Assignee)

Updated

2 years ago
Attachment #8863777 - Attachment is obsolete: true
Attachment #8863777 - Flags: review?(boris.chiou)
Comment on attachment 8863782 [details] [diff] [review]
v2 of patch, includes all changes

Review of attachment 8863782 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating.
Attachment #8863782 - Flags: review?(boris.chiou) → review+
(In reply to Boris Chiou [:boris] from comment #17)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0c6b42ce036ca13123cdf2341e12261994939da0

wait for try
(In reply to Boris Chiou [:boris] from comment #18)
> (In reply to Boris Chiou [:boris] from comment #17)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=0c6b42ce036ca13123cdf2341e12261994939da0
> 
> wait for try

Test failed:

TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_animation_observers_sync.html | set_iterationComposite - set_iterationComposite: aOptions is not defined
Comment on attachment 8863782 [details] [diff] [review]
v2 of patch, includes all changes

Review of attachment 8863782 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/test/chrome/test_animation_observers_sync.html
@@ +962,5 @@
> +  anim.effect.composite = accumulate;
> +  assert_equals_records(observer.takeRecords(),
> +    [], "no record after setting the same iterationComposite");
> +
> +}, "set_iterationComposite");

Could you please move this test into this place [1], i.e. just after "animtion_order_change", because you use "aOptions", which is defined the that block.

By the way, you can verify this test locally by
  './mach mochitest dom/animation/test/chrome/test_animation_observers_sync.html'
to check if the new added one is passed. 

[1] http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/dom/animation/test/chrome/test_animation_observers_sync.html#819
(Assignee)

Comment 21

2 years ago
Posted patch v3 of patchSplinter Review
I moved the test to the appropriate place (sorry for not catching that before!), and fixed two bugs in the test: the use of `composite` rather than `iterationComposite` for the 3rd assertion, and the use of the `accumulate` enum value.
Attachment #8863782 - Attachment is obsolete: true
Attachment #8864700 - Flags: review?(boris.chiou)
Comment on attachment 8864700 [details] [diff] [review]
v3 of patch

Review of attachment 8864700 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.
Attachment #8864700 - Flags: review?(boris.chiou) → review+
Keywords: checkin-needed

Comment 24

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac39d8cf027
Wrote a test for mutation observer when iterationComposite is changed. r=boris
Keywords: checkin-needed

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fac39d8cf027
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.