Stop accepting string constructor of DOMMatrix on workers

Stop accepting string constructor of DOMMatrix on workers

This conforms to the spec:
https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-dommatrixreadonly

And ensures that we don't call SetMatrixValueFromString inside a worker


BUG=703908
Review-Url: https://codereview.chromium.org/2868953002
Cr-Commit-Position: refs/heads/master@{#471182}
Committed: https://chromium.googlesource.com/chromium/src/+/d72fe5c3a271ab3e63f2ca5c247b3e4e686e275b

[fserb, 2017-05-08 20:40:56]

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

[fserb, 2017-05-08 20:40:56]

Just as a reminder, this is dependent on
https://codereview.chromium.org/2859293002 that unifies the constructors.
That CL is on hold due to WPTests until tomorrow.

[Justin Novosad, 2017-05-09 15:28:19]

Tests are missing.  You should drop a test under WPT that tries to invoke the
"string" constructor on a worker.

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

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:106: if
(execution_context->IsWorkerOrWorkletGlobalScope()) {
The spec says to throw if the global object is not a Window. Are you sure this
condition is exactly equivalent to that?

[Justin Novosad, 2017-05-09 15:29:07]

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

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:108: "String
constructor can't be used on workers.");
This message is unclear.  Are we constructing a String here?

[fserb, 2017-05-09 18:47:45]

fserb@chromium.org changed reviewers:
+ simonp@opera.com

[fserb, 2017-05-09 18:47:46]

simonp: Could you please take a look at: 
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:106

the spec says "If current global object is not a Window object, then throw a
TypeError exception.", is this a reasonable way to check for that? Should we
change the spec to be more explicit about workers?

[chrishtr, 2017-05-11 15:03:50]

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/geometry/DOMMatrix.idl (right):

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrix.idl:11:
Exposed=(Window,Worker),
Why not Exposed=Window and done?

[Justin Novosad, 2017-05-11 15:28:21]

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/geometry/DOMMatrix.idl (right):

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrix.idl:11:
Exposed=(Window,Worker),
On 2017/05/11 15:03:50, chrishtr wrote:
> Why not Exposed=Window and done?

Because the CanvasTransform interface[1] is exposed in Workers and it has a
dependency on DOMMatrix.

[1] https://html.spec.whatwg.org/multipage/scripting.html#canvastransform

[chrishtr, 2017-05-11 15:35:48]

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/geometry/DOMMatrix.idl (right):

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrix.idl:11:
Exposed=(Window,Worker),
On 2017/05/11 at 15:28:21, Justin Novosad wrote:
> On 2017/05/11 15:03:50, chrishtr wrote:
> > Why not Exposed=Window and done?
> 
> Because the CanvasTransform interface[1] is exposed in Workers and it has a
dependency on DOMMatrix.
> 
> [1] https://html.spec.whatwg.org/multipage/scripting.html#canvastransform

Oh sorry, I meant that I thought there was a way to omit specific
constructors or methods on worker via IDL syntax. Is that possible?

[Justin Novosad, 2017-05-11 16:04:41]

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/geometry/DOMMatrix.idl (right):

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrix.idl:11:
Exposed=(Window,Worker),
On 2017/05/11 15:35:48, chrishtr wrote:
> On 2017/05/11 at 15:28:21, Justin Novosad wrote:
> > On 2017/05/11 15:03:50, chrishtr wrote:
> > > Why not Exposed=Window and done?
> > 
> > Because the CanvasTransform interface[1] is exposed in Workers and it has a
> dependency on DOMMatrix.
> > 
> > [1] https://html.spec.whatwg.org/multipage/scripting.html#canvastransform
> 
> Oh sorry, I meant that I thought there was a way to omit specific
> constructors or methods on worker via IDL syntax. Is that possible?

Ah, right. But in this case it is a single constructor with a union arg. 
Overloads are not allowed to differ by arg type only: they must have a different
number of arguments.  Therefore, there is no way of separating the constructor
into two declarations.

[chrishtr, 2017-05-11 17:03:46]

chrishtr@chromium.org changed reviewers:
+ foolip@chromium.org

[chrishtr, 2017-05-11 17:03:46]

+foolip, any ideas on the issue of the best way to suppress for workers?

[fserb, 2017-05-11 17:14:21]

On 2017/05/11 at 17:03:46, chrishtr wrote:
> +foolip, any ideas on the issue of the best way to suppress for workers?

I can't find any example of particular constructors OR exposed.
But if that was the case, then it would have to be something that makes sense on
the IDL too, otherwise we would differ from the spec.

[foolip, 2017-05-11 18:58:09]

On 2017/05/11 17:14:21, fserb wrote:
> On 2017/05/11 at 17:03:46, chrishtr wrote:
> > +foolip, any ideas on the issue of the best way to suppress for workers?
> 
> I can't find any example of particular constructors OR exposed.
> But if that was the case, then it would have to be something that makes sense
on
> the IDL too, otherwise we would differ from the spec.

It can't be done using standard Web IDL, and I assume we don't have any
Blink-specific IDL for it. What DOMMatrix.cpp does is what the spec suggests.

[foolip, 2017-05-11 18:59:47]

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

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrix.cpp:18: if
(execution_context->IsWorkerOrWorkletGlobalScope()) {
To stay pedantically close to the spec and leave no doubt about the only place
it *does* work, !execution_context->IsDocument() instead?

[fserb, 2017-05-11 19:13:55]

Updated with better check and message. :)

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

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrix.cpp:18: if
(execution_context->IsWorkerOrWorkletGlobalScope()) {
On 2017/05/11 at 18:59:47, foolip wrote:
> To stay pedantically close to the spec and leave no doubt about the only place
it *does* work, !execution_context->IsDocument() instead?

yes! That's the thing we were not sure. Thanks :)

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

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:106: if
(execution_context->IsWorkerOrWorkletGlobalScope()) {
On 2017/05/09 at 15:28:19, Justin Novosad wrote:
> The spec says to throw if the global object is not a Window. Are you sure this
condition is exactly equivalent to that?

foolip got that! :)

https://codereview.chromium.org/2868953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:108: "String
constructor can't be used on workers.");
On 2017/05/09 at 15:29:07, Justin Novosad wrote:
> This message is unclear.  Are we constructing a String here?

changed.

[Justin Novosad, 2017-05-11 19:25:08]

lgtm

[chrishtr, 2017-05-11 19:32:41]

lgtm

[fserb, 2017-05-11 19:38:34]

The CQ bit was checked by fserb@chromium.org

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

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 21:52:59]

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

[commit-bot: I haz the power, 2017-05-11 21:53:01]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fserb, 2017-05-11 21:56:10]

The CQ bit was checked by fserb@chromium.org

[commit-bot: I haz the power, 2017-05-11 21:57:01]

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-12 02:04:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494539770472590,
"parent_rev": "60706234db798ccbc85289d12cb2160d92c2e6ee", "commit_rev":
"d72fe5c3a271ab3e63f2ca5c247b3e4e686e275b"}

[commit-bot: I haz the power, 2017-05-12 02:04:28]

Description was changed from

==========
Stop accepting string constructor of DOMMatrix on workers

This conforms to the spec:
https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-dommatrixreadonly

And ensures that we don't call SetMatrixValueFromString inside a worker


BUG=703908
==========

to

==========
Stop accepting string constructor of DOMMatrix on workers

This conforms to the spec:
https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-dommatrixreadonly

And ensures that we don't call SetMatrixValueFromString inside a worker


BUG=703908

Review-Url: https://codereview.chromium.org/2868953002
Cr-Commit-Position: refs/heads/master@{#471182}
Committed:
https://chromium.googlesource.com/chromium/src/+/d72fe5c3a271ab3e63f2ca5c247b...
==========

[commit-bot: I haz the power, 2017-05-12 02:04:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d72fe5c3a271ab3e63f2ca5c247b...