1379516 - stylo: incorrect restyle marker for animations

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

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

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

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

Comment 3

[Bobby Holley (:bholley)]

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?

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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?

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 7

[Brian Birtles (:birtles)]

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.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc12eede1a42fd2854c5ac50255fbfdb99de3b66

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379516 - Get dom.animations.offscreen-throttling preference value just once at startup.

Review commit: https://reviewboard.mozilla.org/r/166312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166312/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379516 - Add isStyledByServo().

DOMWindowUtils.isStyledByServo checks not only the preference value but also
STYLO_FORCE_ENABLED value.

Review commit: https://reviewboard.mozilla.org/r/166314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166314/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379516 - Skip offscreen throttling tests on stylo.

Review commit: https://reviewboard.mozilla.org/r/166316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166316/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379516 - Update the test where an orphaned element is attached to a document.

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.

Review commit: https://reviewboard.mozilla.org/r/166318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166318/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379516 - Add descriptions for re-attaching orphaned animation case for stylo.

Unlike Gecko, Stylo currently invokes a needless RequestRestyle(Standard)
for this test case (bug 1388560). For this reason, the check right after
re-attaching the orphaned element passes accidentally.

Review commit: https://reviewboard.mozilla.org/r/166320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166320/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379516 - Mark animation timerline marker for stylo.

Review commit: https://reviewboard.mozilla.org/r/166322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166322/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379516 - A test case that checks animations on the compositor keeps running on the compositor when unrelated style attribute is changed.

Review commit: https://reviewboard.mozilla.org/r/166324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166324/

Comment 17

[Brian Birtles (:birtles)]

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 18

[Brian Birtles (:birtles)]

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.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(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 20

[Brian Birtles (:birtles)]

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 21

[Brian Birtles (:birtles)]

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 22

[Brian Birtles (:birtles)]

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 23

[Brian Birtles (:birtles)]

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 24

[Brian Birtles (:birtles)]

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 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895163 [details]
Bug 1379516 - Get dom.animations.offscreen-throttling preference value just once at startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166312/diff/1-2/

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895164 [details]
Bug 1379516 - Add isStyledByServo().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166314/diff/1-2/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895165 [details]
Bug 1379516 - Skip offscreen throttling tests on stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166316/diff/1-2/

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895166 [details]
Bug 1379516 - Update the test where an orphaned element is attached to a document.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166318/diff/1-2/

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895167 [details]
Bug 1379516 - Add descriptions for re-attaching orphaned animation case for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166320/diff/1-2/

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895168 [details]
Bug 1379516 - Mark animation timerline marker for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166322/diff/1-2/

Comment 31

[Hiroyuki Ikezoe (:hiro)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166324/diff/1-2/

Comment 32

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 33

[Brian Birtles (:birtles)]

(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).

Comment 34

[Hiroyuki Ikezoe (:hiro)]

(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 35

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379516 - Change is() check to todo_is() for the case where orphaned element is attached to document.

Review commit: https://reviewboard.mozilla.org/r/166946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166946/

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895166 [details]
Bug 1379516 - Update the test where an orphaned element is attached to a document.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166318/diff/2-3/

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895167 [details]
Bug 1379516 - Add descriptions for re-attaching orphaned animation case for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166320/diff/2-3/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895168 [details]
Bug 1379516 - Mark animation timerline marker for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166322/diff/2-3/

Comment 39

[Hiroyuki Ikezoe (:hiro)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166324/diff/2-3/

Comment 40

[Brian Birtles (:birtles)]

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 41

[Brian Birtles (:birtles)]

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 42

[Brian Birtles (:birtles)]

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 43

[Hiroyuki Ikezoe (:hiro)]

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().

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895163 [details]
Bug 1379516 - Get dom.animations.offscreen-throttling preference value just once at startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166312/diff/2-3/

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895164 [details]
Bug 1379516 - Add isStyledByServo().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166314/diff/2-3/

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895165 [details]
Bug 1379516 - Skip offscreen throttling tests on stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166316/diff/2-3/

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895166 [details]
Bug 1379516 - Update the test where an orphaned element is attached to a document.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166318/diff/3-4/

Comment 48

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895167 [details]
Bug 1379516 - Add descriptions for re-attaching orphaned animation case for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166320/diff/3-4/

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895168 [details]
Bug 1379516 - Mark animation timerline marker for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166322/diff/3-4/

Comment 50

[Hiroyuki Ikezoe (:hiro)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166324/diff/3-4/

Comment 51

[Hiroyuki Ikezoe (:hiro)]

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

Comment 52

[Pulsebot]

Pushed by [e-mail]:
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

Comment 53

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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