Closed Bug 1376248 Opened 6 years ago Closed 5 years ago

stylo: Records by animation observer seem to be split into several entries

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: hiro, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
Priority: -- → P2
Assignee: nobody → cam
Status: NEW → ASSIGNED
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
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
Attachment #8889305 - Flags: review?(bbirtles) → 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).
Attachment #8889306 - Flags: review?(bbirtles) → review+
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.
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.
(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.
Oh, forgot to address the line length comment before hitting land.  (Oh well, there are other lines as long in that file...)
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
https://hg.mozilla.org/mozilla-central/rev/f98953cb41ef
https://hg.mozilla.org/mozilla-central/rev/8b5b751e81df
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1388209
You need to log in before you can comment on or make changes to this bug.