Closed Bug 1065031 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(2 files)

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).
The non-gfx/2d directory changes coming up after this is reviewed.
Attachment #8486582 - Flags: review?(bas)
Ignore the comment at the start of the Matrix class. I forgot to remove that.
Depends on: 1065127
Attached patch non-Moz2D codeSplinter Review
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
Attachment #8486582 - Flags: review?(bas) → review+
Attachment #8486830 - Flags: review?(bas)
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?
Attachment #8486830 - Flags: review?(bas) → review+
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
Attachment #8486582 - Flags: checkin+
Attachment #8486830 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/3c06063cfce3
https://hg.mozilla.org/mozilla-central/rev/09d86eb69c01
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.