Issue 2283363003: GeometryInterface: Add DOMMatrixInit and fromMatrix().

Issue 2283363003: GeometryInterface: Add DOMMatrixInit and fromMatrix(). (Closed)

Created:
4 years, 3 months ago by zino

Modified:
4 years, 3 months ago

Reviewers:
haraken, dominicc (has gone to gerrit)

CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, pdr., rwlbuis, sof

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

GeometryInterface: Add DOMMatrixInit and fromMatrix(). Add DOMMatrixInit[1] and fromMatrix()[2]. This CL makes multiply methods take DOMMatrixInit instead of DOMMatrix(ReadOnly) but it doesn't change the existing behavior. [1] https://drafts.fxtf.org/geometry/#dictdef-dommatrixinit [2] https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-frommatrix BUG=388780 Committed: https://crrev.com/37abfa53a3700b5935136cb3c34a5a29e0431e72 Cr-Commit-Position: refs/heads/master@{#418552}

Patch Set 4 : addressed comments #

Total comments: 4

Patch Set 6 : rebase #

Total comments: 1

Created: 4 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+620 lines, -73 lines)			Patch
M	third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix.html	View	1 2 3 4 5	9 chunks	+106 lines, -25 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html	View	1 2 3 4 5	7 chunks	+113 lines, -29 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/dom/resources/geometry-interfaces-test-helpers.js	View	1 2 3	1 chunk	+62 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/bindings/core/v8/BUILD.gn	View	1 2 3 4 5	1 chunk	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/BUILD.gn	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/core_idl_files.gni	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/DOMMatrix.h	View	1 2 3 4 5	3 chunks	+5 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/DOMMatrix.cpp	View	1 2 3 4 5	1 chunk	+27 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/DOMMatrix.idl	View	1 2 3 4 5	2 chunks	+8 lines, -3 lines	0 comments	Download
A	third_party/WebKit/Source/core/dom/DOMMatrixInit.idl	View		1 chunk	+31 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.h	View	1 2 3 4 5	3 chunks	+5 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp	View	1 2 3 4 5	3 chunks	+97 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.idl	View	1 2 3 4 5	3 chunks	+7 lines, -2 lines	0 comments	Download
A	third_party/WebKit/Source/core/dom/DOMMatrixTest.cpp	View	1 2 3 4 5	1 chunk	+154 lines, -0 lines	1 comment	Download

Messages

Total messages: 34 (19 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dominicc (has gone to gerrit)

This looks like very neat work. Thank you for working on this. I notice this ...

4 years, 3 months ago (2016-08-30 02:13:43 UTC) #8

zino

Thank you for review. I addressed your comments. Could you review this again? https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html File ...

4 years, 3 months ago (2016-09-10 10:18:15 UTC) #11

Thank you for review.

I addressed your comments.

Could you review this again?

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html
(right):

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html:132:
assert_throws(new TypeError(), function() {
On 2016/08/30 02:13:42, dominicc wrote:
> Consider using arrow functions. They're slightly more succinct and readable.
> 
> http://es6-features.org/#StatementBodies

Done.

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html:133:
var matrix = DOMMatrixReadOnly.fromMatrix({a: 1, b: 2, m33: 3, m44: 4, is2D:
true});
On 2016/08/30 02:13:43, dominicc wrote:
> Why do you need this variable?

Done.

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix.html
(right):

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix.html:113:
assert_array_equals(matrix.toFloat64Array(), [1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1,
0, 0, 0, 0, 1]);
On 2016/08/30 02:13:43, dominicc (traveling) wrote:
> This assert_array_equals/assert_true/assert_true is very repetitive. What do
you
> think about writing an assert_matrix_equals helper function?

Done.

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix.html:187:

On 2016/08/30 02:13:43, dominicc (traveling) wrote:
> I think it would be worth adding NaN tests, given the WebIDL handling. You
could
> easily imagine an implementation using IEEE-754 and getting that part wrong;
> IIRC IEEE-754 says NaN != NaN. (Do we implement that correctly, actually?)

Done.

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMMatrix.cpp (right):

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMMatrix.cpp:59: if (!otherMatrix->is2D())
On 2016/08/30 02:13:43, dominicc (traveling) wrote:
> Seems a shame to allocate the whole intermediate DOMMatrix, but then the spec
> warns authors about that, so I guess this way is at least clean and simple.

I'll try to file a bug.

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:53: void
DOMMatrixReadOnly::validateAndFixup(DOMMatrixInit& other, ExceptionState&
exceptionState)
On 2016/08/30 02:13:43, dominicc (traveling) wrote:
> Please add tests for NaN. The note about NaN bit patterns at
> https://drafts.fxtf.org/geometry/#validate-and-fixup is interesting.

Done.

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:61:
exceptionState.throwTypeError("[a, b, c, d, e, f] members should equals [m11,
m12, m21, m22, m41, m42].");
On 2016/08/30 02:13:43, dominicc (traveling) wrote:
> Grammar: should equals -> should equal
> 
> It's a bit awkward to refer to [a, b, ...] *members* and then talk about [m11,
> m12, ...] without mentioning members. Is member some jargon for matrices? Be
> consistent.
> 
> I'm also not sure about 'member' at all. JavaScript doesn't have 'members' but
> properties.

Done.

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:77:
other.setIs2D(false);
On 2016/08/30 02:13:43, dominicc wrote:
> Why not save that expression in a variable with a good, descriptive name, and
> then do one line:
> 
> other.setIs2D(niceNameYouFound);
> 
> It's simpler and clearer.

Done.

https://codereview.chromium.org/2283363003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DOMMatrixReadOnly.cpp:94:
validateAndFixup(other, exceptionState);
On 2016/08/30 02:13:43, dominicc wrote:
> The common pattern seems to be
> 
> validateAndFixUp
> check whether there was an exception
>   return early.
> 
> What do you think about having validateAndFixUp return a boolean and simplify
> that to two lines?

Done.

haraken

bindings/ looks good. https://codereview.chromium.org/2283363003/diff/160001/third_party/WebKit/Source/core/dom/DOMMatrixInit.idl File third_party/WebKit/Source/core/dom/DOMMatrixInit.idl (right): https://codereview.chromium.org/2283363003/diff/160001/third_party/WebKit/Source/core/dom/DOMMatrixInit.idl#newcode7 third_party/WebKit/Source/core/dom/DOMMatrixInit.idl:7: dictionary DOMMatrixInit { Don't we want ...

4 years, 3 months ago (2016-09-12 00:04:33 UTC) #14

dominicc (has gone to gerrit)

lgtm https://codereview.chromium.org/2283363003/diff/160001/third_party/WebKit/Source/core/dom/DOMMatrix.idl File third_party/WebKit/Source/core/dom/DOMMatrix.idl (right): https://codereview.chromium.org/2283363003/diff/160001/third_party/WebKit/Source/core/dom/DOMMatrix.idl#newcode41 third_party/WebKit/Source/core/dom/DOMMatrix.idl:41: // FIXME: Should implement some methods (See: crbug.com/388780) ...

4 years, 3 months ago (2016-09-12 05:19:43 UTC) #15

haraken

On 2016/09/12 05:19:43, dominicc wrote: > lgtm > > https://codereview.chromium.org/2283363003/diff/160001/third_party/WebKit/Source/core/dom/DOMMatrix.idl > File third_party/WebKit/Source/core/dom/DOMMatrix.idl (right): > ...

4 years, 3 months ago (2016-09-12 05:41:02 UTC) #16

zino

https://codereview.chromium.org/2283363003/diff/160001/third_party/WebKit/Source/core/dom/DOMMatrix.idl File third_party/WebKit/Source/core/dom/DOMMatrix.idl (right): https://codereview.chromium.org/2283363003/diff/160001/third_party/WebKit/Source/core/dom/DOMMatrix.idl#newcode41 third_party/WebKit/Source/core/dom/DOMMatrix.idl:41: // FIXME: Should implement some methods (See: crbug.com/388780) On ...

4 years, 3 months ago (2016-09-12 09:12:43 UTC) #18

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-12 09:13:02 UTC) #21

commit-bot: I haz the power

Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/127067)

4 years, 3 months ago (2016-09-12 10:09:30 UTC) #23

dominicc (has gone to gerrit)

On 2016/09/12 at 10:09:30, commit-bot wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe ...

4 years, 3 months ago (2016-09-13 08:29:40 UTC) #24

zino

https://codereview.chromium.org/2283363003/diff/260001/third_party/WebKit/Source/core/dom/DOMMatrixTest.cpp File third_party/WebKit/Source/core/dom/DOMMatrixTest.cpp (right): https://codereview.chromium.org/2283363003/diff/260001/third_party/WebKit/Source/core/dom/DOMMatrixTest.cpp#newcode18 third_party/WebKit/Source/core/dom/DOMMatrixTest.cpp:18: EXPECT_FALSE(init.hasA()); I fixed the test. (https://github.com/google/googletest/issues/322)

4 years, 3 months ago (2016-09-14 12:39:43 UTC) #27

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-14 12:40:05 UTC) #30

commit-bot: I haz the power

4 years, 3 months ago (2016-09-14 14:18:16 UTC) #34

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/37abfa53a3700b5935136cb3c34a5a29e0431e72
Cr-Commit-Position: refs/heads/master@{#418552}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages