491080 - SVG SMIL: Support xlink:href targeting of animations

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

SVG animation elements are supposed to parse their "xlink:href" attribute (if present) and use the referenced element as the target of the animation.

Currently, we only support the fallback mode (when no xlink:href is supplied), targeting the immediate parent of <animate>.

The relevant chunk of the spec for this is:
http://www.w3.org/TR/SVG11/animate.html#HrefAttribute

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: WIP v1

Here's a WIP patch to implement this.  It makes use of a "nsReferencedElement" variable called mHrefTarget to keep track of the xlink:href target elem. (I based this off of similar code in nsSVGUseElement and nsSVGEffects).

Comment 2

[Daniel Holbert [:dholbert]]

btw, I'm tracking this patch in my SMIL patch queue, hosted at http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: WIP v2

Here's an updated work-in-progress patch. Main changes:
 - Add 8 reftests
 - Address the XXXdholbert comment from WIP v1.  Now, we reset mHrefTarget in ParseAttribute when possible (when we're in a document), and otherwise, we'll wait until the BindToTree call.
 - Add cycle collection magic to fix shutdown-leaks.  Leaks were of this form:
> WARNING: Textrun cache not empty!: 'mCache.Count() == 0', file /mozilla/gfx/thebes/src/gfxTextRunWordCache.cpp, line 85
> Assertion failed at /mozilla/gfx/cairo/cairo/src/cairo-hash.c:198: hash_table->live_entries == 0
> WARNING: Leaking the RDF Service.: file /mozilla/rdf/build/nsRDFModule.cpp, line 236
> Leaked URLs:
[ snip ]
> nsStringStats
>  => mAllocCount:          17138
>  => mReallocCount:         3260
>  => mFreeCount:           16033  --  LEAKED 1105 !!!
>  => mShareCount:          15614
>  => mAdoptCount:           1093
>  => mAdoptFreeCount:       1091  --  LEAKED 2 !!!

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch v3

Ok, this patch is ready for review.  Here's all it does:
 - Uses a nsReferencedElement subclass to keep track of the xlink:href target.
 - Clears that reference whenever we unset the xlink:href attribute or unbind from a doc
 - Updates the reference when we set the attribute or bind to a doc
 - Requests an animation sample whenever we detect that the target has changed underneath us
 - Adds cycle-collector magic in nsAnimationElement, to traverse and un-link the cycle between the animation element and its nsReferencedElement

Comment 5

[Robert Longson [:longsonr]]

>+  PRBool returnVal =
>+    nsSVGAnimationElementBase::ParseAttribute(aNamespaceID, aAttribute,
>+                                              aValue, aResult);
>+  if (aNamespaceID == kNameSpaceID_XLink &&
>+      aAttribute == nsGkAtoms::href &&
>+      GetCurrentDoc()) {
>+    // NOTE: If we fail the GetCurrentDoc call, it's ok -- we'll update target
>+    // on next BindToTree call.
>+    UpdateHrefTarget(this, aValue);
>+ 

Isn't IsInDoc() more appropriate than GetCurrentDoc()

Comment 6

[Daniel Holbert [:dholbert]]

Yeah -- thanks for catching that! (I actually had already changed that earlier, but it didn't get saved because I made the change in my merge editor when it was working with a temporary copy of the file. :) )

Comment 7

[Daniel Holbert [:dholbert]]

(GetCurrentDoc --> IsInDoc now fixed in my patch queue)
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/3e0d3bb8ad74

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 388754 [details] [diff] [review]
patch v3

Nice!!!

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch v3b [checked in]

Patch landed! Here's the patch, as landed (with the IsInDoc() fix per comment 5)
http://hg.mozilla.org/mozilla-central/rev/547693481fd4

(Carrying forward r+sr from patch v3.)