implement getBufferSubDataAsync prototype

Implement a prototype for the proposed getBufferSubDataAsync entry point specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
BUG=654609
CQ_INCLUDE_TRYBOTS=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
NOTRY=true

Committed: https://crrev.com/884e8e8ad9cf667829d80f4bd222a0369baa088b
Cr-Commit-Position: refs/heads/master@{#426335}

[Kai Ninomiya, 2016-09-30 01:10:34]

Description was changed from

==========
implement getBufferSubDataAsync prototype

BUG=616554
==========

to

==========
implement getBufferSubDataAsync prototype

BUG=616554
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Kai Ninomiya, 2016-09-30 01:10:35]

Description was changed from

==========
implement getBufferSubDataAsync prototype

BUG=616554
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
implement getBufferSubDataAsync prototype

BUG=616554
CQ_INCLUDE_TRYBOTS=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
==========

[Kai Ninomiya, 2016-09-30 01:11:10]

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

[commit-bot: I haz the power, 2016-09-30 01:11:36]

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, 2016-09-30 01:15:05]

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

[commit-bot: I haz the power, 2016-09-30 01:15:06]

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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Kai Ninomiya, 2016-09-30 19:56:16]

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

[commit-bot: I haz the power, 2016-09-30 19:56:58]

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, 2016-09-30 20:14:33]

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

[commit-bot: I haz the power, 2016-09-30 20:14:37]

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

[Kai Ninomiya, 2016-09-30 21:10:30]

The CQ bit was checked by kainino@chromium.org

[Kai Ninomiya, 2016-09-30 21:10:31]

The CQ bit was unchecked by kainino@chromium.org

[Kai Ninomiya, 2016-09-30 21:11:10]

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

[commit-bot: I haz the power, 2016-09-30 21:11:56]

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, 2016-09-30 21:29:46]

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

[commit-bot: I haz the power, 2016-09-30 21:29:47]

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

[Kai Ninomiya, 2016-09-30 21:32:43]

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

[commit-bot: I haz the power, 2016-09-30 21:33:22]

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, 2016-09-30 21:50:13]

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

[commit-bot: I haz the power, 2016-09-30 21:50:14]

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

[Kai Ninomiya, 2016-09-30 21:58:56]

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

[commit-bot: I haz the power, 2016-09-30 21:59:17]

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, 2016-09-30 22:32:49]

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

[commit-bot: I haz the power, 2016-09-30 22:32:49]

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

[Kai Ninomiya, 2016-10-01 00:26:57]

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

[commit-bot: I haz the power, 2016-10-01 00:27: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, 2016-10-01 00:32:41]

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

[commit-bot: I haz the power, 2016-10-01 00:32:42]

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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
  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_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Kai Ninomiya, 2016-10-01 01:17:03]

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

[commit-bot: I haz the power, 2016-10-01 01:17:26]

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, 2016-10-01 01:48:49]

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

[commit-bot: I haz the power, 2016-10-01 01:48:53]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Kai Ninomiya, 2016-10-03 16:44:32]

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

[commit-bot: I haz the power, 2016-10-03 16:44:48]

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, 2016-10-03 17:17:08]

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

[commit-bot: I haz the power, 2016-10-03 17:17:09]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Kai Ninomiya, 2016-10-04 00:56:11]

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

[commit-bot: I haz the power, 2016-10-04 00:56:40]

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, 2016-10-04 02:43:17]

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

[commit-bot: I haz the power, 2016-10-04 02:43:18]

Dry run: This issue passed the CQ dry run.

[Kai Ninomiya, 2016-10-07 19:03:09]

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

[commit-bot: I haz the power, 2016-10-07 19:03:35]

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

[Kai Ninomiya, 2016-10-07 19:06:41]

kainino@chromium.org changed reviewers:
+ dglazkov@chromium.org, esprehn@chromium.org, kbr@chromium.org,
zmo@chromium.org

[Kai Ninomiya, 2016-10-07 19:06:42]

Big CL incoming.

kbr, zmo: please review
* gpu/command_buffer/
* gpu/GLES2/gl2chromium_autogen.h
* third_party/WebKit/Source/modules/webgl/

esprehn, dglazkov: please review
* content/renderer/
* third_party/WebKit/Source/platform/graphics/
* third_party/WebKit/public/platform/

jochen, adamk: I have a question about promise resolution scheduling; please
take a look at the XXX comment on line 438 here:
https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...

Thanks all.

[commit-bot: I haz the power, 2016-10-07 19:06:49]

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

[commit-bot: I haz the power, 2016-10-07 19:06:51]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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_...)

[Kai Ninomiya, 2016-10-07 20:38:07]

On 2016/10/07 19:06:42, Kai Ninomiya wrote:
> Big CL incoming.
> 
> kbr, zmo: please review
> * gpu/command_buffer/
> * gpu/GLES2/gl2chromium_autogen.h
> * third_party/WebKit/Source/modules/webgl/
> 
> esprehn, dglazkov: please review
> * content/renderer/
> * third_party/WebKit/Source/platform/graphics/
> * third_party/WebKit/public/platform/
> 
> jochen, adamk: I have a question about promise resolution scheduling; please
> take a look at the XXX comment on line 438 here:
>
https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
> 
> Thanks all.

Info on this new addition:

We've implemented a proposed asynchronous version of getBufferSubData,
getBufferSubDataAsync, which we want to add to the WebGL 2 core spec. Our
performance results on this CL look very good. We have reimplemented one of
three.js's picking demos with getBufferSubDataAsync, and it solves the problem
of synchronous, blocking readback on the main thread.

Repo with demo: https://github.com/kainino0x/getBufferSubDataAsync-Demo

Time in synchronous picking code: reduced from 27-31ms to 0-1ms.
Time from pick triggered to render complete: reduced from 27-31ms to 17ms.

[adamk, 2016-10-07 23:00:41]

adamk@chromium.org changed reviewers:
+ tyoshino@chromium.org

[adamk, 2016-10-07 23:00:43]

+tyoshino, who knows more about the Blink side of this than me.

https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:441:
self->resolver->resolve(self->dstData);
My knowledge of the Blink event loop is a little rusty (it predates the
scheduler), but it definitely looks like other JS tasks could run, since
ScriptPromiseResolver::resume calls m_timer.startOneShot() in order to kick off
the actual resolution. I don't see how that could guarantee that other JS tasks
haven't been enqueued first.

[Kai Ninomiya, 2016-10-07 23:44:48]

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

[commit-bot: I haz the power, 2016-10-07 23:45:06]

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

[esprehn, 2016-10-08 03:02:49]

Can you write a more detailed change description and link to the spec? What does
this feature do? What is this code for? :)

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:408:
void* mappedData,
What's the purpose and relationship of these two void* ptrs? That's pretty
unusual in blink, are there things you can CHECK below about them?

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:418:
context->contextGL()->FreeSharedMemory(mappedData);
do these fields need to be public?

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:421:
DOMException::create(InvalidStateError, "context lost or destroyed");
Capitalize error messages. "Context ..."

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:427: if
(!self->context || !self->mappedData) {
lets move this into a method instead, you can proxy back if you need a static,
then you can avoid all of this self->

it's just self->doSomething()

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:429:
DOMException::create(InvalidStateError, "context lost or destroyed");
ditto

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:449:
ScriptPromise WebGL2RenderingContextBase::getBufferSubDataAsync(
This function is massive, can we make it into smaller steps?

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:459:
const char* funcName = "getBufferSubDataAsync";
static ?, probably kFunctionName.

Don't abbreviate in blink, actually isn't there a C++ thing for like this
__FUNCTION__?

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:504: if
(subByteLength == 0) {
if (!subByteLength)

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:509:
CheckedNumeric<long long> dstEnd = subByteLength;
everything down here seems like a separate function at least

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:824: :
public RefCounted<GetBufferSubDataAsyncCallback> {
Does this need to be a nested class?

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:827:
Persistent<DOMArrayBufferView> dstData;
We don't generally abbreviate in blink, does the spec use "dst" ?

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:831: long
long subByteLength;
Very few things in blink can actually handle something as big as a long long,
ex. JS can't even represent that. What does this get used for?

[esprehn, 2016-10-08 03:02:50]

Can you write a more detailed change description and link to the spec? What does
this feature do? What is this code for? :)

[commit-bot: I haz the power, 2016-10-08 03:03:40]

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

[commit-bot: I haz the power, 2016-10-08 03:03:42]

Dry run: This issue passed the CQ dry run.

[Kai Ninomiya, 2016-10-10 17:00:08]

Description was changed from

==========
implement getBufferSubDataAsync prototype

BUG=616554
CQ_INCLUDE_TRYBOTS=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

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
CQ_INCLUDE_TRYBOTS=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
==========

[Ken Russell (switch to Gerrit), 2016-10-10 22:21:30]

Thanks for putting this together Kai. It looks great overall and is yielding
great results.

Please add the timing information you sent to the WebGL working group as a
comment on http://crbug.com/616554 . I see you added an abbreviated version as a
comment on the CL, but those comments are hard to dig back into and reference
later. Then please reference your comment on the bug from the CL description.
(In the form: https://bugs.chromium.org/p/chromium/issues/detail?id=616554#c4 ).
Also please provide a few-sentence overview as the second paragraph of the CL
description per Elliott's request.

A few comments. With these and Elliott's addressed, this looks to me like it
will be ready to land.

https://codereview.chromium.org/2379203002/diff/220001/content/test/gpu/gpu_t...
File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right):

https://codereview.chromium.org/2379203002/diff/220001/content/test/gpu/gpu_t...
content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:50:
self.Fail('conformance2/context/methods-2.html', bug=616554)
Given that this is now behind a flag, is this suppression still needed? Let's
remove it if not.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:419:
mappedData = nullptr;
Upon entry please DCHECK(mappedData != NULL). We should not attempt to destroy
these callbacks twice.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:427: if
(!self->context || !self->mappedData) {
On 2016/10/08 03:02:48, esprehn wrote:
> lets move this into a method instead, you can proxy back if you need a static,
> then you can avoid all of this self->
> 
> it's just self->doSomething()

+1. That would make this code more comprehensible.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:440: //
XXX: What would happen if the DOM was suspended when the promise
It's OK to leave this question in place but please make it TODO(kainino) per the
style guide: https://google.github.io/styleguide/cppguide.html#TODO_Comments

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:459:
const char* funcName = "getBufferSubDataAsync";
On 2016/10/08 03:02:48, esprehn wrote:
> static ?, probably kFunctionName.
> 
> Don't abbreviate in blink, actually isn't there a C++ thing for like this
> __FUNCTION__?

Thanks Elliott for pointing that out. Kai, it does look like other code in
Chromium relies on the __FUNCTION__ macro working, so let's use that here.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:473: //
XXX: is this restriction enforced in the original getBufferSubData? Does
It looks like it isn't enforced and it should be. Could you file a follow-on bug
about doing that? Then this comment (which should be a TODO) can be removed. It
looks like that needs to be tested in conformance2/buffers/getBufferSubData.html
, too.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:492: long
long subByteLength = 0;
These "sub" names are confusing, especially when used within the callback. Could
you change the naming conventions to be more clear? Perhaps:

  - dstData -> destinationView
  - dstOffset -> destinationOffset
  - subBaseAddress -> destinationAddress
  - subByteLength -> destinationByteLength

?

If the naming convention for the output variables from
validateSubSourceAndGetData is well established in this file, though, don't
worry about this.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:518:
"beyond the end of buffer");
end of -> end of the?

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:543:
contextGL()->DeleteQueriesEXT(1, &queryID);
It seems incorrect to delete the query here. It seems to me it should be deleted
in the completion callback. See this code for reference:
https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?q=SignalQuery&s...

Maybe the SignalQuery call implicitly references the query object until the
callback is called, though, making the DeleteQueriesEXT call a no-op. Did you
investigate this?

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:825:
private:
Per the Google C++ style guide at
https://google.github.io/styleguide/cppguide.html#Declaration_Order , the public
members should go first in the class.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:829: void*
subBaseAddress;
These variables could use some comments. Also, please group them together
according to relationship. dstData, subBaseAddress and subByteLength should be
together. mappedData should come either before or after, and have a separate
comment.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:831: long
long subByteLength;
On 2016/10/08 03:02:49, esprehn wrote:
> Very few things in blink can actually handle something as big as a long long,
> ex. JS can't even represent that. What does this get used for?

It represents the amount of data read back from the GPU to the CPU. It could use
an OpenGL typedef like GLuint, which is what's used in the IDL and which is
typedef'd there to something representable in JavaScript. Kai, would you make
that change?

[Kai Ninomiya, 2016-10-11 01:34:15]

Addressed most of the comments, still have to split the function tomorrow.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:408:
void* mappedData,
On 2016/10/08 03:02:48, esprehn wrote:
> What's the purpose and relationship of these two void* ptrs? That's pretty
> unusual in blink, are there things you can CHECK below about them?

I've clarified this a lot and added some DCHECKs.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:418:
context->contextGL()->FreeSharedMemory(mappedData);
On 2016/10/08 03:02:48, esprehn wrote:
> do these fields need to be public?

Not sure what you mean. The fields of (WebGL)GetBufferSubDataAsyncCallback don't
need to be (and are not) public.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:419:
mappedData = nullptr;
On 2016/10/10 22:21:30, Ken Russell wrote:
> Upon entry please DCHECK(mappedData != NULL). We should not attempt to destroy
> these callbacks twice.

Done.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:421:
DOMException::create(InvalidStateError, "context lost or destroyed");
On 2016/10/08 03:02:48, esprehn wrote:
> Capitalize error messages. "Context ..."

Done.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:427: if
(!self->context || !self->mappedData) {
On 2016/10/10 22:21:30, Ken Russell wrote:
> On 2016/10/08 03:02:48, esprehn wrote:
> > lets move this into a method instead, you can proxy back if you need a
static,
> > then you can avoid all of this self->
> > 
> > it's just self->doSomething()
> 
> +1. That would make this code more comprehensible.

Done.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:429:
DOMException::create(InvalidStateError, "context lost or destroyed");
On 2016/10/08 03:02:48, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:440: //
XXX: What would happen if the DOM was suspended when the promise
On 2016/10/10 22:21:30, Ken Russell wrote:
> It's OK to leave this question in place but please make it TODO(kainino) per
the
> style guide: https://google.github.io/styleguide/cppguide.html#TODO_Comments

Still working on this issue, will address this comment before committing.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:449:
ScriptPromise WebGL2RenderingContextBase::getBufferSubDataAsync(
On 2016/10/08 03:02:49, esprehn wrote:
> This function is massive, can we make it into smaller steps?

Great point, thanks. I will work on this tomorrow.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:459:
const char* funcName = "getBufferSubDataAsync";
On 2016/10/10 22:21:30, Ken Russell wrote:
> On 2016/10/08 03:02:48, esprehn wrote:
> > static ?, probably kFunctionName.
> > 
> > Don't abbreviate in blink, actually isn't there a C++ thing for like this
> > __FUNCTION__?
> 
> Thanks Elliott for pointing that out. Kai, it does look like other code in
> Chromium relies on the __FUNCTION__ macro working, so let's use that here.

Done. There is future work to do if we want to remove the "funcName" pattern in
other parts of this file.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:473: //
XXX: is this restriction enforced in the original getBufferSubData? Does
On 2016/10/10 22:21:30, Ken Russell wrote:
> It looks like it isn't enforced and it should be. Could you file a follow-on
bug
> about doing that? Then this comment (which should be a TODO) can be removed.
It
> looks like that needs to be tested in
conformance2/buffers/getBufferSubData.html
> , too.

Filed and assigned to myself: http://crbug.com/654609

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:492: long
long subByteLength = 0;
On 2016/10/10 22:21:30, Ken Russell wrote:
> These "sub" names are confusing, especially when used within the callback.
Could
> you change the naming conventions to be more clear? Perhaps:
> 
>   - dstData -> destinationView
>   - dstOffset -> destinationOffset
>   - subBaseAddress -> destinationAddress
>   - subByteLength -> destinationByteLength
> 
> ?
> 
> If the naming convention for the output variables from
> validateSubSourceAndGetData is well established in this file, though, don't
> worry about this.
> 

For dstData and dstOffset, I don't want to change these to mismatch the spec,
since the code for error messages will be very confusing.

For subBaseADdress and subByteLength, the names are well established. But I did
rename all of the fields in WebGLGetBufferSubDataAsyncCallback.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:504: if
(subByteLength == 0) {
On 2016/10/08 03:02:48, esprehn wrote:
> if (!subByteLength)

Done.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:518:
"beyond the end of buffer");
On 2016/10/10 22:21:30, Ken Russell wrote:
> end of -> end of the?

Done.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:543:
contextGL()->DeleteQueriesEXT(1, &queryID);
On 2016/10/10 22:21:30, Ken Russell wrote:
> It seems incorrect to delete the query here. It seems to me it should be
deleted
> in the completion callback. See this code for reference:
>
https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?q=SignalQuery&s...
> 
> Maybe the SignalQuery call implicitly references the query object until the
> callback is called, though, making the DeleteQueriesEXT call a no-op. Did you
> investigate this?

I didn't investigate this previously, but I wrote it assuming that the deletion
wouldn't be processed by the command buffer until after the query was signalled
(since both are ordered in the command stream). I'm not sure whether that is
true.

The query is removed from the client side query tracker immediately, but pretty
much everything else is on the service side, which I haven't looked into. The
round trip makes it difficult to figure out. Of course, it works currently, but
I will move it since I think it makes more sense.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:824: :
public RefCounted<GetBufferSubDataAsyncCallback> {
On 2016/10/08 03:02:49, esprehn wrote:
> Does this need to be a nested class?

I'll simplify this.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:825:
private:
On 2016/10/10 22:21:30, Ken Russell wrote:
> Per the Google C++ style guide at
> https://google.github.io/styleguide/cppguide.html#Declaration_Order , the
public
> members should go first in the class.

Done.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:827:
Persistent<DOMArrayBufferView> dstData;
On 2016/10/08 03:02:49, esprehn wrote:
> We don't generally abbreviate in blink, does the spec use "dst" ?

Yes, the spec uses "dst." However, it does not need to be abbreviated this way
here, so I'm going to rename several things here.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:831: long
long subByteLength;
On 2016/10/10 22:21:30, Ken Russell wrote:
> On 2016/10/08 03:02:49, esprehn wrote:
> > Very few things in blink can actually handle something as big as a long
long,
> > ex. JS can't even represent that. What does this get used for?
> 
> It represents the amount of data read back from the GPU to the CPU. It could
use
> an OpenGL typedef like GLuint, which is what's used in the IDL and which is
> typedef'd there to something representable in JavaScript. Kai, would you make
> that change?

esprehn: Excellent point, I agree. If it's alright, I'm going to handle this in
another CL. This type comes from existing code (validateSubSourceAndGetData) and
I will need to change it and all of its (four) usages at once, as well as
closely inspect changes to validateSubSourceAndGetData, which is a security
sensitive range validation function.

[Kai Ninomiya, 2016-10-11 21:54:43]

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:449:
ScriptPromise WebGL2RenderingContextBase::getBufferSubDataAsync(
On 2016/10/11 01:34:14, Kai Ninomiya wrote:
> On 2016/10/08 03:02:49, esprehn wrote:
> > This function is massive, can we make it into smaller steps?
> 
> Great point, thanks. I will work on this tomorrow.

I factored out a lot of common validation between getBufferSubData and
getBufferSubDataAsync, which makes it much shorter.

https://codereview.chromium.org/2379203002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:509:
CheckedNumeric<long long> dstEnd = subByteLength;
On 2016/10/08 03:02:48, esprehn wrote:
> everything down here seems like a separate function at least

Done.

[Kai Ninomiya, 2016-10-11 22:00:56]

(All comments should be addressed now, PTAL.)

[Ken Russell (switch to Gerrit), 2016-10-11 22:31:16]

Looks excellent. Few minor updates then LGTM.

https://codereview.chromium.org/2379203002/diff/260001/content/test/gpu/gpu_t...
File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right):

https://codereview.chromium.org/2379203002/diff/260001/content/test/gpu/gpu_t...
content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:50:
self.Fail('conformance2/context/methods-2.html', bug=616554)
Again - please remove, assuming this is passing now.

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:201: //
XXX: What would happen if the DOM was suspended when the promise
Still needs to be addressed. OK to leave as a TODO.

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:222: //
Pointer into the offset into destinationArrayBufferView
Add ".".

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:456: //
If the length of the copy is zero, this is a no-op
Add ".".

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:503: //
If the length of the copy is zero, this is a no-op
Add ".".

[Kai Ninomiya, 2016-10-11 22:42:17]

address comments

[Kai Ninomiya, 2016-10-11 22:42:35]

https://codereview.chromium.org/2379203002/diff/260001/content/test/gpu/gpu_t...
File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right):

https://codereview.chromium.org/2379203002/diff/260001/content/test/gpu/gpu_t...
content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:50:
self.Fail('conformance2/context/methods-2.html', bug=616554)
On 2016/10/11 22:31:16, Ken Russell wrote:
> Again - please remove, assuming this is passing now.

Sorry, forgot to reply on that. It is still not passing, so I didn't remove it.
Is that unexpected?

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:201: //
XXX: What would happen if the DOM was suspended when the promise
On 2016/10/11 22:31:16, Ken Russell wrote:
> Still needs to be addressed. OK to leave as a TODO.

Done. This is on my personal TODO for this project for the moment, but will move
to a crbug if it isn't addressed by the time I retire this list.

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:222: //
Pointer into the offset into destinationArrayBufferView
On 2016/10/11 22:31:16, Ken Russell wrote:
> Add ".".

Done.

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:456: //
If the length of the copy is zero, this is a no-op
On 2016/10/11 22:31:16, Ken Russell wrote:
> Add ".".

Done.

https://codereview.chromium.org/2379203002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:503: //
If the length of the copy is zero, this is a no-op
On 2016/10/11 22:31:16, Ken Russell wrote:
> Add ".".

Done.

[Ken Russell (switch to Gerrit), 2016-10-12 00:10:51]

https://codereview.chromium.org/2379203002/diff/260001/content/test/gpu/gpu_t...
File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right):

https://codereview.chromium.org/2379203002/diff/260001/content/test/gpu/gpu_t...
content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:50:
self.Fail('conformance2/context/methods-2.html', bug=616554)
On 2016/10/11 22:42:34, Kai Ninomiya wrote:
> On 2016/10/11 22:31:16, Ken Russell wrote:
> > Again - please remove, assuming this is passing now.
> 
> Sorry, forgot to reply on that. It is still not passing, so I didn't remove
it.
> Is that unexpected?

Argh. I checked and the harness is in fact passing
--enable-experimental-canvas-features for other reasons (to get WebGL
conformance tests related to ImageBitmap, etc. to run). Sorry about that. OK to
leave this suppression. We'll probably end up moving this to an extension
anyway.

[yhirano, 2016-10-12 09:32:01]

yhirano@chromium.org changed reviewers:
+ yhirano@chromium.org

[yhirano, 2016-10-12 09:32:03]

https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:441:
self->resolver->resolve(self->dstData);
On 2016/10/07 23:00:43, adamk wrote:
> My knowledge of the Blink event loop is a little rusty (it predates the
> scheduler), but it definitely looks like other JS tasks could run, since
> ScriptPromiseResolver::resume calls m_timer.startOneShot() in order to kick
off
> the actual resolution. I don't see how that could guarantee that other JS
tasks
> haven't been enqueued first.

When the window is suspended, ScriptPromiseResolver will wait for window to be
resumed. Otherwise, ScriptPromiseResolver will call actual resolve()
immediately, but the callbacks attached to the promise will be called later.

[tyoshino (SeeGerritForStatus), 2016-10-12 10:00:39]

https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:441:
self->resolver->resolve(self->dstData);
On 2016/10/07 23:00:43, adamk wrote:
> My knowledge of the Blink event loop is a little rusty (it predates the
> scheduler), but it definitely looks like other JS tasks could run, since
> ScriptPromiseResolver::resume calls m_timer.startOneShot() in order to kick
off
> the actual resolution. I don't see how that could guarantee that other JS
tasks
> haven't been enqueued first.

As this looked related to promises, I asked yhirano@ to review this.

What do you want to guarantee? Please point the algorithm in
https://github.com/KhronosGroup/WebGL/pull/2079/files if any.

[Kai Ninomiya, 2016-10-12 18:43:33]

On 2016/10/12 10:00:39, tyoshino wrote:
>
https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:441:
> self->resolver->resolve(self->dstData);
> On 2016/10/07 23:00:43, adamk wrote:
> > My knowledge of the Blink event loop is a little rusty (it predates the
> > scheduler), but it definitely looks like other JS tasks could run, since
> > ScriptPromiseResolver::resume calls m_timer.startOneShot() in order to kick
> off
> > the actual resolution. I don't see how that could guarantee that other JS
> tasks
> > haven't been enqueued first.
> 
> As this looked related to promises, I asked yhirano@ to review this.
> 
> What do you want to guarantee? Please point the algorithm in
> https://github.com/KhronosGroup/WebGL/pull/2079/files if any.


Thanks, yhirano, tyoshino. Your input is greatly appreciated.

The best test, so far, to consider is this one (which currently seems to pass):
https://github.com/kainino0x/WebGL/blob/81121c75361f914e148b3737bce087adae69b...

It kicks off many promises at once. All of these promises are writing into the
SAME ArrayBufferView. However, the results for each one will be different. In
Blink, the promise resolution is done right after a memcpy:

    memcpy(self->subBaseAddress, self->mappedData, self->subByteLength);
    self->resolver->resolve(self->dstData);

What we want to guarantee is that no Javascript code (Promise callback or
otherwise) will ever see the new contents (from Promise _k_) of the
ArrayBufferView before the execution of Promise _k_'s callback. If this
guarantee is impossible or impractical we will have to consider a slightly
different approach.

[Zhenyao Mo, 2016-10-12 23:19:36]

patchset 15 -> patchset 17 change lgtm

[Kai Ninomiya, 2016-10-13 22:39:58]

Description was changed from

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
CQ_INCLUDE_TRYBOTS=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

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
BUG=654609
CQ_INCLUDE_TRYBOTS=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
==========

[Kai Ninomiya, 2016-10-14 01:02:26]

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

[commit-bot: I haz the power, 2016-10-14 01:02:55]

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

[yhirano, 2016-10-14 01:34:37]

On 2016/10/12 18:43:33, Kai Ninomiya wrote:
> On 2016/10/12 10:00:39, tyoshino wrote:
> >
>
https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2379203002/diff/200001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:441:
> > self->resolver->resolve(self->dstData);
> > On 2016/10/07 23:00:43, adamk wrote:
> > > My knowledge of the Blink event loop is a little rusty (it predates the
> > > scheduler), but it definitely looks like other JS tasks could run, since
> > > ScriptPromiseResolver::resume calls m_timer.startOneShot() in order to
kick
> > off
> > > the actual resolution. I don't see how that could guarantee that other JS
> > tasks
> > > haven't been enqueued first.
> > 
> > As this looked related to promises, I asked yhirano@ to review this.
> > 
> > What do you want to guarantee? Please point the algorithm in
> > https://github.com/KhronosGroup/WebGL/pull/2079/files if any.
> 
> 
> Thanks, yhirano, tyoshino. Your input is greatly appreciated.
> 
> The best test, so far, to consider is this one (which currently seems to
pass):
>
https://github.com/kainino0x/WebGL/blob/81121c75361f914e148b3737bce087adae69b...
> 
> It kicks off many promises at once. All of these promises are writing into the
> SAME ArrayBufferView. However, the results for each one will be different. In
> Blink, the promise resolution is done right after a memcpy:
> 
>     memcpy(self->subBaseAddress, self->mappedData, self->subByteLength);
>     self->resolver->resolve(self->dstData);
> 
> What we want to guarantee is that no Javascript code (Promise callback or
> otherwise) will ever see the new contents (from Promise _k_) of the
> ArrayBufferView before the execution of Promise _k_'s callback. If this
> guarantee is impossible or impractical we will have to consider a slightly
> different approach.

So a JS developer provides a destination buffer to the browser via
getBufferSubDataAsync and the browser will resolve the returned promise after
copying some data to the destination buffer, right?

We cannot assume that no JS code can run between the buffer copy and callback
execution in general. ScriptPromiseResolver::resolve just calls the Promise
Resolve function[1] and it posts a microtask (a.k.a. Job) that runs fulfill
callbacks. If some other microtasks are already queued, they will run in order.
If one of them has the reference to the destination buffer, it will see that the
buffer is updated.

Maybe your situation is like the following JS code, I guess?

let r1, r2, x;
let p1 = new Promise(res => r1 = res);
let p2 = new Promise(res => r2 = res);
p1.then(() => console.log('x = ' + x));
p2.then(() => console.log('X = ' + x));
x = 0;
r1();
x = 1;
r2();
x = 2;

// => 'x = 2' and 'X = 2' will be printed.

1: http://www.ecma-international.org/ecma-262/6.0/#sec-promise-resolve-functions

[commit-bot: I haz the power, 2016-10-14 02:51:36]

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

[commit-bot: I haz the power, 2016-10-14 02:51:37]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-10-18 01:44:17]

please fix the naming before landing. My bad for not publishing my comments
before!

https://codereview.chromium.org/2379203002/diff/280001/gpu/command_buffer/com...
File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right):

https://codereview.chromium.org/2379203002/diff/280001/gpu/command_buffer/com...
gpu/command_buffer/common/gles2_cmd_format_autogen.h:15204: void Init(GLenum
_target,
we don't usually name mangle like this, do they do this in chromium?

In blink and content we'd write:

this->target = target;

instead. Also can we make this a class instead and just use the constructor?

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:155:
class WebGLGetBufferSubDataAsyncCallback
This should be garbage collected, and use a HeapHashSet instead

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:204:
this->promiseResolver->resolve(this->destinationArrayBufferView);
remove this-> and use m_

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:216:
void* shmReadbackResultData;
m_ for all the private fields

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:452: if
(message) {
You drop the message on the floor here, should you be throwing an exception? I
guess this is an old API, maybe add a TODO that it should possibly throw and we
should file a bug about the spec?

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:838:
std::set<WebGLGetBufferSubDataAsyncCallback*>
Use HashSet, you don't need the ordering

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:841: friend
class WebGLGetBufferSubDataAsyncCallback;
We usually discourage friends, can we add correct public API here?

https://codereview.chromium.org/2379203002/diff/340001/content/renderer/webgr...
File content/renderer/webgraphicscontext3d_provider_impl.cc (right):

https://codereview.chromium.org/2379203002/diff/340001/content/renderer/webgr...
content/renderer/webgraphicscontext3d_provider_impl.cc:49: void
WebGraphicsContext3DProviderImpl::signalQuery(
I know this whole file does it but we don't usually abbreviate, so this would be
callback

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:199:
memcpy(this->destinationDataPtr, this->shmReadbackResultData,
m_ and all of these should be private

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:223:
void* destinationDataPtr;
m_ and remove this-> on all accesses

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:51: GLenum,
Add argument names for all primitive types

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:824: const
char* validateGetBufferSubData(const char*,
Ditto

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:838:
std::set<WebGLGetBufferSubDataAsyncCallback*>
HashSet or ListHashSet

[esprehn, 2016-10-18 01:45:23]

Let's use HashSet or ListHashSet. Please fix the naming and style comments
before landing.

Lgtm

[Kai Ninomiya, 2016-10-19 17:54:11]

esprehn: Thanks!

yhirano: Thank you, your comments will be extremely helpful. We will deliberate
and see what we can do to address the issue - we may need to consider API
changes. We'll be in touch if we have more questions.

All, thank you for the assistance. I'm going to get this landed ASAP and fix up
any remaining issues in further CLs - this is still gated behind both
enable-unsafe-es3-apis and enable-experimental-canvas-features.

https://codereview.chromium.org/2379203002/diff/280001/gpu/command_buffer/com...
File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right):

https://codereview.chromium.org/2379203002/diff/280001/gpu/command_buffer/com...
gpu/command_buffer/common/gles2_cmd_format_autogen.h:15204: void Init(GLenum
_target,
On 2016/10/18 01:44:16, esprehn wrote:
> we don't usually name mangle like this, do they do this in chromium?
> 
> In blink and content we'd write:
> 
> this->target = target;
> 
> instead. Also can we make this a class instead and just use the constructor?

This is autogenerated code, so I can't do anything about it - at least not right
now.

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:204:
this->promiseResolver->resolve(this->destinationArrayBufferView);
On 2016/10/18 01:44:16, esprehn wrote:
> remove this-> and use m_

Done.

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:216:
void* shmReadbackResultData;
On 2016/10/18 01:44:16, esprehn wrote:
> m_ for all the private fields

Done.

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:452: if
(message) {
On 2016/10/18 01:44:16, esprehn wrote:
> You drop the message on the floor here, should you be throwing an exception? I
> guess this is an old API, maybe add a TODO that it should possibly throw and
we
> should file a bug about the spec?

Yeah, it's an already specified API and the spec doesn't throw. But it does set
a WebGL error (inside the validateGetBufferSubData function). I'll add a
comment, at least.

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:838:
std::set<WebGLGetBufferSubDataAsyncCallback*>
On 2016/10/18 01:44:16, esprehn wrote:
> Use HashSet, you don't need the ordering

Done.

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:841: friend
class WebGLGetBufferSubDataAsyncCallback;
On 2016/10/18 01:44:16, esprehn wrote:
> We usually discourage friends, can we add correct public API here?

Done.

https://codereview.chromium.org/2379203002/diff/340001/content/renderer/webgr...
File content/renderer/webgraphicscontext3d_provider_impl.cc (right):

https://codereview.chromium.org/2379203002/diff/340001/content/renderer/webgr...
content/renderer/webgraphicscontext3d_provider_impl.cc:49: void
WebGraphicsContext3DProviderImpl::signalQuery(
On 2016/10/18 01:44:16, esprehn wrote:
> I know this whole file does it but we don't usually abbreviate, so this would
be
> callback

Done.

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:199:
memcpy(this->destinationDataPtr, this->shmReadbackResultData,
On 2016/10/18 01:44:16, esprehn wrote:
> m_ and all of these should be private

Done.

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:223:
void* destinationDataPtr;
On 2016/10/18 01:44:16, esprehn wrote:
> m_ and remove this-> on all accesses

Done.

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:51: GLenum,
On 2016/10/18 01:44:17, esprehn wrote:
> Add argument names for all primitive types

Done.

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:824: const
char* validateGetBufferSubData(const char*,
On 2016/10/18 01:44:17, esprehn wrote:
> Ditto

Done.

https://codereview.chromium.org/2379203002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:838:
std::set<WebGLGetBufferSubDataAsyncCallback*>
On 2016/10/18 01:44:17, esprehn wrote:
> HashSet or ListHashSet

Done.

[Kai Ninomiya, 2016-10-19 18:17:49]

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2379203002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:155:
class WebGLGetBufferSubDataAsyncCallback
On 2016/10/18 01:44:16, esprehn wrote:
> This should be garbage collected, and use a HeapHashSet instead

Done.

[Ken Russell (switch to Gerrit), 2016-10-19 18:50:32]

Looks great. Still LGTM.

[Kai Ninomiya, 2016-10-19 18:54:19]

The CQ bit was checked by kainino@chromium.org

[Kai Ninomiya, 2016-10-19 18:54:20]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org,
esprehn@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2379203002/#ps420001
(title: "small clarification")

[commit-bot: I haz the power, 2016-10-19 18:55: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, 2016-10-19 23:00:38]

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

[commit-bot: I haz the power, 2016-10-19 23:00:41]

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

[Ken Russell (switch to Gerrit), 2016-10-19 23:09:43]

On 2016/10/19 23:00:41, commit-bot: I haz the power wrote:
> 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_...)

We filed http://crbug.com/657646 about this unrelated failure. I'm forcing this
through the CQ.

[Ken Russell (switch to Gerrit), 2016-10-19 23:09:57]

Description was changed from

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
BUG=654609
CQ_INCLUDE_TRYBOTS=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

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
BUG=654609
CQ_INCLUDE_TRYBOTS=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
NOTRY=true
==========

[Ken Russell (switch to Gerrit), 2016-10-19 23:10:07]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-10-19 23:11:04]

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

[Kai Ninomiya, 2016-10-19 23:13:14]

On 2016/10/19 23:09:43, Ken Russell OOO-till-Oct-19 wrote:
> On 2016/10/19 23:00:41, commit-bot: I haz the power wrote:
> > 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_...)
> 
> We filed http://crbug.com/657646 about this unrelated failure. I'm forcing
this
> through the CQ.

Correction: the blocking bug here is http://crbug.com/657514

[Ken Russell (switch to Gerrit), 2016-10-19 23:27:09]

On 2016/10/19 23:13:14, Kai Ninomiya wrote:
> On 2016/10/19 23:09:43, Ken Russell OOO-till-Oct-19 wrote:
> > On 2016/10/19 23:00:41, commit-bot: I haz the power wrote:
> > > 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_...)
> > 
> > We filed http://crbug.com/657646 about this unrelated failure. I'm forcing
> this
> > through the CQ.
> 
> Correction: the blocking bug here is http://crbug.com/657514

Thanks for the correction and sorry for my mistake.

[commit-bot: I haz the power, 2016-10-19 23:34:44]

Description was changed from

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
BUG=654609
CQ_INCLUDE_TRYBOTS=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
NOTRY=true
==========

to

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
BUG=654609
CQ_INCLUDE_TRYBOTS=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
NOTRY=true
==========

[commit-bot: I haz the power, 2016-10-19 23:34:48]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2016-10-21 13:13:13]

Description was changed from

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
BUG=654609
CQ_INCLUDE_TRYBOTS=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
NOTRY=true
==========

to

==========
Implement a prototype for the proposed getBufferSubDataAsync entry point
specified in this WebGL PR:

https://github.com/KhronosGroup/WebGL/pull/2079

BUG=616554
BUG=654609
CQ_INCLUDE_TRYBOTS=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
NOTRY=true

Committed: https://crrev.com/884e8e8ad9cf667829d80f4bd222a0369baa088b
Cr-Commit-Position: refs/heads/master@{#426335}
==========

[commit-bot: I haz the power, 2016-10-21 13:13:16]

Patchset 22 (id:??) landed as
https://crrev.com/884e8e8ad9cf667829d80f4bd222a0369baa088b
Cr-Commit-Position: refs/heads/master@{#426335}