541884 - Add support for SMIL animation of the viewBox attribute in SVG.

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should add support for SMIL animation of the viewBox attribute in SVG.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

Comment on attachment 423285 [details] [diff] [review]
patch


>+  return NS_ERROR_FAILURE; // there is no concept of distance between rectanglular values

You just made up rectanglular ;-)

> 
>   if (content->HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
>     // XXXjwatt we need to fix our viewBox code so that we can tell whether the
>     // viewBox attribute specifies a valid rect or not.

Actually we have this now it's viewBox->IsValid() can you replace the HasAttr with mViewBox->IsValid? since you're changing this file anyway?

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 423285 [details] [diff] [review]
patch

>+nsresult
>+SVGViewBoxSMILType::Init(nsSMILValue& aValue) const
>+  if (aValue.mType != this || !aValue.mU.mPtr) {
>+    // Different type, or no data member: allocate memory and set type
[SNIP]
>+    aValue.mU.mPtr = viewBox;
>+    aValue.mType = this;

So, despite what nsSMILFloatType::Init might have you believe, we actually maintain the invariant that any nsSMILValue argument passed into Init() will be null-typed (aka have mType == nsSMILNullType::sSingleton).  Note that nsSMILCSSValueType::Init has an NS_ABORT_IF_FALSE to this effect. (though nsSMILFloatType and others aren't as strict in their assertions)

Given that invariant, your Init() method can be simplified a good bit:
 - The initial two NS_PRECONDITION/NS_ASSERTION lines can be replaced with just a single assertion that aValue.IsNull() (I prefer NS_ABORT_IF_FALSE for stronger enforcement / noticeability if it starts failing. But use your assertion of choice. :))
 - The "if/else" block can just be replaced with just the first clause ("Different type" -- null type, in this case)

Sorry that this wasn't clear -- we should probably go through and change this in all the preconditions for all the nsISMILType Init() methods.

>+nsresult
>+SVGViewBoxSMILType::Assign(nsSMILValue& aDest, const nsSMILValue& aSrc) const
>+{
>+  NS_PRECONDITION(aDest.mType == aSrc.mType, "Incompatible SMIL types.");
>+  NS_PRECONDITION(aDest.mType == this, "Unexpected SMIL value.");

Nix the period at the end of precondition / assertion warning messages. (Affects other places in the patch, too.)

>+nsresult
>+SVGViewBoxSMILType::Add(nsSMILValue& aDest, const nsSMILValue& aValueToAdd,
[snip]
>+  return NS_ERROR_FAILURE; // viewBox values can't be added to each other

Why can't they be added together?  I don't see why we shouldn't support that.... e.g. maybe you start out with a viewBox of "0 0 100 100" and you want to animate by="0 0 200 200", for a final viewbox of "0 0 300 300".  Note that we already support addition for the rect-valued CSS property "clip".

It's actually really easy to implement both Add and Interpolate with a helper function -- see nsStyleAnimation::AddWeighted[1] and its uses in nsStyleAnimation::Add and nsStyleAnimation::Interpolate[2]
[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp#550
[2] http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.h#127

You can probably do something like that with your existing Interpolate animation.

>+SVGViewBoxSMILType::ComputeDistance(const nsSMILValue& aFrom,
[snip]
>+  return NS_ERROR_FAILURE; // there is no concept of distance between rectanglular values

Sure there is -- 4-dimensional euclidean distance.  We can & should add support for that here.
That will allow us to handle cases like this:
 <animate calcMode="paced" attributeType="viewBox"
          values="0 0 10 10; 0 0 20 20; 0 0 50 50" ...>
There, we'd like to grow at a constant speed the whole time -- spend 1/4 of the duration going from the first to the second value, and 3/4 of the duration going from the second to third value.

See our existing ComputeDistance implementation for rect-valued CSS properties, in nsStyleAnimation:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp#266

>+void
>+nsSVGViewBox::SetAnimValue(float aX, float aY, float aWidth, float aHeight,
>+                           nsSVGElement *aSVGElement)
>+{
>+  if (!mAnimVal) {
>+    mAnimVal = new nsSVGViewBoxRect(aX, aY, aWidth, aHeight);
>+  } else {
>+    mAnimVal->x = aX;
>+    mAnimVal->y = aY;
>+    mAnimVal->width = aWidth;
>+    mAnimVal->height = aHeight;
>+  }
>+  aSVGElement->DidAnimateViewBox();
> }

We probably need to check the result of "new" there, right?  So perhaps this function should return nsresult, or PRBool, so that it can pass up a failure status to its caller if "new" fails.

The only caller right now is nsSVGViewBox::SMILRect::SetAnimValue.  That function always returns NS_OK right now, but if nsSVGViewBox::SetAnimValue started returning a PRBool status, then the SMILRect version could just check for that and return NS_ERROR_OUT_OF_MEMORY on failure.

(I don't know how much that really matters though... I guess worst-case, this would just mean the nsSVGViewBox would behave as if it didn't have an animated value, which isn't horrible)

>+nsSVGViewBox::SMILRect::ValueFromString(const nsAString& aStr,
[snip]
>+  nsSMILValue val(&SVGViewBoxSMILType::sSingleton);
>+  *static_cast<nsSVGViewBoxRect*>(val.mU.mPtr) = viewBox;

This will crash on that second line, if we're running out of memory. The first line (using the nsSMILValue constructor) makes a call to SVGViewBoxSMILType::Init, which allocates a new nsSVGViewBoxRect, and dutifully returns an error status if "new" fails.  However, the patch doesn't check for that right now, so if that happens, the second line will dereference null and die.

Although we're probably going to die pretty soon anyway in that case, it's actually easy enough to handle this gracefully there (and we might as well, as long as Init() tells us whether it succeeded). Just replace the first line above with:
  nsSMILValue val;
  nsresult rv = SVGViewBoxSMILType::sSingleton.Init(val);
  NS_ENSURE_SUCCESS(rv, rv);
  
>+nsSMILValue
>+nsSVGViewBox::SMILRect::GetBaseValue() const
>+{
>+  nsSMILValue val(&SVGViewBoxSMILType::sSingleton);
>+  val.mU.mPtr = new nsSVGViewBoxRect(mVal->mBaseVal);
>+  return val;
>+}

Two things here:
- This leaks.  The nsSMILValue constructor there implicitly calls SVGViewBoxSMILType::Init(), which *already allocates memory* for val.mU.mPtr, and then the explicit "new" call allocates *more* memory and overwrites the old pointer. The "new" call here needs to be replaced with direct value-assignment.
- This chunk has the same needing-to-check-whether-Init-failed problem that I described for ValueFromString, above, though with some tweaking because the return type is different.

So, combining those points, I think this GetBaseValue implementation needs to be replaced with something like:
  nsSMILValue val;
  if (NS_SUCCEEDED(SVGViewBoxSMILType::sSingleton.Init(val)) {
    *static_cast<nsSVGViewBoxRect*>(val.mU.mPtr) = mVal->mBaseVal;
  }
  return val;

(Note that GetBaseValue can indicate failure by returning a value with mType==nsSMILNullType -- that's what the new code above will do if it fails).

>diff --git a/content/svg/content/src/nsSVGViewBox.h b/content/svg/content/src/nsSVGViewBox.h
>--- a/content/svg/content/src/nsSVGViewBox.h
>+  nsSVGViewBoxRect(const nsSVGViewBoxRect &rhs) :
>+    x(rhs.x), y(rhs.y), width(rhs.width), height(rhs.height) {}

Nit: It looks like reference-ampersands are type-hugging in the code below this, in this file.

So, s/const nsSVGViewBoxRect &/const nsSVGViewBoxRect& /  :)

>+#ifdef MOZ_SMIL
>+  struct SMILRect : public nsISMILAttr
>+  {
>+  public:
>+    SMILRect(nsSVGViewBox* aVal, nsSVGElement* aSVGElement)

This struct should probably be called SMILViewBox, or perhaps SMILViewBoxAttr. The name "SMILRect" is misleading, because...
 - it's not at the "rect" level of generic-ness -- it takes a nsSVGViewBox* in its constructor, it uses SVGViewBoxSMILType-typed values, and it lives in the nsSVGViewBox source file.
 - it's not really a "rect" at all -- it's a handle for SMIL to use to mutate an element's viewBox *attribute*.

>+# animate some viewBox attributes
>+== anim-svg-viewBox-01.svg lime.svg
>+

We should test the behavior of Add() and ComputeDistance(), assuming we implement those methods, as I suggested that we do higher up in this comment. Maybe those can be tested via mochitests, but it's probably worth having at least one reftest that tests at least one of those.

Comment 4

[Robert Longson [:longsonr]]

Doesn't viewBox fall under "All other data types used in animatable attributes and properties" in http://www.w3.org/TR/SVG/animate.html#AnimationAttributesAndProperties which would mean it should not be additive.

Comment 5

[Daniel Holbert [:dholbert]]

Hmm, I don't entirely trust that table, since it really should have an "interpolatable" column, but instead "Additive" is kind of overloaded to mean both (or might be interpreted that way), as Cameron noted here:
http://lists.w3.org/Archives/Public/www-svg/2009Jul/0051.html
So it seems odd to me that we'd interpolate something without supporting addition for it.

But anyway... for now, yeah, I agree -- viewBox is <list of numbers>, as defined here
  http://www.w3.org/TR/SVGTiny12/coords.html#ViewBoxAttribute
which puts it in the <list of XXX> case.  This means it should behave like stroke-dasharray.  And we do allow interpolation of stroke-dasharray (as do Opera & Webkit), while enforcing that it's non-additive, here:
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCSSValueType.cpp#190

So, nevermind about implementing Add() -- instead, please include Robert's link from Comment 4 in the source code there, to indicate why we're disallowing addition.

We should still implement ComputeDistance, though -- that relates to normal (non-additive) interpolation, with calcMode="paced".

Comment 6

[Daniel Holbert [:dholbert]]

Can you also tweak SVGViewBoxSMILType to match the changes I made to the various nsISMILType subclasses (e.g. SMILEnumType) in this changeset:
  http://hg.mozilla.org/mozilla-central/rev/d9afac0e2458
See bug 542731 comment 7 & 8 for an overview of the (minor) changes.

Comment 7

[Daniel Holbert [:dholbert]]

BTW, per my first suggestion in comment 3 (about simplifying SVGViewBoxSMILType::Init) -- I actually just landed a cleanup patch that simplified nsSVGTransformSMILType::Init, to make similar changes there.
http://hg.mozilla.org/mozilla-central/diff/5f645dd4152a/content/svg/content/src/nsSVGTransformSMILType.cpp

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 425738 [details] [diff] [review]
patch

>+SVGViewBoxSMILType::ComputeDistance(const nsSMILValue& aFrom,
[SNIP]
>+  float dLeft = to->x - from->x;
>+  float dBottom = to->y = from->y;

You want "-", not "=", at the end of that second line there.  (Does that even compile as-is?  Currently, it's (unintentionally) modifying the const struct that |to| points to, which I should probably cause a compile error, unless I'm mistaken)

>+  float dRight = ( to->x + to->width ) - ( from->x + from->width );
>+  float dTop = ( to->y + to->height ) - ( from->y + from->height );

I don't think we should use the "compound" parameters dRight and dLeft in the distance computation -- it strikes me as more intuitive (and simpler computationally) to just use dWidth and dHeight. Given that our four components are <x, y, width, height>, and given that Interpolate() treats each of those components the same, it seems like ComputeDistance should treat the components the same, too. (rather than constructing "compound" components out of some of them)

Here's a real-world example where this makes a difference -- suppose we had a sequence of viewboxes, where we first increase 'x' by 1, and then we increase 'width' by 1. (holding y and height constant the whole time).  So, we've got these values for x (aka 'left'), width, and 'right':
 r1:  x=0  width=1  --> right = 1
 r2:  x=1  width=1  --> right = 2
 r3:  x=1  width=2  --> right = 3

So using the patch's current distance metric "D_r" (r for 'right'):
  D_r(r1, r2) = sqrt( (1 - 0)^2 + (2 - 1)^2 ) = sqrt(1 + 1) = sqrt(2) = 1.4
  D_r(r2, r3) = sqrt( (1 - 1)^2 + (3 - 2)^2 ) = sqrt(0 + 1) = 1
So currently, we'd judge the second interval to be "shorter" than the first.  (That's important because it means we'd allocate less time to it, in a paced-mode animation across these viewboxes.)

But using the simplified distance metric "D_w" (w for 'width), we get:
  D_w(r1, r2) = sqrt( (1 - 0)^2 + (1 - 1)^2 ) = sqrt(1 + 0) = 1
  D_w(r1, r2) = sqrt( (1 - 1)^2 + (2 - 1)^2 ) = sqrt(0 + 1) = 1
So if we were using dWidth in our distance computation, these two intervals would the same.

The latter behavior makes more intuitive sense and is a less "surprising" result, IMHO, given the format of "viewbox" -- it's specified as x/y/height/width, and it's not immediately clear to a developer why a change in any one of those values should inherently be valued more than a change in any other.

> void
> nsSVGViewBox::SetBaseValue(float aX, float aY, float aWidth, float aHeight,
>                            nsSVGElement *aSVGElement, PRBool aDoSetAttr)
> {
>-  mAnimVal = nsnull;
>   mBaseVal = nsSVGViewBoxRect(aX, aY, aWidth, aHeight);
>   mHasBaseVal = PR_TRUE;
> 
>   aSVGElement->DidChangeViewBox(aDoSetAttr);
>+#ifdef MOZ_SMIL
>+  if (mAnimVal) {
>+    aSVGElement->AnimationNeedsResample();
>+  }
>+#endif
> }

We should probably keep the removed "mAnimVal = nsnull" line there for the non-MOZ_SMIL case, right?  Rather than removing that line, we probably just want to move it to an "#else" case at the end there.

>+++ b/layout/reftests/svg/smil/reftest.list

We need a test of some sort to exercise the ComputeValue behavior (i.e. using a calcMode="paced" animation, making sure that we're not just giving equal time to each viewbox as we would in non-paced mode).

r=dholbert with the above addressed.

Comment 10

[Robert Longson [:longsonr]]

(In reply to comment #9)
> (From update of attachment 425738 [details] [diff] [review])
> >+SVGViewBoxSMILType::ComputeDistance(const nsSMILValue& aFrom,
> [SNIP]
> >+  float dLeft = to->x - from->x;
> >+  float dBottom = to->y = from->y;
> 
> You want "-", not "=", at the end of that second line there.  (Does that even
> compile as-is?  Currently, it's (unintentionally) modifying the const struct
> that |to| points to, which I should probably cause a compile error, unless I'm
> mistaken)

I suspect that's interpreted as dBottom = to->y, dBottom = from->y; so it would compile. I'm not arguing that it's right though ;-)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> We should probably keep the removed "mAnimVal = nsnull" line there for the
> non-MOZ_SMIL case, right?

Sorry, ignore this review comment.  The patch is correct in removing that line -- we don't want SetBaseValue to clear the animated value, regardless of whether SMIL is disabled in the build.

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to comment #9)
> Here's a real-world example where this makes a difference...

I'm not *completely* convinced that the animations between the three viewBox states that you describe should be given equal weighting. I confess it does feel pretty much intuitively right, but I find the following counter-example more definitely "intuitively right", and a convincing argument to prefer the code as it is in the patch now over changing it as you suggest.

Consider a 2x1 rectangle.

Case 1: animate its right edge left, until the rectangle is a 1x1 square
Case 2: animate its left edge right, until the rectangle is a 1x1 square

It seems (to me at least) that intuitively these animations are both "equivalent" in terms of "change". Unfortunately, with your approach:

Case 1: distance = sqrt(2), since only the width and height change by 1
Case 2: distance = sqrt(4), since x, y, width, height all change by 1

With the current approach in the patch, both these cases are given equal weighting, and I'm more convinced that the cases in this example should be given equal weighting than the cases in your example in comment 9.

I've been considering this issue for a while now, and it's because I couldn't find a concept of "distance" that really made sense for all the cases I considered that I initially plonked for saying paced animation wasn't supported. To be honest though, I don't really understand what you mean when you suggest using "4-dimensional euclidean distance" to obtaining a "distance" between *rectangles*. From what I understand, changing the number of dimensions in euclidean space only changes the number of coordinates that need to be squared before summing and taking the square root to find "euclidean distance". I.e. in 4D you're still taking the distance between points, not shapes, no? However, it could well be that the math textbooks I have don't cover this concept, and my searching of the Web to try and educate myself is lacking, so maybe you could elaborate a bit?

Comment 13

[Robert Longson [:longsonr]]

We don't really have a linear numeric range here do we so we shouldn't do distance/paced animations should we?

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> (In reply to comment #9)
> With the current approach in the patch, both these cases are given equal
> weighting, and I'm more convinced that the cases in this example should be
> given equal weighting than the cases in your example in comment 9.

Hmm, that's fair.

I agree that your suggestion is the more sensible & consistent metric for measuring "distance between two rectangles in 2D space".  My suggestion is the simpler "distance between two points in 4D space (ignoring the significance of the points, for simplicity)".

> From what I understand, changing the number of dimensions
> in euclidean space only changes the number of coordinates that need to be
> squared before summing and taking the square root to find "euclidean distance".
> I.e. in 4D you're still taking the distance between points, not shapes, no?
> maybe you could elaborate a bit?

Exactly correct -- and I was suggesting treating them just as "dumb" 4D points.  Purely numerically, I think that's the most straightforward way to answer "how much is the value of my |viewBox| attribute changing?".

But you've made a good case that |dRight| etc. is perhaps more sensible given the actual 2D spacial representation of a viewBox.

So, given that this behavior is unspecified, I think I'd be happy with either metric, as long as we have a comment justifying it in the code (i.e. "// just treat viewboxes as 4D points for distance computation", or "// use right/bottom for distance-computation instead of width/height, so we'll get the same result when we shrink the right side of the viewBox as when we shrink the left side. See bug 541884 comment 12")

Also, one nit -- if you end up sticking with the patch's current strategy (using dRight), I think you technically should switch the variable-names of dTop and dBottom around.  Since our coordinate-space grows from the upper-left corner, "y+height" is visually lower than "y" -- so "y" should be the top and "y+height" the bottom, I believe.

(In reply to comment #13)
> We don't really have a linear numeric range here do we

Yes we do, in 4D space. :)  (ignoring the 2d meaning of our values, as I said above) Just like we have a 3D "linear numeric range" for rgb colors.

> so we shouldn't do distance/paced animations should we?

I think we should support paced animation whenever we support interpolation.  So far, we've stuck to that (including for the rect-valued 'clip' property & the arbitrary-long-list-valued 'stroke-dasharray' property).  I don't see why we shouldn't support it here, when we can pretty easily.  As I say above in this comment, I don't particularly care about our choice of distance metric, as long as we have some justification for it.

Comment 15

[Robert Longson [:longsonr]]

(In reply to comment #14)

> I think we should support paced animation whenever we support interpolation. 
> So far, we've stuck to that (including for the rect-valued 'clip' property &
> the arbitrary-long-list-valued 'stroke-dasharray' property).  I don't see why
> we shouldn't support it here, when we can pretty easily.  As I say above in
> this comment, I don't particularly care about our choice of distance metric, as
> long as we have some justification for it.

OK.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jonathan's approach amounts to interpolating the positions of all four edges of the box, instead of its left and top edges and its width and height, right? That sounds fine to me. However, this needs to be raised with the SVG WG because the spec needs to be clear.

Comment 17

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/e2863ca85819

I believe I've addressed all the comments above, with the following caveats:

(In reply to comment #3)
> We probably need to check the result of "new" there, right?  So perhaps this
> function should return nsresult, or PRBool, so that it can pass up a failure
> status to its caller if "new" fails.

There's no point in passing up an error code for OOM here, so I didn't make any change for this comment.

> Just replace the first line above with:
>   nsSMILValue val;
>   nsresult rv = SVGViewBoxSMILType::sSingleton.Init(val);
>   NS_ENSURE_SUCCESS(rv, rv);

I didn't follow the two suggestions to call sSingleton.Init() explicitly in order to get its return value because this method is now protected. This SMIL type is a special case, so rather than muck with the code to make Init() public or make nsSVGViewBox::SMILViewBox a frient, I simply check val.mU.mPtr for null. This seems cleaner to me and is just as easy.

Comment 18

[Jonathan Watt [:jwatt]]

I filed the following issues for the unclear parts of the spec:

http://www.w3.org/Graphics/SVG/WG/track/issues/2307
http://www.w3.org/Graphics/SVG/WG/track/issues/2308

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to comment #17)
> > Just replace the first line above with:
> >   nsSMILValue val;
> >   nsresult rv = SVGViewBoxSMILType::sSingleton.Init(val);
> >   NS_ENSURE_SUCCESS(rv, rv);
> 
> I didn't follow the two suggestions to call sSingleton.Init() explicitly in
> order to get its return value because this method is now protected. This SMIL
> type is a special case, so rather than muck with the code to make Init() public
> or make nsSVGViewBox::SMILViewBox a frient, I simply check val.mU.mPtr for
> null. This seems cleaner to me and is just as easy.

Instead of checking val.mU.mPtr for null, it'd be cleaner if we instead checked "val.IsNull()" -- i.e. check whether we have the expected type vs the null type.  That behavior (ending up with null type when Init fails) is guaranteed & documented.  The mU.mPtr behavior is not.  (It happens to work in this case, but it would stop working e.g. if we changed the Init() implementation to only set mU.Ptr if allocation succeeded.)

Comment 20

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/816b6a3a4a56

Comment 21

[Daniel Holbert [:dholbert]]

This bug's patch broke "--disable-smil" builds:
{
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:40:25: error: nsISMILType.h: No such file or directory
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:38:25: error: nsSMILValue.h: No such file or directory
distcc[20852] ERROR: compile ../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp on gandalf failed
distcc[20852] (dcc_build_somewhere) Warning: remote compilation of '../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp' failed, retrying locally
distcc[20852] Warning: failed to distribute ../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp to gandalf, running locally instead
In file included from ../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:37:
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:40:25: error: nsISMILType.h: No such file or directory
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:38:25: error: nsSMILValue.h: No such file or directory
In file included from ../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:37:
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:45: error: expected class-name before ‘{’ token
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:53: error: ‘nsresult’ does not name a type
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:54: error: ‘nsSMILValue’ has not been declared
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:55: error: ‘nsresult’ does not name a type
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:56: error: ‘nsresult’ does not name a type
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:58: error: ‘nsresult’ does not name a type
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.h:61: error: ‘nsresult’ does not name a type
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:48: error: ‘nsresult mozilla::SVGViewBoxSMILType::Init’ is not a static member of ‘class mozilla::SVGViewBoxSMILType’
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:48: error: ‘nsSMILValue’ was not declared in this scope
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:48: error: ‘aValue’ was not declared in this scope
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:48: error: expected ‘,’ or ‘;’ before ‘const’
../../../../../mozilla/content/svg/content/src/SVGViewBoxSMILType.cpp:149: error: expected `}' at end of input
}

In the makefile, SVGViewBoxSMILType.cpp needs to be moved down a bit, into the SMIL-only chunk (where SVGOrientSMILType.cpp is).  I think that fixes everything.

Comment 22

[Daniel Holbert [:dholbert]]

I pushed a --disable-smil bustage fix per comment 21:
  http://hg.mozilla.org/mozilla-central/rev/9bfdba1fbc94

Comment 23

[Jonathan Watt [:jwatt]]

Ah! Thanks Daniel.