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

RESOLVED FIXED in Firefox 56

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hiro, Assigned: heycam)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 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
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 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
Attachment #8889305 - Flags: review?(bbirtles) → review+

Comment 6

2 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).
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.
(Assignee)

Comment 8

2 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.
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Oh, forgot to address the line length comment before hitting land.  (Oh well, there are other lines as long in that file...)

Comment 13

2 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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f98953cb41ef
https://hg.mozilla.org/mozilla-central/rev/8b5b751e81df
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Reporter)

Updated

2 years ago
Blocks: 1388209
You need to log in before you can comment on or make changes to this bug.