[SharedArrayBuffer] Add "AllowShared" extended attribute, used for WebGL

[SharedArrayBuffer] Add "AllowShared" extended attribute, used for WebGL

The [AllowShared] extended attribute can be applied to a method's
ArrayBufferView parameter to indicate that the view can be backed by a
SharedArrayBuffer. For example:

    interface Context {
        void bufferData1([AllowShared] ArrayBufferView param);
        void bufferData2([AllowShared] Uint8Array param);
    }

By default, ArrayBufferView parameters can not be a view of a
SharedArrayBuffer, and will throw an exception in that case.

BUG=569681
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2815793002
Cr-Commit-Position: refs/heads/master@{#465787}
Committed: https://chromium.googlesource.com/chromium/src/+/ed24fe201ef7afca59b9562443cdbce3b1ce8142

[binji, 2017-04-12 01:07:01]

Description was changed from

==========
[SharedArrayBuffer] Add "AllowShared" extended attribute, used for WebGL

The [AllowShared] extended attribute can be applied to a method's
ArrayBufferView parameter to indicate that the view can be backed by a
SharedArrayBuffer. For example:

    interface Context {
        void bufferData1([AllowShared] ArrayBufferView param);
        void bufferData2([AllowShared] Uint8Array param);
    }

By default, ArrayBufferView parameters can not be a view of a
SharedArrayBuffer, and will throw an exception in that case.

BUG=569681
==========

to

==========
[SharedArrayBuffer] Add "AllowShared" extended attribute, used for WebGL

The [AllowShared] extended attribute can be applied to a method's
ArrayBufferView parameter to indicate that the view can be backed by a
SharedArrayBuffer. For example:

    interface Context {
        void bufferData1([AllowShared] ArrayBufferView param);
        void bufferData2([AllowShared] Uint8Array param);
    }

By default, ArrayBufferView parameters can not be a view of a
SharedArrayBuffer, and will throw an exception in that case.

BUG=569681
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[binji, 2017-04-12 01:11:29]

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

[binji, 2017-04-12 01:12:13]

On 2017/04/12 at 01:11:30, binji wrote:
> 

PTAL, I think this is relatively straightforward given the previous CL.

[binji, 2017-04-12 01:58:43]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 01:59:08]

Dry run: 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-04-12 03:21:40]

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

[commit-bot: I haz the power, 2017-04-12 03:21:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2017-04-12 04:37:13]

Mostly looks good.

- Can we add an error message to the IDL compiler when [AllowShared] is used on
types other than ArrayBufferViews?

- Can we add a layout test to test the behavior of [AllowShared]?

- This CL will need an approval of an API owner.

https://codereview.chromium.org/2815793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MaybeShared.h (right):

https://codereview.chromium.org/2815793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MaybeShared.h:24: class MaybeShared {

A couple of suggestions (which you can address in follow-up CLs):

- NotShared<> and MaybeShared<> sound a bit too generic. I'm fine with the class
names but would it be possible to put them in ArrayBufferViewHelper.h to clarify
that they are helper classes for ArrayBuffers?

- Would it be possible to add some DCHECK or static_assert to check that
NotShared<> and MaybeShared<> are used only for ArrayBufferViews?

[binji, 2017-04-12 21:15:27]

binji@chromium.org changed reviewers:
+ kbr@chromium.org

[binji, 2017-04-12 21:15:58]

On 2017/04/12 at 04:37:13, haraken wrote:
> - This CL will need an approval of an API owner.

+kbr for WebGL stuff

Working on addressing the rest of the comments now.

[Ken Russell (switch to Gerrit), 2017-04-12 23:33:52]

kbr@chromium.org changed reviewers:
+ zmo@chromium.org

[Ken Russell (switch to Gerrit), 2017-04-12 23:33:52]

I'd like to please defer review to zmo@, with kainino@ as a backup -- too
swamped today and tomorrow.

Mo, Kai, the idea behind these changes is to make WebGL work in the context of
WebAssembly with threads.

[Kai Ninomiya, 2017-04-12 23:50:50]

On 2017/04/12 23:33:52, Ken Russell wrote:
> I'd like to please defer review to zmo@, with kainino@ as a backup -- too
> swamped today and tomorrow.
> 
> Mo, Kai, the idea behind these changes is to make WebGL work in the context of
> WebAssembly with threads.

modules/webgl looks good - no real changes. (Non-owner.)

[Ken Russell (switch to Gerrit), 2017-04-13 01:59:47]

lgtm based on Kai's review.

[binji, 2017-04-13 04:30:30]

PTAL, I added a WebGL layout test, which made me realize that I had to modify
webgl code a little.

Essentially, we have DCHECKS that ensure that we aren't accessing
SharedArrayBuffer data unless it's specifically mentioned at the callsite (e.g.
"*MaybeShared")

[binji, 2017-04-13 04:30:57]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 04:31:33]

Dry run: 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-04-13 04:35:45]

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

[commit-bot: I haz the power, 2017-04-13 04:35:46]

Dry run: 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_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[haraken, 2017-04-13 04:51:13]

LGTM

(Please get an approval from an API owner.)

https://codereview.chromium.org/2815793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ArrayBufferViewHelpers.h (right):

https://codereview.chromium.org/2815793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ArrayBufferViewHelpers.h:17: // A wrapper
template type that is used to ensure that a TypedArray is not

Nit: Move the comment to above the class declaration.

https://codereview.chromium.org/2815793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ArrayBufferViewHelpers.h:73: // A wrapper
template type that specifies that a TypedArray may be backed by a

Ditto.

[Ken Russell (switch to Gerrit), 2017-04-13 05:10:23]

Good work on the layout test. I should have pushed for that earlier. It looks
good, but please let zmo@ take a look tomorrow as co-owner of this code.

[binji, 2017-04-13 18:30:55]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 18:31:30]

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

[binji, 2017-04-13 18:42:00]

https://codereview.chromium.org/2815793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ArrayBufferViewHelpers.h (right):

https://codereview.chromium.org/2815793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ArrayBufferViewHelpers.h:17: // A wrapper
template type that is used to ensure that a TypedArray is not
On 2017/04/13 at 04:51:12, haraken wrote:
> Nit: Move the comment to above the class declaration.

Done.

https://codereview.chromium.org/2815793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ArrayBufferViewHelpers.h:73: // A wrapper
template type that specifies that a TypedArray may be backed by a
On 2017/04/13 at 04:51:12, haraken wrote:
> Ditto.

Done.

[commit-bot: I haz the power, 2017-04-13 20:29:36]

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

[commit-bot: I haz the power, 2017-04-13 20:29:38]

Dry run: This issue passed the CQ dry run.

[binji, 2017-04-17 17:30:44]

+zmo PTAL

[Zhenyao Mo, 2017-04-17 17:35:45]

lgtm

[binji, 2017-04-17 17:44:05]

The CQ bit was checked by binji@chromium.org

[binji, 2017-04-17 17:44:06]

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

[commit-bot: I haz the power, 2017-04-17 17:44:38]

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-04-17 17:47:11]

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

[commit-bot: I haz the power, 2017-04-17 17:47:13]

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...)

[binji, 2017-04-18 18:18:45]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 18:19:14]

Dry run: 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-04-18 18:23:29]

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

[commit-bot: I haz the power, 2017-04-18 18:23:30]

Dry run: 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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[binji, 2017-04-19 20:41:34]

The CQ bit was checked by binji@chromium.org

[binji, 2017-04-19 20:41:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kbr@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2815793002/#ps80001
(title: "merge HEAD")

[commit-bot: I haz the power, 2017-04-19 20:42:05]

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-04-19 22:35:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492634494581600,
"parent_rev": "416a5cb2803e731f8d048bb138e9da4de72585eb", "commit_rev":
"ed24fe201ef7afca59b9562443cdbce3b1ce8142"}

[commit-bot: I haz the power, 2017-04-19 22:35:11]

Description was changed from

==========
[SharedArrayBuffer] Add "AllowShared" extended attribute, used for WebGL

The [AllowShared] extended attribute can be applied to a method's
ArrayBufferView parameter to indicate that the view can be backed by a
SharedArrayBuffer. For example:

    interface Context {
        void bufferData1([AllowShared] ArrayBufferView param);
        void bufferData2([AllowShared] Uint8Array param);
    }

By default, ArrayBufferView parameters can not be a view of a
SharedArrayBuffer, and will throw an exception in that case.

BUG=569681
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[SharedArrayBuffer] Add "AllowShared" extended attribute, used for WebGL

The [AllowShared] extended attribute can be applied to a method's
ArrayBufferView parameter to indicate that the view can be backed by a
SharedArrayBuffer. For example:

    interface Context {
        void bufferData1([AllowShared] ArrayBufferView param);
        void bufferData2([AllowShared] Uint8Array param);
    }

By default, ArrayBufferView parameters can not be a view of a
SharedArrayBuffer, and will throw an exception in that case.

BUG=569681
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2017-04-19 22:35:13]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ed24fe201ef7afca59b9562443cd...