In PrepareAndTraverseSubtree() we are passing |forAnimationOnly| to AutoRestyleTimelineMarker [1], but apparently it's wrong. I know |forAnimationOnly| is really misleading. It doesn't mean 'animation-only restyle' what we use generally. The |forAnimationOnly| is true only when we handle events (e.g. mouse movement, click, etc.), and its purpose is to synchronize transform animations which is running on the compositor to the main thread (See description in GeckoRestyleManager::UpdateOnlyAnimationStyles). (I think we should move the description into Restylemanager::UpdateOnlyAnimationStyle, btw) [1] https://hg.mozilla.org/mozilla-central/file/a418121d4625/layout/style/ServoStyleSet.cpp#l348

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > In PrepareAndTraverseSubtree() we are passing |forAnimationOnly| to > AutoRestyleTimelineMarker [1], but apparently it's wrong. I know > |forAnimationOnly| is really misleading. It doesn't mean 'animation-only > restyle' what we use generally. The |forAnimationOnly| is true only when we > handle events (e.g. mouse movement, click, etc.), and its purpose is to > synchronize transform animations which is running on the compositor to the > main thread (See description in > GeckoRestyleManager::UpdateOnlyAnimationStyles). (I think we should move the > description into Restylemanager::UpdateOnlyAnimationStyle, btw) Ah, that's quite confusing! What's a good way to determine "animation-only restyle" for the Stylo case then?

I think it's in EffectCompositor::PreTraverseSubtree() [1] and in nsSMILAnimationController::PreTraverseInSubtree, I thought the function names were raised when we discussed about the marker at the last all hands, but I don't remember what the conclusion was. :-) [1] https://hg.mozilla.org/mozilla-central/file/6fec4855b534/dom/animation/EffectCompositor.cpp#l1056

It seems to me that getting this marker right would be hard, since we do both the animation pass and the regular pass within a single call to Servo_TraverseSubtree. Ryan, can we just get rid of this boolean?

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3) > It seems to me that getting this marker right would be hard, since we do > both the animation pass and the regular pass within a single call to > Servo_TraverseSubtree. Ryan, can we just get rid of this boolean? At the moment, we're exposing this data because supposedly animation restyle tests make use of it: http://searchfox.org/mozilla-central/search?q=isAnimationOnly&case=true&path= If those tests can be changed to no longer make use of the marker, then it should be okay to remove. Hiro, any opinion on this?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4) > If those tests can be changed to no longer make use of the marker, then it > should be okay to remove. To be more specific, they would need to stop depending on the animation flag inside the marker. The markers themselves would still be there.

For those test cases, we don't need accurate marker position, we just want to know that, something like this, 'this element was restyled by CSS Animation restyle in this tick?'. So I think it's not hard to implement it. As I wrote in comment 2, EffectCompositor::PreTraverseSubtree() is a good place for it. Have someone already tried it? I think kuoe0 starts working on this, but if not, I can take this. Anyway, the test cases are pretty important for animation optimization, we definitely need it. Oh, one more thing. If you are talking about the marker in PrepareAndTraverseSubtree(), it does not work for animations at all since it's called in wrong place, so we can drop the |forThrottledAnimationFlush| flag, we can just set false there.

P3 for now: we definitely want this since it's important for performance regression tests, but I'm not sure if it should block release.

Taking, since I'd like to run test_restyles.html as soon as possible to avoid any regressions by me. :)

Comment on attachment 8895163 [details] Bug 1379516 - Get dom.animations.offscreen-throttling preference value just once at startup. https://reviewboard.mozilla.org/r/166312/#review171956

Comment on attachment 8895164 [details] Bug 1379516 - Add isStyledByServo(). https://reviewboard.mozilla.org/r/166314/#review172020 ::: commit-message-be5db:3 (Diff revision 1) > +DOMWindowUtils.isStyledByServo checks not only the preference value but also > +STYLO_FORCE_ENABLED value. (I don't quite follow this comment. It seems to me that DOMWindowUtils.isStyledByServo just checks if the current document is styled by servo which could depend on a number of factors?) ::: dom/animation/test/testcommon.js:323 (Diff revision 1) > return SpecialPowers.DOMWindowUtils.layerManagerRemote && > SpecialPowers.getBoolPref(OMTAPrefKey); > } > > /** > + * Returns true if the document is styled by servo. perhaps "if the document" could be "if this window's document"? The thinking being that nsIDOMWindowUtils is associated with a window, and this just takes the document associated with that window.

(In reply to Brian Birtles (:birtles) from comment #18) > Comment on attachment 8895164 [details] > Bug 1379516 - Add isStyledByServo(). > > https://reviewboard.mozilla.org/r/166314/#review172020 > > ::: commit-message-be5db:3 > (Diff revision 1) > > +DOMWindowUtils.isStyledByServo checks not only the preference value but also > > +STYLO_FORCE_ENABLED value. > > (I don't quite follow this comment. It seems to me that > DOMWindowUtils.isStyledByServo just checks if the current document is styled > by servo which could depend on a number of factors?) Yes, right. An important factor is STYLO_FORCE_ENABLED, if STYLO_FORCE_ENABLED=1, the document is styled by servo even if the preference value is *false*! Actually the pref value is false on our CI even if the test target is stylo.

Comment on attachment 8895165 [details] Bug 1379516 - Skip offscreen throttling tests on stylo. https://reviewboard.mozilla.org/r/166316/#review172066 ::: dom/animation/test/chrome/test_restyles.html:71 (Diff revision 1) > var omtaEnabled = isOMTAEnabled(); > > var isAndroid = !!navigator.userAgent.includes("Android"); > var offscreenThrottlingEnabled = > - SpecialPowers.getBoolPref('dom.animations.offscreen-throttling'); > + SpecialPowers.getBoolPref('dom.animations.offscreen-throttling') && > + !isStyledByServo(); // Bug 1303235 As discussed, let's add a comment explaining this

Comment on attachment 8895166 [details] Bug 1379516 - Update the test where an orphaned element is attached to a document. https://reviewboard.mozilla.org/r/166318/#review172068 ::: commit-message-330fc:1 (Diff revision 1) > +Bug 1379516 - Correct the description for the case where script animations on orphaned element is attached to document. r?birtles This summary is inaccurate since this patch does more than just change the description. ::: commit-message-330fc:3 (Diff revision 1) > +We have a chance to run styling process when we attach an orphaned element to > +a document in this test setup. Precisely, we can process a restyle between > +rAF callbacks and Promise.then() callback for waitForAnimationFrames(). > +So if we call RequestRestyle(Layer) when we attach the element to the document, > +the animation starts restyling in the first frame. As I understand it, once bug 1193394 lands this will no longer be true? Or else I don't understand this comment. Once bug 1193394 lands, I believe we will exhaust the Promise microtask queue before we go on to do restyling, right? ::: dom/animation/test/chrome/test_restyles.html:766 (Diff revision 1) > markers = await observeStyling(1); > - // We are observing restyles in rAF callback which is processed before > - // restyling process in each frame, so in the first frame there should be > - // no observed restyle since we don't process restyle while the element > - // is not attached to the document. > - is(markers.length, 0, > + // Bug 1388557: We should call RequestRestyle(Layer) when an element which > + // has running script animations is attached to document. > + todo_is(markers.length, 1, > + 'Bug 1388557 We should observe one restyle in the first frame ' + > + 'right after re-atatching to the document'); re-attaching

Comment on attachment 8895168 [details] Bug 1379516 - Mark animation timerline marker for stylo. https://reviewboard.mozilla.org/r/166322/#review172078 ::: layout/base/ServoRestyleManager.cpp:206 (Diff revision 1) > // EffectCompositor::mElementsToRestyle still has unbinded old pseudo > // element. We should drop it. > return; > } > > + AutoRestyleTimelineMarker marker(mPresContext->GetDocShell(), true); Nit: we could possible make these bools a little easier to follow if we write: ..., true /* animation-only */); assuming that fits on the line. Of course, long-term we should just replace the bool with something else like an enum or named static bool or something of the sort. ::: layout/style/ServoStyleSet.cpp:329 (Diff revision 1) > MOZ_ASSERT(MayTraverseFrom(const_cast<Element*>(aRoot))); > - bool forThrottledAnimationFlush = !!(aFlags & ServoTraversalFlags::AnimationOnly); > > - AutoRestyleTimelineMarker marker(mPresContext->GetDocShell(), forThrottledAnimationFlush); > + AutoRestyleTimelineMarker marker(mPresContext->GetDocShell(), false); Why is it ok to pass false for aIsAnimationOnly here? ::: layout/style/ServoStyleSet.cpp:354 (Diff revision 1) > // We don't need to trigger a second traversal if this restyle only for > // flushing throttled animations. That's because the first traversal only > // performs the animation-only restyle, skipping the normal restyle, and so > // will not generate any SequentialTask that could update animation state > // requiring a subsequent traversal. > - if (forThrottledAnimationFlush) { > + if (!!(aFlags & ServoTraversalFlags::AnimationOnly)) { I wonder if we need the !! here actually. I suppose if (aFlags & ServoTraversalFlags::AnimationOnly) { ... would do

Comment on attachment 8895169 [details] Bug 1379516 - A test case that checks animations on the compositor keeps running on the compositor when unrelated style attribute is changed. https://reviewboard.mozilla.org/r/166324/#review172082

Comment on attachment 8895167 [details] Bug 1379516 - Add descriptions for re-attaching orphaned animation case for stylo. https://reviewboard.mozilla.org/r/166320/#review172074 This is fine but I want to quickly double-check the updated comments ::: dom/animation/test/chrome/test_restyles.html:766 (Diff revision 1) > > markers = await observeStyling(1); > + if (isServo) { > + // In Servo, we explicitly set important_rules_change flag to the element > + // in compute_style() during the initial normal traversal right after > + // re-attaching, it leads to invoke a SequentialTask for CascadeResults s/, it leads/ which leads/ s/invoke/invoking/ ::: dom/animation/test/chrome/test_restyles.html:767 (Diff revision 1) > markers = await observeStyling(1); > + if (isServo) { > + // In Servo, we explicitly set important_rules_change flag to the element > + // in compute_style() during the initial normal traversal right after > + // re-attaching, it leads to invoke a SequentialTask for CascadeResults > + // which ends up calling RequestRestyle(Standard), as a result, the s/, as a result/. As a result/ ::: dom/animation/test/chrome/test_restyles.html:769 (Diff revision 1) > + // frame. If we fixed bug 1388557 before bug 1388560, we should definitely > + // add another test case for bug 1388560. Quoting bug numbers like this makes the comment impossible to understand without looking up the bugs. "If we fix the behavior when we when attach an orphaned element with script animations to a document so that it requests a layer restyle (bug 1388557) before fixing important_rules_change in compute_style() so that it no longer dispatches a needless standard restyle (bug 1388560), we should add a test case that..." ::: dom/animation/test/chrome/test_restyles.html:773 (Diff revision 1) > + // animation is restyled during a second animation restyle in the first > + // frame. If we fixed bug 1388557 before bug 1388560, we should definitely > + // add another test case for bug 1388560. > + is(markers.length, 1, > + 'We should observe one restyle in the first frame right after ' + > + 're-atatching to the document'); re-attaching ::: dom/animation/test/chrome/test_restyles.html:773 (Diff revision 1) > + // animation is restyled during a second animation restyle in the first > + // frame. If we fixed bug 1388557 before bug 1388560, we should definitely > + // add another test case for bug 1388560. > + is(markers.length, 1, > + 'We should observe one restyle in the first frame right after ' + > + 're-atatching to the document'); re-attaching

Comment on attachment 8895166 [details] Bug 1379516 - Update the test where an orphaned element is attached to a document. https://reviewboard.mozilla.org/r/166318/#review172068 > This summary is inaccurate since this patch does more than just change the description. I did add a comment about it in the second paragraph. > As I understand it, once bug 1193394 lands this will no longer be true? Or else I don't understand this comment. > > Once bug 1193394 lands, I believe we will exhaust the Promise microtask queue before we go on to do restyling, right? Yeah, right. Also I left a comment about the micro task change.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #32) > Comment on attachment 8895166 [details] > Bug 1379516 - Correct the description for the case where script animations > on orphaned element is attached to document. > > https://reviewboard.mozilla.org/r/166318/#review172068 > > > This summary is inaccurate since this patch does more than just change the description. > > I did add a comment about it in the second paragraph. The summary is still not accurate though. Please update the summary (first line).

(In reply to Brian Birtles (:birtles) from comment #33) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #32) > > Comment on attachment 8895166 [details] > > Bug 1379516 - Correct the description for the case where script animations > > on orphaned element is attached to document. > > > > https://reviewboard.mozilla.org/r/166318/#review172068 > > > > > This summary is inaccurate since this patch does more than just change the description. > > > > I did add a comment about it in the second paragraph. > > The summary is still not accurate though. Please update the summary (first > line). It was too long for the first line, I will split the commit into two.

Comment on attachment 8895674 [details] Bug 1379516 - Change is() check to todo_is() for the case where orphaned element is attached to document. https://reviewboard.mozilla.org/r/166946/#review172162 Please merge this with the previous patch. Just make the message something like "Update the test where an orphaned element is attached to a document".

Comment on attachment 8895166 [details] Bug 1379516 - Update the test where an orphaned element is attached to a document. https://reviewboard.mozilla.org/r/166318/#review172160 ::: commit-message-330fc:3 (Diff revision 3) > +And is() check changed to todo_is() to change this behavior in bug 1388557. > + This is not correct. ::: commit-message-330fc:6 (Diff revision 3) > +element to a document in this test setup. Precisely, we can process a restyle > +between rAF callbacks and Promise.then() callback for waitForAnimationFrames(). > +So if we call RequestRestyle(Layer) when we attach the element to the document, > +the animation starts restyling in the first frame. *BUT* this behavior will > +also change once our micro tasks handling becomes the HTML spec compliance > +(bug 1193394). When the micro tasks handling changes, we should also fix a > +bunch of test cases and test utilities in bug 1388557. I think Olli is going to change the tests as part of bug 1193394. I don't like adding tests that depend on the current behavior (or in this case adding a todo_is that depends on it). Can we just make the test expect the behavior introduced in bug 1193394? ::: dom/animation/test/chrome/test_restyles.html:764 (Diff revision 3) > > document.body.appendChild(div); > > markers = await observeStyling(1); > - // We are observing restyles in rAF callback which is processed before > - // restyling process in each frame, so in the first frame there should be > + // Bug 1388557: We should call RequestRestyle(Layer) when an element which > + // has running script animations is attached to document. to a document

Comment on attachment 8895167 [details] Bug 1379516 - Add descriptions for re-attaching orphaned animation case for stylo. https://reviewboard.mozilla.org/r/166320/#review172166 ::: dom/animation/test/chrome/test_restyles.html:774 (Diff revision 3) > + // restyle (bug 1388560), we should add a test case that are able to check > + // the needless standard restyle. "add a test case that fails if we continue to unnecessarily request a standard restyle." ? ::: dom/animation/test/chrome/test_restyles.html:778 (Diff revision 3) > + // compute_style() so that it no longer dispatches a needless standard > + // restyle (bug 1388560), we should add a test case that are able to check > + // the needless standard restyle. > + is(markers.length, 1, > + 'We should observe one restyle in the first frame right after ' + > + 're-atatching to the document'); re-attaching

Comment on attachment 8895166 [details] Bug 1379516 - Update the test where an orphaned element is attached to a document. https://reviewboard.mozilla.org/r/166318/#review172160 > I think Olli is going to change the tests as part of bug 1193394. I don't like adding tests that depend on the current behavior (or in this case adding a todo_is that depends on it). Can we just make the test expect the behavior introduced in bug 1193394? The expected behavior is is(markes.length, 1, ..), that can not be passed for now, then we need to use todo_is().

A final try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=62080185f89dd389c73b8a4f8858d8a122323a3c Note that tests on Mac has not finished yet, but I think that's OK. Also note that the hazard failure [1] seems to be caused by the revision that the try based on. I did another try based on another revision which seems to be OK. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ddc24f749652f7ca893f0b1a7c789cd23d62270 [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=62080185f89dd389c73b8a4f8858d8a122323a3c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=122396734

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cff0baa2b08d Get dom.animations.offscreen-throttling preference value just once at startup. r=birtles https://hg.mozilla.org/integration/autoland/rev/b0abe749660b Add isStyledByServo(). r=birtles https://hg.mozilla.org/integration/autoland/rev/2bc896ae1668 Skip offscreen throttling tests on stylo. r=birtles https://hg.mozilla.org/integration/autoland/rev/511aca5eb28b Update the test where an orphaned element is attached to a document. r=birtles https://hg.mozilla.org/integration/autoland/rev/bd7ce9e76553 Add descriptions for re-attaching orphaned animation case for stylo. r=birtles https://hg.mozilla.org/integration/autoland/rev/89a0a21e64c4 Mark animation timerline marker for stylo. r=birtles https://hg.mozilla.org/integration/autoland/rev/6433c832637f A test case that checks animations on the compositor keeps running on the compositor when unrelated style attribute is changed. r=birtles

https://hg.mozilla.org/mozilla-central/rev/cff0baa2b08d https://hg.mozilla.org/mozilla-central/rev/b0abe749660b https://hg.mozilla.org/mozilla-central/rev/2bc896ae1668 https://hg.mozilla.org/mozilla-central/rev/511aca5eb28b https://hg.mozilla.org/mozilla-central/rev/bd7ce9e76553 https://hg.mozilla.org/mozilla-central/rev/89a0a21e64c4 https://hg.mozilla.org/mozilla-central/rev/6433c832637f