We need to add support for SMIL animation of the <path> element's 'd' attribute.

This patch is pretty much complete except that 'by' animation isn't really working properly ('to' animation is fine).

dholbert, can you shed any light on why this would result in 'start' being empty in SVGPathSegListSMILType::Interpolate?

SVGPathSeg should probably be named SVGPathSegUtils? mozilla::SVGAnimatedLengthList* GetAnimatedLengthList(PRUint8 aAttrEnum); + virtual mozilla::SVGAnimatedPathSegList* GetAnimPathSegList() { Put a "typedef mozilla::SVGAnimatedLengthList SVGAnimatedLengthList;" in the class declaration to avoid having to use mozilla:: in these declarations. Obviously there will need to be more tests... Real comments later :-)

Comment on attachment 482197 [details] [diff] [review] patch >+#ifdef DEBUG >+ // These shifts are in sync with the flag member's in the header. Did you mean flag's members? >+ NS_ABORT_IF_FALSE(aList && >+ aListIndex < (1 << 31) && >+ aIsAnimValItem < (1 << 1), "bad arg"); >+ if (aIsAnimValItem && >+ mListIndex >= Element()->GetAnimPathSegList()->GetAnimValue().CountItems() || >+ !aIsAnimValItem && >+ mListIndex >= Element()->GetAnimPathSegList()->GetBaseValue().CountItems()) { >+ NS_ABORT_IF_FALSE(0, "Bad aListIndex!"); s/0/PR_FALSE/ >+ mList = nsnull; Isn't this line unreachable? >+void >+DOMSVGPathSeg::RemovingFromList() >+{ >+ PRUint32 argCount = SVGPathSeg::ArgCountForType(Type()); >+ memcpy(PtrToMemberArgs(), InternalItem() + 1, argCount * sizeof(float)); >+ mList = nsnull; >+ mIsAnimValItem = 0; If the above is a boolean (mIs and all) it should be set to PR_FALSE no? >+} >+ >+void >+DOMSVGPathSeg::ToSVGPathSegEncodedData(float* raw) >+{ >+ NS_ABORT_IF_FALSE(raw, "null pointer"); >+ PRUint32 argCount = SVGPathSeg::ArgCountForType(Type()); >+ if (IsInList()) { >+ memcpy(raw, InternalItem(), (1 + argCount) * sizeof(float)); >+ } else { >+ raw[0] = SVGPathSeg::EncodeType(Type()); >+ memcpy(raw+1, PtrToMemberArgs(), argCount * sizeof(float)); spaces around + >+// XXX okay to memcpy 0 bytes, right? right. >+ >+#define IMPL_SVGPATHSEG_SUBCLASS_COMMON(segName, segType, letter) \ >+ DOMSVGPathSeg##segName(const float *args) \ aArgs I don't know if you want to fix up the other methods later in the patch to start with a too? >+ : DOMSVGPathSeg() \ >+ { \ >+ memcpy(mArgs, args, SVGPathSeg::ArgCountForType(PRUint32(segType)) * sizeof(float));\ wrap line. >+ } \ >+ DOMSVGPathSeg##segName(DOMSVGPathSegList *aList, \ >+ PRUint32 aListIndex, \ >+ PRUint8 aIsAnimValItem) \ >+ : DOMSVGPathSeg(aList, aListIndex, aIsAnimValItem) \ >+ { \ >+ } \ >+ /* From nsIDOMSVGPathSeg: */ \ >+ NS_IMETHODIMP \ >+ GetPathSegType(PRUint16 *aPathSegType) \ >+ { \ >+ *aPathSegType = segType; \ >+ return NS_OK; \ >+ } \ >+ NS_IMETHODIMP \ >+ GetPathSegTypeAsLetter(nsAString &aPathSegTypeAsLetter) \ >+ { \ >+ aPathSegTypeAsLetter.Assign(letter); \ >+ return NS_OK; \ >+ } \ >+ /* From DOMSVGPathSeg: */ \ >+ virtual PRUint32 \ >+ Type() const \ >+ { \ >+ return segType; \ >+ } \ >+ virtual DOMSVGPathSeg* \ >+ Copy() \ >+ { \ >+ float *args = IsInList() ? InternalItem() + 1 : mArgs; \ >+ return new DOMSVGPathSeg##segName(args); \ >+ } \ >+ virtual float* \ >+ PtrToMemberArgs() \ I think you could define a cast operator. e.g. http://msdn.microsoft.com/en-us/library/5s5sk305%28VS.80%29.aspx instead. Would that make the callers simpler? >+// This must come after DOMSVGPathSegClosePath et. al. have been declared. >+/* static */ DOMSVGPathSeg* >+DOMSVGPathSeg::CreateFor(DOMSVGPathSegList *aList, >+ PRUint32 aListIndex, >+ PRBool aIsAnimValItem) >+{ >+ PRUint32 dataIndex = aList->mIndexMap[aListIndex]; >+ float *data = &aList->InternalList().mData[dataIndex]; >+ PRUint32 type = SVGPathSeg::DecodeType(data[0]); >+ >+ switch (type) >+ { { on previous line >+ /** >+ * This method is called to notify this DOM object that it is about to be >+ * removed from its current DOM list so that it can first make a copy of its >+ * internal counterpart's values. (If it didn't do this, then it would >+ * "loose" its value on being removed.) loose? Don't you mean lose? >+ */ >+ void RemovingFromList(); >+ >+ void ToSVGPathSegEncodedData(float *data); Comment about what this does? >+ while (index < length && dataIndex < dataLength) { >+ newSegType = SVGPathSeg::DecodeType(aNewValue.mData[dataIndex]); >+ if (mItems[index]) { >+ if (mItems[index]->Type() != newSegType) { combine if tests into one. >+PRBool >+DOMSVGPathSegList::AttrIsAnimating() const? >+{ >+ return InternalAList().IsAnimating(); >+} >+ /** >+ * Returns true if our attribute is animating (in which case our animVal is >+ * not simply a mirror of our baseVal). >+ */ >+ PRBool AttrIsAnimating(); const? >+#if 0 use it or lose it ;-) >+PRBool >+SVGPathData::InsertItem(PRUint32 aDataIndex, PRUint32 aType, const float *aArgs) >+{ >+ case nsIDOMSVGPathSeg::PATHSEG_CURVETO_QUADRATIC_REL: >+ if (segType == nsIDOMSVGPathSeg::PATHSEG_CURVETO_QUADRATIC_ABS) { >+ cp1 = gfxPoint(mData[i], mData[i+1]); >+ segEnd = gfxPoint(mData[i+2], mData[i+3]); were the spaces intended to line up? >+ } else { >+ cp1 = segStart + gfxPoint(mData[i], mData[i+1]); >+ segEnd = segStart + gfxPoint(mData[i+2], mData[i+3]); here too. >+ // F.6.5.1: >+ angle = angle*M_PI/180.0; could do with spaces around the * and sprinkled throughout the opertors in the code that follows this. >+ float x1p = cos(angle)*(segStart.x-segEnd.x)/2.0 >+ + sin(angle)*(segStart.y-segEnd.y)/2.0; >+ float y1p = -sin(angle)*(segStart.x-segEnd.x)/2.0 >+ + cos(angle)*(segStart.y-segEnd.y)/2.0; >+ >+ // This is the root in F.6.5.2 and the numerator under that root: >+ float root; >+ float numerator = rx*rx*ry*ry - rx*rx*y1p*y1p - ry*ry*x1p*x1p; >+ >+ if (numerator < 0.0) { >+ // F.6.6 step 3 - |numerator < 0.0| is equivalent to the result of >+ // F.6.6.2 (lamedh) being greater than one. What we have here is radii >+ // that do not reach between segStart and segEnd, so we need to correct >+ // them. >+ float lamedh = 1.0 - numerator/(rx*rx*ry*ry); // equiv to F.6.6.2 eqn >+ float s = sqrt(lamedh); >+ rx *= s; // F.6.6.3 >+ ry *= s; >+ // rx and ry changed, so we have to recompute numerator >+ numerator = rx*rx*ry*ry - rx*rx*y1p*y1p - ry*ry*x1p*x1p; >+ NS_ABORT_IF_FALSE(numerator >= 0, >+ "F.6.6.3 should prevent this - will sqrt(-num)!"); >+ } >+ root = sqrt(numerator/(rx*rx*y1p*y1p + ry*ry*x1p*x1p)); >+ >+#ifdef DEBUG >+ PRUint32 CountItems() const; >+#endif >+ >+ /** >+ * Returns the number of *floats* in the encoding array, and NOT the number >+ * of segments encoded in this object. (For that, see CountItems() above.) Perhaps say what count items does as a comment there then? >+ */ >+ PRUint32 Length() const { >+ >+#if 0 use it or lose it? >+ PRBool InsertItem(PRUint32 aDataIndex, PRUint32 aType, const float *aArgs); >+ >+ // XXX make sure caller checks new rv! >+ PRBool ReplaceItem(PRUint32 aDataIndex, PRUint32 aType, const float *aArgs); >+ >+ void RemoveItem(PRUint32 aDataIndex); >+ >+ PRBool AppendItem(PRUint32 aType, const float *aArgs); >+#endif >+ >+ static PRUint32 ArgCountForType(PRUint32 aType) { >+ NS_ABORT_IF_FALSE(IsValidType(aType), "Seg type not recognized"); >+ >+ static PRUint32 table[] = { >+ 0, // 0 == PATHSEG_UNKNOWN >+ 0, // 1 == PATHSEG_CLOSEPATH I wonder if there could be something on the individual pathseg classes which could return how many arguments they have. That might not help if all you have is a type though. Alternatively you could PR_ASSERT_IF_FALSE that these values are in sync in the individual path seg classes e.g. DOMSVGPathSegCurvetoCubicRel where you have mArgs[n] so you could do PR_ASSERT_IF_FALSE(sizeof(mArgs) / sizeof(mArgs[0]) == ArgCountForType(whatever)) in each of those. >+ PRUint32 i = 0, type, segEnd; >+ while (i < dest.Length()) { >+ type = SVGPathSeg::DecodeType(dest[i]); >+ if (type != SVGPathSeg::DecodeType(valueToAdd[i])) { >+ // nsSVGUtils::ReportToConsole - can't yet animate between different >+ // types, although it would make sense to allow animation between >+ // some. >+ return NS_ERROR_OUT_OF_MEMORY; surely the wrong error? NS_ERROR_FAILURE perhaps? >+ PRUint32 i = 0, type, segEnd; >+ while (i < end.Length()) { >+ type = SVGPathSeg::DecodeType(end[i]); >+ if (type != SVGPathSeg::DecodeType(start[i])) { >+ // nsSVGUtils::ReportToConsole - can't yet interpolate between different >+ // types, although it would make sense to allow interpolation between >+ // some. >+ return NS_ERROR_OUT_OF_MEMORY; as above >+ double cosTheta1 = cos(mTheta); >+ double sinTheta1 = sin(mTheta); >+ double theta2 = mTheta + mDelta; >+ double cosTheta2 = cos(theta2); >+ double sinTheta2 = sin(theta2); > > // a) calculate endpoint of the segment: >- *x3 = mCosPhi * mRx*cosTheta2 - mSinPhi * mRy*sinTheta2 + mCx; >- *y3 = mSinPhi * mRx*cosTheta2 + mCosPhi * mRy*sinTheta2 + mCy; >+ to->x = mCosPhi * mRx*cosTheta2 - mSinPhi * mRy*sinTheta2 + mC.x; >+ to->y = mSinPhi * mRx*cosTheta2 + mCosPhi * mRy*sinTheta2 + mC.y; even more intermediates may be more efficient e.g. something = mRy * sinTheta2 Overall this is really complicated so I'm just scratching the surface here. Well done for getting this far.

(In reply to comment #4) > I wonder if there could be something on the individual pathseg classes which > could return how many arguments they have. That might not help if all you have > is a type though. Alternatively you could PR_ASSERT_IF_FALSE that these values > are in sync in the individual path seg classes e.g. > DOMSVGPathSegCurvetoCubicRel where you have mArgs[n] so you could do > PR_ASSERT_IF_FALSE(sizeof(mArgs) / sizeof(mArgs[0]) == Note, there's an NS_ something or other macro for sizeof(mArgs) / sizeof(mArgs[0]

(In reply to comment #2) > dholbert, can you shed any light... Never mind, I remember how it works now.

Comment on attachment 482197 [details] [diff] [review] patch Haven't done a thorough review yet, but here's what jumped out at me from a quick skim. >diff --git a/content/svg/content/src/DOMSVGPathSeg.cpp b/content/svg/content/src/DOMSVGPathSeg.cpp >+DOMSVGPathSeg::DOMSVGPathSeg(DOMSVGPathSegList *aList, >+ PRUint32 aListIndex, >+ PRUint8 aIsAnimValItem) >+ : mList(aList) >+ , mListIndex(aListIndex) >+ , mIsAnimValItem(aIsAnimValItem) ... and from DOMSVGPathSeg.h: >+ PRUint32 mIsAnimValItem:1; It looks to me like mIsAnimValItem and aIsAnimValItem should be declared as PRBool and PRPackedBool:1, respectively -- not PRUint8 and PRUint32. And ++ to longsonr's comments about using PR_FALSE instead of 0 in various places where mIsAnimValItem gets set. >+ // These shifts are in sync with the flag member's in the header. s/member's/members/ >+ if (aIsAnimValItem && >+ mListIndex >= Element()->GetAnimPathSegList()->GetAnimValue().CountItems() || >+ !aIsAnimValItem && >+ mListIndex >= Element()->GetAnimPathSegList()->GetBaseValue().CountItems()) { >+ NS_ABORT_IF_FALSE(0, "Bad aListIndex!"); >+ mList = nsnull; >+ } >+#endif A few things: - Add parens around && inside of ||, for clarity & to avoid spammy GCC warnings. - It looks like the "if" check followed by the NS_ABORT_IF_FALSE(0,...) can be merged into a single NS_ABORT_IF_FALSE, since (as longsonr pointed out) the "mList = nsnull" won't ever be reached. - At that point, the #ifdef DEBUG won't be needed anymore and can be dropped. >+void >+DOMSVGPathSeg::InsertingIntoList(DOMSVGPathSegList *aList, >+ PRUint32 aListIndex, >+ PRUint8 aIsAnimValItem) >+{ >+ NS_ABORT_IF_FALSE(!HasOwner(), "Inserting item that is already in a list"); >+ NS_ABORT_IF_FALSE((mIsAnimValItem && >+ aListIndex < aList->Element()->GetAnimPathSegList()->GetAnimValue().CountItems()) || >+ (!aIsAnimValItem && >+ aListIndex < aList->Element()->GetAnimPathSegList()->GetBaseValue().CountItems()), >+ "mListIndex too big"); Looks like this is the same moderately-large check as in the chunk quoted above. Might make sense to split it out into a helper function. (If not, these lines are pretty long -- 106 chars I think -- and could use some beautification) Also, "mListIndex too big" message is indented to a weird position. >+void >+DOMSVGPathSeg::RemovingFromList() >+{ >+ PRUint32 argCount = SVGPathSeg::ArgCountForType(Type()); >+ memcpy(PtrToMemberArgs(), InternalItem() + 1, argCount * sizeof(float)); Why +1? (can you add a comment indicating why?) >+void >+DOMSVGPathSeg::ToSVGPathSegEncodedData(float* raw) >+{ >+ NS_ABORT_IF_FALSE(raw, "null pointer"); >+ PRUint32 argCount = SVGPathSeg::ArgCountForType(Type()); >+ if (IsInList()) { >+ memcpy(raw, InternalItem(), (1 + argCount) * sizeof(float)); >+ } else { >+ raw[0] = SVGPathSeg::EncodeType(Type()); >+ memcpy(raw+1, PtrToMemberArgs(), argCount * sizeof(float)); Again, why these +1's in the "1 + argCount" and "raw+1" here? (Also, add space around the "+" in the latter) A comment for each of the "if" clauses would be nice to explain what's going on. >diff --git a/content/svg/content/src/DOMSVGPathSegList.h b/content/svg/content/src/DOMSVGPathSegList.h >+ /// Used to determine if this list is the baseVal or animVal list. Extra "/" >+ * To simplyfy the code we just have this one method for obtaining both "simplify" >+ /// Creates an instance of the appropriate DOMSVGPathSeg sub-class for >+ // aIndex, if it doesn't already exist. >+ void EnsureItemAt(PRUint32 aIndex); There's an extra "/" on that first line. GENERAL NOTE: It looks like this patch has a number of changes like this (just taking nsSVGPathSeg as an example, since it's the first one I noticed): - Completely delete nsSVGPathSeg.cpp - Create a completely new file SVGPathSeg.cpp with a lot of the same code This deletes all the HG history associated with this code, which is bad. It'd be much better to do this as a hg mv + edits to moved file. You can make that change pretty easily -- with your patch applied, something like this should work: - revert the deletion of nsSVGPathSeg.cpp and the addition of SVGPathSeg.cpp, which I think is: hg revert -r -2 nsSVGPathSeg.cpp SVGPathSeg.cpp - mv SVGPathSeg.cpp SVGPathSeg.cpp.jwatt # back up your version - hg mv nsSVGPathSeg.cpp SVGPathSeg.cpp # do the hg mv - mv SVGPathSeg.cpp.jwatt SVGPathSeg.cpp # copy your version on top Then hg qrefresh to update the patch, and you're done! That has these benefits: - preserves HG history for untouched/lightly-touched lines. - reduces patch size - makes reviews for this patch much easier -- it lets us easily see which lines in the new file are untouched vs. mild changes (e.g. class renames) vs. actual new code. If you could do that for all this patch's deleted files that have new-file analogs, that would be awesome. I'm holding off at looking at the rest of this patch for now, because I think the above change (plus the removal of the "#ifdef 0" code that longsonr suggested) will make the rest of this patch a good deal smaller. :) (and hence easier to review)

(In reply to comment #5) > Note, there's an NS_ something or other macro for sizeof(mArgs) / > sizeof(mArgs[0] NS_ARRAY_LENGTH

Comment on attachment 482197 [details] [diff] [review] patch Waiting for existing comments to be addressed

This patch addresses the comments so far, with the caveats below. (In reply to comment #4) > >+ mIsAnimValItem = 0; > > If the above is a boolean (mIs and all) it should be set to PR_FALSE no? It's one bit of an unsigned int, not a boolean: + PRUint32 mListIndex:31; + PRUint32 mIsAnimValItem:1; The reason mIsAnimValItem is an unsigned int instead of a boolean is because compilers don't seem to compress adjacent bit flag variables unless they are of exactly the same type. > aArgs > > I don't know if you want to fix up the other methods later in the patch to > start with a too? I don't really fancy going through a patch this size trying to find these, no. I personally find the "a" prefix useful in functions with longish bodies, but roc seems to dislike it, and in very short functions it doesn't seem like it's worth worrying about to me. I fixed up that case nevertheless, and if there are others you particularly want changed then feel free to point them out. > I think you could define a cast operator. e.g. > http://msdn.microsoft.com/en-us/library/5s5sk305%28VS.80%29.aspx instead. Would > that make the callers simpler? You mean to replace PtrToMemberArgs? The DOMSVGPathSeg can be reading its arguments from two different places depending on whether it currently belongs to a list or not. This method very specifically returns a pointer to the args that are part of its instance, regardless of whether the object currently belongs to a list. It never returns a pointer into the SVGPathData instance that it's wrapping. So I don't think it makes sense to use a cast operator in this case. > >+ static PRUint32 ArgCountForType(PRUint32 aType) { > >+ NS_ABORT_IF_FALSE(IsValidType(aType), "Seg type not recognized"); > >+ > >+ static PRUint32 table[] = { > >+ 0, // 0 == PATHSEG_UNKNOWN > >+ 0, // 1 == PATHSEG_CLOSEPATH > > I wonder if there could be something on the individual pathseg classes which > could return how many arguments they have. That might not help if all you have > is a type though. Yeah, ArgCountForType is used by SVGPathData and SVGPathSegListSMILType. > Alternatively you could PR_ASSERT_IF_FALSE that these values > are in sync in the individual path seg classes Good idea! Done! > even more intermediates may be more efficient e.g. something = mRy * sinTheta2 Maybe, but really I was just converting nsSVGArcConverter to use gfxPoint and double to make it fit with the consumer code and make that code easier to read. Can you file another bug about that if you think it's worth it? > Well done for getting this far. Thanks! (In reply to comment #7) > It looks to me like mIsAnimValItem and aIsAnimValItem should be declared as > PRBool and PRPackedBool:1, respectively -- not PRUint8 and PRUint32. See my reply to longsonr on that one. > And ++ to longsonr's comments about using PR_FALSE instead of 0 in various > places where mIsAnimValItem gets set. I'm not sure that assigning PR_TRUE/PR_FALSE to an unsigned int bit flag can be relied upon to do right thing, so it seems safer to use 0/1. (At least I'm not sure it can if PRBool is changed to a straight boolean at some point. > Again, why these +1's in the "1 + argCount" and "raw+1" here? Because there's an encoded segment type before its float arguments. Understanding this code required understanding SVGPathData, which has a nice big comment. I'm added some comments where requested though. > Extra "/" That's intentional. The three /// marks this as a documenting Doxygen comment so that Doxygen picks it up. > GENERAL NOTE: > It looks like this patch has a number of changes like this (just taking > nsSVGPathSeg as an example, since it's the first one I noticed): > - Completely delete nsSVGPathSeg.cpp > - Create a completely new file SVGPathSeg.cpp with a lot of the same code ... > I'm holding off at looking at the rest of this patch for now, because I think > the above change (plus the removal of the "#ifdef 0" code that longsonr > suggested) will make the rest of this patch a good deal smaller. :) (and hence > easier to review) Unfortunately it doesn't really help. The code design and implementation details have change so much that there is really no worthwile context in the diffs left untouched. It seemed less confusing to me to have clean fresh files.

(In reply to comment #10) > (In reply to comment #4) > > aArgs > > > > I don't know if you want to fix up the other methods later in the patch to > > start with a too? > > I don't really fancy going through a patch this size trying to find these, no. > I personally find the "a" prefix useful in functions with longish bodies, but > roc seems to dislike it, and in very short functions it doesn't seem like it's > worth worrying about to me. I fixed up that case nevertheless, and if there are > others you particularly want changed then feel free to point them out. I dislike this rule, but I still follow the rules and this code should too. Please fix them. > (In reply to comment #7) > I'm not sure that assigning PR_TRUE/PR_FALSE to an unsigned int bit flag can be > relied upon to do right thing, so it seems safer to use 0/1. (At least I'm not > sure it can if PRBool is changed to a straight boolean at some point. It can. Use PR_TRUE/PR_FALSE.

+#define INDEX_IS_VALID \ + (aIsAnimValItem && \ + aListIndex < \ + aList->Element()->GetAnimPathSegList()->GetAnimValue().CountItems()) || \ + (!aIsAnimValItem && \ + aListIndex < \ + aList->Element()->GetAnimPathSegList()->GetBaseValue().CountItems()) This is a bit ugly. Why not just have a helper method that you call *after* setting mList/mListIndex/mIsAnimValItem that does the assertion? Call it VerifyIndex, say. + float mArgs[0]; // To allow IMPL_SVGPATHSEG_SUBCLASS_COMMON above to compile Unfortunately zero-sized arrays are not allowed in C++. I suggest you just make these length 1. You'll have to change some of your static asserts. Another possibility is to just dynamically allocate mArgs as needed, so make mArgs nsAutoArrayPtr<float>. But on balance I think that's probably not better than what you have. +#ifdef DEBUG + #define CHECK_ARG_COUNT_IN_SYNC(segType) \ + NS_ABORT_IF_FALSE(NS_ARRAY_LENGTH(mArgs) == \ + SVGPathSeg::ArgCountForType(PRUint32(segType)), \ + "Arg count/array size out of sync") +#else + #define CHECK_ARG_COUNT_IN_SYNC(segType) +#endif Why not just remove the #ifdef DEBUG here? It comes to the same thing. + NS_IMETHODIMP \ + GetPathSegType(PRUint16 *aPathSegType) \ + { \ + *aPathSegType = segType; \ + return NS_OK; \ + } \ This should just be a method in DOMPathSeg that calls Type(). + virtual DOMSVGPathSeg* \ + Copy() \ + { \ + /* InternalItem() + 1, because we're skipping the encoded seg type */ \ + float *args = IsInList() ? InternalItem() + 1 : mArgs; \ + return new DOMSVGPathSeg##segName(args); \ + } \ I think Clone() would be a better name. Instead of having two implementations of IMPL_PROP_WITH_TYPE for with/without SMIL, how about defining empty nsSVGElement::FlushAnimations, nsSVGElement::AnimationNeedsResample, and nsSVGElement::AttrIsAnimating. Just #ifndef out their bodies. +#define IMPL_PROP(segName, propName, index) \ + IMPL_PROP_WITH_TYPE(segName, propName, index, float) Let's call this IMPL_FLOAT_PROP. It's clearer. +DOMSVGPathSegList::Clear() ... + mItems.Clear(); + mIndexMap.Clear(); I think these are not needed, InternalListWillChangeTo will have set their length to 0. + nsTArray<DOMSVGPathSeg*> mItems; ... + nsTArray<PRUint32> mIndexMap; I think it would make things slightly simpler and slightly more efficient if we had a single nsTArray containing a struct with two fields. In SVGPathData::operator==, why not just compare the two mData arrays and be done with it? + // NOTE! ‘float’ is promoted to ‘double’ when passed through ‘...’! + // XXX is that in the C spec? We NEED consistent behavior here! Yes. + // There are nineteen switch cases below. We should maybe consider using + // path segment subclasses and putting these under a virtual call so we can + // get rid of this switch. Take this out; I think the switch is fine. + static PRUint32 table[] = { Might as well make it a PRUint8, and const. + static getValueAsStringFunc valueAsStringFuncTable[20] = { Can be const. +static void +GetValueAsStringOfCurvetoQuadraticRel(const float *args, nsAString& aValue) +{ + PRUnichar buf[96]; + nsTextFormatter::snprintf(buf, sizeof(buf)/sizeof(PRUnichar), + NS_LITERAL_STRING("q%g,%g %g,%g").get(), + args[0], args[1], args[2], args[3]); + aValue.Assign(buf); +} All these functions are actually making an unnecessary copy. You can use nsTextFormatter::ssprintf to write directly into aValue. Then you can go further; I think you don't need these helper functions, just write: nsTextFormatter::ssprintf(buf, formats[type], args[0], args[1], args[2], args[3], args[4], args[5]); I guess you'll need one other path to handle the arc cases. + return sqrt((p2.x - p1.x) * (p2.x - p1.x) + (p2.y - p1.y) * (p2.y - p1.y)); Use NS_hypot. + PRUint32 i = 0, type, segEnd; Move type and segEnd declarations into where they're assigned. + for (; i < segEnd; ++i) { + dest[i] += valueToAdd[i]; + } Shouldn't this be doing something special for the arc flags? It seems like we could end up with values > 1 for those booleans. + for (; i < segEnd; ++i) { + result[i] = start[i] + (end[i] - start[i]) * aUnitDistance; + } Same here. I guess we'll need tests for animation with arc flags, too. + nsIAtom *attr = Tag() == nsGkAtoms::path ? nsGkAtoms::d : nsGkAtoms::path; + SetAttr(kNameSpaceID_None, attr, newStr, PR_TRUE); How about introducing a new virtual method that returns the name of the path attribute, or null if there isn't one. Then just call it here. + // Check for SVGAnimatedPathSegList attribute + if (Tag() == nsGkAtoms::path && aAttribute == nsGkAtoms::d || + Tag() == nsGkAtoms::animateMotion && aAttribute == nsGkAtoms::path) { + SVGAnimatedPathSegList* segList = GetAnimPathSegList(); Call that new method here too. It's better not to hardcode the list of elements that have path attributes. + virtual mozilla::SVGAnimatedPathSegList* GetAnimPathSegList() { use a typedef in the class to avoid sprinkling mozilla:: in places. That's all I have! I didn't really check our changes to the mark calculations or the distance calculations, I hope you moved those over correctly. Overall it looks wonderful!

(In reply to comment #13) > All these functions are actually making an unnecessary copy. You can use > nsTextFormatter::ssprintf to write directly into aValue. Then you can go > further; I think you don't need these helper functions, just write: > nsTextFormatter::ssprintf(buf, formats[type], args[0], args[1], args[2], > args[3], args[4], args[5]); I guess that won't work if we don't have enough values left in 'args'. I suppose we need one code path per number of arguments.

(In reply to comment #10) > The reason mIsAnimValItem is an unsigned int instead of a boolean is because > compilers don't seem to compress adjacent bit flag variables unless they are of > exactly the same type. Ah, you're right -- confirmed that yesterday with this try push: http://hg.mozilla.org/try/rev/3f8261b1fec0 At bz's suggestion, I used a PR_STATIC_ASSERT to check whether a PRUint32:31 and a PRPackedBool:1 pack to 4 bytes, and that check failed on MSVC. So, PRUint32:1 it is then. :) > That's intentional. The three /// marks this as a documenting Doxygen comment Gotcha - sorry, I'd just never seen that syntax before. :) > Unfortunately it doesn't really help. The code design and implementation > details have change so much that there is really no worthwile context in the > diffs left untouched. It seemed less confusing to me to have clean fresh files. Ok -- sorry for the extra work there. The code that particularly made me think this would be useful was the series of functions like "GetValueAsStringOfMovetoAbs()", which all look like PRUnichar buf[MAGIC_NUMBER]; snprintf(buf, ...); aValue.assign(buf)" That series of functions seems to very closely match a similar series in nsSVGPathSeg.cpp. But from diffing (new) SVGPathSeg.cpp vs (old) nsSVGPathSeg.cpp, it looks like the context & ordering is different enough that the corresponding blocks don't get matched anyway. So, I guess that means the per-line hg history wouldn't get preserved very well, even with an |hg mv|. So, nevermind about that. :) Thanks anyway for humoring me, & sorry for the time you ended up wasting on that. On to the new patch... >diff --git a/content/svg/content/src/DOMSVGPathSeg.h b/content/svg/content/src/DOMSVGPathSeg.h >+ float* InternalItem(); >+ >+ virtual float *PtrToMemberArgs() = 0; Nit: s/float */float* / on the PtrToMemberArgs decl, for consistency with all the other method decls surrounding it. >+ if (delta != 0U) { >+ return NS_MAX(0U, segIndex - 1); // -1 because while loop takes us 1 too far >+ if (dest.Length() == 0U) { Nit: Do we really need the "U" on 0U, in these three lines? I don't imagine we hit unsigned/signed conversion or comparison warnings with just a normal 0, do we? >diff --git a/content/svg/content/src/SVGPathData.cpp b/content/svg/content/src/SVGPathData.cpp >+void >+SVGPathData::GetMarkerData(nsTArray<nsSVGMark> *aMarks) const [...] >+ aMarks->AppendElement(nsSVGMark(segEnd.x, segEnd.y, 0)); We probably need to handle AppendElement failure here, right? (for OOM) The other SVGPathData::GetFoo(nsTArray<nsFoo> *aFoo) methods around this one are vigilant about checking for OOM after AppendElement failures, but it looks like this this one might've been missed. >diff --git a/content/svg/content/src/SVGPathData.h b/content/svg/content/src/SVGPathData.h [...] >+ void GetMarkerData(nsTArray<nsSVGMark> *aMarks) const; >+ >+ PRBool GetSegmentLengths(nsTArray<double> *aLengths) const; >+ >+ /** >+ * Returns PR_TRUE, except on OOM, in which case returns PR_FALSE. >+ */ >+ PRBool GetDistancesFromOriginToEndsOfVisibleSegments(nsTArray<double> *array) const; Looks like this comment applies to the method right above it (GetSegmentLengths), too. Maybe shift it up & change to say "The following getters return [etc]") Perhaps it should shift up even further, above GetMarkerData, & apply to that, too (if that method ends up changing to return PRBool, per my comment immediately above this one about AppendElement OOM). >+class SVGPathDataAndOwner : public SVGPathData [...] >+ /** >+ * Exposed so that SVGPathData baseVals can be copied to >+ * SVGPathDataAndOwner objects. Note that callers should also call >+ * SetElement() when using this method! >+ */ [...] >+ const float& operator[](PRUint32 aIndex) const { >+ return SVGPathData::operator[](aIndex); >+ } The const version of operator[] is already public in the superclass (SVGPathData), so you don't need to re-expose it here, right? (I think you can just get rid of this declaration.) >+GetValueAsStringOfMovetoAbs(const float *args, nsAString& aValue) >+{ >+ PRUnichar buf[48]; [...] >+GetValueAsStringOfCurvetoCubicAbs(const float *args, nsAString& aValue) >+{ >+ PRUnichar buf[144]; [...] >+GetValueAsStringOfCurvetoQuadraticAbs(const float *args, nsAString& aValue) >+{ >+ PRUnichar buf[96]; What's the significance of these magic numbers? It looks like we're allocating 24 characters per %g value printed -- is that right? I think it'd be better to #define these buffer-sizes up top, e.g. #define BUFF_SIZE_ONE_ARG 24 #define BUFF_SIZE_TWO_ARGS 48 #define BUFF_SIZE_FOUR_ARGS 96 #define BUFF_SIZE_SIX_ARGS 144 with some explanation above that: // We allow 24 PRUnichars per snprintf %g argument, because [why?] That would make the functions themselves more readable and the buffer sizes less arbitrary-looking. >+nsresult >+SVGPathSegListSMILType::Add(nsSMILValue& aDest, Adding two path-data-commands seems a little strange to me. Do we have tests for that, and do you know if other browsers support it? >+nsresult >+SVGPathSegListSMILType::ComputeDistance(const nsSMILValue& aFrom, >+ const nsSMILValue& aTo, >+ double& aDistance) const >+{ >+ NS_PRECONDITION(aFrom.mType == this, "Unexpected SMIL type"); >+ NS_PRECONDITION(aTo.mType == this, "Incompatible SMIL type"); >+ >+ // nsSVGUtils::ReportToConsole >+ return NS_ERROR_FAILURE; >+} This needs an implementation. (Generally, any SMIL type that has Interpolate should also have ComputeDistance.) It should be pretty easy -- you can basically copy your Interpolate impl, but replace the line > result[i] = start[i] + (end[i] - start[i]) * aUnitDistance; with something like > float componentDist = to[i] - from[i]); > runningTally += componentDist * componentDist; And then at the end, return sqrt(runningTally). >+nsSVGElement::DidAnimatePathSegList() >+{ [...] >+ nsIFrame* frame = GetPrimaryFrame(); >+ >+ if (frame) { >+ nsIAtom *attr = Tag() == nsGkAtoms::path ? nsGkAtoms::d : nsGkAtoms::path; The check on that last line is unnecessary -- animateMotion doesn't get a frame, so we won't reach that line with an animateMotion elem. So -- inside the "if (frame)" case, we should probably assert that we're a <path> elem (or at least that we're *not* an animateMotion element), and we can assume that the modified attribute is "nsGkAtoms::d". Also: I noticed some "#ifdef MOZ_SMIL" chunks in various parts of the patch -- well done for remembering those. :) That reminds me to ask, though -- have you verified that this builds successfully with --disable-smil? (just to make sure we don't trigger "disable-SMIL builds are busted" bug reports after this lands) Generally, looks great!

(In reply to comment #13) > I think it would make things slightly simpler and slightly more efficient > if we had a single nsTArray containing a struct with two fields. Still to do. Probably I should just add a member to DOMSVGPathSeg to hold the internal data index actually rather than create another struct. > In SVGPathData::operator==, why not just compare the two mData arrays and > be done with it? I think I was thinking SVGPathSegUtils::EncodeType could potentially produce a NaN bit pattern, but a quick check of the format of NaNs confirms I don't need to worry about that. > All these functions are actually making an unnecessary copy. You can > use nsTextFormatter::ssprintf to write directly into aValue. Then you > can go further; I think you don't need these helper functions Killed 'em. > Shouldn't this be doing something special for the arc flags? It seems > like we could end up with values > 1 for those booleans. Good catch! > I guess we'll need tests for animation with arc flags, too. Yup. > Overall it looks wonderful! Thanks! (In reply to comment #15) > Perhaps it should shift up even further, above GetMarkerData, & apply to > that, too (if that method ends up changing to return PRBool, per my > comment immediately above this one about AppendElement OOM). Checking the callers, it doesn't seem useful to have that method return anything, so I addressed the first half of this comment. > The const version of operator[] is already public in the superclass > (SVGPathData), so you don't need to re-expose it here, right? (I > think you can just get rid of this declaration.) I don't seem to be able to. If I don't have this code then gcc seems to get unhappy about me throwing away const-ness since it seems to only see the operator on the subclass. > What's the significance of these magic numbers? I've removed that code entirely now. > Adding two path-data-commands seems a little strange to me. Do we > have tests for that, and do you know if other browsers support it? Basically if you want to move one point in the path in one way (eg linearly) but another point in another way (non-lirearly) then it is useful to be able to composite together animations. Tests in progress. > This needs an implementation. I'd actually rather prevent people from coming to rely on weird behavior. I can't think of any non-contrived and useful cases for paced animation with paths, and before we support that (if ever) I'd rather understand any use cases and make sure we compute "distances" in a useful way to match those use cases. > have you verified that this builds successfully with --disable-smil? Checking...

(In reply to comment #16) > (In reply to comment #13) > > I think it would make things slightly simpler and slightly more efficient > > if we had a single nsTArray containing a struct with two fields. > > Still to do. Probably I should just add a member to DOMSVGPathSeg to hold the > internal data index actually rather than create another struct. But that would require you to allocate a DOMSVGPathSeg for every segment as soon as we create the PathSegList, which I don't think we want to do. Creating them lazily as you do is nice. > > In SVGPathData::operator==, why not just compare the two mData arrays and > > be done with it? > > I think I was thinking SVGPathSegUtils::EncodeType could potentially produce a > NaN bit pattern, but a quick check of the format of NaNs confirms I don't need > to worry about that. Hmm, good call! Thanks for checking. Actually it's probably best just to do a memcmp to avoid that issue --- probably faster too.

(In reply to comment #16) > I'd actually rather prevent people from coming to rely on weird behavior. I don't think the metric I suggested would produce weird behavior -- on the contrary, I think it produces the simplest & most straightforward behavior. Your patch already treats a path-specification with N numeric values as an N-vector, and you interpolate each component independently. I think it's reasonable to extend that to compute distance with an N-dimensional Euclidian distance metric. We already do that with every other multi-numeric-component value already, I believe (e.g. rgb colors, stroke-dasharray, and CSS rect values), so it should be unsurprising to authors that we'd have the same behavior here. > can't think of any non-contrived and useful cases for paced animation with > paths, and before we support that (if ever) I'd rather understand any use cases > and make sure we compute "distances" in a useful way to match those use cases. Maybe this is "contrived" :) but here's a simple use case: > <path ...> > <animate attributeName="d" calcMode="paced" > values="m0 0 h50; m0 0 h60; m0 0 h10" ...> > </path> An author would expect that to produce constant-speed animation of the path-length -- increasing from 50 to 60 and then reducing to 10, all at the same speed. Anyway -- if you really want to leave that method unimplemented, I'd rather we have it return NS_ERROR_NOT_IMPLEMENTED & have an XXX comment explaining why we're not implementing it yet, rather than return NS_ERROR_FAILURE with no explanation.

+ // Special case: + if (type == nsIDOMSVGPathSeg::PATHSEG_ARC_ABS || + type == nsIDOMSVGPathSeg::PATHSEG_ARC_REL) { + PRBool largeArcFlag = aSeg[4] != 0.0f; + PRBool sweepFlag = aSeg[5] != 0.0f; + nsTextFormatter::ssprintf(aValue, + NS_LITERAL_STRING("%c%g,%g %g %d,%d %g,%g").get(), + typeAsChar, aSeg[1], aSeg[2], aSeg[3], + largeArcFlag, sweepFlag, aSeg[6], aSeg[7]); + } Shouldn't there be a return here?

> // Only now may me modify mBaseVal! I assume you meant Only now may we modify mBaseVal

Addressed comments. Not added support for paced animation since I couldn't get comfortable with it, and have run out of time. I also tried quite hard to be smart with the flags in the arc command, but ended up aborting that work too since although we could be clever, I couldn't get my head round smart interpolation when the other arguments are animating at the same time as the flags (radii, rotation and end points). I think this is the best I can do for now, and maybe we can fix up some other bugs post b7. (In reply to comment #19) > Shouldn't there be a return here? Argh, yes. Good catch, thanks! (In reply to comment #20) > > // Only now may me modify mBaseVal! > > I assume you meant > > Only now may we modify mBaseVal Indeed. :) Thanks.

Comment on attachment 487268 [details] [diff] [review] patch >diff --git a/content/svg/content/src/DOMSVGPathSegList.h b/content/svg/content/src/DOMSVGPathSegList.h >+ * that our DOMSVGPathSeg items wrap (the internal segment data is or varying >+ * length, so we can't just use the index of our DOMSVGPathSeg items s/is or/is of/ >diff --git a/content/svg/content/src/SVGPathData.cpp b/content/svg/content/src/SVGPathData.cpp >+ // NOTE! âfloatâ is promoted to âdoubleâ when passed through â...â! You've got fancy non-ASCII single-quote characters on this line. Replace with '. (Mozilla code generally avoids fancy quote characters in comments, AFAIK.) >+SVGPathSegListSMILType::Interpolate(const nsSMILValue& aStartVal, [...] >+ } else { >+ for (; i < segEnd; ++i) { >+ result[i] = end[i] * aUnitDistance; >+ } >+ } Nix end-of-line whitespace on that last line. >+ NS_ABORT_IF_FALSE(type > 0 && type < sizeof(lengthFuncTable) / >+ sizeof(lengthFuncTable[0]), >+ "Seg type not recognized"); The second check can be "type < NS_ARRAY_LENGTH(lengthFuncTable)", right? >diff --git a/content/svg/content/src/nsSVGElement.h b/content/svg/content/src/nsSVGElement.h > #ifdef MOZ_SMIL > virtual nsISMILAttr* GetAnimatedAttr(PRInt32 aNamespaceID, nsIAtom* aName); > void AnimationNeedsResample(); > void FlushAnimations(); >+#else >+ void AnimationNeedsResample() { /* do nothing */ } >+ void FlushAnimations() { /* do nothing */ } > #endif So up until now, we've wrapped calls to these functions in "#ifdef MOZ_SMIL" blocks, which meant we didn't need an "#else" case here. I'm guessing you added this because your patch adds non-MOZ_SMIL-guarded calls. Could you either... - wrap your calls in #ifdef MOZ_SMIL to match the existing convention (in which case this "else" clause is unnecessary & should be removed) ...or... - file a followup bug on removing "#ifdef MOZ_SMIL" from other calls to these methods? (since we've now got no-op impls for --disable-smil builds, which renders all those existing #ifdefs unnecessary) >+nsSVGPathDataParserToInternal::StoreEllipticalArc(PRBool absCoords, >+ float x, float y, >+ float r1, float r2, >+ float angle, >+ PRBool largeArcFlag, >+ PRBool sweepFlag) >+{ >+ PRUint32 type = absCoords ? >+ PRUint32(nsIDOMSVGPathSeg::PATHSEG_ARC_ABS) : >+ PRUint32(nsIDOMSVGPathSeg::PATHSEG_ARC_REL); >+ >+ return mPathSegList->AppendSeg(type, r1, r2, angle, >+ largeArcFlag ? 0.1f : 0.0f, >+ sweepFlag ? 0.1f : 0.0f, >+ x, y); "0.1f" is a bit of a magic number here -- could you add a short comment explaining its significance? Something like this (if it's correct): // The value "0.1f" below only matters in that it evaluates to "true" r=dholbert with the above. Great job here - this looks like it was a lot of work!

Comment on attachment 487268 [details] [diff] [review] patch Only nits... >+ /// This method is called to notify this object that its list index changed. Use javadoc style comments to be consistent with other methods. >+ void UpdateListIndex(PRUint8 aListIndex) { >+ mListIndex = aListIndex; >+ } >+ >+ } >+ } >+ >+ // Only now may we truncate mItems >+ mItems.SetLength(newLength); >+ } >+ else if (dataIndex < dataLength) { else on same line as } >+ >+ /// Used to determine if this list is the baseVal or animVal list. javadoc style comment >+ PRBool IsAnimValList() const { >+ return mIsAnimValList; >+ } >+ >+ >+ /// Creates an instance of the appropriate DOMSVGPathSeg sub-class for >+ // aIndex, if it doesn't already exist. >+ void EnsureItemAt(PRUint32 aIndex); here too. > nsSVGElement::NumberInfo nsSVGPathElement::sNumberInfo = > { &nsGkAtoms::pathLength, 0 }; Probably best not indented like this. See other existing sNumberInfo instances.

Never mind about the /// stuff I see Daniel already commented and you replied. I've never seen it either :-)

Comment on attachment 487268 [details] [diff] [review] patch Requesting approval for b7 as discussed with roc. I've run this through Try Server.

It's too late for b7, that's done.

Ok, I guess the approval request flag is still the right one though.

Comment on attachment 487268 [details] [diff] [review] patch Let's do this.

Land it!

Landed http://hg.mozilla.org/mozilla-central/rev/7907a5f28490 WOO!

I've just tested this in Firefox 4 RC2 or final (I understand that this patch was made publicly available since 4.0b8). But it doesn't seem to work at all for me. Just try the animation file @ http://towerofbabel.free.fr/stroke%20order/vertical.svg It's working fine in Opera but I just get a black line without animation in Firefox 4.

cyrilapan: Your testcase's "from" path contains >l0.392,0.296 which corresponds to this chunk of the "to" path: >L51.563,91.9 Those are different segment-types ("l" vs "L"), so they won't work in Firefox right now -- currently we only support smooth interpolation between path segments of the same type. Bug 619498 will improve this, to also allow interpolation between absolute & relative segments of the same type -- so your testcase should work after that bug is fixed. In the meantime, however, if you're building something that you'd like to work in both Opera and Firefox, you should be fine if you make sure that the path-segment-types all match.

Thanks a lot for your tip Daniel :) A last thing, I have this final animation running fine: http://towerofbabel.free.fr/stroke%20order/vertical2.svg When you click on the page, the animation resets and re-runs. However, the exact same animation doesn't work anymore when a XSL transformation is involved: http://towerofbabel.free.fr/stroke%20order/vertical3.svg Yet again, both versions work fine in Opera. Actually vertical2.svg is what I got from vertical3.svg when ran through "Web Developer" > "View generated source". Is this also an already logged bug?

I don't recall seeing any bugs filed on interactions between XSL and SMIL animation. If you could file a bug on your testcase (with the XSL stylesheet reduced to be as minimal as possible, ideally), that'd be great! Thanks.

Ok, created bug 644534