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)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(1 file, 2 obsolete files)
11.32 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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?)
Assignee | ||
Updated•16 years ago
|
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
Assignee | ||
Updated•16 years ago
|
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 | ||
Updated•15 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #429807 -
Flags: review?(roc)
Assignee | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
(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?
Assignee | ||
Comment 5•15 years ago
|
||
(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)
Assignee | ||
Comment 6•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #429843 -
Attachment is obsolete: true
Assignee | ||
Comment 7•15 years ago
|
||
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?
Assignee | ||
Comment 9•15 years ago
|
||
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.
Attachment #429853 -
Flags: review?(roc) → review+
Assignee | ||
Comment 10•15 years ago
|
||
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.
Description
•