Rik Cabanier Assignee
	Description • 10 years ago

Expose new global objects named 'DOMMatrix' and 'DOMMatrixReadOnly' that offer a matrix abstraction.
Spec: http://dev.w3.org/fxtf/geometry/#DOMMatrix

	Alex Vincent [:WeirdAl]
	Comment 1 • 10 years ago

Hmm, that's pretty cool.  I have a linear algebra class this fall, and this is one very clear application thereof.  If no one's done it by the end of the year, I'd probably take a stab at this.

	Rik Cabanier Assignee
	Comment 3 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #1)
> Hmm, that's pretty cool.  I have a linear algebra class this fall, and this
> is one very clear application thereof.  If no one's done it by the end of
> the year, I'd probably take a stab at this.

Thanks! The geometry spec contains all the formulas but I wouldn't mind having another set of eyes!
I checked in my first version. It should just miss the correct exception and stringification.

	Rik Cabanier Assignee
	Comment 5 • 10 years ago

Comment on attachment 8436189 [details] [diff] [review]
More complete impementation

Does this look OK? Did I make any obvious mistakes? :-)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 10 years ago

Comment on attachment 8436189 [details] [diff] [review]
More complete impementation

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

Mostly looks good.

::: content/base/src/DOMMatrix.cpp
@@ +29,5 @@
> +  }\
> +  if (mMatrix3D) {\
> +    return mMatrix3D->entry3D;\
> +  }\
> +  return default;\

Seems to me it would be OK to simplify things by requiring one of mMatrix2D and mMatrix3D to be non-null. I don't imagine we'll have a lot of matrix objects which stay as the identity. That would simplify this code a bit.

@@ +74,5 @@
> +DOMMatrixReadOnly::M31() const Get3DMatrixMember(_31, 0)
> +double
> +DOMMatrixReadOnly::M32() const Get3DMatrixMember(_32, 0)
> +double
> +DOMMatrixReadOnly::M33() const Get3DMatrixMember(_33, 1)

Be consistent using 1.0

@@ +84,5 @@
> +DOMMatrixReadOnly::M42() const GetMatrixMember(_32, _42, 0)
> +double
> +DOMMatrixReadOnly::M43() const Get3DMatrixMember(_43, 0)
> +double
> +DOMMatrixReadOnly::M44() const Get3DMatrixMember(_44, 1)

I would put all these in the .h file so they can be inlined into the binding.

@@ +92,5 @@
> +                             double ty,
> +                             double tz) const
> +{
> +  nsRefPtr<DOMMatrix> retval = new DOMMatrix(mParent, *this);
> +  retval = retval->TranslateThis(tx, ty, tz);

As I point out below, TranslateThis should return void. There's certainly no point in assigning retval here, which would just cause unnecessary refcount churn.

@@ +246,5 @@
> +
> +bool
> +DOMMatrixReadOnly::MaybeHasTransform() const
> +{
> +  return (mMatrix2D != nullptr) || (mMatrix3D != nullptr);

I've come around to agreeing with Dirk and others that we should just check the values of the coefficients.

@@ +307,5 @@
> +  data[11] = M34();
> +  data[12] = M41();
> +  data[13] = M42();
> +  data[14] = M43();
> +  data[15] = M44();

You'll need to insert explicit float casts here to avoid warnings on Windows.

You could save duplication between this and ToFloat64Array by having a static helper function like so:
template <typename T> void SetDataFromMatrix(DOMMatrixReadOnly* aMatrix, T* aData)
{
  aData[0] = static_cast<T>(aMatrix->M11());
  ...
}

@@ +391,5 @@
> +DOMMatrix::Constructor(const GlobalObject& aGlobal, const RootedTypedArray<Float32Array>& array32, ErrorResult& aRv)
> +{
> +  nsRefPtr<DOMMatrix> obj = new DOMMatrix(aGlobal.GetAsSupports());
> +  const float* data = array32.Data();
> +  if (array32.Length() == 6) {

Similar to above, you can use a static templated helper function here to avoid duplication with the Float64Array case. Pass in the length as a parameter to the function.

@@ +463,5 @@
> +DOMMatrix::Constructor(const GlobalObject& aGlobal, const binding_detail::AutoSequence<double>& numberSequence, ErrorResult& aRv)
> +{
> +  nsRefPtr<DOMMatrix> obj = new DOMMatrix(aGlobal.GetAsSupports());
> +  const double* data = numberSequence.Elements();
> +  if (numberSequence.Length() == 6) {

Ah, you can use the same helper function here too.

@@ +684,5 @@
> +}
> +
> +already_AddRefed<DOMMatrix>
> +DOMMatrix::RotateFromVectorThis(double x,
> +                              double y)

Bunch of indenting to fix

@@ +833,5 @@
> +                 mMatrix3D->_22  * mMatrix3D->_43 * mMatrix3D->_34 -
> +                 mMatrix3D->_23  * mMatrix3D->_32  * mMatrix3D->_44 +
> +                 mMatrix3D->_23  * mMatrix3D->_42  * mMatrix3D->_34 +
> +                 mMatrix3D->_24 * mMatrix3D->_32  * mMatrix3D->_43 -
> +                 mMatrix3D->_24 * mMatrix3D->_42  * mMatrix3D->_33;

this should move to Matrix4x4.

@@ +981,5 @@
> +    mMatrix3D->_43 = NAN;
> +    mMatrix3D->_14 = NAN;
> +    mMatrix3D->_24 = NAN;
> +    mMatrix3D->_34 = NAN;
> +    mMatrix3D->_44 = NAN;

Add a SetNAN() method to Matrix4x4 and call it from here (and from Matrix4x4::Invert()).

::: content/base/src/DOMMatrix.h
@@ +111,5 @@
> +  JSObject*                   ToFloat64Array(JSContext* cx) const;
> +protected:
> +  nsCOMPtr<nsISupports> mParent;
> +  gfx::Matrix*          mMatrix2D;
> +  gfx::Matrix4x4*       mMatrix3D;

nsAutoPtr for both of these

::: dom/tests/mochitest/general/test_DOMMatrix.html
@@ +126,5 @@
> +
> +function testCreateMatrix()
> +{
> +  var m = new DOMMatrix();
> +  

trailing whitespace

::: dom/webidl/DOMMatrix.webidl
@@ +57,5 @@
> +    DOMMatrix rotate(unrestricted double angle,
> +                     optional unrestricted double originX = 0,
> +                     optional unrestricted double originY = 0);
> +    DOMMatrix rotateFromVector(unrestricted double x,
> +                                        unrestricted double y);

fix indent

@@ +71,5 @@
> +    DOMMatrix inverse();
> +
> +    // Helper methods
> +    boolean             is2D();
> +    boolean             maybeHasTransform();

I'm currently leaning towards isIdentity().

Remind me why these aren't boolean attributes?

@@ +140,5 @@
> +                                  unrestricted double y,
> +                                  unrestricted double z,
> +                                  unrestricted double angle);
> +    DOMMatrix skewXThis(unrestricted double sx);
> +    DOMMatrix skewYThis(unrestricted double sy);

Looks like we're going with "self".

Shouldn't these all return void, per spec? I think that's right behavior anyway; makes it harder to accidentally use the in-place method when you meant the functional method.

@@ +141,5 @@
> +                                  unrestricted double z,
> +                                  unrestricted double angle);
> +    DOMMatrix skewXThis(unrestricted double sx);
> +    DOMMatrix skewYThis(unrestricted double sy);
> +    boolean invert();

This should be "invertSelf()".

	Rik Cabanier Assignee
	Comment 8 • 10 years ago

Addressed most things from your feedback.
The in-place methods are spec'ed to return the object so I left that in. ie http://dev.w3.org/fxtf/geometry/#dom-dommatrix-multiplyself

	Rik Cabanier Assignee
	Comment 9 • 10 years ago

try build: https://tbpl.mozilla.org/?tree=Try&rev=93e79665a479

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 10 years ago

Comment on attachment 8439603 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

::: content/base/src/DOMMatrix.cpp
@@ +38,5 @@
> +                         double originX,
> +                         double originY) const
> +{
> +  nsRefPtr<DOMMatrix> retval = new DOMMatrix(mParent, *this);
> +  retval = retval->ScaleSelf(scale, originX, originY);

Like I said before, no pointing in assigning "retval" here. We know it's going to be unchanged and this just causes unnecessary refcount churn.

@@ +175,5 @@
> +
> +bool
> +DOMMatrixReadOnly::Is2D() const
> +{
> +  return mMatrix3D == nullptr;

We should check the coefficients of mMatrix3D if it's not null, I think. Dirk convinced me :-).

@@ +232,5 @@
> +  }
> +
> +  float* data = JS_GetFloat32ArrayData(array);
> +  data[0] = M11();
> +  data[1] = M12();

How about my helper function idea?

@@ +315,5 @@
> +  aMatrix->SetB(static_cast<T>(aData[1]));
> +  aMatrix->SetC(static_cast<T>(aData[2]));
> +  aMatrix->SetD(static_cast<T>(aData[3]));
> +  aMatrix->SetE(static_cast<T>(aData[4]));
> +  aMatrix->SetF(static_cast<T>(aData[5]));

You can fold these two helper functions into a single function SetDataFromMatrix that takes a length parameter and does the "if (length == 6)" stuff as well.

These static casts don't do anything. Only GetDataFromMatrix would need casts.

::: content/base/src/DOMMatrix.h
@@ +34,5 @@
> +  DOMMatrixReadOnly(nsISupports* aParent, const DOMMatrixReadOnly& other)
> +    : mParent(aParent)
> +  {
> +    if (other.mMatrix2D)
> +      mMatrix2D = new gfx::Matrix(*other.mMatrix2D);

{}

@@ +35,5 @@
> +    : mParent(aParent)
> +  {
> +    if (other.mMatrix2D)
> +      mMatrix2D = new gfx::Matrix(*other.mMatrix2D);
> +    if (other.mMatrix3D)

This can just be "else" now

@@ +36,5 @@
> +  {
> +    if (other.mMatrix2D)
> +      mMatrix2D = new gfx::Matrix(*other.mMatrix2D);
> +    if (other.mMatrix3D)
> +      mMatrix3D = new gfx::Matrix4x4(*other.mMatrix3D);

{}

@@ +163,5 @@
> +  Constructor(const GlobalObject& aGlobal, const binding_detail::AutoSequence<double>& numberSequence, ErrorResult& aRV);
> +
> +  nsISupports* GetParentObject() const { return mParent; }
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;
> +  

trailing whitespace

::: dom/webidl/DOMMatrix.webidl
@@ +70,5 @@
> +    DOMMatrix flipY();
> +    DOMMatrix inverse();
> +
> +    // Helper methods
> +    readonly attribute boolean is2D;

is2d to match scale3d?

::: gfx/2d/Matrix.cpp
@@ +101,5 @@
>  
>    return Rect(min_x, min_y, max_x - min_x, max_y - min_y);
>  }
>  
> +bool 

trailing whitespace

@@ +150,3 @@
>  }
> +
> +void 

trailing whitespace

	Rik Cabanier Assignee
	Comment 12 • 10 years ago

Comment on attachment 8439603 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

::: content/base/src/DOMMatrix.cpp
@@ +38,5 @@
> +                         double originX,
> +                         double originY) const
> +{
> +  nsRefPtr<DOMMatrix> retval = new DOMMatrix(mParent, *this);
> +  retval = retval->ScaleSelf(scale, originX, originY);

That causes a crash :-)
If I don't make that assignment, the refcount goes to 0 and the object is deleted.

@@ +175,5 @@
> +
> +bool
> +DOMMatrixReadOnly::Is2D() const
> +{
> +  return mMatrix3D == nullptr;

No,
Dirk is fine with this. Once it's a 3d matrix, it will always be one. for instance, see http://dev.w3.org/fxtf/geometry/#dom-dommatrix-translateself

@@ +232,5 @@
> +  }
> +
> +  float* data = JS_GetFloat32ArrayData(array);
> +  data[0] = M11();
> +  data[1] = M12();

I did it for the other way, but not this one.
Will fix!

@@ +315,5 @@
> +  aMatrix->SetB(static_cast<T>(aData[1]));
> +  aMatrix->SetC(static_cast<T>(aData[2]));
> +  aMatrix->SetD(static_cast<T>(aData[3]));
> +  aMatrix->SetE(static_cast<T>(aData[4]));
> +  aMatrix->SetF(static_cast<T>(aData[5]));

ok. Will fix

::: dom/webidl/DOMMatrix.webidl
@@ +70,5 @@
> +    DOMMatrix flipY();
> +    DOMMatrix inverse();
> +
> +    // Helper methods
> +    readonly attribute boolean is2D;

not sure what you mean. Can you elaborate?

	Rik Cabanier Assignee
	Comment 14 • 10 years ago

Comment on attachment 8439679 [details] [diff] [review]
Implementation + tests for DOMMatrix

try run:  https://tbpl.mozilla.org/?tree=Try&rev=3652302b8fdf

Let me know if/how I need to rewrite the code to avoid the extra assignment

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 15 • 10 years ago

(In reply to Rik Cabanier from comment #12)
> ::: content/base/src/DOMMatrix.cpp
> @@ +38,5 @@
> > +                         double originX,
> > +                         double originY) const
> > +{
> > +  nsRefPtr<DOMMatrix> retval = new DOMMatrix(mParent, *this);
> > +  retval = retval->ScaleSelf(scale, originX, originY);
> 
> That causes a crash :-)
> If I don't make that assignment, the refcount goes to 0 and the object is
> deleted.

I don't know why it would crash, but I guess it would leak instead.

You can make the *Self methods return a raw DOMMatrix* without add-reffing. We should definitely do that no matter what. Then remove this assignment to retval here. If we still crash, tell me why :-).

> Dirk is fine with this. Once it's a 3d matrix, it will always be one. for
> instance, see http://dev.w3.org/fxtf/geometry/#dom-dommatrix-translateself

OK

> ::: dom/webidl/DOMMatrix.webidl
> @@ +70,5 @@
> > +    DOMMatrix flipY();
> > +    DOMMatrix inverse();
> > +
> > +    // Helper methods
> > +    readonly attribute boolean is2D;
> 
> not sure what you mean. Can you elaborate?

Why does scale3d use lowercase 'd' but is2D uses uppercase 'D'?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 10 years ago

Comment on attachment 8439679 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

::: content/base/src/DOMMatrix.cpp
@@ +206,5 @@
> +    retval->SetX(transformedPoint.x);
> +    retval->SetY(transformedPoint.y);
> +    retval->SetZ(transformedPoint.z);
> +    retval->SetW(transformedPoint.w);
> +  } else if (mMatrix2D) {

just 'else' since mMatrix2D is guaranteed non-null at this point

@@ +327,5 @@
> +  const float* data = array32.Data();
> +  if (array32.Length() == 6) {
> +    SetDataFromMatrix<float, 6>(obj, data);
> +  } else if (array32.Length() == 16) {
> +    SetDataFromMatrix<float, 16>(obj, data);

Hmm, was I not clear before?

SetDataFromMatrix should take T* aData, uint32_t aLength, and ErrorResult& aRv. Then you can move these length checks into SetDataFromMatrix so they're not repeated.

::: content/base/src/DOMMatrix.h
@@ +177,5 @@
> +
> +#define Set3DMatrixMember(entry3D) \
> +{ \
> +  Ensure3DMatrix(); \
> +  mMatrix3D->entry3D = v; \

The spec says that setting a 3D-only attribute to its default value should not set is2D to false, but this code will.

	Rik Cabanier Assignee
	Comment 17 • 10 years ago

Comment on attachment 8439679 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

::: content/base/src/DOMMatrix.cpp
@@ +206,5 @@
> +    retval->SetX(transformedPoint.x);
> +    retval->SetY(transformedPoint.y);
> +    retval->SetZ(transformedPoint.z);
> +    retval->SetW(transformedPoint.w);
> +  } else if (mMatrix2D) {

will do

@@ +327,5 @@
> +  const float* data = array32.Data();
> +  if (array32.Length() == 6) {
> +    SetDataFromMatrix<float, 6>(obj, data);
> +  } else if (array32.Length() == 16) {
> +    SetDataFromMatrix<float, 16>(obj, data);

OK. I was trying to be clever and "bake" in the length so it would be faster.

	Rik Cabanier Assignee
	Comment 18 • 10 years ago

integrated your comments + added test case for 3d matrix initialisation

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 10 years ago

Comment on attachment 8439693 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

Please add a test that setting a 3d-only member to its default value does not set is2D to false.

::: content/base/src/DOMMatrix.cpp
@@ +296,5 @@
> +  return obj.forget();
> +}
> +
> +
> +template <typename T> void SetDataFromMatrix(DOMMatrix* aMatrix, const T* aData, int length, ErrorResult& aRv)

aLength

::: content/base/src/DOMMatrix.h
@@ +176,5 @@
> +}
> +
> +#define Set3DMatrixMember(entry3D, default) \
> +{ \
> +  if ((v != default) || mMatrix3D) { \

Swap the order of || checks here.

	Rik Cabanier Assignee
	Comment 23 • 10 years ago

Comment on attachment 8439693 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

I added a test for that in the last review. line 382 in test_DOMMatrix.html

::: content/base/src/DOMMatrix.h
@@ +176,5 @@
> +}
> +
> +#define Set3DMatrixMember(entry3D, default) \
> +{ \
> +  if ((v != default) || mMatrix3D) { \

done

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 25 • 10 years ago

Comment on attachment 8439697 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

::: content/base/src/DOMMatrix.cpp
@@ +309,5 @@
> +  aMatrix->SetM13(aData[2]);
> +  aMatrix->SetM14(aData[3]);
> +  aMatrix->SetM21(aData[4]);
> +  aMatrix->SetM22(aData[5]);
> +  if (aLength == 16) {

I think you need completely separate paths for 6 vs 16. E.g. when aLength is 6, aData[5] should map to _41 instead of _22. Probably just call setA...setF instead of SetM.

Also, needs a test I guess :-)

	Rik Cabanier Assignee
	Comment 26 • 10 years ago

Comment on attachment 8439697 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

::: content/base/src/DOMMatrix.cpp
@@ +309,5 @@
> +  aMatrix->SetM13(aData[2]);
> +  aMatrix->SetM14(aData[3]);
> +  aMatrix->SetM21(aData[4]);
> +  aMatrix->SetM22(aData[5]);
> +  if (aLength == 16) {

o yes. This doesn't work correctly.
I'll fix it and add tests.

	Alex Vincent [:WeirdAl]
	Comment 27 • 10 years ago

Comment on attachment 8439697 [details] [diff] [review]
Implementation + tests for DOMMatrix

For C++ code, feel free to ignore everything I say.  For JS code, I think you should seriously consider this an r-.

>diff --git a/content/base/src/DOMMatrix.cpp b/content/base/src/DOMMatrix.cpp
>+static const double radPerDegree = 2.0 * M_PI / 360.0;

M_PI / 180.0 ?

>+already_AddRefed<DOMMatrix>
>+DOMMatrixReadOnly::Translate(double tx,
>+                             double ty,
>+                             double tz) const
>+{
>+  nsRefPtr<DOMMatrix> retval = new DOMMatrix(mParent, *this);
>+  retval = retval->TranslateSelf(tx, ty, tz);
>+
>+  return retval.forget();
>+}

nsRefPtr<DOMMatrix> m = new DOMMatrix(mParent, *this);
return m->TranslateSelf(tx, ty, tz).forget() ?

I'm not sure this would be better from a debugging standpoint.

>+already_AddRefed<DOMMatrix>
>+DOMMatrixReadOnly::FlipX() const
>+{
>+  nsRefPtr<DOMMatrix> retval = new DOMMatrix(mParent, *this);
>+  if (mMatrix3D) {
>+    gfx::Matrix4x4 m;
>+    m._11 = -1;
>+    retval->mMatrix3D = new gfx::Matrix4x4(m * *mMatrix3D);
>+  } else {
>+    gfx::Matrix m;
>+    m._11 = -1;
>+    retval->mMatrix2D = new gfx::Matrix(mMatrix2D? m * *mMatrix2D : m);
>+  }
>+
>+  return retval.forget();
>+}

A note on the * operator:  someone new to C++ might think you were deferencing mMatrix3D or mMatrix2D twice.  I suggest wrapping *mMatrix3D and *mMatrix2D in parentheses, even if it's not strictly speaking necessary.


>+already_AddRefed<DOMPoint>
>+DOMMatrixReadOnly::TransformPoint(const DOMPointInit& point) const

>+    retval->SetX(transformedPoint.x);
>+    retval->SetY(transformedPoint.y);
>+    retval->SetZ(transformedPoint.z);
>+    retval->SetW(transformedPoint.w);

Wouldn't it be nice if you could make a single C++ call to set all four coordinates at once?  Maybe DOMMatrix and DOMPoint could be friends of one another (particularly since they're closely related).

>+DOMMatrix*
>+DOMMatrix::MultiplySelf(const DOMMatrix& other)
>+{
...
>+}
>+
>+DOMMatrix*
>+DOMMatrix::PreMultiplySelf(const DOMMatrix& other)
>+{
...
>+}

Would it be a good idea for one of these to call the other with inverted arguments, to reduce duplication of code?

>+DOMMatrix*
>+DOMMatrix::RotateAxisAngleSelf(double x, double y,
>+                               double z, double angle)


>+  angle *= radPerDegree;
>+  double sc = sin(angle / 2) * cos(angle / 2);

sin 2k = 2 * sin k * cos k
(sin 2k) / 2 = sin k * cos k

k = angle/2 -> 2k = angle
double sc = sin(angle) / 2

It's a tiny optimization, but worth doing.  The original can be a comment.  Or, if it's *not* worth doing, please include a comment why for us novices.  :)

>+  double sq = pow(sin(angle / 2), 2);

sin^2 k = (1 - cos 2k) / 2
k = angle/2 -> 2k = angle
double sq = (1 - cos(angle)) / 2.

Again, worth converting and commenting.

>+DOMMatrix*
>+DOMMatrix::SkewXSelf(double sx)

...

>+DOMMatrix*
>+DOMMatrix::SkewYSelf(double sy)

Honestly, I wish the spec provided SkewXRadSelf and SkewYRadSelf methods.  After all, Math.sin, Math.cos, etc. all base their arguments on radians - and for good reason, since calculus shows the sine, cosine, etc. functions can be generated from a Maclaurin series based in radians.

We really should push on the spec editors for this!  When the rest of JS land's trigonometry functions are radian-based, there's no reason to make JS developers do a conversion from radians to degrees, and then the internal code converting back from degrees to radians.  Nor should there be a risk of developers accidentally passing in radians and expecting a transform in radians - and getting a transform in degrees.

I suppose I should ask why there isn't a SkewZSelf method.  I bet the answer's obvious to someone who knows what they're talking about.

>+bool
>+DOMMatrix::InvertSelf()
>+{
>+  if (mMatrix3D) {
>+    if (!mMatrix3D->Invert()) {
>+      mMatrix3D->SetNAN();
>+      return false;
>+    }
>+    return true;
>+  }
>+
>+  if (!mMatrix2D->Invert()) {
>+    mMatrix2D = nullptr;
>+
>+    mMatrix3D = new gfx::Matrix4x4();
>+    mMatrix3D->SetNAN();
>+  }
>+
>+  return true;
>+}

I'm betting it's in the spec, but why does SetNaN() not always mean InvertSelf returns false?  (Looking at the 2-D path.)

>diff --git a/content/base/src/DOMMatrix.h b/content/base/src/DOMMatrix.h
>new file mode 100644
>--- /dev/null
>+++ b/content/base/src/DOMMatrix.h

I think it would be wise to provide a link to the specification as a comment.  Just so people like me who don't fully understand matrix transforms can know where to look for the definitive guide.

>diff --git a/dom/tests/mochitest/general/test_DOMMatrix.html b/dom/tests/mochitest/general/test_DOMMatrix.html

When I implemented XMLHttpRequest.timeout, someone asked to port the tests to the W3C test suite.  See bug 525816 comment 86.  Just for convenience's sake, you might want to think about granting permission for the same.

Also, since you base many of your calculations on trigonometric functions, perhaps it would be a good idea to point to a reference SVG document with some illustrative figures in comments.  This would be so readers could see a visualization of the transformations, and compare them to the results you're trying to achieve.  (This is a pain in the backside requirement, to be sure, but numbers can sometimes hide the beauty of geometry.)

>+// Lightweight dummy Matrix class for representing arrays that get passed in
>+function MatrixFromArray(a)
>+{
>+  this.a = a[0];
>+  this.b = a[1];
>+  this.c = a[2];
>+  this.d = a[3];
>+  this.e = a[4];
>+  this.f = a[5];
>+}

Array comprehensions, anyone?

>+function cmpMatrix(a, b, msg)
>+{
>+  if (a.constructor === Array)
>+    a = new MatrixFromArray(a);
>+  if (b.constructor === Array)
>+    b = new MatrixFromArray(b);

Don't do this.  Array.isArray(a), Array.isArray(b).

>+  ok(a.a == b.a &&
>+     a.b == b.b &&
>+     a.c == b.c &&
>+     a.d == b.d &&
>+     a.e == b.e &&
>+     a.f == b.f,
>+     msg + " - got " + formatMatrix(a)
>+         + ", expected " + formatMatrix(b));

Suggestion:

const abcdef = ["a", "b", "c", "d", "e", "f"];
function cmpMatrixHelper(m1, m2, h) {
  return m1[h] == m2[h];
}

function cmpMatrix(m1, m2, msg) {
  if (Array.isArray(m1))
    m1 = new MatrixFromArray(m1);
  if (Array.isArray(b))
    m2 = new MatrixFromArray(m2);

  ok(abcdef.every(cmpMatrixHelper.bind(m1, m2), null),
     msg + " - got " + formatMatrix(m1) + ", expected " + formatMatrix(m2));
}

Something like that.


>+function roughCmpMatrix(a, b, msg)
>+{
>+  if (a.constructor === Array)
>+    a = new MatrixFromArray(a);
>+  if (b.constructor === Array)
>+    b = new MatrixFromArray(b);
>+  const tolerance = 1 / 65535;
>+  ok(Math.abs(b.a - a.a) < tolerance &&
>+     Math.abs(b.b - a.b) < tolerance &&
>+     Math.abs(b.c - a.c) < tolerance &&
>+     Math.abs(b.d - a.d) < tolerance &&
>+     Math.abs(b.e - a.e) < tolerance &&
>+     Math.abs(b.f - a.f) < tolerance,
>+     msg + " - got " + formatMatrix(a)
>+         + ", expected " + formatMatrix(b));
>+}

This is even better for using every:

function roughCmpMatrixHelper(m1, m2, tolerance, h) {
  return Math.abs(m1[h] - m2[h]) < tolerance;
}

function roughCmpMatrix(m1, m2, message) {
  if (Array.isArray(m1))
    m1 = new MatrixFromArray(m1);
  if (Array.isArray(m2))
    m2 = new MatrixFromArray(m2);
  const tolerance = 1 / 65535;
  ok(abcdef.every(roughCmpMatrixHelper.bind(null, m1, m2, tolerance),
     msg + " - got " + formatMatrix(a) + ", expected " + formatMatrix(b));
}

>+function formatMatrix(m)
>+{
>+  if (m.constructor != Array)

if (!Array.isArray(m))

Actually, you should be asserting (m instanceof DOMMatrix) somewhere, if m isn't an array.

>+function main()
>+{

>+  for (var i = 0; i < tests.length; i++) {
>+    tests[i]();
>+  }

You'll want all tests to run, so a try...catch, and maybe a forEach, is probably a good idea:

var firstExn = null;
tests.forEach(function(test) {
  try {
    test();
  }
  catch (e) {
    if (!firstExn)
      firstExn = e;
  }
}, null);
if (firstExn)
  throw firstExn;

There might be an even shorter way of doing that. (I keep hearing about function*, but I have never used it and don't know what it means.)

>+// DOMMatrix multiply(in DOMMatrix secondMatrix);
>+function testMultiply()
>+{
>+  var m1 = createMatrix(1, 0, 0, 1, 50, 90);
>+  var m2 = createMatrix(0.707, -0.707, 0.707, 0.707, 0, 0);
>+  var m3 = createMatrix(1, 0, 0, 1, 130, 160);
>+  var result = m1.multiply(m2).multiply(m3);
>+  roughCmpMatrix(result, [0.707, -0.707, 0.707, 0.707, 255.03, 111.21],
>+    "Unexpected result after multiplying matrices");

These numbers need some explanation, and probably can be more precise.  What's the significance of 255.03, 111.21?  130, 160?

For 0.707, I'd say use Math.SQRT1_2.  There's also Math.SQRT2 and Math.PI available.  Express your magical decimal numbers in these terms, please.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/

That might also help with your tolerances in roughCmpMatrix; you can make the tolerances even tighter - or possibly eliminate them altogether.

It wouldn't hurt to have radians-to-degrees and degrees-to-radians conversion functions available:
function degToRad(x) {
  return x * 180 / Math.PI;
}
function radToDeg(x) {
  return x * Math.PI / 180;
}

Again, JS devs tend to think in terms of radians, so the spec requiring degrees is really bizarre from that standpoint.

>+// DOMMatrix inverse() raises(SVGException);
>+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);
>+  m = m.inverse();
>+  ok(m.a != m.a, "Failed to invalidate inverted singular matrix, got " + m.a);

Is this an isNaN test?  If so, assert so:
ok(isNaN(m.a), ...);
Better to check all six members (abcdef).  That way, an accidental bug where isNaN(m.a) but !isNaN(m.b) doesn't get by you.

>+  ok(!m.is2D, "Failed to mark invalidated inverted singular matrix as 3D.");

It seems like every one of your generated matrices should have a .is2D check, yea or nay.

>+// DOMMatrix rotate(in float angle);
>+function testRotate()
>+{
>+  var m = createMatrix(2, 0, 0, 1, 120, 100);
>+  roughCmpMatrix(m.rotate(45),
>+                 [2*Math.cos(Math.PI/4), Math.sin(Math.PI/4),
>+                  2*-Math.sin(Math.PI/4), Math.cos(Math.PI/4),
>+                  120, 100],
>+                 "Unexpected result after rotate");
>+}

Again, Math.SQRT2, Math.SQRT1_2, etc. are your friends.  (Though to be honest, this kind of explanation is also good.  Maybe put these calculated results in a comment, and use the actual results they would generate from the Math constants.)

>+// DOMMatrix rotateFromVector(in float x, in float y) raises(SVGException);
>+function testRotateFromVector()
>+{
>+  var m = createMatrix(2, 0, 0, 1, 120, 100);
>+  // Make a 150 degree angle
>+  var result = m.rotateFromVector(-2, 1.1547);
>+  roughCmpMatrix(result,
>+                 [2*Math.cos(5*Math.PI/6), Math.sin(5*Math.PI/6),
>+                  2*-Math.sin(5*Math.PI/6), Math.cos(5*Math.PI/6),
>+                  120, 100],
>+                 "Unexpected result after rotateFromVector");

... and again...

>+
>+  // Test bad input (1)
>+  try {
>+    m.rotateFromVector(1, 0);
>+    ok(true, "did not throw exception with zero coord for rotateFromVector");
>+  } catch (e) {
>+    ok(false,
>+      "Got unexpected exception " + e + ", expected NotSupportedError");
>+  }
>+
>+  // Test bad input (2)
>+  try {
>+    m.rotateFromVector(0, 1);
>+    ok(true, "did not throw exception with zero coord for rotateFromVector");
>+  } catch (e) { }
>+}

var exn = null;
try {
 // ...
}
catch (e) {
  exn = e;
}
ok(!exn, "should not throw exception (" + exn + ") with ...");

Empty catch block above defeats the purpose of the ok().

I see no tests here where a call to the DOMMatrix API should throw.  You really need to test that too, using SimpleTest.doesThrow at the least.  http://mxr.mozilla.org/mozilla-release/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#275

I'd feel more comfortable if you checked the type (using instanceof), the message and error code (if there is one) of thrown exceptions as well.

In particular, test a divide-by-zero situation if you haven't already!  Better to throw an exception on what should not be legal than to crash because we didn't check it (and someone else did).

>+// DOMMatrix skewX(in float angle);
>+function testSkewX()
>+{
>+  var m = createMatrix(2, 0, 0, 1, 120, 100);
>+  roughCmpMatrix(m.skewX(30), [2, 0, 2*Math.tan(Math.PI/6), 1, 120, 100],
>+                 "Unexpected result after skewX");
>+}

const SQRT3_3 = Math.sqrt(3) / 3; // == Math.tan(Math.PI / 6);
const SQRT3_2 = Math.sqrt(3) / 2; // == Math.cos(Math.PI / 6);

So you won't need to compute it more than once.

>+// DOMMatrix multiply(in DOMMatrix secondMatrix);
>+function testMultiplyInPlace()

... more magical numbers.

>+function testRotateInPlace()

Math.SQRT2, Math.SQRT1_2, etc.

>+function testRotateFromVectorInPlace()
>+  // Make a 150 degree angle
>+  m.rotateFromVectorSelf(-2, 1.1547);

m.rotateFromVectorSelf(-2, degToRad(150));

>+  roughCmpMatrix(m,
>+                 [2*Math.cos(5*Math.PI/6), Math.sin(5*Math.PI/6),
>+                  2*-Math.sin(5*Math.PI/6), Math.cos(5*Math.PI/6),
>+                  120, 100],
>+                 "Unexpected result after rotateFromVector");

cos(5 * pi / 6) = -cos(pi / 6) = -Math.sqrt(3) / 3, and sin(5 * pi / 6) = sin(pi / 6) = 0.5, I believe.  For that matter, consider you don't need to calculate cos twice here, sin twice here, and 5 * Math.pi / 6 four times.

Again, putting it in a comment for illustration is great.  But go with values that are known constants.

>+function testCreateMatrix3D()
>+  ok(m.is2D == true, "should not produce 3d matrix");

ise(m.is2D, true, "should not produce 3d matrix");

>+  ok(m.is2D == false, "should produce 3d matrix");

ise(m.is2D, false, "should produce 3d matrix");

== is not as strong a check as ===. (undefined == false, but undefined !== false)

Beyond this point, I'm going to try and not repeat myself so much.  A lot of what I've said above can apply below.

>+Matrix3D.prototype = {
>+translate: function(x, y, z, result) {

Please indent the properties and methods of Matrix3D.prototype by two spaces more.  I thought at first there was a syntax error when the prototype ended.

>+inverse: function(matrix, result) {

Oh, boy.  I hope this is correct. (I haven't studied linear algebra in college yet.)  I'd suggest lining each term up vertically, even if that means extra whitespace, because it'll be hard to check the coordinates otherwise.  Even m[ 2] might be better than m[2], for readability.

Are any of these terms duplicated?  (Meaning, can they be pre-calculated before being assigned in the sums?)

I'd feel safer if you pointed to an explicit hyperlink reference that confirms this calculation is correct.

>+  var det = m[0]*r[0] + m[1]*r[4] + m[2]*r[8] + m[3]*r[12];
>+  for (var i = 0; i < 16; i++) r[i] /= det;

Heaven help you if det === 0.  (You need to test for this!)

>+multiply: function(left, result) {
Reference hyperlink.

>+scale: function(x, y, z, result) {

I don't think this function does what you want it to do, unless the matrix you're transforming is already an identity matrix.  (Again, because I haven't studied linear algebra in over 15 years, I could be wrong here.)

>+rotate: function(a, x, y, z, result) {
>+  var d = Math.sqrt(x*x + y*y + z*z);

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/hypot :)

Reference hyperlink.

>+swap: function(result){
Reference hyperlink.

>diff --git a/dom/webidl/DOMMatrix.webidl b/dom/webidl/DOMMatrix.webidl
>\ No newline at end of file

Tsk, tsk.  :)

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>+// Is support for DOMMatrix enabled?
>+pref("layout.css.DOMMatrix.enabled", true);

Do we really want this enabled by default?

	Alex Vincent [:WeirdAl]
	Comment 28 • 10 years ago

Comment on attachment 8439697 [details] [diff] [review]
Implementation + tests for DOMMatrix

Sorry, roc, didn't mean to blow away your r-.

	Alex Vincent [:WeirdAl]
	Comment 29 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #27)
> It wouldn't hurt to have radians-to-degrees and degrees-to-radians
> conversion functions available:
> function degToRad(x) {
>   return x * 180 / Math.PI;
> }
> function radToDeg(x) {
>   return x * Math.PI / 180;
> }

I got these backwards.  It should've been:

function radToDeg(x) {
  return x * 180 / Math.PI;
}
function degToRad(x) {
  return x * Math.PI / 180;
}

	Alex Vincent [:WeirdAl]
	Comment 30 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #27)
> cos(5 * pi / 6) = -cos(pi / 6) = -Math.sqrt(3) / 3, and sin(5 * pi / 6) =
> sin(pi / 6) = 0.5, I believe.  For that matter, consider you don't need to
> calculate cos twice here, sin twice here, and 5 * Math.pi / 6 four times.

Sorry, another mistake.  cos (pi / 6) = Math.sqrt(3) / 2, not Math.sqrt(3) / 3.

	Rik Cabanier Assignee
	Comment 31 • 10 years ago

Fixed bug that roc spotted + added tests that detect error conditions.

Implemented most of Alex' suggestions.

	Alex Vincent [:WeirdAl]
	Comment 32 • 10 years ago

Comment on attachment 8442477 [details] [diff] [review]
Implementation + tests for DOMMatrix

>+DOMMatrix*
>+DOMMatrix::RotateAxisAngleSelf(double x, double y,
>+                               double z, double angle)

>+  // sin(angle / 2) * cos(angle / 2)
>+  double sc = sin(angle / 2);

That is *not* the same as sin (angle/2) * cos(angle/2).  Presumably you reran the tests before posting this patch, so how did this slip by?  (It indicates a need for yet another test.)

>diff --git a/dom/tests/mochitest/general/test_DOMMatrix.html b/dom/tests/mochitest/general/test_DOMMatrix.html

>+function CompareMatrix(dm, m)
>+{
>+  var ma = m.toFloat32Array();
>+  for(var x = 0; x < ma.length; x++) {
>+    if(Math.abs(ma[x] - dm.m[x]) > 0.000001)
>+      return false;
>+  }
>+
>+  return true;
>+}
>+
>+function CompareDOMMatrix(dm1, dm2)
>+{
>+  var m1 = dm1.toFloat32Array();
>+  var m2 = dm2.toFloat32Array();
>+
>+  for(var x = 0; x < m1.length; x++) {
>+    if(Math.abs(m1[x] - m2[x]) > 0.000001)
>+      return false;
>+  }
>+
>+  return true;
>+}
>+
>+function RoughCompareDOMMatrix(dm1, dm2)
>+{
>+  var m1 = dm1.toFloat32Array();
>+  var m2 = dm2.toFloat32Array();
>+
>+  const tolerance = 1 / 65535;
>+  for(var x = 0; x < m1.length; x++) {
>+    if(Math.abs(m1[x] - m2[x]) > tolerance)
>+      return false;
>+  }
>+
>+  return true;
>+}

You should check in all three of these functions that m1.length and m2.length are equal.

>+function main()
...
>+  for (var i = 0; i < tests.length; i++) {
>+    try{
>+      tests[i]();
>+    } catch (e) {
>+      ok(false, "uncaught exception in test " + i);
>+    }
>+  }

Can you tell people what the exception was?  :)

>+function testMultiply()
>+{
>+  var m1 = createMatrix(1, 0, 0, 1, 50, 90);
>+  var m2 = createMatrix(0.707, -0.707, 0.707, 0.707, 0, 0);
>+  var m3 = createMatrix(1, 0, 0, 1, 130, 160);
>+  var result = m1.multiply(m2).multiply(m3);
>+  roughCmpMatrix(result, [0.707, -0.707, 0.707, 0.707, 255.03, 111.21],
>+    "Unexpected result after multiplying matrices");
>+
>+  // Check orig matrices are unchanged
>+  cmpMatrix(m1, [1, 0, 0, 1, 50, 90], "Matrix changed after multiplication");
>+  roughCmpMatrix(m2, [0.707, -0.707, 0.707, 0.707, 0, 0],
>+                 "Matrix changed after multiplication");
>+  cmpMatrix(m3, [1, 0, 0, 1, 130, 160], "Matrix changed after multiplication");
>+}

I was quite serious about using Math.SQRT1_2, and explaining the other numbers.  Was there something that didn't work about that?  If so, we need to understand what it was.

	Rik Cabanier Assignee
	Comment 33 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #32)
> Comment on attachment 8442477 [details] [diff] [review]
> Implementation + tests for DOMMatrix
> 
> >+DOMMatrix*
> >+DOMMatrix::RotateAxisAngleSelf(double x, double y,
> >+                               double z, double angle)
> 
> >+  // sin(angle / 2) * cos(angle / 2)
> >+  double sc = sin(angle / 2);
> 
> That is *not* the same as sin (angle/2) * cos(angle/2).  Presumably you
> reran the tests before posting this patch, so how did this slip by?  (It
> indicates a need for yet another test.)

Yeah, that was wrong. Is it:
  double sc = sin(angle) / 2;

> >diff --git a/dom/tests/mochitest/general/test_DOMMatrix.html b/dom/tests/mochitest/general/test_DOMMatrix.html
> 
> >+function CompareMatrix(dm, m)
> >+{
> >+  var ma = m.toFloat32Array();
> >+  for(var x = 0; x < ma.length; x++) {
> >+    if(Math.abs(ma[x] - dm.m[x]) > 0.000001)
> >+      return false;
> >+  }
> >+
> >+  return true;
> >+}
> >+
> >+function CompareDOMMatrix(dm1, dm2)
> >+{
> >+  var m1 = dm1.toFloat32Array();
> >+  var m2 = dm2.toFloat32Array();
> >+
> >+  for(var x = 0; x < m1.length; x++) {
> >+    if(Math.abs(m1[x] - m2[x]) > 0.000001)
> >+      return false;
> >+  }
> >+
> >+  return true;
> >+}
> >+
> >+function RoughCompareDOMMatrix(dm1, dm2)
> >+{
> >+  var m1 = dm1.toFloat32Array();
> >+  var m2 = dm2.toFloat32Array();
> >+
> >+  const tolerance = 1 / 65535;
> >+  for(var x = 0; x < m1.length; x++) {
> >+    if(Math.abs(m1[x] - m2[x]) > tolerance)
> >+      return false;
> >+  }
> >+
> >+  return true;
> >+}
> 
> You should check in all three of these functions that m1.length and
> m2.length are equal.

will do

> 
> >+function main()
> ...
> >+  for (var i = 0; i < tests.length; i++) {
> >+    try{
> >+      tests[i]();
> >+    } catch (e) {
> >+      ok(false, "uncaught exception in test " + i);
> >+    }
> >+  }
> 
> Can you tell people what the exception was?  :)

will do

> 
> >+function testMultiply()
> >+{
> >+  var m1 = createMatrix(1, 0, 0, 1, 50, 90);
> >+  var m2 = createMatrix(0.707, -0.707, 0.707, 0.707, 0, 0);
> >+  var m3 = createMatrix(1, 0, 0, 1, 130, 160);
> >+  var result = m1.multiply(m2).multiply(m3);
> >+  roughCmpMatrix(result, [0.707, -0.707, 0.707, 0.707, 255.03, 111.21],
> >+    "Unexpected result after multiplying matrices");
> >+
> >+  // Check orig matrices are unchanged
> >+  cmpMatrix(m1, [1, 0, 0, 1, 50, 90], "Matrix changed after multiplication");
> >+  roughCmpMatrix(m2, [0.707, -0.707, 0.707, 0.707, 0, 0],
> >+                 "Matrix changed after multiplication");
> >+  cmpMatrix(m3, [1, 0, 0, 1, 130, 160], "Matrix changed after multiplication");
> >+}
> 
> I was quite serious about using Math.SQRT1_2, and explaining the other
> numbers.  Was there something that didn't work about that?  If so, we need
> to understand what it was.

I tried that and it created more rounding issues. I'll take another look.

Do you want to take over writing of the test? Dirk is also creating W3C tests that we should import.

	Rik Cabanier Assignee
	Comment 34 • 10 years ago

Adressed bug Alex found and integrated his change.
Did not add a better test for rotation yet.

Alex, do you want to take over writing the test?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 35 • 10 years ago

Comment on attachment 8442611 [details] [diff] [review]
Implementation + tests for DOMMatrix

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

Just some cosmetic changes... the rest looks good.

Needs DOM peer review though

::: content/base/src/DOMMatrix.cpp
@@ +24,5 @@
> +
> +already_AddRefed<DOMMatrix>
> +DOMMatrixReadOnly::Translate(double tx,
> +                             double ty,
> +                             double tz) const

Oops, I should have mentioned this before, but you need to follow the 'a' prefix parameter naming convention all through this patch.

@@ +295,5 @@
> +  nsRefPtr<DOMMatrix> obj = new DOMMatrix(aGlobal.GetAsSupports(), other);
> +  return obj.forget();
> +}
> +
> +

Unnecessary blank line

::: dom/tests/mochitest/general/test_DOMMatrix.html
@@ +100,5 @@
> +    return false;
> +
> +  const tolerance = 1 / 65535;
> +  for(var x = 0; x < m1.length; x++) {
> +    if(Math.abs(m1[x] - m2[x]) > tolerance)

Space between "if" and "(" (many places in this file, also "for(")

@@ +111,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +function main()
> +{
> +  var tests = [ 

trailing whitespace

@@ +171,5 @@
> +  ok(!m.is2D, "Failed to mark matrix as 3D.");
> +
> +  var exn = null;
> +  try {
> +    m = new DOMMatrix([0]); 

here too

	Alex Vincent [:WeirdAl]
	Comment 37 • 10 years ago

(In reply to Rik Cabanier from comment #33)
> Do you want to take over writing of the test? Dirk is also creating W3C
> tests that we should import.

Ask me again in six months, when I know more about what we're talking about here.  :)

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 10 years ago

Comment on attachment 8442627 [details] [diff] [review]
Implementation + tests for DOMMatrix

So as a first note, the spec is vague enough about things here that reviewing this is pretty hard.  I did NOT review most of the actual matrix manipulation details, because I can't tell from the spec what would be correct anyway.  We really need to get this spec fixed.  As a simple example, none of the scale or rotate methods explain what the origin arguments do, exactly.  And the spec's usage of post-multiply tends to be pretty vague...

>+++ b/content/base/src/DOMMatrix.cpp
>+using namespace mozilla;
>+using namespace mozilla::dom;

Standard DOM code style for new files is to wrap in those namespaces, not to pull them in using "using".

>+    retval->mMatrix2D = new gfx::Matrix(mMatrix2D? m * *mMatrix2D : m);

Needs space before '?'.  And the "* *" bit is really hard to read, though I'm not sure I have a better proposal offhand.  There are multiple instances of this.

>+  return mMatrix3D == nullptr;

return !mMatrix3D is preferred, I think.

>+  if (mMatrix3D != nullptr) {

Likewise, if (mMatrix3D).

>+DOMMatrixReadOnly::TransformPoint(const DOMPointInit& point) const

Is the 2d behavior correct?  It's weird to have a 2d transformation like this affect x and y in a way that doesn't depend on w.

In particular, (2, 2, 2, 2) and (1, 1, 1, 1) are conceptually the same 3d point, assuming these are homogeneous coordinates, but will be transformed to different 3d points if TransformPoint is called with a 2d matrix, right?  Why is this the behavior we're implementing?

>+JSObject*
>+DOMMatrixReadOnly::ToFloat32Array(JSContext* aCx) const

This is the wrong function signature, as of June 11 or so.  This needs to take a mutable handle instead.

>+  JS::Rooted<JSObject*> array(aCx, JS_NewFloat32Array(aCx, 16));

This is wrong, unfortunately (even if we ignore it crashing on out of memory, i actually doesn't create objects in the right global in the long term).  The right way to do this sort of thing is more like this:

  nsAutoTArray<float, 16> arr;
  // Get your values into arr
  JS::Rooted<JS::Value> value(aCx);
  if (!ToJSValue(aCx, TypedArrayCreator<Float32Array>(arr), &value)) {
    // throw here; the function needs to be declared as throwing in the IDL
  }
  aRetVal.set(&value.toObject());

or so.

Same thing for the Float64 version.

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(DOMMatrix, mParent)

This needs to be on DOMMatrixReadOnly, no?

>+template <typename T> void SetDataFromMatrix(DOMMatrix* aMatrix, const T* aData, int aLength, ErrorResult& aRv)

SetDataInMatrix, perhaps?

Or SetMatrixFromData?

>+DOMMatrix::Constructor(const GlobalObject& aGlobal, const RootedTypedArray<Float32Array>& aArray32, ErrorResult& aRv)

const Float32Array& for the second argument.

>+  SetDataFromMatrix(obj, aArray32.Data(), aArray32.Length(), aRv);

Was this tested?  It's pretty conspicuously missing a ComputeLengthAndData() call, which has been required since before the try runs I see in this bug...  And I see no obvious test for this codepath in the attached tests.

Also, this codepath will allocate a 2d matrix only to immediately throw it away.  It might be a good idea to have a way to construct a DOMMatrix with a 3d matrix up front.

>+DOMMatrix::Constructor(const GlobalObject& aGlobal, const RootedTypedArray<Float64Array>& aArray64, ErrorResult& aRv)

Again |const Float64Array& aArray64|.  And need a ComputeLengthAndData() in the body.

>+already_AddRefed<DOMMatrix>
>+DOMMatrix::Constructor(const GlobalObject& aGlobal, const binding_detail::AutoSequence<double>& aNumberSequence, ErrorResult& aRv)

Which part of binding_detail was unclear?  ;)  The second argument here should be of type "const Sequence<double>&".

I strongly recommend either looking at https://developer.mozilla.org/en/Mozilla/WebIDL_bindings or at example codegen when deciding what signatures to use for WebIDL implementations, instead of looking at the generated binding code.

>+DOMMatrix::MultiplySelf(const DOMMatrix& aOther)

This, and PreMultiplySelf, don't obviously match the spec in terms of multiplication order.  If this is because what we're storing is actually a transpose of what he spec is storing (not that the spec seems to actually describe what it's storing at first glance), that deserves a nice comment.  Plus making the spec clearer, of course.

>+DOMMatrix::TranslateSelf(double aTx,
>+  if (!aTx && !aTy && !aTz) {

Here I think comparing to 0 would make it clearer what's being tested.

>+  if (mMatrix3D || aTz) {

And here.

>+DOMMatrix::Scale3dSelf(double aScale, double aOriginX,
>+  ScaleNonUniformSelf(aScale, aScale, 1.0, aOriginX, aOriginY, aOriginZ);

Why is the third argument not aScale?

>+DOMMatrix::ScaleNonUniformSelf(double aScaleX,
>+  if ((aScaleX == 1.0) && (aScaleY == 1.0) && (aScaleZ == 1.0)) {

This is overparenthesized.

>+  if (mMatrix3D || (aScaleZ != 1.0) || aOriginZ) {

And here.

>+bool
>+DOMMatrix::InvertSelf()

This return value doesn't match the spec.

>+++ b/content/base/src/DOMMatrix.h
>+#ifndef MOZILLA_DOMMATRIX_H_

"mozilla_dom_DOMMatrix_h" is the typical header guard we'd use here.  In particular, 

>+  DOMMatrixReadOnly(const DOMMatrixReadOnly&);
>+  DOMMatrixReadOnly& operator=(const DOMMatrixReadOnly&);

MOZ_DELETE on those, please.

And maybe make the constructor protected if this is not supposed to ever be instantiated on its own?

>+  DOMMatrix(nsISupports* aParent = NULL)

nullptr, not NULL.  But also, are there any cases that _don't_ pass a parent?

You should consistently use "aRv".  Or at least not randomly mix "aRv" and "aRV" as you do in the various constructor signatures.

>+++ b/dom/webidl/DOMMatrix.webidl
>\ No newline at end of file

Should be one.

Please post an interdiff for the update once you have one..

	Dirk Schulze
	Comment 39 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #38)

More commenting from the spec point of view...

> Comment on attachment 8442627 [details] [diff] [review]
> Implementation + tests for DOMMatrix
> 
> So as a first note, the spec is vague enough about things here that
> reviewing this is pretty hard.  I did NOT review most of the actual matrix
> manipulation details, because I can't tell from the spec what would be
> correct anyway.  We really need to get this spec fixed.  As a simple
> example, none of the scale or rotate methods explain what the origin
> arguments do, exactly.

Fair enough. I'll add translateSelf() operations before and after scaleSelf/rotateSelf to make this clear.

> And the spec's usage of post-multiply tends to be
> pretty vague...

The spec has a definition for post-multiply, multiply and pre-multiply. So this part should be clear.
http://dev.w3.org/fxtf/geometry/#post-multiply

> 
> >+DOMMatrix::MultiplySelf(const DOMMatrix& aOther)
> 
> This, and PreMultiplySelf, don't obviously match the spec in terms of
> multiplication order.  If this is because what we're storing is actually a
> transpose of what he spec is storing (not that the spec seems to actually
> describe what it's storing at first glance), that deserves a nice comment. 
> Plus making the spec clearer, of course.

I didn't look at the code here. The definition of multiply is defined earlier. From the spec point of view, what else should be defined? IMO the spec is clear how each element is stored in the matrix as well in the intro of DOMMatrix. Could elaborate what you think would make it more clear please?

	Boris Zbarsky [:bzbarsky]
	Comment 40 • 10 years ago

> http://dev.w3.org/fxtf/geometry/#post-multiply

Ah, ok.  And the mapping of the coords to the actual matrix is given by the little diagram in <http://dev.w3.org/fxtf/geometry/#DOMMatrix>?  In that case I agree that the spec has this defined clearly, yes.  I missed that diagram the first time through.

Note that my question about transformPoint is sort of a spec question too.

	Rik Cabanier Assignee
	Comment 41 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #38)
> Comment on attachment 8442627 [details] [diff] [review]
> Implementation + tests for DOMMatrix
> 
> So as a first note, the spec is vague enough about things here that
> reviewing this is pretty hard.  I did NOT review most of the actual matrix
> manipulation details, because I can't tell from the spec what would be
> correct anyway.  We really need to get this spec fixed.  As a simple
> example, none of the scale or rotate methods explain what the origin
> arguments do, exactly.  And the spec's usage of post-multiply tends to be
> pretty vague...
> 
> >+++ b/content/base/src/DOMMatrix.cpp
> >+using namespace mozilla;
> >+using namespace mozilla::dom;
> 
> Standard DOM code style for new files is to wrap in those namespaces, not to
> pull them in using "using".

OK. I looked at the latest DOMxxx additions such as DOMPoint and DOMQuad and they used 'using'.
Will update.

> 
> >+    retval->mMatrix2D = new gfx::Matrix(mMatrix2D? m * *mMatrix2D : m);
> 
> Needs space before '?'.  And the "* *" bit is really hard to read, though
> I'm not sure I have a better proposal offhand.  There are multiple instances
> of this.
> 
> >+  return mMatrix3D == nullptr;
> 
> return !mMatrix3D is preferred, I think.

done

> 
> >+  if (mMatrix3D != nullptr) {
> 
> Likewise, if (mMatrix3D).

done

> 
> >+DOMMatrixReadOnly::TransformPoint(const DOMPointInit& point) const
> 
> Is the 2d behavior correct?  It's weird to have a 2d transformation like
> this affect x and y in a way that doesn't depend on w.
> 
> In particular, (2, 2, 2, 2) and (1, 1, 1, 1) are conceptually the same 3d
> point, assuming these are homogeneous coordinates, but will be transformed
> to different 3d points if TransformPoint is called with a 2d matrix, right? 
> Why is this the behavior we're implementing?

Yes, this is wrong. Should I check if z and w are different from 1 and then apply it to a 3d matrix?

> 
> >+JSObject*
> >+DOMMatrixReadOnly::ToFloat32Array(JSContext* aCx) const
> 
> This is the wrong function signature, as of June 11 or so.  This needs to
> take a mutable handle instead.

Do you have an example of this?
 
> >+  JS::Rooted<JSObject*> array(aCx, JS_NewFloat32Array(aCx, 16));
> 
> This is wrong, unfortunately (even if we ignore it crashing on out of
> memory, i actually doesn't create objects in the right global in the long
> term).  The right way to do this sort of thing is more like this:
> 
>   nsAutoTArray<float, 16> arr;
>   // Get your values into arr
>   JS::Rooted<JS::Value> value(aCx);
>   if (!ToJSValue(aCx, TypedArrayCreator<Float32Array>(arr), &value)) {
>     // throw here; the function needs to be declared as throwing in the IDL
>   }
>   aRetVal.set(&value.toObject());
> 
> or so.

will do

> 
> Same thing for the Float64 version.
> 
> >+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(DOMMatrix, mParent)
> 
> This needs to be on DOMMatrixReadOnly, no?
> 
> >+template <typename T> void SetDataFromMatrix(DOMMatrix* aMatrix, const T* aData, int aLength, ErrorResult& aRv)
> 
> SetDataInMatrix, perhaps?
> 
> Or SetMatrixFromData?

roc suggested that name but I can change it.

> >+DOMMatrix::Constructor(const GlobalObject& aGlobal, const RootedTypedArray<Float32Array>& aArray32, ErrorResult& aRv)
> 
> const Float32Array& for the second argument.
> 
> >+  SetDataFromMatrix(obj, aArray32.Data(), aArray32.Length(), aRv);
> 
> Was this tested?  It's pretty conspicuously missing a ComputeLengthAndData()
> call, which has been required since before the try runs I see in this bug...
> And I see no obvious test for this codepath in the attached tests.

Yes, there are tests for 1, 6, 15, 16 and 17 arguments.
 
> Also, this codepath will allocate a 2d matrix only to immediately throw it
> away.  It might be a good idea to have a way to construct a DOMMatrix with a
> 3d matrix up front.
> 
> >+DOMMatrix::Constructor(const GlobalObject& aGlobal, const RootedTypedArray<Float64Array>& aArray64, ErrorResult& aRv)
> 
> Again |const Float64Array& aArray64|.  And need a ComputeLengthAndData() in
> the body.
> 
> >+already_AddRefed<DOMMatrix>
> >+DOMMatrix::Constructor(const GlobalObject& aGlobal, const binding_detail::AutoSequence<double>& aNumberSequence, ErrorResult& aRv)
> 
> Which part of binding_detail was unclear?  ;)  The second argument here
> should be of type "const Sequence<double>&".
> 
> I strongly recommend either looking at
> https://developer.mozilla.org/en/Mozilla/WebIDL_bindings or at example
> codegen when deciding what signatures to use for WebIDL implementations,
> instead of looking at the generated binding code.

will do

> >+DOMMatrix::MultiplySelf(const DOMMatrix& aOther)
> 
> This, and PreMultiplySelf, don't obviously match the spec in terms of
> multiplication order.  If this is because what we're storing is actually a
> transpose of what he spec is storing (not that the spec seems to actually
> describe what it's storing at first glance), that deserves a nice comment. 
> Plus making the spec clearer, of course.
> 
> >+DOMMatrix::TranslateSelf(double aTx,
> >+  if (!aTx && !aTy && !aTz) {
> 
> Here I think comparing to 0 would make it clearer what's being tested.
> 
> >+  if (mMatrix3D || aTz) {
> 
> And here.
> 
> >+DOMMatrix::Scale3dSelf(double aScale, double aOriginX,
> >+  ScaleNonUniformSelf(aScale, aScale, 1.0, aOriginX, aOriginY, aOriginZ);
> 
> Why is the third argument not aScale?

good catch. this is wrong.
 
> >+DOMMatrix::ScaleNonUniformSelf(double aScaleX,
> >+  if ((aScaleX == 1.0) && (aScaleY == 1.0) && (aScaleZ == 1.0)) {
> 
> This is overparenthesized.
> 
> >+  if (mMatrix3D || (aScaleZ != 1.0) || aOriginZ) {
> 
> And here.

done

> 
> >+bool
> >+DOMMatrix::InvertSelf()
> 
> This return value doesn't match the spec.

ah, the spec was changed. will update

> 
> >+++ b/content/base/src/DOMMatrix.h
> >+#ifndef MOZILLA_DOMMATRIX_H_
> 
> "mozilla_dom_DOMMatrix_h" is the typical header guard we'd use here.  In
> particular, 
> 
> >+  DOMMatrixReadOnly(const DOMMatrixReadOnly&);
> >+  DOMMatrixReadOnly& operator=(const DOMMatrixReadOnly&);
> 
> MOZ_DELETE on those, please.
> 
> And maybe make the constructor protected if this is not supposed to ever be
> instantiated on its own?
> 
> >+  DOMMatrix(nsISupports* aParent = NULL)
> 
> nullptr, not NULL.  But also, are there any cases that _don't_ pass a parent?

I don't think so. Should I make that constructor private?

> 
> You should consistently use "aRv".  Or at least not randomly mix "aRv" and
> "aRV" as you do in the various constructor signatures.
> 
> >+++ b/dom/webidl/DOMMatrix.webidl
> >\ No newline at end of file
> 
> Should be one.
> 
> Please post an interdiff for the update once you have one..

How do I create that?

	Dirk Schulze
	Comment 42 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #40)

I did the changes to the origin taking transformation functions in the spec. The specification text explicitly states how to translate with the origin and how to undo the translation.

> Note that my question about transformPoint is sort of a spec question too.

Note that DOMMatrix is implemented as 2D in some cases in Rik's patch. This is not part of the specification and an implementation detail. However, I added a note to transformPoint(). The note explicitly says that a 4x4 matrix multiplication must be performed if point.z is not 0 or point.w is not 1. I hope that helps.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 44 • 10 years ago

> roc suggested that name but I can change it.

If I did, I didn't mean to. SetMatrixFromData is better.

	Boris Zbarsky [:bzbarsky]
	Comment 45 • 10 years ago

Comment on attachment 8443156 [details] [diff] [review]
Interdiff with changes following bz's review

>Do you have an example of this?

You want this:

  void
  DOMMatrixReadOnly::ToFloat32Array(JSContext* aCx,
                                    JS::MutableHandle<JSObject*> aResult)

>+  GetDataFromMatrix(this, JS_GetFloat32ArrayData(*aResult));

I don't think this works correctly, actually, since you already created the array with a 0 size!  What you want is:

  nsAutoTArray<float, 16> arr;
  arr.SetLength(16);
  GetDataFromMatrix(this, arr.Elements())

(or change GetDataFromMatrix to take an nsTArray).  Then go ahead and do the ToJSValue thing.  Looks like nothing in the tests checks the array lengths...

Also, if ToJSValue returns false you need to throw an exception to script, like I said in the previous review.  Or rather tell the JS engine that an exception has already been thrown, which it has...  The point is, you need to mark this method [Throws] in the IDL and aRv.Throw(NS_ERROR_OUT_OF_MEMORY) before returning if ToJSValue fails.

> roc suggested that name but I can change it.

I think roc got it confused with GetDataFromMatrix or something.  ;)

> Yes, there are tests for 1, 6, 15, 16 and 17 arguments.

With a typed array constructor argument?  Where?

> I don't think so. Should I make that constructor private?

I wouldn't worry about that.

Please do add tests for constructing from typed arrays if there aren't any yet. and fix the typed array getters...

	Boris Zbarsky [:bzbarsky]
	Comment 47 • 10 years ago

Comment on attachment 8443234 [details] [diff] [review]
Interdiff with changes following bz's review

>+            "DOMMatrix should produce the same matrix with float32array constructor");

"float64array", for the second one, right?

r=me

	Rik Cabanier Assignee
	Comment 49 • 10 years ago

debug try build: https://tbpl.mozilla.org/?tree=Try&rev=91f72e1786b4

	Carsten Book [:Tomcat]
	Comment 50 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa2fa82aa14

	Carsten Book [:Tomcat]
	Comment 51 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/6aa2fa82aa14

	Sylvestre Ledru [:Sylvestre]
	Comment 52 • 10 years ago

Added to the release notes with the wording "DOMMatrix interface implemented". I will add a link to MDN once the doc is available.

	adria
	Comment 53 • 10 years ago

I'm guessing with the name DOMMatrix, this won't be avalible in workers.

	adria
	Comment 54 • 10 years ago

(In reply to adria from comment #53)
> I'm guessing with the name DOMMatrix, this won't be avalible in workers.
Disregard comment. I didn't immediately get that this isn't a general purpose matrix API.

	Rik Cabanier Assignee
	Comment 55 • 10 years ago

(In reply to adria from comment #53)
> I'm guessing with the name DOMMatrix, this won't be avalible in workers.

Yes, DOMMatrix should be available in workers. The "DOM" prefix is just there to avoid clashes with user defined "matrix"

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 56 • 10 years ago

Nothing in this patch makes it available in workers.

	Boris Zbarsky [:bzbarsky]
	Comment 57 • 10 years ago

Nothing in the spec does either.  Per spec right now it's only available in Windows.

You presumably want some Exposed annotations both in the spec and in our IDL, along with an audit to make sure the implementation is OK in workers.

	Dirk Schulze
	Comment 58 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #57)
> Nothing in the spec does either.  Per spec right now it's only available in
> Windows.
> 
> You presumably want some Exposed annotations both in the spec and in our
> IDL, along with an audit to make sure the implementation is OK in workers.

But just if it is declared as [Expose=Window&Worker] ;)

Will add it to the spec.