Hiroyuki Ikezoe (:hiro) Reporter
	Description • 7 years ago

For example in 'single_transition_reversed' [1], on stylo firstAnimation in removed entry is recorded in a different entry of the record of secondAnimation.
I haven't dug into detail yet.


[1] https://hg.mozilla.org/mozilla-central/file/d50abca6521b/dom/animation/test/chrome/test_animation_observers_async.html#l327

	Cameron McCormack (:heycam) Assignee
	Comment 1 • 7 years ago

My guess is that we don't have an nsAutoAnimationMutationBatch on the stack when we are processing the animation sequential task, the equivalent of the one here:

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/layout/style/nsTransitionManager.cpp#582

	Cameron McCormack (:heycam) Assignee
	Comment 4 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7430f4f99d4cd51af4026aeae2af185d79883dac&group_state=expanded

	Brian Birtles (:birtles)
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8889305 [details]
Bug 1376248 - Part 1: Use an nsAutoAnimationMutationBatch when updating animations for an element in stylo.

https://reviewboard.mozilla.org/r/160370/#review165990

	Brian Birtles (:birtles)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8889306 [details]
Bug 1376248 - Part 2: Allow tree_ordering sub-test to generate records in any order when simultaneously starting animations.

https://reviewboard.mozilla.org/r/160372/#review165992

::: commit-message-ccf18:1
(Diff revision 1)
> +Bug 1376248 - Part 2: Allow tree_ordering sub-test to generate records in any order for elements simultaneously starting animations. r?birtles

(This sentence reads a little odd -- is the 'elements' necessary? Or is it missing a 'with')

::: dom/animation/test/chrome/test_animation_observers_async.html:152
(Diff revision 1)
> +function assert_records_any_order(expected, desc) {
> +  // Generate a unique label for each Animation object.
> +  let animation_labels = new Map();
> +  let animation_counter = 0;
> +  for (let record of gRecords) {
> +    for (let a of [...record.addedAnimations, ...record.changedAnimations, ...record.removedAnimations]) {

Nit: I think we prefer 80 char line length for JS

::: dom/animation/test/chrome/test_animation_observers_async.html:166
(Diff revision 1)
> +  function record_label(record) {
> +    // Generate a label of the form:
> +    //
> +    //   <added-animations>:<changed-animations>:<removed-animations>
> +    let added   = record.addedAnimations   || record.added;
> +    let changed = record.changedAnimations || record.changed;
> +    let removed = record.removedAnimations || record.removed;
> +    return [added  .map(a => animation_labels.get(a)).sort().join(),
> +            changed.map(a => animation_labels.get(a)).sort().join(),
> +            removed.map(a => animation_labels.get(a)).sort().join()]
> +           .join(":");
> +  }
> +
> +  // Sort records by their label.
> +  gRecords.sort((a, b) => record_label(a) < record_label(b));
> +  expected.sort((a, b) => record_label(a) < record_label(b));
> +
> +  // Assert the sorted record lists are equal.
> +  assert_records(expected, desc);

For my own notes, using this scheme we'll get labels that look like, e.g.

  "1112::"

Which *must* mean 11,12, or 1,112, or 1112, because we sort the labels before concatenating them (so it can't mean 1,11,2, for example).

And even if these values are ambiguous there's probably never going to be a case where we accidentally pass (since it's unlikely we'd have a missing/additional value which just happened to cause us to create an ambiguous value that happened to be what we're looking for).

	Brian Birtles (:birtles)
	Comment 7 • 7 years ago

I thought I remembered that smaug had concerns about the ordering of these records being indeterminate, but I suspect that was actually about event dispatch, not mutation observer records.

	Cameron McCormack (:heycam) Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8889306 [details]
Bug 1376248 - Part 2: Allow tree_ordering sub-test to generate records in any order when simultaneously starting animations.

https://reviewboard.mozilla.org/r/160372/#review165992

> For my own notes, using this scheme we'll get labels that look like, e.g.
> 
>   "1112::"
> 
> Which *must* mean 11,12, or 1,112, or 1112, because we sort the labels before concatenating them (so it can't mean 1,11,2, for example).
> 
> And even if these values are ambiguous there's probably never going to be a case where we accidentally pass (since it's unlikely we'd have a missing/additional value which just happened to cause us to create an ambiguous value that happened to be what we're looking for).

Array.join() with no argument will use commas to join the elements, so we shouldn't have this ambiguity.

	Brian Birtles (:birtles)
	Comment 9 • 7 years ago

(In reply to Cameron McCormack (:heycam) from comment #8)
> Array.join() with no argument will use commas to join the elements, so we
> shouldn't have this ambiguity.

Oh, I didn't know that.

	Cameron McCormack (:heycam) Assignee
	Comment 12 • 7 years ago

Oh, forgot to address the line length comment before hitting land.  (Oh well, there are other lines as long in that file...)

	Pulsebot
	Comment 13 • 7 years ago

Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f98953cb41ef
Part 1: Use an nsAutoAnimationMutationBatch when updating animations for an element in stylo. r=birtles
https://hg.mozilla.org/integration/autoland/rev/8b5b751e81df
Part 2: Allow tree_ordering sub-test to generate records in any order when simultaneously starting animations. r=birtles

	Carsten Book [:Tomcat]
	Comment 14 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f98953cb41ef
https://hg.mozilla.org/mozilla-central/rev/8b5b751e81df