Implement translate() and translateSelf() in DOMMatrix.

Implement translate() and translateSelf() in DOMMatrix.

The methods multiply a translation transformation on the current matrix.

The specification:
  http://dev.w3.org/fxtf/geometry/

Intent to Implement:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/V_bJNtOg0oM

BUG=388780
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180418

[zino, 2014-08-06 09:23:51]

Please take a look.

Thank you.

[Rik, 2014-08-06 16:30:37]

lgtm

[abarth-chromium, 2014-08-06 20:11:22]

This looks plausible, but I'm not really interested in reviewing these CLs. 
You'll need to find someone else to review them.

[zino, 2014-08-07 01:14:31]

+ @krit

Dear Dirk,

I heard you was interested in implementing geometry interfaces in WebKit.
So, could you review this CL and other things in the future?
Rik has already been doing good quality reviews but he doesn't have a owner
permission in Blink.
If you don't have enough time, I'll try to find someone else.

Thank you.

[krit, 2014-08-08 06:24:15]

Looks great! Just some snippets. I think we should share more code with
TransformationMatrix

https://codereview.chromium.org/446803002/diff/1/LayoutTests/fast/dom/geometr...
File LayoutTests/fast/dom/geometry-interfaces-dom-matrix-translate.html (right):

https://codereview.chromium.org/446803002/diff/1/LayoutTests/fast/dom/geometr...
LayoutTests/fast/dom/geometry-interfaces-dom-matrix-translate.html:5: <script
src="../../resources/js-test.js"></script>
As mentioned in another CL, it would be great if we could use testharness.js to
share the tests with the rest of the world.

https://codereview.chromium.org/446803002/diff/1/Source/core/dom/DOMMatrix.cpp
File Source/core/dom/DOMMatrix.cpp (right):

https://codereview.chromium.org/446803002/diff/1/Source/core/dom/DOMMatrix.cp...
Source/core/dom/DOMMatrix.cpp:63: if (isIdentity()) {
Why don't you use TransformationMatrix here?

[zino, 2014-08-16 03:23:32]

Hi Dirk,

I addressed all your comments.
Could you review again?

Thank you.

https://codereview.chromium.org/446803002/diff/1/LayoutTests/fast/dom/geometr...
File LayoutTests/fast/dom/geometry-interfaces-dom-matrix-translate.html (right):

https://codereview.chromium.org/446803002/diff/1/LayoutTests/fast/dom/geometr...
LayoutTests/fast/dom/geometry-interfaces-dom-matrix-translate.html:5: <script
src="../../resources/js-test.js"></script>
On 2014/08/08 06:24:15, krit wrote:
> As mentioned in another CL, it would be great if we could use testharness.js
to
> share the tests with the rest of the world.

Done.

https://codereview.chromium.org/446803002/diff/1/Source/core/dom/DOMMatrix.cpp
File Source/core/dom/DOMMatrix.cpp (right):

https://codereview.chromium.org/446803002/diff/1/Source/core/dom/DOMMatrix.cp...
Source/core/dom/DOMMatrix.cpp:63: if (isIdentity()) {
On 2014/08/08 06:24:15, krit wrote:
> Why don't you use TransformationMatrix here?

Done.

[krit, 2014-08-16 07:03:06]

https://codereview.chromium.org/446803002/diff/60001/Source/core/dom/DOMMatri...
File Source/core/dom/DOMMatrix.cpp (right):

https://codereview.chromium.org/446803002/diff/60001/Source/core/dom/DOMMatri...
Source/core/dom/DOMMatrix.cpp:40: if (m_matrix.isIdentity()) {
Just use translate3d() and translate() don't duplicate code. The isIdentity
check shouldn't be here.

[zino, 2014-08-16 16:50:42]

Hi Dirk,

Could you take a look again?

https://codereview.chromium.org/446803002/diff/60001/Source/core/dom/DOMMatri...
File Source/core/dom/DOMMatrix.cpp (right):

https://codereview.chromium.org/446803002/diff/60001/Source/core/dom/DOMMatri...
Source/core/dom/DOMMatrix.cpp:40: if (m_matrix.isIdentity()) {
On 2014/08/16 07:03:06, krit wrote:
> Just use translate3d() and translate() don't duplicate code. The isIdentity
> check shouldn't be here.

Okay, I agree with you. I'll remove the codes.

https://codereview.chromium.org/446803002/diff/60001/Source/core/dom/DOMMatri...
Source/core/dom/DOMMatrix.cpp:48: m_matrix.setM41(m_matrix.m41() + tx *
m_matrix.m11() + ty * m_matrix.m21());
However, I think it is better to leave this codes than using translate().
We don't need calculate m33, m34 because DOMMatrix has the |m_is2D|.
(When m_is2D is true)

[krit, 2014-08-16 18:07:57]

On 2014/08/16 16:50:42, zino wrote:
> Hi Dirk,
> 
> Could you take a look again?
> 
>
https://codereview.chromium.org/446803002/diff/60001/Source/core/dom/DOMMatri...
> File Source/core/dom/DOMMatrix.cpp (right):
> 
>
https://codereview.chromium.org/446803002/diff/60001/Source/core/dom/DOMMatri...
> Source/core/dom/DOMMatrix.cpp:40: if (m_matrix.isIdentity()) {
> On 2014/08/16 07:03:06, krit wrote:
> > Just use translate3d() and translate() don't duplicate code. The isIdentity
> > check shouldn't be here.
> 
> Okay, I agree with you. I'll remove the codes.
> 
>
https://codereview.chromium.org/446803002/diff/60001/Source/core/dom/DOMMatri...
> Source/core/dom/DOMMatrix.cpp:48: m_matrix.setM41(m_matrix.m41() + tx *
> m_matrix.m11() + ty * m_matrix.m21());
> However, I think it is better to leave this codes than using translate().
> We don't need calculate m33, m34 because DOMMatrix has the |m_is2D|.
> (When m_is2D is true)

IMO code optimizations should go to TransformationMatrix. In general,
multiplication with 0 is not going to be a performance killer. It is more the
question if it could make a previously 2D matrix a 3D matrix. I don't think that
this can happen if you pass 0.

[zino, 2014-08-17 00:12:17]

On 2014/08/16 18:07:57, krit wrote:
> IMO code optimizations should go to TransformationMatrix. In general,
> multiplication with 0 is not going to be a performance killer.

I got it! Thank you for the explanation.
I'm just using transform().
Could you take a look?

Thank you :)

[krit, 2014-08-17 08:40:59]

lgtm

[zino, 2014-08-17 08:56:51]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-08-17 08:57:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/446803002/100001

[commit-bot: I haz the power, 2014-08-17 09:52:07]

Committed patchset #5 (100001) as 180418