Use union instead of overload fo DOMMatrix/DOMMatrixReadOnly

Use union instead of overload fo DOMMatrix/DOMMatrixReadOnly

Fixes DOMMatrix-001 tests that were just broken as written (missing brackets)

BUG=716169
Review-Url: https://codereview.chromium.org/2859293002
Cr-Commit-Position: refs/heads/master@{#470827}
Committed: https://chromium.googlesource.com/chromium/src/+/6156f7b21c2555d4ce52cecff7f5851ebb57652e

[fserb, 2017-05-04 20:49:43]

fserb@chromium.org changed reviewers:
+ junov@chromium.org

[fserb, 2017-05-04 20:49:43]

Baby genius of IDL! \o/

[Justin Novosad, 2017-05-04 21:03:58]

lgtm with Nit

https://codereview.chromium.org/2859293002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2859293002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:108: if
(init.isUnrestrictedDoubleSequence()) {
Could this just be a DCHECK insted of an if?  Then we would not need the
NOTREACHED and extra return at the end.

[Justin Novosad, 2017-05-04 21:09:07]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2017-05-04 21:09:48]

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-05-04 21:18:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 21:18:22]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[fserb, 2017-05-04 21:30:13]

haraken: third_party/WebKit/Source/bindings/core/v8/BUILD.gn
thanks :)

[fserb, 2017-05-04 21:30:54]

fserb@chromium.org changed reviewers:
+ haraken@chromium.org

[fserb, 2017-05-04 21:30:54]

haraken: third_party/WebKit/Source/bindings/core/v8/BUILD.gn
thanks a lot! :)

[haraken, 2017-05-05 00:05:45]

LGTM

[fserb, 2017-05-05 03:05:58]

The CQ bit was checked by fserb@chromium.org

[commit-bot: I haz the power, 2017-05-05 03:06:15]

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-05-05 05:05:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 05:05:58]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fserb, 2017-05-05 15:35:20]

Junov, PTAL for web platform tests, please.

[Justin Novosad, 2017-05-08 14:50:39]

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html
(left):

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:81:
var string = 'scale(2 2) translateX(5) translateY(5)';
It seems the old version of the test was quite deliberate about testing scale()
with and without the comma.  I think it is possible that the comma-less notation
is correct (though not supported by blink).  Members of the CSS WG have been
working on updating the notation, but I am not sure what is the current status
of that work.  See: https://wiki.csswg.org/ideas/functional-notation#transforms

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:90:
assert_throws('SyntaxError', function() { new
DOMMatrix('scale(2)translateX(5px)'); });
Why is this removed? Are you sure this case was not revealing a bug in Chrome?

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:132:
assert_throws(new TypeError(), function() { new DOMMatrixReadOnly(); });
Based on my reading of the spec, this call to the constructor should just make
an identity matrix.  I am guessing that is youre interpretation too.  The
original author of this test must have had a reason though.  I think you should
reach out to them.

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html
(right):

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:128:
assert_throws(new SyntaxError(), function() { new DOMMatrixReadOnly(string); });
I agree with this correction under the assumption that this test was meant to
test an error in parsing the transform string. Unfortunately this test is not
well documented (no comments, no use of the description argument of
assert_throwa), so it is hard to guess what the intent it.  In the spec, there
are conditions under which the constructor throws a TypeError().  The spec says
a type error should be thrown if the current global object is not Window.  And I
think we might be lacking test coverage for that case. Basically: passing a
string to the DOMMatrix constructor on a worker should throw.  

This aspect of the spec is a bit weak IMHO, I bet it has to do with avoiding to
call the CSS parser off the main thread of the browser.  Though style cannot be
safely computed off the main thread, CSS parsing can be safe, and that is all we
need here.  It is basically like the work we did for canvas filters and color
values for OffscreenCanvas. Anyways, this getting off topic, but I think it
deserves a follow-up. Please create an issue either against the spec or in crbug
to get to the bottom of this.

[fserb, 2017-05-08 15:31:18]

fserb@chromium.org changed reviewers:
+ dschulze@chromium.org

[fserb, 2017-05-08 15:31:19]

dschulze: I'm changing some DOMMatrix-001 WPT, would nice to have your input on
this.

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html
(left):

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:81:
var string = 'scale(2 2) translateX(5) translateY(5)';
On 2017/05/08 at 14:50:39, Justin Novosad wrote:
> It seems the old version of the test was quite deliberate about testing
scale() with and without the comma.  I think it is possible that the comma-less
notation is correct (though not supported by blink).  Members of the CSS WG have
been working on updating the notation, but I am not sure what is the current
status of that work.  See:
https://wiki.csswg.org/ideas/functional-notation#transforms

It seems even the draft CSS transform doesn't accept the comma-less style. Also,
the unitless "5" is clearly not ok.

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:90:
assert_throws('SyntaxError', function() { new
DOMMatrix('scale(2)translateX(5px)'); });
On 2017/05/08 at 14:50:39, Justin Novosad wrote:
> Why is this removed? Are you sure this case was not revealing a bug in Chrome?

No spaces between function and (.
But the space between ) and the next function is optional.

https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:132:
assert_throws(new TypeError(), function() { new DOMMatrixReadOnly(); });
On 2017/05/08 at 14:50:39, Justin Novosad wrote:
> Based on my reading of the spec, this call to the constructor should just make
an identity matrix.  I am guessing that is youre interpretation too.  The
original author of this test must have had a reason though.  I think you should
reach out to them.

This is clearly off. The previous implementation had a empty constructor, now it
has a full optional one.

[simonp, 2017-05-09 12:23:11]

On 2017/05/08 15:31:19, fserb wrote:
> dschulze: I'm changing some DOMMatrix-001 WPT, would nice to have your input
on
> this.
> 
>
https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
> File
> third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html
> (left):
> 
>
https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:81:
> var string = 'scale(2 2) translateX(5) translateY(5)';
> On 2017/05/08 at 14:50:39, Justin Novosad wrote:
> > It seems the old version of the test was quite deliberate about testing
> scale() with and without the comma.  I think it is possible that the
comma-less
> notation is correct (though not supported by blink).  Members of the CSS WG
have
> been working on updating the notation, but I am not sure what is the current
> status of that work.  See:
> https://wiki.csswg.org/ideas/functional-notation#transforms
> 
> It seems even the draft CSS transform doesn't accept the comma-less style.
Also,
> the unitless "5" is clearly not ok.
> 
>
https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:90:
> assert_throws('SyntaxError', function() { new
> DOMMatrix('scale(2)translateX(5px)'); });
> On 2017/05/08 at 14:50:39, Justin Novosad wrote:
> > Why is this removed? Are you sure this case was not revealing a bug in
Chrome?
> 
> No spaces between function and (.
> But the space between ) and the next function is optional.
> 
>
https://codereview.chromium.org/2859293002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-001.html:132:
> assert_throws(new TypeError(), function() { new DOMMatrixReadOnly(); });
> On 2017/05/08 at 14:50:39, Justin Novosad wrote:
> > Based on my reading of the spec, this call to the constructor should just
make
> an identity matrix.  I am guessing that is youre interpretation too.  The
> original author of this test must have had a reason though.  I think you
should
> reach out to them.
> 
> This is clearly off. The previous implementation had a empty constructor, now
it
> has a full optional one.

The test was updated upstream in
https://github.com/w3c/web-platform-tests/pull/5719 -- please sync this with
latest wpt master.

[fserb, 2017-05-10 15:53:53]

Description was changed from

==========
Use union instead of overload fo DOMMatrix/DOMMatrixReadOnly

BUG=716169
==========

to

==========
Use union instead of overload fo DOMMatrix/DOMMatrixReadOnly

Fixes DOMMatrix-001 tests that were just broken as written (missing brackets)

BUG=716169
==========

[fserb, 2017-05-10 15:54:18]

The CQ bit was checked by fserb@chromium.org

[fserb, 2017-05-10 15:54:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2859293002/#ps60001
(title: "x")

[commit-bot: I haz the power, 2017-05-10 15:55:40]

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-05-10 17:10:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 17:11:00]

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

[fserb, 2017-05-10 21:13:01]

The CQ bit was checked by fserb@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:14:16]

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-05-11 00:00:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 00:00:56]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fserb, 2017-05-11 04:29:03]

The CQ bit was checked by fserb@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:29:52]

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-05-11 05:45:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494476943737220,
"parent_rev": "443ea5e342701a3b20212c655c7816cbd28b53c8", "commit_rev":
"6156f7b21c2555d4ce52cecff7f5851ebb57652e"}

[commit-bot: I haz the power, 2017-05-11 05:45:32]

Description was changed from

==========
Use union instead of overload fo DOMMatrix/DOMMatrixReadOnly

Fixes DOMMatrix-001 tests that were just broken as written (missing brackets)

BUG=716169
==========

to

==========
Use union instead of overload fo DOMMatrix/DOMMatrixReadOnly

Fixes DOMMatrix-001 tests that were just broken as written (missing brackets)

BUG=716169

Review-Url: https://codereview.chromium.org/2859293002
Cr-Commit-Position: refs/heads/master@{#470827}
Committed:
https://chromium.googlesource.com/chromium/src/+/6156f7b21c2555d4ce52cecff7f5...
==========

[commit-bot: I haz the power, 2017-05-11 05:45:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6156f7b21c2555d4ce52cecff7f5...