619498 - Allow animation between absolute and relative path segments of the same type

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should allow animation between absolute and relative path segments of the same type. I.e. the bottom row of:

http://dev.w3.org/SVG/profiles/1.1F2/test/svg/animate-elem-83-t.svg

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Eliminate some forwarding member functions with using declarations

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Support interpolation and addition of similar SVG path segment types

Random notes:
  * removed a declaration SVGPathSegUtils::ToString which had no implementation
  * restored alphabetical order to _TEST_FILES in
    content/svg/content/test/Makefile.in
  * added a few compile-time assertions so that if a new path segment were
    added, it would force the developer to check that various conditions still
    hold in the helper member functions of SVGPathSegUtils
  * removed superfluous "inline" keyword off the inline SVGPathSegUtils member
    functions
  * exposed SVGPathData contents with iterators (using begin/end functions)
    since it seemed more useful for some of the static functions in
    SVGPathSegListSMILType.cpp to take a pointer rather than an SVGPathData
    object and an index
  * replaced a couple of `blah.Length() == 0` with `blah.IsEmpty()`s
  * made SVGPathSegListSMILType::Add do something with aCount

Comment 3

[Cameron McCormack (:heycam)]

Oh, and is it a problem that the test case takes 10 seconds to run on my MBP?

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 2: Support interpolation and addition of similar SVG path segment types

Now with more context.

Comment 5

[Daniel Holbert [:dholbert]]

Per IRC discussion, I think we should probably only allow "compatibility" between relative & absolute commands, rather than between similar types of commands.  Reasoning:
 (a) With the current patch, we can only interpolate "similar" commands in one direction (e.g. from=horizontal to=line works, but not the reverse), which is counterintuitive from a user's perspective
 (b) The cutoff for which commands are "similar" is a bit fuzzy.
   (arguably you could promote an "h" command to a curve representation, just as easily as you can convert it to a line representation, albeit with some ambiguity.  And you could arguably convert any "z/Z" command into a line representation, too.)
 (c) I doubt (correct me if I'm wrong?) that other browsers support interpolating between the exact same sets of "similar" commands that your patch here has.

I fear that the above points would make for harder-to-understand behavior and poorer interop, so I'd prefer we stick with stricter & simpler command-compatibility allowances -- in particular, allowing conversion between relative & absolute commands (as is required by the test in Comment 0), but probably nothing further.

If it's all right, I'm going to hold off on further review for now, since the above change will probably simplify/remove a good bit of the patch, I think.

Here's one nit that I ran across when scanning the patch, though:
> SVGPathSegListSMILType::Add(nsSMILValue& aDest,
>+  // Allow addition to empty dest.
>+  if (dest.IsEmpty()) {
>+    return dest.CopyFrom(valueToAdd);
>+  }
[...]
>   if (dest.Length() != valueToAdd.Length()) {
>-    // Allow addition to empty dest:
>-    if (dest.Length() == 0) {
>+    if (dest.IsEmpty()) {
>       return dest.CopyFrom(valueToAdd);

This lower |dest.IsEmpty()| check will never succeed, because there's now an earlier |dest.IsEmpty()| clause higher up.  Probably better to replace the lower one with something like:
  NS_ABORT_IF_FALSE(!dest.IsEmpty(), "should have already handled empty-dest case");
(I'm not particularly worried that we'll ever fail that NS_ABORT_IF_FALSE, but it's nice for readibility & explicitly stating assumptions.)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> Oh, and is it a problem that the test case takes 10 seconds to run on my MBP?

I think mochitests are assumed to have timed out when they reach 300s, so you're fine until you start approaching that order of magnitude.  10s is a bit long, but as long as it's from doing useful tests rather than e.g. "setTimeout(foo, 9500)", then you're in the clear. :)

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 2: Support interpolation and addition of similar SVG path segm ent types (v2)

This version converts only between absolute and relative versions of path segments.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 510536 [details] [diff] [review]
Part 2: Support interpolation and addition of similar SVG path segm ent types (v2)

Great work here!  Notes below...

>diff --git a/content/svg/content/src/SVGPathData.h b/content/svg/content/src/SVGPathData.h
> class SVGPathDataAndOwner : public SVGPathData
> {
> public:
>+  using SVGPathData::iterator;

Can you move this declaration down, to be with the other "using" declarations (the ones added in the first patch here)?

Right now it's not immediately clear what its purpose is -- but if it were grouped with the others, it'd benefit from their explanatory "* Exposed so that..." comment.

>+static PRBool
>+ArcFlagsDiffer(SVGPathDataAndOwner::const_iterator aPathData1,
>+               SVGPathDataAndOwner::const_iterator aPathData2)
>+{
>+  NS_ABORT_IF_FALSE(SVGPathSegUtils::IsArcType(SVGPathSegUtils::DecodeType(aPathData1[0])), "ArcFlagsDiffer called with non-arc segment");
>+  NS_ABORT_IF_FALSE(SVGPathSegUtils::IsArcType(SVGPathSegUtils::DecodeType(aPathData2[0])), "ArcFlagsDiffer called with non-arc segment");

These lines are too long -- can you make them less than 80 chars, or at least add a newline before the warning-messages to be in the ballpark of 80 chars? :)

This applies to many other other places in the patch, too -- lines should generally be less than 80 chars, when possible.  According to
  grep ".\{82,\}" bug-619498-path-anim  | grep -v "^diff" | wc -l
there are 39 lines in the patch that are at least 81 characters that could use some shortening and/or newlining attention.  (I'm grepping with count>=82 to disregard the initial "+") 

>+  return aPathData1[4] != aPathData2[4] ||
>+         aPathData1[5] != aPathData2[5];

Add comments indicating what the [4] and [5] entries that you're comparing refer to, e.g.:
    return aPathData1[4] != aPathData2[4] ||  // large-arc-flag
           aPathData1[5] != aPathData2[5];    // sweep flag
(I don't know if that exact labeling is correct)

>+static inline PathInterpolationResult
>+CanInterpolate(const SVGPathDataAndOwner& aStart,
>+               const SVGPathDataAndOwner& aEnd)

This method is pretty large for an |inline|.  (though it only has one caller, so it's only inlined once, thankfully :))

I'd rather that we nix the |inline| here.  IIUC, compilers are smart enough to automatically inline single-caller-static-methods on their own, so I don't think the keyword buys us anything here (other then potentially larger-codesize, if we ever add new callers).

This applies to 2 other significantly-large |static inline| methods added to this file, too:
 InterpolatePathSegmentData
 ConvertPathSegmentData


>+  while (pStart != pStartDataEnd && pEnd != pEndDataEnd) {
[...]
>+    pStart += 1 + SVGPathSegUtils::ArgCountForType(startType);
>+    pEnd += 1 + SVGPathSegUtils::ArgCountForType(endType);
>+  }

If we ever get a buffer with corrupt data (or if we have a logic bug), we could easily jump *past* the end of one/both buffers, and then we'd never satisfy the |while| condition, and be stuck in this loop forever (or at least, for much longer than we'd like).

To protect against that potential foot-gun, could you...
 (a) change the "while" condition from "!=" to "<"
 (b) add something like this right after the loop's closing brace, so we'll know if we ever hit this very-bad condition:
     NS_ABORT_IF_FALSE(pStart <= pStartDataEnd && pEnd <= pEndDataEnd,
                       "iterated past end of buffer!! (corrupt data?)");

>+static inline PathInterpolationResult
>+InterpolatePathSegmentData(SVGPathDataAndOwner::const_iterator& aStart,
[...]
>+{
>+  PRUint32 startType = SVGPathSegUtils::DecodeType(*aStart);
>+  PRUint32 endType = SVGPathSegUtils::DecodeType(*aEnd);
>+
>+  if (startType != endType) {
>+    return eRequiresConversion;
>+  }

See my comments below regarding Interpolate() - given those comments, I'd rather that this method receive already-sanity-checked data, which means it can return void (or perhaps PRBool for "uh oh, unexpected failure", if we need), and we can convert the type checks & ArcFlagsDiffer() check into NS_ABORT_IF_FALSE's.

>+  PRUint32 argCount = SVGPathSegUtils::ArgCountForType(startType);

>+  *aResult++ = *aStart++;
>+  ++aEnd;

Add a comment like:
  // Skip over word for segment-type (copying |aStart|'s type into |aResult|)

>+UpdatePathTraversalState(SVGPathDataAndOwner::const_iterator& aData,
>+                         SVGPathTraversalState& aState)

This method seems to effectively duplicate the behavior that we already have implemented in SVGPathSegUtils::GetLength() and its various GetXXXLength() helper-methods (all of which update a SVGPathTraversalState as part of their operation).  We should call into that existing code instead.

>+AdjustPathDataForRelativeness(RelativenessAdjustmentType aAdjustmentType,
>+                              const SVGPathDataAndOwner::iterator& aResult,
>+                              const SVGPathTraversalState& aState)

Could you rename "aResult" here to "aPathPointToAdjust" or something like that?  In a file full of "iterator for an array of floats" variables, it's nice to have the variable-names self-document as much as possible. :)

>+static inline PRBool
>+ConvertPathSegmentData(SVGPathDataAndOwner::const_iterator& aStart,
[...]
>+  if (startType == endType) {
>+    aEnd += startLength;
>+    while (startLength) {
>+      *aResult++ = *aStart++;
>+      --startLength;
>+    }

Add a comment at the top of this block like:
  "// No conversion needed -- just directly copy |aStart|"

>+  PRUint32 endLength = 1 + SVGPathSegUtils::ArgCountForType(endType);

endELength here must match |startLength|, right? (since it's SameTypeModuloRelativeness)  So I think we should remove endLength, because its presence implies that they might not be the same.

We can instead NS_ABORT_IF_FALSE(startLength == 1 + SVGPathSegUtils::ArgCountForType(endType);)
(and maybe rename |startLength| to |commandLengthIncludingType| or something, to indicate that it's not just for aStart)


>+static PRBool
>+ConvertAllPathSegmentData(SVGPathDataAndOwner::const_iterator aStart,
[...]
>+{
>+  while (aStart != aStartDataEnd && aEnd != aEndDataEnd) {
>+    if (!ConvertPathSegmentData(aStart, aEnd, aResult, aState)) {
>+      return PR_FALSE;
>+    }
>+  }

As above, can you future-proof this against corrupt buffer-data by using < instead of != and asserting that we don't end up past the end of either buffer?

>+static inline SVGPathTraversalState
>+PathTraversalState(SVGPathDataAndOwner::const_iterator aDataBegin,
>+                   SVGPathDataAndOwner::const_iterator aDataEnd)
>+{
>+  SVGPathTraversalState state;
>+  while (aDataBegin != aDataEnd) {
>+    UpdatePathTraversalState(aDataBegin, state);
>+  }
>+  return state;

I think this method wants to be named something like "BuildPathTraversalState".

> nsresult
> SVGPathSegListSMILType::Add(nsSMILValue& aDest,
[...]
>+  if (check == eRequiresConversion) {
>+    if (!ConvertAllPathSegmentData(dest.begin(), dest.end(),
>+                                   valueToAdd.begin(), valueToAdd.end(),
>+                                   dest.begin(),
>+                                   SVGPathTraversalState())) {
>+      NS_WARNING("Path segment conversion failed, but we previously checked that it would succeed!");
>+      return NS_ERROR_FAILURE;
>     }

This NS_WARNING isn't something we expect to happen, right?  (i.e. it would indicate a bug in our code)

If so, this should probably be NS_ERROR() (the 'assertion'-type variant of NS_WARNING), so that tinderbox will turn orange if this ever fails.

>   PRUint32 i = 0;
>   while (i < dest.Length()) {
[...]
>   }
> 
>-  NS_ABORT_IF_FALSE(i == dest.Length(), "Very, very bad - path data corrupt");
>+  NS_ABORT_IF_FALSE(i == valueToAdd.Length(), "Very, very bad - path data corrupt");

Revert this s/dest/valueToAdd/ change.  I think the reasoning behind of the existing assertion was:
  "|i| is iterating up to |dest.Length()|, and we want to check
   that it hits the mark exactly rather than overstepping."
(note that |dest.Length()| is in the while() condition *and* the assertion at the end)

If we were to replace dest with valueToAdd, it would obfuscate this reasoning.

If you want to sanity-check valueToAdd.Length(), feel free to add an NS_ABORT_IF_FALSE() to check that it matches dest.Length() here.  (Probably not necessary, though, given that we already checked that with the "CanInterpolate" call & early-return up higher.)

In SVGPathSegListSMILType::Interpolate():
>   if (start.IsEmpty()) { // identity path
[...]
>   } else {
>+    SVGPathDataAndOwner::const_iterator pStart = start.begin();
>+    SVGPathDataAndOwner::const_iterator pStartDataEnd = start.end();
>+    SVGPathDataAndOwner::const_iterator pEnd = end.begin();
>+    SVGPathDataAndOwner::const_iterator pEndDataEnd = end.end();
>+    SVGPathDataAndOwner::iterator pResult = result.begin();

So, the patch right now has us check for command-type mismatches on-the-fly -- as we're interpolating, which is cool -- but it has the odd property that "eRequiresConversion" needs to mean *different things* when it's returned by CanInterpolate() vs. InterpolatePathSegmentData().  (The former returns it to mean "SameTypeModuloRelativeness", whereas the latter returns it to counterintuitively mean "Not the same type (and might not even be convertible).")

I'd rather we structure this in a slightly more straightforward way:
 (a) Call "CanInterpolate()" up-front, and use that to return early in case of command-mismatches. (This actually can replace the existing "start.Length() != end.length()" check, since CanInterpolate makes that check on our behalf.)
 (b) If needed, call "ConvertAllPathSegmentData()" right after (and update |aStart| (or some derivative pointer))
 (c) Remove the return-type from InterpolatePathSegmentData() (since now its data will have been pre-screened)

I recognize that this means we'll potentially make one extra path-data-walk (for the up-front CanInterpolate check), but it's not asymptotically slower.  It seems worth the minor hit, because it lets us fail earlier, it saves us the (more expensive) aResult.SetLength()-allocation in failure cases, and it's structurally simpler / more maintainable (from my perspective, at least).  It also fixes the "eRequiresConversion" ambiguity noted above, which is a Readability Win. :)

> SVGPathSegUtils::GetValueAsString(const float *aSeg, nsAString& aValue)
> {
>   // Adding new seg type? Is the formatting below acceptable for the new types?
>-  PR_STATIC_ASSERT(NS_SVG_PATH_SEG_MAX_ARGS == 7);
>+  PR_STATIC_ASSERT(NS_SVG_PATH_SEG_LAST_VALID_TYPE == nsIDOMSVGPathSeg::PATHSEG_CURVETO_QUADRATIC_SMOOTH_REL);

I like the new PR_STATIC_ASSERT, but I think the old one is also valuable & should remain alongside it.

It's there because this function contains a "case" block below that has magic numbers going from 0 to 6 -- and if we every support more than 7 args per command, we need to be sure to extend that "case" block.

>+++ b/content/svg/content/test/test_pathAnimInterpolation.xhtml
[...]
>+<svg xmlns="http://www.w3.org/2000/svg" id="svg" visibility="hidden"/>
[...]
>+  gSVG.pauseAnimations();

Per bug 550975, Best Practices in SMIL mochitests include calling "pauseAnimations" in the <svg> onload handler, like in
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/test/test_animLengthReadonly.xhtml?mark=15-17
because the window's load-handler (which triggers your "run" method) can fire noticably later than SVGLoad/animation-start-time.  I'm not sure if your test is in danger of any hitting problems from this issue, but it's probably still good to use the svg load-handler, for consistency / reliability / future-proofing.

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 510536 [details] [diff] [review]
Part 2: Support interpolation and addition of similar SVG path segm ent types (v2)

>+
>+  if (pStart != pStartDataEnd || pEnd != pEndDataEnd) {
>+    result = eFailed;

just return eFailed

>+
>+  if (aStart != aStartDataEnd || aEnd != aEndDataEnd) {
>+    return PR_FALSE;
>+  }
>+
>+  return PR_TRUE;

return aStart == aStartDataEnd && aEnd == aEndDataEnd;

Comment 10

[Cameron McCormack (:heycam)]

Thanks for the detailed review comments!

One question:

(In reply to comment #8)
> >+UpdatePathTraversalState(SVGPathDataAndOwner::const_iterator& aData,
> >+                         SVGPathTraversalState& aState)
> 
> This method seems to effectively duplicate the behavior that we already have
> implemented in SVGPathSegUtils::GetLength() and its various GetXXXLength()
> helper-methods (all of which update a SVGPathTraversalState as part of their
> operation).  We should call into that existing code instead.

The SVGPathSegUtils::GetLength and its helper methods do two things that are UpdatePathTraversalState doesn't, and which aren't necessary for the absolute <-> relative conversion:

1. They update the control point variables of the SVGPathTraversalState.

2. They actually do the length calculations, which can result in a bit of work (e.g. for curves).

#1 probably isn't too bad, but it would be good to avoid #2.  Should I be factoring out the traversal state updating from those helper methods so that they can be called independently of the path length calculation parts?

Comment 11

[Daniel Holbert [:dholbert]]

Per IRC conversation, I think we can probably add a single flag to the SVGPathTraversalState to indicate to the helper methods whether we care about the path-length and control points.

This means we probably need to rename the helper methods to something more like "TraverseSegment" rather than "GetSegmentLength" or whatever they're called now.

Comment 12

[Cameron McCormack (:heycam)]

I thought we were talking about a flag passed to all of these methods, but probably a flag on the SVGPathTraversalState is better to avoid passing more arguments.  Storing the length in that object would also avoid a bunch of useless `return 0`s in the only-updating-the-SVGPathTraversalState case, too.

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Part 2: Support interpolation and addition of similar SVG path segment types (v3)

This should address all of the review comments.

The path traversal functions in SVGPathData now can be used for either tracking everything (including length) or just the current subpath starting point & current point, controlled by the mode variable on SVGPathTraversalState.

I also renamed the identifiers in the PathInterpolationResult enumeration now that they are solely used for CanInterpolate.

(In SVGPathSegUtils I also did a bit of minor style cleanups of some of the helper functions; I hope that's OK!)

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 520421 [details] [diff] [review]
Part 2: Support interpolation and addition of similar SVG path segment types (v3)

> (In SVGPathSegUtils I also did a bit of minor style cleanups of some of the
> helper functions; I hope that's OK!)

Ah nice -- a lot of s/parameterName/aParameterName/ and return-type-gets-its-own-line fixes, it looks like.

Much as I like spirit of the cleanup, I'd defer to jwatt on these bits, since they're non-functional and have the potential downside of obfuscating hg blame for significant portions of SVGPathSegUtils.  (It may be worth splitting the purely-style-cleanup chunks into a separate patch, too, since they're pretty self-contained & are unrelated to the functional changes in this patch.)

--> Requesting feedback from jwatt on that part of the patch.
(jwatt, this is for the chunk of the patch from the beginning of SVGPathSegUtils.cpp to the first "s/GetLengthOfClosePath/TraverseClosePath/"-type change)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 520421 [details] [diff] [review]
Part 2: Support interpolation and addition of similar SVG path segment types (v3)

> nsresult
> SVGPathSegListSMILType::Add(nsSMILValue& aDest,
[...]
>+  PathInterpolationResult check = CanInterpolate(dest, valueToAdd);
>+
>+  if (check == eCannotInterpolate) {
>+    // nsSVGUtils::ReportToConsole - can't add path segment lists with different
>+    // numbers of segments, or with arcs with different flag values.
>     return NS_ERROR_FAILURE;

As long as you're mentioning 2 out of the 3 types of failures here, you might as well mention the third:
  "...or with incompatible segment types."

(Hm -- this chunk of the patch also conflicts with my recently-written logic-cleanup "patch 4" in bug 641393.  It's probably simplest for me to rewrite that other patch to apply on top of your patch here, rather than the reverse.  I'll add a note-to-self to that effect in bug 641393.)

>+  PathInterpolationResult check = CanInterpolate(start, end); 
>+  if (check == eCannotInterpolate) {
>+    // nsSVGUtils::ReportToConsole - can't add path segment lists with different
>+    // numbers of segments, or with arcs with different flag values.
>     return NS_ERROR_FAILURE;

s/add/interpolate/ (this chunk is inside of Interpolate())

See also the comment above about the related chunk inside of "Add()" - that applies here, too.

Also, extreme-nit, as long as you're touching these chunks: in the Add() chunk where you declare "PathInterpolationResult check", you've got a newline between the CanInterpolate call and your examination of its return-value -- but in the identical code in Interpolate(), there's no such newline.  Might as well make those consistent, one way or the other.

>+    NS_ABORT_IF_FALSE(pStart == pStartDataEnd && pEnd == pEndDataEnd &&
>+                        pResult == result.end(),
>+                      "Very, very bad - path data corrupt");

No need to have extra indentation on that second line there. (IIRC, we do indent if the previous line ended with "==", but not if it ended with && or ||.)

>diff --git a/content/svg/content/src/SVGPathSegUtils.cpp b/content/svg/content/src/SVGPathSegUtils.cpp
>-CalcDistanceBetweenPoints(const gfxPoint &p1, const gfxPoint &p2)
>+CalcDistanceBetweenPoints(const gfxPoint& aP1, const gfxPoint& aP2)
> {
>-  return NS_hypot(p2.x - p1.x, p2.y - p1.y);
>+  return NS_hypot(aP2.x - aP1.x, aP2.y - aP1.y);
[...]
>-static void SplitQuadraticBezier(const gfxPoint *curve,
>-                                 gfxPoint *left,
>-                                 gfxPoint *right)
>+static void
>+SplitQuadraticBezier(const gfxPoint* aCurve, gfxPoint* aLeft, gfxPoint* aRight)

(See Comment 14 on these style fixes)
 
>-static float GetLengthOfClosePath(const float *aArgs, SVGPathTraversalState &aState)
>+static void
>+TraverseClosePath(const float* aArgs, SVGPathTraversalState& aState)
> {
>-  float dist = CalcDistanceBetweenPoints(aState.pos, aState.start);
>-  aState.pos = aState.cp1 = aState.cp2 = aState.start;
>-  return dist;
>+  if (aState.UpdateAll()) {

"UpdateAll()" sounds like it modifies something.  This method probably wants to be named something more interrogative, like "ShouldUpdateAll()".  (or maybe "ShouldUpdateLengthAndCPs()", since that's what it actually ends up meaning?)

>+TraverseMovetoRel(const float* aArgs, SVGPathTraversalState& aState)
> {
>-  // aState.pos must be second from right due to +=
>-  aState.start = aState.cp1 = aState.cp2 = aState.pos += gfxPoint(aArgs[0], aArgs[1]);
>-  return 0.0;
>+  aState.start = aState.pos += gfxPoint(aArgs[0], aArgs[1]);
>+  if (aState.UpdateAll()) {
>+    aState.cp1 = aState.cp2 = aState.start;
>+  }

Since these "MoveTo" functions are the only ones to not update |aState.length|, and we don't have an explicit 'return 0.0' anymore, it'd be worth adding a comment to explain why we're not touching the length:
    if (aState.UpdateAll()) {
      // Note: length is unchanged; 'move' commands don't affect path-length
      aState.cp1 = aState.cp2 = aState.start;
    }
(This applies to "TraverseMovetoAbs", too)

>+static void
>+TraverseLinetoHorizontalRel(const float* aArgs, SVGPathTraversalState& aState)
> {
>-  aState.cp1 = aState.cp2 = aState.pos += gfxPoint(aArgs[0], 0.0);
>-  return fabs(aArgs[0]);
>+  aState.pos += gfxPoint(aArgs[0], 0.0);

The gfxPoint() constructor made sense before when we had a chain of "= ... +=" all on one line, but it's unnecessary after your changes.  Just replace this with:
    aState.pos.x += aArgs[0];

>+static void
>+TraverseLinetoVerticalRel(const float* aArgs, SVGPathTraversalState& aState)
> {
>-  aState.cp1 = aState.cp2 = aState.pos += gfxPoint(0.0, aArgs[0]);
>-  return fabs(aArgs[0]);
>+  aState.pos += gfxPoint(0.0, aArgs[0]);

Same here -- this should just be:
   aState.pos.y += aArgs[0];

>+SVGPathSegUtils::TraversePathSegment(const float* aData,
>+                                     SVGPathTraversalState& aState)
> {
>-  NS_ABORT_IF_FALSE(IsValidType(type), "Seg type not recognized");
[...]
>+  PRUint32 type = DecodeType(aData[0]);
>+  gTraverseFuncTable[type](aData + 1, aState);
> }

It looks like we don't sanity-check |type| at all right now.  I think we should keep the removed NS_ABORT_IF_FALSE that I quoted above (and it should go right after you declare |type|).

>diff --git a/content/svg/content/src/SVGPathSegUtils.h b/content/svg/content/src/SVGPathSegUtils.h
> struct SVGPathTraversalState
> {
[...]
>   gfxPoint cp1;   // quadratic control point - if the previous segment was a
>                   // quadratic bezier curve then this is set to the absolute
>                   // position of its control point, otherwise its set to pos

while you're touching the code around this, could you fix: s/its/it's/ here :)

>diff --git a/content/svg/content/test/test_pathAnimInterpolation.xhtml b/content/svg/content/test/test_pathAnimInterpolation.xhtml
[...[
>+// Returns whether interpolating between the two given types is valid.
>+function validInterpolation(aFromType, aToType)
>+{
>+  return (aFromType + aToType) in gSuffixes;
>+}

This defines 'validity' in terms of 'do I have a subtest for it', which seems fragile. :)

Wouldn't this be better defined with a direct comparison between aFromType/aToType, maybe using toUpperCase()?

(This function probably should be called "isValidInterpolation()", too)

>+// so for example,
>+//
>+//   "Mm": [[10, 20], [30, 40], [-30, -20], [-120, -100]]
>+//
>+// tests interpolating between "M10,20" and "m30,40".  The expected result
>+// half way through the interpolation is "m-30,-20" if the animation is not
>+// additive, and "m-120,-100" if it is.

Per IRC discussion, this needs a bit more explanation -- in particular:
 (a) Mention that M10,20 is converted into m-90,-80 w.r.t. 100,100
 (b) Mention that we set up the test such that base value is the same as
     the 'from' value (even though that's not the case in general) -- and
     so in the 'addititve' case, we're adding *to* M10,20 == m-90,-80

r=dholbert with the above

Comment 16

[Cameron McCormack (:heycam)]

I'm certainly happy to split out those irrelevant style cleanups to a separate patch.  (It'd be nice if it were possible to annotate a patch with a marker that indicates it's a minor change, so that smart hg blame browsers could omit them.)

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 1.5: Some nearby style cleanups (no change to code behavior)

Split the irrelevant cleanups out, asking for explicit review.

Comment 18

[Cameron McCormack (:heycam)]

Attachment: Part 2: Support interpolation and addition of similar SVG path segment types (v4)

Addressed review comments, carrying over r=dholbert.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 520840 [details] [diff] [review]
Part 1.5: Some nearby style cleanups (no change to code behavior)

Thanks for splitting these out, Cameron!

I'll mark r=dholbert on them, as they look safe enough, but I'll defer to jwatt's call about whether the style-consistency-win is worth the hg-blame-indirection.

Comment 20

[Cameron McCormack (:heycam)]

Landed on cedar: http://hg.mozilla.org/projects/cedar/rev/f4790a0a4cb8

Comment 21

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9e8156f46a4a
http://hg.mozilla.org/mozilla-central/rev/3f55422367a6
http://hg.mozilla.org/mozilla-central/rev/f4790a0a4cb8