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

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).

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

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 !!!

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

>+ 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()

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. :) )

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

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

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.)