Closed Bug 540090 Opened 16 years ago Closed 15 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch fix (obsolete) — Splinter Review
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?
Attached patch fix v2 (obsolete) — Splinter Review
(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)
Attached patch fix v2aSplinter Review
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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: