I originally saw this work as part of bug 960465, but I'm splitting it out into a separate bug because it can land sooner, and it's the part that's needed for bug 977991. As I wrote in https://groups.google.com/d/msg/mozilla.dev.tech.layout/0Ehe1BO5ixw/78F_kCoG3TYJ this is to: refactor the current miniflush code (code to update the animation styles to the current refresh driver time while leaving style data otherwise out-of-date, so that we can do correct style comparisons when deciding whether to start CSS transitions when we've suppressed style updates on the main thread while running animations on the compositor) to do coalescing using a RestyleTracker rather than their current ad-hoc tree-walking and coalescing code (which will also, I believe, fix some correctness bugs). This also means introducing the miniflush's ability to replace certain special style rules without rerunning selector matching into the RestyleTracker, which is useful in bug 977991. There are also a bunch of correctness fixes lurking in here that I discovered while doing the work.

More platforms at https://tbpl.mozilla.org/?tree=Try&rev=33ce7938c9f0

Comment on attachment 8447633 [details] [diff] [review] patch 1 - Perform a miniflush on both animations and transitions before processing restyles Review of attachment 8447633 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles We may need to add some small margin of error to the matrix comparisons when we turn on OMTA on Android (see bug 1029969) but that can happen later.

Comment on attachment 8447634 [details] [diff] [review] patch 2 - Change the public API to updating main-thread-suppressed animation styles (miniflush) in preparation for refactoring how it works Review of attachment 8447634 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles

Comment on attachment 8447635 [details] [diff] [review] patch 3 - Move the knowledge of when we last updated main-thread-suppressed animation styles into the restyle manager rather than have two separate but always equal timestamps for animations and transitions Review of attachment 8447635 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with possible comment tweak. ::: layout/base/nsPresContext.cpp @@ +1017,5 @@ > > // Initialise refresh tick counters for OMTA > + mLastStyleUpdateForAllAnimations = mRefreshDriver->MostRecentRefresh(); > + > + // Initialize after initializing the refresh driver. I don't understand this sentence. Initialize what?

(In reply to Brian Birtles (:birtles) from comment #31) > > + // Initialize after initializing the refresh driver. > > I don't understand this sentence. Initialize what? restyle manager after refresh driver; I'll fix the comment. It turns out that since I last tested, one of the todo()s in patch 1 has started passing (the second passes; first still fails). Not sure what fixed it, but there's still one failure (bug already filed and noted in patch). That said, I also discovered that patch 14 (which was the last patch I wrote) introduced some test failures (in test_animations_omta.html and test_animations_omta_start.html), I think because setting the elementForAnimation makes us recur back into the animation code. I need to figure out the right way to suppress that problem.

Comment on attachment 8447636 [details] [diff] [review] patch 4 - Add a new type of RestyleTracker for handling animation-only style flushes Review of attachment 8447636 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/Element.h @@ +87,5 @@ > + // it was). > + ELEMENT_HAS_PENDING_ANIMATION_ONLY_RESTYLE = ELEMENT_FLAG_BIT(4), > + > + // Set if the element is a potential animation-only restyle root (that > + // is, has an animation style change pending _and_ that style change s/animation/animation-only/

This removes each test element when we're done with it so that each successive test element isn't 100px lower. This is required to keep the third test element (added in the next patch) onscreen when running tests on the B2G emulator (other than when running a single test at a time). This, in turn, is required to get animations in that test properly shipped to the compositor thread, which is required for the test to pass.

This affects the correctness of transitions that take over from a running animation. (In the current code this can happen on a single element; once the cascading changes in bug 960465 are complete it can only happen via inheritance.) This version of the patch changes to do the test using opacity rather than transform, since testing using transforms encountered issues related to bug 1031688: the presence of phantom transitions due to the interaction of the computed value rules for transforms distinguishing between values that the interpolation rules consider identical. (These problems only appear after patch 24 in this bug changes the coalescing order between a parent with animations and a child with transitions so that the parent is handled before the child, instead of transitions being handled before animations.) The final two added tests fail without the patch and pass with the patch. (With the opacity version, the third to last test also fails without the patch, probably due to the value not having yet been sent to the layer. This was not an issue pre-patch with the original test using transform, though.)

Comment on attachment 8461804 [details] [diff] [review] patch 0 - Fix test_animations_omta_start.html so that additional tests will involve onscreen layers Review of attachment 8461804 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles ::: layout/style/test/test_animations_omta_start.html @@ +93,5 @@ > var transform = gUtils.getOMTAStyle(target, "transform"); > ok(matricesRoughlyEqual(convertTo3dMatrix(transform), > convertTo3dMatrix("matrix(1, 0, 0, 1, 50, 0)")), > "transform is set on compositor thread after delayed start"); > + target.parentNode.removeChild(target); This is fine, but I'm pretty sure we can now just do: target.remove() (https://developer.mozilla.org/en-US/docs/Web/API/ChildNode.remove)

Comment on attachment 8461805 [details] [diff] [review] patch 1 - Perform a miniflush on both animations and transitions before processing restyles Review of attachment 8461805 [details] [diff] [review]: ----------------------------------------------------------------- I was looking at this exact code yesterday wondering why we weren't updating animations! One reason we see different behavior for opacity as opposed to transforms is that getOMTAStyle behaves differently for each one. For transforms we have a flag on the layer to indicate if the transform was set by animation or not and we only return a value if that flag is set. For opacity we don't have such a flag so we return the value on the layer whether or not it was set by animation. The other main difference is, as the commit message points out, redundant transform changes getting skipped meaning the animation doesn't get sent to the compositor immediately in some cases. I'm still not sure why the third-to-last test passed with transforms but I agree that the the failure with opacity appears to be bug 1039799. r=birtles ::: layout/style/test/test_animations_omta_start.html @@ +157,5 @@ > + todo_is(opacity, "0.4", > + "transition that interrupted animation is correct"); > + gUtils.advanceTimeAndRefresh(0); > + waitForAllPaints(function() { > + var opacity = gUtils.getOMTAStyle(child, "opacity"); We already have "opacity" in this scope so "var" is unnecessary here (and below) but perhaps its more clear/portable if we have it? @@ +167,5 @@ > + is(opacity, "0.7", > + "transition that interrupted animation is correct"); > + is(childCS.opacity, "0.7", > + "transition that interrupted animation is correct"); > + parent.parentNode.removeChild(parent); parent.remove() if you prefer.

Landed patch 0 through patch 4: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5bed5f6da61c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa40edea3705 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/16065088f957 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f36e474edcd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/68f101ad08d2

https://hg.mozilla.org/mozilla-central/rev/5bed5f6da61c https://hg.mozilla.org/mozilla-central/rev/fa40edea3705 https://hg.mozilla.org/mozilla-central/rev/16065088f957 https://hg.mozilla.org/mozilla-central/rev/7f36e474edcd https://hg.mozilla.org/mozilla-central/rev/68f101ad08d2

Comment on attachment 8447637 [details] [diff] [review] patch 5 - Move the guts of UpdateThrottledStyle into nsStyleSet, where it can be reused Review of attachment 8447637 [details] [diff] [review]: ----------------------------------------------------------------- I see the FIXME is addressed in patch 12. (I wonder if the "TODO: REVIEW THIS CAREFULLY" commit message in patch 12 is aimed at yourself at me!)

Comment on attachment 8447639 [details] [diff] [review] patch 7 - Add new restyle types that replace only the data from CSS transitions or animations Review of attachment 8447639 [details] [diff] [review]: ----------------------------------------------------------------- Maybe s/a refactoring/part of a refactoring/ in the commit message correct, because just looking at the patch by itself it doesn't seem like a refactoring of those classes.

Comment on attachment 8447640 [details] [diff] [review] patch 8 - Pass the replacements through to ResolveStyleWithReplacement Review of attachment 8447640 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleSet.cpp @@ +1330,5 @@ > + NS_ABORT_IF_FALSE(!(aReplacements & ~(eRestyle_CSSTransitions | > + eRestyle_CSSAnimations)), > + nsPrintfCString("unexpected replacement bits 0x%lX", > + uint32_t(aReplacements)).get()); > + I have a DEBUG-only function that prints an nsRestyleHint symbolically that I should upload as part of bug 979133 and which could be used here, once bug 931668 is landed.

Comment on attachment 8447641 [details] [diff] [review] patch 9 - Make nsStyleSet::ResolveStyleWithReplacement handle changing between having and not having animation or transition rules, make it set IsImportantRule on rule nodes correctly, and merge the bogus ResolveStyleForRules into it Review of attachment 8447641 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleSet.cpp @@ +1324,5 @@ > + { nsStyleSet::eOverrideSheet, true, nsRestyleHint(0) }, > + { nsStyleSet::eUserSheet, true, nsRestyleHint(0) }, > + { nsStyleSet::eAgentSheet, true, nsRestyleHint(0) }, > + { nsStyleSet::eTransitionSheet, false, eRestyle_CSSTransitions }, > +}; Would be nice if we could use this table in FileRules, to keep the cascade order in sync automatically, though the XBL stuff and the scoped sheet handling might make that tricky. @@ +1358,5 @@ > + bool doReplace = level->mLevelReplacementHint & aReplacements; > + if (doReplace) { > + switch (level->mLevelReplacementHint) { > + case eRestyle_CSSAnimations: > + { Style guide says to write it this like: switch (...) { case ...: { ... break; }

Comment on attachment 8447644 [details] [diff] [review] patch 12 - Fix the visited rule node handling in ResolveStyleWithReplacement Review of attachment 8447644 [details] [diff] [review]: ----------------------------------------------------------------- Please add a test with this patch. Here's one you can adapt I was just trying locally in a Nightly without this patch queue applied, which I assume will work with the queue up to this patch applied (where work means it will transition from white to green instead of nearly-red to red): <!DOCTYPE html> <style> span { transition: 1s background-color; font: 48px sans-serif; } :link span { background-color: rgb(254, 0, 0); } :link span:hover { background-color: red; } :visited span { background-color: white; } :visited span:hover { background-color: green; } </style> <a href=""><span>some text here</span></a> The patch itself looks OK.

(In reply to Cameron McCormack (:heycam) from comment #47) > Please add a test with this patch. Here's one you can adapt I was just > trying locally in a Nightly without this patch queue applied, which I assume > will work with the queue up to this patch applied (where work means it will > transition from white to green instead of nearly-red to red): Or will we not be running this code yet because background-color isn't a property that gets animated on the compositor?

Comment on attachment 8447648 [details] [diff] [review] patch 16 - Add comment about potential performance impromevent to ResolveStyleWithReplacement Review of attachment 8447648 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleSet.cpp @@ +1341,5 @@ > > + // FIXME (perf): This should probably not rebuild the whole path, but > + // only the path from the last change in the rule tree, like > + // nsAnimationManager::CheckAnimationRule does. (That could then > + // perhaps share this code, too?) Which bit of nsAnimationManager::CheckAnimationRule are you referring to?

Comment on attachment 8447649 [details] [diff] [review] patch 17 - Add comment about how ResolveStyleWithReplacement interacts with nsTransitionManager and nsAnimationManager Review of attachment 8447649 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure the commit message matches the comments?

(In reply to Cameron McCormack (:heycam) from comment #49) > Comment on attachment 8447648 [details] [diff] [review] > patch 16 - Add comment about potential performance impromevent to > ResolveStyleWithReplacement ... > Which bit of nsAnimationManager::CheckAnimationRule are you referring to? Er, sorry, I really meant the ReplaceAnimationRule function in nsStyleSet. (In reply to Cameron McCormack (:heycam) from comment #50) > Comment on attachment 8447649 [details] [diff] [review] > patch 17 - Add comment about how ResolveStyleWithReplacement interacts with > nsTransitionManager and nsAnimationManager ... > I'm not sure the commit message matches the comments? How about: Add comment about how RuleNodeWithReplacement should interact with nsTransitionManager and nsAnimationManager. ?

Comment on attachment 8447652 [details] [diff] [review] patch 20 - Make restyling exact - Avoid rerunning selector matching on everything when the basis of rem units changes Review of attachment 8447652 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.cpp @@ +1394,5 @@ > + // because they're changing things that affect data computation rather > + // than selector matching; we could have a restyle hint passed in, and > + // substantially improve the performance of things like pref changes > + // and the restyling that we do for downloadable font loads. > + DoRebuildAllStyleData(mPendingRestyles, aExtraHint, eRestyle_Subtree); Can you file a bug to track this followup work? @@ +1419,5 @@ > } > > + if (aRestyleHint & ~eRestyle_Subtree) { > + // We want this hint to apply to the root node's primary frame > + // rather than the root frame. Can you explain why? @@ +1433,5 @@ > // (but note that nsPresShell::SetPreferenceStyleRules currently depends > // on us re-running rule matching here > nsStyleChangeList changeList; > // XXX Does it matter that we're passing aExtraHint to the real root > // frame and not the root node's primary frame? Is this comment obsoleted given the above change? @@ +2340,5 @@ > nsRestyleHint hintToRestore = nsRestyleHint(0); > + if (mContent && mContent->IsElement() && > + // If we're we're resolving from the root of the frame tree (which > + // we do in DoRebuildAllStyleData), we need to avoid getting the > + // root's restyle data until we get to its primary frame. Can you help me understand why that's important? @@ +2740,4 @@ > mFrame->StyleContext(), > undisplayed->mStyle, > thisChildHint); > + } else if (styleSet->IsInRuleTreeReconstruct()) { Can you assert somewhere that the restyle hint will be 0 when IsInRuleTreeReconstruct() is true?

Comment on attachment 8447656 [details] [diff] [review] patch 24 - Use a RestyleTracker for the coalescing in the animation-only style flush (miniflush) Review of attachment 8447656 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: layout/style/nsAnimationManager.cpp @@ +805,5 @@ > > +void > +nsAnimationManager::AddStyleUpdatesTo(RestyleTracker& aTracker) > +{ > + PRCList *next = PR_LIST_HEAD(&mElementCollections); "*" next to type. @@ +807,5 @@ > +nsAnimationManager::AddStyleUpdatesTo(RestyleTracker& aTracker) > +{ > + PRCList *next = PR_LIST_HEAD(&mElementCollections); > + while (next != &mElementCollections) { > + ElementAnimationCollection *collection = static_cast<ElementAnimationCollection*>(next); And here. ::: layout/style/nsTransitionManager.cpp @@ +123,5 @@ > UpdateAllThrottledStylesInternal(); > } > > void > +nsTransitionManager::AddStyleUpdatesTo(RestyleTracker& aTracker) Consider factoring out these two methods and having a common implementation on CommonAnimationManager that takes an nsIAtom* argument and restyle hint? @@ +127,5 @@ > +nsTransitionManager::AddStyleUpdatesTo(RestyleTracker& aTracker) > +{ > + PRCList *next = PR_LIST_HEAD(&mElementCollections); > + while (next != &mElementCollections) { > + ElementAnimationCollection *collection = static_cast<ElementAnimationCollection*>(next); Ditto for these two lines.

(In reply to Cameron McCormack (:heycam) from comment #52) > Comment on attachment 8447652 [details] [diff] [review] > patch 20 - Make restyling exact - Avoid rerunning selector matching on > everything when the basis of rem units changes > > + if (aRestyleHint & ~eRestyle_Subtree) { > > + // We want this hint to apply to the root node's primary frame > > + // rather than the root frame. > > Can you explain why? Because that's the one whose style context has the styles for the root element. Thus, if we have an eRestyle_Self, CSSAnimations, or CSSTransitions for the root, and we apply it to an ancestor of the root node's primary frame, we'll end up not doing the restyle that we need to do. > @@ +1433,5 @@ > > // (but note that nsPresShell::SetPreferenceStyleRules currently depends > > // on us re-running rule matching here > > nsStyleChangeList changeList; > > // XXX Does it matter that we're passing aExtraHint to the real root > > // frame and not the root node's primary frame? > > Is this comment obsoleted given the above change? No. We might want to switch the extra hint processing in the same way -- then again, it's not causing a problem and extra hints are rare. > @@ +2340,5 @@ > > nsRestyleHint hintToRestore = nsRestyleHint(0); > > + if (mContent && mContent->IsElement() && > > + // If we're we're resolving from the root of the frame tree (which > > + // we do in DoRebuildAllStyleData), we need to avoid getting the > > + // root's restyle data until we get to its primary frame. > > Can you help me understand why that's important? Same as above. > > + } else if (styleSet->IsInRuleTreeReconstruct()) { > > Can you assert somewhere that the restyle hint will be 0 when > IsInRuleTreeReconstruct() is true? I don't see why that would be true. We might have pending style changes at the time we do a rule tree reconstruct. I'm just using ResolveStyleWithReplacement instead of ReparentStyleContext because ReparentStyleContext would reuse the rule node from the old rule tree, whereas ResolveStyleWithReplacement rebuilds the rule tree path and thus returns the equivalent style context using the new rule tree. That probably deserves a comment, though, since it took me a minute to remember.

Comment on attachment 8447657 [details] [diff] [review] patch 25 - Remove the old (now-unused) miniflush code (preserving one of the header comments) Review of attachment 8447657 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.h @@ +164,5 @@ > // whose updating is suppressed on the main thread (to save > // unnecessary work), while leaving all other aspects of style > // out-of-date. > + // > + // Performs a 'mini-flush' to make styles from throttled transitions The bug title says we're renaming the term "mini-flush", so call it "animation-only flush" here? ::: layout/style/AnimationCommon.h @@ -124,5 @@ > - ElementAnimationCollection* collection = \ > - static_cast<ElementAnimationCollection*>(next); \ > - next = PR_NEXT_LINK(next); \ > - \ > - if (collection->mFlushGeneration == now) { \ In the new code in the previous patch we don't do any generation checking. Can we run into the same problem of flushing the animation styles twice in the new code, or will the use of the RestyleTracker prevent that?

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #54) > (In reply to Cameron McCormack (:heycam) from comment #52) > > Comment on attachment 8447652 [details] [diff] [review] > > patch 20 - Make restyling exact - Avoid rerunning selector matching on > > everything when the basis of rem units changes > > > > + if (aRestyleHint & ~eRestyle_Subtree) { > > > + // We want this hint to apply to the root node's primary frame > > > + // rather than the root frame. > > > > Can you explain why? > > Because that's the one whose style context has the styles for the root > element. Thus, if we have an eRestyle_Self, CSSAnimations, or > CSSTransitions for the root, and we apply it to an ancestor of the root > node's primary frame, we'll end up not doing the restyle that we need to do. Understood. > > > + } else if (styleSet->IsInRuleTreeReconstruct()) { > > > > Can you assert somewhere that the restyle hint will be 0 when > > IsInRuleTreeReconstruct() is true? > > I don't see why that would be true. We might have pending style changes at > the time we do a rule tree reconstruct. Oh right we would want to redo selector matching there. > I'm just using ResolveStyleWithReplacement instead of ReparentStyleContext > because ReparentStyleContext would reuse the rule node from the old rule > tree, whereas ResolveStyleWithReplacement rebuilds the rule tree path and > thus returns the equivalent style context using the new rule tree. That > probably deserves a comment, though, since it took me a minute to remember. I figured so, but yes a comment would be good.

(In reply to Cameron McCormack (:heycam) from comment #55) > In the new code in the previous patch we don't do any generation checking. > Can we run into the same problem of flushing the animation styles twice in > the new code, or will the use of the RestyleTracker prevent that? I believe the RestyleTracker should merge fully over the tree with no duplication, so the single overall last-update check in patch 3 should be sufficient. (I also didn't like the term "generation" there. I prefer to restrict the term generation to things that increase every time something happens (e.g., on every style change that we flush, which can happen more than once per refresh cycle) rather than things that increase with the refresh clock. This is the way we use the term for mAnimationGeneration, which is used to track what animation data has been shipped to the compositor thread.)

Comment on attachment 8465832 [details] [diff] [review] patch 16 - Add comment about potential performance impromevent to ResolveStyleWithReplacement Review of attachment 8465832 [details] [diff] [review]: ----------------------------------------------------------------- r=me with s/ResolveStyleWithReplacement/RuleNodeWithReplacement/ in the commit message.

Comment on attachment 8447657 [details] [diff] [review] patch 25 - Remove the old (now-unused) miniflush code (preserving one of the header comments) r=me with s/a 'mini-flush'/an "animation-only style flush/ or something similar in the comment.

(In reply to Cameron McCormack (:heycam) from comment #47) > Comment on attachment 8447644 [details] [diff] [review] > patch 12 - Fix the visited rule node handling in ResolveStyleWithReplacement > Please add a test with this patch. Here's one you can adapt I was just > trying locally in a Nightly without this patch queue applied, which I assume > will work with the queue up to this patch applied (where work means it will > transition from white to green instead of nearly-red to red): > > <!DOCTYPE html> > <style> > span { transition: 1s background-color; font: 48px sans-serif; } > :link span { background-color: rgb(254, 0, 0); } > :link span:hover { background-color: red; } > :visited span { background-color: white; } > :visited span:hover { background-color: green; } > </style> > <a href=""><span>some text here</span></a> (In reply to Cameron McCormack (:heycam) from comment #48) > Or will we not be running this code yet because background-color isn't a > property that gets animated on the compositor? This is a testcase for bug 868975 rather than something fixed by this patch. Writing a testcase for the problem fixed by this bug is somewhat more complicated. I think it could be done by setting up a compositor-driven animation on a visited link, and while that animation is running making a style change on a different (independent) element to trigger an animation-only style flush, and then using DOMWindowUtils.getVisitedDependentComputedStyle() to observe that the :visited styles have disappeared. I guess that's not as hard as I was initially thinking it would be, so maybe I'll give it a shot.

continued from the previous comment: I tried writing that test, and then debugged why the test didn't fail without the patch. This led me to conclude that the bug being fixed in patch 12 is currently untestable, but might become testable in the future. In particular, currently, PresShell::FlushPendingNotifications has this chunk of code: if (aFlush.mFlushAnimations && !mPresContext->StyleUpdateForAllAnimationsIsUpToDate()) { if (mPresContext->AnimationManager()) { mPresContext->AnimationManager()-> FlushAnimations(CommonAnimationManager::Cannot_Throttle); } if (mPresContext->TransitionManager()) { mPresContext->TransitionManager()-> FlushTransitions(CommonAnimationManager::Cannot_Throttle); } mPresContext->TickLastStyleUpdateForAllAnimations(); } right before its call to ProcessPendingRestyles. This posts restyles for everything that is animating -- restyles that will be processed after the animation-only style flush (which happens inside ProcessPendingRestyles). These calls thus cover up any damage done by incorrect behavior of the animation-only style flush (and also duplicate its work!) except in so far as that damage relates to incorrect starting of transitions. I think I plan to land the test anyway, even though it doesn't currently test anything useful, because it might do so if we reduce the duplication involved (which I may well do as part of bug 960465).

That said, I do have a test that now: * without the change in patch 12, hits the fatal assertion at http://hg.mozilla.org/mozilla-central/file/a4f779bd7cc2/layout/style/nsStyleContext.cpp#l154 due to the bug being fixed in patch 12 * without the patch *and* with the rearrangement to depend on the animation-only style flush more (i.e., remove the chunk of code in the previous comment and make the call to UpdateOnlyAnimationStyles in RestyleManager::ProcessPendingRestyles unconditional) *and* with a change to work around the fatal assertion by tweaking the flags in ResolveStyleWithReplacement, shows a test failure * passes with the patches So I think that's definitely worth adding.

> > void > > +nsTransitionManager::AddStyleUpdatesTo(RestyleTracker& aTracker) > > Consider factoring out these two methods and having a common implementation > on CommonAnimationManager that takes an nsIAtom* argument and restyle hint? I'm actually going to have the entire implementation on CommonAnimationManager with no extra hintsparameters, since the data is on the collection, and that approach fits better with the patches higher in my queue for bug 960465.

https://hg.mozilla.org/integration/mozilla-inbound/rev/96186774f07c https://hg.mozilla.org/integration/mozilla-inbound/rev/8da0b361d546 https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a810bc1b36 https://hg.mozilla.org/integration/mozilla-inbound/rev/d9db9020d57a https://hg.mozilla.org/integration/mozilla-inbound/rev/4a395d400f60 https://hg.mozilla.org/integration/mozilla-inbound/rev/720eed827027 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9de658d4b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/d13154302d77 https://hg.mozilla.org/integration/mozilla-inbound/rev/119416a35fa8 https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee0ebcf0602 https://hg.mozilla.org/integration/mozilla-inbound/rev/a911c666406f https://hg.mozilla.org/integration/mozilla-inbound/rev/06ffae59cea9 https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b4fbbcbcd4 https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe97c2db729 https://hg.mozilla.org/integration/mozilla-inbound/rev/dc528616d916 https://hg.mozilla.org/integration/mozilla-inbound/rev/10438983fda7 https://hg.mozilla.org/integration/mozilla-inbound/rev/d776e50cd140 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1136091a68 https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7a3787b584 https://hg.mozilla.org/integration/mozilla-inbound/rev/ebf41f7c81b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/9719c08c3144

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #64) > So I think that's definitely worth adding. Worth noting, though, that as of today it won't actually run anywhere, since B2G doesn't have global history, and all other platforms don't have OMT animations. But it should start running automatically if other platforms get OMT animations turned on, and it will TEST-UNEXPECTED-PASS if B2G gets global history.

Partially backed out in: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d4dd2261a5

According to try runs, the orange was from patch 24: https://tbpl.mozilla.org/?tree=Try&rev=7c06c13be60b https://tbpl.mozilla.org/?tree=Try&rev=c970a0ba48e0 which really doesn't make much sense to me since that code should only run on platforms with OMT animations enabled.

Oh, except https://hg.mozilla.org/mozilla-central/file/e6614d8d85f9/layout/style/crashtests/972199-1.html is setting the pref to true, so we have OMT animations enabled for the latter part of the crashtest run (order is in https://hg.mozilla.org/mozilla-central/file/e6614d8d85f9/testing/crashtest/crashtests.list ).

(It should probably be using pushPrefEnv.) That said, it may well be showing a real bug, although I also can't repro that bug by enabling OMT animations locally on Linux.

Anyway, relanded patches 18 through 22: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2919311231ec remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/69c78fb96b8a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dd86a9d3fd78 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9429f3db79 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e148599c0bba

https://hg.mozilla.org/mozilla-central/rev/96186774f07c https://hg.mozilla.org/mozilla-central/rev/8da0b361d546 https://hg.mozilla.org/mozilla-central/rev/a8a810bc1b36 https://hg.mozilla.org/mozilla-central/rev/d9db9020d57a https://hg.mozilla.org/mozilla-central/rev/4a395d400f60 https://hg.mozilla.org/mozilla-central/rev/720eed827027 https://hg.mozilla.org/mozilla-central/rev/6a9de658d4b4 https://hg.mozilla.org/mozilla-central/rev/d13154302d77 https://hg.mozilla.org/mozilla-central/rev/119416a35fa8 https://hg.mozilla.org/mozilla-central/rev/7ee0ebcf0602 https://hg.mozilla.org/mozilla-central/rev/a911c666406f https://hg.mozilla.org/mozilla-central/rev/06ffae59cea9 https://hg.mozilla.org/mozilla-central/rev/b5b4fbbcbcd4 https://hg.mozilla.org/mozilla-central/rev/2919311231ec https://hg.mozilla.org/mozilla-central/rev/69c78fb96b8a https://hg.mozilla.org/mozilla-central/rev/dd86a9d3fd78 https://hg.mozilla.org/mozilla-central/rev/2f9429f3db79 https://hg.mozilla.org/mozilla-central/rev/e148599c0bba

Given that I've been unable to reproduce the failure locally either on Linux (with OMT animations on) or on B2G emulator, I'm inclined to just fix comment 70. While it's probably papering over something, if that something turns up again it seems likely to be turning up in a more reproducable way.

I fixed the crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb35f913cc5 and relanded the remaining patches: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f481de3386 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/96483fbec785 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/12959495c74e

Also, the try run showing the intermittent crashtest orange is fixed with that change: https://tbpl.mozilla.org/?tree=Try&rev=af55063ec9b8 (I had equivalent orange try runs without it.)

https://hg.mozilla.org/mozilla-central/rev/a2f481de3386 https://hg.mozilla.org/mozilla-central/rev/96483fbec785 https://hg.mozilla.org/mozilla-central/rev/12959495c74e