602759 - SVG SMIL: Implement SVGAnimatedTransformList properly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

When we implemented animated transform list for <animateTransform> I don't think we did it properly from a DOM point of view.

For a start, the animVal is not read-only:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGAnimatedTransformList.h#87

I think there were other outstanding issues regarding liveness. From memory, I think we thought the matrices should not be live but everything above that should be however I notice in SVG 1.1F2 it's said to be live:
http://www.w3.org/TR/SVG/coords.html#__svg__SVGTransform__matrix

Currently it looks like our implementation makes the matrix live.

So I think the hierarchy is something like this:
SVGAnimatedTransformList<>-(2x)SVGTransformList<>-(nx)SVGTransform<>-(1x)SVGMatrix

And it all needs to be live.

Some other issues include:
* Do we really need to keep two independent data structures when the transform is not animated? (I think that's what we're doing now)
* If we don't, we need to be careful about object identities (i.e. use the tearoff map or something similar)

I'm sure jwatt, rlongson and co. will know much better what's still required here based on their work on other animated length types.

Comment 1

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

We should do this too

Comment 2

[Cameron McCormack (:heycam)]

This prevents two SVG 1.1 test suite tests from passing:

http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObjectMiniApproved/types-dom-02-f.html
http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObjectMiniApproved/types-dom-07-f.html

Comment 3

[Brian Birtles (:birtles)]

roc suggested I take this one. Any objections, wip patches etc?

Comment 4

[Jonathan Watt [:jwatt]]

No objections. Last I talked to roc he said he was going to whip up a patch, so maybe he has something wip? I'm afraid I don't.

Comment 5

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

I don't :-)

Comment 6

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

I really just want this fixed so that Santa's Workshop is fast :-)
http://ie.microsoft.com/testdrive/Performance/SantasWorkshop/Default.html

Comment 7

[Brian Birtles (:birtles)]

Stealing from Jonathan.

Comment 8

[Brian Birtles (:birtles)]

Attachment: Part 1 - Refactor SVG list tests to correct assumptions about SMIL

Comment 9

[Brian Birtles (:birtles)]

Attachment: Part 2 - Add tests for transform lists

Comment 10

[Brian Birtles (:birtles)]

Attachment: Part 3 - rewrite nsSVGMatrix and use gfxMatrix instead

Comment 11

[Brian Birtles (:birtles)]

Attachment: Part 4 - Update DOMSVGPoint to use gfxMatrix

Comment 12

[Brian Birtles (:birtles)]

Attachment: Part 5 - Refactor nsSVGTransform into SVGTransform and DOMSVGTransform

Comment 13

[Brian Birtles (:birtles)]

Attachment: Part 6 - Refactor nsSVGTransformList into SVGTransformList and DOMSVGTransformList

Comment 14

[Brian Birtles (:birtles)]

Attachment: Part 7 - Refactor nsSVGAnimatedTransformList into SVGAnimatedTransformList and DOMSVGAnimatedTransformList and incorporate nsSVGTransformSMILAttr

Comment 15

[Brian Birtles (:birtles)]

Attachment: Part 8 - Move nsSVGSMILTransform to SVGTransform

Comment 16

[Brian Birtles (:birtles)]

Attachment: Part 9 - Refactor nsSVGTransformSMILType to use SVGTransformSMILData

Comment 17

[Brian Birtles (:birtles)]

Attachment: Part 10 - Update SVG elements to use new SVG transform types

Comment 18

[Brian Birtles (:birtles)]

Attachment: Part 11 - Update nsSVGTransformListParser to use new SVG transform types

Comment 19

[Brian Birtles (:birtles)]

Attachment: Part 12 - Update DOM bindings to use new SVG transform types

Comment 20

[Brian Birtles (:birtles)]

Attachment: Part 13 - Update layout to use new matrix and transform types

Comment 21

[Brian Birtles (:birtles)]

Attachment: Part 14 - Mark tests that now pass as such

Comment 22

[Brian Birtles (:birtles)]

Attachment: Part 15 - Rename nsSVGTransformListParser as SVGTransformListParser

Comment 23

[Brian Birtles (:birtles)]

The above patch queue should address the current shortcomings in our implementation of SVGAnimatedTransformList and friends.

Notably:
* Read-only animated counterparts now behave as such
* As with the other list types, now uses lightweight internal versions of the objects when DOM wrappers are not required
* nsSVGMatrix is now gone (along with all the conversion routines) and we now use gfxMatrix internally for everything
* A few other minor correctness fixes (e.g. SVGTransformList::consolidate should always produce a single item of matrix type)

Comment 24

[Jonathan Watt [:jwatt]]

Ideally we would be able to successfully build each successive patch and have all the tests pass so that it's much easier to track down which code changes cause regressions. It would be too much work to rework this patch series to do that now though, so I'm just going to proceed with review. :) Besides, I didn't even break up my list-type patches, so you're going one better than me. ;)

Comment 25

[Robert Longson [:longsonr]]

Comment on attachment 543860 [details] [diff] [review]
Part 10 - Update SVG elements to use new SVG transform types

There aren't any old base types left now so nsSVGElement::ResetOldStyleBaseType can go, as can

  // Now check for one of the old style basetypes going away
  nsCOMPtr<nsISVGValue> svg_value = GetMappedAttribute(aNamespaceID, aName);

  if (svg_value) {
    mSuppressNotification = PR_TRUE;
    ResetOldStyleBaseType(svg_value);
    mSuppressNotification = PR_FALSE;
  }


> // Check for SVGAnimatedTransformList attribute

This code should go in the the viewbox, aspectratio and class bit under the one !foundMatch test.

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 543853 [details] [diff] [review]
Part 3 - rewrite nsSVGMatrix and use gfxMatrix instead

r=jwatt, but please either keep the comment at the top of the header and fix up the last bit starting "Since the SVG implementation...", or else file a bug on me to reinstate a suitable comment somewhere after this bug is fixed. Also you have indented some lines by six spaces instead of four when doing NS_ADDREF() - please use four since I don't think we indent by extra like that anywhere else.

Comment 27

[Brian Birtles (:birtles)]

(In reply to comment #6)
> I really just want this fixed so that Santa's Workshop is fast :-)
> http://ie.microsoft.com/testdrive/Performance/SantasWorkshop/Default.html

A quick comparison between a nightly and a TryServer build with this patch applied on my machine (Win 7):

Nightly:
In 1 minute: 11 presents packed, max elves 1

With patch:
In 1 minute: 79 presents packed, max 8~10 elves. After about 50s drops back to 1 elf

Chrome 12:
In 1 minute: 243 presents packed, max 32 elves. After about 2.5min the no. of elves starts dropping. By about 3 minutes there is only 1 elf.

In each case, I've waited 15 seconds after loading the test case before clicking the "start" button. This seems to make considerable difference.

So we've still got a way to go but these patches seem to be pointing us in the right direction.

Comment 28

[Brian Birtles (:birtles)]

(In reply to comment #24)
> Ideally we would be able to successfully build each successive patch and
> have all the tests pass so that it's much easier to track down which code
> changes cause regressions. It would be too much work to rework this patch
> series to do that now though, so I'm just going to proceed with review. :)
> Besides, I didn't even break up my list-type patches, so you're going one
> better than me. ;)

Yeah, I definitely agree. I looked into reworking the patches to make them more-or-less atomic but I didn't think the effort was warranted (would take an extra 2~3 weeks given I'm only working a few mornings a week and am often busy fixing crashers recently.) So I just split the patches fairly roughly to make reviewing easier.

Comment 29

[Brian Birtles (:birtles)]

Attachment: Part 10 - Update SVG elements to use new SVG transform types, v1b

(In reply to comment #25)
> Comment on attachment 543860 [details] [diff] [review] [review]
> Part 10 - Update SVG elements to use new SVG transform types
> 
> There aren't any old base types left now so
> nsSVGElement::ResetOldStyleBaseType can go...

Great! Thanks Robert! Deleting code is my favourite kind of programming. :)

> This code should go in the the viewbox, aspectratio and class bit under the
> one !foundMatch test.

Great, fixed.

Comment 30

[Brian Birtles (:birtles)]

Attachment: Part 3 - Rewrite nsSVGMatrix and use gfxMatrix instead, v1b, r=jwatt

(In reply to comment #26)
> r=jwatt, but please either keep the comment at the top of the header and fix
> up the last bit starting "Since the SVG implementation..."

Actually, I re-read it a few times and I think it's still accurate. We do represent SVG transforms (well, their matrices anyway) using gfxMatrix but we store them in SVG order. And we do pre-multiply them. So I think it's still accurate.

> Also you have indented some lines by six spaces instead of four when doing
> NS_ADDREF() - please use four since I don't think we indent by extra like
> that anywhere else.

Sorry about that. Turns out to be c-indent's default setting for line breaks within parentheses. ("set cino=(1s" fixes it)

Comment 31

[Robert Longson [:longsonr]]

Comment on attachment 544106 [details] [diff] [review]
Part 10 - Update SVG elements to use new SVG transform types, v1b

>+#include "SVGAnimatedTransformList.h"
> #include "nsIDOMSVGUnitTypes.h"
> #include "nsIDOMSVGPointList.h"
> #include "nsIDOMSVGAnimatedPoints.h"
> #include "nsIDOMSVGTransformList.h"
> #include "nsIDOMSVGAnimTransformList.h"
> #include "nsIDOMSVGAnimatedRect.h"
> #include "nsIDOMSVGGradientElement.h"
> #include "nsIDOMSVGPatternElement.h"

Some/all of the above includes can go too can't they?

> #include "nsSVGRect.h"
> #include "nsIFrame.h"
> #include "prdtoa.h"
> #include <stdarg.h>
> #ifdef MOZ_SMIL
> #include "nsSMILMappedAttribute.h"
>-#include "nsSVGTransformSMILAttr.h"
>-#include "nsSVGAnimatedTransformList.h"
> #include "SVGMotionSMILAttr.h"
> #include "nsIDOMSVGTransformable.h"

Is nsIDOMSVGTransformable needed either?

> /* readonly attribute nsIDOMSVGAnimatedTransformList gradientTransform; */
> NS_IMETHODIMP nsSVGGradientElement::GetGradientTransform(nsIDOMSVGAnimatedTransformList * *aGradientTransform)
> {
>-  if (!mGradientTransform && NS_FAILED(CreateTransformList()))
>+  if (!mGradientTransform &&
>+      !(mGradientTransform = new SVGAnimatedTransformList()))
>     return NS_ERROR_OUT_OF_MEMORY;


Malloc is infallible now. Just call nsSVGGradientElement::GetAnimatedTransformList here.

(Same for patterns)

> 
>-  *aGradientTransform = mGradientTransform;
>-  NS_IF_ADDREF(*aGradientTransform);
>-  return NS_OK;
>+  *aGradientTransform =
>+    DOMSVGAnimatedTransformList::GetDOMWrapper(mGradientTransform, this).get();
>+  return *aGradientTransform ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

Can this fail? If it can it can't be beacuse we're out of memory.

>diff --git a/content/svg/content/src/nsSVGGraphicElement.cpp b/content/svg/content/src/nsSVGGraphicElement.cpp
>--- a/content/svg/content/src/nsSVGGraphicElement.cpp
>+++ b/content/svg/content/src/nsSVGGraphicElement.cpp
>@@ -34,28 +34,30 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsSVGGraphicElement.h"
> #include "nsSVGSVGElement.h"
>-#include "nsSVGTransformList.h"
>-#include "nsSVGAnimatedTransformList.h"
>+#include "DOMSVGAnimatedTransformList.h"
>+#include "DOMSVGAnimatedTransformList.h"

An include that's so good it's included twice!

> /* nsIDOMSVGMatrix getCTM (); */
> NS_IMETHODIMP nsSVGGraphicElement::GetCTM(nsIDOMSVGMatrix * *aCTM)
> {
>   gfxMatrix m = nsSVGUtils::GetCTM(this, PR_FALSE);
>-  *aCTM = m.IsSingular() ? nsnull : NS_NewSVGMatrix(m).get();
>+  *aCTM = m.IsSingular() ? nsnull : new DOMSVGMatrix(m);
>+  NS_IF_ADDREF(*aCTM);

if (m.IsSingular) {} with NS_ADDREF would be simpler and more efficient.

>   return NS_OK;
> }
> 
> /* nsIDOMSVGMatrix getScreenCTM (); */
> NS_IMETHODIMP nsSVGGraphicElement::GetScreenCTM(nsIDOMSVGMatrix * *aCTM)
> {
>   gfxMatrix m = nsSVGUtils::GetCTM(this, PR_TRUE);
>-  *aCTM = m.IsSingular() ? nsnull : NS_NewSVGMatrix(m).get();
>+  *aCTM = m.IsSingular() ? nsnull : new DOMSVGMatrix(m);
>+  NS_IF_ADDREF(*aCTM);

ditto.

>-NS_IMETHODIMP nsSVGGraphicElement::GetTransform(nsIDOMSVGAnimatedTransformList * *aTransform)
>+NS_IMETHODIMP nsSVGGraphicElement::GetTransform(
>+    nsIDOMSVGAnimatedTransformList **aTransform)
> {
>-  if (!mTransforms && NS_FAILED(CreateTransformList()))
>+  if (!mTransforms && !(mTransforms = new SVGAnimatedTransformList()))
>     return NS_ERROR_OUT_OF_MEMORY;

Call nsSVGGraphicElement::GetAnimatedTransformList instead.

>-      
>-  *aTransform = mTransforms;
>-  NS_ADDREF(*aTransform);
>-  return NS_OK;
>+
>+  *aTransform =
>+    DOMSVGAnimatedTransformList::GetDOMWrapper(mTransforms, this).get();
>+  return *aTransform ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

Same as above.

> NS_IMETHODIMP nsSVGPatternElement::GetPatternTransform(nsIDOMSVGAnimatedTransformList * *aPatternTransform)
> {
>-  if (!mPatternTransform && NS_FAILED(CreateTransformList()))
>+  if (!mPatternTransform &&
>+      !(mPatternTransform = new SVGAnimatedTransformList()))
>     return NS_ERROR_OUT_OF_MEMORY;

As above.

> 
>-  *aPatternTransform = mPatternTransform;
>-  NS_IF_ADDREF(*aPatternTransform);
>-  return NS_OK;
>+  *aPatternTransform =
>+    DOMSVGAnimatedTransformList::GetDOMWrapper(mPatternTransform, this).get();
>+  return *aPatternTransform ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

Ditto.

> /* nsIDOMSVGMatrix getCTM (); */
> NS_IMETHODIMP
> nsSVGSVGElement::GetCTM(nsIDOMSVGMatrix * *aCTM)
> {
>   gfxMatrix m = nsSVGUtils::GetCTM(this, PR_FALSE);
>-  *aCTM = m.IsSingular() ? nsnull : NS_NewSVGMatrix(m).get();
>+  *aCTM = m.IsSingular() ? nsnull : new DOMSVGMatrix(m);
>+  NS_IF_ADDREF(*aCTM);

if (m.IsSingular()) {}

>   return NS_OK;
> }
> 
> /* nsIDOMSVGMatrix getScreenCTM (); */
> NS_IMETHODIMP
> nsSVGSVGElement::GetScreenCTM(nsIDOMSVGMatrix **aCTM)
> {
>   gfxMatrix m = nsSVGUtils::GetCTM(this, PR_TRUE);
>-  *aCTM = m.IsSingular() ? nsnull : NS_NewSVGMatrix(m).get();
>+  *aCTM = m.IsSingular() ? nsnull : new DOMSVGMatrix(m);
>+  NS_IF_ADDREF(*aCTM);

ditto

r=longsonr with comments above addressed

Comment 32

[Jonathan Watt [:jwatt]]

Comment on attachment 543855 [details] [diff] [review]
Part 5 - Refactor nsSVGTransform into SVGTransform and DOMSVGTransform

Review of attachment 543855 [details] [diff] [review]:
-----------------------------------------------------------------

Here's a first pass on this patch.

::: content/svg/content/src/nsSVGTransform.cpp
@@ +86,5 @@
> +                                 PRUint8 aIsAnimValItem)
> +  : mList(aList)
> +  , mListIndex(aListIndex)
> +  , mIsAnimValItem(aIsAnimValItem)
> +  , mValue(nsnull)

Can you call this mTransform?

@@ +102,5 @@
> +  : mList(nsnull)
> +  , mListIndex(0)
> +  , mIsAnimValItem(PR_FALSE)
> +  , mValue(new SVGTransform()) // Default ctor initialises to matrix type
> +                               // with identity matrix

s/Default ctor/ctor for objects not in a list/ please. (We can see it's the default ctor, but what's maybe not immediately apparent is that it's only used for objects not in a list.)

@@ +109,3 @@
>  }
>  
> +DOMSVGTransform::DOMSVGTransform(const DOMSVGTransform &aCopy)

Can you s/aCopy/aOther/? The copy is the object we're creating to my mind. :)

@@ +109,5 @@
>  }
>  
> +DOMSVGTransform::DOMSVGTransform(const DOMSVGTransform &aCopy)
> +  : mList(aCopy.mList)
> +  , mListIndex(aCopy.mListIndex)

I don't think it's good to assign the list here. That should be handled by the code that handles insertion into the list, no? Same idea on the list index - we're not in a list yet, so we don't have an index. We don't want two objects floating around with the same index in the same list.

@@ +237,4 @@
>    NS_ENSURE_FINITE(angle, NS_ERROR_ILLEGAL_VALUE);
>  
> +  nsresult rv = Transform().SetSkewX(angle);
> +  NS_ENSURE_SUCCESS(rv,rv);

Don't use NS_ENSURE_SUCCESS here, since that uses NS_WARNING which is for warning about potential programming errors, not bad user input. Just use an |if|.

Same in SetSkewY.

@@ +266,5 @@
> +DOMSVGTransform::InsertingIntoList(DOMSVGTransformList *aList,
> +                                   PRUint32 aListIndex,
> +                                   PRUint8 aIsAnimValItem)
> +{
> +  NS_ASSERTION(!HasOwner(), "Inserting item that is already in a list");

Try and avoid NS_ASSERTION in new code, since it's better if these things are fatal so they don't go unnoticed. Use NS_ABORT_IF_FALSE instead.

@@ +316,5 @@
> +#endif // DEBUG
> +
> +
> +//----------------------------------------------------------------------
> +// Interface for DOMSVGMatrix

For "DOMSVGMatrix's use".

::: content/svg/content/src/nsSVGTransform.h
@@ +77,5 @@
> +   * Generic ctor for DOMSVGTransform objects that are created for an attribute.
> +   */
> +  DOMSVGTransform(DOMSVGTransformList *aList,
> +                  PRUint32 aListIndex,
> +                  PRUint8 aIsAnimValItem);

aIsAnimValItem should be PRBool. I know you copied this from one of the other classes, but the newer ones have switched to PRBool. I should file a followup to fix the classes that still use PRUint8.

This comment applies elsewhere too.

@@ +105,5 @@
> +    // Our matrix tear-off pointer should be cleared before we are destroyed
> +    // (since matrix tear-offs keep an owning reference to their transform, and
> +    // clear the tear-off pointer themselves if unlinked).
> +    NS_ABORT_IF_FALSE(!mMatrixTearoff, "Matrix tear-off pointer not cleared."
> +        " Transform being destroyed before matrix?");

Put a line break in here? :)

@@ +118,5 @@
> +  /**
> +   * Create an unowned copy of an owned transform. The caller is responsible for
> +   * the first AddRef().
> +   */
> +  DOMSVGTransform* Copy() {

Newer list classes renamed this to Clone, which you should use too.

@@ +166,5 @@
> +   * "lose" its value on being removed.)
> +   */
> +  void RemovingFromList();
> +
> +  // Interface for DOMSVGMatrix

For "DOMSVGMatrix's use"

@@ +170,5 @@
> +  // Interface for DOMSVGMatrix
> +  const PRBool IsAnimVal() const { return mIsAnimValItem; }
> +  const gfxMatrix& Matrix() const { return Transform().Matrix(); }
> +  void SetMatrix(const gfxMatrix& aMatrix);
> +  void ClearMatrixTearoff(DOMSVGMatrix* aMatrix);

I think it would be really bad for ClearMatrixTearoff to be called by anything other than the tearoff. I think it would be better to make this method private and make DOMSVGMatrix a friend. It's the approach that's been taken in the other list classes.

@@ +197,5 @@
>    }
> +  SVGTransform& Transform() {
> +    return HasOwner() ? InternalItem() : *mValue;
> +  }
> +  void UpdateList();

Can you call this NotifyListOfChange or something? The list itself isn't being updated (we've already done that effectively), it just needs to send out invalidation calls.

@@ +215,5 @@
> +  // with any particular list and thus, no internal SVGTransform object. In
> +  // that case we allocate an SVGTransform object on the heap to store the data.
> +  nsAutoPtr<SVGTransform> mValue;
> +
> +  // Pointer to DOMSVGMatrix tearoff. The DOMSVGMatrix object will take of

s/Pointer/Weak ref/ please.

::: content/svg/content/src/SVGTransform.cpp
@@ +52,5 @@
> +  PRUnichar buf[256];
> +
> +  switch (mType) {
> +    case nsIDOMSVGTransform::SVG_TRANSFORM_TRANSLATE:
> +      {

Style in the other classes is to have the case and curly brackets indented to the same level as the switch statement. In fact you don't even need the curly brackets in these cases, but I guess I don't care about that too much.

@@ +121,5 @@
> +  mType    = nsIDOMSVGTransform::SVG_TRANSFORM_MATRIX;
> +  mMatrix  = aMatrix;
> +  mAngle   = 0.f;
> +  mOriginX = 0.f;
> +  mOriginY = 0.f;

Not sure it's worth setting the members that shouldn't ever be used given the value mType is being set to. Just seems like extra code to load and cycles to me. :)

::: content/svg/content/src/SVGTransform.h
@@ +56,5 @@
> +    mAngle(0.f),
> +    mOriginX(0.f),
> +    mOriginY(0.f),
> +    mType(nsIDOMSVGTransform::SVG_TRANSFORM_MATRIX)
> +  { }

Can you use the same style as the other list classes for consistency? That is indent the ":" by two more spaces, put the commas under the colon, and have no space between the {}?

Same request for all the other classes in their respective patches. It just helps when comparing different classes side by side for divergence, and we've already had at least one round of patches for the previously updated classes to do this.

@@ +71,5 @@
> +    return mType == rhs.mType &&
> +      MatricesEqual(mMatrix, rhs.mMatrix) &&
> +      mAngle == rhs.mAngle &&
> +      mAngle == rhs.mOriginX &&
> +      mAngle == rhs.mOriginY;

Bug on the proceeding two lines.

@@ +76,5 @@
> +  }
> +
> +  void GetValueAsString(nsAString& aValue) const;
> +
> +  float Angle() const { return mAngle; }

I prefer implementations to be put on a new line to make it stand out that the implementation is inline, but we're really inconsistent on this one, so up to you I guess.

@@ +77,5 @@
> +
> +  void GetValueAsString(nsAString& aValue) const;
> +
> +  float Angle() const { return mAngle; }
> +  void GetRotationOrigin(float& aOriginX, float& aOriginY) const {

I think out params should generally be pointers to make it clearer at the callers that the objects being passed to a function may/will be written to, but probably not a big deal in this case.

@@ +94,5 @@
> +
> +protected:
> +  gfxMatrix mMatrix;
> +  float mAngle, mOriginX, mOriginY;
> +  PRUint16 mType;

I really, really prefer all members to come right at the end to make in easier to see how much memory a class instance uses. Can you move these down? Same in other classes/patches I guess.

Comment 33

[Jonathan Watt [:jwatt]]

Comment on attachment 543856 [details] [diff] [review]
Part 6 - Refactor nsSVGTransformList into SVGTransformList and DOMSVGTransformList

Review of attachment 543856 [details] [diff] [review]:
-----------------------------------------------------------------

r=jwatt with the following comments addressed.

::: content/svg/content/src/nsSVGTransformList.cpp
@@ +118,4 @@
>    }
>  
> +  nsRefPtr<DOMSVGTransformList> kungFuDeathGrip;
> +  if (oldLength && !aNewLength) {

Hmm. I know you just copied this from the other list classes, but this should be |if (aNewLength < oldLength)|. The items are created lazily, and it's perfectly possible that there's just one item at an arbitrary index holding the last reference to us. I've provided a patch to fix the other classes in bug 669584.

You'll need to fix the kungFuDeathGrip in DOMSVGAnimatedTransformList too.

@@ +149,5 @@
>  {
> +  SVGAnimatedTransformList *alist = Element()->GetAnimatedTransformList();
> +  return IsAnimValList() && alist->mAnimVal ?
> +    *alist->mAnimVal :
> +    alist->mBaseVal;

Can you leave this all on one line as is the other list classes?

@@ +161,1 @@
>  {

Not sure why you started putting the return type on the same line as the signature here, but can you put in line breaks here and below for consistency?

@@ +381,1 @@
>  

Bug 631437 added GetLength to the DOM interface. Can you add that to the IDL file and add its implementation here?

::: content/svg/content/src/SVGTransformList.cpp
@@ +42,5 @@
> +using namespace mozilla;
> +
> +gfxMatrix
> +SVGTransformList::GetConsolidationMatrix() const
> +{

You should use RVO here. See SVGAnimatedNumberList::SMILAnimatedNumberList::GetBaseValue(). Add the comment you find there here, and change the code as appropriate.

@@ +66,5 @@
> +
> +nsresult
> +SVGTransformList::CopyFrom(const nsTArray<SVGTransform>& aTransformArray)
> +{
> +  if (!mTransforms.SetCapacity(aTransformArray.Length()))

Can you add the "// Yes, we do want fallible alloc here" as found in SVGPointList.cpp?

::: content/svg/content/src/SVGTransformList.h
@@ +59,5 @@
> +  friend class SVGAnimatedTransformList;
> +  friend class DOMSVGTransformList;
> +  friend class DOMSVGTransform;
> +
> +public:

Please put an empty ctor and dtor here like SVGPointList.h has so it's clear up front that creating and destroying objects of this type invokes no code.

@@ +116,5 @@
> +  /**
> +   * This may fail (return PR_FALSE) on OOM if the internal capacity is being
> +   * increased, in which case the list will be left unmodified.
> +   */
> +  PRBool SetLength(PRUint32 aTransformOfItems) {

aTransformOfItems? Heh. Make that aNumberOfItems please. ;)

@@ +157,5 @@
> +  }
> +
> +protected:
> +
> +  nsTArray<SVGTransform> mTransforms;

Can you call this mItems for consistency between list classes please?

And please put the comment from the corresponding member in SVGPointList.h before this member too.

Comment 34

[Jonathan Watt [:jwatt]]

Comment on attachment 543857 [details] [diff] [review]
Part 7 - Refactor nsSVGAnimatedTransformList into SVGAnimatedTransformList and DOMSVGAnimatedTransformList and incorporate nsSVGTransformSMILAttr

Review of attachment 543857 [details] [diff] [review]:
-----------------------------------------------------------------

r=jwatt with the comments addressed.

::: content/svg/content/src/nsSVGAnimatedTransformList.cpp
@@ +43,2 @@
>  
> +using namespace mozilla;

|namespace mozilla {| I'm afraid. :(

@@ +100,5 @@
>  }
>  
> +/* static */ DOMSVGAnimatedTransformList*
> +DOMSVGAnimatedTransformList::GetDOMWrapperIfExists(
> +    SVGAnimatedTransformList *aList)

Funny four-space indentation in this file too.

@@ +125,5 @@
> +  // bad memory)!!
> +
> +  nsRefPtr<DOMSVGAnimatedTransformList> kungFuDeathGrip;
> +  if (mBaseVal) {
> +    if (!aNewLength) {

Don't forget to fix this |if|.

::: content/svg/content/src/SVGAnimatedTransformList.cpp
@@ +44,5 @@
> +#include "nsSVGUtils.h"
> +#include "prdtoa.h"
> +#endif // MOZ_SMIL
> +
> +using namespace mozilla;

You should use |namespace mozilla {| here apparently. :(

@@ +76,5 @@
> +    // Attempting to increase mBaseVal's length failed - reduce domWrapper
> +    // back to the same length:
> +    domWrapper->InternalBaseValListWillChangeLengthTo(mBaseVal.Length());
> +  }
> +  mIsAttrSet = PR_TRUE;

This should be wrapped in an |else| statement...I think.

@@ +95,5 @@
> +  // Caller notifies
> +}
> +
> +nsresult
> +SVGAnimatedTransformList::SetAnimValue(const SVGTransformList& aValue,

Can you call this aNewAnimValue for consistency?

@@ +164,5 @@
> +  // 2) DOM call -- simply fetching the baseVal doesn't mean the transform value
> +  //    has been set. It is set if that baseVal has one or more transforms in
> +  //    the list.
> +  // 3) Animation -- which will case the mAnimVal member to be allocated
> +  return mIsAttrSet || !mBaseVal.IsEmpty() || mAnimVal;

Hmm, so I'd argue that mAttrIsSet should be set when the DOM list interface is used to set a list item, otherwise it's value lies. I believe that currently happens under the SetParsedAttr call made by DidChangeTransformList (correct me if I'm wrong!), so there should be no need to check mBaseVal.IsEmpty(). I'd also prefer this method to be inlined in the header like it is in other classes, and for the comment to be a normal doxygen comment on the method there. (Given how simple the implementation is though, I'm not sure that the comment is valuable - 'mAttrIsSet || mAnimVal' seems fairly self explanatory to me. Maybe it wasn't to you though, in which case feel free to keep it (maybe cutting it down a bit?).)

@@ +179,5 @@
> +SVGAnimatedTransformList::SMILAnimatedTransformList::ValueFromString(
> +    const nsAString& aStr,
> +    const nsISMILAnimationElement* aSrcElement,
> +    nsSMILValue& aValue,
> +    PRBool& aPreventCachingOfSandwich) const

Funny four-space indentation in this file too.

@@ +183,5 @@
> +    PRBool& aPreventCachingOfSandwich) const
> +{
> +  NS_ENSURE_TRUE(aSrcElement, NS_ERROR_FAILURE);
> +  NS_ASSERTION(aValue.IsNull(),
> +    "aValue should have been cleared before calling ValueFromString");

NS_ABORT_IF_FALSE please.

@@ +186,5 @@
> +  NS_ASSERTION(aValue.IsNull(),
> +    "aValue should have been cleared before calling ValueFromString");
> +
> +  const nsAttrValue* typeAttr = aSrcElement->GetAnimAttr(nsGkAtoms::type);
> +  const nsIAtom* transformType = nsGkAtoms::translate;

Append "// default val" to end of this line?

@@ +190,5 @@
> +  const nsIAtom* transformType = nsGkAtoms::translate;
> +  if (typeAttr) {
> +    if (typeAttr->Type() != nsAttrValue::eAtom) {
> +      // Recognized values of |type| are parsed as an atom -- so if we have
> +      // something other than an atom, then it means our |type| was invalid.

s/it means/know already that/ s/was/is/

@@ +207,5 @@
> +    const nsAString& aSpec,
> +    const nsIAtom* aTransformType,
> +    nsSMILValue& aResult)
> +{
> +  NS_ASSERTION(aResult.IsNull(), "Unexpected type for SMIL value");

NS_ABORT_IF_FALSE

@@ +251,5 @@
> +
> +  nsSMILValue val(&SVGTransformListSMILType::sSingleton);
> +  SVGTransformSMILData transform(transformType, params);
> +  if (NS_FAILED(SVGTransformListSMILType::AppendTransform(transform, val))) {
> +    return;

Append "// OOM"

@@ +324,5 @@
> +  return val;
> +}
> +
> +void
> +SVGAnimatedTransformList::SMILAnimatedTransformList::ClearAnimValue()

Can you put the definition of ClearAnimValue after SetAnimValue for consistency?

@@ +336,5 @@
> +SVGAnimatedTransformList::SMILAnimatedTransformList::SetAnimValue(
> +    const nsSMILValue& aValue)
> +{
> +  NS_ASSERTION(aValue.mType == &SVGTransformListSMILType::sSingleton,
> +               "Unexpected type to assign animated value");

NS_ABORT_IF_FALSE

::: content/svg/content/src/SVGAnimatedTransformList.h
@@ +99,5 @@
> +  PRBool IsAnimating() const {
> +    return !!mAnimVal;
> +  }
> +
> +  PRBool IsExplicitlySet() const;

Can you move this up to just before IsAnimating for consistency with number list?

@@ +115,5 @@
> +  // the empty string (<set to="">).
> +
> +  SVGTransformList mBaseVal;
> +  nsAutoPtr<SVGTransformList> mAnimVal;
> +  PRPackedBool mIsAttrSet:1; // True if the base value is set by markup

No need for this to be a bit flag, since that doesn't save us any space here, and just adds code for packing/unpacking.

Also, the reference to "set by markup" isn't strictly correct. It's just if the attribute is set. Personally I'd just call it mAttrIsSet and remove the comment as redundant.

Comment 35

[Brian Birtles (:birtles)]

Attachment: Roll-up patch for cross-reference

As requested, posting a roll-up patch of parts 1 to 15 for cross-referencing during review.

Comment 36

[Jonathan Watt [:jwatt]]

Comment on attachment 543852 [details] [diff] [review]
Part 2 - Add tests for transform lists

Loving these tests - you seem to have covered most bases. :-) Just a few things:

>+function testInverse()
>+{
>+  // Test inversion
>+  var m = createMatrix(2, 0, 0, 4, 110, -50);
>+  roughCmpMatrix(m.inverse(), [0.5, 0, 0, 0.25, -55, 12.5],
>+    "Unexpected result after inverting matrix");
>+
>+  // Test non-invertable
>+  m = createMatrix(0, 0, 1, 0, 0, 0);
>+  try {
>+    m.inverse();
>+    ok(false, "Failed to throw exception when inverting singular matrix");
>+  } catch (e) {
>+    if (e.code != SVGException.SVG_MATRIX_NOT_INVERTABLE) {
>+      ok(false, "Got unexpected exception " + e);
>+    }

Instead of the 'if' statement and ok() with hardcoded 'false', just use is(e.code, SVGException.SVG_MATRIX_NOT_INVERTABLE, "Checking the exception is SVG_MATRIX_NOT_INVERTABLE").

Same in testRotateFromVector.

Same in testReadOnly in test_SVGTransformList.xhtml.

>+function testCreateSVGTransformFromMatrix(g)
>+{
>+  var m = createMatrix(1, 2, 3, 4, 5, 6);
>+
>+  // "Creates an SVGTransform object which is initialized to transform of type
>+  // SVG_TRANSFORM_MATRIX and whose values are the given matrix. The values from
>+  // the parameter matrix are copied, the matrix parameter is not adopted as
>+  // SVGTransform::matrix."
>+  var list = g.transform.baseVal;
>+  list.clear();
>+  var t = list.createSVGTransformFromMatrix(m);
>+
>+  // Check that list hasn't changed
>+  is(list.numberOfItems, 0,
>+     "Transform list changed after calling createSVGTransformFromMatrix");
>+
>+  // Check return value
>+  is(t.type, SVGTransform.SVG_TRANSFORM_MATRIX,
>+     "Returned transform not of type matrix");
>+  cmpMatrix(t.matrix, [1, 2, 3, 4, 5, 6],
>+     "Unexpected returned matrix value");
>+
>+  // Check values are copied
>+  ok(t.matrix != m, "Matrix should be copied not adopted");
>+  m.a = 2;
>+  is(t.matrix.a, 1,
>+     "Changing source matrix should not affect newly created transform");
>+
>+  // Try passing in bad values (null, "undefined" etc.)
>+  try {
>+    t = list.createSVGTransformFromMatrix(null);
>+    ok(false, "Failed to throw for null input to createSVGTransformFromMatrix");
>+  } catch(e) { }
>+  try {
>+    t = list.createSVGTransformFromMatrix("undefinied");
>+    ok(false,
>+       "Failed to throw for string input to createSVGTransformFromMatrix");
>+  } catch(e) { }
>+  try {
>+    t = list.createSVGTransformFromMatrix(SVGMatrix(t));
>+    ok(false, "Failed to throw for bad input to createSVGTransformFromMatrix");
>+  } catch(e) { }

Rather than having no "pass" when an exception is thrown, I think it would be better if we had code along the lines of:

  exception = null;
  try {
    ...
  } catch(e) {
    exception = e;
  }
  is(exception, ..., "Checking XXX was thrown");
  exception = null;
  ...

Same in testReadOnly and testOrphan.

>+function AdditionTestCase(desc, parentVal, animSpecs, expectedTransformList)
>+{
>+  this.desc = desc;
>+  this.parentVal = parentVal;
>+  this.animSpecs = animSpecs;
>+  this.expectedTransformList = expectedTransformList;
>+}

Can you rename 'parentVal' to 'baseVal' please? It would make the code quicker to grok.

>+  replaceChildNodes(elem);

Can you pass in an explicit 'null' here please?

> function create_animate_elements(test)
> {
>   var SVG_NS = 'http://www.w3.org/2000/svg';
>   var df = document.createDocumentFragment();
> 
>   if (test.attr_name == 'transform' ||
>       test.attr_name == 'gradientTransform' ||
>       test.attr_name == 'patternTransform') {
>-    ok(false, 'Need to update create_animate_element to handle transforms?')
>-    throw new Error('Don\'t know how to handle transforms');
>-    var animate1 =
>-      document.createElementNS(SVG_NS, 'animateTransform');
>-    var animate2 =
>-      document.createElementNS(SVG_NS, 'animateTransform');
>-    var animate3 =
>-      document.createElementNS(SVG_NS, 'animateTransform');
>-  } else {
>-    var animate1 = document.createElementNS(SVG_NS, 'animate');
>-    var animate2 = document.createElementNS(SVG_NS, 'animate');
>-    var animate3 = document.createElementNS(SVG_NS, 'animate');
>+    // animateTransform is "special". Although it targets an
>+    // nsSVGAnimatedTransformList it only takes nsSVGTransform values as
>+    // animation values. Therefore all the assumptions we're testing about the
>+    // length of lists don't apply. We simply have to test it separately.
>+    // This is done in test_SVGTransformListAddition.xhtml.
>+    return null;
>   }

Why not just return the empty 'df', since then you can skip the changes at this function's caller?

>-    // Skip if there is no animVal for this test
>-    if (!t.animVal)
>+    // Skip if there is no animVal for this test or if we failed to add
>+    // animation elements as children (as is the case for transform lists since
>+    // these need to be handled specially)
>+    if (!t.animVal || t.element.childNodes.length == 0)
>       continue;

I think I'd rather have an isTransformAttribute() helper and call that to more directly test for the condition you're checking for, than to check that there are no child nodes.

Comment 37

[Jonathan Watt [:jwatt]]

Comment on attachment 543858 [details] [diff] [review]
Part 8 - Move nsSVGSMILTransform to SVGTransform

r=jwatt, although if you can be bothered you could:

+SVGTransformSMILData::SVGTransformSMILData(const SVGTransform& aTransform)
>+  : mTransformType(aTransform.Type())
>+{
>+  NS_ABORT_IF_FALSE(
>+      mTransformType >= nsIDOMSVGTransform::SVG_TRANSFORM_MATRIX &&
>+      mTransformType <= nsIDOMSVGTransform::SVG_TRANSFORM_SKEWY,
>+      "Unexpected transform type");

Indent this my four spaces instead of six.

>+  switch (mTransformType) {
>+    case nsIDOMSVGTransform::SVG_TRANSFORM_MATRIX:
>+      {
>+        const gfxMatrix& mx = aTransform.Matrix();
>+        mParams[0] = static_cast<float>(mx.xx);
>+        mParams[1] = static_cast<float>(mx.yx);
>+        mParams[2] = static_cast<float>(mx.xy);
>+        mParams[3] = static_cast<float>(mx.yy);
>+        mParams[4] = static_cast<float>(mx.x0);
>+        mParams[5] = static_cast<float>(mx.y0);
>+      }
>+      break;

Line up the case and { so it's consistent with other SVG code:

  switch (...) {
  case ...:
  {
    ...

Comment 38

[Jonathan Watt [:jwatt]]

Comment on attachment 543859 [details] [diff] [review]
Part 9 - Refactor nsSVGTransformSMILType to use SVGTransformSMILData

r=jwatt

>-nsSVGTransformSMILType::AppendTransform(const nsSVGSMILTransform& aTransform,
>-                                        nsSMILValue& aValue)
>+SVGTransformListSMILType::AppendTransform(
>+    const SVGTransformSMILData& aTransform,
>+    nsSMILValue& aValue)

This doesn't need to be broken onto a new line like this, since it's 79 chars (less than 80) if not.

Comment 39

[Jonathan Watt [:jwatt]]

Comment on attachment 543863 [details] [diff] [review]
Part 13 - Update layout to use new matrix and transform types

r=jwatt, though you may want to fix these nits:

> nsSVGMaskFrame::GetCanvasTM()
> {
>   NS_ASSERTION(mMaskParentMatrix, "null parent matrix");
> 
>   nsSVGMaskElement *mask = static_cast<nsSVGMaskElement*>(mContent);
> 
>-  return nsSVGUtils::AdjustMatrixForUnits(nsSVGUtils::ConvertSVGMatrixToThebes(mMaskParentMatrix),
>-                                          &mask->mEnumAttributes[nsSVGMaskElement::MASKCONTENTUNITS],
>-                                          mMaskParent);
>+  return nsSVGUtils::AdjustMatrixForUnits(
>+      mMaskParentMatrix ? *mMaskParentMatrix : gfxMatrix(),
>+      &mask->mEnumAttributes[nsSVGMaskElement::MASKCONTENTUNITS],
>+      mMaskParent);

Indentation.

>-  nsIDOMSVGAnimatedTransformList* GetPatternTransformList(nsIContent* aDefault);
>+  mozilla::SVGAnimatedTransformList* GetPatternTransformList(
>+      nsIContent* aDefault);

ditto

Comment 40

[Jonathan Watt [:jwatt]]

Comment on attachment 544106 [details] [diff] [review]
Part 10 - Update SVG elements to use new SVG transform types, v1b

Looks good to me too FWIW.

Comment 41

[Jonathan Watt [:jwatt]]

Thanks for the combined patch, it certainly helped. I'm still going over a few things, but you're pretty much good to go.

Comment 42

[Brian Birtles (:birtles)]

(In reply to Jonathan Watt [:jwatt] from comment #41)
> Thanks for the combined patch, it certainly helped. I'm still going over a
> few things, but you're pretty much good to go.

Thanks heaps Jonathan. I've been gradually working through the review feedback you and Robert provided previously and hopefully I'll get some updated patches posted within the week. All the feedback is much appreciated.

Comment 43

[Brian Birtles (:birtles)]

Attachment: Part 1 - Refactor SVG list tests to correct assumptions about SMIL v1b; r=jwatt

Comment 44

[Brian Birtles (:birtles)]

Attachment: Part 2 - Add tests for transform lists v1b; r=jwatt

Comment 45

[Brian Birtles (:birtles)]

Attachment: Part 3 - Rewrite nsSVGMatrix and use gfxMatrix instead v1c; r=jwatt

Comment 46

[Brian Birtles (:birtles)]

Attachment: Part 4 - Update DOMSVGPoint to use gfxMatrix v1b; r=jwatt

Comment 47

[Brian Birtles (:birtles)]

Attachment: Part 5 - Refactor nsSVGTransform into SVGTransform and DOMSVGTransform v1b

Comment 48

[Brian Birtles (:birtles)]

Attachment: Part 6 - Refactor nsSVGTransformList into SVGTransformList and DOMSVGTransformList v1b; r=jwatt

Comment 49

[Brian Birtles (:birtles)]

Attachment: Part 7 - Refactor nsSVGAnimatedTransformList into SVGAnimatedTransformList and DOMSVGAnimatedTransformList and incorporate nsSVGTransformSMILAttr v1b; r=jwatt

Comment 50

[Brian Birtles (:birtles)]

Attachment: Part 8 - Move nsSVGSMILTransform to SVGTransform; r=jwatt

Comment 51

[Brian Birtles (:birtles)]

Attachment: Part 9 - Refactor nsSVGTransformSMILType to use SVGTransformSMILData v1b; r=jwatt

Comment 52

[Brian Birtles (:birtles)]

Attachment: Part 10 - Update SVG elements to use new SVG transform types v1c; r=longsonr, jwatt

Comment 53

[Brian Birtles (:birtles)]

Attachment: Part 11 - Update nsSVGTransformListParser to use new SVG transform types v1b; r=jwatt

Comment 54

[Brian Birtles (:birtles)]

Attachment: Part 12 - Update DOM bindings to use new SVG transform types v1b; r=jwatt

Comment 55

[Brian Birtles (:birtles)]

Attachment: Part 13 - Update layout to use new matrix and transform types v1b; r=jwatt

Comment 56

[Brian Birtles (:birtles)]

Attachment: Part 14 - Mark tests that now pass as such v1b; r=jwatt

Comment 57

[Brian Birtles (:birtles)]

Attachment: Part 15 - Rename nsSVGTransformListParser as SVGTransformListParser v1b; r=jwatt

Comment 58

[Brian Birtles (:birtles)]

Attachment: Test case for transform IsExplicitlySet

Comment 59

[Brian Birtles (:birtles)]

Updated all patches to fix bitrot and address review feedback. Some specific parts of the feedback I had questions about:

(In reply to Robert Longson from comment #31)
> Comment on attachment 544106 [details] [diff] [review]
> Part 10 - Update SVG elements to use new SVG transform types, v1b
> 
> >diff --git a/content/svg/content/src/nsSVGGraphicElement.cpp b/content/svg/content/src/nsSVGGraphicElement.cpp
> >--- a/content/svg/content/src/nsSVGGraphicElement.cpp
> >+++ b/content/svg/content/src/nsSVGGraphicElement.cpp
> 
> > /* nsIDOMSVGMatrix getCTM (); */
> > NS_IMETHODIMP nsSVGGraphicElement::GetCTM(nsIDOMSVGMatrix * *aCTM)
> > {
> >   gfxMatrix m = nsSVGUtils::GetCTM(this, PR_FALSE);
> >-  *aCTM = m.IsSingular() ? nsnull : NS_NewSVGMatrix(m).get();
> >+  *aCTM = m.IsSingular() ? nsnull : new DOMSVGMatrix(m);
> >+  NS_IF_ADDREF(*aCTM);
> 
> if (m.IsSingular) {} with NS_ADDREF would be simpler and more efficient.

I think I don't understand. As in:

if (m.IsSingular()) {
  NS_ADDREF(*aCTM = new DOMSVGMatrix(m));
} else {
  *aCTM = nsnull;
}

?

If that's the case then I don't think the extra verbosity is justified for the negligible perf win.

(In reply to Jonathan Watt [:jwatt] from comment #32)
> Comment on attachment 543855 [details] [diff] [review]
> Part 5 - Refactor nsSVGTransform into SVGTransform and DOMSVGTransform
> 
> Review of attachment 543855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/svg/content/src/nsSVGTransform.h
> @@ +105,5 @@
> > +    // Our matrix tear-off pointer should be cleared before we are destroyed
> > +    // (since matrix tear-offs keep an owning reference to their transform, and
> > +    // clear the tear-off pointer themselves if unlinked).
> > +    NS_ABORT_IF_FALSE(!mMatrixTearoff, "Matrix tear-off pointer not cleared."
> > +        " Transform being destroyed before matrix?");
> 
> Put a line break in here? :)

I'm not sure I understand. If I put the line break after the comma I still have to break up the string but instead it takes three lines.

> ::: content/svg/content/src/SVGTransform.cpp
> @@ +52,5 @@
> > +  PRUnichar buf[256];
> > +
> > +  switch (mType) {
> > +    case nsIDOMSVGTransform::SVG_TRANSFORM_TRANSLATE:
> > +      {
> 
> Style in the other classes is to have the case and curly brackets indented
> to the same level as the switch statement. In fact you don't even need the
> curly brackets in these cases, but I guess I don't care about that too much.

This is just moving code (from the old nsSVGTransform) and I was trying not to change it too much. I've removed the curly braces but kept the indenting since it matches the Mozilla Coding Style Guide which I think is the direction we ought to be heading. Does that sound ok?

> @@ +121,5 @@
> > +  mType    = nsIDOMSVGTransform::SVG_TRANSFORM_MATRIX;
> > +  mMatrix  = aMatrix;
> > +  mAngle   = 0.f;
> > +  mOriginX = 0.f;
> > +  mOriginY = 0.f;
> 
> Not sure it's worth setting the members that shouldn't ever be used given
> the value mType is being set to. Just seems like extra code to load and
> cycles to me. :)

Actually, we need it for now since:
* operator== tests for equality on all members regardless of mType, and
* mAngle can be accessed via the DOM and is defined to return 0 when it's not used

We could add code to operator==() and Angle() to address that but I'd rather not. In part because it would be more complicated and bug-prone and probably balance out any win we'd get by removing the current code, but also because if these members ever are accessed inappropriately (such as if we kept the naive implementation of operator==) we could end up with a bug that's really hard to reproduce because it only occurs when an unset member takes on certain values.

(In reply to Jonathan Watt [:jwatt] from comment #33)
> ::: content/svg/content/src/nsSVGTransformList.cpp
> @@ +149,5 @@
> >  {
> > +  SVGAnimatedTransformList *alist = Element()->GetAnimatedTransformList();
> > +  return IsAnimValList() && alist->mAnimVal ?
> > +    *alist->mAnimVal :
> > +    alist->mBaseVal;
> 
> Can you leave this all on one line as is the other list classes?

I'm afraid not, it goes over 80 chars (as do some of the other classes, e.g. DOMSVGLengthList, 81 chars)

(In reply to Jonathan Watt [:jwatt] from comment #34)
> @@ +164,5 @@
> > +  // 2) DOM call -- simply fetching the baseVal doesn't mean the transform value
> > +  //    has been set. It is set if that baseVal has one or more transforms in
> > +  //    the list.
> > +  // 3) Animation -- which will case the mAnimVal member to be allocated
> > +  return mIsAttrSet || !mBaseVal.IsEmpty() || mAnimVal;
> 
> Hmm, so I'd argue that mAttrIsSet should be set when the DOM list interface
> is used to set a list item, otherwise it's value lies. I believe that
> currently happens under the SetParsedAttr call made by
> DidChangeTransformList (correct me if I'm wrong!), so there should be no
> need to check mBaseVal.IsEmpty().

Unfortunately that's not the case. SetParsedAttr doesn't result in a call to SetBaseValueString. In fact, there are no calls to SVGAnimatedTransformList as a result of this operation.

I've attached a test case (attachment 555295 [details]) which will fail if we remove the !mBaseVal.IsEmpty() condition.

A couple of possibilities include:

1) Leave it as it (although perhaps renaming mAttrIsSet to mAttrIsSetByMarkup)
2) Make DOMSVGTransformList call SVGAnimatedTransformList at each point where the list might become populated (basically insertItemBefore and appendItem) and set the mAttrIsSet flag

I don't like (2) a lot since it's hacky (increases coupling) and bug-prone (there might be some case where the list becomes populated that we miss). But I can be persuaded otherwise.

If we go with (1) then do you still want this inlined? I think the comment is justified in this case and would obscure the header file so I was thinking of leaving it in the implementation file.

In any case, I'll work on putting together a suitable reftest for this bit of code since I think it's only partially covered at the moment (by anim-pattern-attr-preserve-0{1,2}.svg).

(In reply to Jonathan Watt [:jwatt] from comment #36)
> Comment on attachment 543852 [details] [diff] [review]
> Part 2 - Add tests for transform lists
> 
> >+function testCreateSVGTransformFromMatrix(g)
> >+{
> >+  var m = createMatrix(1, 2, 3, 4, 5, 6);
> >+
> >+  // "Creates an SVGTransform object which is initialized to transform of type
> >+  // SVG_TRANSFORM_MATRIX and whose values are the given matrix. The values from
> >+  // the parameter matrix are copied, the matrix parameter is not adopted as
> >+  // SVGTransform::matrix."
> >+  var list = g.transform.baseVal;
> >+  list.clear();
> >+  var t = list.createSVGTransformFromMatrix(m);
> >+
> >+  // Check that list hasn't changed
> >+  is(list.numberOfItems, 0,
> >+     "Transform list changed after calling createSVGTransformFromMatrix");
> >+
> >+  // Check return value
> >+  is(t.type, SVGTransform.SVG_TRANSFORM_MATRIX,
> >+     "Returned transform not of type matrix");
> >+  cmpMatrix(t.matrix, [1, 2, 3, 4, 5, 6],
> >+     "Unexpected returned matrix value");
> >+
> >+  // Check values are copied
> >+  ok(t.matrix != m, "Matrix should be copied not adopted");
> >+  m.a = 2;
> >+  is(t.matrix.a, 1,
> >+     "Changing source matrix should not affect newly created transform");
> >+
> >+  // Try passing in bad values (null, "undefined" etc.)
> >+  try {
> >+    t = list.createSVGTransformFromMatrix(null);
> >+    ok(false, "Failed to throw for null input to createSVGTransformFromMatrix");
> >+  } catch(e) { }
> >+  try {
> >+    t = list.createSVGTransformFromMatrix("undefinied");
> >+    ok(false,
> >+       "Failed to throw for string input to createSVGTransformFromMatrix");
> >+  } catch(e) { }
> >+  try {
> >+    t = list.createSVGTransformFromMatrix(SVGMatrix(t));
> >+    ok(false, "Failed to throw for bad input to createSVGTransformFromMatrix");
> >+  } catch(e) { }
> 
> Rather than having no "pass" when an exception is thrown, I think it would
> be better if we had code along the lines of:
> 
>   exception = null;
>   try {
>     ...
>   } catch(e) {
>     exception = e;
>   }
>   is(exception, ..., "Checking XXX was thrown");
>   exception = null;
>   ...

Do you actually want to test the exception code too in this case? These methods don't define what error code should be thrown for bad input. We're currently throwing SVGException.SVG_WRONG_TYPE_ERR but I just noticed that SVG 1.1 F2 says this exception code is really only around for backwards compatibility with SVG 1.1 1stEd. Should we be throwing SVGException.SVG_INVALID_VALUE_ERR instead?

> Same in testReadOnly and testOrphan.

These are quite different. Is the concern here to have the same number of passes/fails? I've adjusted testOrphan to that end but I've left testReadOnly for now since the current setup allows us to distinguish between failing to throw an exception at all and throwing one with the wrong type. I guess I'm just not sure exactly what we're trying to fix here.

(In reply to Jonathan Watt [:jwatt] from comment #38)
> >-nsSVGTransformSMILType::AppendTransform(const nsSVGSMILTransform& aTransform,
> >-                                        nsSMILValue& aValue)
> >+SVGTransformListSMILType::AppendTransform(
> >+    const SVGTransformSMILData& aTransform,
> >+    nsSMILValue& aValue)
> 
> This doesn't need to be broken onto a new line like this, since it's 79
> chars (less than 80) if not.

You need a new text editor mate, it's 81 :) but I've removed the extra indenting (the fault of *my* editor which I've fixed now).

Comment 60

[Brian Birtles (:birtles)]

Attachment: Roll-up patch for cross-reference v1b

Update roll-up patch

Comment 61

[Brian Birtles (:birtles)]

Attachment: Part 16 - Reftest for deteting presence of transform lists

Add reftest for checking what we're doing in SVGAnimatedTransformList::IsExplicitlySet

Comment 62

[Mozilla RelEng Bot]

Try run for 8f4e0eb78690 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=8f4e0eb78690
Results (out of 167 total builds):
    success: 157
    warnings: 10
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8f4e0eb78690

Comment 63

[Brian Birtles (:birtles)]

Attachment: Part 10 - Update SVG elements to use new SVG transform types v1d; r=longsonr, jwatt

Fix a bug in changes to nsSVGGraphicElement::SetAnimateMotionTransform

Comment 64

[Brian Birtles (:birtles)]

Attachment: Part 17 - Remove BeforeSetAttr overrides

Just another (really small!) patch that removes our overrides of BeforeSetAttr. These overrides should be using unfallible malloc so there's no need to do the memory allocation and checking out of band.

Comment 65

[Robert Longson [:longsonr]]

(In reply to Brian Birtles (:birtles) from comment #64)
> Created attachment 561114 [details] [diff] [review]
> Part 17 - Remove BeforeSetAttr overrides
> 
> Just another (really small!) patch that removes our overrides of
> BeforeSetAttr. These overrides should be using unfallible malloc so there's
> no need to do the memory allocation and checking out of band.

The overrides are there to lazily create the transforms since most elements don't have transforms in practice. Are we doing that in some other way now?

Comment 66

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #65)
> The overrides are there to lazily create the transforms since most elements
> don't have transforms in practice. Are we doing that in some other way now?

Yeah, we're still lazily creating them in, e.g. nsSVGGraphicElement::GetAnimatedTransformList() from patch 10.

Comment 67

[Robert Longson [:longsonr]]

(In reply to Brian Birtles (:birtles) from comment #66)
> (In reply to Robert Longson from comment #65)
> > The overrides are there to lazily create the transforms since most elements
> > don't have transforms in practice. Are we doing that in some other way now?
> 
> Yeah, we're still lazily creating them in, e.g.
> nsSVGGraphicElement::GetAnimatedTransformList() from patch 10.

Ahh yes. In that case you're right we no longer need that code.

Comment 68

[Brian Birtles (:birtles)]

Attachment: Part 10 - Update SVG elements to use new SVG transform types v1e; r=longsonr, jwatt

Fix bitrot and remove unnecessary line for reporting parse errors to the console.

Comment 69

[Brian Birtles (:birtles)]

Attachment: Part 13 - Update layout to use new matrix and transform types v1c; r=jwatt

Fix bitrot

Comment 70

[Jonathan Watt [:jwatt]]

I pushed everything up to and including part 17 to mozilla-inbound.

(In reply to Brian Birtles (:birtles) from comment #59)
> Actually, we need it for now since:
> * operator== tests for equality on all members regardless of mType, and
> * mAngle can be accessed via the DOM and is defined to return 0 when it's
> not used

I added a comment explaining this, since it's not immediately obvious, and otherwise someone might "fix" it. :)

I still want to come back to the mIsAttrSet thing, and the exception code thing, (both comment 59), but no need to hold things up on that.

Comment 71

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7924111705c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d681a73cfc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/38e6f7980e5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2afd7f5962d
https://hg.mozilla.org/integration/mozilla-inbound/rev/85e56f66704c
https://hg.mozilla.org/integration/mozilla-inbound/rev/caa562226631
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e2d6577aa6
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d244bc338f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e23063321cee
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8fcaa2794f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec7fb3b6bbaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/af1b9b6267bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9a9397f6db3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e72c9d97a42
https://hg.mozilla.org/integration/mozilla-inbound/rev/28537c7d79b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bb849d7c99
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbde35bf04e9

Thanks Jonathan!

Comment 72

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/c7924111705c
https://hg.mozilla.org/mozilla-central/rev/1d681a73cfc7
https://hg.mozilla.org/mozilla-central/rev/38e6f7980e5e
https://hg.mozilla.org/mozilla-central/rev/e2afd7f5962d
https://hg.mozilla.org/mozilla-central/rev/85e56f66704c
https://hg.mozilla.org/mozilla-central/rev/caa562226631
https://hg.mozilla.org/mozilla-central/rev/e5e2d6577aa6
https://hg.mozilla.org/mozilla-central/rev/04d244bc338f
https://hg.mozilla.org/mozilla-central/rev/e23063321cee
https://hg.mozilla.org/mozilla-central/rev/b8fcaa2794f3
https://hg.mozilla.org/mozilla-central/rev/ec7fb3b6bbaf
https://hg.mozilla.org/mozilla-central/rev/af1b9b6267bf
https://hg.mozilla.org/mozilla-central/rev/c9a9397f6db3
https://hg.mozilla.org/mozilla-central/rev/6e72c9d97a42
https://hg.mozilla.org/mozilla-central/rev/28537c7d79b7
https://hg.mozilla.org/mozilla-central/rev/b6bb849d7c99
https://hg.mozilla.org/mozilla-central/rev/dbde35bf04e9

Comment 73

[Jonathan Watt [:jwatt]]

Oh, and thanks a lot Brian! Hugely appreciated!!

Comment 74

[Brian Birtles (:birtles)]

(In reply to Jonathan Watt [:jwatt] (away 1-9 Oct, inc.) from comment #73)
> Oh, and thanks a lot Brian! Hugely appreciated!!

Thanks for reviewing! (And for getting rid of nsSVGValue!)

Comment 75

[Eric Shepherd [:sheppy]]

There are a lot of comments here; is there by any chance a tl;dr version of what I need to look for when documenting this? :)

Comment 76

[Jeremie Patonnier :Jeremie]

Hi sheppy :)

I plan to write some basic documentation for SVGAnimatedTransformList by tomorrow.

To all, is there any non standard features attached to the Gecko implementation of SVGAnimatedTransformList ?

Comment 77

[Jonathan Watt [:jwatt]]

Thanks Jeremie. The only non-standard thing of note is that we have a .length property that is a synonym for the .numberOfItems property:

http://www.w3.org/TR/SVG11/coords.html#InterfaceSVGTransformList

I believe Opera, Webkit and IE all also have this extension in their latest releases (or will do very soon, since everyone agreed to add it at).

Comment 78

[Jeremie Patonnier :Jeremie]

(In reply to Jonathan Watt [:jwatt] from comment #77)
> Thanks Jeremie. The only non-standard thing of note is that we have a
> .length property that is a synonym for the .numberOfItems property

Ok so I guest that it's like others SVGxxxList and any instance of SVGTransformList is accessible like an Array

> I believe Opera, Webkit and IE all also have this extension in their latest
> releases (or will do very soon, since everyone agreed to add it at).

Oh ! That's good news, I miss this on the SVG WG ML.