This is a first pass at creating a "new style" SVGAnimatedLengthList. The non animated length lists that it contains are still old-style and use the old notification system for their SVGLength elements to communicate changes. The non-animated lists contact their parent animated list more directly.

We should use a TearoffTable here in ToDOMAnimatedLengthList, e.g. http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGLength2.cpp#100 This will ensure that ToDOMAnimatedLengthList always returns the same JS/DOM object (as long as the tearoff is alive), which is required by the spec now. And we should have a test for that... Why does nsSVGAnimatedLengthList have mSVGElement? Is that just temporary until the inner length lists are modernized also? + *aResult = (mVal->mAnimVal) ? mVal->mAnimVal : mVal->mBaseVal; This probably needs an XXX comment because it's incorrect --- we should be returning a different object for the animVal than the baseVal. And we should have a test for that...

Comment on attachment 399160 [details] [diff] [review] Patch >diff --git a/content/svg/content/src/nsSVGTSpanElement.cpp b/content/svg/content/src/nsSVGTSpanElement.cpp >--- a/content/svg/content/src/nsSVGTSpanElement.cpp >+++ b/content/svg/content/src/nsSVGTSpanElement.cpp >@@ -37,36 +37,35 @@ > * ***** END LICENSE BLOCK ***** */ > > #include "nsSVGStylableElement.h" > #include "nsGkAtoms.h" > #include "nsIDOMSVGTSpanElement.h" > #include "nsCOMPtr.h" > #include "nsSVGAnimatedLengthList.h" > #include "nsSVGLengthList.h" >-#include "nsSVGSVGElement.h" >+//#include "nsSVGSVGElement.h" If you don't need the include just remove the line. Please also add a lengthlist test to test_valueLeaks.xhtml (content/svg/content/test). I'd like to see another version with roc's issues addressed but it looks pretty good so far.

This patch still needs a fair bit of work. Feel free to look at it to get an idea of how I'm changing things, but don't spend too much time reviewing the details.

I think BV and AV might as well be BaseVal and AnimVal. I think we should merge nsSVGLength2 into this so that non-list attributes use the same classes. However, this should probably be done in a separate patch. + /* It might seem like we should use nsAutoTArray<SVGLength, 1> instead of + * nsTArray<SVGLength>. That would preallocate space for one SVGLength and + * avoid an extra memory allocation call in the common case of the 'x' + * and 'y' attributes each containing a single length (and the 'dx' and 'dy' + * attributes being empty). However, consider this: I wouldn't worry about this, I think nsTArray<SVGLength> is fine here. + * SVGUULengthList Needs a better name. +DOMSVGLengthBV::RemovingFromList() +{ + if (mRefCnt > 1) { What's this mRefCnt check for? If it's an optimization, I wouldn't bother. Basically I think the structure is great.

This patch still needs some work but it's pretty close now and is probably a good time for reviewers to start getting their teeth into it (given its size). I've tried to highlight the remaining problems that I'm aware of with XXX flagged comments. These include a couple of design issues/questions and some error handling/reporting issues that I'm unhappy with. Don't worry too much about the lack of tests at this stage, and the existing "object identity" test needs to be overhauled since it assumes an identity model that I have since changed. I have a bunch of ad hock tests that I've been using as I've been writing the code and have yet to turn into automated tests.

Comment on attachment 432293 [details] [diff] [review] patch - has issues; needs testing; ready for initial review pass longsonr, can you review the non-SMIL parts; dholbert, can you review the SMIL parts?

Note that this patch depends on the patch in bug 551299 (although probably nobody else should bother spending time testing ATM until I've done a more thorough job).

A couple of general points: Does it pass all the length lists tests from: http://dev.w3.org/SVG/profiles/1.1F2/errata/implementation-report.html I don't DOM methods need to report to console in general as they will throw to indicate an error.

It does, yes, but I'm sure I still have undiscovered issues to work out. I'll go back over the ReportToConsole comments and reevaluate if a message could add much value or not. In some cases I think it could do, to explain to any clueless users why on earth an exception is being thrown. :-)

> Both of those things depend on us staying alive! Grr! Why? Could we avoid this problem by storing the BaseValDOMSVGLengthList and AnimValDOMSVGLengthList in the element itself, perhaps as node properties? + * This class is used to create the DOM tearoff objects that wrap internal "This is the class of DOM tearoff objects ..." Also, I think this comment it would be good to have a description of all the classes involved in supporting a DOMSVGAnimatedLengthList, and maybe even an ASCII-art diagram showing the references. + * since when script changes those, they now needs to know how to tell the "they now need ..." + * factor is undefined, we return a value of 90 CSS px/inch. 96 probably. Also, you can try walking the nsIDocument::GetParentDocument chain to find a document that has a prescontext; that would make display:none IFRAMEs inherit the value from their parent.

(In reply to comment #10) > > Both of those things depend on us staying alive! Grr! > > Why? Because if the DOMSVGAnimatedLengthList get's GC'ed, it's baseVal, animVal and their items will be disassociated from the element. Actually I just realized that the code to do that doesn't exist...but it should; we shouldn't allow objects from different list tree's to be active for the same attribute at the same time, and if script has a strong ref to something in the old tree, the DOMSVGAnimatedLengthList get's GC'ed, then script tried to access the list again via the element, a second DOMSVGAnimatedLengthList tree will be created, so that's exactly what would happen if we don't disassociate everything in the old tree from the element when it's DOMSVGAnimatedLengthList is CG'ed. Assuming we do the disassociation as I think we would have to, the comment above is true. Basically I think we need to stop the DOMSVGAnimatedLengthList from being GC'ed if its baseVal, animVal and any of their items are still being referenced from somewhere, otherwise those referenced objects would become "randomly" disassociated from the element (whenever there was a GC). > Could we avoid this problem by storing the BaseValDOMSVGLengthList and > AnimValDOMSVGLengthList in the element itself, perhaps as node properties? I don't think so. That wouldn't prevent the DOMSVGAnimatedLengthList from being GC'ed when objects under it still have strong references being held to them. I was wondering if we could do what you suggest, but with the DOMSVGAnimatedLengthList. That only helps if the element's reference to it is a *strong* reference though, and it would create the following strong reference loops: element -> DOMSVGAnimatedLengthList -> element element -> DOMSVGAnimatedLengthList -> DOMSVGLengthList -> element element -> DOMSVGAnimatedLengthList -> DOMSVGLengthList -> DOMSVGLength -> element Hence it would only be a solution if we can accept the consequences of these strong loops (basically we will never free up memory from these DOM wrappers until the document dies (or the elements are removed from the doc tree), even if they only existed due to a script doing a brief sweep over length list attributes in the document). > + * This class is used to create the DOM tearoff objects that wrap internal > > "This is the class of DOM tearoff objects ..." > > Also, I think this comment it would be good to have a description of all the > classes involved in supporting a DOMSVGAnimatedLengthList, and maybe even an > ASCII-art diagram showing the references. Can you suggest a diagrammatic notation, or a tool to create the diagrams and export them to ASCII art? > 96 probably. Also, you can try walking the nsIDocument::GetParentDocument chain > to find a document that has a prescontext; that would make display:none IFRAMEs > inherit the value from their parent. Will the value returned by the prescontexts for parent documents always be the same for their children? If the document in the iframe is zoomed, that's not going to be the case, right?

(In reply to comment #11) > I was wondering if we could do what you suggest, but with the > DOMSVGAnimatedLengthList. That only helps if the element's reference to it is > a *strong* reference though, and it would create the following strong > reference loops: > > element -> DOMSVGAnimatedLengthList -> element > element -> DOMSVGAnimatedLengthList -> DOMSVGLengthList -> element > element -> DOMSVGAnimatedLengthList -> DOMSVGLengthList -> DOMSVGLength -> > element How about this: 1) create a class, let's call it DOMSVGLengthListParts, that contains non-owning pointers to DOMSVGAnimatedLengthList, base and animated DOMSVGLengthList, and their DOMSVGLengths. 2) every element that needs one can have an nsAutoPtr<> to a DOMSVGLengthListParts, which we create lazily 3) DOMSVGAnimatedLengthList, DOMSVGLengthList and DOMSVGLength have strong pointers back to the element, as now 4) when they're destroyed, we null out the reference to them from DOMSVGLengthListParts 5) Whenever you need to get a DOMSVGAnimatedLengthList, DOMSVGLengthList or DOMSVGLength, you get it through the DOMSVGLengthListParts for the element > > + * This class is used to create the DOM tearoff objects that wrap internal > > > > "This is the class of DOM tearoff objects ..." > > > > Also, I think this comment it would be good to have a description of all the > > classes involved in supporting a DOMSVGAnimatedLengthList, and maybe even an > > ASCII-art diagram showing the references. > > Can you suggest a diagrammatic notation, or a tool to create the diagrams and > export them to ASCII art? No ... maybe you could create an SVG diagram, upload it as an attachment to wiki.mozilla.org, and link to it in the comment? > > 96 probably. Also, you can try walking the nsIDocument::GetParentDocument chain > > to find a document that has a prescontext; that would make display:none IFRAMEs > > inherit the value from their parent. > > Will the value returned by the prescontexts for parent documents always be the > same for their children? If the document in the iframe is zoomed, that's not > going to be the case, right? Correct. So you should use the prescontext for the child document if it has one, otherwise walk up to the parent.

Licensing... Did you write this in 2009 as it's now 2010 Also you don't seem to be compliant with this: http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html I'm not sure I understand how if you have a length list with 2 items say and you remove the first item using DOM. Where is the mListIndex value for the second item changed from 1 to 0? Or am I wrong in assuming it needs to be? Really needs tests for that which I know you're intending to do.... Same for adding. Why have nsSVGElement::LengthListAttributesInfo::Reset return an nsresult when it's always NS_OK? Just make it void + // XXX ReportToConsole + // XXX uh, why are we doing this?? Shouldn't we leave this alone if setAttribute fails? What do other UAs do?

Here's my feedback on the SMIL stuff: >diff --git a/content/smil/nsISMILType.h b/content/smil/nsISMILType.h > // We keep the data and type separate rather than just providing different > // subclasses of nsSMILValue as this allows nsSMILValues to be allocated on the > // stack and directly assigned to one another provided performance benefits for > // the animation code. >+// jwatt: isn't the real reason so that sizeof(nsSMILValue) is the same for all >+// value types, allowing us to have a type agnostic nsTArray of nsSMILValue >+// objects? Yes, I believe so. >@@ -84,16 +87,18 @@ protected: >+ // jwatt: Init sounds like we're initializing the nsISMILType. >+ // Rename to InitValue? > virtual nsresult Init(nsSMILValue& aValue) const = 0; Sure, that makes sense. If we do that, we should do the same for nsISMILType::Destroy(), though -- rename it to DestroyValue. >+++ b/content/svg/content/src/DOMSVGLength.cpp >+BaseValDOMSVGLength::SetValue(float aUserUnitValue) [...] >+ if (mElement) {val >+ InternalLength().SetFromUserUnitValue(aUserUnitValue, mElement, GetAxis()); >+ mElement->DidChangeLengthList(mAttrEnum, PR_TRUE); >+#ifdef MOZ_SMIL >+ mElement->AnimationNeedsResample(); >+#endif Elsewhere, (e.g. nsSVGViewBox::SetBaseValue, nsSVGLength2::SetBaseValue), we *only* call AnimationNeedsResample if we have an animated value. Can we do anything like that here? (and elsewhere in this file) The extra calls to AnimationNeedsResample won't cause any harm, I suppose, but they introduce some inefficiency. >+SVGAnimatedLengthList:: >+ SMILAnimatedLengthList::ValueFromString(const nsAString& aStr, [...] >+{ >+ SVGLengthList lengthList; >+ nsresult res = lengthList.SetValueFromString(aStr); >+ if (NS_FAILED(res)) { >+ return res; >+ } Quoting Mozilla Coding Style Guide: "nsresult should be declared as rv. Not res, not result, not foo. rv." https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide#COM_and_pointers >+ nsSMILValue val(&SVGLengthListSMILType::sSingleton); >+ // Check for OOM when the nsSMILValue ctor called SVGLengthListSMILType::Init: >+ if (val.IsNull()) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } See bug 550593 (and perhaps put its current patch in your patch queue below this one) -- infallible malloc means that nsISMILType::Init (and the nsSMILValue() constructor) will never fail anymore. Up until now, that bug was waiting on bug 550611 in order to have Assign() always succeed, too. But I actually just now decided that we'd be better off leaving Assign alone for now (as I just noted in bug 550593 comment 12). So, I'll post a new patch there to only change Init(), and I'll land that ASAP. So anyway, as a heads up: once that lands, you'll want to change SVGLengthListSMILType::Init to return void, and you'll want to remove all the IsNull() checks on freshly constructed nsSMILValues. >+ SVGLengthListAndInfo *llai = static_cast<SVGLengthListAndInfo*>(val.mU.mPtr); >+ res = llai->CopyFrom(lengthList); >+ if (NS_FAILED(res)) { >+ return res; // OOM (just a note): Hm, despite infallible malloc, this looks like this is an OOM situation that we still need to check for, since CopyFrom uses EnsureCapacity which isn't infallible. So this early-return is good. >+ aValue = val; // XXX not checking for OOM on this copy Yeah -- we technically should check for failure there, too, since nsISMILType::Assign (which gets called under the hood) is still fallible. (And in particular, the implementation in SVGLengthListAndInfo::Assign can definitely fail, if its call to CopyFrom fails) I'm not sure if we have a good way of checking whether nsSMILValue::operator()= failed, though... Perhaps we should change that so all Assign() implementations have to set their outparam to have nsSMILNullType on failure? (I'm not sure if that would break anything.) >+ aCanCache = PR_FALSE; // XXX When would this be PR_TRUE??? Firstly, to be clear, it's technically allowable to leave this as-is, with aCanCache always set to PR_FALSE -- that's what we currently do for nsSMILValueType -- but it's nice to do something a little smarter, if we can. Basically, we'd like to set aCanCache to PR_TRUE whenever we can guarantee that the input-string |aStr| doesn't depend on any context for its meaning. Put another way: if |aStr| unambiguously encodes the same nsSMILValue and always will (*and* if the resulting nsSMILValue will always behave the same when passed to Add / Interpolate), then we'd like to set aCanCache to PR_TRUE. In this case, I'm guessing that aCanCache can be PR_TRUE if |aStr| contains only px-units or unitless values. It needs to be PR_FALSE if we've got any em / ex / % values, which is important because e.g. the font or percent-basis could change out from under us, which would change the value that we produce.) >+ SMILAnimatedLengthList(SVGAnimatedLengthList* aVal, >+ nsSVGElement* aSVGElement, >+ PRUint32 aAttrEnum, >+ PRUint8 aAxis) >+ : mVal(aVal) >+ , mElement(aSVGElement) >+ , mAttrEnum(PRUint8(aAttrEnum)) >+ , mAxis(aAxis) >+ {} Looks like we never check that aAttrEnum is a valid PRUint8 -- that seems broken. IMHO... - we should make aAttrEnum a PRUint8 in this constructor - up one level in SVGAnimatedLengthList::ToSMILAttr, we should add a NS_ABORT_IF_FALSE to verify that its argument (a PRUint32) is between 0 and UCHAR_MAX. That function should then do the conversion to PRUint8 as it passes it into the constructor. >+nsresult >+SVGLengthList::CopyFrom(const SVGLengthList& rhs) >+{ >+ // Before we assign, ensure we have sufficient memory: >+ if (!mLengths.SetCapacity(rhs.Length())) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } >+ mLengths = rhs.mLengths; >+ return NS_OK; >+} If you like, this function could be made infallible if it were to use an infallible nsTArray. (from bug 550611) Otherwise, it doesn't really need to return nsresult, does it? Can't it just return PR_TRUE on success, PR_FALSE on failure? (IIRC one goal of deCOM is to remove unnecessary nsresult return values) >+nsresult >+SVGLengthListSMILType::Init(nsSMILValue &aValue) const >+{ >+ NS_ABORT_IF_FALSE(aValue.IsNull(), "Unexpected value type"); >+ >+ SVGLengthListAndInfo* lengthList = new SVGLengthListAndInfo(); >+ if (!lengthList) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } Again, no need to check for OOM here, now that |new| is infallible. (and this needs to return void, once bug 550593 is fixed) Also, just thinking out loud: So, now we have a case where a nsSMILValue effectively has a raw nsIContent pointer, inside of its SVGLengthListAndInfo object. This is slightly sketchy, because there's actually one nsSMILValue that lasts beyond the lifetime of a sample: the cached base value. We hold on to that so we can detect when it hasn't changed (in which case we don't need to recompose, if nothing else has changed). See bug 533291 patch D and nsSMILCompositor::UpdateCachedBaseValue. So it's a little scary that this is an instance where we effectively hold onto a raw nsIContent pointer beyond the sample in which it was created... but I think it actually works out ok, because we check the cached base value against the up-to-date base value (using nsISMILType::IsEqual) before we use the cached value for anything. And if they're not equal (i.e. if we had a stale pointer in the cached value), we'll update the cache and get rid of the old cached value. So this is probably ok. >+nsresult >+SVGLengthListSMILType::Assign(nsSMILValue& aDest, >+ const nsSMILValue& aSrc) const >+{ >+ NS_PRECONDITION(aDest.mType == aSrc.mType, "Incompatible SMIL types."); >+ NS_PRECONDITION(aDest.mType == this, "Unexpected SMIL value."); >+ >+ const SVGLengthListAndInfo* src = >+ static_cast<const SVGLengthListAndInfo*>(aSrc.mU.mPtr); >+ SVGLengthListAndInfo* dest = >+ static_cast<SVGLengthListAndInfo*>(aDest.mU.mPtr); >+ >+ return dest->CopyFrom(*src); >+} If CopyFrom fails (and this returns an error code), do we end up in a consistent state? >+nsresult >+SVGLengthListSMILType::Add(nsSMILValue& aDest, [...] >+ if (dest.Length() != valueToAdd.Length()) { >+ // TODO: There are same cases where we should probably allow 'dest' to >+ // contain fewer items than 'valueToAdd'. For example: >+ // >+ // <text x="100" ...> >+ // <animate attributeName="x" by="0 20 20 20 20" .../> >+ // </text> I think it's fine to just fail on this mismatched-lengths case for now. >+ >+ // XXX nsSVGUtils::ReportToConsole >+ return NS_ERROR_FAILURE; >+ } We don't actually want to directly report to console here, since then we'd spam the console with a report every single sample. (I think you filed a bug on having some way to report per-sample errors only once, right? Might want to mention that bug # instead of ReportToConsole here.) Same for in ComputeDistance and Interpolate. >+ // We use this temporary so 'dest' is untouched if we return an error: >+ SVGLengthListAndInfo result; 'result' is a bit of a confusing name here, when you justapose Add with Interpolate (which are structurally basically the same). In Add, 'result' is a temporary variable that we use to initialize the outparam, but in Interpolate, 'result' *is* the outparam, and we have a *different* temporary variable ('temp') that plays the role that 'result' plays in Add. Might be better to call the temporary variable "temp" in both functions, or perhaps "tempDest" & "tempResult". (named after what we copy them into) >+ if (!result[i].IsValid()) { >+ return NS_ERROR_FAILURE; >+ } What are we worried about here? In particular... - Are we assuming that |dest[i]| and |valueToAdd[i]| are both valid? (If yes, we should probably assert that somewhere, and if no, we should be bailing out earlier, before we've started to do math with dest[i] and valueToAdd[i]) - If dest[i] and valueToAdd[i] are both valid, then shouldn't their combination be valid? >+ return dest.CopyFrom(result); >+} We should assert that this CopyFrom call succeeds, since we've already called dest.SetCapacity(), and AFAIK CopyFrom could only fail on lack-of-capacity-plus-OOM. >+nsresult >+SVGLengthListSMILType::ComputeDistance(const nsSMILValue& aFrom, [...] >+ aDistance = sqrt(total); >+ >+ return NS_ERROR_FAILURE; >+} I think you want NS_OK there. :) >+SVGLengthListSMILType::Interpolate(const nsSMILValue& aStartVal, [...] >+ if (!temp[i].IsValid()) { >+ return NS_ERROR_FAILURE; >+ } See the note above for SVGLengthListSMILType::Add() about the similar chunk there -- that all applies here too. >+ return result.CopyFrom(temp); >+} Same as in Add() -- we should assert that CopyFrom succeeds here, since we've already called SetCapacity to make sure we've got enough space. >@@ -1857,16 +1954,28 @@ nsSVGElement::GetAnimatedAttr(const nsIA >+ return info.mLengthLists[i].ToSMILAttr(this, >+ i, >+ info.mLengthListInfo[i].mAxis); Nit: Couldn't "i" be moved up a line there? :) That's all I've got for now. Generally, looks good!

> So it's a little scary that this is an instance where we effectively hold onto > a raw nsIContent pointer beyond the sample in which it was created... but I > think it actually works out ok, because we check the cached base value against > the up-to-date base value (using nsISMILType::IsEqual) before we use the cached > value for anything. And if they're not equal (i.e. if we had a stale pointer in > the cached value), we'll update the cache and get rid of the old cached value. It's possible that the referenced element is destroyed, and a new element is created with the same address, so that the cached based value and the up-to-date base value compare equal when they really shouldn't. So I think this is still a problem.

(In reply to comment #15) We're actually safe against that, too -- I thought of that, but didn't go into it because my post (and that 'thinking out loud' chunk) was already kind of long. :) We end up being safe against Comment 15 right now, because we have an nsRefPtr<> handle on the target element in nsSMILTargetIdentifier. That structure (stored on the nsSMILAnimationFunction) holds a reference to the target element as long as our animation is targeting something on that element. So if the target (and hence base value) were to change in a particular sample, the target-element would be alive at least until partway through that sample (when we get rid of the old nsSMILTargetIdentifier and lose the reference). And the base value would change during that same sample, too, so the base-value-equality-check would be reliabl -- because even if the base value's pointer is stale, it's only *barely* stale, and we won't have had a chance to construct new content in the memory that it points to. So, nsSMILTargetIdentifier's owning reference saves us here... Still, I don't think that's something we should rely on.

We can solve the problem by adding a strong reference to the nsIContent in SVGLengthListSMILType, I guess.

(In reply to comment #14) > >+ aCanCache = PR_FALSE; // XXX When would this be PR_TRUE??? > > Firstly, to be clear, it's technically allowable to leave this as-is, with > aCanCache always set to PR_FALSE -- that's what we currently do for > nsSMILValueType -- but it's nice to do something a little smarter, if we can. Sorry, I meant 'nsSMILCSSValueType' there (not nsSMILValueType)

Here's another snapshot, mostly for roc's benefit. I've addressed most, but not all of the comments above. I'll post another update shortly, with replies to all the above.

+ ok(threw == true, + 'The method '+t.list_type+'.insertItemBefore() should throw if passed '+ + 'an object of the wrong type.'); Just do ok(threw, ... + ok(t.baseVal.numberOfItems == 4, + 'The '+t.list_type+' object should contain four list items.'); + ok(res === item, + 'The list item returned by '+t.list_type+'.replaceItem() should be '+ + 'the exact same object as the item that was passed to that method, '+ + 'since the item that was passed to that method did not already belong '+ + 'to a list.'); + ok(t.baseVal.getItem(2) === item, + 'The list item at index 2 should be the exact same object as the '+ + 'object that was passed to the '+t.list_type+'.replaceItem() method, '+ + 'since the item that was passed to that method did not already belong '+ + 'to a list.'); + ok(t.baseVal.getItem(3) === old_items[3], + 'The list item that was at index 3 should still be at index 3 after '+ + 'the item at index 2 was replaced using the '+t.list_type+ + '.replaceItem() method.'); these should all use "is"

Comment on attachment 449590 [details] [diff] [review] patch v5 + * "loose" its value from script's perspective on being removed from the list. "lose" +#if 0 + /** + * Performance enhancing class to lookup length context data on demand and + * cache it for reuse with other lengths. + * Create one of these on the stack and pass it into each GetValueInUserUnits + * call when you need to get the user unit length of multiple SVGLength + * objects belonging to the same element. + */ Just remove this #if 0 code? + /* Rational for using nsTArray<SVGLength> and not nsTArray<SVGLength, 1>: Rationale yay!!!

Comment on attachment 449590 [details] [diff] [review] patch v5 >+ * >+ * The only time this method could fail is on OOM when trying to increase the >+ * length of the DOm list. If that happens then this method simply clears the s/DOm/DOM/ >+ >+ /// Get a reference to this DOM wrapper object's internal counterpart. Two / will do to begin :-) >+{ >+#ifdef DEBUG >+ NS_ABORT_IF_FALSE(aList && >+ aAttrEnum < 4194303 && >+ aListIndex < 17 && >+ aIsAnimValItem < 2, "bad arg"); Not immediately clear why these magic numbers although I do understand now. Can you do 2^x or hex or something and/or have a comment here too. >+ if (aIsAnimValItem && >+ mListIndex >= Element()->GetAnimatedLengthList(mAttrEnum)->GetAnimValue().Length() || >+ !aIsAnimValItem && >+ mListIndex >= Element()->GetAnimatedLengthList(mAttrEnum)->GetBaseValue().Length()) { >+ NS_ERROR("Bad aListIndex!"); Could this NS_ERROR also be NS_ABORT...? >+ /** >+ * Create an unowned copy of an owned length. The caller is responsible for >+ * the first AddRef(). >+ */ >+ DOMSVGLength* Copy() { const? >+ NS_ASSERTION(mList, "unexpected caller"); >+ DOMSVGLength *copy = new DOMSVGLength(); >+ SVGLength &length = InternalItem(); >+ copy->NewValueSpecifiedUnits(length.GetUnit(), length.GetValueInCurrentUnits()); >+ return copy; >+ } >+ >+ PRBool IsInList() { >+ return !!mList; >+ } const >+ >+ /** >+ * In future, if this class is used for non-list lengths, this will be >+ * different to IsInList(). >+ */ >+ PRBool HasOwner() { >+ return !!mList; >+ } And const. >+ >+ SVGLength ToSVGLength(); const? >+ >+private: >+ >+ nsSVGElement* Element() { >+ return mList->Element(); >+ } >+ >+ PRUint8 AttrEnum() { >+ return mAttrEnum; >+ } const. >+ /** >+ * Get the axis that this length lies along. This method must only be called >+ * when this object is associated with an element (HasOwner() returns true). >+ */ >+ PRUint8 Axis() { >+ return mList->Axis(); >+ } const? >+NS_IMETHODIMP >+DOMSVGLengthList::InsertItemBefore(nsIDOMSVGLength *newItem, >+ PRUint32 index, >+ nsIDOMSVGLength **_retval) >+{ ... >+ if (domItem->HasOwner()) { >+ domItem = new DOMSVGLength(); Does this handle removing the item from its previous list? Not sure if I see where that happens. Are there tests for removal from previous list? >+NS_IMETHODIMP >+DOMSVGLengthList::ReplaceItem(nsIDOMSVGLength *newItem, >+ PRUint32 index, >+ nsIDOMSVGLength **_retval) >+{ ... >+ if (domItem->HasOwner()) { >+ domItem = new DOMSVGLength(); Does this handle removing the item from its previous list? Not sure if I see where that happens here either. >+ PRUint8 AttrEnum() { >+ return mAList->mAttrEnum; >+ } const? >+ >+ PRUint8 Axis() { >+ return mAList->mAxis; >+ } const >+ >+ /// Used to determine if this list is the baseVal or animVal list. >+ PRBool IsAnimValList() { >+ return this == mAList->mAnimVal; >+ } const >+ >+ PRBool IsAnimating() const { >+ return !!mAnimVal; >+ } See you do know how to do const ;-) >+#ifdef DEBUG >+ PRBool IsValid() { const >+ >+static inline char* SkipWhitespace(char* str) >+{ >+ while (NS_IsAsciiWhitespace(*str)) or IsSVGWhitespace in nsSVGUtils perhaps? >+ ++str; >+ return str; >+} >+ >+nsresult >+SVGLengthList::SetValueFromString(const nsAString& aValue) >+{ >+ SVGLengthList temp; >+ >+ NS_ConvertUTF16toUTF8 value(aValue); >+ char* start = SkipWhitespace(value.BeginWriting()); >+ >+ // We can't use strtok with SVG_COMMA_WSP_DELIM because to correctly handle >+ // invalid input in the form of two commas without a value between them, we >+ // would need to know if strtok overwrote a comma or not. Can dholbert's delimiter tokenizer classes work help here? >+void >+nsSVGElement::GetAnimatedLengthListValues(SVGUserUnitList *aFirst, ...) >+{ >+ LengthListAttributesInfo info = GetLengthListInfo(); >+ >+ // XXX does it matter that we get here with <a>? How does that happen?

dholbert's delimeter tokeniser conversion could be a follow-up.

> How does that happen? I suppose nsSVGAFrame inherits from nsSVGTSpanFrame but it's contents are an nsSVGAElement which does not have the attributes. So you need nsSVGAFrame::GetXY and GetDxDy that do nothing don't you?

jwatt and I discussed animation of length-lists in IRC a bit -- here's my summary of the results of that discussion, with some basic background/explanation below. (correct me if I'm wrong on any of this, jwatt) SUMMARY: (1) We need to add a "can append zeroes" flag to the SVGLengthListAndInfo class (the nsSMILValue.mU.mPtr payload for svg length list values). (1a) We want to set this flag to PR_TRUE in every place where we construct a SVGLengthListAndInfo, except for in the <text>/<tspan> x & y attributes' nsISMILAttr::ValueFromString and ::GetBaseValue methods. (2) For now, we won't support cases like <text><animate attributeName="x" by="10 10 10"/></text> (3) We should still be able to handle cases like <text x="50 60 70"><animate attributeName="x" by="10 10 10"/></text> Also, once we've added this flag: (4) I don't think we actually need to fix bug 570453 (though it wouldn't hurt) BASIC BACKGROUND/EXPLANATION: The "can append zeroes" flag lets us pad values under-the-hood so we can animate between different-length lists in cases like: > <text>ABC <animate attributeName="dx" dur="1" by="5 5" /></text> > <text dx="2">ABC <animate attributeName="dx" dur="1" by="5 5 5" /></text> > <text>ABC <animate attributeName="dx" dur="1" from="5 5 5" to="" /></text> In these cases, it's sensible for Add/ComputeDistance/Interpolate to effectively extend the shorter values with trailing zeroes, since that doesn't change the semantics of the value. (Any dx or dy value can have any number of zeroes appended at the end, without changing the value's meaning.) In contrast, for values of the <text> element's "x" and "y" attributes, it *does* change the meaning of a value if we add trailing zeroes -- so we'd need to turn OFF the "can append zeroes" flag for values used with that attribute. (And as a result, we won't support animation between e.g. x="10" and x="20 40" -- at least for now. Fixing that would require our SMIL code to be able to compute glyph offsets, which seems messy.)

Comment on attachment 449590 [details] [diff] [review] patch v5 I'm assuming I should just look at the SMIL-related code. Comments below: >diff --git a/content/smil/nsISMILType.h b/content/smil/nsISMILType.h >+// subclasses of nsSMILValue. This is so that sizeof(nsSMILValue) is the same >+// for all value types, allowing us to have a type agnostic nsTArray of s/type agnostic/type-agnostic/ (add hyphen) >diff --git a/content/smil/nsSMILValue.h b/content/smil/nsSMILValue.h >+ * case. The nsSMILValue objects obtained from attributes' base values is >+ * cached s/is cached/are cached/ (Thanks for adding this clarifying comment!) >diff --git a/content/svg/content/src/SVGAnimatedLengthList.h b/content/svg/content/src/SVGAnimatedLengthList.h >+#ifdef MOZ_SMIL >+#include "nsISMILAttr.h" >+class nsSMILValue; >+class nsISMILType; >+#endif // MOZ_SMIL No need for those class declarations (nsSMILValue/nsISMILType) -- nsISMILAttr.h already declares those. >diff --git a/content/svg/content/src/SVGAnimatedLengthList.cpp b/content/svg/content/src/SVGAnimatedLengthList.cpp >+nsresult >+SVGAnimatedLengthList:: >+ SMILAnimatedLengthList::ValueFromString(const nsAString& aStr, [...] >+ nsresult rv = llai->SetValueFromString(aStr); >+ if (NS_SUCCEEDED(rv)) { >+ llai->SetInfo(mElement, mAxis); >+ aValue.Swap(val); >+ aCanCache = PR_TRUE; This "aCanCache = PR_TRUE" line is overly optimistic & could prevent us from recomposing when we really should. In particular, it needs to be PR_FALSE whenever we see units whose meaning depends on a piece of context that could change out from under us -- that is, 'em' or 'ex' units (whose meaning could change if the font changes out from under us) or % units (whose meaning could change if the %-basis changes). Below is how we handle this in nsSVGLength2 -- something analogous (extended for lists) should work here. http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGLength2.cpp?mark=538-540#521 >diff --git a/content/svg/content/src/SVGLengthListSMILType.cpp b/content/svg/content/src/SVGLengthListSMILType.cpp >+SVGLengthListSMILType::Destroy(nsSMILValue& aValue) const >+{ >+ NS_PRECONDITION(aValue.mType == this, "Unexpected SMIL value type."); Mega-nit: ideally there shouldn't be a period at the end of the warning message here (since it will have punctuation appended when it's printed to the console). This isn't particularly important, and we aren't fully consistent with this in existing code -- I'm just pointing it out in case you feel like fixing it in your added code. :) (It happens in a number of places.) >+SVGLengthListSMILType::Add(nsSMILValue& aDest, [...] >+ // Ensure we have sufficient memory (note SetCapacity and/vs SetLength!): >+ if (!dest.SetCapacity(valueToAdd.Length()) || >+ !temp.SetLength(valueToAdd.Length())) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } [...] >+ return dest.CopyFrom(temp); // the SetLength() above means this can't fail Nit: In that comment on the final line there -- technically it's the *SetCapacity* (not SetLength) call that keeps this from failing, right? (the SetCapacity call is what pre-allocates enough space in dest, which is what prevents us from failing here.) This applies to the last line of Interpolate(), too. >+SVGLengthListSMILType::ComputeDistance(const nsSMILValue& aFrom, >+ const nsSMILValue& aTo, >+ double& aDistance) const >+{ [...] >+ // We return the root of the sum of the squares of all the user unit >+ // differences between each corresponding length. Useful? Who knows. This comment doesn't inspire confidence. :) This distance-computation method is "useful" as long as it gives us the right behavior in paced-calcMode animation. e.g. if we have a testcase like: > <animate attributeName="x" calcMode="paced" dur="1" values="100 200 300; 101 201 301; 110 210 310" ...> then at halfway through the duration, we should expect to have x="105 205 305". As long as ComputeDistance gives us that behavior, it's useful enough for our purposes. We need a test that checks for this -- it looks like this patch don't have any tests for paced calcMode yet. (though anim-text-x-y-dx-dy-01.svg has a comment indicating that it needs testing) >+ double total = 0; s/0/0.0/ >diff --git a/content/svg/content/src/SVGLengthListSMILType.h b/content/svg/content/src/SVGLengthListSMILType.h >+#include "nsISMILType.h" >+#include "nsTArray.h" Nothing in this header file needs nsTArray, AFAICT -- nix the #include "nsTArray.h" >diff --git a/layout/reftests/svg/smil/anim-text-x-y-dx-dy-01.svg b/layout/reftests/svg/smil/anim-text-x-y-dx-dy-01.svg >+ <!-- One of the things that this file tests is interpolation between lengths >+ of different units. One issue with this type of test is knowing the >+ values to use in the reference file. For example, what length should >+ be used in the reference to comparing against an animation that's mid >+ way between 50px and 10in? The SMIL engine will convert the start >+ length to the unit of the end length and then interpolate, but how >+ many inches is 50px? To get around this problem our tests mainly pair >+ different units with a knows, fixed, relationship. For example, >+ animating between cm and mm, or between 'in' and pt (72 pt/in) or 'in' >+ and pc (6 pc/in). s/knows/known/ Responding to this comment: one other good situation to test is em --> px, after you've set an explicit font-size. For example: you can set "font-size: 10px", and then run an animation from="50px" to="6em", and it should basically be equivalent to animating from 50px to 60px. (or 5em to 6em -- whichever units end up getting used in the final value) >+ <!-- Test calcMode="paced". It really doesn't make any sense to use paced >+ animation with a length list. >+ --> As noted above, this needs tests. :) Also: Aww, c'mon, don't be so down on paced-calcMode -- sure it makes sense to use paced animation with a length list! :) At least, it doesn't make any *less* sense than it does for other attributes. It's just a way of getting constant-speed animation across all of the entries in a values list. (Of course, it's only useful when you've got a values-list with >=3 entries -- otherwise, it makes no difference.) >+ <!-- Test calcMode="discrete". The SMIL engine treats the 'from' and 'to' as >+ two discrete values to animate between. That's possibly not what >+ authors would expect. Your comment here (I've clipped it early) suggests that this isn't a particularly useful or intuitive use case for calcMode="discrete", and I'd generally agree. A slightly more meaningful use-case for calcMode="discrete" is with a values list -- e.g. <animate ... values="A; B; C; D" calcMode="discrete"/> -- which will jump between the various values given. (Here, each one of of A/B/C/D is a length list, of course.) It might be worth testing that sort of case, too, or perhaps replacing your |from="A" to="B"| verison with the equivalent |values="A; B"| formulation, if that seems more sensible. Just an idea, if it makes more sense to you. r=dholbert with the above (though I'd like to see the code for comment 28 too, when it's ready -- maybe that's a good candidate for a separate followup patch?)

(In reply to comment #28) > jwatt and I discussed animation of length-lists in IRC a bit -- here's my > summary of the results of that discussion, with some basic > background/explanation below. (correct me if I'm wrong on any of this, jwatt) Your summary is good, but I'm afraid that the changes we discussed aren't working. I've changed the code to pass the "can pad with zeros" flag through the different layers, but again everything comes unstuck in the SMIL code because of its reliance on being able to just Init() nsSMILValue objects to get a "zero" value for the type in question. Because of this SVGLengthListSMILType's methods are frequently passed nsSMILValue objects that have been created by the SMIL code without regard to the attribute in question. I think we do need to fix bug 570453 I'm afraid.

So if you make Init() create an "paddable-with-zeroes empty-list", and you handle that value correctly in Add/Interpolate/ComputeDistance (see below), I do really think that should keep you from hitting issues like bug 570453. Perhaps you're running into problems because you've made Add/Interpolate/ComputeDistance only support paddable+paddable and nonpaddable+nonpaddable, without also supporting paddable+nonpaddable? If you don't support that last combined case, you will indeed run into issues like bug 570453. In general, paddable+nonpaddable would be non-trivial to work with, but when you know that the paddable one is a "zero" value, it should be easy to handle with a special case. Just treat the paddable-zero as if it were a list whose length matches your nonpaddable value. (I think you should be able to assume that, in any paddable+nonpaddable situation, the paddable parameter is a just-Initted "zero-length paddable-zero" value. So this should be the only case you'd need to handle, for the paddable+nonpaddable combination.)

(In reply to comment #31) > Perhaps you're running into problems because you've made > Add/Interpolate/ComputeDistance only support paddable+paddable and > nonpaddable+nonpaddable, without also supporting paddable+nonpaddable? If you > don't support that last combined case, you will indeed run into issues like bug > 570453. It seems a bit "yuck" to be getting some values passed in with the wrong state, but I think I have this working now. Doing as you suggest above, plus remembering to set the "can pad" flag on the output variable (cough) seems to get all my cases working.

(In reply to comment #32) > Doing as you suggest above, plus remembering to set the "can pad" flag on > the output variable (cough) seems to get all my cases working. Actually, not quite: 'by' animation for 'x'/'y', even with two lists of the same length, will not work.

I've been struggling with some non-determinism causing very infrequent rendering glitches. One source was an uninitialized variable, which was an easy fix once I tracked it down. Much less obvious is the fix for another source I've only just figured out (I hope). The problem goes something like this: occasionally some <text> elements fail to render at all. Turns out that a common factor is having a percentage length in the _from_ list of an animation when the corresponding length in the _to_ list has a different unit. This sometimes causes NaN to make it through to the paint code. The reason for the occasional NaN is because userUnitsPerNewUnit in SVGLength::GetValueInSpecifiedUnit sometimes ends up being zero, which results in that function doing a divide by zero. The reason that userUnitsPerNewUnit is occasionally zero seems to be that sometimes nsSVGSVGElement's mViewportWidth and mViewportHeight are zero. This in turn seems to be because sometimes nsSVGOuterSVGFrame::Reflow has not yet been called, so it hasn't called nsSVGSVGElement::SetViewportSize, and thus mViewportWidth and mViewportHeight have not yet been set. I'm guessing what's happening here is that there's a race between the reflow event that triggers the initial reflow, and the initial SMIL sample. It seems to me that we shouldn't start the SMIL timeline (start sampling) until after we've had our initial reflow.

So currently, we start the SMIL timeline when the <svg> element's "load" event fires. It sounds like you're saying that event can be fired before we've completed our initial reflow (and before we've established a viewport size). Are you sure that's true? Assuming it is true, it seems like we'd hit similar non-determinacy in non-SMIL situations. Consider e.g. <svg width="200" onload="foo()"><rect width="50%"/></svg> where the "foo()" method gets the width (in pixels) of the rect element. The author's intention would be for foo() to get 100px, but given what you say, I'd imagine it'd get "0" every now and then, since the load handler is firing before we're done with our reflow. That seems broken to me -- is that actually what happens? And if it doesn't happen, how do we avoid the non-determinacy in this sort of example?

the methods that foo would call to get the layout all do a layout flush, so that's how that works.

So the question is, can we do a layout flush before starting the SMIL timeline?

(In reply to comment #36) > the methods that foo would call to get the layout all do a layout flush, so > that's how that works. Ok, thanks Robert -- I'd thought that might be happening. (In reply to comment #37) > So the question is, can we do a layout flush before starting the SMIL timeline? Or rather, "Can we do a layout flush **before firing SVGLoad?**" (assuming we keep the SMIL timeline-beginning locked to that event). I think the answer to that question is "yes, but we'd rather not when we can avoid it", since it'd potentially make pageload take longer.

See also bug 552938 comment 33 and onwards. Instead of forcing the initial layout flush to happen earlier, would it make any sense to defer the SVG load events until the initial layout flush? Potential problem: The SVG load events need to defer the document's load event, so if the SVG load events waited for layout flush, documents with SVG in them would always flush layout before the document's load event and presumably reflush soon after. On the face of things, it seems very bad to have DOM behavior tied to layout having happened. Is there any way to decouple the SMIL calculations from layout?

(In reply to comment #35) > So currently, we start the SMIL timeline when the <svg> element's "load" event > fires. It sounds like you're saying that event can be fired before we've > completed our initial reflow (and before we've established a viewport size). > Are you sure that's true? Yes. You had me doubting myself, but I've managed to catch the problem again, and when it occurs we are indeed getting a sample before nsSVGSVGElement::Reflow() has been called, and when it doesn't occur, Reflow() is being called first. It occurs to me that even if we change the point at which we start the timeline, that will not completely eliminate the problem. For example, script may access something that causes a synchronous sample to be taken well before either the timeline starts of the frame has had its initial reflow. For example script could access an animVal property, or it could call setCurrentTime(), both of which force a synchronous sample. Furthermore, in the case of our invalidation reftests where we precede a setCurrentTime() call with a pauseAnimation() call, the bad sample will be frozen. (In reply to comment #39) > Instead of forcing the initial layout flush to happen earlier, would it make > any sense to defer the SVG load events until the initial layout flush? Since that wouldn't eliminate this problem entirely (as explained above), I don't think that's the solution here. However I do think there are other good reasons we should consider doing that (or something like it) - mainly the problems in bug 552938, so let's discuss it there. > On the face of things, it seems very bad to have DOM behavior tied to layout > having happened. Is there any way to decouple the SMIL calculations from > layout? Well getting the number of pixels per percentage unit depends on the dimensions of the viewport element, and the dimensions of the viewport element are only set once it's had an initial reflow. There seem to be three options: 1) don't start the timeline (sampling) until after the initial reflow - unfortunately since script can cause a resample at any time, that doesn't solve the problem 2) we force a synchronous reflow whenever someone asks for the dimensions of a viewport element if it hasn't yet been initialized - but that doesn't work when script is the triggering cause, since we're not allowed to reflow directly under script 3) we allow bad values to be read, but make sure to force a resample as soon as we can so that bad values don't "show" for long I think maybe what we should do is force a full resample when nsSVGOuterSVGFrame objects get their initial reflow, even if their timeline is paused.

(In reply to comment #40) > 2) we force a synchronous reflow whenever someone asks for the dimensions > of a viewport element if it hasn't yet been initialized - but that > doesn't work when script is the triggering cause, since we're not > allowed to reflow directly under script Script is definitely allowed to force a reflow. Lots of DOM methods call FlushPendingNotifications(Flush_Layout).

Hmm, I must have gotten confused with not allowing reflow under paint. So maybe nsSVGSVGElement::SetViewportSize() should do that then.

(In reply to comment #40) > Furthermore, in the case of our > invalidation reftests where we precede a setCurrentTime() call with a > pauseAnimation() call, the bad sample will be frozen. I think invalidation reftests should be fine, right? MozReftestInvalidate only gets fired after we've fully completed our initial reflow & paint, and any samples after that point (e.g. from a scripted setCurrentTime() call) would be dealing with an already-reflowed document.

We usually call pauseAnimations + setCurrentTime on SVGLoad, not on MozReftestInvalidate. And it would be good to allow pauseAnimations + setCurrentTime to be called any time before document load. Anyway, it turns out that the race is masked by another necessary change we (dholbert and I) have been discussing off-bug. Basically for the cases where length units can cause the calculation to go wrong, we actually need to prevent caching of the animation sandwich anyway. This forces the animVal to be recomputed every single time sample, so the bad value is quickly overridden. For a better explanation see the comment in SMILAnimatedLengthList::ValueFromString in the patch coming up. A better fix would be to fix bug 575201 to prevent the bad value being returned at all, but that doesn't block this bug since my tests will now no longer cause random orange.

I believe this patch addresses all review comments so far. For the most part I've just integrated everyone's feedback, but I have a few replies below. (In reply to comment #22) > + ok(t.baseVal.getItem(3) === old_items[3], > + 'The list item that was at index 3 should still be at index 3 after '+ > + 'the item at index 2 was replaced using the '+t.list_type+ > + '.replaceItem() method.'); > > these should all use "is" As discussed with roc in one of our weekly telcons, I only changed the test to use "is" where it seemed to make sense. Specifically when comparing the length of the list to its expected length, since knowing the expected vs actual lengths is useful debugging information. However, as I understand it, that's the only real benefit of "is": it produces a message along the lines of "expected X, but got Y" if the test fails. In the case where we're checking one object is the exact same object as another, that's not very useful, since it just produces error messages along the lines of: blah blah - got [object SVGLength @ 0x19f85de0 (native @ 0x19f85a30)], expected [object SVGLength @ 0x19f85e40 (native @ 0x19f85e20)] That gives you no useful information over what we'd have with the current "ok" checks. "is" also uses == instead of ===. Plus many of the "ok" checks actually check a bunch of state to avoid making the file too unwieldy, so they're not very amenable to conversion to "is" where you can only check one thing at a time. (At least I'd rather not to split them up into tons of little micro checks with their own separate error messages.) For these reasons I didn't converted many of the "ok" checks. (In reply to comment #25) > >+ /// Get a reference to this DOM wrapper object's internal counterpart. > > Two / will do to begin :-) longsonr: Actually that's deliberate; it's the way that you mark a one line comment for Doxygen. I made the const changes you pointed out where they don't have tricky knock-on affects. > Does this handle removing the item from its previous list? Not sure if I see > where that happens. Are there tests for removal from previous list? Deliberately, no. After several conversations going over the arguments for and against inserting vs copying with roc and other implementers, we concluded that a good compromise to meet user expectations while minimizing unexpected behavior is to insert the object itself only if it doesn't belong to a list, and to insert a copy of it if it does. That way anyone using createSVGLength() gets what they expect, but anyone trying to copy lengths from an existing list doesn't have the existing list mutating from under them. The file test_SVGxxxList.xhtml tests these sorts of scenarios to make sure we behave as intended. (In reply to comment #26) > dholbert's delimeter tokeniser conversion could be a follow-up. longsonr: Yes, I'll leave that for now. (In reply to comment #29) > This comment doesn't inspire confidence. :) This distance-computation method > is "useful" as long as it gives us the right behavior in paced-calcMode > animation. > > ... > > Also: Aww, c'mon, don't be so down on paced-calcMode -- sure it makes sense to > use paced animation with a length list! :) At least, it doesn't make any > *less* sense than it does for other attributes. It's just a way of getting > constant-speed animation across all of the entries in a values list. (Of > course, it's only useful when you've got a values-list with >=3 entries -- > otherwise, it makes no difference.) dholbert: I'm now testing 'paced'. Don't get me wrong, paced mode animation is important and very useful for animateMotion. I just don't see the point of it anywhere else. Providing intermediate values is pretty pointless if all you're going to get is linear interpolation between the first and last values anyway, right? Lists may actually be an exception to the "pointless" argument since in the general case the interpolation wouldn't be linear between the first and last values...in those cases though the adjective to describe paced mode animation would probably be "unusable" or "unuseful". Only in the simpler examples such as the one you give in this comment where interpolation will be linear is that not the case, but then we're back to "pointless". :) > It might be worth testing that sort of case, too, or perhaps replacing your > |from="A" to="B"| verison with the equivalent |values="A; B"| formulation, if > that seems more sensible. Just an idea, if it makes more sense to you. I don't think it really makes much difference; I think the important thing is that we test that the switch is made, and at the right time. > r=dholbert with the above (though I'd like to see the code for comment 28 > too, when it's ready -- maybe that's a good candidate for a separate > followup patch?) That's now in the current patch.

(In reply to comment #45) > Don't get me wrong, paced mode animation is important and very useful for > animateMotion. I just don't see the point of it anywhere else. Providing > intermediate values is pretty pointless if all you're going to get is linear > interpolation between the first and last values anyway, right? Ah, good point -- if the values are all monotonically increasing/decreasing, I agree. But when they're *not* monotonically increasing/decreasing (e.g. values="0; 10; 5; 10") it's a bit trickier for an author to get constant-speed animation without using calcMode="paced" You could do it with keyTimes of course, but that's a bit hacky.

Comment on attachment 454465 [details] [diff] [review] patch v6 (r=dholbert, sr=roc) Do reftests and/or mochitests need some license text? Please raise a follow up for considering the tokeniser change.

Our tests generally do not need license text. So this can land? Woohoo!!!

(In reply to comment #48) > Our tests generally do not need license text. (But you're welcome to add such text if you want to.)

Why hasn't this landed?

It has landed, but it's been causing random orange. I'll close it once that's fixed.

If you landed it and didn't back out, at least mention that in the bug with a link to the hg changeset.

The push was http://hg.mozilla.org/mozilla-central/rev/9712aa5f77e1 The bug filed for the orange due to failure of the new reftest anim-text-x-y-dx-dy-01.svg is bug 579588.

I'm closing this bug. The intermittent orange is an intermittent failure in the new functionality rather than a regression, so at this stage I think it's best to just fix that as a followup in bug 579588.