Last Comment Bug 520487 - Support URI values in SVG/SMIL
: Support URI values in SVG/SMIL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 504652 520239
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-04 17:42 PDT by Daniel Holbert [:dholbert]
Modified: 2009-12-10 09:47 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: Add URI support to nsStyleAnimation (17.85 KB, patch)
2009-11-02 12:05 PST, Daniel Holbert [:dholbert]
dbaron: review-
Details | Diff | Splinter Review
patch: enable URI-valued properties (and tests) on SMIL side (6.73 KB, patch)
2009-11-02 12:06 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2009-10-04 17:42:50 PDT
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?
Comment 1 Daniel Holbert [:dholbert] 2009-10-27 16:51:31 PDT
oops, meant to set status to 'assigned', not 'resolved/fixed'. :)
Comment 2 Daniel Holbert [:dholbert] 2009-11-02 12:05:58 PST
Created attachment 409743 [details] [diff] [review]
patch 1: Add URI support to nsStyleAnimation
Comment 3 Daniel Holbert [:dholbert] 2009-11-02 12:06:49 PST
Created attachment 409747 [details] [diff] [review]
patch: enable URI-valued properties (and tests) on SMIL side
Comment 4 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2009-11-18 14:01:14 PST
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.
Comment 5 Boris Zbarsky [:bz] 2009-11-18 14:11:05 PST
> 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.
Comment 6 Daniel Holbert [:dholbert] 2009-11-18 17:16:05 PST
(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 7 Daniel Holbert [:dholbert] 2009-11-19 14:32:57 PST
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.
Comment 8 Daniel Holbert [:dholbert] 2009-11-19 14:34:28 PST
Shifting component to SVG, since now the only changes here are in patch 2, which just touches SMIL-specific code.
Comment 9 Daniel Holbert [:dholbert] 2009-11-23 14:34:30 PST
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.
Comment 10 Daniel Holbert [:dholbert] 2009-12-10 09:47:33 PST
Landed: http://hg.mozilla.org/mozilla-central/rev/2544d0f0d7ef

Note You need to log in before you can comment on or make changes to this bug.