Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2423753002:
[GeometryInterface] add transformPoint(point) function. (Closed)
|
Created:
4 years, 2 months ago by Hwanseung Lee Modified:
4 years ago CC:
blink-bindings-reviews_chromium.org, blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, Hwanseung Lee(hs1217.lee), rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[GeometryInterface] add transformPoint(point) function.
Let point object be the result of invoking create
a DOMPoint from the dictionary point.
Point object is post-multiplied to the current matrix
and returns the resulting point.
The passed argument does not get modified.
spec list:
https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-transformpoint
BUG=388780
Committed: https://crrev.com/0954207674a2b1b1bc703553d3d5aa2c4036bf5c
Cr-Commit-Position: refs/heads/master@{#432762}
Patch Set 1 #Patch Set 2 : add test. #Patch Set 3 : [GeometryInterface] add transformPoint(point) method. #
Total comments: 4
Patch Set 4 : [GeometryInterface] add transformPoint(point) method. #
Total comments: 3
Patch Set 5 : [GeometryInterface] add transformPoint(point) method. #Patch Set 6 : [GeometryInterface] add transformPoint(point) method. #Patch Set 7 : [GeometryInterface] add transformPoint(point) method. #
Total comments: 4
Patch Set 8 : update test msg. #
Messages
Total messages: 47 (26 generated)
Description was changed from ========== [GeometryInterface] add transformPoint(point) method. Let point object be the result of invoking create a DOMPoint from the dictionary point. Point object is post-multiplied to the current matrix and returns the resulting point. The passed argument does not get modified. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-transformpoint BUG=388780 ========== to ========== [GeometryInterface] add transformPoint(point) function. Let point object be the result of invoking create a DOMPoint from the dictionary point. Point object is post-multiplied to the current matrix and returns the resulting point. The passed argument does not get modified. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-transformpoint BUG=388780 ==========
hs1217.lee@samsung.com changed reviewers: + dominicc@chromium.org, jinho.bang@samsung.com
@dominicc, zino. PTAL, thank you.
Thank you for this work. I left some comments. https://codereview.chromium.org/2423753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp (right): https://codereview.chromium.org/2423753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:215: DOMPoint* DOMMatrixReadOnly::transformPoint(DOMPointInit& point) { The spec says that "Even if is2D of the current matrix returns true, a 4x4 matrix multiplication will be performed if the z attribute of point is not 0 or the w attribute of point is not 1." In other words, we can optimize this as follows: if (is2D() && point.z() == 0 && point.w() == 1) { double x = point.x * m11 + point.y * m12 + m41; double y = point.x * m12 + point.y * m22 + m42; return DOMPoint::create(x, y, 0, 1); } Also, please add some tests for this. https://codereview.chromium.org/2423753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.h (right): https://codereview.chromium.org/2423753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.h:81: DOMPoint* transformPoint(DOMPointInit&); nit: const DOMPointInit&
@zino i did fix it. Thank you. https://codereview.chromium.org/2423753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp (right): https://codereview.chromium.org/2423753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:215: DOMPoint* DOMMatrixReadOnly::transformPoint(DOMPointInit& point) { On 2016/10/17 15:44:44, zino wrote: > The spec says that "Even if is2D of the current matrix returns true, a 4x4 > matrix multiplication will be performed if the z attribute of point is not 0 or > the w attribute of point is not 1." > > In other words, we can optimize this as follows: > > if (is2D() && point.z() == 0 && point.w() == 1) { > double x = point.x * m11 + point.y * m12 + m41; > double y = point.x * m12 + point.y * m22 + m42; > return DOMPoint::create(x, y, 0, 1); > } > > Also, please add some tests for this. Done. https://codereview.chromium.org/2423753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.h (right): https://codereview.chromium.org/2423753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.h:81: DOMPoint* transformPoint(DOMPointInit&); On 2016/10/17 15:44:44, zino wrote: > nit: const DOMPointInit& Done.
@dominicc PTAL. thank you.
dominicc@chromium.org changed reviewers: + meade@chromium.org
https://codereview.chromium.org/2423753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-transformPoint.html (right): https://codereview.chromium.org/2423753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-transformPoint.html:7: function transformPoint(matrix, point) { I think this test strategy of having two implementations written by the same person and testing one against the other is a bit weak. I'd be more comfortable of having transformed cases written inline, or both.
I agree with Dom's comment, otherwise this seems fine.
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hs1217.lee@samsung.com
The CQ bit was checked by hs1217.lee@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@dominicc PTAL. thank you. https://codereview.chromium.org/2423753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-transformPoint.html (right): https://codereview.chromium.org/2423753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-transformPoint.html:7: function transformPoint(matrix, point) { On 2016/10/28 02:25:19, dominicc wrote: > I think this test strategy of having two implementations written by the same > person and testing one against the other is a bit weak. I'd be more comfortable > of having transformed cases written inline, or both. i just removed if statement. it is same with below logic. actually it is not necessary.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/29 at 12:11:08, hs1217.lee wrote: > @dominicc > PTAL. thank you. > > https://codereview.chromium.org/2423753002/diff/60001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-transformPoint.html (right): > > https://codereview.chromium.org/2423753002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-transformPoint.html:7: function transformPoint(matrix, point) { > On 2016/10/28 02:25:19, dominicc wrote: > > I think this test strategy of having two implementations written by the same > > person and testing one against the other is a bit weak. I'd be more comfortable > > of having transformed cases written inline, or both. > > i just removed if statement. it is same with below logic. actually it is not necessary. Could you add some of those basic hand-coded tests, things like rotating a point at (42,0,0) around the x-axis doesn't change it; around the z axis rotates 42 into y; some basic translations; etc.? The problem with having these two similar implementations is they both might be wrong in the same way, but we can't easily check it from the tests; and if a test does fail, it doesn't help us understand what's broken whereas knowing rotations is z are broken but other rotations, translations aren't would narrow things down quickly.
@dominicc PTAL, thank you. https://codereview.chromium.org/2423753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-transformPoint.html (right): https://codereview.chromium.org/2423753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-transformPoint.html:7: function transformPoint(matrix, point) { On 2016/10/29 12:11:08, Hwanseung Lee wrote: > On 2016/10/28 02:25:19, dominicc wrote: > > I think this test strategy of having two implementations written by the same > > person and testing one against the other is a bit weak. I'd be more > comfortable > > of having transformed cases written inline, or both. > > i just removed if statement. it is same with below logic. actually it is not > necessary. sorry. i had been misunderstood.
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@diminicc PTAL, thank you.
Superficially it's all in order, but I'm curious whether it is expensive to pass DOMPoint[ReadOnly] to something which takes DOMPointInit. https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/resources/geometry-interfaces-test-helpers.js (right): https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/resources/geometry-interfaces-test-helpers.js:68: assert_equals(actual.x, expected.x, "actual.x is different with expected.x"); 'different to' Maybe it is sufficient to just say 'x', 'y' or 'z'; or something like 'point equality: x differs' or something? https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp (right): https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:231: DOMPoint* DOMMatrixReadOnly::transformPoint(const DOMPointInit& point) { I'm curious but what's the efficiency in our bindings of passing DOMPoint to a method which takes DOMPointInit? Is it possible we want to make this fast(er) by possibly special-casing DOMPoint, DOMPointReadOnly?
PTAL, thank you. https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/resources/geometry-interfaces-test-helpers.js (right): https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/resources/geometry-interfaces-test-helpers.js:68: assert_equals(actual.x, expected.x, "actual.x is different with expected.x"); On 2016/11/08 08:55:34, dominicc wrote: > 'different to' > > Maybe it is sufficient to just say 'x', 'y' or 'z'; or something like 'point > equality: x differs' or something? Done. https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp (right): https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:231: DOMPoint* DOMMatrixReadOnly::transformPoint(const DOMPointInit& point) { On 2016/11/08 08:55:34, dominicc wrote: > I'm curious but what's the efficiency in our bindings of passing DOMPoint to a > method which takes DOMPointInit? Is it possible we want to make this fast(er) by > possibly special-casing DOMPoint, DOMPointReadOnly? i think it is almost same. DOMPointInit have all attribute of DOMPoint or DOMPointReadOnly. when DOMPointInit binding to cpp from javascript object, all attribute was binding. this processing is similar at DOMPoint or DOMPointReadOnly. actually i think DOMPointInit is more simpler than DOMPoint or DOMPointReadOnly in oder to binding. because DOMPointInit is dictionary type of IDL. how do you think? is it right my thought? refer to below link. DOMPointInit https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8D... DOMPoint https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8D... DOMPointReadOnly https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8D...
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/13 02:47:10, Hwanseung Lee wrote: > PTAL, thank you. > > https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/dom/resources/geometry-interfaces-test-helpers.js > (right): > > https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/dom/resources/geometry-interfaces-test-helpers.js:68: > assert_equals(actual.x, expected.x, "actual.x is different with expected.x"); > On 2016/11/08 08:55:34, dominicc wrote: > > 'different to' > > > > Maybe it is sufficient to just say 'x', 'y' or 'z'; or something like 'point > > equality: x differs' or something? > > Done. > > https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp (right): > > https://codereview.chromium.org/2423753002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:231: DOMPoint* > DOMMatrixReadOnly::transformPoint(const DOMPointInit& point) { > On 2016/11/08 08:55:34, dominicc wrote: > > I'm curious but what's the efficiency in our bindings of passing DOMPoint to a > > method which takes DOMPointInit? Is it possible we want to make this fast(er) > by > > possibly special-casing DOMPoint, DOMPointReadOnly? > > i think it is almost same. > DOMPointInit have all attribute of DOMPoint or DOMPointReadOnly. > when DOMPointInit binding to cpp from javascript object, all attribute was > binding. > this processing is similar at DOMPoint or DOMPointReadOnly. > actually i think DOMPointInit is more simpler than DOMPoint or DOMPointReadOnly > in oder to binding. > because DOMPointInit is dictionary type of IDL. > how do you think? is it right my thought? > > refer to below link. > DOMPointInit > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8D... > DOMPoint > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8D... > DOMPointReadOnly > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8D... additionally, if DOMPointInit were cast to DOMPoint or DOMPointReadOnly in transformPoint function, your opinion is right, but we only read attribute of DOMPointInit without casting. so i think it is not need to change.
dominicc@chromium.org changed reviewers: + bashi@chromium.org
Bashi-san, could you give us your opinion about how this interface is set up? The spec says to take a dictionary, which the point/point readonly objects conform to. But I think this will cause a lot of copying when called with a point--out of the point into the dictionary, etc. How expensive is that? Would it be web-observable to have overloads which handle points directly? How will this API be used? To transform lots of points in a model? Transform the mouse cursor?
On 2016/11/15 00:12:31, dominicc wrote: > Bashi-san, could you give us your opinion about how this interface is set up? > > The spec says to take a dictionary, which the point/point readonly objects > conform to. But I think this will cause a lot of copying when called with a > point--out of the point into the dictionary, etc. How expensive is that? > > Would it be web-observable to have overloads which handle points directly? > > How will this API be used? To transform lots of points in a model? Transform the > mouse cursor? (Please let me know if I misunderstand your question) Our binding layer allocates dictionary objects (DOMPointInit) on stack so it's basically fast enough. Copies will happen when we pass around dictionaries but it seems that the purpose of transformPoint() is to create a DOMPoint object from a dictionary and as far as I can see this CL doesn't create a copy of dictionary. I'm not sure how transformPoint() is used, but if we end up creating a DOMPoint object at some point, it looks reasonable to me.
lgtm Thanks Bashi for your input. So it sounds like this could be faster when the argument is a DOMPoint by copying from it directly, but at least the dictionary is a stack object etc. and so the complexity may not be worth it.
The CQ bit was checked by hs1217.lee@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [GeometryInterface] add transformPoint(point) function. Let point object be the result of invoking create a DOMPoint from the dictionary point. Point object is post-multiplied to the current matrix and returns the resulting point. The passed argument does not get modified. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-transformpoint BUG=388780 ========== to ========== [GeometryInterface] add transformPoint(point) function. Let point object be the result of invoking create a DOMPoint from the dictionary point. Point object is post-multiplied to the current matrix and returns the resulting point. The passed argument does not get modified. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-transformpoint BUG=388780 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [GeometryInterface] add transformPoint(point) function. Let point object be the result of invoking create a DOMPoint from the dictionary point. Point object is post-multiplied to the current matrix and returns the resulting point. The passed argument does not get modified. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-transformpoint BUG=388780 ========== to ========== [GeometryInterface] add transformPoint(point) function. Let point object be the result of invoking create a DOMPoint from the dictionary point. Point object is post-multiplied to the current matrix and returns the resulting point. The passed argument does not get modified. spec list: https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-transformpoint BUG=388780 Committed: https://crrev.com/0954207674a2b1b1bc703553d3d5aa2c4036bf5c Cr-Commit-Position: refs/heads/master@{#432762} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0954207674a2b1b1bc703553d3d5aa2c4036bf5c Cr-Commit-Position: refs/heads/master@{#432762} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
