1123523 - implement a notification/observer mechanism for Web Animations API state changes

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Cameron McCormack (:heycam)]

For devtools to be able to inspect animations, it will need to listen to notifications for changes to existing animations, creation of new ones, etc.

This bug will be for implementing the notification/observer mechanism.  Other bugs depending on this one will be for specific events.

Comment 1

[Cameron McCormack (:heycam)]

Boris, I'm planning on adding some kind observation API for chrome to watch for Web Animations API state changes.  I can think of a few ways of doing it: (1) dispatching DOM Events, (2) dispatching Mutation Observer like notifications, (3) having something like nsIDocumentObserver (or even extending that very interface).  The state changes can happen in response to script in the page poking at the animation objects, and perhaps the observers might want to poke at the animation objects in response.  Given that, should I prefer one of the three approaches above, or is there a fourth way?

Also, if I go for something other than (1), should I be adding a ChromeOnly .webidl interface rather than an XPIDL one?

Comment 2

[Cameron McCormack (:heycam)]

See comment 1.

Comment 3

[Brian Birtles (:birtles)]

Just a couple of extra considerations:
* State changes can also happen in response to style flushes
* We will eventually want to standardise something in this area since it should be possible to build animation dev tools without chrome privileges

Comment 4

[Boris Zbarsky [:bzbarsky]]

nsIDocumentObserver is not terribly scriptable, right?

Extending mutation observers makes sense to me, I think, but interested in what Olli thinks.  Seems to me like this will still not need new interfaces created...

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Since our DOM MutationObserver is implemented as an implementation of
nsIMutationObserver, and nsIDocumentObserver extends nsIMutationObserver, adding
some [ChromeOnly] stuff to MutationObserver sounds ok.

Need to be just a bit careful when to fire the notifications äwhich then call nsIMutationObserver objects so
that we don't slow down performance too much.
Given bug 1034110, perhaps we could add
nsIDevtoolsMutationObserver (which extends nsIMutationObserver) or some such, and MutationObserver would be
the one to use it. Then nsDocument would have a flag to indicate if there are any MutationObservers using the
devtools specific observing stuff, and nsNodeUtils would fire the notifications only if that is the case,
and to fire the notifications, it would need to QI nsIMutationObservers to nsIDevtoolsMutationObserver.

A question is, do we always have a clear "target" node for the animations?
And what would the MutationRecord look like? MutationRecord could be extended to have some
[ChromeOnly] stuff.
https://dom.spec.whatwg.org/#interface-mutationrecord

Comment 6

[Brian Birtles (:birtles)]

(In reply to Olli Pettay [:smaug] from comment #5)
> A question is, do we always have a clear "target" node for the animations?

We currently do, but in future we probably won't. It should be possible to create an animation without a target element and attach a custom javascript sample callback to it (or simply leave it empty and use it for timing purposes). I'm not sure when we'll get to implementing that part though, it might not be for quite a while.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Olli, could you take a look at what I have so far in terms of the mutation observers?  I've done this:

* added an nsIAnimationObserver that inherits from nsIMutationObserver
* added a separate nsAnimationReceiver that inherits from nsMutationReceiver
* in nsDOMMutationObserver, created an nsAnimationReceiver when animations:true
  is passed to MutationObserver.observe(), or an nsMutationReceiver otherwise
* added a flag on nsIDocument that records whether an nsIAnimationObserver has
  been registered on a node in the document
* added an nsAutoAnimationMutationBatch class to handle batching of animation
  notifications

The split between nsMutationReceiver and nsAnimationReceiver is a bit awkward, which makes me wonder whether nsIAnimationObserver should not inherit from nsIMutationObserver and instead we just keep a separate list of them on nodes.

Comment 8

[Cameron McCormack (:heycam)]

Also see the addition of NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS_WITH_QI in nsTObserverArray.h that lets me notify nsIAnimationObservers in the nsIMutationObserver list.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8566389 [details] [diff] [review]
WIP v0.1

Wrong patch.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Why do we need a new nsAnimationReceiver? Does it provide something so different to 
nsMutationReceiver that we really want a separate receiver?

Comment 11

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

I have too many files named a.patch on my computer.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #10)
> Why do we need a new nsAnimationReceiver? Does it provide something so
> different to 
> nsMutationReceiver that we really want a separate receiver?

The mechanism I used to determine whether we can skip dispatching nsIAnimationObserver notifications at all is whether an nsIAnimationObserver was registered.  In order to make MutationObserver.observe(..., { animations: true }) trigger this, but other MutationObserver.observe() calls not, I needed to separate the receiver classes so that only the one needed to translate animation events into MutationRecords implements nsIAnimationObserver

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Well, couldn't you just use animations: true as a flag somewhere?

But looking the patch...

Comment 14

[Cameron McCormack (:heycam)]

Not nsIAnimationObserver is a general internal mechanism separate from the MutationObservers, since it's the former we want to avoid dispatching if we can.

Comment 15

[Cameron McCormack (:heycam)]

*Not if

Comment 16

[Cameron McCormack (:heycam)]

Boris, I want to make the .webidl additions in this patch [ChromeOnly].  That's fine for the new MutationRecord members, but it doesn't work for the animations dictionary member on MutationObserverInit.  Should I add support for [ChromeOnly] on dictionary members (treating it as if the property weren't specified in non-chrome documents) or should I be doing the same kind of checks myself that ChromeOnly does in nsDOMMutationObserver::Observe?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8566683 [details] [diff] [review]
WIP v0.1

>   nsCOMPtr<nsINode>             mTarget;
>   nsCOMPtr<nsIAtom>             mType;
>   nsCOMPtr<nsIAtom>             mAttrName;
>   nsString                      mAttrNamespace;
>   nsString                      mPrevValue;
>   nsRefPtr<nsSimpleContentList> mAddedNodes;
>   nsRefPtr<nsSimpleContentList> mRemovedNodes;
>   nsCOMPtr<nsINode>             mPreviousSibling;
>   nsCOMPtr<nsINode>             mNextSibling;
>+  AnimationPlayerArray          mAddedAnimations;
>+  AnimationPlayerArray          mRemovedAnimations;
>+  AnimationPlayerArray          mChangedAnimations;
MutationRecord is getting rather heavy. I guess we'll need to have specialized MutationRecords for different types.
But that can be done in a separate bug.

>   void AddMutationObserverUnlessExists(nsIMutationObserver* aMutationObserver)
>   {
>     nsSlots* s = Slots();
>     s->mMutationObservers.AppendElementUnlessExists(aMutationObserver);
>+    NoteAddedAnimationObserver(aMutationObserver);
>   }

Oh, that is a bit ugly.
I would add
void AddAnimationObserver( and UnlessExists)(nsIAnimationObserver*)
which then calls AddMutationObserverUnlessExists and 
OwnerDoc()->SetMayHaveAnimationObservers();

It would be up to the nsMutationReceiverBase to use the right one.
Usually that could get the information from the parent receiver, but for the
root receiver, created in nsDOMMutationObserver::GetReceiverFor, one would need to
pass explicitly the flag whether to observe animations. And you do that with your patch already.

In other words, I'm missing to see the reason for AnimationReceiver, or I think
supporting one QI isn't strong enough reason to have it.


>+#define IMPL_ANIMATION_NOTIFICATION(func_, content_, params_)     \
>+  PR_BEGIN_MACRO                                                  \
>+  bool needsEnterLeave = doc->MayHaveDOMMutationObservers();      \
>+  if (needsEnterLeave) {                                          \
>+    nsDOMMutationObserver::EnterMutationHandling();               \
>+  }                                                               \
>+  nsINode* node = content_;                                       \
>+  do {                                                            \
>+    nsINode::nsSlots* slots = node->GetExistingSlots();           \
>+    if (slots && !slots->mMutationObservers.IsEmpty()) {          \
>+      /* No need to explicitly notify the first observer first    \
>+         since that'll happen anyway. */                          \
>+      NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS_WITH_QI(                 \
>+        slots->mMutationObservers, nsIMutationObserver,           \
>+        nsIAnimationObserver, func_, params_);                    \
>+    }                                                             \
I hope this isn't too slow. Probably not if we don't have too many observers.
Just worried about the QI+Addref+Release performance


FYI, bug 1013743 will make gecko work properly in Shadow DOM case, but that is waiting for gaia updates.

+void
+AnimationPlayer::UpdateRelevance()
+{
+  bool wasRelevant = mIsRelevant;
+  mIsRelevant = HasCurrentSource() || HasInEffectSource();
+
+  // Notify animation observers.
+  if (wasRelevant && !mIsRelevant) {
+    nsDOMMutationObserver::EnterMutationHandling();
+    nsNodeUtils::AnimationRemoved(this);
+    nsDOMMutationObserver::LeaveMutationHandling();
+  } else if (!wasRelevant && mIsRelevant) {
+    nsDOMMutationObserver::EnterMutationHandling();
+    nsNodeUtils::AnimationAdded(this);
+    nsDOMMutationObserver::LeaveMutationHandling();
+  }
+}
Why doesn't the nsNodeUtils methods deal with Enter/Leave?


Somewhat surprising to see AnimationPlayer objects in the MutationRecord.
I would have expected MutationRecord to tell about the state change.
That is how MutationObserver works now - it is all about state changes so that one can reconstruct what has happened.
Or do I misunderstand what AnimationPlayer represents.

Comment 18

[Cameron McCormack (:heycam)]

(In reply to Olli Pettay [:smaug] (no new reviews before Feb 22, please) from comment #17)
> >   void AddMutationObserverUnlessExists(nsIMutationObserver* aMutationObserver)
> >   {
> >     nsSlots* s = Slots();
> >     s->mMutationObservers.AppendElementUnlessExists(aMutationObserver);
> >+    NoteAddedAnimationObserver(aMutationObserver);
> >   }
> 
> Oh, that is a bit ugly.
> I would add
> void AddAnimationObserver( and UnlessExists)(nsIAnimationObserver*)
> which then calls AddMutationObserverUnlessExists and 
> OwnerDoc()->SetMayHaveAnimationObservers();

OK, I can do that.  And assert in AddMutationObserver(UnlessExists) that the nsIMutationObserver isn't also an nsIAnimationObserver?

> It would be up to the nsMutationReceiverBase to use the right one.
> Usually that could get the information from the parent receiver, but for the
> root receiver, created in nsDOMMutationObserver::GetReceiverFor, one would
> need to
> pass explicitly the flag whether to observe animations. And you do that with
> your patch already.
> 
> In other words, I'm missing to see the reason for AnimationReceiver, or I
> think
> supporting one QI isn't strong enough reason to have it.

With that approach then yes there is no need to separate the Receiver classes.

> >+#define IMPL_ANIMATION_NOTIFICATION(func_, content_, params_)     \
> >+  PR_BEGIN_MACRO                                                  \
> >+  bool needsEnterLeave = doc->MayHaveDOMMutationObservers();      \
> >+  if (needsEnterLeave) {                                          \
> >+    nsDOMMutationObserver::EnterMutationHandling();               \
> >+  }                                                               \
> >+  nsINode* node = content_;                                       \
> >+  do {                                                            \
> >+    nsINode::nsSlots* slots = node->GetExistingSlots();           \
> >+    if (slots && !slots->mMutationObservers.IsEmpty()) {          \
> >+      /* No need to explicitly notify the first observer first    \
> >+         since that'll happen anyway. */                          \
> >+      NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS_WITH_QI(                 \
> >+        slots->mMutationObservers, nsIMutationObserver,           \
> >+        nsIAnimationObserver, func_, params_);                    \
> >+    }                                                             \
> I hope this isn't too slow. Probably not if we don't have too many observers.
> Just worried about the QI+Addref+Release performance

Do you think it is still best to keep the nsIAnimationObservers in mMutationObservers, rather than adding a separate mAnimationObservers on Slots?

> FYI, bug 1013743 will make gecko work properly in Shadow DOM case, but that
> is waiting for gaia updates.
> 
> +void
> +AnimationPlayer::UpdateRelevance()
> +{
> +  bool wasRelevant = mIsRelevant;
> +  mIsRelevant = HasCurrentSource() || HasInEffectSource();
> +
> +  // Notify animation observers.
> +  if (wasRelevant && !mIsRelevant) {
> +    nsDOMMutationObserver::EnterMutationHandling();
> +    nsNodeUtils::AnimationRemoved(this);
> +    nsDOMMutationObserver::LeaveMutationHandling();
> +  } else if (!wasRelevant && mIsRelevant) {
> +    nsDOMMutationObserver::EnterMutationHandling();
> +    nsNodeUtils::AnimationAdded(this);
> +    nsDOMMutationObserver::LeaveMutationHandling();
> +  }
> +}
> Why doesn't the nsNodeUtils methods deal with Enter/Leave?

Yeah, they do; I can remove these calls.

> Somewhat surprising to see AnimationPlayer objects in the MutationRecord.
> I would have expected MutationRecord to tell about the state change.
> That is how MutationObserver works now - it is all about state changes so
> that one can reconstruct what has happened.
> Or do I misunderstand what AnimationPlayer represents.

There are two things I want to expose currently.  One is addition and removal of AnimationPlayer objects from the list of AnimationPlayers that target an Element.  This is effectively the same as changes to the list of AnimationPlayers returned by Element.getAnimationPlayers().  Knowing the order isn't important.  The second thing is notifying changes to the timing state of an AnimationPlayer.  I'm not exposing the details of exactly what changed, since the consumer probably doesn't need that level of detail.

It would be a bit tricky to set things like mPreviousAnimationSibling/mNextAnimationSibling to know where the AnimationPlayers were added/removed from the list, so I guess I'd rather not, if it's not providing information the consumer will use.

Do you still think MutationObservers are the right mechanism to expose these notifications with?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Adding support for ChromeOnly dictionary members makes some sense if we want this to really be invisible to content code.  Anything else would be web-observable, right?

Does the IDL grammar even allow extended attributes on dictionary members right now?

The other non-web-observable option is to have a observeChrome method on MutationObserver, but that seems really hacky...

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #19)
> Adding support for ChromeOnly dictionary members makes some sense if we want
> this to really be invisible to content code.  Anything else would be
> web-observable, right?

Yep.

> Does the IDL grammar even allow extended attributes on dictionary members
> right now?

In the spec it does.

> The other non-web-observable option is to have a observeChrome method on
> MutationObserver, but that seems really hacky...

Yeah that doesn't sound great.  I'll look into ChromeOnly on the dictionary member.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Cameron McCormack (:heycam) from comment #18)

> Do you think it is still best to keep the nsIAnimationObservers in
> mMutationObservers, rather than adding a separate mAnimationObservers on
> Slots?
If this is for devtools usage for now, then definitely no separate thing on slots.
Slots is getting way too heavy anyway.


> There are two things I want to expose currently.  One is addition and
> removal of AnimationPlayer objects from the list of AnimationPlayers that
> target an Element.  This is effectively the same as changes to the list of
> AnimationPlayers returned by Element.getAnimationPlayers().
Ok, that similar to changes to childList in DOM. Fine


> Do you still think MutationObservers are the right mechanism to expose these
> notifications with?
How often do we get new / remove AnimationPlayers?
If that happens relatively rarely, perhaps chrome only Event which has the AnimationPlayer object as an
attribute would be simpler.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

comment 1 was talking about state changes, so I was perhaps thinking about different kind of state changes than adding/removing players.

Comment 23

[Cameron McCormack (:heycam)]

The add/remove notifications happen at the start/end of every CSS transition and animation.  The change notifications happen only if you change some properties of a CSS animation (such as its duration) while it is running.  It turns out due to the way AnimationPlayer objects get created in response to style changes, the coalescing that nsAutoAnimationMutationBatch helps avoid exposing some temporarily created objects.

I think for now I'll continue with the MutationObserver approach.

Comment 24

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add an nsIAnimationObserver interface to watch adding/changing/removing AnimationPlayer objects.

Comment 25

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add an "animations" option for MutationObservers and expose chrome-only AnimationPlayer/animation members on MutationRecords.

Comment 26

[Cameron McCormack (:heycam)]

Attachment: Part 3: Store a flag on AnimationPlayer for whether it is exposed by Element.getAnimationPlayers().

Comment 27

[Cameron McCormack (:heycam)]

Attachment: Part 4: Add macro for notifying observers only when they implement a specific derived interface.

Comment 28

[Cameron McCormack (:heycam)]

Attachment: Part 5: Record on a document whether it might have any nsIAnimationObservers registered.

Comment 29

[Cameron McCormack (:heycam)]

Attachment: Part 6: Listen for nsIAnimationObserver notifications and translate them to MutationObserver notifications.

Comment 30

[Cameron McCormack (:heycam)]

Attachment: Part 7: Add utility functions to notify registered nsIAnimationObservers.

Comment 31

[Cameron McCormack (:heycam)]

Attachment: Part 8: Dispatch nsIAnimationObserver notifications when an animation is added or removed.

Comment 32

[Cameron McCormack (:heycam)]

Attachment: Part 9: Dispatch an nsIAnimationObserver notification when an animation is changed.

Comment 33

[Cameron McCormack (:heycam)]

Attachment: Part 10: Tests.

Comment 34

[Cameron McCormack (:heycam)]

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

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8575832 [details] [diff] [review]
Part 2: Add an "animations" option for MutationObservers and expose chrome-only AnimationPlayer/animation members on MutationRecords.

Ok, this depends on bug 1141916, and if that takes some time, one could explicitly check that the caller is chrome if .animations was used.

Could you file a followup bug to decrease the size of MutationRecords
(we effectively want to have different storage for different record types I think, at least if we don't get MutationRecords to be GC'ed during minor GCs, Bug 1120016).

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8575836 [details] [diff] [review]
Part 6: Listen for nsIAnimationObserver notifications and translate them to MutationObserver notifications.

>       if (!transientExists) {
>         // Make sure the elements which are removed from the
>         // subtree are kept in the same observation set.
>-        transientReceivers->AppendObject(new nsMutationReceiver(aChild, orig));
>+        nsRefPtr<nsMutationReceiver> tr;
>+        if (orig->Animations()) {
>+          tr = nsAnimationReceiver::Create(aChild, orig);
>+        } else {
>+          tr = nsMutationReceiver::Create(aChild, orig);
>+        }
>+        transientReceivers->AppendObject(tr);
Could you perhaps use nsMutationReceiver* here, not nsRefPtr<nsMutationReceiver> to reduce the extra addref/release

>-nsDOMMutationObserver::GetReceiverFor(nsINode* aNode, bool aMayCreate)
>+nsDOMMutationObserver::GetReceiverFor(nsINode* aNode, bool aMayCreate,
>+                                      bool aWantsAnimations)
Could you assert here that if aMayCreate == false, also aWantsAnimations should be false

>-            transientReceivers->AppendObject(new nsMutationReceiver(removed, orig));
>+            nsRefPtr<nsMutationReceiver> tr;
>+            if (orig->Animations()) {
>+              tr = nsAnimationReceiver::Create(removed, orig);
>+            } else {
>+              tr = nsMutationReceiver::Create(removed, orig);
>+            }
>+            transientReceivers->AppendObject(tr);
This could also use raw nsMutationReceiver* so that extra addref/release is avoided.

> protected:
>   nsMutationReceiverBase(nsINode* aTarget, nsDOMMutationObserver* aObserver)
>   : mTarget(aTarget), mObserver(aObserver), mRegisterTarget(aTarget)
>   {
>-    mRegisterTarget->AddMutationObserver(this);
>-    mRegisterTarget->SetMayHaveDOMMutationObserver();
>-    mRegisterTarget->OwnerDoc()->SetMayHaveDOMMutationObservers();
>   }
> 
>   nsMutationReceiverBase(nsINode* aRegisterTarget,
>                          nsMutationReceiverBase* aParent)
>   : mTarget(nullptr), mObserver(nullptr), mParent(aParent),
>     mRegisterTarget(aRegisterTarget), mKungFuDeathGrip(aParent->Target())
>   {
>     NS_ASSERTION(mParent->Subtree(), "Should clone a non-subtree observer!");
>-    mRegisterTarget->AddMutationObserver(this);
>+  }
>+
>+  virtual void AddMutationObserver() = 0;
>+
>+  void AddObserver()
>+  {
>+    AddMutationObserver();
>     mRegisterTarget->SetMayHaveDOMMutationObserver();
>     mRegisterTarget->OwnerDoc()->SetMayHaveDOMMutationObservers();
>   }
> 
>   bool ObservesAttr(mozilla::dom::Element* aElement,
>                     int32_t aNameSpaceID,
>                     nsIAtom* aAttr)
>   {
>@@ -297,24 +301,32 @@ private:
> 
> 
> class nsMutationReceiver : public nsMutationReceiverBase
> {
> protected:
>   virtual ~nsMutationReceiver() { Disconnect(false); }
> 
> public:
>-  nsMutationReceiver(nsINode* aTarget, nsDOMMutationObserver* aObserver);
>+  static already_AddRefed<nsMutationReceiver>
>+      Create(nsINode* aTarget, nsDOMMutationObserver* aObserver)
>+  {
>+    nsRefPtr<nsMutationReceiver> r =
>+      new nsMutationReceiver(aTarget, aObserver);
>+    r->AddObserver();
>+    return r.forget();
>+  }
> 
>-  nsMutationReceiver(nsINode* aRegisterTarget, nsMutationReceiverBase* aParent)
>-  : nsMutationReceiverBase(aRegisterTarget, aParent)
>+  static already_AddRefed<nsMutationReceiver>
>+      Create(nsINode* aRegisterTarget, nsMutationReceiverBase* aParent)
>   {
>-    NS_ASSERTION(!static_cast<nsMutationReceiver*>(aParent)->GetParent(),
>-                 "Shouldn't create deep observer hierarchies!");
>-    aParent->AddClone(this);
>+    nsRefPtr<nsMutationReceiver> r =
>+      new nsMutationReceiver(aRegisterTarget, aParent);
>+    r->AddObserver();
>+    return r.forget();
>   }
So this makes us behave a bit differently. AddObserver
always calls mRegisterTarget->SetMayHaveDOMMutationObserver(); but currently the flag isn't set for transient observers.
Could you please not change that behavior, so that GetReceiverFor and especially GetAllSubtreeObserversFor isn't de-optimized.
I guess AddObserver could just check if mParent is null, and only in that case call SetMayHaveDOMMutationObserver stuff.

>+  struct Entry {
nit, { should be in the next line

Comment 37

[Brian Birtles (:birtles)]

Comment on attachment 8575833 [details] [diff] [review]
Part 3: Store a flag on AnimationPlayer for whether it is exposed by Element.getAnimationPlayers().

> void
> AnimationPlayer::SetSource(Animation* aSource)
> {
>   if (mSource) {
>     mSource->SetParentTime(Nullable<TimeDuration>());
>   }
>+  UpdateRelevance();
>+
>   mSource = aSource;
>   if (mSource) {
>     mSource->SetParentTime(GetCurrentTime());
>   }
>+  UpdateRelevance();
> }

I was a bit concerned this would end up generating spurious records but I believe nsAutoAnimationMutationBatch (in a later patch) filters out the redundant records for us. Let me know if I've misunderstood, however.

> protected:
>-  virtual ~AnimationPlayer() { }
>+  virtual ~AnimationPlayer()
>+  {
>+    UpdateRelevance();
>+  }

I don't think I quite understand how this works. Is the idea here to make sure we end up calling nsNodeUtils::AnimationRemoved if we haven't already (once part 8 is applied)? If so, what causes UpdateRelevance to notice that this player is no longer relevant? Are we relying on the source content to have been nulled out first? I'm pretty sure I'm missing something here.

> NS_IMETHODIMP
> Element::GetInnerHTML(nsAString& aInnerHTML)
>diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp
>index 418ff00..f689e97 100644
>--- a/layout/style/nsAnimationManager.cpp
>+++ b/layout/style/nsAnimationManager.cpp
>@@ -345,16 +345,20 @@ nsAnimationManager::CheckAnimationRule(nsStyleContext* aStyleContext,
>         //
>         // Although we're doing this while iterating this is safe because
>         // we're not changing the length of newPlayers and we've finished
>         // iterating over the list of old iterations.
>         newPlayer->Cancel();
>         newPlayer = nullptr;
>         newPlayers.ReplaceElementAt(newIdx, oldPlayer);
>         collection->mPlayers.RemoveElementAt(oldIdx);
>+
>+        // We've touched the old animation's timing properties, so this
>+        // could update the old player's relevance.
>+        oldPlayer->UpdateRelevance();

Thinking out loud here, I wonder how all this should work once we allow manipulating these properties directly.

For example, now if you do

  elem.style.animation = "move 3s";

Then after 2s you do

  elem.style.animationDuration = "1s";
  elem.clientTop;

You'll get an animationremoved record (I think so, anyway, but Jonathan told me yesterday we didn't appear to be honouring dynamic changes to animation-duration).

Later you'll be able to do:

  player.source.timing.duration = 1000;

(If it's a CSS animation, you'll have a set a mutable copy of the animation first, of course.)

Should that update the relevance immediately?

What if you do:

  player.source.timing.duration = 1000;
  // Current time is now past the end of the active interval
  player.source.timing.delay = 1500;
  // Current time is now back in the active interval

Should you get animationremoved and animationadded in that case?

If, for example, we spec that changes to these timing properties queue a task to check the player's relevance then we could probably use that same mechanism here.

I think we can look into this when we do finishing behavior (bug 1074630) since the same question arises there, i.e. should this kind of redundant change trigger the finished promise to be resolved? (At first I thought, "no", but if you were to query the playState in between those lines it would return "finished" so maybe the answer is "yes"?). Eventually I think updating the relevance will get lumped together with updating finished state so whatever we decide there will probably apply here too.

Comment 38

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #37)
> Comment on attachment 8575833 [details] [diff] [review]
> Part 3: Store a flag on AnimationPlayer for whether it is exposed by
> Element.getAnimationPlayers().
> 
> > void
> > AnimationPlayer::SetSource(Animation* aSource)
> > {
> >   if (mSource) {
> >     mSource->SetParentTime(Nullable<TimeDuration>());
> >   }
> >+  UpdateRelevance();
> >+
> >   mSource = aSource;
> >   if (mSource) {
> >     mSource->SetParentTime(GetCurrentTime());
> >   }
> >+  UpdateRelevance();
> > }
> 
> I was a bit concerned this would end up generating spurious records but I
> believe nsAutoAnimationMutationBatch (in a later patch) filters out the
> redundant records for us. Let me know if I've misunderstood, however.

I guess that only works if we have a nsAutoAnimationMutationBatch in place, however. Once we expose this method to script we'll need to be careful to add one here so that:

  player.source = new Animation(...);

doesn't generate spurious records.

Comment 39

[Brian Birtles (:birtles)]

Comment on attachment 8575840 [details] [diff] [review]
Part 9: Dispatch an nsIAnimationObserver notification when an animation is changed.

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

r=me with that comment addressed.

As I understand it, we don't need any changes to nsTransitionManager.cpp because transitions don't change once they start and all the add/removed notification is handled in part 7 and the AnimationPlayer/Animation.

::: dom/smil/nsSMILKeySpline.h
@@ +52,5 @@
> +           mY1 == aOther.mY1 &&
> +           mX2 == aOther.mX2 &&
> +           mY2 == aOther.mY2 &&
> +           mozilla::PodEqual(mSampleValues, aOther.mSampleValues,
> +                             mozilla::ArrayLength(mSampleValues));

We don't need to compare the mSampleValues. That's calculated from the other parameters. It's just a cached lookup table for getting a faster initial guess.

Comment 40

[Brian Birtles (:birtles)]

Comment on attachment 8575841 [details] [diff] [review]
Part 10: Tests.

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

This looks great! One thing I'd like to see however, is a couple more tests for redundant changes.

For example, change the delay of an animation by some trivial amount then cancel it (e.g. by setting the duration to be much shorter) and test that we only get a removed record, not a changed record.

Likewise, for an animation that gets added and changed within the same style flush.

Similarly, adding an animation whose negative delay means it is already finished should probably not generate any records.

And then tweaking two properties at once that cancel each other out with regards to making the animation relevant.

Do any of those seem worthwhile testing to you?

Also, there's the subtree: true tests you mentioned.

r=me with whichever of those tests seem worthwhile to you and the following comments addressed.

::: dom/animation/test/chrome/test_animation_observers.html
@@ +127,5 @@
> +  });
> +}
> +
> +function flush() {
> +  getComputedStyle(e);

Does this work? I was under the impression that, for example, calling:

 getComputedStyle(e).backgroundColor

flushed style, while:

 getComputedStyle(e).marginLeft

flushed layout. Is that true? And if so, is this doing what we want?

@@ +341,5 @@
> +// Test that multiple transitions starting and ending on an element
> +// at the same time get batched up into a single MutationRecord.
> +addAsyncAnimTest("multiple_transitions", function*() {
> +  // Start three long transitions.
> +  e.style = "transition-duration: 100s; transition-property: color, background-color, line-height; color: blue; background-color: lime; line-height: 24px;";

Nit: This should probably be <80 columns.

@@ +467,5 @@
> +  assert_records([{ added: players, changed: [], removed: [] }],
> +                 "records after animation start");
> +
> +  // Wait until at least a second of the animation has run.
> +  yield await_timeout(1000);

This seems like a long time to wait. Bug 1072037 should now be on m-c so we can seek animations by setting their currentTime (and bug 1073379 has been there a little longer which also lets us seek animations by setting their startTime). Perhaps we can add something like players[0].currentTime += 1000 here instead?

@@ +532,5 @@
> +  assert_records([{ added: players, changed: [], removed: [] }],
> +                 "records after animation start");
> +
> +  // Wait until we are definitely filling.
> +  yield await_timeout(1000);

We could just wait for animationend here. Since we use endpoint-exclusive timing, even if we happen to land at *exactly* the end of the active interval we will still only be considered relevant if we have a forwards fill.

@@ +569,5 @@
> +  assert_records([{ added: players, changed: [], removed: [] }],
> +                 "records after animation start");
> +
> +  // Wait until we are definitely past the first iteration.
> +  yield await_timeout(1000);

Again, we should probably seek the player's currentTime here.

@@ +629,5 @@
> +
> +    // Wait for the addition, change and removal MutationRecords to be delivered.
> +    yield await_frame();
> +    assert_records([{ added: [], changed: [], removed: players }],
> +                    "records after animation end");

Should we be setting e.style = "" at the end here? Otherwise if we add tests after this I guess we'll leak setting animation-duration to 100s?

Comment 41

[Cameron McCormack (:heycam)]

(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 8575836 [details] [diff] [review]
> Part 6: Listen for nsIAnimationObserver notifications and translate them to
> MutationObserver notifications.
> 
> >       if (!transientExists) {
> >         // Make sure the elements which are removed from the
> >         // subtree are kept in the same observation set.
> >-        transientReceivers->AppendObject(new nsMutationReceiver(aChild, orig));
> >+        nsRefPtr<nsMutationReceiver> tr;
> >+        if (orig->Animations()) {
> >+          tr = nsAnimationReceiver::Create(aChild, orig);
> >+        } else {
> >+          tr = nsMutationReceiver::Create(aChild, orig);
> >+        }
> >+        transientReceivers->AppendObject(tr);
> Could you perhaps use nsMutationReceiver* here, not
> nsRefPtr<nsMutationReceiver> to reduce the extra addref/release

Indeed we can.  (An earlier version of the patch did some other operations on the newly created receiver object that did an AddRef/Release before it got registered on the node, which caused me to hold on to it strongly here.)

> >-nsDOMMutationObserver::GetReceiverFor(nsINode* aNode, bool aMayCreate)
> >+nsDOMMutationObserver::GetReceiverFor(nsINode* aNode, bool aMayCreate,
> >+                                      bool aWantsAnimations)
>
> Could you assert here that if aMayCreate == false, also aWantsAnimations
> should be false

Yeah I guess it doesn't matter what value aWantsAnimations takes when looking up an existing observer.

But I realise now that I meant to make subtree:true work with animation observers, and I haven't handled that yet.  I'll work on a followup patch for that.

> > protected:
> >   nsMutationReceiverBase(nsINode* aTarget, nsDOMMutationObserver* aObserver)
> >   : mTarget(aTarget), mObserver(aObserver), mRegisterTarget(aTarget)
> >   {
> >-    mRegisterTarget->AddMutationObserver(this);
> >-    mRegisterTarget->SetMayHaveDOMMutationObserver();
> >-    mRegisterTarget->OwnerDoc()->SetMayHaveDOMMutationObservers();
> >   }
> > 
> >   nsMutationReceiverBase(nsINode* aRegisterTarget,
> >                          nsMutationReceiverBase* aParent)
> >   : mTarget(nullptr), mObserver(nullptr), mParent(aParent),
> >     mRegisterTarget(aRegisterTarget), mKungFuDeathGrip(aParent->Target())
> >   {
> >     NS_ASSERTION(mParent->Subtree(), "Should clone a non-subtree observer!");
> >-    mRegisterTarget->AddMutationObserver(this);
> >+  }
> >+
> >+  virtual void AddMutationObserver() = 0;
> >+
> >+  void AddObserver()
> >+  {
> >+    AddMutationObserver();
> >     mRegisterTarget->SetMayHaveDOMMutationObserver();
> >     mRegisterTarget->OwnerDoc()->SetMayHaveDOMMutationObservers();
> >   }
> > 
> >   bool ObservesAttr(mozilla::dom::Element* aElement,
> >                     int32_t aNameSpaceID,
> >                     nsIAtom* aAttr)
> >   {
> >@@ -297,24 +301,32 @@ private:
> > 
> > 
> > class nsMutationReceiver : public nsMutationReceiverBase
> > {
> > protected:
> >   virtual ~nsMutationReceiver() { Disconnect(false); }
> > 
> > public:
> >-  nsMutationReceiver(nsINode* aTarget, nsDOMMutationObserver* aObserver);
> >+  static already_AddRefed<nsMutationReceiver>
> >+      Create(nsINode* aTarget, nsDOMMutationObserver* aObserver)
> >+  {
> >+    nsRefPtr<nsMutationReceiver> r =
> >+      new nsMutationReceiver(aTarget, aObserver);
> >+    r->AddObserver();
> >+    return r.forget();
> >+  }
> > 
> >-  nsMutationReceiver(nsINode* aRegisterTarget, nsMutationReceiverBase* aParent)
> >-  : nsMutationReceiverBase(aRegisterTarget, aParent)
> >+  static already_AddRefed<nsMutationReceiver>
> >+      Create(nsINode* aRegisterTarget, nsMutationReceiverBase* aParent)
> >   {
> >-    NS_ASSERTION(!static_cast<nsMutationReceiver*>(aParent)->GetParent(),
> >-                 "Shouldn't create deep observer hierarchies!");
> >-    aParent->AddClone(this);
> >+    nsRefPtr<nsMutationReceiver> r =
> >+      new nsMutationReceiver(aRegisterTarget, aParent);
> >+    r->AddObserver();
> >+    return r.forget();
> >   }
> So this makes us behave a bit differently. AddObserver
> always calls mRegisterTarget->SetMayHaveDOMMutationObserver(); but currently
> the flag isn't set for transient observers.

Isn't the flag currently set for transient receivers too?  Both of nsMutationReceiverBase's constructors call SetMayHaveDOMMutationObserver currently.

> Could you please not change that behavior, so that GetReceiverFor and
> especially GetAllSubtreeObserversFor isn't de-optimized.
> I guess AddObserver could just check if mParent is null, and only in that
> case call SetMayHaveDOMMutationObserver stuff.

I can add that check, but please confirm for me that we're not doing that check already.

Comment 42

[Cameron McCormack (:heycam)]

Attachment: Part 6: Listen for nsIAnimationObserver notifications and translate them to MutationObserver notifications. (v2)

Comment 43

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles) from comment #37)
> Comment on attachment 8575833 [details] [diff] [review]
> Part 3: Store a flag on AnimationPlayer for whether it is exposed by
> Element.getAnimationPlayers().
> 
> > void
> > AnimationPlayer::SetSource(Animation* aSource)
> > {
> >   if (mSource) {
> >     mSource->SetParentTime(Nullable<TimeDuration>());
> >   }
> >+  UpdateRelevance();
> >+
> >   mSource = aSource;
> >   if (mSource) {
> >     mSource->SetParentTime(GetCurrentTime());
> >   }
> >+  UpdateRelevance();
> > }
> 
> I was a bit concerned this would end up generating spurious records but I
> believe nsAutoAnimationMutationBatch (in a later patch) filters out the
> redundant records for us. Let me know if I've misunderstood, however.

It will, but that makes me think that we don't need two UpdateRelevance calls, just the one at the end; it'll always be the same AnimationPlayer object that it'll send notifications for.

> > protected:
> >-  virtual ~AnimationPlayer() { }
> >+  virtual ~AnimationPlayer()
> >+  {
> >+    UpdateRelevance();
> >+  }
> 
> I don't think I quite understand how this works. Is the idea here to make
> sure we end up calling nsNodeUtils::AnimationRemoved if we haven't already
> (once part 8 is applied)? If so, what causes UpdateRelevance to notice that
> this player is no longer relevant? Are we relying on the source content to
> have been nulled out first? I'm pretty sure I'm missing something here.

So yes, it was to ensure we end up calling AnimationRemoved, but I added this early on in the patch queue development and I neglected to record the test I was using that seemed to need it.  I probably was assuming that mSource had been nulled out at that point, but if that's the case, then UpdateSource should then catch it.

Are there cases where we will be destroying an AnimationPlayer while mIsRelevant is true, and we should be dispatching the animation removed notification by doing something in the destructor -- SetSource(nullptr) maybe?

> Thinking out loud here, I wonder how all this should work once we allow
> manipulating these properties directly.
> 
> For example, now if you do
> 
>   elem.style.animation = "move 3s";
> 
> Then after 2s you do
> 
>   elem.style.animationDuration = "1s";
>   elem.clientTop;
> 
> You'll get an animationremoved record (I think so, anyway, but Jonathan told
> me yesterday we didn't appear to be honouring dynamic changes to
> animation-duration).

I think this is working; one of the tests in part 10 does that.

> Later you'll be able to do:
> 
>   player.source.timing.duration = 1000;
> 
> (If it's a CSS animation, you'll have a set a mutable copy of the animation
> first, of course.)
> 
> Should that update the relevance immediately?

Good question.  What happens when you call play() or cancel()?  Does that have an immediate effect, or does it go around the event loop first?  Probably it should be the same if you update the timing properties like this directly.

> What if you do:
> 
>   player.source.timing.duration = 1000;
>   // Current time is now past the end of the active interval
>   player.source.timing.delay = 1500;
>   // Current time is now back in the active interval
> 
> Should you get animationremoved and animationadded in that case?

Because of how the notifications are batched, I think you should get a single animationchanged notification.  The assignment to duration will batch an animationchanged, updating relevance will batch an animationremoved, then the second statement will remove the animationremoved from the batch, leaving the animationchanged.

One thing the batching won't handle "correctly" is making a change to the timing and then reverting that change.  We'll still have recorded a change on the batch object, so will dispatch an animationchanged notification.  I'm not sure if that's really an issue though -- I think DOM mutations are handled in the same way (e.g. if you change an attribute to one value, then change it again to its original value, before the observers are notified).

> If, for example, we spec that changes to these timing properties queue a
> task to check the player's relevance then we could probably use that same
> mechanism here.

Yes.  I am not sure if this is what we want or not; matching start() and play() behaviour is the only constraint I can think of.

> I think we can look into this when we do finishing behavior (bug 1074630)
> since the same question arises there, i.e. should this kind of redundant
> change trigger the finished promise to be resolved? (At first I thought,
> "no", but if you were to query the playState in between those lines it would
> return "finished" so maybe the answer is "yes"?). Eventually I think
> updating the relevance will get lumped together with updating finished state
> so whatever we decide there will probably apply here too.

OK.

Comment 44

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles) from comment #39)
> Comment on attachment 8575840 [details] [diff] [review]
> Part 9: Dispatch an nsIAnimationObserver notification when an animation is
> changed.
> 
> Review of attachment 8575840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that comment addressed.
> 
> As I understand it, we don't need any changes to nsTransitionManager.cpp
> because transitions don't change once they start and all the add/removed
> notification is handled in part 7 and the AnimationPlayer/Animation.

Yes that's right.

> ::: dom/smil/nsSMILKeySpline.h
> @@ +52,5 @@
> > +           mY1 == aOther.mY1 &&
> > +           mX2 == aOther.mX2 &&
> > +           mY2 == aOther.mY2 &&
> > +           mozilla::PodEqual(mSampleValues, aOther.mSampleValues,
> > +                             mozilla::ArrayLength(mSampleValues));
> 
> We don't need to compare the mSampleValues. That's calculated from the other
> parameters. It's just a cached lookup table for getting a faster initial
> guess.

OK, will remove.

Comment 45

[Cameron McCormack (:heycam)]

[Oh my goodness, that's twice I lost my comment due to the web content process hanging.]

Thanks for the detailed comments.

(In reply to Brian Birtles (:birtles) from comment #40)
> This looks great! One thing I'd like to see however, is a couple more tests
> for redundant changes.

Yes those tests are a good idea.
> > +  getComputedStyle(e);
> 
> Does this work?

No it doesn't.  Turns out anyway I don't need to flush, since all of those flush() calls are followed either by an await_frame or a DOM call that does a style flush for me.  I will remove all the flush() calls.

> > +  // Wait until at least a second of the animation has run.
> > +  yield await_timeout(1000);
> 
> This seems like a long time to wait. Bug 1072037 should now be on m-c so we
> can seek animations by setting their currentTime (and bug 1073379 has been
> there a little longer which also lets us seek animations by setting their
> startTime). Perhaps we can add something like players[0].currentTime += 1000
> here instead?

That works.

> @@ +532,5 @@
> > +  assert_records([{ added: players, changed: [], removed: [] }],
> > +                 "records after animation start");
> > +
> > +  // Wait until we are definitely filling.
> > +  yield await_timeout(1000);
> 
> We could just wait for animationend here. Since we use endpoint-exclusive
> timing, even if we happen to land at *exactly* the end of the active
> interval we will still only be considered relevant if we have a forwards
> fill.

That works too.

> @@ +629,5 @@
> > +
> > +    // Wait for the addition, change and removal MutationRecords to be delivered.
> > +    yield await_frame();
> > +    assert_records([{ added: [], changed: [], removed: players }],
> > +                    "records after animation end");
> 
> Should we be setting e.style = "" at the end here? Otherwise if we add tests
> after this I guess we'll leak setting animation-duration to 100s?

Yes we should.

Comment 46

[Cameron McCormack (:heycam)]

So we already do have some immediate change notifications going out: when changing AnimationPlayer.currentTime, we'll queue a notification without batching it.  So if you do two consecutive currentTime assignments, you'll get to animation change notifications.  If we really wanted to batch these, I'm not sure where the appropriate place to stick an nsAutoAnimationMutationBatch object would be.  But I think it's probably OK that they don't get batched.

Comment 47

[Cameron McCormack (:heycam)]

Upon reflection, one of these tests might not be worth adding.

(In reply to Brian Birtles (:birtles) from comment #40)
> Likewise, for an animation that gets added and changed within the same style
> flush.

If it is within the same style flush, then this is just the same as assigning to e.style multiple times.  This is equivalent to just setting the final styles in one go, so I don't think we gain anything by testing this.  (Once we have the ability to change an animation through the Web Animations API, we can add such a test.)

> Similarly, adding an animation whose negative delay means it is already
> finished should probably not generate any records.

This one is currently generating a record.  I'll find out why. :-)

Comment 48

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #47)
> This one is currently generating a record.  I'll find out why. :-)

I was mis-testing.

Comment 49

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #41)
> But I realise now that I meant to make subtree:true work with animation
> observers, and I haven't handled that yet.  I'll work on a followup patch
> for that.

Oh, this works just fine.  I forgot traversing up the tree is handled by the IMPL_ANIMATION_NOTIFICATION macro in nsNodeUtils.cpp rather than something in nsDOMMutationObserver.cpp.

I've added subtree tests to test_animation_observers.html just by running all tests again while observing the parent rather than the animation target.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Cameron McCormack (:heycam) from comment #41)
> Isn't the flag currently set for transient receivers too?  Both of
> nsMutationReceiverBase's constructors call SetMayHaveDOMMutationObserver
> currently.
You're very right. My mistake. Somehow I read the diff wrongly.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8577062 [details] [diff] [review]
Part 6: Listen for nsIAnimationObserver notifications and translate them to MutationObserver  notifications. (v2)

>-nsDOMMutationObserver::GetReceiverFor(nsINode* aNode, bool aMayCreate)
>+nsDOMMutationObserver::GetReceiverFor(nsINode* aNode, bool aMayCreate,
>+                                      bool aWantsAnimations)
> {
>+  NS_PRECONDITION(aMayCreate || !aWantsAnimations,
>+                  "the value of aWantsAnimations doesn't matter when aMayCreate is "
>+                  "false, so just pass in false for it");
Assertion please, not precondition.
MOZ_ASSERT would be strong enough.

>+  void AddObserver()
>+  {
>+    AddMutationObserver();
>+    if (!mParent) {
>+      // This is a transient receiver, not a receiver for an actualy
>+      // MutationObserver, so no need to note it.
>+      mRegisterTarget->SetMayHaveDOMMutationObserver();
>+      mRegisterTarget->OwnerDoc()->SetMayHaveDOMMutationObservers();
>+    }
So I was on crack on this one. No need to check !mParent and remove the comment.

Comment 52

[Cameron McCormack (:heycam)]

(In reply to Olli Pettay [:smaug] from comment #35)
> Comment on attachment 8575832 [details] [diff] [review]
> Ok, this depends on bug 1141916, and if that takes some time, one could
> explicitly check that the caller is chrome if .animations was used.

I'm going to do this, and un-comment the [ChromeOnly] and remove the explicit check once bug 1141916 lands.

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

Comment 53

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0303ba703a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca29a89ca26b
https://hg.mozilla.org/integration/mozilla-inbound/rev/98a32495dbb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/77bf91131c49
https://hg.mozilla.org/integration/mozilla-inbound/rev/f78134a84572
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a2e255c12b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3461fbe72a
https://hg.mozilla.org/integration/mozilla-inbound/rev/77bb5ac2e1f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1067ec3655aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f29af1f9c80
https://hg.mozilla.org/integration/mozilla-inbound/rev/94fa4a005c33

Comment 54

[Cameron McCormack (:heycam)]

Followup static analysis failure fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4292757e62

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like this is causing a decent number of intermittent mochitest-other failures, particularly on Mac, of the form:

652 INFO TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_animation_observers.html | [single_transition] records after transition start - number of records - got 2, expected 1
654 INFO TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_animation_observers.html | [single_transition] records after transition end - number of records - got 0, expected 1

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 1143314 on that failure.

Comment 57

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ca29a89ca26b
https://hg.mozilla.org/mozilla-central/rev/98a32495dbb1
https://hg.mozilla.org/mozilla-central/rev/77bf91131c49
https://hg.mozilla.org/mozilla-central/rev/f78134a84572
https://hg.mozilla.org/mozilla-central/rev/25a2e255c12b
https://hg.mozilla.org/mozilla-central/rev/fc3461fbe72a
https://hg.mozilla.org/mozilla-central/rev/77bb5ac2e1f6
https://hg.mozilla.org/mozilla-central/rev/1067ec3655aa
https://hg.mozilla.org/mozilla-central/rev/5f29af1f9c80
https://hg.mozilla.org/mozilla-central/rev/94fa4a005c33
https://hg.mozilla.org/mozilla-central/rev/7d4292757e62