Bug 894773's patch is a bit broken. It requires three property changes to trigger active layers, not two as intended. We should also consider making scripted property changes that run during a setTimeout or requestAnimationFrame immediately trigger active layers, on the grounds that they're likely to be part of an animation.

We should also make layers eagerly active when we have CSS animations/transitions present --- even if those transitions/animations are not being offloaded to the compositor.

Without this patch it takes three property changes to make the layer active: 1) Create the LayerActivity with mutation count 0. 2) Increment the mutation count to 1. (Still considered inactive.) 3) Increment the mutation count to 2. (Now considered active.)

This can improve performance and smoothness by making layers active as quickly as possible, before the animation starts. It also makes OMTC more consistent with non-OMTC behavior. Maybe it violates the CSS spec, but no more so than our existing OMTA code, and only in a barely observable way. I guess it can be observed by triggering a transition on 'transform' or 'opacity' and then using elementFromPoint to detect that a stacking context has already been created even though 'transform'/'opacity' still have their default values? What do you think?

I'd probably be ok with one frame discontinuity. But does it handle animations that have an extended period where opacity is >=1 and therefore there should be no stacking context? (I haven't looked at the code yet.) This can happen with animation-delay or with cubic-bezier() timing functions that have y1 or y2 outside of [0, 1], or with step timing functions.

(In reply to David Baron [:dbaron] (needinfo? me; away Aug 28 - Sep 3) from comment #4) > I'd probably be ok with one frame discontinuity. But does it handle > animations that have an extended period where opacity is >=1 and therefore > there should be no stacking context? (I haven't looked at the code yet.) > This can happen with animation-delay or with cubic-bezier() timing functions > that have y1 or y2 outside of [0, 1], or with step timing functions. No, it doesn't. Neither does our off-main-thread-animation code, I believe. But I'm going to back off on this patch for now.

The MarkLayersActive code was getting somewhat out of control, doing several things at once, so I've refactored it with much more precise API.

https://hg.mozilla.org/mozilla-central/rev/e9df5541b113 https://hg.mozilla.org/mozilla-central/rev/b23aae5cb0c2

Comment on attachment 799427 [details] [diff] [review] Part 4. Add API to detect whether an nsGlobalWindow is running a timeout handler This seems OK, but do we want to detect all timeouts or just "nested" timeouts? Timeouts keep track of a nesting level, so we could detect just the latter if we want to... That would fail on cases like "rAF callback calls setTimeout which does all the real work", but people shouldn't be doing that anyway.

(In reply to Boris Zbarsky [:bz] from comment #15) > This seems OK, but do we want to detect all timeouts or just "nested" > timeouts? Timeouts keep track of a nesting level, so we could detect just > the latter if we want to... That would fail on cases like "rAF callback > calls setTimeout which does all the real work", but people shouldn't be > doing that anyway. I want to detect all timeouts. A nested timeout would typically correspond the second frame of an animation, at which point we'll have seen two style changes and guess that we're in an animation anyway. The goal here is to guess when a style change is the first frame of an animation. Treating every style.opacity or style.transform change in a timeout handler as the first frame of an animation may be overkill, but I think it's worth trying. (And until recently we basically treated every style change as potentially the first frame of an animation, so this change is just backing off bug 894773 a bit.)

Comment on attachment 798688 [details] [diff] [review] Part 1: Update mutation count for first as well as subsequent mutations [Approval Request Comment] Bug caused by (feature/regressing bug #): 894773 User impact if declined: Unnecessarily sluggish starting of jQuery animations Testing completed (on m-c, etc.): none Risk to taking this patch (and alternatives if risky): low risk. It's small simple change. String or IDL/UUID changes made by this patch: none

Comment on attachment 798688 [details] [diff] [review] Part 1: Update mutation count for first as well as subsequent mutations Review of attachment 798688 [details] [diff] [review]: ----------------------------------------------------------------- Never mind, we decided to back 894773 out of Aurora instead.

Comment on attachment 799421 [details] [diff] [review] Part 3: Create nsLayoutUtils::HasAnimations, and suppress opacity:0 optimizations whenever there's any opacity animation even if it's not using the compositor911889-animations r=dbaron though I think nsLayoutUtils::HasAnimationsForCompositor and nsLayoutUtils::HasAnimations would be clearer if they ended with: return has-animations || has-transitions; rather than: if (has-animations) { return true; } return has-transitions; (where "has-animations" and "has-transitions" stand for the function calls computing that). Feel free to fix that while you're there.

Comment on attachment 799429 [details] [diff] [review] Part 5. Add API to detect whether an nsRefreshDriver is in the middle of a refresh r=dbaron if you assert that it's the expected value before setting it (for both sets). (Perhaps even better would be use of an RAII class like the AutoToggle patch sitting in bug 518756, since that would be exception-safe too. I did promise once to review for exception-safety when people were advocating use of C++ exceptions.)

Comment on attachment 799432 [details] [diff] [review] Part 6: A scripted change to element.style.opacity or element.style.transform in a setTimeout or requestAnimationFrame callback should trigger our "style property is animated" heuristic > ActiveLayerTracker::NotifyAnimated(nsIFrame* aFrame, nsCSSProperty aProperty) > { > LayerActivity* layerActivity = GetLayerActivityForUpdate(aFrame); > uint8_t& mutationCount = layerActivity->RestyleCountForProperty(aProperty); > // We know this is animated, so just hack the mutation count. > mutationCount = 0xFF; > } >+/* static */ void >+ActiveLayerTracker::NotifyInlineStyleRuleModified(nsIFrame* aFrame, >+ nsCSSProperty aProperty) >+{ >+ if (!IsPresContextInScriptAnimationCallback(aFrame->PresContext())) { >+ return; >+ } >+ LayerActivity* layerActivity = GetLayerActivityForUpdate(aFrame); >+ uint8_t& mutationCount = layerActivity->RestyleCountForProperty(aProperty); >+ // Assume this is animated right away, so just hack the mutation count. >+ mutationCount = 0xFF; >+} Maybe call NotifyAnimated here rather than copying it into the last three lines of NotifyInlineStyleRuleModified? r=dbaron

Comment on attachment 799419 [details] [diff] [review] Part 2: Refactor MarkLayersActive code into its own class and be much more explicit about what it does Review of attachment 799419 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.cpp @@ +246,5 @@ > needInvalidatingPaint = true; > nsIFrame* childFrame = > GetFrameForChildrenOnlyTransformHint(aFrame)->GetFirstPrincipalChild(); > for ( ; childFrame; childFrame = childFrame->GetNextSibling()) { > + ActiveLayerTracker::NotifyRestyle(aFrame, eCSSProperty_transform); Matt points out that this should be childFrame, not aFrame.

With that fixed, plus an uninitialized mContentActive fixed, part 2 is green on mochitest-other finally. I'm still not sure what the problem was.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5cc7d7b84d https://hg.mozilla.org/integration/mozilla-inbound/rev/76bec3e3cdb9 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4cb7d2fff80 https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9e965ed653 https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9136052068 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a88dee3b92b

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/cd94525c17a4 - you may have been green in Moth, but you were rather orange in mochitest-5, racking up 77 unexpected "Not yet in refresh: '!mInRefresh'" assertions in test_transitions_per_property.html in https://tbpl.mozilla.org/php/getParsedLog.php?id=29821142&tree=Mozilla-Inbound

Not entirely green in Moth either, since https://tbpl.mozilla.org/php/getParsedLog.php?id=29821668&full=1&branch=mozilla-inbound is test_mousecapture.xul being surprised by your assertions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f04406171f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a294c17e175 https://hg.mozilla.org/integration/mozilla-inbound/rev/6f26ebdd5751 https://hg.mozilla.org/integration/mozilla-inbound/rev/9b63189605b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/24662d1aed8f https://hg.mozilla.org/integration/mozilla-inbound/rev/989403e34775

Backed out for reftest failures on Android: https://tbpl.mozilla.org/php/getParsedLog.php?id=29963670&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=29963259&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/93bce3f109dc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a639df3fefcc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b873aefc383 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ac2635cb38 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8189b03564a2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab529371380

https://hg.mozilla.org/integration/mozilla-inbound/rev/09abc00886bd https://hg.mozilla.org/integration/mozilla-inbound/rev/4adca2b4f651 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5460f84de3e https://hg.mozilla.org/integration/mozilla-inbound/rev/a871905cadb7 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7763980b35 https://hg.mozilla.org/integration/mozilla-inbound/rev/82578c63ceb5 https://hg.mozilla.org/integration/mozilla-inbound/rev/eb66a2aa121e

https://hg.mozilla.org/mozilla-central/rev/09abc00886bd https://hg.mozilla.org/mozilla-central/rev/4adca2b4f651 https://hg.mozilla.org/mozilla-central/rev/f5460f84de3e https://hg.mozilla.org/mozilla-central/rev/a871905cadb7 https://hg.mozilla.org/mozilla-central/rev/ed7763980b35 https://hg.mozilla.org/mozilla-central/rev/82578c63ceb5 https://hg.mozilla.org/mozilla-central/rev/eb66a2aa121e