SVG SMIL: Add support for animating CSS properties

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 3 bugs, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-Opera][parity-webkit], URL)

Attachments

(4 attachments, 20 obsolete attachments)

473 bytes, image/svg+xml
Details
19.67 KB, patch
Details | Diff | Splinter Review
827 bytes, patch
dbaron
: review+
Details | Diff | Splinter Review
280.86 KB, patch
dholbert
: review+
dholbert
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
I'm filing this bug on adding support for animating CSS properties in SVG, via
 <animate attributeType="CSS ...>
Without looking into it, since bug 435442 and bug 435441 rely on bug 332335, I suspect this one does too?
(Assignee)

Comment 2

9 years ago
Good question, but no, this bug doesn't depend on bug 332335.

I just talked to zwol, and he said that bug 435442 and bug 435441 have that dependency specifically because those bugs would add new CSS properties/values (and bug 332335 would make it a lot easier to incorporate new types of CSS properties/values into our code base).

In contrast, this bug here doesn't create any new CSS properties or values, so it shouldn't depend on bug 332335.
Blocks: 482402

Updated

9 years ago
Duplicate of this bug: 486944

Comment 4

9 years ago
Created attachment 371394 [details]
animate opacity testcase

Updated

9 years ago
Keywords: testcase

Comment 5

9 years ago
bug 487178 SVG:SMIL implement <set>

is this dependent?

Updated

9 years ago
Duplicate of this bug: 487178
(Assignee)

Comment 7

9 years ago
(In reply to comment #5)
> is this dependent?
(see response in Bug 487178 comments 2-3)
(Assignee)

Updated

9 years ago
Assignee: nobody → dholbert
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

9 years ago
Created attachment 373764 [details] [diff] [review]
getoverridestyle patch WIP v1
(Assignee)

Comment 9

9 years ago
Created attachment 373765 [details] [diff] [review]
smil_css patch WIP v1
(Assignee)

Comment 10

9 years ago
I've just attached 2 WIP patches here. These patches are also hosted in my "smil-patches" patch queue, at http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/ -- the attached versions are identical to the ones in that patch-queue as of now. (at revision 12b0ddfd50a6)

To use these patches in your own debug build, apply getoverridestyle.patch first, and then apply smil_css.

Here's a quick summary of what these patches do:
* getoverridestyle.patch:
   - Adds an additional "override" style rule on each SVG Element, to store SMIL-animated style.  This rule gets applied immediately after the Inline Style Rule in the cascade. (see "HTMLCSSStyleSheetImpl::RulesMatching" for where this happens) This rule is readable/writeable via a nsIDOMCSSStyleDeclaration structure, obtainable via the new method "nsIContent::GetOverrideStyle(nsIDOMCSSStyleDeclaration** aStyle)"

* smil_css:
  - Animate CSS lengths (e.g. font-size, stroke-width)
  - Animate CSS "numbers" (e.g. opacity, as used in attachment 371394 [details])
  - Animate CSS colors, with these caveats:
     ** only via <animate> right now -- there's no <animateColor> element yet
     ** interpolation only works if you specify hex / rgb color values right now (e.g. "#FF0000").  If you use keywords (e.g "red"), then the patch sets the beginning & ending color correctly, but it won't interpolate between them yet.

The above animation all support "by", "from-by", "to", and "from-to" animations.  They don't support paced animation mode yet, just because I haven't implemented ComputeDistance for the underlying types yet.  (but that should be trivial)

I've added a bunch of reftests (in layout/reftests/svg/smil/style/) to demonstrate (& verify) correct behavior for most of the above.

Note that the pref "svg.smil.enabled" is still off by default, so for anyone testing with the above patches, remember to enable that pref (or no SMIL functionality will work. :))

I've fired off a try-server build (with the SMIL pref auto-enabled) to make sure this builds & passes tests on other platforms, and I'll post links to the completed builds when they finish.
(Assignee)

Comment 11

9 years ago
(In reply to comment #10)
> * getoverridestyle.patch:
>    - Adds an additional "override" style rule on each SVG Element

Sorry -- I meant "on each nsStyledElement" -- I initially put the style rule there, since it could be useful for other sorts of styled elements, too (e.g. CSS transitions).

(I should probably limit this to just SVG elements for now, though.)
(Assignee)

Comment 12

9 years ago
(In reply to comment #10)
> I'll post links to the
> completed builds when they finish.

Slightly-updated builds appearing here (including a minor fix to getoverridestyle.patch):
https://build.mozilla.org/tryserver-builds/2009-04-21_10:39-dholbert@mozilla.com-try-d1149fe600a/

The tryserver run last night showed that some reftests were failing on Linux due to what looks like a slight rounding error -- I'm currently investigating that.
(Assignee)

Comment 13

8 years ago
(In reply to comment #12)
> The tryserver run last night showed that some reftests were failing on Linux
> due to what looks like a slight rounding error

It turns out the reftest failure there was just a precision issue with using pts, on machines where the the app-units-per-point value is something irregular.  Basically, I was expecting that "font-size: 2pt" would make "N em" == "2N pt" exactly, but in fact, these lengths can actually end up being *slightly* different when converted to app units, for some values of N.

At dbaron's suggestion, I've changed some unit-conversion reftests to use px units instead of pts, since the app-unit-to-px ratio is more likely to be something "nice".  Now, all unittests pass on all 3 platforms.  (I made a few other small code fixes, too.)

Here's a link to the test-passing tryserver build:
https://build.mozilla.org/tryserver-builds/2009-04-22_10:24-dholbert@mozilla.com-try-3913313be40/
(Assignee)

Comment 14

8 years ago
Created attachment 375681 [details] [diff] [review]
getoverridestyle patch v2

Here's an updated version of the "getoverridestyle" patch.  I think this part is ready for review.

Description of implementation:
 (1) Adds a protected nsCOMPtr<nsICSSStyleRule> to each nsStyledElement, to store that element's override style rules.
   (1) Provide access to this rule via methods nsIContent::Get/SetOverrideStyleRule. (This mirrors "Get/SetInlineStyle" in that interface)
 (2) Also provide read/write access via a simpler "nsIContent::GetOverrideStyle" method, which returns a "nsIDOMCSSStyleDeclaration" that can be queried and modified to transparently update the override style rule. (This mirrors the "GetStyle" method in nsGenericElement and nsStyledElement.)
  (a) Add a new flag to nsDOMCSSAttrDeclaration (the implementation for nsIDOMCSSStyleDeclaration), so that it can handle either Inline Style or Override Style.  (Previously, it was specific to Inline Style.)
  (b) Add an nsDOMCSSDeclaration to the nsDOMSlots class, mirroring "mStyle" in  that class. (This is the backing store for GetOverrideStyle's returned structure.)

Note that this patch doesn't actually expose a "GetOverrideStyle" method to JS -- it's meant more for internal, C++-only use at this point (particularly for SMIL and other animation frameworks).

However, if we later decide to actually implement the GetOverrideStyle API for JS, I'm pretty sure this would be a suitable backing for it.  (No other browsers actually support the GetOverrideStyle API, last I checked)
Attachment #373764 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #373765 - Attachment description: Patches WIP v1 chunk 2: smil_css → smil_css WIP v1
(Assignee)

Updated

8 years ago
Attachment #373764 - Attachment description: Patches WIP v1 chunk 1: getoverridestyle.patch → getoverridestyle patch WIP v1
(Assignee)

Updated

8 years ago
Attachment #375681 - Flags: superreview?(dbaron)
Attachment #375681 - Flags: review?(dbaron)
(Assignee)

Comment 15

8 years ago
Created attachment 375683 [details] [diff] [review]
smil_css patch WIP v2

Here's an updated version of the SMIL + CSS patch, to go with the updated getOverrideStyle patch.

This one's not ready for review yet, but it will be soon.
(Assignee)

Updated

8 years ago
Attachment #375683 - Attachment description: smil_css WIP v2 → smil_css patch WIP v2
(Assignee)

Updated

8 years ago
Attachment #373765 - Attachment description: smil_css WIP v1 → smil_css patch WIP v1
Attachment #373765 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Whiteboard: [parity-Opera][parity-webkit]
Comment on attachment 375681 [details] [diff] [review]
getoverridestyle patch v2

A few issues I want to think about here:

 * given that we're not implementing the DOM override style API, maybe we should call the methods something slightly different, like SMILOverrideStyle

 * Is it ok that we're only implementing this on nsStyledElement and not nsGenericElement.  What elements are nsGenericElement but not nsStyledElement?

We might want to talk to sicking or bz about these...
(Assignee)

Comment 17

8 years ago
(In reply to comment #16)
>  * given that we're not implementing the DOM override style API, maybe we
> should call the methods something slightly different, like SMILOverrideStyle

How about "AnimatedStyle"?  (I'm hoping this structure can be used for other animation frameworks, like CSS Transitions and/or CSS Animations)

>  * Is it ok that we're only implementing this on nsStyledElement and not
> nsGenericElement.

I think it's ok. I noticed that nsGenericElement doesn't support InlineStyle at all -- all of its InlineStyle methods are no-ops [1].  So, I was assuming that the OverrideStyle (or AnimatedStyle) methods should be no-ops there, too.

> What elements are nsGenericElement but not nsStyledElement?

I don't know :) sicking's gone for the next few days, but I'll ping bz if I see him.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#2929
(In reply to comment #17)
> (In reply to comment #16)
> >  * given that we're not implementing the DOM override style API, maybe we
> > should call the methods something slightly different, like SMILOverrideStyle
> 
> How about "AnimatedStyle"?  (I'm hoping this structure can be used for other
> animation frameworks, like CSS Transitions and/or CSS Animations)

I think CSS transitions are going to be at a different level of the system; I suspect CSS animations will as well, although I don't know much about them.

> >  * Is it ok that we're only implementing this on nsStyledElement and not
> > nsGenericElement.
> 
> I think it's ok. I noticed that nsGenericElement doesn't support InlineStyle at
> all -- all of its InlineStyle methods are no-ops [1].  So, I was assuming that
> the OverrideStyle (or AnimatedStyle) methods should be no-ops there, too.

Well, it's ok not to support inline style for elements where the elements don't have a style attribute.  The only things that should support it are the languages that do have a style attribute.

On the other hand, what you're designing here seems like it's supposed to be generic.  (Then again, I'm not sure which way the DOM overrideStyle API is.)
(Assignee)

Comment 19

8 years ago
(In reply to comment #18)
> I think CSS transitions are going to be at a different level of the system; I
> suspect CSS animations will as well, although I don't know much about them.

Ok, SMILOverrideStyle sounds good then.

> What elements are nsGenericElement but not nsStyledElement?

I talked to sicking this morning, and he said basically raw XML nodes (with no namespace) are the only things that'd be nsGenericElement but not nsStyledElement.

He also said that these nsGenericElements *are* stylable via CSS stylesheets, regardless of the fact that they don't have a style attribute.  (I think this is related to what dbaron was saying towards the end of comment 18.)

So, it probably makes sense to place override style support at the nsGenericElement level. (rather than at the nsStyledElement level)

(Initially I was only supporting nsStyledElement because I misunderstood the meaning of the class names. I'd thought nsStyledElements supported styling via CSS, and other versions of nsGenericElement didn't. But now I understand that this isn't the case.)
(In reply to comment #18)
> I think CSS transitions are going to be at a different level of the system; I
> suspect CSS animations will as well, although I don't know much about them.

They're just an extension of CSS transitions. So yeah, we won't be sharing code here and the APIs can just be named SMILOverrideStyle.

> Well, it's ok not to support inline style for elements where the elements don't
> have a style attribute.  The only things that should support it are the
> languages that do have a style attribute.
> 
> On the other hand, what you're designing here seems like it's supposed to be
> generic.  (Then again, I'm not sure which way the DOM overrideStyle API is.)

Yes, I think these should go on nsGenericElement.
(Assignee)

Comment 21

8 years ago
Created attachment 378813 [details] [diff] [review]
SMILOverrideStyle Patch v3

Here's an updated "Override Style" patch.
Notable changes:
 - Renamed OverrideStyle to SMILOverrideStyle
 - Moved implementations of OverrideStyle methods from nsStyledElement to nsGenericElement
 - Added #ifdef MOZ_SMIL everywhere (now that this is definitively SMIL-specific)
Attachment #375681 - Attachment is obsolete: true
Attachment #378813 - Flags: superreview?(dbaron)
Attachment #378813 - Flags: review?(dbaron)
Attachment #375681 - Flags: superreview?(dbaron)
Attachment #375681 - Flags: review?(dbaron)
(Assignee)

Comment 22

8 years ago
Created attachment 378815 [details] [diff] [review]
smil_css_stylechanges WIP patch v3

I'm splitting out my changes in layout/style into a separate patch, attached here. (An earlier version of this code was previously included in the ""smil_css" patch.)

This patch basically does three things:
 - Adds utility functions for grabbing nsCSSValue, nsCSSRect, etc. for a particular property from a nsCSSDeclaration. (for use in parsing begin/end value strings into nsSMILValues)
 - Adds utility functions for appending  nsCSSValue, nsCSSRect, etc. to strings (for use in setting the final animated value)
 - Make the static method nsRuleNode::SetColor public (for use in converting nsCSSValues into a nscolor, for interpolation/adding/etc)
Attachment #375683 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #378815 - Attachment description: smil_css_stylechanges WIP patch v2 → smil_css_stylechanges WIP patch v3
(Assignee)

Comment 23

8 years ago
Created attachment 378819 [details] [diff] [review]
smil_css WIP patch v3

And here's the updated smil_css patch (which stacks on top of the two other patches I've just attached).

Notable changes:
 * Add support for nsCSSRect-valued properties
 * Better support for the various types of values inside of a nsCSSValues
 * Support for named color identifiers (on top of existing support for hex & rgb() values)
 * Misc cleanup / addressing XXX comments
 * Add a giant mochitest (test_smilCSSProps.xhtml + svg_property_database.js), to verify correct animation behavior on every property in the SVG 1.1 Property Index [1].
  For each property, this mochitest test creates a simple animation targeting an appropriate SVG element, and it compares the computed style at the beginning, middle, and end of the animation against expected output.  Most properties are tested with multiple "interesting" pairs of from/to values.
  For properties that are explicitly non-animatable (e.g. 'direction'), the test verifies that animations have no effect on its computed style.
  Currently, this mochitest results in 1202 passes, 4 failures (because I don't yet support dynamically looking up the result of 'inherit' for the clip property, which breaks interpolation if 'inherit' is used), and a few 'todo' properties ('cursor' and 'stroke-dasharray', since I don't yet have support for handling list-valued CSS properties; and also the properties 'font', 'marker', and 'overflow, since I don't yet have support for shorthand properties.)

[1] http://www.w3.org/TR/SVG/propidx.html
Attachment #378813 - Flags: superreview?(dbaron)
Attachment #378813 - Flags: review?(dbaron)
Attachment #378813 - Flags: review+
Comment on attachment 378813 [details] [diff] [review]
SMILOverrideStyle Patch v3

>+nsGenericDOMDataNode::SetSMILOverrideStyleRule(nsICSSStyleRule* aStyleRule,
>+                                           PRBool aNotify)

You should fix the weird indentation here.

>diff --git a/content/base/src/nsGenericElement.cpp b/content/base/src/nsGenericElement.h

In nsGenericElement, I'd think both new members should be in mSlots, right, not just one of them.  It's true that both aren't accessed unless SMIL stuff uses them, right?

>+nsICSSStyleRule*
>+nsGenericElement::GetSMILOverrideStyleRule()
>+{
>+  if (!mSMILOverrideStyleRule) {
>+    // Create an empty declaration to base our style rule off of.
>+    nsCSSDeclaration *declaration = new nsCSSDeclaration();
>+    if (!declaration) {
>+      NS_WARNING("Failed to allocate nsCSSDeclaration");
>+      return nsnull;
>+    }
>+    if (!declaration->InitializeEmpty()) {
>+      NS_WARNING("could not initialize nsCSSDeclaration");
>+      declaration->RuleAbort();  // deletes declaration
>+      return nsnull;
>+    }
>+
>+    // Now, create the style rule.
>+    nsresult rv = NS_NewCSSStyleRule(getter_AddRefs(mSMILOverrideStyleRule),
>+                                     nsnull, declaration);
>+    if (NS_FAILED(rv)) {
>+      NS_WARNING("could not create contentstylerule");
>+      declaration->RuleAbort();  // deletes declaration
>+      return nsnull;
>+    }
>+  }

This is a decent-sized chunk of code.  Any chance you could share it between here and nsDOMCSSAttributeDeclaration::GetCSSDeclaration ?

>+NS_IMETHODIMP
>+nsGenericElement::SetSMILOverrideStyleRule(nsICSSStyleRule* aStyleRule,
>+                                           PRBool aNotify)
>+{
>+  mSMILOverrideStyleRule = aStyleRule;
>+
>+  // Pretend that the style attribute changed, so that the SMIL override style
>+  // modification will be noticed
>+  // XXXdholbert we probably need a way to specifically notify that the
>+  // SMIL override style has changed.
>+  if (aNotify) {
>+    PRUint8 modType = static_cast<PRUint8>(nsIDOMMutationEvent::MODIFICATION);
>+    nsNodeUtils::AttributeChanged(this, kNameSpaceID_None, nsGkAtoms::style,
>+                                  modType, 0);
>+  }

I don't think we want to fake an AttributeChanged notification.  We might just want to add a new method to nsIMutationObserver or nsIDocumentObserver.  But I have mixed feelings about this.


r=dbaron with those changes, although I probably want to take another look at the eventual result of the notification issue.

You need the review of a content peer anyway, though.  Whoever that is might have some ideas about what we should do for the notifications...
(Assignee)

Comment 25

8 years ago
Created attachment 384144 [details] [diff] [review]
smil_overridestyle patch v4

(In reply to comment #24)
> You should fix the weird indentation here.

Done.  (fixed a few instances of trailing whitespace and >80-char lines, too)

> In nsGenericElement, I'd think both new members should be in mSlots, right, not
> just one of them.  It's true that both aren't accessed unless SMIL stuff uses
> them, right?

Done.  And yes, that's correct -- they're only accessed by SMIL stuff.

> >+nsGenericElement::GetSMILOverrideStyleRule()
> This is a decent-sized chunk of code.  Any chance you could share it between
> here and nsDOMCSSAttributeDeclaration::GetCSSDeclaration ?

Good point -- it turns out I was actually doing extra work here.  Both of the methods you mentioned were prepared to allocate a new style rule if there wasn't one, even though only one of them needs this capability.  So, I've now made GetSMILOverrideStyleRule into a simple accessor, with no auto-allocation -- if there's no existing style rule, it just returns nsnull.

> >+  // Pretend that the style attribute changed, so that the SMIL override style
> >+  // modification will be noticed
> >+  // XXXdholbert we probably need a way to specifically notify that the
> >+  // SMIL override style has changed.
> I don't think we want to fake an AttributeChanged notification.  We might just
> want to add a new method to nsIMutationObserver or nsIDocumentObserver.  But I
> have mixed feelings about this.

I talked a bit to dbaron and bz in person about how best to handle this -- we decided that the overhead of using a nsIMutationObserver/nsIDocumentObserver method wouldn't be worth it, so instead, I've added a nsPresContext method  ("SMILOverrideStyleChanged") which posts a restyle event on the frame constructor.

> r=dbaron with those changes, although I probably want to take another look at
> the eventual result of the notification issue.
> You need the review of a content peer anyway, though.

Ok -- requesting followup review from dbaron on the updated notification mechanism, and also from bz as a content peer.
Attachment #378813 - Attachment is obsolete: true
Attachment #384144 - Flags: superreview?(bzbarsky)
Attachment #384144 - Flags: review?(dbaron)
(Assignee)

Comment 26

8 years ago
Created attachment 384145 [details] [diff] [review]
smil_css_stylechanges patch v4

Here's my updated patch with changes to layout/style.  (for getting data into & out of the processed nsCSS[Struct] types.)

Changes from previous patch are:
 - Added 'nsCSSDeclaration::GetCSSValueList'
 - The added 'AppendCSS[Struct]ToString' methods now return PRBool as an indication of success
 - Some cleanup

I think this patch is ready for review.
Attachment #378815 - Attachment is obsolete: true
Attachment #384145 - Flags: review?(dbaron)
(Assignee)

Comment 27

8 years ago
Created attachment 384154 [details] [diff] [review]
smil_css patch v4

Here's the updated version of my main SMIL+CSS patch.

Main changes since previous smil_css patch:
 - Added support for Shorthand-valued and List-valued CSS Properties
 - Added "nsISMILCSSType" interface (inheriting from nsISMILType), which adds some methods common to CSS value structures.  (This let me remove a lot of long switch statements and if-waterfalls in nsCSSProperty.cpp, replacing them with virtual function calls)
 - Created "nsSMILAnimationContext" struct, which holds a reference to the style context & PresContext (for unit conversion) & the owner SVG Element (for use in converting percentages into lengths)
 - Implemented ComputeDistance for rects & colors, with a mochitest to verify correct behavior on paced animations of a number of properties
 - Made all NS_ASSERTIONS into NS_ABORT_IF_FALSE
 - Make nsSMILCSSUtils methods return PRBool instead of nsresult
 - When parsing from/by/to values, convert them into a computed style value, so as to translate values like 'inherit' and 'currentColor' into interpolatable values
 - Added support for animation by/from/to negative CSS values (letting clamping be performed by CSS engine at final stage), per SVG 1.1 Appendix F.4
 - Add support for percent length values
 - Expand mochitests & reftests to test new behavior

NOTABLE TODO:
 - Respect "color-interpolation: linearRGB" during color animations. I began implementing this, but I'm not 100% sure on my understanding of the expected behavior (see long comment near the top of nsSMILCSSUtils.cpp),  and I couldn't find an SVG implementation with support for this (looked at WebKit, Opera, Adobe SVG Viewer), so I'm deferring this to a follow-up bug for now.

I think this is ready for review.  r?-ing roc, and also dbaron for the CSS-structure-related bits (primarily nsSMILCSS*Type and nsSMILCSSUtils)
Attachment #378819 - Attachment is obsolete: true
Attachment #384154 - Flags: superreview?(dbaron)
Attachment #384154 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #384154 - Flags: superreview?(dbaron) → review?(dbaron)
(Assignee)

Updated

8 years ago
Attachment #384144 - Flags: superreview?(bzbarsky) → review?(bzbarsky)

Comment 28

8 years ago
We've only implemented support for color-interpolation-filters in layout, color-interpolation is currently ignored.
(Assignee)

Comment 29

8 years ago
Ah, thanks Robert.  Maybe we can enable "color-interpolation" support in animation as part of the eventual patch that enables it elsewhere (hopefully sharing the underlying colorspace-conversion code).

Also: try-server builds with the latest "v4" patches (and with the SMIL pref default-enabled) are available here:

https://build.mozilla.org/tryserver-builds/dholbert@mozilla.com-try-45fec6bea62/

Comment 30

8 years ago
That would be bug 298281; you would need cairo to do linearRGB gradients for that.

I think you're better dropping the partial code for color-interpolation from this patch but if you really want to keep it I think the following methods should be part of nsSVGUtils so that gsRGBToLinearRGBMap can stay as an implementation detail of colour conversion.

+inline void
+ConvertSRGBToLinearRGB(PRUint8& aR, PRUint8& aG, PRUint8& aB)
+{
+  aR = nsSVGUtils::gsRGBToLinearRGBMap[aR];
+  aG = nsSVGUtils::gsRGBToLinearRGBMap[aG];
+  aB = nsSVGUtils::gsRGBToLinearRGBMap[aB];
+}
+
+inline void
+ConvertLinearRGBToSRGB(PRUint8& aR, PRUint8& aG, PRUint8& aB)
+{
+  aR = nsSVGUtils::glinearRGBTosRGBMap[aR];
+  aG = nsSVGUtils::glinearRGBTosRGBMap[aG];
+  aB = nsSVGUtils::glinearRGBTosRGBMap[aB];
+}
I agree, let's drop color-interpolation here.
(Assignee)

Comment 32

8 years ago
Thanks for that feedback -- I've now dropped the "color-interpolation: linearRGB" helper-code (and the giant comment about it) in the copy of this patch within my public patch-queue.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/5aa001b26ca4
Brian, whatever feedback you can give on these patches would be appreciated.
+  // Methods specific to CSS types
+  // ----------------------------------
+  virtual PRBool ValueFromCSSDeclaration(nsCSSProperty aPropID,
+                                         const nsCSSDeclaration& aDecl,
+                                         nsSMILValue& aValue) const = 0;
+  virtual PRBool ValueToString(nsCSSProperty aPropID,
+                               const nsSMILValue& aValue,
+                               nsAString& aString) const = 0;
+  virtual void ExtractStringValueForProperty(nsCSSProperty aPropID,
+                                             nsCOMPtr<nsICSSDeclaration> aDecl,
+                                             nsAString& aValue) const

Add documentation for these

+class nsSMILAnimationContext {

This needs a comment explaining exactly what this class is for.

+ public:
+ private:

No leading space

+nsPresContext*
+nsSMILAnimationContext::GetPresContext()
+{
+  return mPresContext;
+}
+
+nsStyleContext*
+nsSMILAnimationContext::GetStyleContext()
+{
+  return mStyleContext;
+}
+
+nsSVGSVGElement*
+nsSMILAnimationContext::GetSVGContext()
+{
+  return mSVGContext;
+}

Can't these be inline in the class?

+// XXXdholbert: For animation from/to 'inherit' values to work correctly when
+// the inherited value is *also* being animated, we really should be traversing
+// our animated nodes in a parents-first order.  That is, we need to compute
+// the animated style of a given node before we compute its children's animated
+// style, so that its children will get the correct value for 'inherit' if
+// applicable.  (Breadth-first or depth-first would both be fine)
+// We probably don't want to traverse the whole content tree at each sample,
+// though... that would be slow.

Interesting. Do you have a failing test case for this?

This shouldn't be too hard to fix. We'd need to make nsSMILCompositorTable a two-level hash table, so it maps from nsIContent* to a map from (nsIAtom*, isCSS) to the list of nsSMILAnimationFunctions. Then iterating over it becomes a loop over the nsIContents, with an inner loop over the attributes of each element. Then break the inner loop out into its own function that does all the composition for an element; in that function, walk up the ancestor chain checking each ancestor to see if it's in nsSMILCompositorTable and not processed yet; if it is, recursively invoke the composition function on the ancestor.

In very deeply nested documents traversing all the ancestors of each animated element could get slow, but I don't think we have to worry about that; if we do, then we could add a flag on elements (as nsINode properties if necessary) to indicate that we've already processed an element and all its ancestors.

I agree with Robert, access to gsRGBToLinearRGBMap etc should be encapsulated behind an (inline) function.

More coming...
Is the negative-value code in nsSMILCSSProperty::ValueFromString a general solution to the problem of having to specify animation target values that are out of bounds --- does it solve the whole problem? Or does it just solve one part of that problem?

Comment 36

8 years ago
(In reply to comment #32)
> Thanks for that feedback -- I've now dropped the "color-interpolation:
> linearRGB" helper-code (and the giant comment about it) in the copy of this
> patch within my public patch-queue.
> http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/5aa001b26ca4

This is insufficient, you need to revert the nsSVGUtils change that makes the maps public too.
Including the 3 patches from this bug, makes the hang condition on this test

http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-30-t.html

that was previously reported in bug 483584 worse.
(Assignee)

Comment 38

8 years ago
(In reply to comment #37)
Bill, thanks for that report -- I see the worse-hang too -- now instead of being less responsive, Firefox now becomes completely unresponsive when loading that page (at least in a debug build).

From a quick run of the sysprof profiling tool, it looks like we're spending virtually all of our time parsing, in nsSMILCSSProperty::ComputedValueFromString (inside of that, nsComputedDOMStyle::GetPropertyValue, and inside of that, PresShell::FlushPendingNotifications)

I also get infinite spamming of this warning, which I don't think I've seen before:
WARNING: Someone passed native anonymous content directly into frame construction.  Stop doing that!: file nsCSSFrameConstructor.cpp, line 5979

I'll investigate this further.
That assertion typically means that we're reframing the anonymous child of some element.
(Assignee)

Comment 40

8 years ago
I think I've addressed all the review suggestions posted so far in the "smil_css" patch in my patch queue, located at
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches
The updated smil_css patch is available there -- current revision is 6f29ebb62153.  (I can repost it here, too, if requested -- otherwise, I'm planning to wait the end of this review-cycle before reposting, to minimize attachment-spam.)

(In reply to comment #34)
> +  // Methods specific to CSS types
> Add documentation for these

Done.

> +class nsSMILAnimationContext {
> This needs a comment explaining exactly what this class is for.

Done.

> + public:
> + private:
> No leading space

Done.

> +nsSMILAnimationContext::GetPresContext()
> +nsSMILAnimationContext::GetStyleContext()
> +nsSMILAnimationContext::GetSVGContext()
> Can't these be inline in the class?

Yes, they can.  Done.

> 
> +// XXXdholbert: For animation from/to 'inherit' values to work correctly when
> +// the inherited value is *also* being animated, we really should be
> traversing
> +// our animated nodes in a parents-first order. 
> Interesting. Do you have a failing test case for this?

Yes -- I've now included a mochitest (test_smilCSSInherit.xhtml) with deeply-nested animations, each animating from "inherit" to a different fixed value.  Right now, the innermost element (a <text> node) ends up with a seemingly-random font-size, depending on the (random) order in which we process the animations.

Your solution sounds good -- I'll file a separate bug on implementing that fix.

> I agree with Robert, access to gsRGBToLinearRGBMap etc should be encapsulated
> behind an (inline) function.

Ok -- I've actually just removed all mention of gsRGBToLinearRGBMap etc in my patch for now, per comment 30, comment 31, & comment 37.  (Robert Longon, thanks for catching the nsSVGUtils changes in comment 37 -- I've reverted those now)

(In reply to comment #35)
> Is the negative-value code in nsSMILCSSProperty::ValueFromString a general
> solution to the problem of having to specify animation target values that are
> out of bounds --- does it solve the whole problem? Or does it just solve one
> part of that problem?

That code just solves part of the out-of-bounds-values problem.  Specifically, it allows us to handle negative values whose absolute values are <= the largest in-bounds positive value.  So, e.g. for opacity, rather than only allowing values in the range [0,1] my negative-value code lets us accept values in the range [-1,1]. (which is useful so we can have negative "by" values, to animate opacity from="1" by="-1")

There are two main out-of-bounds situaions that my code doesn't handle:
 - compound values -- it won't try to pick out negative signs in the middle of a compound value, like rgb(50, -50, 50).
 - too-large values -- if the CSS parser clamps a value as being too big.

This problem is result of using the CSS parser for our value-parsing here.  Perhaps a better general solution would be to implement some sort of "lax bounds-checking mode" for the parser?

In any case, this issue shouldn't hurt us too much right now.  Off the top of my head, the only properties which the parser would currently clamp when we don't want it to are:
 ** The "opacity" and "XXX-opacity" properties: values >1 would be clamped to 1 by the parser, and values < -1 would be clamped to -1. (specifically, they'd be converted to positive, parsed & clamped to 1, and then re-converted to negative.)
 ** Color values with bizarre rgb values (e.g. rgb(1000, -50, 256) will be clamped to a valid range (e.g. rgb(255,0,255))  by the CSS parser, I think.  However, even if the CSS parser *could* handle these, I think we'd still fail on them for an entirely different reason -- the "nscolor" type only allows 8 bits per channel, so in interpolation/addition math, out-of-bounds values will already end up wrapping around rather than being clamped.  So, it'll take some more significant changes to correctly handle out-of-bounds color-channel values.

I don't think either of those (animating colors and opacity outside their reasonable boundaries) would be common use cases, so I tend to think that this relaxed-bounds-checking issue is something that can be fixed later on (again, possibly with a special flag on the parser).
(In reply to comment #40)
> There are two main out-of-bounds situaions that my code doesn't handle:
>  - compound values -- it won't try to pick out negative signs in the middle of
> a compound value, like rgb(50, -50, 50).
>  - too-large values -- if the CSS parser clamps a value as being too big.
> 
> This problem is result of using the CSS parser for our value-parsing here. 
> Perhaps a better general solution would be to implement some sort of "lax
> bounds-checking mode" for the parser?

I think this is probably the cleanest solution, yes.

> In any case, this issue shouldn't hurt us too much right now.  Off the top of
> my head, the only properties which the parser would currently clamp when we
> don't want it to are:
>  ** The "opacity" and "XXX-opacity" properties: values >1 would be clamped to 1
> by the parser, and values < -1 would be clamped to -1. (specifically, they'd be
> converted to positive, parsed & clamped to 1, and then re-converted to
> negative.)
>  ** Color values with bizarre rgb values (e.g. rgb(1000, -50, 256) will be
> clamped to a valid range (e.g. rgb(255,0,255))  by the CSS parser, I think. 
> However, even if the CSS parser *could* handle these, I think we'd still fail
> on them for an entirely different reason -- the "nscolor" type only allows 8
> bits per channel, so in interpolation/addition math, out-of-bounds values will
> already end up wrapping around rather than being clamped.  So, it'll take some
> more significant changes to correctly handle out-of-bounds color-channel
> values.

Ugh. Good point.

Still, I think having a relaxed-mode CSS parser would be cleaner and more extensible. Do you think perhaps we should just removing this negative-value code for now and do the parser as a separate fix?
(Assignee)

Comment 42

8 years ago
I think the negative-value code is useful as a partial solution until we've got a relaxed-mode CSS parser.

It handles what I see as the most common use cases that involve normally-out-of-bounds CSS values: animating by negative opacity values as large as -1 (which is enough to get you between any two values in the valid range of opacities), and animating by negative lengths (for font-size, stroke-width, etc).

I think the cases that the negative-value code fixes are the ones that authors are most likely to encounter, whereas the cases it doesn't fix are the ones that authors are much less likely to encounter.
(Assignee)

Updated

8 years ago
Blocks: 501183
(Assignee)

Comment 43

8 years ago
(In reply to comment #40)
> Your solution sounds good -- I'll file a separate bug on implementing that fix.

I filed bug 501183 about compositing animations on ancestors first, in order to reliably handle 'inherit'.
(Assignee)

Updated

8 years ago
Blocks: 501188
(Assignee)

Comment 44

8 years ago
and I filed bug 501188 on the relaxed-mode CSS parser.
Comment on attachment 384144 [details] [diff] [review]
smil_overridestyle patch v4

>+++ b/content/base/public/nsIContent.h
>+  virtual nsresult GetSMILOverrideStyle(nsIDOMCSSStyleDeclaration** aStyle) = 0;

This looks unused.  I assume your other patch adds callers?

>+   * Get the SMIL override style rule for this content node.  If the rule
>+   * doesn't exist yet, an empty rule will be created & returned.

That doesn't match what the implementation in nsGenericElement does.  Is the comment wrong, or the code?

>+   * Set the SMIL override style rule for this node.  This will send an
>+   * appropriate AttributeChanged notification aNotify is true.

Not at all.  Document what it actually does if aNotify is true?

>+  NS_IMETHOD SetSMILOverrideStyleRule(nsICSSStyleRule* aStyleRule,

Why NS_IMETHOD instead of just |virtual nsresult|?

Are there actually consumers that pass both values of aNotify?

>+++ b/content/base/src/nsGenericDOMDataNode.cpp
>+nsGenericDOMDataNode::GetSMILOverrideStyle(nsIDOMCSSStyleDeclaration** aStyle)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;

Set *aStyle to null too?

>+++ b/content/base/src/nsGenericElement.cpp
>+nsGenericElement::GetSMILOverrideStyleRule()
>+{
>+  nsGenericElement::nsDOMSlots *slots = GetDOMSlots();

This will force slots creation any time this method is called.  That seems undesirable, no?  Use GetExistingDOMSlots(), and return null if that returns null.

>+nsGenericElement::SetSMILOverrideStyleRule(nsICSSStyleRule* aStyleRule,
>+    nsIDocument* doc = GetOwnerDoc();

This should almost certainly be GetCurrentDoc(), no?  Not sure how that affects your NS_ABORT_IF_FALSE...

>+++ b/layout/base/nsPresContext.cpp
>+nsPresContext::SMILOverrideStyleChanged(nsIContent* aContent)
>+{
>+  mShell->FrameConstructor()->PostRestyleEvent(aContent, eReStyle_Self,
>+                                               nsChangeHint_ReconstructFrame);

Whoa.  Why that last bit?  I'd think NS_STYLE_HINT_NONE would be a more appropriate value for that argument.  Why do we want to force a frame reconstruct on each override style change, instead of just figuring our the right action based on what styles changed?

>+++ b/layout/style/nsDOMCSSAttrDeclaration.h
>+  PRBool mIsSMILOverride;

Another option, of course, is to have two different subclasses, have pure virtual functions in the superclass for getting and setting the style rules, and have the subclasses implement them differently.

I guess that replaces conditional branches with virtual function calls and saves 4 bytes per inline style decl.... Not sure it's a win, offhand.  Does make it clearer that this boolean can't change though.

If we do keep the boolean instead of going that route, can we make it const and still have the initializer work?  I'd think we should be able to.
Attachment #384144 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 46

8 years ago
Below are responses to bz's review comments.  I've pushed my fixes to my public patch queue, and I'll be posting a series of updated patches shortly.  I've also verified that my automated tests still pass.

(In reply to comment #45)
> >+  virtual nsresult GetSMILOverrideStyle(nsIDOMCSSStyleDeclaration** aStyle) = 0;
> This looks unused.  I assume your other patch adds callers?

Yes -- the patch labeled "smil_css" calls this method.

> That doesn't match what the implementation in nsGenericElement does.  Is the
> comment wrong, or the code?

Oops, good catch!  The comment is wrong.  It accurately describes behavior from an earlier patch revision :) but I changed the method's behavior in comment 25, because only caller of this method (nsDOMCSSAttributeDeclaration::GetCSSDeclaration) already handles the allocation if the style rule doesn't exist.

I've now fixed the comment.

> >+   * Set the SMIL override style rule for this node.  This will send an
> >+   * appropriate AttributeChanged notification aNotify is true.
> Not at all.  Document what it actually does if aNotify is true?

Oops -- fixed.  Updated text: "If aNotify is true, this method will notify the document's pres context, so that the style changes will be noticed."

> Why NS_IMETHOD instead of just |virtual nsresult|?

I was just mirroring the declaration for the similar method "SetInlineStyleRule()", in the same file.  I'm happy to change this if one way's better -- which should I use here? (I actually wasn't aware that there was a significant difference between NS_IMETHOD vs. |virtual nsresult| -- isn't this basically a stylistic thing?)

> Are there actually consumers that pass both values of aNotify?

Yes -- there are calls with both values in nsDOMCSSAttrDeclaration.cpp.  Specifically, we pass in PR_FALSE when creating an initial (empty) style rule, but we pass in PR_TRUE when the style rule changes.  (This matches the existing calls to the similar SetInlineStyleRule method.)

> Set *aStyle to null too?

Fixed.

> >+nsGenericElement::GetSMILOverrideStyleRule()
> >+{
> >+  nsGenericElement::nsDOMSlots *slots = GetDOMSlots();
> This will force slots creation any time this method is called.  That seems
> undesirable, no?  Use GetExistingDOMSlots(), and return null if that returns
> null.

Good call -- fixed.

> >+nsGenericElement::SetSMILOverrideStyleRule(nsICSSStyleRule* aStyleRule,
> >+    nsIDocument* doc = GetOwnerDoc();
> This should almost certainly be GetCurrentDoc(), no?  Not sure how that affects
> your NS_ABORT_IF_FALSE...

Fixed.

This shouldn't affect the NS_ABORT_IF_FALSE -- all animation targets should necessarily have a CurrentDocument, when they're being actively animated.

REASONING: If an <animate> node is currently being sampled, it must have received a Document in its last BindToTree call (because it needs that document in order to register itself for sampling).  This Document becomes the <animate> node's CurrentDocument towards the end of nsGenericElement::BindToTree.  Moreover, the <animate> node's *target* must be in the same document as the <animate> node. (Currently, the target will always be the parent of the animate node, but even once we support more flexible xlink:href targeting, the target still must be "part of the current SVG document fragment" according to the SVG section 19.2.4)

Hence, all actively-animated targets must have a CurrentDocument.

> >+++ b/layout/base/nsPresContext.cpp
> >+nsPresContext::SMILOverrideStyleChanged(nsIContent* aContent)
> >+{
> >+  mShell->FrameConstructor()->PostRestyleEvent(aContent, eReStyle_Self,
> >+                                               nsChangeHint_ReconstructFrame);
> 
> Whoa.  Why that last bit?  I'd think NS_STYLE_HINT_NONE would be a more
> appropriate value for that argument.  Why do we want to force a frame
> reconstruct on each override style change, instead of just figuring our the
> right action based on what styles changed?

Thanks for the tip -- the "nsChangeHint_ReconstructFrame" was just a guess there.  I was passing that since we *might* need to reconstruct frames, and I hadn't realized that we'd still be able to figure that out elsewhere. I fixed it to use NS_STYLE_HINT_NONE now, as you suggested.

> >+++ b/layout/style/nsDOMCSSAttrDeclaration.h
> >+  PRBool mIsSMILOverride;
> 
> Another option, of course, is to have two different subclasses
[snip]
> Not sure it's a win, offhand.  Does
> make it clearer that this boolean can't change though.
> 
> If we do keep the boolean instead of going that route, can we make it const and
> still have the initializer work?  I'd think we should be able to.

I, too, am not sure whether the subclass-based alternative would be a win.  For now I've simply made "mIsSMILOverride" const, per your later suggestion, but I can change to a subclass-based alternative if you prefer.
> I actually wasn't aware that there was a significant difference between
> NS_IMETHOD vs. |virtual nsresult|

On Windows they have different calling conventions, iirc.

For things that don't need the COM ABI, |virtual nsresult| is preferred.

I think I'm fine with the const mIsSMILOverride.
(Assignee)

Comment 48

8 years ago
Created attachment 386150 [details] [diff] [review]
smil_overridestyle patch v4b

Here's the updated smil_overridestyle patch, with fixes from comment 46.
Attachment #384144 - Attachment is obsolete: true
Attachment #386150 - Flags: review?(bzbarsky)
Attachment #384144 - Flags: review?(dbaron)
(Assignee)

Comment 49

8 years ago
Created attachment 386151 [details] [diff] [review]
smil_overridestyle interdiff between v4 & v4b
Attachment #386151 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #386150 - Flags: review?(bzbarsky)
Attachment #386151 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 50

8 years ago
Created attachment 386153 [details] [diff] [review]
smil_css patch v4b

Here's the updated main smil_css patch, incorporating the fixes mentioned in comment 40.
Attachment #384154 - Attachment is obsolete: true
Attachment #386153 - Flags: superreview?(dbaron)
Attachment #386153 - Flags: review?(roc)
Attachment #384154 - Flags: review?(roc)
Attachment #384154 - Flags: review?(dbaron)
(Assignee)

Comment 51

8 years ago
Created attachment 386155 [details] [diff] [review]
smil_css interdiff between v4 & v4b

Here's the interdiff showing what's changed in the new smil_css patch.

(I haven't made any changes to the "smil_css_stylechanges" patch, so I'm not posting a new version of that one -- attachment 384145 [details] [diff] [review] is still the latest version.)
(Assignee)

Updated

8 years ago
Attachment #386150 - Flags: review?(dbaron)
(Assignee)

Comment 52

8 years ago
(In reply to comment #47)
> For things that don't need the COM ABI, |virtual nsresult| is preferred.

Ok, that makes sense.  I've fixed that on SetSMILOverrideStyleRule in my patch queue -- s/NS_IMETHOD/virtual nsresult/ on the declarations, and s/NS_IMETHODIMP/nsresult/ on the definitions. (in nsIContent.h, nsGenericElement.*, and nsGenericDOMDataNode.*)

I've also r?'d dbaron on attachment 386150 [details] [diff] [review] (smil_overridestyle patch v4b) in case he's got any concerns on the updated notification method, per his note at the end of comment 24:
> r=dbaron with those changes, although I probably want to take another look at
> the eventual result of the notification issue.

The notification code is nsPresContext::SMILOverrideStyleChanged, which is called by nsGenericElement::SetSMILOverrideStyleRule.
(Assignee)

Comment 53

8 years ago
(In reply to comment #38)
> (In reply to comment #37)
> I see the worse-hang too -- now instead of
> being less responsive, Firefox now becomes completely unresponsive when loading
> that page (at least in a debug build).

This is actually an infinite-recursion bug. Here's what happens on the testcase that Bill linked, with my latest patches:
  (1) As part of an animation sample, we reparse the "to"/"from"/"by" values on an <animate> node.
  (2) As part of the parsing, we do a temporary restyle, to convert a specified value into its corresponding computed-style value. (This happens in nsSMILCSSProperty::ComputedValueFromString, within the smil_css patch.)  (This behavior is required primarily to change 'inherit' and 'currentColor' into interpolatable units, as mentioned in comment 27.)
  (3) This temporary restyle triggers some frame reconstruction
  (4) The frame reconstruction triggers a call to nsSVGElement::GetAnimatedLengthValues, which calls nsSVGElement::FlushAnimations, which initiates a new sample.

So, we've now started a new animation sample **while we're already in an animation sample**.  This happens recursively in a death-spiral, and we hang (slowly eating up memory).

With my most recently-posted patches, I actually *don't* get the "Someone passed native anonymous content" warning anymore. (mentioned in comment 38-39) That's probably fixed because I'm using "NS_STYLE_HINT_NONE" instead of "nsChangeHint_ReconstructFrame" for notification now, per comment 46.  (But apparently there's still *some* necessary frame construction going on.)

I'm not immediately sure what the correct solution is for this.
>  (3) This temporary restyle triggers some frame reconstruction

Is this because we're animating properties that lead to style reconstructs (which would be unfortunate) or for some other reason?

Can you breakpoint at the point at which the hint is updated with ReconstructFrame during ReResolveStyleContext?
It seems like the right thing to do here is to prevent the restyle triggered by ComputedValueFromString from actually triggering any changes to the frame tree, or triggering reflow, or invalidation.  None of those are desirable here, right?

It would be really nice if we could avoid messing with the "live" style while doing this, for that matter.  If all we really need to do is deal with inherit and currentColor, can't we just do those directly using the computed style of the element's parent and the element respectively?
(Assignee)

Comment 56

8 years ago
(In reply to comment #55)
> If all we really need to do is deal with inherit
> and currentColor, can't we just do those directly using the computed style of
> the element's parent and the element respectively?

Yeah, you're right -- I've got a patch in the works that uses the parent's computed style, in the case of 'inherit', rather than modifying our "live" style.

And as for currentColor, I've realized that nsRuleNode::SetColor already handles that (using the passed-in StyleContext).  So I don't actually need to worry about "translating" the currentColor keyword ahead of time, after all.
(Assignee)

Comment 57

8 years ago
I posted an update about the "full-animate-elem-30-t.html" hang issue in bug 483584 comment 12.  Let's move any further discussion of that testcase & its hangs to that bug.
(Assignee)

Comment 58

8 years ago
Created attachment 387751 [details] [diff] [review]
smil_css patch v4c

Here's an updated smil_css patch that minimizes messing with "live" style, per comment 55.  Now, the only places that we tweak the style during sampling are:
  - nsSMILCSSProperty::SetAnimValue() (obviously -- that's where an animation's effects actually take effect)
  - nsSMILCSSProperty::GetBaseValue() (we need to temporarily clear the SMILOverrideStyle there in order to look up the "underlying" style value, in order for "by" and "to" animations to work)

I'm also now bundling the CSS Property ID with each nsCSSValue, so that we know the correct percent basis to use when adding / interpolating / computing distances.  ('font-size' uses the inherited font-size as its percent basis, and the rest use the viewport's normalized hypotenuse.)  My previous patch didn't need to do this -- it just assumed the percent basis was always the viewport's normalized hypotenuse. That worked because font-size percentages were always converted away early-on, when I passed them through computed style.  But now that I'm not passing parsed values through computed style, I need to handle percent-values for font-size, too.
Attachment #386153 - Attachment is obsolete: true
Attachment #386153 - Flags: superreview?(dbaron)
Attachment #386153 - Flags: review?(roc)
(Assignee)

Comment 59

8 years ago
Created attachment 387753 [details] [diff] [review]
smil_css interdiff between v4b & v4c
The decl of ComputedValueFromString can go away too, right?
(Assignee)

Updated

8 years ago
Attachment #387751 - Flags: review?(roc)
(Assignee)

Comment 61

8 years ago
oops, yes. :) Thanks for catching that.  Fixed in my smil-patches repo; not reposting here to avoid patch-spam, but that fix will be in my next posted patch revision. (with other review fixes)
(Assignee)

Updated

8 years ago
Attachment #387751 - Flags: review?(dbaron)
+nsSMILAnimationContext::nsSMILAnimationContext(nsIContent* aTargetElement)
+  : mPresContext(nsnull), mStyleContext(nsnull), mSVGContext(nsnull)
+{
+  nsIDocument* doc = aTargetElement->GetDocument();
+  if (doc) {
+    nsIPresShell* shell = doc->GetPrimaryShell();
+    if (shell) {
+      mPresContext = shell->GetPresContext();
+      nsIFrame* frame = shell->GetPrimaryFrameFor(aTargetElement);
+      if (frame) {
+        mStyleContext = frame->GetStyleContext();
+      }
+    }

Seems to me that we should also handle the case where the content doesn't have a frame. You need to do something like nsInspectorCSSUtils::GetStyleContextForContent or maybe even call it directly (except the flushing behaviour isn't what you want, probably). And then I guess you'd need a test.

+  // (2) Get Computed Style
+  nsAutoString computedStyleVal;
+  GetCSSComputedValue(mElement, mPropID, computedStyleVal);

This is still going to flush and potentially trigger reflows, possibly frame reconstruction, etc.

Also, what if the computed value depends on the parent style and the parent is style is also animated? Don't we need all ancestors to have their override style temporarily unset, in theory?

Maybe we need some kind of flag on the style set that we can set that a) disables all override styles and b) suppresses style changes having any effect? Then for decent performance we probably need to set that flag, compute all base values we're going to need, and then clear the flag.

+  nsCOMPtr<nsIDocument> doc = aElem->GetDocument();

This doesn't need to be an nsCOMPtr.

+      nsCOMPtr<nsIDOMCSSStyleDeclaration> computedStyle;
+      nsCOMPtr<nsIDOMElement> domElement(do_QueryInterface(aElem));
+      viewCSS->GetComputedStyle(domElement, EmptyString(),
+                                getter_AddRefs(computedStyle));
+      if (computedStyle) {
+        nsCOMPtr<nsICSSDeclaration> computedDecl =
+          do_QueryInterface(computedStyle);
+        if (computedDecl) {

Could we add a method to nsPIDOMWindow that does GetCmputedStyle and returns us an already_AddRefed<nsDOMComputedStyle> directly?

Likewise, can we make GetSMILOverrideStyle/SetSMILOverrideStyle return an already_AddRefed<nsDOMCSSAttributeDeclaration>?

+    // * SPECIAL CASE #1: time = 0 -- we want to exactly match the "from"
+    // value, so we'll just copy it, instead of running the more generalized
+    // interpolation code (which would replace 'auto' values with their numeric
+    // equivalents)
+    // * SPECIAL CASE #2: Both the 'from' & 'to' values are simply 'auto', so
+    // the result should be 'auto' as well, at all points during the animation.

So when 'to' is auto and we compute the final value, Interpolate just doesn't get called? Should say so in this comment.

+  // In SVG 1.1 & CSS2, each value in a CSS "rect()" is interpreted as
+  // distance-from-that-side, with 'auto' meaning 0 length. Since 'auto' is the
+  // only non-length value type we're expecting here, we'll just substitute in
+  // a zero-length value for all non-length-values.
+  // XXXdholbert NOTE: In CSS2.1 (as compared to SVG1.1/CSS2), rect() lengths
+  // are interpreted as distances *from the origin*, rather than from the
+  // corresponding side. (So 'auto' does NOT generally mean 0 length for the
+  // 'top' and 'bottom' components).  If we ever try try to use SMIL to animate
+  // non-SVG content, we'll need to account for this.

Is there anything preventing people from animating non-SVG content using this code? I don't think so. I think we should assume the CSS 2.1 behaviour for all rect() values. If SVG 1.1 doesn't agree, it needs to be fixed to conform to CSS 2.1.

+    case eCSSProperty_font: {
+      FontWrapper* data = static_cast<FontWrapper*>(shorthandVal->mData);
+      delete data;
+    } break;

I think I'd prefer to give the Wrapper classes a common base class with a virtual destructor, so we don't need this switch.

It seems to me that a lot of this SMIL code belongs in the style system. For example, it's not good that special code for font, marker and overflow has to exist in the SMIL code. I don't know what the right interface to the style system would be, though.

+      nsSVGSVGElement* ctx = aAnimContext.GetSVGContext();
+      if (ctx) {
+        return nsPresContext::CSSPixelsToAppUnits(
+                                        ctx->GetLength(nsSVGUtils::XY));
+      }
+    } break;

It seems like this should do something for non-SVG content.

+    // XXXdholbert Handle linearRGB?
+    destR += toAddR * aCount;
+    destG += toAddG * aCount;
+    destB += toAddB * aCount;

Seems like there should be something detecting or avoiding overflow here.

Does the spec say how to do color distances and interpolation? I find it hard to believe that treating colors as RGB 3-vectors for computing distance and interpolating produces good results.

+  // work & any conversion errors from going to linearRGB color space & back.

Fix comment, since you're not converting to linearRGB space and back.

+  // 'inherit', and we should have already translated 'inherit' values away by
+  // this point.)

Add an assertion to that effect?

+PRInt32
+nsSMILParserUtils::CheckForNegativeNumber(const nsAString& aStr)
+{
+  PRInt32 absValLocation = -1;
+
+  NS_ConvertUTF16toUTF8 spec(aStr);

Wouldn't it be simpler and less overhead to parse UTF16 directly?

At least I read the whole patch this time! Phew!
Depends on: 504652
(In reply to comment #58)
> Created an attachment (id=387751) [details]
> smil_css patch v4c

This patch seems to have bit-rotted quite a bit.
(Assignee)

Comment 64

8 years ago
(In reply to comment #62)
> Seems to me that we should also handle the case where the content doesn't have
> a frame.

Ok -- what sorts of situations would hit that, out of curiosity?  When I first read your comment, I was thinking of content within display:none iframes, but those won't even have a PresContext (which means that even nsInspectorCSSUtils::GetStyleContextForContent would fail for them), so I don't think there's much we can do for those.

i.e. in what situations do we have a PresContext, but not a StyleContext?

> +  GetCSSComputedValue(mElement, mPropID, computedStyleVal);
> This is still going to flush and potentially trigger reflows, possibly frame
> reconstruction, etc.

Well, "nsStyleFont::CalcDifference" suggests that font-size changes will trigger a NS_STYLE_HINT_REFLOW, which I presume means we'd end up reflowing. (Perhaps that doesn't happen in SVG, though?)
And nsStyleSVG::CalcDifference adds "nsChangeHint_ReflowFrame" to the reflow hint when we modify the "text-rendering" property (which is animatable), with the comment "// May be needed for non-svg frames".  Perhaps that's not an issue, though, because presumably any non-SVG frames that would need reflowing wouldn't be animated using SVG animation?  (So the frames that would be reconstructed wouldn't be animated ones?)

> Also, what if the computed value depends on the parent style and the parent is
> style is also animated? Don't we need all ancestors to have their override
> style temporarily unset, in theory?

No.  To get the base value, we need to strip animation effects off of the current element, but *not* off of its ancestors.

I think an example helps here -- consider this sample testcase snippet, which I've also got posted at 
http://people.mozilla.org/~dholbert/tests/474049/inherit-fontsize-1.svg
and a similar testcase using 'fill':
http://people.mozilla.org/~dholbert/tests/474049/inherit-fill-1.svg

 <g font-size="5pt">
   <set attributeName="font-size" attributeType="CSS"
        begin="0" dur="indefinite" to="30pt"/>
   <text y="50">
     First Text Node
   </text>
   <text y="100">
     Second Text Node
     <animate attributeName="font-size" attributeType="CSS"
              begin="1s" dur="1s" by="10pt" fill="freeze"/>
   </text>
 </g>

QUESTION: What should we use as the base value for the 'by' animation inside the second <text> node?
 OPTION A: 30pt (inherited value, *including* animations on ancestors)
  - Result: 
    * Initially, both text nodes are 30pt
    * At time=1s, second text node smoothly grows up to 40pt.
 OPTION B: 5pt  (inherited value, *excluding* animations on ancestors)
  - Result: 
    * Initially, both text nodes are 30pt
    * Then, the second text node suddenly jumps down to a font-size of 5pt, and grows from there up to 15pt

The results of Option "A" seem much more reasonable to me. Here's how different implementations handle this:
 - Opera (10.00 beta) goes with "OPTION A" in both of the testcases linked above.
 - My posted patch goes with "OPTION A", too. (though 100% correctness in this will depend on bug 501183 being fixed, per comment 43).
 - WebKit Trunk (r45934) sort of goes with "OPTION B", but it does so in a very broken way -- it makes the second <text> node ignore its parent's animation **even during the first second**, before that text node's animation begins.  So I'm not sure this was a design decision on WebKit's part -- I think it might be just a limitation of their current implementation.

I think "Option A" is by far the most sensible.  AFAIK, the spec doesn't explicitly state this, though. I'll email www-svg and/or www-style to get clarification on this.

> This doesn't need to be an nsCOMPtr.

Fixed.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/fdd58b4ba3c2

> Could we add a method to nsPIDOMWindow that does GetCmputedStyle and returns us
> an already_AddRefed<nsDOMComputedStyle> directly?

I take it you meant nsIDOMCSSStyleDeclaration or nsICSSDeclaration? (which hold the computed style) (I don't think there's such thing as a nsDOMComputedStyle/nsIDOMComputedStyle)

I think I fixed this:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/5de3e43c7a8f
Let me know if that's what you had in mind.

> Likewise, can we make GetSMILOverrideStyle/SetSMILOverrideStyle return an
> already_AddRefed<nsDOMCSSAttributeDeclaration>?

Well, currently, the signatures of GetSMILOverrideStyle / GetSMILOverrideStyleRule exactly mirror those of GetStyle/GetInlineStyleRule.  (Similar accessors for similar types of "local" style.)  This change would break that similarity.  But maybe that's not a big deal?

> So when 'to' is auto and we compute the final value, Interpolate just doesn't
> get called? Should say so in this comment.

Correct -- the smil_css patch adds an NS_ABORT_IF_FALSE to this effect, in nsSMILAnimationFunction::InterpolateResult.
Comment fixed per your suggestion.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/fdd58b4ba3c2

> I think we should assume the CSS 2.1 behaviour for all
> rect() values. If SVG 1.1 doesn't agree, it needs to be fixed to conform to CSS
> 2.1.

Ok.  That's actually a bit tricker -- when we interpolate using a rect that has 'auto' for its |bottom| or |right| components, now we'll have to look up the computed height/width + borderpadding height/width (which, in the future, could feasibly be animated themselves... :))  But, it makes sense that we should stick with the more up-to-date spec's understanding.

I'm leaving this as a to-do for now, because this code is going to need rewriting / refactoring as part of bug 504652, anyway.  I've updated the comment a bit, though.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/b845adc77259

> I think I'd prefer to give the Wrapper classes a common base class with a
> virtual destructor, so we don't need this switch.

Good idea! Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/dab3edfdb7f1

> It seems to me that a lot of this SMIL code belongs in the style system. For
> example, it's not good that special code for font, marker and overflow has to
> exist in the SMIL code. I don't know what the right interface to the style
> system would be, though.

I think this refactoring will happen in bug 504652. (I'll rework this patch to use the code that'll be introduced in that bug.)

> +      nsSVGSVGElement* ctx = aAnimContext.GetSVGContext();
> +      if (ctx) {
> +        return nsPresContext::CSSPixelsToAppUnits(
> +                                        ctx->GetLength(nsSVGUtils::XY));
> +      }
> +    } break;
> 
> It seems like this should do something for non-SVG content.

For non-SVG content, this method currently returns 0 as the percent basis (which effectively means that percentage values translate to 0), and I claim that's the correct behavior.  The only non-SVG-specific properties that could hit this clause are "word-spacing" and "letter-spacing", and AFAICT, those properties don't support percentage values outside of SVG.  So, it makes sense to treat percentage-values as "0" outside of SVG, because that's equivalent to the default value of "normal".  (Note that SVG 1.1 *does* support percentages for these values, since it includes percentages within the <length> type, unlike CSS2.1)

References:
http://www.w3.org/TR/CSS21/text.html#spacing-props
http://www.w3.org/TR/CSS21/syndata.html#value-def-length
http://www.w3.org/TR/SVG/types.html#DataTypeLength

> +    destR += toAddR * aCount;
> +    destG += toAddG * aCount;
> +    destB += toAddB * aCount;
> 
> Seems like there should be something detecting or avoiding overflow here.

Yes -- fixed & added tests:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/fd2442f41193

> Does the spec say how to do color distances and interpolation? I find it hard
> to believe that treating colors as RGB 3-vectors for computing distance and
> interpolating produces good results.

Yes, it does -- it's under the definition of <animateColor>.  The SVG spec links to the SMIL Animation spec, which has a "Note to implementers" that says:
> Interpolation is defined on a per-color-channel basis.
and:
> the animation function should assume Euclidean RGB-cube distance to compute the distance and pacing.
[ ref: http://www.w3.org/TR/smil-animation/#animateColorElement ]

IIUC, that means we're supposed to just treat colors as RGB 3-vectors, in interpolation & distance 

> +  // work & any conversion errors from going to linearRGB color space & back.
> Fix comment, since you're not converting to linearRGB space and back.

Fixed.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/fdd58b4ba3c2

> +  // 'inherit', and we should have already translated 'inherit' values away by
> +  // this point.)
> Add an assertion to that effect?

Fixed.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/fdd58b4ba3c2

> +PRInt32
> +nsSMILParserUtils::CheckForNegativeNumber(const nsAString& aStr)
> +{
> +  PRInt32 absValLocation = -1;
> +
> +  NS_ConvertUTF16toUTF8 spec(aStr);
> 
> Wouldn't it be simpler and less overhead to parse UTF16 directly?

Yes -- fixed.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/4f7adc189acc

(In reply to comment #63)
> This patch seems to have bit-rotted quite a bit.

Bill: I'll post a new version soon, but if you like, you can grab the latest patch versions from my patch queue (linked in comment 10).  Click the "files" link at the top, and then grab & apply smil_overridestyle, smil_css_stylechanges, and smil_css (in that order).
(In reply to comment #64)
> (In reply to comment #62)
> Bill: I'll post a new version soon, but if you like, you can grab the latest
> patch versions from my patch queue (linked in comment 10).  Click the "files"
> link at the top, and then grab & apply smil_overridestyle,
> smil_css_stylechanges, and smil_css (in that order).

Ahh.  The "files" link.

That's the magic I could not figure out.

It's obvious now. :-)
> i.e. in what situations do we have a PresContext, but not a StyleContext?

display:none content in a visible document.

> Well, "nsStyleFont::CalcDifference" suggests that font-size changes will
> trigger a NS_STYLE_HINT_REFLOW, which I presume means we'd end up reflowing.
> (Perhaps that doesn't happen in SVG, though?)

SVG doesn't reflow, but we're going to have to deal with animation affecting
non-SVG content, so this is an issue. But I think we'll just have to deal.
We just have to make sure that we're not holding onto frame pointers.

> // XXXdholbert Is this correct? (doing these on the nsCOMPtr directly)
> NS_IF_ADDREF(computedStyle);
> return computedStyle.get();

I think you can just do computedStyle.forget() to avoid the extra addref.
But maybe LookupComputedStyleFor could return an
already_AddRefed<nsComputedDOMStyle>, so we can avoid the QI to
nsICSSDeclaration?

> Well, currently, the signatures of GetSMILOverrideStyle /
> GetSMILOverrideStyleRule exactly mirror those of GetStyle/GetInlineStyleRule. 
> (Similar accessors for similar types of "local" style.)  This change would
> break that similarity.  But maybe that's not a big deal?

They should probably all be fixed. But I guess you can leave them consistent
for now.

> For non-SVG content, this method currently returns 0 as the percent basis
> (which effectively means that percentage values translate to 0), and I claim
> that's the correct behavior.

OK

The rest of your replies sound good. Thanks!!!
roc asked me to review this so here are my comments for the smil_css patch.

First, let me say, this is awesome. Thanks Daniel for doing this. It's really comprehensive and I particularly like all the testing.

Second, I reviewed patch v4c so maybe you've already addressed many of these comments in the latest patch in your queue. Sorry, I forgot I could go straight to your queue. Hopefully you can just ignore what I've written here.

> diff --git a/content/smil/Makefile.in b/content/smil/Makefile.in
> +	nsSMILCSSProperty.cpp \
> +	nsSMILCSSUtils.cpp \
> +	nsSMILCSSValueListType.cpp \
> +	nsSMILCSSRectType.cpp \
> +	nsSMILCSSShorthandType.cpp \
> +	nsSMILCSSValueType.cpp \
> +	nsSMILCSSValuePairType.cpp \

I think roc already mentioned this, but I guess a fair bit of this CSS-specific code will be moved out of the SMIL code in bug 504652?

> +		-I$(srcdir)/../svg/content/src \

Is it possible to remove linkage with SVG? Again, maybe this is something to address in bug 504652?

> diff --git a/content/smil/nsISMILAttr.h b/content/smil/nsISMILAttr.h
> ... 
>    /**
> +   * Returns the targeted content node.  This is needed to e.g. retrieve the
> +   * presentation & style contexts, so we can convert between different units
> +   * of length).
> +   */
> +   virtual nsIContent* GetTargetElement() = 0;

See my comment on nsSMILAnimationContext below. I'm wondering why *SMIL* needs to convert between different units.

> diff --git a/content/smil/nsISMILCSSType.h b/content/smil/nsISMILCSSType.h
> ...
> +   *
> +   * @param aPropID The CSS property whose value is requested.
> +   * @param aDest   The declaration we'll query for the property value.
> +   * @param aValue  The structure to be populated with the obtained value.
> +   */
> +  virtual PRBool ValueFromCSSDeclaration(nsCSSProperty aPropID,
> +                                         const nsCSSDeclaration& aDecl,
> +                                         nsSMILValue& aValue) const = 0;

Do we need to document what the return type means here? (Likewise for ValueToString and IsInherit)

> diff --git a/content/smil/nsSMILAnimationContext.h b/content/smil/nsSMILAnimationContext.h
> ...
> +//----------------------------------------------------------------------
> +// nsSMILAnimationContext
> +//
> +// The SMIL Animation Context is a simple bundle for several objects that
> +// help us convert between units.  For example, suppose we're interpolating
> +// between (or adding) the following values:
> +//   * 10em, 10px
> +//   * 5in, 10%
> +//   * red, rgb(0,128,128)
> +// In each case, we need to convert the given values into the same units before
> +// we can interpolate (or add) them.  To do the required unit-conversions,
> +// we'll need access to the PresContext, StyleContext, and/or SVG Context.
> +// This class provides easy access to these Context objects.
> +//
> +// WARNING: Instances of this class must not last beyond the end of a sample,
> +// because the member variables aren't ref-counted.
> +//

I'm sure you've thought of this, but I'm just wondering if this class is necessary.

I can see you need it for converting between different unit types but I think the original idea was that nsISMILAttr::GetBaseValue would return an nsSMILValue with sufficient source context for (the appropriate subclass of) nsISMILType to do correct interpolation/addition etc. Then nsISMILAttr::SetAnimValue would interpret the value in light of the target context. So the concrete implementations of nsISMILAttr would keep track of the necessary style context and pres context and bundle whatever is needed for interpolation/addition/etc. into the nsSMILValue. That way you don't need to pass around animation context objects.

I'm guessing there are some situations with CSS where this doesn't work though? Or is the issue that the context isn't available to nsSMILCSSProperty? Or is this approach more efficient?

I'm just curious anyway.

> +  /*
> +   * Returns the SVG Context (<svg> element) associated with the target element.
> +   */
> +  nsSVGSVGElement* GetSVGContext()   { return mSVGContext;   }

As above, I wonder if we can somehow remove the dependency on SVG.

> diff --git a/content/smil/nsSMILAnimationFunction.cpp b/content/smil/nsSMILAnimationFunction.cpp
> ...
> +  // XXXdholbert removing const-ness here -- maybe aSMILAttr shouldn't be const?
> +  nsIContent* targetElem =
> +    const_cast<nsISMILAttr*>(&aSMILAttr)->GetTargetElement();

Could we just do a const overload on GetTargetElement() that returns a const nsIContent? Or does that involve making nsIContent const-correct as well?

> diff --git a/content/smil/nsSMILCSSProperty.cpp b/content/smil/nsSMILCSSProperty.cpp
> ...
> +nsresult
> +nsSMILCSSProperty::ValueFromString(const nsAString& aStr,
> +                                   const nsISMILAnimationElement* aSrcElement,
> +                                   nsSMILValue& aValue) const
> +{
> ...
> +  if (smilType == &nsSMILCSSValueType::sSingleton) {
> ...
> +    rv = SimpleValueFromString(subString, aValue);
> +  } else {
> +    rv = SimpleValueFromString(aStr, aValue);
> +  }    
> +
> +  // Return early if we've already failed.
> +  NS_ENSURE_TRUE(NS_SUCCEEDED(rv), rv);

I'm just wondering how the system should behave when we load an SVG document where the animation targets a CSS property that is not animatable (or not supported).

Currently I think we may get something like:

  * nsSMILCSSProperty::ValueFromCSSDeclaration gives a warning ("Trying to extract CSS Value from a non-animatable (or unsupported) property")

  (* So nsSMILCSSProperty::SimpleValueFromString return NS_ERROR_FAILURE)

  * So nsSMILCSSProperty::GetBaseValue() gives a warning ("Failed at creating base animated value")

  * And here in nsSMILCSSProperty::ValueFromString we get another warning that NS_ENSURE_TRUE failed.

That's probably ok I guess. Just checking it's the behaviour we want. And will this happen for every sample? i.e. will the console clog up with these messages?

> diff --git a/content/smil/nsSMILCSSProperty.h b/content/smil/nsSMILCSSProperty.h
> ...
> +class nsSMILCSSProperty : public nsISMILAttr
> +{

I think this class needs a comment explaining what it does.

> diff --git a/content/smil/nsSMILCSSRectType.cpp b/content/smil/nsSMILCSSRectType.cpp
> ...
> +// nsISMILType Methods
> +// -------------------
> +nsresult
> +nsSMILCSSRectType::Init(nsSMILValue& aValue) const
> +{
> +  NS_ABORT_IF_FALSE(aValue.IsNull(),
> +                    "Unexpected value type");
> +
> +  aValue.mU.mPtr = new nsCSSRect();
> +  aValue.mType = this;
> +  return NS_OK;
> +}

I think we need to check for OOM here (like we do for nsSMILCSSValuePairType::Init).

> +void
> +nsSMILCSSRectType::Destroy(nsSMILValue& aValue) const
> +{
> +  NS_ABORT_IF_FALSE(aValue.mType == this, "Unexpected SMIL value type");
> +  NS_ABORT_IF_FALSE(aValue.mU.mPtr,
> +                    "nsSMILValue for CSS val should have non-null pointer");
> +
> +  delete static_cast<nsCSSRect*>(aValue.mU.mPtr);

The second check is not strictly necessary--I guess it's just a consistency check but on that matter see the next comment.

> +nsresult
> +nsSMILCSSRectType::Assign(nsSMILValue& aDest,
> +                          const nsSMILValue& aSrc) const
> +{
> +  NS_ABORT_IF_FALSE(aDest.mType == aSrc.mType, "Incompatible SMIL types");
> +  NS_ABORT_IF_FALSE(aDest.mType == this, "Unexpected SMIL value type");
> +  NS_ABORT_IF_FALSE(aDest.mU.mPtr,
> +                    "nsSMILValue for CSS val should have non-null pointer");
> +  NS_ABORT_IF_FALSE(aSrc.mU.mPtr,
> +                    "nsSMILValue for CSS val should have non-null pointer");

The last two checks here are fine, but I think what we did with nsSVGTransformSMILType is to ensure that mType is only ever set in Init() and Destroy() and in those two methods we only set mType if we successfully allocate space for mU.mPtr. That way if mType is set to the transform type, we can assume that the pointer is not null.

Obviously it's not a problem here, I just mention it because there are a lot of checks for null pointers both in this class (e.g. ComputeDistance, ValueFromCSSDeclaration, ValueToString, IsInherit) and in others (nsSMILCSSValueListType etc.). We could drop them all if we assume that so long as mType matches our type, mU.mPtr is not null.

> +nsresult
> +nsSMILCSSRectType::Add(nsSMILAnimationContext& aAnimContext,
> +                       nsSMILValue& aDest, const nsSMILValue& aValueToAdd,
> +                       PRUint32 aCount) const
> +{
> +  NS_ABORT_IF_FALSE(aValueToAdd.mType == aDest.mType,
> +                    "Trying to add invalid types");
> +  NS_ABORT_IF_FALSE(aValueToAdd.mType == this, "Unexpected SMIL value type");
> +
> +  return NS_ERROR_NOT_IMPLEMENTED;

Are rects additive? The text for the 'clip' property doesn't explicitly say they're not (unlike, say, stroke-dasharray) but perhaps they come under the "All other data types used in animatable attributes and properties" category in SVG 1.1, 19.2.5? In which case they're not additive.

If they're not additive then I think we should return NS_ERROR_FAILURE here. Otherwise, if they are additive then NS_ERROR_NOT_IMPLEMENTED is fine until we implement it.

> +// static
> +PRBool
> +nsSMILCSSRectType::InterpolateSide(nsSMILAnimationContext& aAnimContext,
> ...
> +  // Assuming property ID is 'clip', since that's the only supported
> +  // rect-valued property in SVG.
> +  return nsSMILCSSUtils::InterpolateLength(aAnimContext, eCSSProperty_clip,
> +                                           startLengthVal, endLengthVal,
> +                                           aUnitDistance, aResult);

Is it at all possible to test this assumption with an assertion? I just wonder if this is likely to trip us up if a later version of SVG adds another rect property. Maybe it's not worth the extra plumbing though.

> diff --git a/content/smil/nsSMILCSSRectType.h b/content/smil/nsSMILCSSRectType.h
> ...
> +class nsSMILCSSRectType : public nsISMILCSSType
> +{

Does this class need a brief comment?

> diff --git a/content/smil/nsSMILCSSShorthandType.cpp b/content/smil/nsSMILCSSShorthandType.cpp
> +/**
> + * Helper Struct: ShorthandValue
> + * This struct is meant to be used as the target of nsSMILValue.mU.mPtr.
> + * It contains its own generic data pointer, as well as a CSS Property ID,
> + * which lets us know how to interpret the data pointer.
> + */
> +struct ShorthandValue {
> +  ShorthandValue() : mPropID(NO_PROPERTY), mData(nsnull) {}
> +
> +  nsCSSProperty mPropID;
> +  void*         mData;  // contents depends on the shorthand property
> +};

It seems like we have a lot of little objects being allocated at every sample. I'm still concerned about the cost of all this memory allocation. Is there any way we can coalesce some of these objects together here? It seems like we allocate an nsSMILValue object, which allocates storage for it's mU.mPtr member (a ShorthandValue object) which allocates storage for its mData member (which may be a SingleValueWrapper or FontWrapper), then they all get deleted at the end of each sample. Is that right?

> +nsresult
> +nsSMILCSSShorthandType::Init(nsSMILValue& aValue) const
> +{
> +  NS_ABORT_IF_FALSE(aValue.IsNull(),
> +                    "Unexpected value type");
> +  aValue.mType = this;
> +  aValue.mU.mPtr = new ShorthandValue();
> +  return NS_OK;
> +}

As before (with nsSMILCSSRectType), I think we need to check for OOM here.

(Also, as with nsSMILCSSRectType, if we ensure mType is only set when memory is successfully allocated for mU.mPtr we can drop a lot of null-pointer checks in other parts of the code.)

> +nsresult
> +nsSMILCSSShorthandType::Assign(nsSMILValue& aDest,
> +                               const nsSMILValue& aSrc) const
> ...
> +    case eCSSProperty_font: {
> +      FontWrapper* srcData = static_cast<FontWrapper*>(srcShorthandVal->mData);
> +      FontWrapper* destData = needToAlloc ? new FontWrapper() :
> +        static_cast<FontWrapper*>(destShorthandVal->mData);
> +      *destData = *srcData;

Should we check for OOM here?

> +      SingleValueWrapper* destData = needToAlloc ? new SingleValueWrapper() :
> +        static_cast<SingleValueWrapper*>(destShorthandVal->mData);
> +      *destData = *srcData;

And here?

> +nsresult
> +nsSMILCSSShorthandType::Add(nsSMILAnimationContext& aAnimContext,
> +                            nsSMILValue& aDest, const nsSMILValue& aValueToAdd,
> +                            PRUint32 aCount) const
> +{
> +  NS_ABORT_IF_FALSE(aValueToAdd.mType == aDest.mType,
> +                    "Trying to add invalid types");
> +  NS_ABORT_IF_FALSE(aValueToAdd.mType == this, "Unexpected source type");
> +
> +  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> +  return rv;
> +}

As with nsSMILCSSRectType, can I just check if nsSMILCSSShorthandType is, in fact, sometimes additive? For example, the 'font-size-adjust' component is not additive but the 'font-size' component is. So if you had an animation on the 'font' property where only the 'font-size' component changed (e.g. by: '10px serif') presumably that would support additive behaviour right?

I'm just checking my understanding. If that's right, then this is fine (and we need to implement it later). Otherwise, if we're saying that nsSMILCSSShorthandType is never additive then I think we should return NS_ERROR_FAILURE here.

> +nsresult
> +nsSMILCSSShorthandType::ComputeDistance(nsSMILAnimationContext& aAnimContext,
> +                                        const nsSMILValue& aFrom,
> +                                        const nsSMILValue& aTo,
> +                                        double& aDistance) const
> +{
> +  NS_ABORT_IF_FALSE(aFrom.mType == aTo.mType,
> +                    "Trying to compare different types");
> +  NS_ABORT_IF_FALSE(aFrom.mType == this, "Unexpected source type");
> +
> +  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> +  return rv;
> +}

As above, I presume this actually will be implemented at a later stage.

> +nsresult
> +nsSMILCSSShorthandType::Interpolate(nsSMILAnimationContext& aAnimContext,
> +                                    const nsSMILValue& aStartVal,
> +                                    const nsSMILValue& aEndVal,
> +                                    double aUnitDistance,
> +                                    nsSMILValue& aResult) const
> +{
> +  NS_ABORT_IF_FALSE(aStartVal.mType == aEndVal.mType,
> +                    "Trying to interpolate different types");
> +  NS_ABORT_IF_FALSE(aStartVal.mType == this,
> +                    "Unexpected types for interpolation");
> +  NS_ABORT_IF_FALSE(aResult.mType == this, "Unexpected result type");
> +  NS_ABORT_IF_FALSE(aUnitDistance >= 0.0 && aUnitDistance < 1.0,
> +                    "unit distance value out of bounds");

Just wondering is NS_PRECONDITION makes more sense for these sort of checks? From a documentation point of view at least. (After all, it's testing a precondition. And am I the only one who finds ABORT_IF_FALSE *really* confusing? :)

> +  // No interpolation for shorthand properties. Just set result = start val.
> +  Assign(aResult, aStartVal);
> +  return NS_OK;

As mentioned above, we should be able to interpolate if only the 'font-size' component changes right?

e.g. from: '10px serif', to: '20px serif' ?

If, on the other hand, we decide this property is not able to be interpolated I think we need to decide what the behaviour should be.

The documentation for nsISMILType::Interpolate in nsISMILType.h says that we should return NS_ERROR_FAILURE if the data type cannot be interpolated.

Either we update the documentation in nsISMILType.h and always just set result = start val as here. Or we return NS_ERROR_FAILURE and provide some decent handling in nsSMILAnimationFunction. Currently, I think if we were to return NS_ERROR_FAILURE then we'd get a whole lot of warnings if we ever tried to interpolate a property that can't be interpolated. The warnings are here:

  http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#286
  (see http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#441)

We haven't had to address this so far because everything we've been able to animate up until now has been able to be interpolated.

> +PRBool
> +nsSMILCSSShorthandType::ValueFromCSSDeclaration(nsCSSProperty aPropID,
> +                                                const nsCSSDeclaration& aDecl,
> +                                                nsSMILValue& aValue) const
> +{
> ...
> +    case eCSSProperty_font: {
> +      FontWrapper* fontWrapper = new FontWrapper();

As before, do we need to check for OOM here?

> +        SingleValueWrapper* data = new SingleValueWrapper();
> +        data->mCSSValue = xValue;

Ditto.

> +PRBool
> +nsSMILCSSShorthandType::ValueToString(nsCSSProperty aPropID,
> +                                      const nsSMILValue& aValue,
> +                                      nsAString& aString) const
> +{
> ...
> +      nsCSSDeclaration* declaration = new nsCSSDeclaration();
> +      declaration->CompressFrom(&block);

Ditto.

> +    case eCSSProperty_marker: {
> +      SingleValueWrapper* data =
> +        static_cast<SingleValueWrapper*>(shorthandVal->mData);
> +      if (data->mCSSValue.GetUnit() == eCSSUnit_Null) {
> +        // null unit means there's no value set --> nothing to do.
> +        success = PR_TRUE;
> +      } else {
> +        // Pretend value is for for non-shorthand version of property. This is
> +        // fine, becuase it has the same syntax as the shorthand property.
> +        success =
> +          nsCSSDeclaration::AppendCSSValueToString(eCSSProperty_marker_end,
> +                                                   data->mCSSValue, aString);
> +      }
> +    } break;
> +    case eCSSProperty_overflow: {
> +      SingleValueWrapper* data =
> +        static_cast<SingleValueWrapper*>(shorthandVal->mData);
> +      if (data->mCSSValue.GetUnit() == eCSSUnit_Null) {
> +        // null unit means there's no value set --> nothing to do.
> +        success = PR_TRUE;
> +      } else {
> +        // Pretend value is for for non-shorthand version of property. This is
> +        // fine, becuase it has the same syntax as the shorthand property.
> +        success =
> +          nsCSSDeclaration::AppendCSSValueToString(eCSSProperty_overflow_x,
> +                                                   data->mCSSValue, aString);
> +      }
> +    } break;

It seems like there's a fair bit of repeated code in these two cases which could be reused.

(Also, I'm curious about how overflow works. Do we need to add tests to make sure that CSS3 syntax for overflow (e.g. 'visible hidden') can't be used in SVG?)

> +void
> +nsSMILCSSShorthandType::StringFromCSSDeclaration(nsCSSProperty aPropID,
> +                                             nsCOMPtr<nsICSSDeclaration> aDecl,
> +                                             nsAString& aString) const
> +{
> +  // Not implementing.  This method is currently only used when setting up base
> +  // value of the property (with no animations).  By returning the empty string
> +  // here, we end up with a base shorthand value whose sub-values are all of
> +  // type eCSSUnit_Null, which is fine.  This would only cause a problem if we
> +  // were going to try to do addition or interpolation with these values, but
> +  // we won't, because shorthand properties don't support that.
> +  aString = EmptyString();
> +}

Assuming the verdict regarding the questions I raised above (about interpolating the 'font' property) is that, as this comment says, shorthand properties can't be interpolated, I think we should have a test case for this.

> diff --git a/content/smil/nsSMILCSSUtils.cpp b/content/smil/nsSMILCSSUtils.cpp
> +// static
> +nscoord
> +nsSMILCSSUtils::GetLengthFromCSSValue(nsSMILAnimationContext& aAnimContext,
> ...
> +  }
> +}
> +// static
> +nscolor
> +nsSMILCSSUtils::GetColorFromCSSValue(nsSMILAnimationContext& aAnimContext,

(nit) Is there a space missing between these two methods?

> diff --git a/content/smil/nsSMILCSSValueListType.cpp b/content/smil/nsSMILCSSValueListType.cpp
> ...
> +    if (aWrapper.mValueList) {
> +      delete aWrapper.mValueList;
> +      aWrapper.mValueList = nsnull;
> +    }

(nit) The null test isn't necessary right?

> +nsresult
> +nsSMILCSSValueListType::Add(nsSMILAnimationContext& aAnimContext,
> +                            nsSMILValue& aDest, const nsSMILValue& aValueToAdd,
> +                            PRUint32 aCount) const
> +{
> +  NS_ABORT_IF_FALSE(aValueToAdd.mType == aDest.mType,
> +                    "Trying to add invalid types");
> +  NS_ABORT_IF_FALSE(aValueToAdd.mType == this, "Unexpected source type");
> +
> +  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> +  return rv;
> +}

Is this the right error code? Lists are not additive (according to SVG 1.1, section 19.2.15 and notes for the dash-array property in 11.4) so should this return NS_ERROR_FAILURE instead?

> +nsresult
> +nsSMILCSSValueListType::ComputeDistance(nsSMILAnimationContext& aAnimContext,
> +                                        const nsSMILValue& aFrom,
> +                                        const nsSMILValue& aTo,
> +                                        double& aDistance) const
> +{
> +  NS_ABORT_IF_FALSE(aFrom.mType == aTo.mType,
> +                    "Trying to compare different types");
> +  NS_ABORT_IF_FALSE(aFrom.mType == this, "Unexpected source type");
> +
> +  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> +  return rv;
> +}

This return value, on the other hand, is correct because lists can be interpolated (according to SVG 1.1, section 19.2.15 and as noted in the XXXdholbert comment in nsSMILCSSValueListType::Interpolate). So will this be implemented in a separate bug?

> +PRBool
> +nsSMILCSSValueListType::ValueToString(nsCSSProperty aPropID,
> +                                      const nsSMILValue& aValue,
> +                                      nsAString& aString) const
> +{

(nit) For nsISMILCSSType::ValueToString, aString is presumably empty. Just wondering if we should (a) mention this in the documentation in nsISMILCSSType.h, (b) add an assertion to that effect here here, (c) explicitly clear the string here?

> diff --git a/content/smil/nsSMILCSSValuePairType.cpp b/content/smil/nsSMILCSSValuePairType.cpp

I wonder if we should document somewhere here what the meaning of mXValue and mYValue are for fills and strokes? Or refer to where it is documented?

> +nsresult
> +nsSMILCSSValuePairType::Add(nsSMILAnimationContext& aAnimContext,
> +                            nsSMILValue& aDest, const nsSMILValue& aValueToAdd,
> +                            PRUint32 aCount) const
> +{
> ...
> +  // NOTE: URL-based paints will fail the following HasNullOrColorUnit checks,
> +  // so they won't attempt any addition, which is good, because they're not
> +  // supposed to be additive.
> +  PRBool success = PR_FALSE;
> +  if (nsSMILCSSUtils::HasNullOrColorUnit(destPair->mXValue) &&
> +      nsSMILCSSUtils::HasNullOrColorUnit(toAddPair->mXValue)) {

I think we need to state here that 'currentColor' has already been converd to an RGB value. (That's right isn't it?) And perhaps also in ComputeDistance and Interpolate.

> diff --git a/content/smil/nsSMILCSSValueType.cpp b/content/smil/nsSMILCSSValueType.cpp
> +nsresult
> +nsSMILCSSValueType::Init(nsSMILValue& aValue) const
> +{
> +  NS_ABORT_IF_FALSE(aValue.IsNull(),
> +                    "Unexpected value type");
> +
> +  aValue.mU.mPtr = new ValueWrapper();
> +  if (!aValue.mU.mPtr) {
> +    // XXXdholbert Is this worth checking for, or are we basically dead already?
> +    return NS_ERROR_OUT_OF_MEMORY;

Whatever you decide, we ought to be consistent. In some of these we check, and in some we don't. I'm pretty sure that throughout the rest of the SMIL code we check for OOM.

> +nsresult
> +nsSMILCSSValueType::Add(nsSMILAnimationContext& aAnimContext,
> +                        nsSMILValue& aDest, const nsSMILValue& aValueToAdd,
> +                        PRUint32 aCount) const
> +{
> ...
> +  if (nsSMILCSSUtils::HasNullOrLengthUnit(destWrapper->mCSSValue) &&
> +      nsSMILCSSUtils::HasNullOrLengthUnit(valueToAddWrapper->mCSSValue)) {
> +    success = nsSMILCSSUtils::AddLength(aAnimContext,
> +                                        valueToAddWrapper->mPropID,
> +                                        destWrapper->mCSSValue,
> +                                        valueToAddWrapper->mCSSValue, aCount);
> +  } else if (nsSMILCSSUtils::HasNullOrThisUnit(destWrapper->mCSSValue, 
> +                                               eCSSUnit_Number) &&
> +             nsSMILCSSUtils::HasNullOrThisUnit(valueToAddWrapper->mCSSValue,
> +                                               eCSSUnit_Number)) {
> +    success = nsSMILCSSUtils::AddNumber(destWrapper->mCSSValue,
> +                                        valueToAddWrapper->mCSSValue, aCount);
> +  }

Does this need to support colors as well? <color> appears to be additive in SVG 1.1 and SVGT1.2. And we support color in ComputeDistance and Interpolate for this class.

Also, font-size-adjust is explicitly non-additive even though it's a length (http://www.w3.org/TR/SVG11/text.html#FontSizeAdjustProperty). Do we need special handling here to disallow adding font-size-adjust animations?

> +nsresult
> +nsSMILCSSValueType::Interpolate(nsSMILAnimationContext& aAnimContext,
> +                                const nsSMILValue& aStartVal,
> +                                const nsSMILValue& aEndVal,
> +                                double aUnitDistance,
> +                                nsSMILValue& aResult) const
> +{
> ...
> +    // Assume it's not interpolatable -- just copy the start value.
> +    // (Since aUnitDistance < 1.0, we shouldn't be at 'end' value yet.)
> +    resultWrapper->mCSSValue = startWrapper->mCSSValue;

See comment about nsSMILCSSShorthandType::Interpolate. Basically, the documentation for nsISMILType says we should return NS_ERROR_FAILURE if we can't interpolate and we need to decide if this is right.

> +PRBool
> +nsSMILCSSValueType::IsInherit(const nsSMILValue& aValue) const
> +{
> +  const ValueWrapper* wrapper = ExtractValueWrapper(aValue);
> +  return wrapper->mCSSValue.GetUnit() == eCSSUnit_Inherit;
> +}

In the other IsInherit methods we check if the pointer is null. Do we need to do that here as well?

> diff --git a/content/smil/nsSMILCompositor.cpp b/content/smil/nsSMILCompositor.cpp
> @@ -90,25 +92,29 @@ nsSMILCompositor::ComposeAttribute()
>  {
>    if (!mKey.mElement)
>      return;
>  
>    // FIRST: Get the nsISMILAttr (to grab base value from, and to eventually
>    // give animated value to)
>    nsAutoPtr<nsISMILAttr> smilAttr;
>    if (mKey.mIsCSS) {
> -    // XXX Look up style system for the CSS property. The set of CSS properties
> -    // should be the same for all elements so we don't need to query the element
> -    // itself.
> +    nsAutoString name;
> +    mKey.mAttributeName->ToString(name);
> +    nsCSSProperty propId = nsCSSProps::LookupProperty(name);
> +    if (propId != eCSSProperty_UNKNOWN) {
> +      smilAttr = new nsSMILCSSProperty(mKey.mAttributeName.get(),
> +                                       mKey.mElement.get());
> +    }
>    } else {
>      smilAttr = mKey.mElement->GetAnimatedAttr(mKey.mAttributeName);
>    }

I think I'm missing something here. The copy of nsSMILCompositor on trunk doesn't seem to have that line "if (mKey.mIsCSS) {" and I don't see it in the stylechanges or overridestyle patches either. Is there yet another patch that adds this or am I missing something?

Also, a lot hangs on the mIsCSS property being accurate. But I'm not sure that it is. See comment in nsSMILAnimationController.cpp:

  http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationController.cpp#480

Does the part of nsSMILAnimationController that determines if an "auto" animation attribute is XML or CSS need fixing? And can we do that now?
 
> diff --git a/content/smil/nsSMILParserUtils.cpp b/content/smil/nsSMILParserUtils.cpp
> ...
> +PRInt32
> +nsSMILParserUtils::CheckForNegativeNumber(const nsAString& aStr)
> +{
> ...
> +    // Check for numeric character
> +    if (start != end &&
> +        *start >= '0' && *start <= '9') {

Should we use NS_IS_DIGIT here?

> diff --git a/content/smil/test/db_smilCSSProps.js b/content/smil/test/db_smilCSSProps.js
> ...
> +            // NOTE: The urls in the following two lines need to be updated
> +            // if this test is ever moved or renamed.
> +            fromComp: "url(\"" + document.URL + "#clipA\")",
> +            toComp: "url(\"" + document.URL + "#clipB\")",

Is this comment still valid?

> +    "fill": {
> +        animatable: true,
> +        appliesTo: "rect",
> +        valueBundles: [
> +          { from: "rgb(100, 100, 100)",
> +            to: "rgb(200, 200, 200)",
> +            midComp: "rgb(150, 150, 150)"
> +          },
> ...

I'd really like to test 'currentColor' for fill and stroke.

> +    "fill-opacity": {
> +        animatable: true,
> +        appliesTo: "rect",
> +        valueBundles: [
> +          { from: "1",
> +            to: "0",
> +            midComp: "0.5"
> +          },
> ...

Is this a good place to test some out-of-range values and check clamping works correctly? Or do we have to wait for bug 501188 for that?

> +    "filter": {
> +        animatable: true,
> +        appliesTo: "rect",
> +        valueBundles: [
> +          { from: "url(#filterA)",
> +            to: "url(#filterB)",
> +            // NOTE: The urls in the following two lines need to be updated
> +            // if this test is ever moved or renamed.
> +            fromComp: "url(\"" + document.URL + "#filterA\")",
> +            toComp: "url(\"" + document.URL + "#filterB\")",

As before, is this comment still valid?

> +    "font": {
> +        // NOTE: line-height is hard-wired at 10px in test_smilCSSProps.xhtml
> +        // because if it's not explicitly set, its value varies across platforms
> +        //  XXXdholbert Test system font values?
> +        //  ('caption', 'icon', 'menu', etc.)
> +        animatable: true,
> +        appliesTo: "text",
> +        valueBundles: [
> +          { from: "10px serif",
> +            to: "30px sans-serif",
> +            fromComp: "normal normal 400 10px / 10px serif",
> +            toComp: "normal normal 400 30px / 10px sans-serif",
> +          },
> ...

As mentioned above, is it reasonable to interpolate the following:

  from: "10px serif"
  to: "20px serif"

?

So that midComp gives us "15px serif"?

I haven't tested what Opera and WebKit do here but it seems reasonable to me.

> +    "font-size-adjust": {
> +        animatable: true,
> +        appliesTo: "text",
> +        valueBundles: [
> +          { from: "0.9",
> +            to: "0.1",
> +            midComp: "0.5"
> +          },
> +          { from: "0.5",
> +            to: "0.6",
> +            midComp: "0.55"
> +          },
> +          { from: "none",
> +            to: "0.4"
> +          }
> +        ]
> +     },

As mentioned earlier as well, we need to test that font-size-adjust is not additive.

> +    /* 'glyph-orientation-horizontal' property -- not supported in Mozilla */
> +    /* 'glyph-orientation-vertical' property -- not supported in Mozilla */

Seeing as these properties aren't animatable anyway, should we just put in a test case to ensure that they don't later become animatable accidentally?

> diff --git a/content/smil/test/db_smilCSSPropsPaced.js b/content/smil/test/db_smilCSSPropsPaced.js
> ...
> +// TODO: test more properties here.
> +var gSVGCSSPropertiesPaced = {
> +    "fill": {
> +        appliesTo: "rect",
> +        valueBundles: [
> +          { values:  "rgb(2,  4,  6); " +
> +                     "rgb(4,  8, 12); " + 
> +                     "rgb(8, 16, 24)",
> +            comp0:   "rgb(2, 4, 6)",
> +            comp1_6: "rgb(3, 6, 9)",
> +            comp1_3: "rgb(4, 8, 12)",
> +            comp2_3: "rgb(6, 12, 18)",
> +            comp1:   "rgb(8, 16, 24)"
> +          },
> ...

Can we test 'currentColor' here too?

> +    "stroke-dashoffset": {

Do we need a TODO here to remind us to test stroke-dasharray when that's implemented later?

> diff --git a/content/smil/test/test_smilCSSProps.xhtml b/content/smil/test/test_smilCSSProps.xhtml
> ...
> +// List of known-broken properties listed in property database, with a reason
> +// for brokenness of each.  This makes us skip all of its tests and make a
> +// "todo()" call insteaad.
> +var gKnownBrokenProperties = {
> +  // e.g. "some-broken-property" : "reason why some-broken-property is broken",
> +}

Seeing as we have this neat system for broken properties, could we write the tests for, e.g. stroke-dasharray, and let them show up as todo's instead?

> +  // CLEAN UP
> +  if (isFreeze) {
> +    // XXXdholbert This clause is a hacky workaround for bug 487450 -- if we
> +    // don't force-clear animated style like this, it'll stick around
> +    // even after the <animate> node is removed.

Apparently this bug is resolved so can we update this now?

> diff --git a/layout/reftests/svg/smil/anim-fillopacity-1css.svg b/layout/reftests/svg/smil/anim-fillopacity-1css.svg
> ...
> diff --git a/layout/reftests/svg/smil/anim-fillopacity-1none.svg b/layout/reftests/svg/smil/anim-fillopacity-1none.svg
> ...
> diff --git a/layout/reftests/svg/smil/anim-fillopacity-1xml.svg b/layout/reftests/svg/smil/anim-fillopacity-1xml.svg

Is it worth combining these three tests into one? For example, you could have three additive <animate> elements targetting the same rect with a to-value of 0.3, and attributeTypes of 'CSS', 'XML', and 'auto'. Then the reference image would ensure the result was 0.6 rather than 0.9. This approach would ensure that our keys into the compositor table line up despite the differences in the attributeType. (It's more of a black box test and would help test that the mIsCSS of the compositor table key is set correctly.)

I don't know if we need to be concerned about the cost of adding reftests in terms of execution time, but this might ease the maintenance burden.

> diff --git a/layout/reftests/svg/smil/anim-strokewidth-1css.svg b/layout/reftests/svg/smil/anim-strokewidth-1css.svg
> ...
> diff --git a/layout/reftests/svg/smil/anim-strokewidth-1none.svg b/layout/reftests/svg/smil/anim-strokewidth-1none.svg
> ...
> diff --git a/layout/reftests/svg/smil/anim-strokewidth-1xml.svg b/layout/reftests/svg/smil/anim-strokewidth-1xml.svg

Do we need to test the attributeType lookup twice like this? I thought one stroke-width test would be enough here?

> diff --git a/layout/reftests/svg/smil/reftest.list b/layout/reftests/svg/smil/reftest.list
> ... 
>  fails == anim-fillcolor-1.svg      anim-standard-ref.svg # color support
> -fails == anim-fillopacity-1.svg anim-standard-ref.svg    # non-length-numeric values support
> +== anim-fillopacity-1none.svg      anim-standard-ref.svg
> +== anim-fillopacity-1css.svg       anim-standard-ref.svg
> +fails == anim-fillopacity-1xml.svg  anim-standard-ref.svg  # non-length-numeric values support

I don't understand the comment here: "non-length-numeric values support." Maybe it should be moved up two lines because it governs the three following tests right?

> +== anim-strokewidth-1css.svg       anim-standard-ref.svg
> +== anim-strokewidth-1none.svg      anim-standard-ref.svg
> +fails == anim-strokewidth-1xml.svg anim-standard-ref.svg # non-length-numeric values support

Ditto.

> diff --git a/layout/reftests/svg/smil/style/anim-css-fill-1-from-to-ident.svg b/layout/reftests/svg/smil/style/anim-css-fill-1-from-to-ident.svg
> ...
> +  <style>
> +    rect {
> +      fill: yellow;
> +      stroke: black;
> +      stroke-width: 1pt;
> +    }
> +  </style>
> +  <script xlink:href="../smil-util.js" type="text/javascript"/>
> +  <rect x="20pt" y="20pt" width="15pt" height="15pt">
> +    <animate attributeName="fill" attributeType="CSS"
> +             from="yellow" to="blue" begin="4.0s" dur="2s" fill="freeze"/>
> +  </rect>
> +  <rect x="70pt" y="20pt" width="15pt" height="15pt">
> +    <animate attributeName="fill" attributeType="CSS"
> +             from="yellow" to="blue" begin="3.0s" dur="2s" fill="freeze"/>
> +  </rect>

Might it make more sense here to make the 'from' value different to the style applied to the rect so we can differentiate between the color applied by the animation and the unanimated color?

> diff --git a/layout/reftests/svg/smil/style/anim-css-fill-2-from-by-ident.svg b/layout/reftests/svg/smil/style/anim-css-fill-2-from-by-ident.svg
> ...
> +  <!-- This is a series of animations from Indigo (#4B0082)  -->
> +  <!-- to WhiteSmoke (#F5F5F5), frozen at various points.    -->
> ...
> +  <rect x="20pt" y="20pt" width="15pt" height="15pt">
> +    <animate attributeName="fill" attributeType="CSS"
> +             from="indigo" by="#AAF573" begin="4.0s" dur="2s" fill="freeze"/>
> +  </rect>

Is it worth testing a by-value that takes a named color? To check that named colors are additive?

Once again, this would be a good place to test currentColor. (Both (i) as a to-value or by-value but also (ii) by animating the 'color' property on a parent group. Or even, (iii) both. Although I suppose (iii) depends on what we decide to do about incorporating base values from ancestors as per Daniel's query: http://lists.w3.org/Archives/Public/www-svg/2009Jul/0037.html)

> diff --git a/layout/reftests/svg/smil/style/anim-css-font-1.svg b/layout/reftests/svg/smil/style/anim-css-font-1.svg

In this test we cover:
* inherit -> caption
* caption -> inherit
* caption -> icon
* caption -> menu
* caption -> message-box
* caption -> small-caption

I wonder if it's enough to test just the first three? If "caption -> icon" works, won't "caption -> menu"? As far as the code that's under test is concerned there's no difference right? I suppose I just wonder if the tests could be a little more targetted? For the sake of making it easier to maintain as much as anything.

The same goes for all the font-size tests. I wonder if we could combine the various permutations into one or two more comprehensive tests?

And then for the stroke-width tests, I wonder how much new code is covered by these tests? Aren't the code paths quite similar? Wouldn't it be enough to create one test that includes all the different types of units and animation (i.e. from-by, to, etc.) and just takes a snapshot mid-way through the animation?

There are also other cases not covered here, such as the various situations when one animation is targetting an ancestor of another animation. I guess it's still unclear what should happen in such situations so perhaps that's why we haven't tested it yet.


That's all from me. They're mostly just questions and curiosities. Overall this is really great. Thanks Daniel!
(Assignee)

Comment 68

8 years ago
 (In reply to comment #67)
Thanks for the review comments, Brian!  First chunk of responses below, more coming soon.

> I think roc already mentioned this, but I guess a fair bit of this CSS-specific
> code will be moved out of the SMIL code in bug 504652?

Correct.

> > +  virtual PRBool ValueFromCSSDeclaration(nsCSSProperty aPropID,
> Do we need to document what the return type means here? (Likewise for
> ValueToString and IsInherit)

Yeah -- fixed.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/d9a33969448d

> > +// nsSMILAnimationContext
> 
> I'm sure you've thought of this, but I'm just wondering if this class is
> necessary.
> 
> I can see you need it for converting between different unit types but I think
> the original idea was that nsISMILAttr::GetBaseValue would return an
> nsSMILValue with sufficient source context for (the appropriate subclass of)
> nsISMILType to do correct interpolation/addition etc.
[snip]
> would bundle [...] whatever is needed for
> interpolation/addition/etc. into the nsSMILValue. That way you don't need to
> pass around animation context objects.

Yeah, I initially thought about bundling the "context" into the nsSMILValue -- the problem there is it causes a lot of redundant copies of the animation context. We'd be bundling a copy with every single CSS-related nsSMILValue, and in the interpolation/addition/distance functions, we'd be effectively passing in *two* copies of the exact same context (one with each value), when we only need one.  It's one more thing we need to copy / generate on every nsSMILValue that we populate/assign.  So, it seems a lot of redundancy, I guess.

Having said that, I agree that the addition of the nsSMILAnimationContext structure is perhaps a bit messy, and it did require some perhaps-undesirable tweaks to our internal SMIL interfaces.  So, perhaps it would still be cleaner to bundle the animation context with every nsSMILValue, despite the redundancy.  I need to think about this a bit.

> > +  nsSVGSVGElement* GetSVGContext()   { return mSVGContext;   }
> 
> As above, I wonder if we can somehow remove the dependency on SVG.

Well, we definitely need direct access to the <SVG> element *somewhere*, so we can use its size to convert percentage lengths into absolute lengths.  Maybe that can be abstracted away, though, depending on potential reworking of animation contexts (per my previous comment-response), as well as on the work in bug 504652.

> > +  // XXXdholbert removing const-ness here -- maybe aSMILAttr shouldn't be const?
> > +  nsIContent* targetElem =
> > +    const_cast<nsISMILAttr*>(&aSMILAttr)->GetTargetElement();
> 
> Could we just do a const overload on GetTargetElement() that returns a const
> nsIContent? Or does that involve making nsIContent const-correct as well?

Sadly, we need a NON-const nsIContent here, in order to get its StyleContext & SVG Context. (specifically, the functions "GetPrimaryFrameFor" and "GetOwnerSVGElement" take a non-const nsIContent)

However, if the AnimationContext moves inside the nsSMILValue somehow, as you suggested, this messiness would go away...

> where the animation targets a CSS property that is not animatable (or not
> supported).
>   * nsSMILCSSProperty::ValueFromCSSDeclaration gives a warning ("Trying to
> extract CSS Value from a non-animatable (or unsupported) property")
[snip]
> will the console clog up with these
> messages?

Agreed, this is annoying/bad.  I think this warning would be more useful if we could just print it once per pageload, but I don't think we can easily do that, and it's kind of evil to have it spam forever on stdout.  I tweaked the error-handling behavior to avoid this problem:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/81c5f010b829
Now we'll just silently fail if we're asked to animate an explicitly un-animatable property.

> > +class nsSMILCSSProperty : public nsISMILAttr
> 
> I think this class needs a comment explaining what it does.

Fixed.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ceab56065f6c

> > +nsresult
> > +nsSMILCSSRectType::Init(nsSMILValue& aValue) const
> 
> I think we need to check for OOM here (like we do for
> nsSMILCSSValuePairType::Init).

Good catch. Fixed as part of:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1e9c3cfd5fcc
(Assignee)

Comment 69

8 years ago
(In reply to comment #67) -- second chunk of responses:
> The second check is not strictly necessary--I guess it's just a consistency
> check but on that matter see the next comment.
[snip]
> > +  NS_ABORT_IF_FALSE(aDest.mU.mPtr,
> > +                    "nsSMILValue for CSS val should have non-null pointer");
> > +  NS_ABORT_IF_FALSE(aSrc.mU.mPtr,
> > +                    "nsSMILValue for CSS val should have non-null pointer");
> 
> The last two checks here are fine, but I think what we did with
> nsSVGTransformSMILType is to ensure that mType is only ever set in Init() and
> Destroy() and in those two methods we only set mType if we successfully
> allocate space for mU.mPtr.

BTW, I believe that invariant is true in this patch, too -- i.e. we only set mType if we've got space allocated for mU.mPtr -- aside from the OOM issues that you pointed out in Init() functions, which are now fixed in my patch queue.

> there are a lot of
> checks for null pointers
[snip]
> We could drop them all if we assume that so long
> as mType matches our type, mU.mPtr is not null.

I think it's still useful to assert non-nullness here.  Beyond being just a "this invariant should still hold" statement, it's also helpful because nsSMILValue's fields are all public, and so they could really be modified by *any* code that sees the nsSMILValue.  So even if Init() and Destroy() are sane, a mistake somewhere else could still break this, and the assertion is good for catching that sort of issue.  (Side point -- maybe we should change things so that nsSMILValue's fields are private, with nsISMILType being a friend class?  We could use a const accessor to look up the mType, in places where we need it.  With this sort of change, we could be significantly more sure that nothing can mess with mType & mU.mPtr.)

> > +nsSMILCSSRectType::Add(nsSMILAnimationContext& aAnimContext,
> > +                       nsSMILValue& aDest, const nsSMILValue& aValueToAdd,
> > +                       PRUint32 aCount) const
[snip]
> > +  return NS_ERROR_NOT_IMPLEMENTED;
> 
> Are rects additive? The text for the 'clip' property doesn't explicitly say
> they're not (unlike, say, stroke-dasharray) but perhaps they come under the
> "All other data types used in animatable attributes and properties" category in
> SVG 1.1, 19.2.5? In which case they're not additive.

I had been assuming that they are supposed to be additive, since addition seems like a sensible operation on them.
But you're correct -- according to the letter of the spec, they shouldn't be additive, since <shape> isn't mentioned in the table in 19.2.15.

I just tried to test Opera's implementation, to see whether they consider it additive, and it looks like they don't support animating "clip" at all (that is, animations don't affect clip's computed style in Opera).

Anyway, I think it's fine to assume this isn't supposed to be additive. (particularly because Mozilla doesn't even appear to use this property in SVG, AFAICT. :))

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/b3b74c9748a3
And added test:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/60765c5422fc

> > +  // Assuming property ID is 'clip', since that's the only supported
> > +  // rect-valued property in SVG.
>
> Is it at all possible to test this assumption with an assertion?

That comment will soon become obsolete, actually.  It relates to the different semantics of rect() values in CSS 2.1 vs SVG 1.1/CSS2.0, and roc says in comment 62 that we should just always use the CSS2.1 semantics. (which I'll probably implement in bug 504652)

> > +class nsSMILCSSRectType : public nsISMILCSSType
> Does this class need a brief comment?

Yeah, it looks like all of the nsSMILCSSxxxType classes do.  Not really worth adding documentation for these right now, though, since they'll be reworked/replaced in bug 504652, but I've added a XXXdholbert reminder above each one to document it eventually. :)
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/96d793bc7918

> It seems like we have a lot of little objects being allocated at every sample.
> I'm still concerned about the cost of all this memory allocation. Is there any
> way we can coalesce some of these objects together here? It seems like we
> allocate an nsSMILValue object, which allocates storage for it's mU.mPtr member
> (a ShorthandValue object) which allocates storage for its mData member (which
> may be a SingleValueWrapper or FontWrapper), then they all get deleted at the
> end of each sample. Is that right?

Yeah, that's correct.  It is indeed a lot of allocations.  Perhaps we could work around it by having ShorthandValue be an abstract class with one field -- mPropID -- and then SingleValueWrapper / FontWrapper could be subsumed in subclasses of Shorthand Value...  That's the only feasible savings I can see.

I'm leaving this as a TODO/to-investigate, since this is in code that will be reworked in bug 504652.  Added a comment to this effect:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/84c1712ec9d6

> > +nsresult
> > +nsSMILCSSShorthandType::Init(nsSMILValue& aValue) const
[snip]
> > +  aValue.mU.mPtr = new ShorthandValue();
> 
> As before (with nsSMILCSSRectType), I think we need to check for OOM here.

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/22939b70ab1c

> > +nsSMILCSSShorthandType::Assign(nsSMILValue& aDest,
[snip]
> > +      FontWrapper* destData = needToAlloc ? new FontWrapper() :
> 
> Should we check for OOM here?

For this & the other OOM cases you found outside of Init() functions, I'm just marking them TODO, since they're in code that will probably be reworked/replaced in bug 504652.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/b18cc32b97c4

> > +nsresult
> > +nsSMILCSSShorthandType::Add(nsSMILAnimationContext& aAnimContext,
> As with nsSMILCSSRectType, can I just check if nsSMILCSSShorthandType is, in
> fact, sometimes additive?
[snip]
> if we're saying that
> nsSMILCSSShorthandType is never additive then I think we should return
> NS_ERROR_FAILURE here.

The nsSMILCSSShorthandType class is never additive. It represents "font", "property", and "overflow" -- the SVG Property Index says that 'font' is animatable but non-additive, and the other two have non-numeric values, so they're not additive either.

Fixed so that Add returns NS_ERROR_FAILURE (in an inline function in the .h file):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1e9c3cfd5fcc

> > +nsSMILCSSShorthandType::ComputeDistance(nsSMILAnimationContext& aAnimContext,
[snip]
> > +  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> 
> As above, I presume this actually will be implemented at a later stage.

Actually, despite the misleading value of rv in the code quoted above, I've been assuming that "font" is *not* interpolatable (and hence ComputeDistance & Interpolate should actually return NS_ERROR_FAILURE).

You're right that one *could* sensibly interpolate between e.g. "10px serif" to "20px serif".  But this seems a bit hacky to me, and I don't think the spec calls for this.  As Cameron McCormack said in a reply to me on www-svg, the SVG spec is a bit blurry on what "additive" means -- his interpretation is that it sometimes includes "can be interpolated". I think this may be one of those cases -- I think that when the spec says that "font" is non-additive, it probably means "non-interpolatable" as well.
Cameron's email: http://lists.w3.org/Archives/Public/www-svg/2009Jul/0051.html

I've posted a testcase for this here:
http://people.mozilla.org/~dholbert/tests/474049/font-interpolate-1.svg
to test Opera & WebKit behavior.  Neither of them do smooth interpolation.  Instead, they both sort of crap out on it. :) (Opera 10b2 ignores the animation change the style at all, and WebKit r46507 jumps to the "from" value and freezes there)

Anyway -- for now, I think I'm assuming "font" is non-interpolatable.  May be worth an email to www-svg, though.
Fixed to use NS_ERROR_FAILURE here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1e9c3cfd5fcc

> > +  NS_ABORT_IF_FALSE(aUnitDistance >= 0.0 && aUnitDistance < 1.0,
> > +                    "unit distance value out of bounds");
> 
> Just wondering is NS_PRECONDITION makes more sense for these sort of checks?
> From a documentation point of view at least. (After all, it's testing a
> precondition. And am I the only one who finds ABORT_IF_FALSE *really*
> confusing? :)

Under the hood, NS_PRECONDITION behaves exactly the same as NS_ASSERTION, and NS_ABORT_IF_FALSE is a better NS_ASSERTION. :) If any of the asserted conditions end up failing, NS_ABORT_IF_FALSE will make sure they get noticed right away, while NS_PRECONDITION/ASSERTION would just print stuff to the STDOUT and potentially go unnoticed. (which is how we've accumulated so many known-failing assertions in our codebase)

See e.g. http://whereswalden.com/2009/01/04/a-reminder-ns_abort_if_false-is-the-new-ns_assertion/

> The documentation for nsISMILType::Interpolate in nsISMILType.h says that we
> should return NS_ERROR_FAILURE if the data type cannot be interpolated.
[snip]
> Either we update the documentation in nsISMILType.h
> [...] or we return NS_ERROR_FAILURE and provide some decent
> handling in nsSMILAnimationFunction.

Ah, very good point!  I like the "return NS_ERROR_FAILURE" strategy -- our fallback behavior can just always do an assignment at that point.

Fixed to use this strategy (with handling in nsSMILAnimationFunction) here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1e9c3cfd5fcc

> > +    case eCSSProperty_marker: {
> > +      SingleValueWrapper* data =
> > +        static_cast<SingleValueWrapper*>(shorthandVal->mData);
[snip]
> > +    case eCSSProperty_overflow: {
> > +      SingleValueWrapper* data =
> > +        static_cast<SingleValueWrapper*>(shorthandVal->mData);
> It seems like there's a fair bit of repeated code in these two cases which
> could be reused.

Good point -- fixed: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/796a80fe0854

> (Also, I'm curious about how overflow works. Do we need to add tests to make
> sure that CSS3 syntax for overflow (e.g. 'visible hidden') can't be used in
> SVG?)

I just tried a brief HTML testcase with <div style="overflow: scroll auto">, and AFAICT, we don't support this syntax yet at all.  If/when we add support for it, I don't see why we'd want to exclude SVG.

> > +void
> > +nsSMILCSSShorthandType::StringFromCSSDeclaration(nsCSSProperty aPropID,
[snip]
> > +  // type eCSSUnit_Null, which is fine.  This would only cause a problem if we
> > +  // were going to try to do addition or interpolation with these values, but
> > +  // we won't, because shorthand properties don't support that.
> 
> Assuming the verdict regarding the questions I raised above (about
> interpolating the 'font' property) is that, as this comment says, shorthand
> properties can't be interpolated, I think we should have a test case for this.

I added a chunk of mochitest for this in changeset 1e9c3cfd5fcc, already linked above. (The added chunk asserts that 'font' isn't interpolatable, even when only the font-size component changes.)

Side note: I discovered a related issue while looking into this -- if a property doesn't support interpolation, we're actually supposed to *force* animation with calcMode="discrete", regardless of the provided value of "calcMode".  This is specified in the first paragraph about of "calcMode" in SVG 1.1 sec 19.2.7.
I've fixed that in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1e9c3cfd5fcc

> > +}
> > +// static
> > +nscolor
> > +nsSMILCSSUtils::GetColorFromCSSValue(nsSMILAnimationContext& aAnimContext,
> 
> (nit) Is there a space missing between these two methods?

Yeah -- fixed in patch-queue.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/272eb5b23ea6

> > +    if (aWrapper.mValueList) {
> > +      delete aWrapper.mValueList;
> > +      aWrapper.mValueList = nsnull;
> > +    }
> 
> (nit) The null test isn't necessary right?

You're right -- fixed in patch-queue.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/272eb5b23ea6

> > +nsSMILCSSValueListType::Add(nsSMILAnimationContext& aAnimContext,
[snip]
> > +  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> > +  return rv;
> > +}
> 
> Is this the right error code? Lists are not additive (according to SVG 1.1,
> section 19.2.15 and notes for the dash-array property in 11.4) so should this
> return NS_ERROR_FAILURE instead?

You're right -- fixed in patch-queue.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/719bd73cab3a

> > +nsSMILCSSValueListType::ComputeDistance(nsSMILAnimationContext& aAnimContext,
[snip]
> > +  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
[snip] 
> will this be
> implemented in a separate bug?

Yeah -- this might be implemented in bug 504652, or as an offshoot/followup on that bug.
I also clarified this code slightly in the last checkin linked above.

> (nit) For nsISMILCSSType::ValueToString, aString is presumably empty. Just
> wondering if we should (a) mention this in the documentation in
> nsISMILCSSType.h, (b) add an assertion to that effect here here, (c) explicitly
> clear the string here?

Hmm -- good point -- I've now done (a): update documentation, and (b): add NS_ABORT_IF_FALSE in all implementations.  Not doing (c) for now, because it adds run-time overhead and because there's just one consumer of this method anyway, who calls it correctly, with an empty outparam.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/cf467fe5533e

> I wonder if we should document somewhere here what the meaning of mXValue and
> mYValue are for fills and strokes? Or refer to where it is documented?

I don't think it's documented anywhere.  That documentation probably doesn't belong in SMIL code, though -- I imagine it belongs near their declaration here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSStruct.h?mark=678,694#669
or in "ParsePaint" in nsCSSParser (which I've refered to in order to understand the meaning of mXValue and mYValue).  I'm not adding it right now, because we may end up using a different container for these properties after bug 504652, anyway.

> > +nsSMILCSSValuePairType::Add(nsSMILAnimationContext& aAnimContext,
[snip]
> I think we need to state here that 'currentColor' has already been converd to
> an RGB value. (That's right isn't it?) And perhaps also in ComputeDistance and
> Interpolate.

Actually, currentColor *hasn't* already been converted away yet -- and despite the ABORT_IF_FALSE that I had nsSMILCSSUtils::GetColorFromCSSValue, we can actually handle it just fine :), since we've got the style context.

Fixed to remove that abort, & added tests:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/10b7574d7615

> > +  aValue.mU.mPtr = new ValueWrapper();
> > +  if (!aValue.mU.mPtr) {
> > +    // XXXdholbert Is this worth checking for, or are we basically dead already?
> > +    return NS_ERROR_OUT_OF_MEMORY;
> 
> Whatever you decide, we ought to be consistent.

Ok -- removed the XXXdholbert comment there.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/815d93a030d5

> > +nsresult
> > +nsSMILCSSValueType::Add(nsSMILAnimationContext& aAnimContext,
> Does this need to support colors as well? <color> appears to be additive in SVG
> 1.1 and SVGT1.2. And we support color in ComputeDistance and Interpolate for
> this class.
> Also, font-size-adjust is explicitly non-additive even though it's a length

ooh, good catches! Fixed both of those in this changeset:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bf698805ebc1

and added a mochitest (test_smilCSSPropsAdditive.xhtml & db_smilCSSPropsAdditive.js) which asserts that font-size-adjust is non-additive:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bd5d978a12a4

> > +nsresult
> > +nsSMILCSSValueType::Interpolate(nsSMILAnimationContext& aAnimContext,
>
> See comment about nsSMILCSSShorthandType::Interpolate. Basically, the
> documentation for nsISMILType says we should return NS_ERROR_FAILURE

Fixed in the changeset listed under your related comment above (1e9c3cfd5fcc).

> > +nsSMILCSSValueType::IsInherit(const nsSMILValue& aValue) const
> 
> In the other IsInherit methods we check if the pointer is null. Do we need to
> do that here as well?

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/b97273226edd

> > @@ -90,25 +92,29 @@ nsSMILCompositor::ComposeAttribute()
> >    if (mKey.mIsCSS) {
> 
> I think I'm missing something here. The copy of nsSMILCompositor on trunk
> doesn't seem to have that line "if (mKey.mIsCSS) {" and I don't see it in the
> stylechanges or overridestyle patches either. Is there yet another patch that
> adds this or am I missing something?

Yeah -- bug 487450 rearranged this code on trunk, after I posted patch v4c here

> Also, a lot hangs on the mIsCSS property being accurate. But I'm not sure that
> it is. See comment in nsSMILAnimationController.cpp:
> http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationController.cpp#480
> Does the part of nsSMILAnimationController that determines if an "auto"
> animation attribute is XML or CSS need fixing? And can we do that now?

Ah, right -- the comment you linked to is "// This doesn't really work for CSS properties that aren't mapped attributes".  While that's true, the only affected properties seem to be: color-interpolation-filters, flood-color, flood-opacity, and font. (I determined this by clearing the attributeType="CSS" setting in my mega-mochitest, and seeing what failed)

My first inclination was to just replace the "IsAttributeMapped" check with a call to "nsCSSProps::LookupProperty()", to test whether the attributeName matches a CSS property name.  But that won't work for e.g. "width", which *is* a CSS property name (and would pass the LookupProperty call), even though it's not a valid CSS property *in SVG*.

Not sure what the correct solution here is at the moment.

> > +    // Check for numeric character
> Should we use NS_IS_DIGIT here?

Yeah -- I forgot about that method.  Thanks!  Fixed in patch queue.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/272eb5b23ea6

> > +            // NOTE: The urls in the following two lines need to be updated
> > +            // if this test is ever moved or renamed.
> > +            fromComp: "url(\"" + document.URL + "#clipA\")",
> Is this comment still valid?

It's obsolete -- thanks for catching that!  Fixed in patch queue. (also fixed the other instance of this that you caught)
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/272eb5b23ea6

> > +    "fill": {
> 
> I'd really like to test 'currentColor' for fill and stroke.

Ok, tests added:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/5a232115476b

> > +    "fill-opacity": {
> Is this a good place to test some out-of-range values and check clamping works
> correctly? Or do we have to wait for bug 501188 for that?

Good idea.  Tests added for this -- they pass already, only because the CSS parser doesn't happen to be super-strict with opacity values -- it doesn't clamp them to be less than 1.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/8bef29ffface

> As mentioned above, is it reasonable to interpolate the following:
> 
>   from: "10px serif"
>   to: "20px serif"

I don't think this should be interpolated -- see response to your related comment about "font" above.

> As mentioned earlier as well, we need to test that font-size-adjust is not
> additive.

Test added in changeset bd5d978a12a4, linked above.

> > +    /* 'glyph-orientation-horizontal' property -- not supported in Mozilla */
> > +    /* 'glyph-orientation-vertical' property -- not supported in Mozilla */
> 
> Seeing as these properties aren't animatable anyway, should we just put in a
> test case to ensure that they don't later become animatable accidentally?

Ok -- tests added:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/76f95fe7a4f8

> > +var gSVGCSSPropertiesPaced = {
> > +    "fill": {
> 
> Can we test 'currentColor' here too?

Yup.  Added:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/a9fa2d97aa7d

> Do we need a TODO here to remind us to test stroke-dasharray when that's
> implemented later?

Added:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/0024f46b31a3

> > diff --git a/content/smil/test/test_smilCSSProps.xhtml b/content/smil/test/test_smilCSSProps.xhtml
> > +var gKnownBrokenProperties = {
> > +  // e.g. "some-broken-property" : "reason why some-broken-property is broken",
> > +}
> 
> Seeing as we have this neat system for broken properties, could we write the
> tests for, e.g. stroke-dasharray, and let them show up as todo's instead?

I've done this in the changeset linked above, in the paced-mode mochitest.

I'm not doing it in test_smilCSSProps.xhtml, though, because that one actually has some useful stroke-dasharray tests (just with no interpolation), and if I were to flag stroke-dasharray as "broken" there, we'd completely skip those tests.  And that'd be bad, because they're the only tests covering CSS List functionality at this point. :)

> > +  // CLEAN UP
> > +  if (isFreeze) {
> > +    // XXXdholbert This clause is a hacky workaround for bug 487450
> 
> Apparently this bug is resolved so can we update this now?

Yeah -- I actually fixed this in my patch-queue when I landed that bug's patch:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/c3bd6c715731

> > diff --git a/layout/reftests/svg/smil/anim-fillopacity-1css.svg b/layout/reftests/svg/smil/anim-fillopacity-1css.svg
> > ...
> > diff --git a/layout/reftests/svg/smil/anim-fillopacity-1none.svg b/layout/reftests/svg/smil/anim-fillopacity-1none.svg
> > ...
> > diff --git a/layout/reftests/svg/smil/anim-fillopacity-1xml.svg b/layout/reftests/svg/smil/anim-fillopacity-1xml.svg
> 
> Is it worth combining these three tests into one? For example, you could have
> three additive <animate> elements targetting the same rect with a to-value of
> 0.3, and attributeTypes of 'CSS', 'XML', and 'auto'. Then the reference image
> would ensure the result was 0.6 rather than 0.9. This approach would ensure
> that our keys into the compositor table line up despite the differences in the
> attributeType. (It's more of a black box test and would help test that the
> mIsCSS of the compositor table key is set correctly.)
> 
> I don't know if we need to be concerned about the cost of adding reftests in
> terms of execution time, but this might ease the maintenance burden.

The stacked CSS + XML animations would perhaps make a useful additional test (particularly once we support animating the non-CSS version of fill-opacity and would be able to pass such a test).  But in general, I think it's good to keep unit tests as granular as possible, so I'd rather not replace these existing distinct tests with a single combined one.  Currently, the separated tests provide a lot of information -- they assert that we do the right thing for attributeType="CSS" and "auto", and that we fail for attributeType="XML".  If we combined them into one, the combined test would have to be marked "fails", and we'd no longer be asserting that "CSS" and "auto" currently work.  And if something breaks down the road (after we support attributeType="XML") and the combined test started failing, it'd take extra debugging to drill down & figure out which component was breaking the test.

> > diff --git a/layout/reftests/svg/smil/anim-strokewidth-1css.svg b/layout/reftests/svg/smil/anim-strokewidth-1css.svg
> > ...
> > diff --git a/layout/reftests/svg/smil/anim-strokewidth-1none.svg b/layout/reftests/svg/smil/anim-strokewidth-1none.svg
> > ...
> > diff --git a/layout/reftests/svg/smil/anim-strokewidth-1xml.svg b/layout/reftests/svg/smil/anim-strokewidth-1xml.svg
> 
> Do we need to test the attributeType lookup twice like this?

No, probably not -- a single attributeType test is probably sufficient.  I've removed the "css" and "none" versions of this test, since "XML" vs "CSS" vs "auto" behavior is already explicitly tested for "fill-opacity" (as described above), and the CSS version of "stroke-width" is already tested pretty thoroughly in the svg/smil/style reftest directory.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/272eb5b23ea6

> > +== anim-fillopacity-1none.svg      anim-standard-ref.svg
> > +== anim-fillopacity-1css.svg       anim-standard-ref.svg
> > +fails == anim-fillopacity-1xml.svg  anim-standard-ref.svg  # non-length-numeric values support
> 
> I don't understand the comment here: "non-length-numeric values support." Maybe
> it should be moved up two lines because it governs the three following tests
> right?

What I meant with that comment was "[This test fails because we're currently lacking] non-length-numeric values support [for XML attributes]".

To be clearer, it in my patch queue, I've now replaced that comment with "# bug 436276" (the bug # whose patch will fix that test). And I've replaced the similar "color support" comments there with a reference to bug 436296.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/272eb5b23ea6
(Assignee)

Updated

8 years ago
Attachment #387751 - Flags: review?(roc)
Attachment #387751 - Flags: review?(dbaron)
(Assignee)

Updated

8 years ago
Attachment #384145 - Flags: review?(dbaron)

Comment 70

8 years ago
(In reply to comment #68)
> > Are rects additive? The text for the 'clip' property doesn't explicitly say
> > they're not (unlike, say, stroke-dasharray) but perhaps they come under the> 
> > "All other data types used in animatable attributes and properties" category in
> > SVG 1.1, 19.2.5? In which case they're not additive.
...
> Anyway, I think it's fine to assume this isn't supposed to be additive.
> (particularly because Mozilla doesn't even appear to use this property in SVG,
> AFAICT. :))

We do support the clip property. See bug 481614 for details.
(Assignee)

Comment 71

8 years ago
(In reply to comment #67) -- final chunk of responses
(all regarding layout/reftests/svg/smil/style)

PREFACE: I've reorganized the reftests in the "layout/reftests/svg/smil/style" directory a lot in the past few days.  The biggest change was the creation of a JS function called "testAnimatedGrid" (and wrappers testAnimatedTextGrid & testAnimatedRectGrid).  This method automatically builds the "standard" 3x3 grid that I use in almost all of these reftests.  With this JS function, each of these reftests is on the order of ~20 lines instead of ~100 lines.  That should hopefully make it a *lot* easier to edit/extend/maintain the reftests in this directory.

Most of these improvements were done in a temporary patch in my patch queue, which I qfolded into "smil_css" here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/diff/6b5a8bc9f508/smil_css

Now, on to Brian's comments:
> > diff --git a/layout/reftests/svg/smil/style/anim-css-fill-1-from-to-ident.svg
> Might it make more sense here to make the 'from' value different to the style
> applied to the rect so we can differentiate between the color applied by the
> animation and the unanimated color?

Firstly: in my patch queue, I've now gotten rid of all the old anim-css-fill-1-* reftests (including the one that this comment is about), since they covered a lot of the same sorts of things that the anim-css-fill-2* tests covered.  (So now the old anim-css-fill-2* tests are renamed to anim-css-fill-1*, etc)

But to respond to your question -- the test "anim-css-fill-3-paced-rgb.svg" (now labeled "2" instead of "3" in my patch queue) already does what you're asking -- it uses an underlying color that is completely different from the animation endpoints.  In the test you referenced, the unanimated color was the same as the "from" value because another version of the test ("anim-css-fill-1-to-hex.svg") used pure "to" animation, which automatically uses the underlying value as "from".  And in order for the "to" and "from-to" tests to match (& share a common reference case), the "from-to" version needed to have its "from" value match the underlying value, too.

> Is it worth testing a by-value that takes a named color? To check that named
> colors are additive?

Yeah -- I'd meant to include one of those. Added:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/0153f05613db

> Once again, this would be a good place to test currentColor. (Both (i) as a
> to-value or by-value but also (ii) by animating the 'color' property on a
> parent group. Or even, (iii) both. Although I suppose (iii) depends on what we
> decide to do about incorporating base values from ancestors as per Daniel's
> query: http://lists.w3.org/Archives/Public/www-svg/2009Jul/0037.html)

(i) added currentColor as to & by-value
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/10b7574d7615

(ii) animating 'color' property on animation's parent, & using fill="currentColor"
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bf698805ebc1
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/f129df5996c0

(iii) is still TODO.

> > diff --git a/layout/reftests/svg/smil/style/anim-css-font-1.svg
> I wonder if it's enough to test just the first three? If "caption -> icon"
> works, won't "caption -> menu"

Not necessarily -- the tricky thing about system-font values is that some of them are identical on some platforms. They just represent specific bundles of font property-values, and some of the bundles may be the same.  (They're actually *all* the same on Ubuntu Linux, IIRC.)  If "caption" & "icon" happen to look the same, then testing "caption --> icon" won't actually test anything.  Since there are only a few possible system-font values anyway, I just tested caption --> all of them.

> I suppose I just wonder if the tests
> could be a little more targetted? For the sake of making it easier to maintain
> as much as anything.

In this particular case (system-font values), I think the comprehensiveness adds value.  However, there was definitely some redundancy elsewhere -- I've addressed this by:
 - removing a bunch of redundant anim-fontsize, anim-strokewidth, and anim-fill tests (15-20 removed, I think)
 - shortening tests using testAnimatedGrid
 - adding better descriptions of each test-group's purpose in reftest.list

> And then for the stroke-width tests, I wonder how much new code is covered by
> these tests? Aren't the code paths quite similar?

Removed some of these & a lot of font-size tests.  Now, length-animation is primarily covered by the stroke-width tests, and only a few font-size tests remain.
See the new-and-improved documentation in reftest.list for what each group of tests covers:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/0153f05613db/smil_css#l10705

> Wouldn't it be enough to
> create one test that includes all the different types of units and animation
> (i.e. from-by, to, etc.)  [...]

That would reduce the granularity of the test suite, which would go against the concept of "unit tests" IMHO.  (and it'd be a pretty huge reftest, if you really wanted it to include *all* the animation types for *all* the different unit types :)).

If it helps, you can consider each numbered reftest group (e.g. anim-css-strokewidth-1*) to be an instance of the "one test" you describe, with each file being a "sub-test".  Each of those groups is effectively an assertion that "these animations should all have identical effects".  By separating the sub-tests into distinct files, we benefit by only requiring a single fairly-simple reference case for the whole group, which we can compare against each of them individually.

> [...] and just takes a snapshot mid-way through the
> animation?

I've actually found it useful to take snapshots at "odd" times (not just halfway through), to catch different sorts of rounding issues and to make sure we aren't just getting lucky on the "easy" cases.  (I caught a number of rounding issues in the patch this way.) I've tried to set up my grid-based reftests' snapshots to capture all the main "interesting" times -- pre-animation, animation-begin, animation-end, post-animation -- plus a couple "easy" times mid-animation, and a couple "harder" fractional times.

> There are also other cases not covered here, such as the various situations
> when one animation is targetting an ancestor of another animation. I guess it's
> still unclear what should happen in such situations so perhaps that's why we
> haven't tested it yet.

I've actually got a mochitest -- test_smilCSSInherit.xhtml -- that covers the particular situation you mentioned -- but that behavior is currently broken, since we don't yet make sure to compose ancestors first (bug 501183).

But generally, yeah -- we should add tests for things that need testing.  Let me know if you think of anything that needs testing, and I'll add a test (or at least a TODO) for it.

Thanks for the helpful & comprehensive review comments, Brian!  I'll post the updated patch here soon.
(Assignee)

Comment 72

8 years ago
(In reply to comment #70)
> We do support the clip property. See bug 481614 for details.

Ah, thanks for the correction! I think I'd played around with the 'clip' property on a <marker>, and I couldn't get it to manifest visually.  I was probably just doing something wrong, though, and I do see that it manifests on <svg> subdocuments in the tests you added in bug 481614.

But in any case, I still think the spec implies <shapes> are non-additive, as described in comment #68.
(Assignee)

Comment 73

8 years ago
(In reply to comment #69)
> > Seeing as we have this neat system for broken properties, could we write the
> > tests for, e.g. stroke-dasharray, and let them show up as todo's instead?
> 
> I've done this in the changeset linked above, in the paced-mode mochitest.
> 
> I'm not doing it in test_smilCSSProps.xhtml, though, because that one actually
> has some useful stroke-dasharray tests (just with no interpolation), and if I
> were to flag stroke-dasharray as "broken" there, we'd completely skip those
> tests.  And that'd be bad, because they're the only tests covering CSS List
> functionality at this point. :)

Oops -- I must've been thinking of an old patch-version when I wrote that second paragraph quoted above.  As it turns out, the smil_css patch in my patch queue already does what you suggested here (i.e. it has "stroke-dasharray" in the "broken properties list" in test_smilCSSProps.xhtml).

So that means the CSS value list code isn't actually tested anywhere right now, in its partially-implemented (no ComputeDistance, no Interpolate) state.  To remedy this, I've added a reftest for it, with two reference cases -- one to check that the current behavior (unimplemented Interpolate()) doesn't accidentally change, and another to check for "correct" behavior (which currently fails).
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/e8848fae24be
Comment on attachment 386150 [details] [diff] [review]
smil_overridestyle patch v4b

r=dbaron, although I'm not sure whether it's worth adding GetSMILOverrideStyle and mSMILOverrideStyle given that they're not used anywhere.  (Or are they used in your later patches?)
Attachment #386150 - Flags: review?(dbaron) → review+
(Assignee)

Comment 75

8 years ago
(In reply to comment #74)
> (From update of attachment 386150 [details] [diff] [review])
> r=dbaron, although I'm not sure whether it's worth adding GetSMILOverrideStyle
> and mSMILOverrideStyle given that they're not used anywhere.  (Or are they used
> in your later patches?)

Yeah -- GetSMILOverrideStyle (and hence mSMILOverrideStyle, which it's an accessor for) is used in the "smil_css" patch, in nsSMILCSSProperty.cpp (in the "GetBaseValue" and "SetAnimValue" methods).

Thanks for the review!
(Assignee)

Comment 76

8 years ago
Created attachment 392389 [details] [diff] [review]
smil_overridestyle patch v5

Here's an updated "smil_overridestyle" patch, from my patch queue. The only changes from v4b are:
 - Final fix from bz's review comments, per Comment 47 (s/NS_IMETHOD/virtual nsresult, s/NS_IMETHODIMP/nsresult)
 - Add needed cycle-collector magic (mirroring that for "mStyle") for mSMILOverrideStyle in nsGenericElement.cpp, because the recent landing of bug 500850 made nsDOMCSSAttributeDeclaration::mContent a ref-counted pointer.
(Assignee)

Updated

8 years ago
Attachment #386150 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #386151 - Attachment is obsolete: true
(Assignee)

Comment 77

8 years ago
Comment on attachment 392389 [details] [diff] [review]
smil_overridestyle patch v5

dbaron -- sorry to bug you again -- could you also review the small chunk of cycle-collector code added in the update to  smil_overridestyle (the patch you just r+'d)?  I feel like that's a significant enough introduction of new code that I shouldn't just carry forward your review on the earlier version.

It's just 5 lines long -- the changeset that added it to my patch-queue is:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bf23a8f58b11

I'm pretty sure it's correct -- as noted in comment #76, I'm doing the same thing we do for "mStyle", and the change fixed the leaks that I started noticing after bug 500850 landed.
Attachment #392389 - Flags: review?(dbaron)
(Assignee)

Comment 78

8 years ago
Created attachment 392394 [details] [diff] [review]
smil_css_stylechanges patch v5

Here's the updated version of the "smil_css_stylechanges" patch. The only difference vs. the old version is that I fixed bitrot after bug 189519 landed.
Attachment #384145 - Attachment is obsolete: true
I think bug 500850 was slightly wrong, per bug 500850 comment 16.
(Assignee)

Comment 80

8 years ago
Created attachment 392395 [details] [diff] [review]
smil_css patch v5

Here's the latest version of "smil_css", the main patch for this bug.  It incorporates all of the fixes since comment 60.

I'm not posting an interdiff, because the patched code has changed enough that it's not possible to generate one. :)

The next version of this patch will most likely be changed to build on top of bug 504652.
Attachment #386155 - Attachment is obsolete: true
Attachment #387751 - Attachment is obsolete: true
Attachment #387753 - Attachment is obsolete: true
(Assignee)

Comment 81

8 years ago
(In reply to comment #79)
> I think bug 500850 was slightly wrong, per bug 500850 comment 16.

Ok, thanks -- I'll watch the other bug to see if/how that's fixed there, and mirror the fix in the smil_overridestyle patch here.
Hi Daniel, thanks for addressing all this. What you've said all looks great.

(In reply to comment #68)
>  (In reply to comment #67)
> > > +// nsSMILAnimationContext
> > 
> > I'm sure you've thought of this, but I'm just wondering if this class is
> > necessary.
[snip]
>  I need to think about this a bit.

As discussed on IRC, this will be addressed in bug 504652 or a subsequent bug after that bug is fixed.

> > > +  nsSVGSVGElement* GetSVGContext()   { return mSVGContext;   }
> > 
> > As above, I wonder if we can somehow remove the dependency on SVG.
> 
> Well, we definitely need direct access to the <SVG> element *somewhere*, so we
> can use its size to convert percentage lengths into absolute lengths.  Maybe
> that can be abstracted away, though, depending on potential reworking of
> animation contexts (per my previous comment-response), as well as on the work
> in bug 504652.

Yes, I would really like to see this SVG-specific reference abstracted away somehow in that bug.

(In reply to comment #69)
> > It seems like we have a lot of little objects being allocated at every
> > sample.
[snip]
> I'm leaving this as a TODO/to-investigate, since this is in code that will be
> reworked in bug 504652.  Added a comment to this effect:
> http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/84c1712ec9d6

Ok sounds good.

> I've been assuming that "font" is *not* interpolatable (and hence
> ComputeDistance & Interpolate should actually return NS_ERROR_FAILURE).
[snip]
> Fixed to use NS_ERROR_FAILURE here:
> http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1e9c3cfd5fcc

Ok, this seems reasonable.

> Under the hood, NS_PRECONDITION behaves exactly the same as NS_ASSERTION, and
> NS_ABORT_IF_FALSE is a better NS_ASSERTION. :) If any of the asserted
> conditions end up failing, NS_ABORT_IF_FALSE will make sure they get noticed
> right away, while NS_PRECONDITION/ASSERTION would just print stuff to the
> STDOUT and potentially go unnoticed. (which is how we've accumulated so many
> known-failing assertions in our codebase)

Yeah, that's fine. My concern was more about documentation, i.e. using 'abort if false' to test what is really a 'precondition'. But that's got nothing to do with this bug.

> Side note: I discovered a related issue while looking into this -- if
> a property doesn't support interpolation, we're actually supposed to *force*
> animation with calcMode="discrete", regardless of the provided value of
> "calcMode".  This is specified in the first paragraph about of "calcMode" in
> SVG 1.1 sec 19.2.7.  I've fixed that in
> http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1e9c3cfd5fcc

That's good. However, with the revised version of nsSMILAnimationFunction::InterpolateResult, what should be the behaviour for types where we haven't yet implemented Interpolate? Should we use discrete animation?

At the moment we have:

> if (GetCalcMode() == CALC_DISCRETE || rv == NS_ERROR_FAILURE) {

So when rv == NS_ERROR_NOT_IMPLEMENTED we won't do anything but return the error code and then bail out of nsSMILAnimationFunction::ComposeResult. Is that what we want? Or is it better to fall back to discrete animation for types that haven't implemented Interpolate (or ComputeDistance)?

> > Also, a lot hangs on the mIsCSS property being accurate. But I'm not sure
> > that it is. See comment in nsSMILAnimationController.cpp:
> > http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationController.cpp#480
> > Does the part of nsSMILAnimationController that determines if an "auto"
> > animation attribute is XML or CSS need fixing? And can we do that now?
> 
> Ah, right -- the comment you linked to is "// This doesn't really work for CSS
> properties that aren't mapped attributes".  While that's true, the only
> affected properties seem to be: color-interpolation-filters, flood-color,
> flood-opacity, and font. (I determined this by clearing the
> attributeType="CSS" setting in my mega-mochitest, and seeing what failed)
> 
> My first inclination was to just replace the "IsAttributeMapped" check with
> a call to "nsCSSProps::LookupProperty()", to test whether the attributeName
> matches a CSS property name.  But that won't work for e.g. "width", which *is*
> a CSS property name (and would pass the LookupProperty call), even though it's
> not a valid CSS property *in SVG*.
> 
> Not sure what the correct solution here is at the moment.

I think this needs attention. I suppose from SMIL's perspective this is host-language dependent so really we should call into something in the SVG code to query what is a CSS property.

Perhaps we could do this in the concrete subclass of nsISMILAnimationElement (which lives in the host language, e.g.  nsSVGAnimationElement). We could add an extra method on nsISMILAnimationElement, say, IsCSSProperty(). We already have a pointer to the nsISMILAnimationElement in nsSMILAnimationController::GetCompositorKeyForAnimation so we can just call IsCSSProperty() there. The implementation in nsSVGAnimationElement could just use IsAttributeMapped with special handling for color-interpolation-filters and the other exceptions (or something similar using nsCSSProps::LookupProperty).  Does that sound ok?

> > > +    "fill-opacity": {
> > Is this a good place to test some out-of-range values and check clamping
> > works correctly? Or do we have to wait for bug 501188 for that?
> 
> Good idea.  Tests added for this -- they pass already, only because the CSS
> parser doesn't happen to be super-strict with opacity values -- it doesn't
> clamp them to be less than 1.
> http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/8bef29ffface

That looks good. We're testing that we don't clamp before interpolation. I'm just wondering if we also need to test that we don't clamp until after addition too. So, if, for example, you had two additive animations as follows:

values: 0, 2, 0
values: 0, -1.2, 0

We need to check that the middle value is actually 0.8 because I guess you could end up clamping after interpolation but before addition (in which case you might get 0.5).

As per SMIL Animation 3.5:

> Ideally, the effect of all the animations active or frozen at a given point
> should be combined, before any clamping is performed. Although individual
> animation functions may yield out of range values, the combination of additive
> animations may still be legal. Clamping only the final result and not the
> effect of the individual animation functions provides support for these cases.
> Intermediate results may be clamped when necessary although this is not
> optimal.

I'm pretty sure we do this but we need to test for it.

> The stacked CSS + XML animations would perhaps make a useful additional test
> (particularly once we support animating the non-CSS version of fill-opacity
> and would be able to pass such a test).  But in general, I think it's good to
> keep unit tests as granular as possible, so I'd rather not replace these
> existing distinct tests with a single combined one.  Currently, the separated
> tests provide a lot of information -- they assert that we do the right thing
> for attributeType="CSS" and "auto", and that we fail for attributeType="XML".
> If we combined them into one, the combined test would have to be marked
> "fails", and we'd no longer be asserting that "CSS" and "auto" currently work.
> And if something breaks down the road (after we support attributeType="XML")
> and the combined test started failing, it'd take extra debugging to drill down
> & figure out which component was breaking the test.

Yeah, good call. I agree we should keep it granular.

Regarding my later suggestions I think what I had in mind wasn't so much that we integrate all the tests in one monster test but simply that we put them within the same image. The reason being that I'm not sure what the overhead is for each reftest. On another project we found the overhead was high so we arranged many small granular tests in a grid format within the one image. With that arrangement it's still fairly easy to discover which component is failing but without the overhead incurred by hundreds of little test files.

Just looking at comment #71 it looks like you've already done something similar anyway.

Also, one more thing I noticed in nsSMILCSSUtils.cpp:

> +    case eCSSProperty_font_size: {
> +      nsStyleContext* styleContext = aAnimContext.GetStyleContext();
> +      if (styleContext) {
> +        nsStyleContext* parentStyleContext = styleContext->GetParent();
> +        return parentStyleContext->GetStyleFont()->mSize;
> +      }
> +    } break;

I'm just wondering what the interaction is here with text zoom? I notice in nsSVGUtils when we're getting the font size we need to factor out the text zoom, presumably because em/ex units should be based on the font-size before any zoom is applied. (I'm really out of my depth here so I could well be wrong about all this.) If GetStyleFont() returns the zoomed font, then do we need to factor out any zoom lest we effectively end up zooming twice?

See http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGUtils.cpp#279

Apart from that, everything looks great! Thanks Daniel!
Comment on attachment 392389 [details] [diff] [review]
smil_overridestyle patch v5

r=dbaron if you also update the Unlink code to match the patch in bug 500850 comment 17
Attachment #392389 - Flags: review?(dbaron) → review+
(Assignee)

Updated

8 years ago
Depends on: 509540

Comment 84

8 years ago
(In reply to comment #82)
> 
> I'm just wondering what the interaction is here with text zoom? I notice in
> nsSVGUtils when we're getting the font size we need to factor out the text
> zoom, presumably because em/ex units should be based on the font-size before
> any zoom is applied. (I'm really out of my depth here so I could well be wrong
> about all this.) If GetStyleFont() returns the zoomed font, then do we need to
> factor out any zoom lest we effectively end up zooming twice?
> 

Text zoom should not do anything to SVG text. The value from GetStyleFont has text zooming applied so you do need to divide by the text zoom before using this value.

Updated

8 years ago
Blocks: 512501
(Assignee)

Comment 85

8 years ago
Created attachment 398281 [details] [diff] [review]
smil_overridestyle patch v6 [landed: c85]

(In reply to comment #83)
> r=dbaron if you also update the Unlink code to match the patch in bug 500850

Fixed that in my patch queue a while back -- here's the updated version, which I've just pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/1a045b4526ba
Attachment #392389 - Attachment is obsolete: true

Comment 86

8 years ago
It breaks Solaris x86 tinderbox.
"/export/home/ffslavex/mozilla-central-solaris-x86/build/content/base/src/nsGenericElement.cpp", line 2963: Error: Ambiguous "?:" expression, second operand of type "nsCOMPtr<nsICSSStyleRule>" and third operand of type "int" can be converted to one another.
1 Error(s) and 2 Warning(s) detected.

The line should be
return slots ? slots->mSMILOverrideStyleRule.get() : nsnull;

Thanks!
(Assignee)

Comment 87

8 years ago
Created attachment 400665 [details] [diff] [review]
smil_overridestyle followup: bustage fix for solaris [landed: c89]

Thanks for the heads up, Ginn!  Here's the bustage fix you suggested, in patch form.  dbaron, is this followup fix ok?
Attachment #400665 - Flags: review?(dbaron)
Comment on attachment 400665 [details] [diff] [review]
smil_overridestyle followup: bustage fix for solaris [landed: c89]

r=dbaron
Attachment #400665 - Flags: review?(dbaron) → review+
(Assignee)

Comment 89

8 years ago
pushed Solaris bustage fix -- http://hg.mozilla.org/mozilla-central/rev/4c28f6939ea5
(Assignee)

Updated

8 years ago
Attachment #398281 - Attachment description: smil_overridestyle patch v6 → smil_overridestyle patch v6 [landed: c85]
(Assignee)

Updated

8 years ago
Attachment #400665 - Attachment description: smil_overridestyle followup: bustage fix for solaris → smil_overridestyle followup: bustage fix for solaris [landed: c89]
(Assignee)

Comment 90

8 years ago
Created attachment 403003 [details] [diff] [review]
smil_css patch v6 (now using nsStyleAnimation)

Here's the updated "main patch" for this bug.  Notable changes:
 - nsSMILAnimationContext is removed.
 - text-zoom is divided out so that it doesn't affect us, per comment 82 / comment 84
 - We now use "nsStyleAnimation" utility class (added in bug 504652) for interpolation / distance-computation / addition / parsing of all CSS values.
   * (NOTE: nsStyleAnimation currently only supports color values, length values, and percentages (not mixed percentages + lengths yet, though), so the same applies to this patch here.  Per bug 504652 comment 47, we're not going to further-extend nsStyleAnimation until after we have SMIL+CSS and/or CSS Transitions landed.  Before those land, we can't regression-test the existing nsStyleAnimation functionality.)
 - We no longer need the "smil_css_stylechanges" patch, since it just facilitated conversion to/from strings, which is now handled by nsStyleAnimation.
 - I've rewritten the mochitests (in content/smil/test/) to be more modular, to share more code between similar tests, to be easier to modify/enable/disable on a per-test (or per-test-bundle) basis, and to be easily extendable to cover non-CSS attributes
 - I've made LookupComputedStyleFor return a already_AddRefed<nsComputedDOMStyle>, per comment 66.  Note that this required me to split out a chunk of nsGlobalWindow::GetComputedStyle, because that method normally takes "nsIDOMCSSStyleDeclaration** aReturn" as its outparam, and that won't compile if I try to pass in getter_addRefs(foo) where foo is a nsRefPtr<nsComputedDOMStyle>.
 - Misc other fixes & documentation-improvements.

This patch was generated by qfolding together these patches from my patch queue (in this order):
smil_css
smil_css_styleanimation
smil_css_newmochitests
smil_css_newmochitests_disableForNotYetLandedTypes

Link to patch-queue state as of this posting:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/257387c4f405
Attachment #392394 - Attachment is obsolete: true
Attachment #392395 - Attachment is obsolete: true
(Assignee)

Comment 91

8 years ago
Brian -- here are responses to your most recent review comments, now that a new patch is posted with them addressed:

(In reply to comment #82)
> > > > +// nsSMILAnimationContext
> As discussed on IRC, this will be addressed in bug 504652 or a subsequent bug
> after that bug is fixed.

This is done -- nsSMILAnimationContext is all gone in smil_css patch v6.

> > > > +  nsSVGSVGElement* GetSVGContext()   { return mSVGContext;   }
> > Well, we definitely need direct access to the <SVG> element *somewhere*, so we
> > can use its size to convert percentage lengths into absolute lengths.
>
> Yes, I would really like to see this SVG-specific reference abstracted away
> somehow in that bug.

This method is now gone (with the rest of nsSMILAnimationContext).  Given percent-unit computed-value inputs (e.g. to Interpolate), we now don't bother converting into pixel units.  nsStyleAnimation just sticks with percent-unit values in this case.

Note that nsStyleAnimation doesn't yet support *mixing* percentages with absolute lengths.  But we do have a plan for that -- once the CSS "calc" function is implemented (bug 363249), we can just use that for our interpolation & addition.  e.g. if we're 30% of the way from "10px" to "50%", Interpolate could output a computed value of "calc(.7*10px + .3*50%)".  That doesn't help us for ComputeDistance, though...  For that, I think we *will* need to convert into common units.  There are a number of ways we can do that when we get there, though (e.g. bundling a reference to the SVG element into *nsSMILValue.mU.mPtr).

> > if (GetCalcMode() == CALC_DISCRETE || rv == NS_ERROR_FAILURE) {
> 
> So when rv == NS_ERROR_NOT_IMPLEMENTED we won't do anything but return the
> error code and then bail out of nsSMILAnimationFunction::ComposeResult. Is that
> what we want? Or is it better to fall back to discrete animation for types that
> haven't implemented Interpolate (or ComputeDistance)?

I agree -- I think it's best to just fall into discrete-mode if we have any failures in Interpolate/ComputeDistance, including NS_ERROR_NOT_IMPLEMENTED errors.  This is fixed in smil_css patch v6.

> > > Also, a lot hangs on the mIsCSS property being accurate.
> > Not sure what the correct solution here is at the moment.
> 
> I think this needs attention. I suppose from SMIL's perspective this is
> host-language dependent so really we should call into something in the SVG code
> to query what is a CSS property.

Agreed.  I filed bug 509540 on this, but it ended up being essentially fixed this in smil_css patch v6.  I now use nsSMILCSSProperty::IsPropertyAnimatable(nsCSSProps::LookupProperty(propertyName)) to check whether a property-name corresponds to a recognized SMIL-animatable property.

One caveat, though -- currently, there are a number of CSS properties that supposed to be as animatable in the SVG spec, but currently return false for "IsPropertyAnimatable" simply because we don't support animating them yet. (generally because nsStyleAnimation doesn't support their type yet).  These properties are included in IsPropertyAnimatable but are commented out.  This technically means that attributeType="auto" would make us try to animate the SVG versions of those properties, when we *should* just bail out since they're CSS-but-not-supported-yet.  I think this is fine for now, because we don't yet support the SVG versions, either, so we still end up bailing out for that reason. :) And by the time we do support the SVG versions, nsStyleAnimation should be extended to support the CSS versions as well.

> I'm
> just wondering if we also need to test that we don't clamp until after addition
> too. So, if, for example, you had two additive animations as follows:
> 
> values: 0, 2, 0
> values: 0, -1.2, 0
> 
> We need to check that the middle value is actually 0.8 because I guess you
> could end up clamping after interpolation but before addition (in which case
> you might get 0.5).

Good point.  We can't actually test this in this bug's patch, since nsStyleAnimation doesn't support float values yet (though I've got a patch to add support in my patch queue).  I'll add a reftest to my patch queue to check for this later today -- it can be part of adding float-value support (a followup bug).

> I'm just wondering what the interaction is here with text zoom?

Good catch -- text-zoom is now divided out in nsSMILCSSValueType::ValueFromString (as noted in comment 90).  

I don't have an automated test for text-zoom-ignoring yet, but I can add one before checking this in. I've confirmed that this is fixed (& used to be broken) with a local font-size animation testcase. (by toggling 'browser.zoom.full' and zooming in/out with ctrl+scrollwheel)
(Assignee)

Comment 92

8 years ago
Comment on attachment 403003 [details] [diff] [review]
smil_css patch v6 (now using nsStyleAnimation)

Requesting review on smil_css patch v6.

If you want to compare this patch vs. the previous version, this patch is composed of:
  (a) The previous patch-version (still labeled "smil_css" in my patch queue), which hasn't changed much since v5 -- see changesets after 2009-08-03 here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/log/257387c4f405/smil_css
2009-08-03
  (b) smil_css_styleanimation patch: has the bulk of the changes. Removes & refactors a lot of old code
  (c) smil_css_newmochitests patch: adds & reworks mochitests. Includes "from-to" mochitests for all CSS properties mentioned in the SVG spec)
  (d) smil_css_newmochitests_disableForNotYetLandedTypes: disables mochitests for not-yet-suppported properties.

As mentioned at the end of comment 90, "smil_css patch v6" is a combination of these patches, as of my patch queue's revision 257387c4f405.
Attachment #403003 - Attachment filename: smil_css_v6_c → smil_css_v6
Attachment #403003 - Flags: superreview?(roc)
Attachment #403003 - Flags: review?(birtles)
(Assignee)

Comment 93

8 years ago
(In reply to comment #91)
> > So, if, for example, you had two additive animations as follows:
> > 
> > values: 0, 2, 0
> > values: 0, -1.2, 0
> > 
> > We need to check that the middle value is actually 0.8 because I guess you
> > could end up clamping after interpolation but before addition (in which case
> > you might get 0.5).

BTW -- I've just checked in two reftests to check for this (clamping after interpolation but before addition) to my patch queue, in this patch: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/21c9e47aa98c/smil_css_newreftests

One of these tests (anim-css-fillopacity-3-clamp-small.svg) uses values that are within the [-1.0, 1.0] range, and the other (anim-css-fillopacity-3-clamp-big.svg) uses one value that's larger than 1.0.  The former test passes (with the float-support patches from my patch queue), but the latter still fails due to bug 501188. (See the discussion of the "negative-value code" in comment 40 thru comment 42)
Hi Daniel, sorry for the delay. It all looks tip top, really good.

There's a very impressive amount of testing!

I haven't gone through it quite as thoroughly as last time because it seems much of the operation is similar (although without all the animation context stuff.)

(In reply to comment #91)
> > This is done -- nsSMILAnimationContext is all gone in smil_css patch v6.

Looks great! Well done!

> > Note that nsStyleAnimation doesn't yet support *mixing* percentages with
> > absolute lengths.  But we do have a plan for that -- once the CSS "calc"
> > function is implemented (bug 363249), we can just use that for our
> > interpolation & addition.  e.g. if we're 30% of the way from "10px" to "50%",
> > Interpolate could output a computed value of "calc(.7*10px + .3*50%)".  That
> > doesn't help us for ComputeDistance, though...  For that, I think we *will*
> > need to convert into common units.  There are a number of ways we can do that
> > when we get there, though (e.g. bundling a reference to the SVG element into
> > *nsSMILValue.mU.mPtr).

Is there any plan to implement calc() in the near future? There doesn't seem to be any activity on bug 363249 for nearly 3 years.

> One caveat, though -- currently, there are a number of CSS properties that
> supposed to be as animatable in the SVG spec, but currently return false for
> "IsPropertyAnimatable" simply because we don't support animating them yet.
> (generally because nsStyleAnimation doesn't support their type yet).  These
> properties are included in IsPropertyAnimatable but are commented out.  This
> technically means that attributeType="auto" would make us try to animate the
> SVG versions of those properties, when we *should* just bail out since they're
> CSS-but-not-supported-yet.  I think this is fine for now, because we don't yet
> support the SVG versions, either, so we still end up bailing out for that
> reason. :) And by the time we do support the SVG versions, nsStyleAnimation
> should be extended to support the CSS versions as well.

Seems reasonable to me.

(In reply to comment #93)
> > I'm
> > just wondering if we also need to test that we don't clamp until after addition
> > too. So, if, for example, you had two additive animations as follows:
> > 
> > values: 0, 2, 0
> > values: 0, -1.2, 0
> > 
> > We need to check that the middle value is actually 0.8 because I guess you
> > could end up clamping after interpolation but before addition (in which case
> > you might get 0.5).
> 
> Good point.  We can't actually test this in this bug's patch, since
> nsStyleAnimation doesn't support float values yet (though I've got a patch to
> add support in my patch queue).  I'll add a reftest to my patch queue to check
> for this later today -- it can be part of adding float-value support (a
> followup bug).

Looks good.

On to the patch.

> diff --git a/content/smil/nsSMILAnimationController.cpp b/content/smil/nsSMILAnimationController.cpp
> +// XXXdholbert: For animation from/to 'inherit' values to work correctly when
> +// the inherited value is *also* being animated, we really should be traversing
> +// our animated nodes in an ancestors-first order -- see bug 501183
> 
>  void
>  nsSMILAnimationController::DoSample(PRBool aSkipUnchangedContainers)
>  {

I think this comment belongs before the SampleAnimation step and after the SampleTimeContainers step. Because I don't think we need to sample the timing model in any way that is dependent on the tree hierarchy (rather we need to sample it in order of timing dependencies, particularly as we implement syncbase timing.)

> diff --git a/content/smil/nsSMILCSSValueType.cpp b/content/smil/nsSMILCSSValueType.cpp
> +static
> +const nsStyleCoord*
> +GetZeroValueForUnit(nsStyleUnit aUnit)
> +{
> ...
> +      NS_NOTREACHED("Calling SetToZero with an unsupported unit");

Is the function name here wrong?

> +nsresult
> +nsSMILCSSValueType::Interpolate(const nsSMILValue& aStartVal,
> +                                const nsSMILValue& aEndVal,
> +                                double aUnitDistance,
> +                                nsSMILValue& aResult) const
> +{
> ...
> +  const nsStyleCoord* startCSSValue;
> +  if (startWrapper->mPropID == eCSSProperty_UNKNOWN) {
> +    NS_ABORT_IF_FALSE(startWrapper->mCSSValue.IsNull(),
> +                      "If property ID is unset, then the unit should be, too");
> +    startCSSValue = GetZeroValueForUnit(endWrapper->mCSSValue.GetUnit());

Just wondering, if we're checking that startWrapper.mCSSValue isn't null, do we have any guarantee that endWrapper.mCSSValue isn't null? It seems like GetUnit will assert if endWrapper.mCSSValue is null.

> +  if (aUnitDistance == 0.0f) {
> +    // At time = 0, directly copy start value, so that we'll preserve
> +    // any special values of "from" at that time (e.g. percent values)

I'm curious about this. Do we have any test cases for this branch? What would be the difference if we just interpolated as usual?

> +fails == anim-fillopacity-1none.svg anim-standard-ref.svg # XXXdholbert float support in nsStyleCoord in bug 504652
> +fails == anim-fillopacity-1css.svg  anim-standard-ref.svg # XXXdholbert float support in nsStyleCoord in bug 504652

Is bug 504652 right? It seems to me most of the patches from that bug are already landed. Will float support be addressed in a separate bug or is there more to land in bug 504652?

> +fails == anim-fillopacity-1xml.svg  anim-standard-ref.svg # bug 436267

This is not the right bug number.
 
> +# check handling of overflowing color values
> +# NOTE: The second test fails because we compute "from + by" as the animation
> +# end-point, and we clamp that final color value and then use the clamped value
> +# for interpolation.  That's earlier than the SVG spec wants us to clamp --
> +# we're only supposed to clamp *final presentation values*.
> +# (Reference: SVG 1.1 Appendix F.4)
> +== anim-css-fill-overflow-1-by.svg       anim-css-fill-overflow-1-ref.svg
> +fails == anim-css-fill-overflow-1-from-by.svg anim-css-fill-overflow-1-ref.svg

Do we have a bug for this? Is this bug 515919?

> +# 'fill-opacity' property
> +# XXXdholbert These currently fail because the opacity properties still 
> +# use |float| instead of |nsStyleCoord|

Again, is there a bug number for this?

> +# 'font' shorthand property
> +# XXXdholbert currently fails because shorthand support is broken

Ditto

> +# 'stroke-dasharray' property, from/to with pixel values only
> +# XXXdholbert: I'm including a reference case ("ref-broken") for the
> +# *current* behavior, until we've got a smart "Interpolate" method implemented.
> +fails == anim-css-strokedasharray-1.svg anim-css-strokedasharray-1-ref.svg
> +fails == anim-css-strokedasharray-1.svg anim-css-strokedasharray-1-ref-broken.svg

So we even fail the broken test case. Cool. :)

Actually, is there are reason for that? I thought the point of the broken test was that it matched our broken behaviour?

> +# 'stroke-width' property, from/by/to with percent values
> +# XXXdholbert the mixed pct + px tests fail right now, because nsStyleAnimation
> +# doesn't support combining pct-values with coord-values yet

This is the calc() feature right?


Looks great Daniel! r=me with the above.
Attachment #403003 - Flags: review?(birtles) → review+
Comment on attachment 403003 [details] [diff] [review]
smil_css patch v6 (now using nsStyleAnimation)

Looks great, just a couple of nits:

+// nsISMILCSSType: Customized version of the nsISMILType interface, with some
+// additional methods for parsing & extracting CSS values, so that the details
+// of the value-storage representation can be abstracted away.

This is now named nsISMILCSSValueType

+static
+PRBool

This can be on one line. Similar stuff elsewhere.
Attachment #403003 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

8 years ago
Blocks: 520239
(Assignee)

Comment 96

8 years ago
(In reply to comment #94)
> Hi Daniel, sorry for the delay. It all looks tip top, really good.

Thanks! :) Thanks for looking through it again.

> I haven't gone through it quite as thoroughly as last time because it seems
> much of the operation is similar (although without all the animation context
> stuff.)

Yeah, not a lot of major things have changed.

> Is there any plan to implement calc() in the near future? There doesn't seem to
> be any activity on bug 363249 for nearly 3 years.

I think so -- now that we have this concrete requirement for it (to support a platform feature), I think it's something that I, dbaron, zwol, or somebody will work on implementing soon.

> (In reply to comment #93)
> > > just wondering if we also need to test that we don't clamp until after addition
[SNIP]
> Looks good.

BTW, the reftests I added for this were:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/5f40d34728ea
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/21c9e47aa98c

Those were supported by this earlier changeset, which extended smil-grid.js to allow multiple animations on each element.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/cda4202e3139

> > +// XXXdholbert: For animation from/to 'inherit' values to work correctly when
[SNIP]
> I think this comment belongs before the SampleAnimation step and after the
> SampleTimeContainers step.

Good point.  I've actually moved this comment to just before this call: 
>  currentCompositorTable->EnumerateEntries(DoComposeAttribute, nsnull);
because that's the step where the animated values are actually computed & applied (in the wrong order for 'inherit' to reliably work, per the comment).

http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/5fb02bad9130

> > +GetZeroValueForUnit(nsStyleUnit aUnit)
> > +      NS_NOTREACHED("Calling SetToZero with an unsupported unit");
> 
> Is the function name here wrong?

Ha.  Yes. :) Fixed.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/14a600e553b8

> > +nsSMILCSSValueType::Interpolate(const nsSMILValue& aStartVal,
[SNIP]
> > +  if (startWrapper->mPropID == eCSSProperty_UNKNOWN) {
> > +    NS_ABORT_IF_FALSE(startWrapper->mCSSValue.IsNull(),
> > +                      "If property ID is unset, then the unit should be, too");
> > +    startCSSValue = GetZeroValueForUnit(endWrapper->mCSSValue.GetUnit());
> 
> Just wondering, if we're checking that startWrapper.mCSSValue isn't null, do we
> have any guarantee that endWrapper.mCSSValue isn't null?

Yes -- for SVG/SMIL, we do have that guarantee.

A null mCSSValue unit represents the identity value -- it means we've just called nsSMILCSSValueType::Init() and haven't done anything further.

The only situation in which a just-initialized "identity value" would be passed to Interpolate (or ComputeDistance for that matter) is as the |aStartVal| argument in pure "by" animation.  In this case, we've got |aEndVal| = the "by value", and we're interested in finding out how much of it we should be adding.  (and |aStartVal| is the identity value, because at |aUnitDistance| = 0, we don't want to be adding anything).

So we could have a null (identity) value as the starting-point for interpolation.  But we'll always have a defined *endpoint* for our interpolation -- it's got to be either the "to" value, the next value in our "values" list, the result of "from + by" in from/by animation, or the "by value" in pure-by animation.  So, endWrapper is guaranteed to be non-null.

I've added some NS_ABORT_IF_FALSEs to this effect, to verify & clarify this assumtion:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/d42bd5426717

> > +  if (aUnitDistance == 0.0f) {
> > +    // At time = 0, directly copy start value, so that we'll preserve
> > +    // any special values of "from" at that time (e.g. percent values)
> 
> I'm curious about this. Do we have any test cases for this branch? What would
> be the difference if we just interpolated as usual?

Ah, good catch -- I used to need that special case in my old patch (because the main Interpolate() code path used to convert all percentages to absolute lengths), but I don't need it anymore.  We can just use interpolate as usual.

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/fc66370fafaf

> > +fails == anim-fillopacity-1none.svg anim-standard-ref.svg # XXXdholbert float support in nsStyleCoord in bug 504652
> > +fails == anim-fillopacity-1css.svg  anim-standard-ref.svg # XXXdholbert float support in nsStyleCoord in bug 504652
> 
> Is bug 504652 right? It seems to me most of the patches from that bug are
> already landed. Will float support be addressed in a separate bug or is there
> more to land in bug 504652?

Yes -- I think all of the patches on that bug have landed.  I want to check with dbaron before closing that bug & filing followup bugs for specific types, though.  So for now I've just clarified this comment to say "bug 504652 or a followup".

> > +fails == anim-fillopacity-1xml.svg  anim-standard-ref.svg # bug 436267 
> This is not the right bug number.

Oops, that needs a s/67/76/.  Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ce052ca9072f

> > +# check handling of overflowing color values
> > +# NOTE: The second test fails because we compute "from + by" as the animation
> > +# end-point, and we clamp that final color value and then use
[SNIP]
> Do we have a bug for this? Is this bug 515919?

Yes, that's the bug. I've added a mention of this bug to that comment:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/b5a293d16295

> > +# 'fill-opacity' property
> > +# XXXdholbert These currently fail because the opacity properties still 
> > +# use |float| instead of |nsStyleCoord|
> 
> Again, is there a bug number for this?

See above -- comment now reads "bug 504652 or a followup".

> > +# 'font' shorthand property
> > +# XXXdholbert currently fails because shorthand support is broken
> 
> Ditto

Filed bug 520239, and added mention of it:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/cc3906746839

> > +# XXXdholbert: I'm including a reference case ("ref-broken") for the
> > +# *current* behavior, until we've got a smart "Interpolate" method implemented.
> > +fails == anim-css-strokedasharray-1.svg anim-css-strokedasharray-1-ref.svg
> > +fails == anim-css-strokedasharray-1.svg anim-css-strokedasharray-1-ref-broken.svg
> 
> So we even fail the broken test case. Cool. :)
> 
> Actually, is there are reason for that? I thought the point of the broken test
> was that it matched our broken behaviour?

Good point. :) I'd meant to remove that reference case.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/e23bab67d462

> > +# XXXdholbert the mixed pct + px tests fail right now, because nsStyleAnimation
> > +# doesn't support combining pct-values with coord-values yet
> 
> This is the calc() feature right?

Yeah -- I've now filed bug 520234 on using calc() to support mixing percent & absolute lengths in animations, and I've added a note about it there in the comment you quote above.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/fd30d5aa6024

> Looks great Daniel! r=me with the above.

Thanks!
(Assignee)

Comment 97

8 years ago
(In reply to comment #95)
> +// nsISMILCSSType: Customized version of the nsISMILType interface, with some
[SNIP]
> This is now named nsISMILCSSValueType

Fixed that, along with the #ifndef / #define at the top of that header file, which had the same problem.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/162a0b435f54

> +static
> +PRBool
> 
> This can be on one line. Similar stuff elsewhere.

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/b295654c490c

Thanks for the reviews!
(Assignee)

Comment 98

8 years ago
Pushed!
http://hg.mozilla.org/mozilla-central/rev/0187a51241b7
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 99

8 years ago
Created attachment 404337 [details] [diff] [review]
smil_css patch v6.1 (as landed) [landed: c98]

Here's the patch as-landed. Carrying forward r+sr.
Attachment #403003 - Attachment is obsolete: true
Attachment #404337 - Flags: superreview+
Attachment #404337 - Flags: review+
(Assignee)

Comment 100

8 years ago
I've just landed a followup reftest-tweak patch, to make the reftests' "grid" positioning using "px" units instead of "pt" units.
  http://hg.mozilla.org/mozilla-central/rev/2b4ab8c8f64f

(Note that the SMIL reftests are currently all disabled in mozilla-central, so this won't affect the tinderbox.)

This was to fix an issue I noticed on the try-servers yesterday (with SMIL and SMIL reftests force-enabled).  The problem was basically this:
 - Some of the testcases specify a 200px-by-200px <svg> element, to provide a basis for percent-width values to use.
 - On the Linux try-server tinderbox, the "pt/px" ratio is high enough that the bottom-right edge of the testcase was positioned outside the 200px-by-200px box, and was getting clipped.

So, I've changed all those "pt" values to "px" values.  That fixed the issue in a subsequent try-server run.

The error log was:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1254514961.1254525028.11990.gz
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
(Assignee)

Updated

8 years ago
Blocks: 534028
(Assignee)

Comment 101

8 years ago
I just pushed a low-probability-randomorange-fix for a mochitest added in this bug, 'test_smilTextZoom.xhtml':
http://hg.mozilla.org/mozilla-central/rev/5803dc30baea

The orange was:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1261439943.1261442178.24825.gz#err0
Linux mozilla-central debug test mochitests-1/5 on 2009/12/21 15:59:03
s: moz2-linux-slave01
{
46264 ERROR TEST-UNEXPECTED-FAIL | /tests/content/smil/test/test_smilTextZoom.xhtml | computed value of font-size - got "47.2px", expected "10px"
46265 ERROR TEST-UNEXPECTED-FAIL | /tests/content/smil/test/test_smilTextZoom.xhtml | computed value of stroke-width - got "23.6px", expected "5px"
}

These failed tests, in the onload handler, implicitly assume that they'll be evaluated before any animations have taken effect (i.e. before document time=1s)  However, in this log, they're apparently being evaluated slightly after 1s, after the animations have started having an effect.

So, the bustage fix just includes an explicit seek to 0s before evaluating those tests.
You need to log in before you can comment on or make changes to this bug.