Closed
Bug 1376248
Opened 7 years ago
Closed 7 years ago
stylo: Records by animation observer seem to be split into several entries
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
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
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7430f4f99d4cd51af4026aeae2af185d79883dac&group_state=expanded
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
Attachment #8889305 -
Flags: review?(bbirtles) → review+
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).
Attachment #8889306 -
Flags: review?(bbirtles) → review+
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.
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.
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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...)
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
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f98953cb41ef https://hg.mozilla.org/mozilla-central/rev/8b5b751e81df
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•