Closed Bug 520487 Opened 11 years ago Closed 11 years ago
Support URI values in SVG/SMIL
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?
Assignee: nobody → dholbert
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
oops, meant to set status to 'assigned', not 'resolved/fixed'. :)
Status: RESOLVED → REOPENED
Resolution: 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.
Attachment #409743 - Flags: review?(dbaron) → review-
(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.)
Attachment #409743 - Attachment is obsolete: true
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.
Component: Style System (CSS) → SVG
Depends on: 520239
QA Contact: style-system → general
Summary: Extend nsStyleAnimation to support URI values → Support URI values in SVG/SMIL
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.
Attachment #409747 - Flags: review?(roc)
Attachment #409747 - Attachment description: patch 2: enable URI-valued properties (and tests) on SMIL side → patch: enable URI-valued properties (and tests) on SMIL side
Attachment #409747 - Flags: review?(roc) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.