1151694 - Make the Animation(Player) IsRunningOnCompositor method/member accurate

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

We currently expose whether an Animation (currently named AnimationPlayer but soon to be renamed) is running on the compositor via a chrome-only attribute. We also use this member internally for various purposes. However, it's not really accurate.

Firstly, I'm fairly sure we *don't* reliably set it back to false when we take an animation off the compositor in all case.

Secondly, in a few cases we deliberately clobber this member, setting it to false in order to trigger a layer transaction. If we were to query this member in the interim between clobbering it and sending the next layer transaction we'd get an inaccurate result.

Thirdly, if only one part of the animation (e.g. the opacity part) is running on the compositor then this member will still return true even though, strictly speaking, the animation is only partly running on the compositor.


We should make this more accurate for two reasons:

a) So that DevTools can use it to present meaningful information (ideally we'd also find a way to expose information on why an animation was *not* run on the compositor--at least Gaia devs would find this useful).

b) So we can use it internally. For example, in AnimationPlayer we have mFinishedAtLastComposeStyle which is only used to force an unthrottled sample the first time after an animation that was running on the compositor ends. If mIsRunningOnCompositor was accurate we could simply use that to force an unthrottled sample whenever it was true and the animation was finished.


I looked at doing this at one point but it was a little more involved than I first anticipated. The main change is that I think we'd need to track whether animations are running on the compositor on a per-property basis. Beyond that, I'm not sure.

We might need another means of indicating that animations need to be re-added to layers but it's possible we have that now (now that bugs like bug 1073336 have made the RestyleTracker recognize differents in layer generations). We could unconditionally set mIsRunningOnCompositor to false for the specified property at the start of AddAnimationsForProperty in nsDisplayList.cpp but that wouldn't cover the case where we didn't create a layer at all so that would need more thought.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Move mIsRunningOnCompositor flag to KeyframeEffectReadOnly

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8647147 [details] [diff] [review]
Part 1: Move mIsRunningOnCompositor flag to KeyframeEffectReadOnly

Sorry. I accidentally hit Enter key.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Remove Animation::IsRunningOnCompositor.

Now KeyframeEffectReadOnly::IsRunningOnCompositor can handle the flag
depending on property.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: ClearIsRunningOnCompositor is not neccessary in Animation::DoPause.

There are two cases in DoPause. a) With a rendered document, b) Without the document.

a) If the animation has a document which renders the animation,
PendingAnimationTracker does render the animation immediately.
And the animation can keep running on compositor when the animation
starts to playing again.

b) If the animation has no rendered document, it means the animation has no
KeyframeEffect. So IsRunningOnCompositor returns false in this case. Even so,
Animation::CanThrottle actually returns *true* whenever the animation has no
KeyframeEffect.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Now I am thinking how we can eliminate Animation.ClearIsRunningOnCompositor in nsAnimationManager::CheckAnimationRule.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Now I am thinking how we can eliminate Animation.ClearIsRunningOnCompositor
> in nsAnimationManager::CheckAnimationRule.

I did miss a patch in bug 1188251 (part 8 patch). Actually I was writing patches for this issue on another cloned mozilla-central tree...

The part 8 patch calls KeyFrameEffectReadOnly.SetEffect() just before ClearIsRunningOnCompositor(), so we can reset the flag in KeyframeEffectReadOnly::SetEffect().

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > Now I am thinking how we can eliminate Animation.ClearIsRunningOnCompositor
> > in nsAnimationManager::CheckAnimationRule.
> 
> I did miss a patch in bug 1188251 (part 8 patch). Actually I was writing
> patches for this issue on another cloned mozilla-central tree...
> 
> The part 8 patch calls KeyFrameEffectReadOnly.SetEffect() just before

Oops. Animation.SetEffect() there. Animation::SetEffect() calls KeyFrameEffectReadOnly::SetParent() in it.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
e part 8 patch calls KeyFrameEffectReadOnly.SetEffect() just before
> 
> Oops. Animation.SetEffect() there. Animation::SetEffect() calls
> KeyFrameEffectReadOnly::SetParent() in it.

s/SetParent/SetParentTime/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Created attachment 8647152 [details] [diff] [review]
> Part 3: ClearIsRunningOnCompositor is not neccessary in Animation::DoPause.
> 
> There are two cases in DoPause. a) With a rendered document, b) Without the
> document.
> 
> a) If the animation has a document which renders the animation,
> PendingAnimationTracker does render the animation immediately.
> And the animation can keep running on compositor when the animation
> starts to playing again.

I might be wrong.. Animation.IsRunningOnCompositor shouldn't return true during the animation is paused even if the animation does not need to render on main thread when the animation gets back to playing.

The state of animation is actually NOT running on compositor during pausing. The animation is running on neither main thread nor compositor. It's just paused.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 0: Rewrite test_running_on_compositor.html with add_task()

To add test cases in incoming patches we need this first.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Move mIsRunningOnCompositor flag to KeyframeEffectReadOnly.

KeyframeEffectReadonly stores the flag for each property respectively.
This patch adds a test case that animation has two properties.
One can be run on the compositor, the other can not be.

One thing is not clear to me is that mIsRunningOnCompositor should be false when the animation is running on both of threads?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Animation.IsRunningOnCompositor should be false when the animation is finished state

This patch adds a test case for the finished state.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Remove Animation::SetIsRunningOnCompositor.

This patch and an incoming patch (part 4) do not contribute the accuracy of IsRunningOnCompositor at all. Both of them just do cleanup.

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Created attachment 8649670 [details] [diff] [review]
> Part 1: Move mIsRunningOnCompositor flag to KeyframeEffectReadOnly.
> 
> KeyframeEffectReadonly stores the flag for each property respectively.
> This patch adds a test case that animation has two properties.
> One can be run on the compositor, the other can not be.
> 
> One thing is not clear to me is that mIsRunningOnCompositor should be false
> when the animation is running on both of threads?

We should update the WebIDL to expose which properties are running on the compositor. I'm not quite sure how that should look. Thinking out loud:

// This is a really bad name, I'm not sure what to call it
enum RunningStatus {
  "compositor",
  "main-thread"
  // Eventually we could extend this to indicate *why* it's not running
  // on the compositor (e.g. layer too large, layer too small, property
  // affects layout etc.)
};

dictionary AnimationPropertyStatus {
  DOMString property;
  RunningStatus runningStatus;
  // Later we might like to add a member like 'bool isOverridden' to indicate
  // if the property is being overridden by another animation (e.g. CSS animations
  // override CSS transitions and in future script-generated animations will
  // override).
};

partial interface KeyframeEffectReadOnly {
  // Not sure if this should be a FrozenArray or not
  [ChromeOnly] sequence<AnimationProperty> getPropertyStatus();
};

For the time being, I suspect IsRunningOnCompositor should return true if *any* of the properties are running on the compositor.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Remove Animation::ClearIsRunningOnCompositor

There were two cases we need to reset IsRunningOnCompositor flag in
nsAnimationManager::CheckAnimationRule.

a) effect timing is changed from style
In this case, KeyframeEffectReadOnly::SetTiming ensures that
ResetIsRunningOnCompositor is called if necessary.

b) the animation is paused from style
In this case, Animation::DoPause() calls ResetIsRunningOnCompositor
there.

I should note about case a).
In most cases of a), animations work well without ResetIsRunningOnCompositor in KeyframeEffectReadOnly::SetTiming() because the animation will be paused when the animation needs to be composed style on effect.SetTiming(). But there are few cases that the animation will not be paused when the animation needs to be composed style. One of the case is in test_animations_omta.html.
http://hg.mozilla.org/mozilla-central/file/f384789a29dc/layout/style/test/test_animations_omta.html#l1337

I don't know why the animation needs to be composed at that time yet. We could remove ResetIsRunningOnCompositor in KeyframeEffectReadOnly::SetTiming somehow, but I would like to leave it in future.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #14)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > Created attachment 8649670 [details] [diff] [review]
> > Part 1: Move mIsRunningOnCompositor flag to KeyframeEffectReadOnly.
> > 
> > KeyframeEffectReadonly stores the flag for each property respectively.
> > This patch adds a test case that animation has two properties.
> > One can be run on the compositor, the other can not be.
> > 
> > One thing is not clear to me is that mIsRunningOnCompositor should be false
> > when the animation is running on both of threads?
> 
> We should update the WebIDL to expose which properties are running on the
> compositor. I'm not quite sure how that should look. Thinking out loud:
> 
> // This is a really bad name, I'm not sure what to call it
> enum RunningStatus {
>   "compositor",
>   "main-thread"
>   // Eventually we could extend this to indicate *why* it's not running
>   // on the compositor (e.g. layer too large, layer too small, property
>   // affects layout etc.)
> };
> 
> dictionary AnimationPropertyStatus {
>   DOMString property;
>   RunningStatus runningStatus;
>   // Later we might like to add a member like 'bool isOverridden' to indicate
>   // if the property is being overridden by another animation (e.g. CSS
> animations
>   // override CSS transitions and in future script-generated animations will
>   // override).
> };
> 
> partial interface KeyframeEffectReadOnly {
>   // Not sure if this should be a FrozenArray or not
>   [ChromeOnly] sequence<AnimationProperty> getPropertyStatus();
> };
> 
> For the time being, I suspect IsRunningOnCompositor should return true if
> *any* of the properties are running on the compositor.

Thanks for the detailed explanations! Filed bug 1196114 for the updating webidl.

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8649667 [details] [diff] [review]
Part 0: Rewrite test_running_on_compositor.html with add_task()

This is good.

>--- a/dom/animation/test/chrome/test_running_on_compositor.html
>+++ b/dom/animation/test/chrome/test_running_on_compositor.html
...
>-/** Test for bug 1045994 - Add a chrome-only property to inspect if an
>-    animation is running on the compositor or not **/
>-
>-SimpleTest.waitForExplicitFinish();
>-
>-var div = document.querySelector('div.target');
>+function addDiv(attrs) {
>+  var div = document.createElement('div');
>+  if (attrs) {
>+    for (var attrName in attrs) {
>+      div.setAttribute(attrName, attrs[attrName]);
>+    }
>+  }
>+  document.body.appendChild(div);
>+  return div;
>+}

I think you could leave the comment at the top?

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8649671 [details] [diff] [review]
Part 2:  Animation.IsRunningOnCompositor should be false when the animation is finished state

This doesn't seem right. Why are we resetting the compositor state when doing finished notifications? It gets taken off the compositor much sooner than that, right?

We should make this member reflect whether the animation is on a layer or not. I think we need some way, during display list processing, to clear all mIsRunningOnCompositor flags and then set them true again for only those animations we add to layers.

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

See comment 0:
> We might need another means of indicating that animations need to be
> re-added to layers but it's possible we have that now (now that bugs like
> bug 1073336 have made the RestyleTracker recognize differents in layer
> generations). We could unconditionally set mIsRunningOnCompositor to false
> for the specified property at the start of AddAnimationsForProperty in
> nsDisplayList.cpp but that wouldn't cover the case where we didn't create a
> layer at all so that would need more thought.

I think what we need is at the point where we get animations for compositor, we call some other method on the manager that basically clears the mIsRunningOnCompositor flag on all animations. Then when we add animations to a layer we'll set it back again.

I think if we do that we can get rid of all other places where we clear the mIsRunningOnCompositor flag (e.g. DoPause etc.).

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8649672 [details] [diff] [review]
Part 3: Remove Animation::SetIsRunningOnCompositor.

Clearing these review requests since I think we will need to redo these patches based on comment 18.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Thanks Brian. I will figure out how we can the mIsRunningOnCompositor flag with corresponding layers.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Rough patch to control mIsRunningCompositor whether the animation is on the layer or not

Still very ugly but seems to work as expected.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP

I am sorry I did misunderstand again in the previous patch.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 0: Rewrite test_running_on_compositor.html with add_task()

The comment pointed out in is left there.
Carrying over review+.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Manage mIsRunningOnCompositor flags for each properties respectively

I quit using std::map to store mIsRunningOnCompositor because of its cost.

Clearing all mIsRunningOnCompositor is done in AnimationCollection::RequestRestyle if RestyleType != Throttled.

All tests were moved into another patch.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Additional tests for Animation.IsRunningOnCompositor

Add some test cases:

1) non-compositor animation
2) compotitor and non-compositor animations
3) at animation.pause()
4) after animation.finish()
5) after finished state
6) in delay time.

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8653417 [details] [diff] [review]
Part 1: Manage mIsRunningOnCompositor flags for each properties respectively

>diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
>--- a/layout/style/AnimationCommon.cpp
>+++ b/layout/style/AnimationCommon.cpp
>@@ -976,16 +976,22 @@ AnimationCollection::RequestRestyle(Rest
>         !CanThrottleAnimation(now)) {
>       aRestyleType = RestyleType::Standard;
>     }
>   }
> 
>   if (aRestyleType >= RestyleType::Standard) {
>     mHasPendingAnimationRestyle = true;
>     PostRestyleForAnimation(presContext);
>+    for (auto& anim : mAnimations) {
>+      dom::KeyframeEffectReadOnly* effect = anim->GetEffect();
>+      if (effect) {
>+        effect->ResetIsRunningOnCompositor();
>+      }
>+    }
>     return;
>   }
> 
>   // Steps for RestyleType::Throttled:
> 
>   MOZ_ASSERT(aRestyleType == RestyleType::Throttled,
>              "Should have already handled all non-throttled restyles");
>   presContext->Document()->SetNeedStyleFlush();

This looks better but I don't quite understand this part. This will clear the flag at the point where we request a restyle. In between this time and when we actually do the restyle, the flag will be incorrect, right?

I thought we could just clear the flags when doing display list building but maybe this is difficult. For example, I thought we could clear the flags at [1] but I guess that won't work when we drop the layer altogether or if it becomes inactive. How hard is it to detect when we drop the layer?

[1] https://dxr.mozilla.org/mozilla-central/rev/f8086bd3c84fc1a42c3625cf3cc2253f0a5e8cfd/layout/base/nsDisplayList.cpp#525

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, busy 22~30 Aug) from comment #27)
> Comment on attachment 8653417 [details] [diff] [review]
> Part 1: Manage mIsRunningOnCompositor flags for each properties respectively
> 
> >diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
> >--- a/layout/style/AnimationCommon.cpp
> >+++ b/layout/style/AnimationCommon.cpp
> >@@ -976,16 +976,22 @@ AnimationCollection::RequestRestyle(Rest
> >         !CanThrottleAnimation(now)) {
> >       aRestyleType = RestyleType::Standard;
> >     }
> >   }
> > 
> >   if (aRestyleType >= RestyleType::Standard) {
> >     mHasPendingAnimationRestyle = true;
> >     PostRestyleForAnimation(presContext);
> >+    for (auto& anim : mAnimations) {
> >+      dom::KeyframeEffectReadOnly* effect = anim->GetEffect();
> >+      if (effect) {
> >+        effect->ResetIsRunningOnCompositor();
> >+      }
> >+    }
> >     return;
> >   }
> > 
> >   // Steps for RestyleType::Throttled:
> > 
> >   MOZ_ASSERT(aRestyleType == RestyleType::Throttled,
> >              "Should have already handled all non-throttled restyles");
> >   presContext->Document()->SetNeedStyleFlush();
> 
> This looks better but I don't quite understand this part. This will clear
> the flag at the point where we request a restyle. In between this time and
> when we actually do the restyle, the flag will be incorrect, right?

Right.

> I thought we could just clear the flags when doing display list building but
> maybe this is difficult. For example, I thought we could clear the flags at
> [1] but I guess that won't work when we drop the layer altogether or if it
> becomes inactive. How hard is it to detect when we drop the layer?

I did try to clear the flags there once but it did not work well when animation is finished.
I could not find where the animation gets off the layer at that time but today I think I probably found the place.

http://hg.mozilla.org/mozilla-central/file/87e23922be37/layout/base/FrameLayerBuilder.cpp#l1767

I will revise part 1 patch.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Manage mIsRunningOnCompositor flags for each properties respectively

Now mIsRunningOnCompositor exactly matches to its property.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Additional tests for Animation.IsRunningOnCompositor v2

Changes:

* adds a test case for animation.cancel().
* waits a frame before checking isRunningOnCompositor flag in some test cases (calling finish(), after finished promise, and calling cancel()) because finished and ready promises are fullfilled/rejected immediately in the cases.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #30)
> * waits a frame before checking isRunningOnCompositor flag in some test
> cases (calling finish(), after finished promise, and calling cancel())
> because finished and ready promises are fullfilled/rejected immediately in
> the cases.

finished promise does not fullfilled immediately but it is fullfilled before updating isRunningOnCompositor anyway.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8653964 [details] [diff] [review]
Part 2: Additional tests for Animation.IsRunningOnCompositor v2

Clearing review request since finish/cancel tests fail occasionally.

Comment 33

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)
> Comment on attachment 8653964 [details] [diff] [review]
> Part 2: Additional tests for Animation.IsRunningOnCompositor v2
> 
> Clearing review request since finish/cancel tests fail occasionally.

The failure is caused by asynchronous of DidComposite. DidComposite leads ClearIsRunningOnCompositor. (DidComposite -> nsRefreshDriver::FinishedWaitingForTransaction -> nsRefreshDriver::Tick -> PresShell::Paint -> nsDisplayList::PaintRoot -> FrameLayerBuilder::WillEndTransaction -> ClearIsRunningOnCompositor)

The tests are succeeded if DidComposite was received before the first rAF. The tests are not succeeded if DidComposite was not received before the first rAF.

As far as I've been confirming locally, waiting one more frame prevents the failures.
In real use cases, it's not particularly problem because it will only happen when animation is finished and cancelled.

Comment 34

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks for doing this. I've been thinking about the different approaches in attachment 8653417 [details] [diff] [review] and attachment 8653959 [details] [diff] [review] for part 1. The second patch is more accurate but it also involves more code, from comment 33, it sounds like it is difficult to test. It also seems to produce less intuitive results since we don't update the flag until one tick later. I'm also somewhat uncomfortable about fiddling with the destructor for DisplayItemData for this and not entirely confident we have covered every possible case where an animation may stop running on the compositor.

So, the original patch (attachment 8653417 [details] [diff] [review]), seems preferable. That patch, however, introduces the problem that we clear the flag before we actually remove the animation on the layer. Furthermore, in some circumstances we will temporarily clear the flag even when the animation *remains* on the layer (for example, when we force an unthrottled sample on a transform animation because it affects the overflow area).

That seems undesirable but I think it's actually ok. If we consider how this flag is actually used, we have:

a) to detect if we can throttle a sample -- suppose we clear this flag even if the animation *remains* on the layer, all that will happen is that for all other animations on the element we will skip requesting a throttled restyle and immediately request a standard restyle -- something that would have happened anyway

b) to report to DevTools that the animation is running on the compositor -- if we clear the flag in RequestRestyle we should process pending restyles before devtools next queries that flag so it should be reset by then -- I need to verify this, however

So I think attachment 8653417 [details] [diff] [review] might actually be ok. What do you think?

Comment 35

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, busy 22~30 Aug) from comment #34)
> Thanks for doing this. I've been thinking about the different approaches in
> attachment 8653417 [details] [diff] [review] and attachment 8653959 [details] [diff] [review]
> [details] [diff] [review] for part 1. The second patch is more accurate but
> it also involves more code, from comment 33, it sounds like it is difficult
> to test. It also seems to produce less intuitive results since we don't
> update the flag until one tick later. I'm also somewhat uncomfortable about
> fiddling with the destructor for DisplayItemData for this and not entirely
> confident we have covered every possible case where an animation may stop
> running on the compositor.
> 
> So, the original patch (attachment 8653417 [details] [diff] [review]), seems
> preferable. That patch, however, introduces the problem that we clear the
> flag before we actually remove the animation on the layer. Furthermore, in
> some circumstances we will temporarily clear the flag even when the
> animation *remains* on the layer (for example, when we force an unthrottled
> sample on a transform animation because it affects the overflow area).
> 
> That seems undesirable but I think it's actually ok. If we consider how this
> flag is actually used, we have:
> 
> a) to detect if we can throttle a sample -- suppose we clear this flag even
> if the animation *remains* on the layer, all that will happen is that for
> all other animations on the element we will skip requesting a throttled
> restyle and immediately request a standard restyle -- something that would
> have happened anyway
> 
> b) to report to DevTools that the animation is running on the compositor --
> if we clear the flag in RequestRestyle we should process pending restyles
> before devtools next queries that flag so it should be reset by then -- I
> need to verify this, however
> 
> So I think attachment 8653417 [details] [diff] [review] might actually be
> ok. What do you think?

Thanks for detailed clarifications. I am just worrying about rAF callback is called between RequestRestyle and Restyle. I will investigate it now.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Manage mIsRunningOnCompositor flags for each properties respectively v2

This patch is based on attachment#8653417 [details] [diff] [review].

Changes from attachment#8653417 [details] [diff] [review]:
* Factor out AnimationCollection::ResetIsRunningOnCompositor.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> Created attachment 8654718 [details] [diff] [review]
> Part 1: Manage mIsRunningOnCompositor flags for each properties respectively
> v2
> 
> This patch is based on attachment#8653417 [details] [diff] [review] [diff] [review].
> 
> Changes from attachment#8653417 [details] [diff] [review] [diff] [review]:
> * Factor out AnimationCollection::ResetIsRunningOnCompositor.

I forgot to say that I confirmed the flag is good in rAF callback. I will update tests soon.

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Additional tests for Animation.IsRunningOnCompositor v3

Changes from v2:
* Remove codes of waiting a frame.
* Add test case to check in rAF callback while animation is running.
* Add test case to check in MutationObserver callback while animation is running.

Comment 39

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8654718 [details] [diff] [review]
Part 1: Manage mIsRunningOnCompositor flags for each properties respectively v2

>+void
>+KeyframeEffectReadOnly::SetIsRunningOnCompositor(nsCSSProperty aProperty)
>+{
>+  MOZ_ASSERT(aProperty == eCSSProperty_opacity ||
>+             aProperty == eCSSProperty_transform,
>+             "To run new property on compositor properly "
>+             "KeyframeEffect.mIsRunningCompositors has to be modified");

"Property being animated on compositor is a recognized compositor-animatable property"

>+  if (aProperty == eCSSProperty_opacity) {
>+    mIsRunningOnCompositors[CSSPropertyOnCompositor::Opacity] = true;
>+  } else if (aProperty == eCSSProperty_transform) {
>+    mIsRunningOnCompositors[CSSPropertyOnCompositor::Transform] = true;
>+  }
>+}

This, and the assertion above, should use the information in CommonAnimationManager::sLayerAnimationInfo.

>+
>+  enum CSSPropertyOnCompositor {
>+    Opacity,
>+    Transform,
>+    LastProperty = Transform
>+  };
>+  bool mIsRunningOnCompositors[CSSPropertyOnCompositor::LastProperty + 1];
> };

This should also use CommonAnimationManager::sLayerAnimationInfo.


The rest looks good but I'd like to have a final look at the above changes. Thanks!

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Manage mIsRunningOnCompositor flags for each properties respectively v3

Including AnimationCommon.h in KeyframeEffect.h and using CommonAnimationManager::csLayerAnimationInfo or CommonAnimationManager::sLayerAnimationInfo auses various compile errors. For exapmle:

In file included from /home/zoe/mozilla-central/layout/style/AnimationCommon.cpp:6:
In file included from /home/zoe/mozilla-central/layout/style/AnimationCommon.h:19:
In file included from ../../dist/include/mozilla/dom/Animation.h:17:
../../dist/include/mozilla/dom/KeyframeEffect.h:324:44: error: use of undeclared identifier 'CommonAnimationManager'; did you mean 'nsAnimationManager'?
  bool mIsRunningOnCompositors[ArrayLength(CommonAnimationManager::sLayerAnimationInfo)];

That is because KeyframeEffect.h is also included from AnimationCommon.h through Animation.h.

So this patch just moves kLayerRecords into KeyFrameEffect.h to avoid those errors. Other stuff related to sLayerAnimationInfo will be moved by another bug.

Comment 41

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8655086 [details] [diff] [review]
Part 1: Manage mIsRunningOnCompositor flags for each properties respectively v3

>diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp
>--- a/dom/animation/KeyframeEffect.cpp
>+++ b/dom/animation/KeyframeEffect.cpp
>+bool
>+KeyframeEffectReadOnly::IsRunningOnCompositor() const
>+{
>+  // We consider animation is running on compositor if there is at least
>+  // one property running on compositor.
>+  // Animation.IsRunningOnCompotitor will return more fine grained
>+  // information in bug 1196114.
>+  const auto& info = CommonAnimationManager::sLayerAnimationInfo;
>+  for (size_t i = 0; i < ArrayLength(info); i++) {

This should just be ArrayLength(mIsRunningOnCompositors). No need for the reference to sLayerAnimationInfo.

>diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
>--- a/dom/animation/KeyframeEffect.h
>+++ b/dom/animation/KeyframeEffect.h
>@@ -304,25 +297,32 @@ public:
>   }
> 
>   // Updates |aStyleRule| with the animation values produced by this
>   // Animation for the current time except any properties already contained
>   // in |aSetProperties|.
>   // Any updated properties are added to |aSetProperties|.
>   void ComposeStyle(nsRefPtr<AnimValuesStyleRule>& aStyleRule,
>                     nsCSSPropertySet& aSetProperties);
>+  bool IsRunningOnCompositor() const;
>+  void SetIsRunningOnCompositor(nsCSSProperty aProperty);
>+  void ResetIsRunningOnCompositor();
>+
>+  static const size_t kLayerRecords = 2;

See comment below: I don't think we should move kLayerRecords here.

> protected:
>   virtual ~KeyframeEffectReadOnly() { }
> 
>   nsCOMPtr<Element> mTarget;
>   Nullable<TimeDuration> mParentTime;
> 
>   AnimationTiming mTiming;
>   nsCSSPseudoElements::Type mPseudoType;
> 
>   InfallibleTArray<AnimationProperty> mProperties;
>+
>+  bool mIsRunningOnCompositors[kLayerRecords];
> };

This should probably be called, mIsPropertyRunningOnCompositor, with a comment such as:

// Parallel array corresponding to CommonAnimationManager::sLayerAnimationInfo
// such that mIsPropertyRunningOnCompositor[x] is true only if this effect has
// an animation of CommonAnimationManager::sLayerAnimationInfo[x].mProperty that
// is currently running on the compositor.
//
// Note that when the owning Animation requests a non-throttled restyle, in
// between calling RequestRestyle on its AnimationCollection and when the
// restyle is performed, this member may temporarily become false even if
// the animation remains on the layer after the restyle.

>diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h
>--- a/layout/style/AnimationCommon.h
>+++ b/layout/style/AnimationCommon.h
>@@ -115,21 +115,19 @@ public:
>   // For CSS properties that may be animated on a separate layer, represents
>   // a record of the corresponding layer type and change hint.
>   struct LayerAnimationRecord {
>     nsCSSProperty mProperty;
>     nsDisplayItem::Type mLayerType;
>     nsChangeHint mChangeHint;
>   };
> 
>-protected:
>-  static const size_t kLayerRecords = 2;
>-
> public:
>-  static const LayerAnimationRecord sLayerAnimationInfo[kLayerRecords];
>+  static const LayerAnimationRecord
>+    sLayerAnimationInfo[dom::KeyframeEffectReadOnly::kLayerRecords];

I think we should leave this here, make it public, and refer to it from KeyframeEffectReadOnly.

Also, can you test using ArrayLength(AnimationCommon::sLayerAnimationInfo) in KeyframeEffectReadOnly?

Long-term ArrayLength should be constexpr on all platforms (currently it's MOZ_CONSTEXPR so it won't work until we upgrade to VS 2015) so if this works on platforms that support constexpr we should add a comment here saying to switch to using AnimationCommon::sLayerAnimationInfo (and make kLayerRecords protected again) once we support constexpr.


r=me with those changes

Comment 42

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Regarding the interaction with requestAnimationFrame, here is my understanding of how it works. Please check if this is correct.

1. nsRefreshDriver::Tick runs and iterates over all style refresh observers.
   https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/layout/base/nsRefreshDriver.cpp#1549
   This will include the animation and transition managers. As they tick their animations they may generate calls to AnimationCollection::RequestRestyle with type RestyleType::Standard. Also, with part 1 from this bug they will clear the "is running on compositor" flags.

2. RequestRestyle will call PostRestyleForAnimation which ultimately translates to a call to RestyleManager::PostRestyleEvent

3. PostRestyleEvent results in adding a pending restyle and then:
   a. Adds the restyle manager as a style flush observer of the refresh driver
   b. Marks the document as needing a style flush

4. Back in the refresh driver we dispatch animation events and run requestAnimationFrame callbacks
   (Note that, as discussed, we get the list of callbacks to call once before calling any of them. It's not recursive. https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/layout/base/nsRefreshDriver.cpp#1388(\)

5. Then *after* that we call style flush observers like those registered in 3a.

So I think, with this patch, we can hit the following situation:

* We have a transform animation running on the compositor
* We tick it and it requests a throttled restyle
* We get to AnimationCollection::RequestRestyle and then call CanThrottleAnimation:
  https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/layout/style/AnimationCommon.cpp?offset=0#976
* Every 200ms CanThrottleAnimation returns false because it calls CanThrottleTransformChanges and it occasionally returns false if the transform animation affects the overflow region
* So we end up clearing the "is running on compositor" flag
* Then we run animation event handlers and requestAnimationFrame callbacks *before* we process the pending restyle--hence script sees "is running on compositor == false"

Does that seem possible? Can you create a test to reproduce that sort of situation?

If so, then I think we could probably fix it simply by making the IsRunningOnCompositor API method call FlushPendingNotifications like we do for various other methods. That seems somewhat undesirable but probably ok so long as this is a chrome-only API.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #41)
> >diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h
> >--- a/layout/style/AnimationCommon.h
> >+++ b/layout/style/AnimationCommon.h
> >@@ -115,21 +115,19 @@ public:
> >   // For CSS properties that may be animated on a separate layer, represents
> >   // a record of the corresponding layer type and change hint.
> >   struct LayerAnimationRecord {
> >     nsCSSProperty mProperty;
> >     nsDisplayItem::Type mLayerType;
> >     nsChangeHint mChangeHint;
> >   };
> > 
> >-protected:
> >-  static const size_t kLayerRecords = 2;
> >-
> > public:
> >-  static const LayerAnimationRecord sLayerAnimationInfo[kLayerRecords];
> >+  static const LayerAnimationRecord
> >+    sLayerAnimationInfo[dom::KeyframeEffectReadOnly::kLayerRecords];
> 
> I think we should leave this here, make it public, and refer to it from
> KeyframeEffectReadOnly.
> 
> Also, can you test using ArrayLength(AnimationCommon::sLayerAnimationInfo)
> in KeyframeEffectReadOnly?

Neither CommonAnimationManager::sLayerAnimationInfo nor CommonAnimationManager::kLayerRecords can be used in KeyframeEffect.h because AnimationCommon.h includes KeyframeEffect.h indirectly. When a file (say AnimationCommon.cpp) includes AnimationCommon.h, KeyframeEffect.h is read from the header of AnimationCommon.h, but at this time CommonAnimationManager::kLayerRecords and LayerAnimationInfo are not declared yet. It results the errors in comment#40. Am I missing something?

Comment 44

[Hiroyuki Ikezoe (:hiro)]

needinfo flag was cleared accidentally.

Comment 45

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #43)
> Neither CommonAnimationManager::sLayerAnimationInfo nor
> CommonAnimationManager::kLayerRecords can be used in KeyframeEffect.h
> because AnimationCommon.h includes KeyframeEffect.h indirectly. When a file
> (say AnimationCommon.cpp) includes AnimationCommon.h, KeyframeEffect.h is
> read from the header of AnimationCommon.h, but at this time
> CommonAnimationManager::kLayerRecords and LayerAnimationInfo are not
> declared yet. It results the errors in comment#40. Am I missing something?

Can't we just move the layer record information to a separate LayerAnimations.h/.cpp file pair?

We can drop the CommonAnimationManager::LayerAnimationRecordFor method at the same time since it's not used anymore.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #45)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #43)
> > Neither CommonAnimationManager::sLayerAnimationInfo nor
> > CommonAnimationManager::kLayerRecords can be used in KeyframeEffect.h
> > because AnimationCommon.h includes KeyframeEffect.h indirectly. When a file
> > (say AnimationCommon.cpp) includes AnimationCommon.h, KeyframeEffect.h is
> > read from the header of AnimationCommon.h, but at this time
> > CommonAnimationManager::kLayerRecords and LayerAnimationInfo are not
> > declared yet. It results the errors in comment#40. Am I missing something?
> 
> Can't we just move the layer record information to a separate
> LayerAnimations.h/.cpp file pair?

Yes, that will work fine.

> We can drop the CommonAnimationManager::LayerAnimationRecordFor method at
> the same time since it's not used anymore.

Sure. I will do it.

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Ooops, needinfo was cleared again.

Well, an interim report about comment#42.
I could check each steps (1 to 5), all of them are true (Thanks for the detail explanations!). And the test is here.

add_task(function* in_request_animation_callback_on_overflow_animation() {
  // Needs scrollbars to cause overflow.
  SpecialPowers.setIntPref("ui.showHideScrollbars", 1);

  var div = addDiv({ style: 'animation: rotate 100s' });
  var animation = div.getAnimations()[0];

  yield animation.ready;

  yield new Promise(function(resolve) {
    var timeAtStart = window.performance.now();
    function handleFrame() {
      is(animation.isRunningOnCompositor, omtaEnabled,
         'Animation reports that it is running on the compositor'
          + ' in requestAnimationFrame callback');

      // we have to wait at least 200ms because this animation is not
      // throttled on every 200ms.
      // See http://hg.mozilla.org/mozilla-central/file/cafb1c90f794/layout/style/AnimationCommon.cpp#l863
      if (window.performance.now() - timeAtStart > 200) {
        resolve();
      }
      window.requestAnimationFrame(handleFrame);
    }

    window.requestAnimationFrame(handleFrame);
  });

  div.parentNode.removeChild(div);
});

As Brian expected this test fails (amazing!), but unfortunately calling Animation::FlushStyle or FlushPendingNotifications like refresh driver does[1] does not solve the failure. I am now investigating the failure reason.

[1] https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/layout/base/nsRefreshDriver.cpp#1587

Comment 48

[Hiroyuki Ikezoe (:hiro)]

I confirmed that FlushPendingNotifications has no effect on fixing the failure. That is because FlushPendingNotifications does not involves calling AddAnimationsForProperty.

ProcessPendingUpdates here[1] involves calling AddAnimationsForProperty.
[1] https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/layout/base/nsRefreshDriver.cpp#1721

Calling ProcessPendingUpdates in CSSAnimation::IsRunningOnCompositor() does solve the failure but it gets 100x slower than without the call. I think this cost is not acceptable.

So we have a couple of choices:

a) Do not clear the isRunningOnCompositor flag when the animation is transform and mStyleRuleRefreshTime is 200ms earlier.

b) Leave this issue as a limitation and document it in Animation.webidl.

c) Back to clearing the flag in nsDisplayItem dtor.

I'd like to take a) for now.

Comment 49

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #48)
> I confirmed that FlushPendingNotifications has no effect on fixing the
> failure. That is because FlushPendingNotifications does not involves calling
> AddAnimationsForProperty.
> 
> ProcessPendingUpdates here[1] involves calling AddAnimationsForProperty.
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> f2518b8a7b97b5bb477e94bc9131584007aac887/layout/base/nsRefreshDriver.cpp#1721
> 
> Calling ProcessPendingUpdates in CSSAnimation::IsRunningOnCompositor() does
> solve the failure but it gets 100x slower than without the call. I think
> this cost is not acceptable.
> 
> So we have a couple of choices:
> 
> a) Do not clear the isRunningOnCompositor flag when the animation is
> transform and mStyleRuleRefreshTime is 200ms earlier.
> 
> b) Leave this issue as a limitation and document it in Animation.webidl.
> 
> c) Back to clearing the flag in nsDisplayItem dtor.
> 
> I'd like to take a) for now.

I don't think we should do (a). There may be all sorts of conditions we add in the future for forcing a throttled sample and we shouldn't have to add logic to undo each of them just for this API.

Also, looking at bug 1166500 comment 12, I think we will have other problems if we temporarily reset isRunningOnCompositor prior to display list building. I guess we need to do something like (c) after all.

Comment 50

[Hiroyuki Ikezoe (:hiro)]

I am still thinking how we can avoid a frame in mochitests mentioned by comment#33. 

Before that I am going to post patches that moves CommonAnimationManager::sLayerAnimationInfo now.

Comment 51

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Move CommonAnimationManager::sLayerAnimationInfo into LayerAnimamtions.(cpp|h).

Comment 52

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: imgTools should be inside mozilla::image namespace.

If KeyframeEffect.h includes a header files in layers we will get below errror:

 /home/zoe/mozilla-central/image/imgTools.cpp:72:34: error: reference to 'ImageFactory' is ambiguous
   nsRefPtr<image::Image> image = ImageFactory::CreateAnonymousImage(mimeType);
                                  ^
 /home/zoe/mozilla-central/image/ImageFactory.h:24:7: note: candidate found by name lookup is 'mozilla::image::ImageFactory'
 class ImageFactory
       ^
 ../dist/include/ImageContainer.h:245:7: note: candidate found by name lookup is 'mozilla::layers::ImageFactory'
 class ImageFactory

That is because KeyframeEffect.h is included from Animation.h, Animation.h is indirectly included from image/SVGDocumentWrapper.cpp, SVGDocumentWrapper.cpp is is unified with imgTools.cpp on unified build.

So this patch just adds namespaces in imgTools to avoid the error.

Comment 53

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8656408 [details] [diff] [review]
Part 2: Move CommonAnimationManager::sLayerAnimationInfo into LayerAnimamtions.(cpp|h).

Looks great. Thanks for doing this.

A few suggestions:

1) I think 'LayerAnimationInfo' might be better than 'LayerAnimations'. (I'd expect a data structure called LayerAnimations to be something like an array of actual animations.) Then maybe rename 'sInfo' to 'sRecords'?

2) In quite a few place we have 'for (auto& layerInfo : LayerAnimations::sInfo)'. I would have expected 'const auto& layerInfo' but does 'auto' here expand to 'const LayerAnimations::Record'?

3) The patch header has a spelling mistake:
> Bug 1151694 - Part 2: Move CommonAnimationManager::sLayerAnimationInfo into LayerAnimamtions.(cpp|h).

Comment 54

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #49)
> Also, looking at bug 1166500 comment 12, I think we will have other problems
> if we temporarily reset isRunningOnCompositor prior to display list
> building. I guess we need to do something like (c) after all.

Expanding on this comment, what I am concerned about is this:

1. We tick animation A which animates the 'left' property of element X on the main thread. Hence it calls AnimationCollection::RequestRestyle(RestyleType::Standard).

2. With the approach in attachment #8655086 [details] [diff] [review], we would call ResetIsRunningOnCompositor() which would clear the 'is running on compositor' flags on all animations for element X.

3. We tick animation B which animates the 'transform' property of element X on the compositor and it has just finished.

4. According to the plan in bug 1166500 comment 12, part d, we would like to use the 'is running on compositor' flag to determine if we need to update layers. Specifically, we'd like to add a method called HasPropertyRunningOnCompositor() and use that so that when a compositor animation finishes, we update the layers.

However, in step 2 we already cleared that flag so we don't know anymore that animation B is running on the compositor.

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Move CommonAnimationManager::sLayerAnimationInfo into LayerAnimamtions.(cpp|h) v2

Addressed comments in comment#53.
The part number of the previous patch was wrong, it should be 'Part 1' actually.

Comment 56

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: imgTools should be inside mozilla::image namespace.

Fixed part number as well.

Comment 57

[Hiroyuki Ikezoe (:hiro)]

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

Comment 58

[Patrick Brosset <:pbro>]

I've been following the activity on this bug a little bit from afar but I see that it's progressed well now. I'd like to discuss about the implications for devtools. Right now devtools only show individual animations, not animated properties (like opacity), and so we've been using this flag to simply signal to the user when an animation was running on the compositor.
Are the patches landing here going to change the way this chrome-only flag works? Or will it simply be more accurate? What about animated properties, how will we know which are running on the compositor from javascript?

Comment 59

[Hiroyuki Ikezoe (:hiro)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #58)

> Are the patches landing here going to change the way this chrome-only flag
> works? Or will it simply be more accurate? What about animated properties,
> how will we know which are running on the compositor from javascript?

The purpose of this bug just makes the isRunningOnCompositor flag more accurate. I think bug 1196114 is what you are looking for.

Comment 60

[Patrick Brosset <:pbro>]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #59)
> The purpose of this bug just makes the isRunningOnCompositor flag more
> accurate. I think bug 1196114 is what you are looking for.
Ah that's it, thanks! Turns out I had already cc'd myself on that bug.

Comment 61

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5785790b3435
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea80856ff2f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/21e38aa30e1b

Comment 62

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5785790b3435
https://hg.mozilla.org/mozilla-central/rev/ea80856ff2f4
https://hg.mozilla.org/mozilla-central/rev/21e38aa30e1b

Comment 63

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Clear isRunningOnCompositor flag in AddAnimationsAndTransitionsToLayer and dtor of DisplayItemData.

This patch is a revised patch of attachment#8653959 [details] [diff] [review].

As I wrote in comment#33, with this approach we sometimes need to wait two frames to be isRunningOnCompositor flag cleared when animation is finished.

After discussion with Brian, I found the reason why we need to wait two frames. Ideally it should be one frame.
The reason is that refresh driver for browser.xul(the refresh driver invokes nsDisplayList::PaintRoot and then invokes AddAnimationsAndTransitionsToLayer or dtor of DisplayItemData) has been removed before the frame due to ObserverCount() check[1]. So the refresh driver did not Tick in first frame after animation is finished.

[1] http://hg.mozilla.org/mozilla-central/file/5fe9ed3edd68/layout/base/nsRefreshDriver.cpp#l1523

Comment 64

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #63)
> Created attachment 8658069 [details] [diff] [review]
> Part 3: Clear isRunningOnCompositor flag in
> AddAnimationsAndTransitionsToLayer and dtor of DisplayItemData.
> 
> This patch is a revised patch of attachment#8653959 [details] [diff] [review] [diff]
> [review].
> 
> As I wrote in comment#33, with this approach we sometimes need to wait two
> frames to be isRunningOnCompositor flag cleared when animation is finished.
> 
> After discussion with Brian, I found the reason why we need to wait two
> frames. Ideally it should be one frame.
> The reason is that refresh driver for browser.xul(the refresh driver invokes
> nsDisplayList::PaintRoot and then invokes AddAnimationsAndTransitionsToLayer
> or dtor of DisplayItemData) has been removed before the frame due to
> ObserverCount() check[1]. So the refresh driver did not Tick in first frame
> after animation is finished.
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/5fe9ed3edd68/layout/base/
> nsRefreshDriver.cpp#l1523

]
> [review].
> 
> As I wrote in comment#33, with this approach we sometimes need to wait two
> frames to be isRunningOnCompositor flag cleared when animation is finished.
> 
> After discussion with Brian, I found the reason why we need to wait two
> frames. Ideally it should be one frame.
> The reason is that refresh driver for browser.xul(the refresh driver invokes
> nsDisplayList::PaintRoot and then invokes AddAnimationsAndTransitionsToLayer
> or dtor of DisplayItemData) has been removed before the frame due to
> ObserverCount() check[1]. So the refresh driver did not Tick in first frame
> after animation is finished.

To ensure that the refresh driver ticks on the first frame after animation is finished, we need to call nsRefreshDriver::ScheduleViewManagerFlush() or nsIFrame::SchedulePaint() to wake up the refresh driver.

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Clear isRunningOnCompositor flag in AddAnimationsAndTransitionsToLayer and when the coresspoinding display item is removed.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #63)

Changes from the attachment #8658069 [details] [diff] [review].

* Clear the isRunninOnCompositor flag when the DisplayItemData which is corresponding to the layer is removed from display list, instead of clearing in dtor of DisplayitemData.


> After discussion with Brian, I found the reason why we need to wait two
> frames. Ideally it should be one frame.
> The reason is that refresh driver for browser.xul(the refresh driver invokes
> nsDisplayList::PaintRoot and then invokes AddAnimationsAndTransitionsToLayer
> or dtor of DisplayItemData) has been removed before the frame due to
> ObserverCount() check[1]. So the refresh driver did not Tick in first frame
> after animation is finished.

This behavior can be also seen when animation.pause() is called. 
From bug 1073336 comment#27:

  pause()
    Posting restyle for animation
  * Tick
    Flushing styles
      Running ComposeStyle
      CheckAnimationRule
  * Tick
    PresShell::ScheduleViewManagerFlush
    PresShell::ScheduleViewManagerFlush
    ViewManager::ProcessingPendingUpdates
      < Paint >

This is exactly the same as what I've been seeing. So I think it's OK that this behavior occurs on finish() and cancel().

Comment 66

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Additional tests for Animation.IsRunningOnCompositor v4

Changes from v3:
* Add a test case commented in comment#47.

Comment 67

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8658479 [details] [diff] [review]
Part 3: Clear isRunningOnCompositor flag in AddAnimationsAndTransitionsToLayer and when the coresspoinding display item is removed.

>diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp
>--- a/dom/animation/KeyframeEffect.cpp
>+++ b/dom/animation/KeyframeEffect.cpp
>+bool
>+KeyframeEffectReadOnly::IsRunningOnCompositor() const
>+{
>+  // We consider animation is running on compositor if there is at least
>+  // one property running on compositor.
>+  // Animation.IsRunningOnCompotitor will return more fine grained
>+  // information in bug 1196114.
>+  for (const bool& isPropertyRunningOnCompositor : mIsPropertyRunningOnCompositors) {

Does using a const-ref here help in any way? I think this might be better
as just 'bool';

Also, this line needs to be wrapped to fit into 80 characters.

>+void
>+KeyframeEffectReadOnly::SetIsRunningOnCompositor(nsCSSProperty aProperty)
>+{
>+  MOZ_ASSERT(nsCSSProps::PropHasFlags(aProperty,
>+                                      CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR),
>+             "Property being animated on compositor is a recognized "
>+             "compositor-animatable property");
>+  const auto& info = LayerAnimationInfo::sRecords;
>+  for (size_t i = 0; i < ArrayLength(mIsPropertyRunningOnCompositors); i++) {
>+    if (info[i].mProperty == aProperty) {
>+      mIsPropertyRunningOnCompositors[i] = true;
>+      return;
>+    }
>+  }
>+}
>+
>+void
>+KeyframeEffectReadOnly::ClearIsRunningOnCompositor(nsCSSProperty aProperty)
>+{
>+  MOZ_ASSERT(nsCSSProps::PropHasFlags(aProperty,
>+                                      CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR),
>+             "Property being animated on compositor is a recognized "
>+             "compositor-animatable property");
>+  const auto& info = LayerAnimationInfo::sRecords;
>+  for (size_t i = 0; i < ArrayLength(mIsPropertyRunningOnCompositors); i++) {
>+    if (info[i].mProperty == aProperty) {
>+      mIsPropertyRunningOnCompositors[i] = false;
>+      return;
>+    }
>+  }
>+}

We should combine these two methods into one SetIsRunningOnCompositor with
a bool argument.

Also, is it possible to add a static assert that checks that
MOZ_ARRAY_LENGTH(LayerAnimationInfo::sRecords) ==
MOZ_ARRAY_LENGTH(mIsPropertyRunningOnCompositors) ?

>diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
>--- a/dom/animation/KeyframeEffect.h
>+++ b/dom/animation/KeyframeEffect.h
>+
>+  bool mIsPropertyRunningOnCompositors[LayerAnimationInfo::kRecords];

See the feedback in comment 41. Specifically:

(In reply to Brian Birtles (:birtles) from comment #41)
> >+
> >+  bool mIsRunningOnCompositors[kLayerRecords];
> > };
> 
> This should probably be called, mIsPropertyRunningOnCompositor, with a
> comment such as:
> 
> // Parallel array corresponding to CommonAnimationManager::sLayerAnimationInfo
> // such that mIsPropertyRunningOnCompositor[x] is true only if this effect has
> // an animation of CommonAnimationManager::sLayerAnimationInfo[x].mProperty that
> // is currently running on the compositor.
> //
> // Note that when the owning Animation requests a non-throttled restyle, in
> // between calling RequestRestyle on its AnimationCollection and when the
> // restyle is performed, this member may temporarily become false even if
> // the animation remains on the layer after the restyle.

>diff --git a/layout/base/FrameLayerBuilder.cpp b/layout/base/FrameLayerBuilder.cpp
>--- a/layout/base/FrameLayerBuilder.cpp
>+++ b/layout/base/FrameLayerBuilder.cpp
>@@ -226,16 +226,39 @@ FrameLayerBuilder::DisplayItemData::~Dis
>   MOZ_RELEASE_ASSERT(sAliveDisplayItemDatas && sAliveDisplayItemDatas->Contains(this));
>   sAliveDisplayItemDatas->RemoveEntry(this);
>   if (sAliveDisplayItemDatas->Count() == 0) {
>     delete sAliveDisplayItemDatas;
>     sAliveDisplayItemDatas = nullptr;
>   }
> }
> 
>+void
>+FrameLayerBuilder::DisplayItemData::ClearIsRunningOnCompositor()
>+{

I think we should call this something that indicates it has to do with
animation. Maybe ClearAnimationCompositorState() ?

>+  if (mDisplayItemKey != nsDisplayItem::TYPE_TRANSFORM &&
>+      mDisplayItemKey != nsDisplayItem::TYPE_OPACITY) {
>+    return;
>+  }
>+
>+  for (nsIFrame* frame : mFrameList) {
>+    if (mDisplayItemKey == nsDisplayItem::TYPE_TRANSFORM) {
>+      frame->PresContext()->AnimationManager()->
>+        ClearIsRunningOnCompositor(frame, eCSSProperty_transform);
>+      frame->PresContext()->TransitionManager()->
>+        ClearIsRunningOnCompositor(frame, eCSSProperty_transform);
>+    } else if (mDisplayItemKey == nsDisplayItem::TYPE_OPACITY) {
>+      frame->PresContext()->AnimationManager()->
>+        ClearIsRunningOnCompositor(frame, eCSSProperty_opacity);
>+      frame->PresContext()->TransitionManager()->
>+        ClearIsRunningOnCompositor(frame, eCSSProperty_opacity);
>+    }
>+  }
>+}

Can you ask mattwoodrow to review the changes to FrameLayerBuilder since
I'm not familiar with this code.

>@@ -519,19 +519,24 @@ nsDisplayListBuilder::AddAnimationsAndTr
>   // animation generation numbers and update the layer indefinitely.
>   uint64_t animationGeneration =
>     RestyleManager::GetMaxAnimationGenerationForFrame(aFrame);
>   aLayer->SetAnimationGeneration(animationGeneration);
> 
>   nsPresContext* presContext = aFrame->PresContext();
>   AnimationCollection* transitions =
>     presContext->TransitionManager()->GetAnimationsForCompositor(aFrame, aProperty);
>+  if (!transitions) {
>+    presContext->TransitionManager()->ClearIsRunningOnCompositor(aFrame, aProperty);
>+  }
>   AnimationCollection* animations =
>     presContext->AnimationManager()->GetAnimationsForCompositor(aFrame, aProperty);
>-
>+  if (!animations) {
>+    presContext->AnimationManager()->ClearIsRunningOnCompositor(aFrame, aProperty);
>+  }
>   if (!animations && !transitions) {
>     return;
>   }

As discussed, I think this would be easier to follow if we just unconditionally
cleared the flag at the start, e.g.:

  nsPresContext* presContext = aFrame->PresContext();
  presContext->TransitionManager()->ClearIsRunningOnCompositor(aFrame,
                                                               aProperty);
  presContext->AnimationManager()->ClearIsRunningOnCompositor(aFrame,
                                                              aProperty);
  AnimationCollection* transitions =
    presContext->TransitionManager()->GetAnimationsForCompositor(aFrame,
                                                                 aProperty);
  AnimationCollection* animations =
    presContext->AnimationManager()->GetAnimationsForCompositor(aFrame,
                                                                aProperty);


Looks good with those changes.

Comment 68

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8658481 [details] [diff] [review]
Part 4:  Additional tests for Animation.IsRunningOnCompositor v4

>diff --git a/dom/animation/test/chrome/test_running_on_compositor.html b/dom/animation/test/chrome/test_running_on_compositor.html
>--- a/dom/animation/test/chrome/test_running_on_compositor.html
>+++ b/dom/animation/test/chrome/test_running_on_compositor.html
>@@ -38,16 +48,33 @@ function addDiv(attrs) {
>     for (var attrName in attrs) {
>       div.setAttribute(attrName, attrs[attrName]);
>     }
>   }
>   document.body.appendChild(div);
>   return div;
> }
> 
>+/**
>+ * Returns a Promise that is resolved after the given number of consecutive
>+ * animation frames have occured (using requestAnimationFrame callbacks).
>+ */
>+function waitForAnimationFrames(frameCount) {
>+  return new Promise(function(resolve, reject) {
>+    function handleFrame() {
>+      if (--frameCount <= 0) {
>+        resolve();
>+      } else {
>+        window.requestAnimationFrame(handleFrame); // wait another frame
>+      }
>+    }
>+    window.requestAnimationFrame(handleFrame);
>+  });
>+}

Can we just import testcommon.js and use re-use its version of
waitForAnimationFrames?

>+add_task(function* non_compositor_property() {
>+  var div = addDiv({ style: 'animation: background 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  yield animation.ready;
>+
>+  is(animation.isRunningOnCompositor, false,
>+     'Animation reports that it is NOT running on the compositor'
>+     + ' if the animation property can not running on the compositor');

Nit: Animation reports that is is NOT running on the compositor
     for animation of 'background'.

>+  div.parentNode.removeChild(div);

Here and elsewhere in this file we should just use:

    div.remove();

>+add_task(function* compositor_and_non_compositor_properties() {
>+  var div = addDiv({ style: 'animation: background_and_translate 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  yield animation.ready;
>+
>+  is(animation.isRunningOnCompositor, omtaEnabled,
>+     'Animation reports that it is running on the compositor'
>+      + ' when the animation has two properties, one can be run'
>+      + ' on the compositor, the other can not be');

Nit: "... two properties, where one can be run on the compositor
      the other cannot"

>+add_task(function* immediately_after_finish_is_called() {
>+  var div = addDiv({ style: 'animation: anim 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  yield animation.ready;
>+
>+  animation.finish();
>+
>+  yield waitForAnimationFrames(2);

Please add a comment here briefly describing (if possible) why sometimes it
takes two ticks to update the animation state.

Better still, as discussed, we could use paint_listener.js and wait for the
next paint to happen (but we should use the non-flushing version).

>+  is(animation.isRunningOnCompositor, false,
>+     'Animation reports that it is NOT running on the compositor'
>+     + ' immediately right after finish() is called');

Nit: immediately after

>+  is(animation.isRunningOnCompositor, false,
>+     'Animation reports that it is NOT running on the compositor'
>+     + ' when animation is finished state');

Nit: "... when manually seeking the animation to the end"

>+  is(animation.isRunningOnCompositor, false,
>+     'Animation reports that it is NOT running on the compositor'
>+     + ' immediately right after cancel() is called');

Nit: immediately after

>+add_task(function* while_waiting_for_playing() {
>+  var div = addDiv({ style: 'animation: anim 100s 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  yield animation.ready;
>+
>+  is(animation.isRunningOnCompositor, false,
>+     'Animation reports that it is NOT running on the compositor'
>+     + ' while waiting for playing');

Nit: while in the delay phase

>+add_task(function* in_request_animation_callback_while_animation_is_running() {
>+  var div = addDiv({ style: 'animation: anim 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  yield animation.ready;
>+
>+  yield new Promise(function(resolve) {
>+    window.requestAnimationFrame(function() {
>+      is(animation.isRunningOnCompositor, omtaEnabled,
>+         'Animation reports that it is running on the compositor'
>+          + ' in requestAnimationFrame callback');
>+      resolve();
>+    });
>+  });

Perhaps we should add a comment:

// This is to test that we don't simply clobber the flag when ticking
// animations and then set it again during painting.

>+add_task(function* in_mutation_observer_callback_while_animation_is_running() {
>+  var div = addDiv({ style: 'animation: anim 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  yield animation.ready;
>+
>+  yield new Promise(function(resolve) {
>+    var observer = new MutationObserver(function() {
>+      is(animation.isRunningOnCompositor, omtaEnabled,
>+         'Animation reports that it is running on the compositor'
>+          + ' in MutationObserver callback');
>+      resolve();
>+    });
>+    observer.observe(div, { animations: true, subtree: false });
>+    div.style.animationDuration = "20s";

It seems safer to update this to 200s instead.

If the test machine is running really slowly 20s might have already elapsed.
Then, perhaps due to future changes in CSS animations, we might decide that we
ignore changes to animation duration after the animation has finished.

Should we assert that the observer record lists 'animation' as one of the
changes?

>+add_task(function* in_request_animation_callback_on_overflow_animation() {
>+  // Needs scrollbars to cause overflow.
>+  SpecialPowers.setIntPref("ui.showHideScrollbars", 1);

We need to restore this pref at the end of the test.

You should use SpecialPowers.pushPrefEnv instead:

https://dxr.mozilla.org/mozilla-central/rev/01ae99b53561a2c3b40533d8c1c92bd3efc42d00/testing/specialpowers/content/specialpowersAPI.js#1049

You might want to wrap it up in a Promise, however, similar to:

https://dxr.mozilla.org/mozilla-central/rev/01ae99b53561a2c3b40533d8c1c92bd3efc42d00/browser/devtools/webconsole/test/browser_webconsole_console_logging_workers_api.js?offset=0#12

>+
>+  var div = addDiv({ style: 'animation: rotate 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  yield animation.ready;
>+
>+  yield new Promise(function(resolve) {
>+    var timeAtStart = window.performance.now();
>+    function handleFrame() {
>+      is(animation.isRunningOnCompositor, omtaEnabled,
>+         'Animation reports that it is running on the compositor'
>+          + ' in requestAnimationFrame callback');
>+
>+      var start = window.performance.now();

This line is not used.

>+      // we have to wait at least 200ms because this animation is
>+      // unthrottled on every 200ms.
>+      // See http://hg.mozilla.org/mozilla-central/file/cafb1c90f794/layout/style/AnimationCommon.cpp#l863
>+      if (window.performance.now() - timeAtStart > 200) {
>+        resolve();
>+        return;
>+      }
>+      window.requestAnimationFrame(handleFrame);
>+    }

We need to add a comment saying that this test is checking that we don't
temporarily clear the "is running on compositor" flag when forcing an
unthrottled sample.

I'm a little bit concerned about adding a test that takes at least 200ms but
perhaps it's ok. If you know of another way of testing this, however, that
would be worthwhile.


We need at least one test for transitions since the code this patch adds has
separate calls to AnimationManager()->ClearIsRunningOnCompositor() and
TransitionManager()->ClearIsRunningOnCompositor().

Also, please make sure to run these tests on all platforms on try a few times
before landing so we can be sure we don't introduce new intermittent failures.


r=me with those comments addressed

Comment 69

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #68)
> >+add_task(function* immediately_after_finish_is_called() {
> >+  var div = addDiv({ style: 'animation: anim 100s' });
> >+  var animation = div.getAnimations()[0];
> >+
> >+  yield animation.ready;
> >+
> >+  animation.finish();
> >+
> >+  yield waitForAnimationFrames(2);
> 
> Please add a comment here briefly describing (if possible) why sometimes it
> takes two ticks to update the animation state.
> 
> Better still, as discussed, we could use paint_listener.js and wait for the
> next paint to happen (but we should use the non-flushing version).

Thanks! Unfortunately waitForPaints in paint_listener.js could not be used there because the function expects that test executes something which involves SchedulePaint() (or something related to it) before calling waitForPaints. But I noticed MozAfterPaint event can be used there. To use MozAfterPaint there we need to wait the first MozAfterPaint event before finish() or cancel() is called because of the delay of the event[1]. I've confirmed that tests with this step and with promise_test to run each test sequentially work fine locally.  I will update the patch.

[1] http://hg.mozilla.org/mozilla-central/file/dd2a1d737a64/layout/base/nsPresContext.cpp#l2472

Comment 70

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #69)
> But I noticed MozAfterPaint event can be used there. To use
> MozAfterPaint there we need to wait the first MozAfterPaint event before
> finish() or cancel() is called because of the delay of the event[1]. I've
> confirmed that tests with this step and with promise_test to run each test
> sequentially work fine locally.  I will update the patch.

Unfortunately the tests with MozAfterPaint causes intermittent failures (1/500 times). In the failure case MozAfterPaint was not come from root refresh driver's Tick, did come from somewhere else.

So I am going to use waitForAnimationFrames(2) and add coments there for now. I think we could use frame timing API to wait for real paint in future.

Comment 71

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8658479 [details] [diff] [review]
Part 3: Clear isRunningOnCompositor flag in AddAnimationsAndTransitionsToLayer and when the coresspoinding display item is removed.

mattwoodrow, can you please take a look of the changes to FrameLayerBuilder?
The purpose there is that clear isRunninOnCompositor flag owned by animation when the layer corresponding to the animation property is not used any more.

Comment 72

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8658479 [details] [diff] [review]
Part 3: Clear isRunningOnCompositor flag in AddAnimationsAndTransitionsToLayer and when the coresspoinding display item is removed.

Review of attachment 8658479 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +239,5 @@
> +    return;
> +  }
> +
> +  for (nsIFrame* frame : mFrameList) {
> +    if (mDisplayItemKey == nsDisplayItem::TYPE_TRANSFORM) {

How about:

nsCSSProperty prop = mDisplayItemKey == nsDisplayItem::TYPE_TRANSFORM ? eCSSProperty_transform : eCSSProperty_opacity;
frame->PresContext()->AnimationManager()->ClearIsRunningOnCompositor(frame, prop);

Comment 73

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Able to use testcommon.js in the window which has no opener

To use testcommon.js directly we need to enclose the part for subwindow.

Comment 74

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Additional tests for Animation.IsRunningOnCompositor v5

This patch is rewritten with testcommon.js and testharness.js to use addDiv and promise_test. So the patch is almost rewritten. Brian, could you please take time to review this patch again?

Changes from v4:
* Uses addDiv in testcommon.js
 * We don't need to remove element explicitly.
* Uses promise_test
 * Each test runs sequentially one by one to avoid receiving MozAfterPaints  or frame timing events caused by other tests.
  * It's not actually useful now.
 * Each test returns a promise.
 * promise_test calls test.done() so we don't need to call it.
* Adds a test for transition
* Removes 'immediately' in comments because I think it's not immediately.
* Other parts addresses comment#68.

Thank you.

Comment 75

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 6: A test case for transtion

(In reply to Hiroyuki Ikezoe (:hiro) from comment #74)
> Created attachment 8659552 [details] [diff] [review]
> * Adds a test for transition

This test was separated in part 6. Sorry for confusion.

Comment 76

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Able to use testcommon.js in the window which has no opener

This is the correct patch.

Comment 77

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8659552 [details] [diff] [review]
Part 5:  Additional tests for Animation.IsRunningOnCompositor v5

>+promise_test(function(t) {
>+  var div = addDiv(t, { style: 'animation: anim 100s' });
>+  var animation = div.getAnimations()[0];
>+
>+  return animation.ready.then(function() {
>+    animation.finish();
>+    // We need to wait for two frames here to ensure isRunningOnCompositor
>+    // flag is cleared. The first frame is for waking root refresh driver up,
>+    // the second one is for clearing the flag.
>+    // In most cases root refresh driver has been dormant because there is
>+    // nothing to do for the root refresh driver (e.g. nothing change on
>+    // chrome window). In the first frame document's refresh driver wakes the
>+    // root refresh driver up as a result of animation's style changes.
>+    // In the second frame the root refresh driver clears the flag as a
>+    // result of a paint.

This comment is good, but we repeat it three times.

For the other two times perhaps we could just say:

    // We need to wait for up to two frames here to ensure the "is running on
    // compositor" flag is cleared.
    // See the description in the 'isRunningOnCompositor is false when the
    // animation.finish() is called' test for the rationale.

>+}, 'isRunningOnCompositor retains true in requestAnimationFrameCallback for ' +
>+   'overflow animation');

Nit: s/retains/remains/

r=me with that. Thanks!

Comment 78

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8659552 [details] [diff] [review]
Part 5:  Additional tests for Animation.IsRunningOnCompositor v5

Actually, I just noticed we're not using t.step_func inside a lot of the callbacks here.

e.g. we have this a lot:

+promise_test(function(t) {
+  var div = addDiv(t, { style: 'animation: anim 100s' });
+  var animation = div.getAnimations()[0];
+
+  return animation.ready.then(function() {

and also this:

+    return new Promise(function(resolve) {

I might be misunderstanding something about promise_test, but I think if we don't use that and there's an error we won't report it correctly.

Comment 79

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8659569 [details] [diff] [review]
Part 6: A test case for transtion

>+  return animation.ready.then(t.step_func(function() {
>+    assert_equals(animation.isRunningOnCompositor, omtaEnabled,
>+       'Animation reports that it is running on the compositor'
>+       + ' during playback for opacity transition');

Nit: Transition reports that...
(It will just make it easier to guess what went wrong from the logs if we
 mention that we are testing transitions.)

Comment 80

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Clear isRunningOnCompositor flag in AddAnimationsAndTransitionsToLayer and when the coresspoinding display item is removed v2

Addressed comment#67.

Comment 81

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #80)
> Created attachment 8661517 [details] [diff] [review]
> Part 3: Clear isRunningOnCompositor flag in
> AddAnimationsAndTransitionsToLayer and when the coresspoinding display item
> is removed v2
> 
> Addressed comment#67.

And comment#72.

Comment 82

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Additional tests for Animation.IsRunningOnCompositor v6

Addressed comment#77 and comment#78.

Comment 83

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 6: A test case for transtion

Addressed comment#79.

Comment 84

[Hiroyuki Ikezoe (:hiro)]

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

Comment 85

[Carsten Book [:Tomcat]]

Hi Hiro, part 3 has problems to apply:

(eg '1-3,5', or 's' to toggle the sort order between id & patch description) 5
adding 1151694 to series file
renamed 1151694 -> move_mIsRunningOnCompositor.patch
applying move_mIsRunningOnCompositor.patch
patching file dom/animation/Animation.h
Hunk #1 FAILED at 52
Hunk #3 FAILED at 271
Hunk #4 FAILED at 407
3 out of 4 hunks FAILED -- saving rejects to file dom/animation/Animation.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh move_mIsRunningOnCompositor.patch

can you take a look thanks!

Comment 86

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Clear isRunningOnCompositor flag in AddAnimationsAndTransitionsToLayer and when the coresspoinding display item is removed v3

Rebased!

Comment 87

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81a35d84fb97
https://hg.mozilla.org/integration/mozilla-inbound/rev/80b4c268049f
https://hg.mozilla.org/integration/mozilla-inbound/rev/328191c5dbba
https://hg.mozilla.org/integration/mozilla-inbound/rev/13046b64bc82

Comment 88

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/81a35d84fb97
https://hg.mozilla.org/mozilla-central/rev/80b4c268049f
https://hg.mozilla.org/mozilla-central/rev/328191c5dbba
https://hg.mozilla.org/mozilla-central/rev/13046b64bc82