Adding matrixTrasnform to DOMPointReadOnly interfaces as following spec.

Adding matrixTrasnform to DOMPointReadOnly interfaces as following spec.

Adding matrixTransform to DOMPointReadOnly interfaces as following spec.
The interfaces require matrixTransform function as following spec[1].

[1] https://drafts.fxtf.org/geometry/#dom-dompointreadonly-matrixtransform

BUG=675437
Committed: https://crrev.com/bf96f8354f45b92870f2d80a194b59d97e102fe2
Cr-Commit-Position: refs/heads/master@{#441193}

[zino, 2016-12-28 16:39:14]

jinho.bang@samsung.com changed reviewers:
+ jinho.bang@samsung.com

[zino, 2016-12-28 16:39:14]

I left some comments.

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-readonly.html
(right):

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-readonly.html:47:
var point1 = new DOMPointReadOnly(5, 4, 0, 1);
var point = new DOMPointReadOnly(5, 4);

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-readonly.html:48:
var point2 = point1.matrixTransform(new DOMMatrixReadOnly([2, 0, 0, 2, 10,
10]));
nit: Better name is transformed_point.

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-readonly.html:49:
assert_point_equals(point2, new DOMPoint(20, 18));
You used |assert_dom_point_equals| in previous CL. The function takes array as
expected argument, right?
But you are using |assert_point_equals| in this CL. The function takes object
instead of array as expected argument.
It's so confusing to me. I think you can merge two functions. Could you follow
it up? (and use it?)

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp (right):

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp:39: DOMPoint* point =
DOMPoint::create(x(), y(), z(), w());
This is unnecessary. You don't need to create this object twice.
Instead you can use just x(), y(), z(), w() and then return final result.

(Actually, we can use DOMMatrixReadOnly::transformPoint but it's not a good way
for performance.)

[Byoungkwon Ko, 2017-01-03 15:54:36]

gogag2@gmail.com changed reviewers:
+ haraken@chromium.org

[Byoungkwon Ko, 2017-01-03 15:54:37]

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-readonly.html
(right):

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-readonly.html:47:
var point1 = new DOMPointReadOnly(5, 4, 0, 1);
On 2016/12/28 16:39:14, zino wrote:
> var point = new DOMPointReadOnly(5, 4);

Done.

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-readonly.html:48:
var point2 = point1.matrixTransform(new DOMMatrixReadOnly([2, 0, 0, 2, 10,
10]));
On 2016/12/28 16:39:14, zino wrote:
> nit: Better name is transformed_point.

Done.

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-readonly.html:49:
assert_point_equals(point2, new DOMPoint(20, 18));
On 2016/12/28 16:39:14, zino wrote:
> You used |assert_dom_point_equals| in previous CL. The function takes array as
> expected argument, right?
> But you are using |assert_point_equals| in this CL. The function takes object
> instead of array as expected argument.
> It's so confusing to me. I think you can merge two functions. Could you follow
> it up? (and use it?)

Done.

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp (right):

https://codereview.chromium.org/2600663002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp:39: DOMPoint* point =
DOMPoint::create(x(), y(), z(), w());
On 2016/12/28 16:39:14, zino wrote:
> This is unnecessary. You don't need to create this object twice.
> Instead you can use just x(), y(), z(), w() and then return final result.
> 
> (Actually, we can use DOMMatrixReadOnly::transformPoint but it's not a good
way
> for performance.)

Done.

[zino, 2017-01-03 16:16:55]

https://codereview.chromium.org/2600663002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp (right):

https://codereview.chromium.org/2600663002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp:38: DOMPoint*
DOMPointReadOnly::matrixTransform(DOMMatrixInit& other, ExceptionState&
exceptionState) {
nit: This line(and other lines) probably should be < 80 bytes.
As I know, the Blink coding style has been changed to Chromium style since last
Oct.

You can easily do it if using this command:
 git cl format third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp

https://codereview.chromium.org/2600663002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl (right):

https://codereview.chromium.org/2600663002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl:8: Constructor(optional
unrestricted double x = 0, optional unrestricted double y = 0,
Please add some tests for this change?

[Byoungkwon Ko, 2017-01-03 16:35:39]

PTAL

https://codereview.chromium.org/2600663002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp (right):

https://codereview.chromium.org/2600663002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp:38: DOMPoint*
DOMPointReadOnly::matrixTransform(DOMMatrixInit& other, ExceptionState&
exceptionState) {
On 2017/01/03 16:16:55, zino wrote:
> nit: This line(and other lines) probably should be < 80 bytes.
> As I know, the Blink coding style has been changed to Chromium style since
last
> Oct.
> 
> You can easily do it if using this command:
>  git cl format third_party/WebKit/Source/core/dom/DOMPointReadOnly.cpp

Done.

https://codereview.chromium.org/2600663002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl (right):

https://codereview.chromium.org/2600663002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl:8: Constructor(optional
unrestricted double x = 0, optional unrestricted double y = 0,
On 2017/01/03 16:16:55, zino wrote:
> Please add some tests for this change?

Done.

[zino, 2017-01-03 16:47:59]

lgtm with some nits. Please fix them.

Also, you should wait for haraken@'s review.

Thank you.

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl (left):

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl:5: //
https://dev.w3.org/fxtf/geometry/#DOMPoint
nit: https://drafts.fxtf.org/geometry/#dompointreadonly

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl:20: // FIXME: Implement
matrixTransform.
nit: You should remove these comments.

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl (right):

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl:23: [RaisesException]
DOMPoint matrixTransform(optional DOMMatrixInit matrix);
If possible, it's better to keep the same order as defined in the spec.

The spec defines as follows:

....
DOMPoint matrixTransform(optional DOMMatrixInit matrix);

serializer = { attribute };
....

[haraken, 2017-01-03 16:57:19]

LGTM with zino's comments addressed.

[Byoungkwon Ko, 2017-01-03 17:12:37]

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl (left):

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl:5: //
https://dev.w3.org/fxtf/geometry/#DOMPoint
On 2017/01/03 16:47:59, zino wrote:
> nit: https://drafts.fxtf.org/geometry/#dompointreadonly

Done.

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl:20: // FIXME: Implement
matrixTransform.
On 2017/01/03 16:47:59, zino wrote:
> nit: You should remove these comments.

Done.

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl (right):

https://codereview.chromium.org/2600663002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMPointReadOnly.idl:23: [RaisesException]
DOMPoint matrixTransform(optional DOMMatrixInit matrix);
On 2017/01/03 16:47:59, zino wrote:
> If possible, it's better to keep the same order as defined in the spec.
> 
> The spec defines as follows:
> 
> ....
> DOMPoint matrixTransform(optional DOMMatrixInit matrix);
> 
> serializer = { attribute };
> ....

Done.

[Byoungkwon Ko, 2017-01-03 18:25:31]

The CQ bit was checked by gogag2@gmail.com

[Byoungkwon Ko, 2017-01-03 18:25:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jinho.bang@samsung.com
Link to the patchset: https://codereview.chromium.org/2600663002/#ps280001
(title: "Adding matrixTrasnform to DOMPointReadOnly interfaces as following
spec.")

[commit-bot: I haz the power, 2017-01-03 18:25:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 19:59:16]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1483467931208630,
"parent_rev": "b9de6503acc1b8c379f198cd0474cdf7ec8806e7", "commit_rev":
"2ed0338c86f418f4d7f72df3742d90eb9c1fc999"}

[commit-bot: I haz the power, 2017-01-03 19:59:50]

Description was changed from

==========
Adding matrixTrasnform to DOMPointReadOnly interfaces as following spec.

Adding matrixTransform to DOMPointReadOnly interfaces as following spec.
The interfaces require matrixTransform function as following spec[1].

[1] https://drafts.fxtf.org/geometry/#dom-dompointreadonly-matrixtransform

BUG=675437
==========

to

==========
Adding matrixTrasnform to DOMPointReadOnly interfaces as following spec.

Adding matrixTransform to DOMPointReadOnly interfaces as following spec.
The interfaces require matrixTransform function as following spec[1].

[1] https://drafts.fxtf.org/geometry/#dom-dompointreadonly-matrixtransform

BUG=675437

Review-Url: https://codereview.chromium.org/2600663002
==========

[commit-bot: I haz the power, 2017-01-03 19:59:51]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2017-01-03 20:02:22]

Description was changed from

==========
Adding matrixTrasnform to DOMPointReadOnly interfaces as following spec.

Adding matrixTransform to DOMPointReadOnly interfaces as following spec.
The interfaces require matrixTransform function as following spec[1].

[1] https://drafts.fxtf.org/geometry/#dom-dompointreadonly-matrixtransform

BUG=675437

Review-Url: https://codereview.chromium.org/2600663002
==========

to

==========
Adding matrixTrasnform to DOMPointReadOnly interfaces as following spec.

Adding matrixTransform to DOMPointReadOnly interfaces as following spec.
The interfaces require matrixTransform function as following spec[1].

[1] https://drafts.fxtf.org/geometry/#dom-dompointreadonly-matrixtransform

BUG=675437

Committed: https://crrev.com/bf96f8354f45b92870f2d80a194b59d97e102fe2
Cr-Commit-Position: refs/heads/master@{#441193}
==========

[commit-bot: I haz the power, 2017-01-03 20:02:23]

Patchset 15 (id:??) landed as
https://crrev.com/bf96f8354f45b92870f2d80a194b59d97e102fe2
Cr-Commit-Position: refs/heads/master@{#441193}