Closed Bug 520487 Opened 15 years ago Closed 15 years ago

Support URI values in SVG/SMIL

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 1 obsolete file)

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: 15 years ago
Resolution: --- → FIXED
oops, meant to set status to 'assigned', not 'resolved/fixed'. :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Attachment #409743 - Flags: review?(dbaron)
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-
> 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.)
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
Landed: http://hg.mozilla.org/mozilla-central/rev/2544d0f0d7ef
Status: ASSIGNED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: