SMIL: Replace most calls to "doc->GetAnimationController()" with a method that avoids lazy initialization

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Smaug just pointed out to me that Shark was showing some nsSMILAnimationController activity in a Firefox session that had no SMIL animations at all.

We try to avoid this by making nsDocument::GetAnimationController lazily initialize the controller -- however, we have places that call GetAnimationController even when there aren't any animations.
 e.g. nsPresContext::SetSMILAnimations 
     nsSVGElement::FlushAnimations()

For these cases, we probably want a different method that skips the lazy initialization.  (e.g. HasAnimationController?)
Summary: Replace most calls to "doc->GetAnimationController()" with an a method that avoids lazy initialization → Replace most calls to "doc->GetAnimationController()" with a method that avoids lazy initialization
Summary: Replace most calls to "doc->GetAnimationController()" with a method that avoids lazy initialization → SMIL: Replace most calls to "doc->GetAnimationController()" with a method that avoids lazy initialization
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Blocks: 547333
Attachment #429807 - Flags: review?(roc)
There are four calls to GetAnimationController that this patch doesn't touch.  I list them below, with explanations for why these calls should remain unchanged (with lazy initialization).

{
>/content/svg/content/src/nsSVGAnimationElement.cpp
>    * line 275 -- nsSMILAnimationController *controller = aDocument->GetAnimationController();
>    * line 304 -- nsSMILAnimationController *controller = doc->GetAnimationController();
}

The above calls are for an <animate> element to register/unregister itself with the controller.  If we're in that situation, we're clearly doing SMIL animation, so it makes sense to go ahead with the lazy initialization.

{
>/content/svg/content/src/nsSVGSVGElement.cpp (View Hg log or Hg annotations)
>    * line 567 -- nsSMILAnimationController* smilController = doc->GetAnimationController();
>    * line 1020 -- smilController = aDocument->GetAnimationController();
}

The first call there is within a SMIL-related API method, SetCurrentTime, which needs an animation controller in order to work.

The second call is within nsSVGSVGElement::BindToTree, which just uses GetAnimationController() as a proxy for whether SMIL is enabled for the current document (i.e. it checks whether the pref is on, and whether this is a data-document.)

That last check (in nsSVGSVGElement::BindToTree) could probably be improved to avoid the unnecessary lazy-initialization -- perhaps with a method like nsIDocument::SupportsAnimation (which would be a one-liner that encompasses the checks at the beginning of GetAnimationController()).  I'll look into doing that in a separate patch.
(In reply to comment #2)
> The second call is within nsSVGSVGElement::BindToTree, which just uses
[...]
> 
> That last check (in nsSVGSVGElement::BindToTree) could probably be improved to
> avoid the unnecessary lazy-initialization [...]
> I'll look into doing that in a separate patch.

Just kidding -- that GetAnimationController() call is indeed necessary, I just didn't scroll down enough to see why.  Lower down in nsSVGSVGElement::BindToTree, we associate our animation controller with our TimeContainer, and we kick off the "document timeline".  This requires an animation controller. (We could still optimize this a bit by throttling the sample-frequency down to 0 until we actually get some animations registered... but that's a separate issue.)
Can we move mAnimationController up to nsIDocument and make HasAnimationController inline?
Created attachment 429843 [details] [diff] [review]
fix v2

(In reply to comment #4)
> Can we move mAnimationController up to nsIDocument and make
> HasAnimationController inline?

Sure -- changed in this version (with some fixes for a result #include cycle).

Testing this, & then I'll re-request review.
Attachment #429807 - Attachment is obsolete: true
Attachment #429807 - Flags: review?(roc)
Created attachment 429853 [details] [diff] [review]
fix v2a

I ran this locally through SMIL reftests, and the SVG & SMIL mochitests, and it passed 'em all.

The nsIDocument uuid *just* changed out from under me on mozilla-central, so I fixed that in this new version (v2a).  :)
Attachment #429853 - Flags: review?(roc)
Attachment #429843 - Attachment is obsolete: true
Oh, I suppose I need to rev NS_ISMILANIMATIONELEMENT_IID too.  I'll make sure to do that before checking in.
Why did GetAnimAttr and HasAnimAttr have to move out of line?
Comment on attachment 429853 [details] [diff] [review]
fix v2a

>diff --git a/content/smil/nsISMILAnimationElement.h b/content/smil/nsISMILAnimationElement.h
>-  PRBool GetAnimAttr(nsIAtom* aAttName, nsAString &aResult) const
>-  {
>-    return Content().GetAttr(kNameSpaceID_None, aAttName, aResult);
>-  }
>+  virtual PRBool GetAnimAttr(nsIAtom* aAttName, nsAString& aResult) const = 0;
> 
>   /*
>    * Check for the presence of an attribute in the global namespace.
>    */
>-  PRBool HasAnimAttr(nsIAtom* aAttName) const
>-  {
>-    return Content().HasAttr(kNameSpaceID_None, aAttName);
>-  }
>+  virtual PRBool HasAnimAttr(nsIAtom* aAttName) const = 0;

FWIW: This quoted change is to keep us from directly using any "nsIContent" values in this header file, so that I can remove the "#include nsIContent.h" from this file. (and break an #include-cycle)

This change might appear to be (marginally) bad for performance, because I'm turning two non-virtual methods into virtual methods.  However, the old non-virtual versions made **calls** to a virtual method -- "Content()". So we haven't actually lost anything.
pushed: http://hg.mozilla.org/mozilla-central/rev/65d602fba99f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.