1065031 - Document and rename Moz2D Matrix's Translate, Scale and Rotate methods

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

As discussed on the gfx weekly call, we should rename Moz2D Matrix's Translate, Scale and Rotate methods to have a "Pre" prefix, and document what they do. This is because:

  myMatrix * Matrix::Translation(x, y)

does not give the same result as:

  myMatrix.Translate(x, y)

as might be expected (the order is in fact the opposite).

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - moz2d code

The non-gfx/2d directory changes coming up after this is reviewed.

Comment 2

[Jonathan Watt [:jwatt]]

Ignore the comment at the start of the Matrix class. I forgot to remove that.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: non-Moz2D code

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8486582 [details] [diff] [review]
part 1 - moz2d code

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

::: gfx/2d/Matrix.h
@@ +129,5 @@
> +  static Matrix Scaling(Float aScaleX, Float aScaleY)
> +  {
> +    return Matrix(aScaleX, 0.0f, 0.0f, aScaleY, 0.0f, 0.0f);
> +  }
> +  

nit: whitespace

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8486830 [details] [diff] [review]
non-Moz2D code

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

Awesome.

::: gfx/layers/basic/BasicCanvasLayer.cpp
@@ +41,5 @@
> +    oldTM = aDT->GetTransform();
> +    Matrix newTM = oldTM;
> +    newTM.PreTranslate(0.0f, mBounds.height);
> +    newTM.PreScale(1.0f, -1.0f);
> +    aDT->SetTransform(newTM);

nit: why not just Matrix(oldTM).PreTranslate().PreScale() like you do elsewhere?

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8486582 [details] [diff] [review]
part 1 - moz2d code

(In reply to Bas Schouten (:bas.schouten) from comment #5)
> nit: why not just Matrix(oldTM).PreTranslate().PreScale() like you do
> elsewhere?

Mostly because that exceeded the 80 column rule. I changed it to that wish some line breaks that I think still make it clear to read.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c06063cfce3
https://hg.mozilla.org/integration/mozilla-inbound/rev/09d86eb69c01

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c06063cfce3
https://hg.mozilla.org/mozilla-central/rev/09d86eb69c01