nsStyleAnimation & nsStyleCoord need to be extended to support URI values (e.g. for the "fill", "filter", and "marker-*" properties). The nsCSSValue class already seems to have a simple solution for this -- adding a "URI*" value to its union member-data. Perhaps nsSytleCoord can do something similar?

oops, meant to set status to 'assigned', not 'resolved/fixed'. :)

Comment on attachment 409743 [details] [diff] [review] patch 1: Add URI support to nsStyleAnimation Sorry for the delay in getting to this. You need pretty much the same change in ComputeDistance that you have in AddWeighted. The nsStyleAnimation::UncomputeValue changes are wrong; UncomputeValue needs to produce the appropriate representation for storing in the nsCSS* structs. In some sense, all the code that has nsCOMPtr<nsIURI> in the style structs is buggy, and it should really be using the appropriate referrer when it loads the resources and the appropriate principal when necessary (when is it necessary)? I think the right thing to do here is: * make the style structs use nsRefPtr<nsCSSValue::URL> where they currently use nsCOMPtr<nsIURI> (to be done in a separate patch under this one) and just adjust the current users to pull the nsIURI* out of that. (This should also include paint server values.) * make your code use nsCSSValue::URL* where it currently uses nsIURI* * just pass the value straight through rather than having all this complexity in UncomputeValue * file bugs on making sure the implementations of those properties actually use the information in the nsCSSValue::URL* structure. And I'd rather do this now since otherwise this patch would further bake in a bad design. I'd like Boris's opinion on whether he agrees with that plan, though. + const nsIURI *uri = + *static_cast<const nsCOMPtr<nsIURI>*>( + StyleDataAtOffset(styleStruct, ssOffset)); This should just be: nsIURI *uri = *static_cast<nsCOMPtr<nsIURI>* const>( StyleDataAtOffset(styleStruct, ssOffset)); with no const_cast later.

> when is it necessary In practice, in cases when someone might care about the principal of the resulting resource and when the URI of the resource is about:blank, javascript:, or data:. Or any extension scheme that sets the right protocol flags.... > I'd like Boris's opinion on whether he agrees with that plan, though. That seems reasonable to me, yes.

(In reply to comment #4) > + const nsIURI *uri = > + *static_cast<const nsCOMPtr<nsIURI>*>( > + StyleDataAtOffset(styleStruct, ssOffset)); > > This should just be: > nsIURI *uri = *static_cast<nsCOMPtr<nsIURI>* const>( > StyleDataAtOffset(styleStruct, ssOffset)); That doesn't work -- I get this error: > error: static_cast from type ‘const void*’ to type ‘nsCOMPtr<nsIURI>* const’ casts away constness I think the original cast is correct, but its result can just be stored in a normal |nsIURI *uri| -- that variable doesn't need to be const. Fixed that here: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/192c5dc8219d (I'll look into the rest of comment 4 later tonight, probably.)

Comment on attachment 409743 [details] [diff] [review] patch 1: Add URI support to nsStyleAnimation Actually, I think "patch 1" here may be entirely unnecessary -- bug 520239 should handle this just as well.

Shifting component to SVG, since now the only changes here are in patch 2, which just touches SMIL-specific code.

Comment on attachment 409747 [details] [diff] [review] patch: enable URI-valued properties (and tests) on SMIL side Requesting review. Patch is trivial -- just uncomments the properties in nsSMILCSSProperty.cpp::IsPropertyAnimatable, and enables their tests.

Landed: http://hg.mozilla.org/mozilla-central/rev/2544d0f0d7ef