Closed
Bug 1345356
Opened 8 years ago
Closed 8 years ago
Update a few comments about animation handling
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox55 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
Details
Attachments
(3 files)
In investigating bug 1192592 I discovered a few comments in our codebase concerning animations that are out of date.
|
Assignee | |
Comment 1•8 years ago
|
||
The first change is that we have a comment saying that in GeckoRestyleManager::UpdateOnlyAnimationStyles() we ideally need to only flush throttled animations. I believe that predates our current setup where we do the animation restyle first. To prove we need to flush all animation restyles I wrote a patch where we only flush throttled animation restyles. I've sent it to try now[1] but I've already confirmed locally that with this patch test_animations_omta.html fails (and not due to some other change in this patch--if I simply drop the check for !iter.Data() the test passes). [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae5f6c42f6ff70cc4365a4df5e51d79e0127cdbc
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 5•8 years ago
|
||
Comment on attachment 8844767 [details] [diff] [review] Patch to only flush throttled animations (not to land -- this is just to prove this approach doesn't work) Review of attachment 8844767 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/EffectCompositor.cpp @@ +606,3 @@ > > + elementSet.Remove(pseudoElem); > + // Note: The pseudoElem.mElement pointer in elementsToRestyle might now You can use Iter(). https://hg.mozilla.org/mozilla-central/file/58753259bfeb/xpcom/ds/nsTHashtable.h#l195
|
|
Assignee | |
Comment 6•8 years ago
|
||
That patch is not supposed to be checked in. It's throwaway code to prove that that approach doesn't work.
|
|
Assignee | |
Comment 7•8 years ago
|
||
(And I'm pretty sure we can't use Iter() since the whole point of copying it into a separate array is because we're mutating the contents of the hashmap as we iterate over it in ways other than just deleting items. It's the separate array where pointers might dangle.)
|
||
Comment 8•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8844772 [details] Bug 1345356 - Drop FIXME comment about only needing to flush throttled animations; https://reviewboard.mozilla.org/r/118098/#review119956
Attachment #8844772 -
Flags: review?(cam) → review+
|
|
||
Comment 9•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8844773 [details] Bug 1345356 - Update references to CheckAnimationRule to refer to UpdateAnimations; https://reviewboard.mozilla.org/r/118100/#review119958
Attachment #8844773 -
Flags: review?(cam) → review+
|
||
Comment 10•8 years ago
|
||
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3a871855cd Drop FIXME comment about only needing to flush throttled animations; r=heycam DONTBUILD (comment-only) https://hg.mozilla.org/integration/mozilla-inbound/rev/f14c95735c48 Update references to CheckAnimationRule to refer to UpdateAnimations; r=heycam DONTBUILD (comment-only)
|
||
Comment 11•8 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/bf3a871855cd https://hg.mozilla.org/mozilla-central/rev/f14c95735c48
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•