We should add support for SMIL animation of number list attributes. These are: 'rotate' on <text>, <tspan>, <tref> and <altGlyph> 'values' on <feColorMatrix> 'tableValues' on <feFuncA>, <feFuncB>, <feFuncG> and <feFuncR> 'kernelMatrix' on <feConvolveMatrix>

Comment on attachment 493601 [details] [diff] [review] patch +namespace mozilla { +class SVGNumberList; +} Move this to nsISVGChildFrame.h + virtual const mozilla::SVGNumberList *GetRotate(); Add "typedef mozilla::SVGNumberList SVGNumberList;" to nsISVGChildFrame instead and remove the mozilla:: prefixes all over the place. nsSVGTextPositioningElement.h: + mozilla::SVGAnimatedNumberList mNumberListAttributes[1]; I think you can drop the mozilla:: here. + NumberListAttributesInfo info = GetNumberListInfo(); + for (PRUint32 i = 0; i < info.mNumberListCount; i++) { + if (aName == *info.mNumberListInfo[i].mName) { + NS_ABORT_IF_FALSE(i <= UCHAR_MAX, "Too many attributes"); Why aren't we calling GetAnimatedNumberList here? + ( IsAnimValList() ? mAList->mAnimVal : mAList->mBaseVal ) = nsnull; I think using the ?: operator on the LHS of an assignment might be a gcc extension. You might want to make it *(... ? &... : &... ) = nsnull + if (aIndex >= mNumbers.Length()) aIndex = mNumbers.Length(); Multiple lines +SVGAnimatedNumberList:: + SMILAnimatedNumberList::ValueFromString(const nsAString& aStr, + const nsISMILAnimationElement* /*aSrcElement*/, + nsSMILValue& aValue, + PRBool& aPreventCachingOfSandwich) const Fix indent + nsTextFormatter::snprintf(buf, sizeof(buf)/sizeof(PRUnichar), Use NS_ARRAY_LENGTH Otherwise looks good!

Comment on attachment 493601 [details] [diff] [review] patch >+#ifdef DEBUG >+ // These shifts are in sync with the flag member's in the header. >+ NS_ABORT_IF_FALSE(aList && >+ aAttrEnum < (1 << 27) && >+ aListIndex < (1 << 4) && >+ aIsAnimValItem < (1 << 1), "bad arg"); >+ if (aIsAnimValItem && >+ mListIndex >= Element()->GetAnimatedNumberList(mAttrEnum)->GetAnimValue().Length() || >+ !aIsAnimValItem && >+ mListIndex >= Element()->GetAnimatedNumberList(mAttrEnum)->GetBaseValue().Length()) { >+ NS_ABORT_IF_FALSE(0, "Bad aListIndex!"); First argument should be PR_FALSE. In fact given the next comment the whole thing can be an NS_ABORT_IF_FALSE and you should probably create an ifdef DEBUG method out of it as you did for DOMSVGPathSeg. See beginning of bug 522306 comment 4 and bug 522306 comment 7. What you did with DOMSVGPathSeg was fine so go with that. >+ mList = nsnull; Unreachable. See above. These comments are also applicable to DOMSVGLength.cpp! >+ } >+#endif >+} >+ >+DOMSVGNumber::DOMSVGNumber() >+ : mList(nsnull) >+ , mListIndex(0) >+ , mAttrEnum(0) >+ , mIsAnimValItem(0) PR_FALSE >+ NS_ASSERTION(!HasOwner(), "Inserting item that is already in a list"); >+ NS_ASSERTION(mIsAnimValItem && >+ aListIndex < aList->Element()->GetAnimatedNumberList(aAttrEnum)->GetAnimValue().Length() || >+ !aIsAnimValItem && >+ aListIndex < aList->Element()->GetAnimatedNumberList(aAttrEnum)->GetBaseValue().Length(), >+ "mListIndex too big"); Already seen a line that looks like this above. >+ >+ mList = aList; >+ mAttrEnum = aAttrEnum; >+void >+DOMSVGNumber::RemovingFromList() >+{ >+ mValue = InternalItem(); >+ mList = nsnull; >+ mIsAnimValItem = 0; PR_FALSE >+ * >+ * To simplyfy the code we just have this one method for obtaining both s/simplyfy/simplify This was a PathSegList comment too be Daniel. Could you go through everybody's comments in bug 522306, see which are applicable and action to both SVGLengthList and SVGNumberList. > + // We get here if |this| is an nsSVGAElement instance, for example > + return nsnull; We do? That sounds bad. Why is that? > > if (!foundMatch) { >+ // Check for SVGAnimatedNumberList attribute >+ NumberListAttributesInfo numberListInfo = GetNumberListInfo(); >+ for (i = 0; i < numberListInfo.mNumberListCount; i++) { >+ if (aAttribute == *numberListInfo.mNumberListInfo[i].mName) { >+ rv = numberListInfo.mNumberLists[i].SetBaseValueString(aValue); >+ if (NS_FAILED(rv)) { >+ // ReportToConsole We really can do a ReportAttributeParseFailure here can't we? >+ numberListInfo.Reset(i); >+ } >+ foundMatch = PR_TRUE; >+ break; >+ } >+ } >+ } >+ >+ if (!foundMatch) { > // Check for SVGAnimatedPathSegList attribute > if (GetPathDataAttrName() == aAttribute) { > SVGAnimatedPathSegList* segList = GetAnimPathSegList(); > if (segList) { > rv = segList->SetBaseValueString(aValue); > if (NS_FAILED(rv)) { > ReportAttributeParseFailure(GetOwnerDoc(), aAttribute, aValue); > // The spec says we parse everything up to the failure, so we don't >@@ -584,16 +605,30 @@ nsSVGElement::UnsetAttr(PRInt32 aNamespa > DidChangeLengthList(i, PR_FALSE); > foundMatch = PR_TRUE; > break; > } > } > }

(In reply to comment #2) > + NumberListAttributesInfo info = GetNumberListInfo(); > + for (PRUint32 i = 0; i < info.mNumberListCount; i++) { > + if (aName == *info.mNumberListInfo[i].mName) { > + NS_ABORT_IF_FALSE(i <= UCHAR_MAX, "Too many attributes"); > > Why aren't we calling GetAnimatedNumberList here? Do you mean why the need for this extra overloaded GetAnimatedNumberList based on atoms? (I don't see how we could call the other enum based GetAnimatedNumberList from here.) I added this atom based version because nsSVGTextContainerFrame::GetRotate() doesn't know the type of its element, and although I could expose (make public) ROTATE on either nsSVGTextElement or nsSVGTextPositioningElement and use it in nsSVGTextContainerFrame::GetRotate(), there's a slight risk that changes in future version of the spec could result in us having the value of ROTATE becomes different on the two element classes. Besides it seemed like ROTATE should really be a protected implementation detail of elements, and the overhead of the atom based GetAnimatedNumberList isn't really significant so that's what I did. > + ( IsAnimValList() ? mAList->mAnimVal : mAList->mBaseVal ) = nsnull; > > I think using the ?: operator on the LHS of an assignment might be a gcc > extension. You might want to make it *(... ? &... : &... ) = nsnull We're already using this in the length-list code so I think it's okay. > +SVGAnimatedNumberList:: > + SMILAnimatedNumberList::ValueFromString(const nsAString& aStr, > + const nsISMILAnimationElement* /*aSrcElement*/, > + nsSMILValue& aValue, > + PRBool& aPreventCachingOfSandwich) const > > Fix indent Are you talking about the two spaces before the SMILAnimatedNumberList line? SMILAnimatedNumberList is a child class of SVGAnimatedNumberList, and I was trying to flag that nesting with the indent.

(In reply to comment #4) > (In reply to comment #2) > > + NumberListAttributesInfo info = GetNumberListInfo(); > > + for (PRUint32 i = 0; i < info.mNumberListCount; i++) { > > + if (aName == *info.mNumberListInfo[i].mName) { > > + NS_ABORT_IF_FALSE(i <= UCHAR_MAX, "Too many attributes"); > > > > Why aren't we calling GetAnimatedNumberList here? > > Do you mean why the need for this extra overloaded GetAnimatedNumberList based > on atoms? (I don't see how we could call the other enum based > GetAnimatedNumberList from here.) I added this atom based version because > nsSVGTextContainerFrame::GetRotate() doesn't know the type of its element, and > although I could expose (make public) ROTATE on either nsSVGTextElement or > nsSVGTextPositioningElement and use it in nsSVGTextContainerFrame::GetRotate(), > there's a slight risk that changes in future version of the spec could result > in us having the value of ROTATE becomes different on the two element classes. > Besides it seemed like ROTATE should really be a protected implementation > detail of elements, and the overhead of the atom based GetAnimatedNumberList > isn't really significant so that's what I did. > What about making the textcontainerframe getRotate pure virtual and then the concrete classes would implement it with full knowledge of their ROTATE value.

Could do, although that still means exposing internal element stuff, and still carries some risk given our tendency to use frames for elements that they wouldn't seem to apply to.

We've always exposed our external elements to the frames that implement them, but not to other frames in general. We don't do that much of the latter point either, I imagine when we implement altGlyph properly we'll move it off of a tspanframe and that's about all we have that's a bit unusual in the text frame arena isn't it? If you really wanted you could create an altglyphframe I didn't only because it's functionally the same as a tspanframe at the moment.

(In reply to comment #3) > > + // We get here if |this| is an nsSVGAElement instance, for example > > + return nsnull; > > We do? That sounds bad. Why is that? We do. It's not really bad, it just means that I didn't override GetRotate() on nsSVGAFrame, so we end up using nsSVGTextContainerFrame::GetRotate() which calls GetAnimatedNumberList.

I think overriding GetRotate on nsSVGAFrame would be better.

Comment on attachment 493601 [details] [diff] [review] patch r=longsonr providing comment 3 and comment 9 addressed.

(In reply to comment #3) > Could you go through everybody's comments in bug 522306, see which are > applicable and action to both SVGLengthList and SVGNumberList. I hate you. :) Yeah, I've pretty much done that now, and after point-list is done I'm going to go through the lists and make sure they're consistent. In general I've been improving on each successive list type I've worked on, but it didn't seem appropriate to start messing with old list types in the middle of large patches for new types, so this should be done anyway. I've addressed you other comments too other than the one on ROTATE which I'll hold off on until roc has commented.

>diff --git a/content/svg/content/src/DOMSVGNumber.cpp b/content/svg/content/src/DOMSVGNumber.cpp >+#endif >+ Extreme nit: looks like this file has an extra blank line at the end. >diff --git a/content/svg/content/src/DOMSVGNumber.h b/content/svg/content/src/DOMSVGNumber.h >+#define MOZILLA_DOMSVGNUMBER_IID \ >+ { 0x2CA92412, 0x2E1F, 0x4DDB, { 0xA1, 0x6C, 0x52, 0xB3, 0xB5, 0x82, 0x27, 0x0D } } >+ greater-than-80-char line -- I think the convention is to put a newline before the second {, right? >+++ b/content/svg/content/src/SVGNumberList.cpp >+nsresult >+SVGNumberList::SetValueFromString(const nsAString& aValue) >+{ >+ SVGNumberList 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. >+ >+ while (*start != '\0') { >+ char *newStart; >+ float num = float(PR_strtod(start, &newStart)); I think we should actually use nsCharSeparatedTokenizer here, for consistency & to minimize independent "parse a comma-or-whitespace separated list" implementations. See existing usage in e.g. nsSVGPointList::SetValueString(). (Your usage here would actually be simpler than that one, because each number would be independent instead of being a member of an ordered pair.) (Actually, I think we *really* want something like nsCharSeparatedTokenizerTemplate<IsSVGWhitespace> in both places, to make sure we're using the right "is whitespace" checker.) Still reading patch -- just wanted to post what I've gotten so far...

(In reply to comment #4) > (In reply to comment #2) > > + NumberListAttributesInfo info = GetNumberListInfo(); > > + for (PRUint32 i = 0; i < info.mNumberListCount; i++) { > > + if (aName == *info.mNumberListInfo[i].mName) { > > + NS_ABORT_IF_FALSE(i <= UCHAR_MAX, "Too many attributes"); > > > > Why aren't we calling GetAnimatedNumberList here? > > Do you mean why the need for this extra overloaded GetAnimatedNumberList based > on atoms? Actually I meant "given you have GetAnimatedNumberList(nsIAtom*), why not use it here?" > (I don't see how we could call the other enum based > GetAnimatedNumberList from here.) I added this atom based version because > nsSVGTextContainerFrame::GetRotate() doesn't know the type of its element, and > although I could expose (make public) ROTATE on either nsSVGTextElement or > nsSVGTextPositioningElement and use it in nsSVGTextContainerFrame::GetRotate(), > there's a slight risk that changes in future version of the spec could result > in us having the value of ROTATE becomes different on the two element classes. > Besides it seemed like ROTATE should really be a protected implementation > detail of elements, and the overhead of the atom based GetAnimatedNumberList > isn't really significant so that's what I did. I agree with Robert, just move the implementation of GetRotate down to the leaf classes. It's a tiny function anyway.

(In reply to comment #4) > > + ( IsAnimValList() ? mAList->mAnimVal : mAList->mBaseVal ) = nsnull; > > > > I think using the ?: operator on the LHS of an assignment might be a gcc > > extension. You might want to make it *(... ? &... : &... ) = nsnull > > We're already using this in the length-list code so I think it's okay. OK > > +SVGAnimatedNumberList:: > > + SMILAnimatedNumberList::ValueFromString(const nsAString& aStr, > > + const nsISMILAnimationElement* /*aSrcElement*/, > > + nsSMILValue& aValue, > > + PRBool& aPreventCachingOfSandwich) const > > > > Fix indent > > Are you talking about the two spaces before the SMILAnimatedNumberList line? > SMILAnimatedNumberList is a child class of SVGAnimatedNumberList, and I was > trying to flag that nesting with the indent. No, I mean the indentation of the parameters. Either they should be indented all the way to match the first parameter or else you can indent them much less.

Comment on attachment 493659 [details] [diff] [review] patch A few notes on SVGNumberListSMILType::Add: >+SVGNumberListSMILType::Add(nsSMILValue& aDest, [...] >+ if (dest.Length() != valueToAdd.Length()) { >+ // Allow addition to "identity" dest: >+ if (dest.Length() == 0 && !dest.Element()) { >+ dest.SetInfo(valueToAdd.Element()); >+ return dest.CopyFrom(valueToAdd); Do we really need to check "dest.Length() == 0" here, or is dest.Element() the real signal that it's "identity"? (I'm guessing we can just rely on dest.Element()'s nullness.) Also: dest.CopyFrom() isn't quite right here -- that's only valid when aCount == 1. For larger values of aCount, we need to multiply dest's list-entries by aCount. I think the code in its current state will break with a testcase like e.g. <animate by="..." repeatCount="5" accumulate="sum"/>. (might be good to include a reftest like that to catch this sort of thing) >+ for (PRUint32 i = 0; i < dest.Length(); ++i) { >+ dest[i] += aCount * valueToAdd[i]; >+ } >+ >+ dest.SetInfo(valueToAdd.Element()); >+ return NS_OK; RE this SetInfo call -- I'd expect that the Element() pointers should already match at this point (unless one of them is nsnull, --> identity). Is that correct? If so, we should probably assert that somewhere. Actually -- I think this whole method would be better structured if we shifted the "identity" cases up to the top. That way, we'll no longer need the final SetInfo() call (IIUC) and we can simplify the "Element()s match" assertion that I suggested above. Like so (some pseudocode interspersed): { // ADDING THE IDENTITY VALUE if (valueToAdd is identity) { Assert that valueToAdd has 0 length; return NS_OK; } // ADDING TO THE IDENTITY VALUE if (dest is identity) { Assert that dest has 0 length; dest.SetInfo(valueToAdd.Element()); if (!dest.CopyFrom(valueToAdd)) { return NS_ERROR_FAILURE; } for (each value in dest's list) { value = value * aCount; } return NS_OK; } NS_ABORT_IF_FALSE(dest.Element() == valueToAdd.Element(), "adding values from different elements...?"); // NON-IDENTITY, WITH LENGTH MISMATCH if (lengths don't match) { return NS_ERROR_FAILURE; } // GENERAL CASE for (PRUint32 i = 0; i < dest.Length(); ++i) { dest[i] += aCount * valueToAdd[i]; } return NS_OK; } (Ok, done picking apart Add :) On to a few other nits...) >+nsresult >+SVGNumberListSMILType::ComputeDistance(const nsSMILValue& aFrom, >+ double total = 0.0; [...] >+ float distance = sqrt(total); >+ if (!NS_FloatIsFinite(distance)) { >+ return NS_ERROR_FAILURE; >+ } >+ aDistance = distance; s/float distance/double distance/ -- no reason to drop precision here, right? >@@ -1232,38 +1218,35 @@ nsSVGFEColorMatrixElement::Filter(nsSVGF [...] > float colorMatrix[20]; [...] >- if (num != 20) >+ if (values.Length() != 20) > return NS_ERROR_FAILURE; While you're touching this -- perhaps we could introduce a #define in this file, to get rid of this "20" magic number? (maybe NUM_ENTRIES_IN_4x5_MATRIX?) Won't hold you to that, but it'd make me happy. :) >diff --git a/content/svg/content/src/nsSVGSVGElement.cpp b/content/svg/content/src/nsSVGSVGElement.cpp >- return NS_NewSVGNumber(_retval); >+ NS_IF_ADDREF(*_retval = new DOMSVGNumber()); >+ return NS_OK; This can be NS_ADDREF, right? (since there's no chance that "new DOMSVGNumber" will return null) r=dholbert with the above & comment 12 addressed.

Comment on attachment 493659 [details] [diff] [review] patch >+void >+nsSVGComponentTransferFunctionElement::DidAnimateNumberList(PRUint8 aAttrEnum) >+{ >+ // We don't have a frame, so use our parent's >+ nsCOMPtr<nsIDOMSVGFEComponentTransferElement> compTrans = >+ do_QueryInterface(GetParent()); >+ if (compTrans) { >+ // nsSVGLeafFrame doesn't implement AttributeChanged. >+ nsIFrame* frame = static_cast<nsSVGFE*>(GetParent())->GetPrimaryFrame(); Also: per the end of bug 615658 comment 11, could we assert that this->GetPrimaryFrame() is null, at the beginning of this function? (Or, alternately, use our own frame, if we end up adding one in bug 615658).

dholbert: I've addressed your comments I believe, but I'd appreciate if you'd take a last look. roc: (In reply to comment #13) > Actually I meant "given you have GetAnimatedNumberList(nsIAtom*), why not use > it here?" Oops - I was reading the wrong lines. So, I could do this, but then I'd have to remove the NS_ABORT_IF_FALSE in GetAnimatedNumberList, and I'd like to keep that. Given that (and that it's only a few lines of code duplication) I'll leave things as they are unless you say otherwise. (In reply to comment #14) > > > +SVGAnimatedNumberList:: > > > + SMILAnimatedNumberList::ValueFromString(const nsAString& aStr, > > > + const nsISMILAnimationElement* /*aSrcElement*/, > > > + nsSMILValue& aValue, > > > + PRBool& aPreventCachingOfSandwich) const > > No, I mean the indentation of the parameters. Either they should be indented > all the way to match the first parameter or else you can indent them much less. I indented those params to keep the widest item just inside the 80 column limit, and aligned the other wrapped params with the left side of that one. Indenting them all the way to align with the first param would take me over the 80 columns limit, and indenting them only a few spaces seems horrible to me since it puts the params on the left (class/method) side of the page rather than on the right (param) side of the page. How many spaces do you want them indented by?

Comment on attachment 494919 [details] [diff] [review] patch I guess this is sufficiently done to request approval. Passed try before the recent changes, and doing another run now to make sure that hasn't changed.

Comment on attachment 494919 [details] [diff] [review] patch OK

Comment on attachment 494919 [details] [diff] [review] patch Discussed in IRC, but just for the record, just 2 nits: >+SVGNumberListSMILType::Add(nsSMILValue& aDest, >+ // "Identity" value varies widely for different number list attributes, and >+ // often depend on the value of other attributes on the same element: Delete this no-longer-valid comment. >+++ b/content/svg/content/src/nsSVGFilters.cpp >+ float colorMatrix[NUM_ENTRIES_IN_4x5_MATRIX]; [...] >+++ b/layout/svg/base/src/nsSVGUtils.h >+#define NUM_ENTRIES_IN_4x5_MATRIX 20 This #define should probably just go in the .cpp file, since that's the only place it's used. r=dholbert. Yay, ready to land! :)

Pushed http://hg.mozilla.org/mozilla-central/rev/4e49d32a5216