Implement EXT_disjoint_timer_query_webgl2

Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

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

Committed: https://crrev.com/3eba0f99e3841bc85072e9a5a097d0fa54a06532
Cr-Commit-Position: refs/heads/master@{#427402}

[Kai Ninomiya, 2016-10-20 22:20:10]

Description was changed from

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

BUG=654134
==========

to

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

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

[Kai Ninomiya, 2016-10-20 22:20:40]

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

[commit-bot: I haz the power, 2016-10-20 22:21:12]

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-20 22:23:41]

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

[commit-bot: I haz the power, 2016-10-20 22:23:42]

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

[Kai Ninomiya, 2016-10-20 22:37:41]

rebase

[Kai Ninomiya, 2016-10-20 22:38:52]

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

[commit-bot: I haz the power, 2016-10-20 22:39:41]

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-20 22:42:57]

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

[commit-bot: I haz the power, 2016-10-20 23:37:55]

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

[commit-bot: I haz the power, 2016-10-20 23:37:56]

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

[Kai Ninomiya, 2016-10-20 23:58:28]

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

[commit-bot: I haz the power, 2016-10-20 23:59:07]

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

[Ken Russell (switch to Gerrit), 2016-10-21 00:18:14]

Thanks for taking care of this. Looks good overall, but a couple of apparent
errors in the query functions.

https://codereview.chromium.org/2437233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2437233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:3904:
return ScriptValue::createNull(scriptState);
I'm not sure this is the same thing as returning the integer 0. Please make sure
the new test verifies this code path and that it checks the returned type. If it
returns null then typeof(null)=="object" where typeof(0)=="number".

https://codereview.chromium.org/2437233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:3912:
return ScriptValue::createNull(scriptState);
Same here -- this needs to explicitly return a boolean.
typeof(false)=="boolean"; please verify this.

[commit-bot: I haz the power, 2016-10-21 03:49:47]

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

[commit-bot: I haz the power, 2016-10-21 03:49:48]

Dry run: This issue passed the CQ dry run.

[Kai Ninomiya, 2016-10-21 17:58:10]

https://codereview.chromium.org/2437233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2437233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:3904:
return ScriptValue::createNull(scriptState);
On 2016/10/21 00:18:13, Ken Russell wrote:
> I'm not sure this is the same thing as returning the integer 0. Please make
sure
> the new test verifies this code path and that it checks the returned type. If
it
> returns null then typeof(null)=="object" where typeof(0)=="number".

This is the case where the given pname (GL_TIMESTAMP_EXT) is not known (because
the extension is not enabled). Isn't returning null consistent with the
following?

"If pname is not in the table above and is not one of parameter names supported
by WebGL 1.0, generates an INVALID_ENUM error and returns null."

This is also the same semantic exposed in WebGL 1 with EXT_disjoint_timer_query.
This code is sort-of copied from WebGLRenderingContextBase::getParameter.

https://codereview.chromium.org/2437233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:3912:
return ScriptValue::createNull(scriptState);
On 2016/10/21 00:18:13, Ken Russell wrote:
> Same here -- this needs to explicitly return a boolean.
> typeof(false)=="boolean"; please verify this.

See above.

[Ken Russell (switch to Gerrit), 2016-10-21 20:53:14]

https://codereview.chromium.org/2437233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2437233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:3904:
return ScriptValue::createNull(scriptState);
On 2016/10/21 17:58:10, Kai Ninomiya wrote:
> On 2016/10/21 00:18:13, Ken Russell wrote:
> > I'm not sure this is the same thing as returning the integer 0. Please make
> sure
> > the new test verifies this code path and that it checks the returned type.
If
> it
> > returns null then typeof(null)=="object" where typeof(0)=="number".
> 
> This is the case where the given pname (GL_TIMESTAMP_EXT) is not known
(because
> the extension is not enabled). Isn't returning null consistent with the
> following?
> 
> "If pname is not in the table above and is not one of parameter names
supported
> by WebGL 1.0, generates an INVALID_ENUM error and returns null."
> 
> This is also the same semantic exposed in WebGL 1 with
EXT_disjoint_timer_query.
> This code is sort-of copied from WebGLRenderingContextBase::getParameter.

OK, I see. Thanks. I have a related concern which I'll raise in the most recent
patch set.

[Ken Russell (switch to Gerrit), 2016-10-21 21:00:20]

lgtm with the following questions addressed.

https://codereview.chromium.org/2437233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2437233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2786:
return ScriptValue::createNull(scriptState);
Is returning null here, rather than a boolean, correct? Does an error need to be
signaled?

https://codereview.chromium.org/2437233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2793:
return ScriptValue::createNull(scriptState);
Same here. Does an error need to be raised?

[Kai Ninomiya, 2016-10-24 20:00:31]

I changed the error checking logic and improved the WebGL tests to test it. PTAL
again.

https://codereview.chromium.org/2437233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2437233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2786:
return ScriptValue::createNull(scriptState);
On 2016/10/21 21:00:20, Ken Russell wrote:
> Is returning null here, rather than a boolean, correct? Does an error need to
be
> signaled?

Done.

https://codereview.chromium.org/2437233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2793:
return ScriptValue::createNull(scriptState);
On 2016/10/21 21:00:20, Ken Russell wrote:
> Same here. Does an error need to be raised?

Done.

[Kai Ninomiya, 2016-10-24 20:02:26]

Description was changed from

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

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

to

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

BUG=654134
TBR=haraken@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Kai Ninomiya, 2016-10-24 20:02:26]

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

[Kai Ninomiya, 2016-10-24 20:04:29]

haraken@: Added you to review only:
third_party/WebKit/Source/modules/modules_idl_files.gni
Thanks.

[haraken, 2016-10-24 20:05:51]

On 2016/10/24 20:04:29, Kai Ninomiya wrote:
> haraken@: Added you to review only:
> third_party/WebKit/Source/modules/modules_idl_files.gni
> Thanks.

LGTM

[Kai Ninomiya, 2016-10-24 20:52:24]

Description was changed from

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

BUG=654134
TBR=haraken@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

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

[Ken Russell (switch to Gerrit), 2016-10-24 21:42:41]

Thanks for the cleanups, code reorganization and the improved conformance tests
(in github.com/KhronosGroup/WebGL). Please roll those in when you have a chance.

Still LGTM.

[Kai Ninomiya, 2016-10-24 21:53:03]

The CQ bit was checked by kainino@chromium.org

[commit-bot: I haz the power, 2016-10-24 21:53:42]

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-24 22:55:00]

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

[commit-bot: I haz the power, 2016-10-24 22:55:01]

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-25 17:00:59]

The CQ bit was checked by kainino@chromium.org

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

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-25 17:47:32]

Description was changed from

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

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

to

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

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

[commit-bot: I haz the power, 2016-10-25 17:47:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-25 18:03:19]

Description was changed from

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

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

to

==========
Implement EXT_disjoint_timer_query_webgl2

Added in https://github.com/KhronosGroup/WebGL/pull/2076

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

Committed: https://crrev.com/3eba0f99e3841bc85072e9a5a097d0fa54a06532
Cr-Commit-Position: refs/heads/master@{#427402}
==========

[commit-bot: I haz the power, 2016-10-25 18:03:20]

Patchset 5 (id:??) landed as
https://crrev.com/3eba0f99e3841bc85072e9a5a097d0fa54a06532
Cr-Commit-Position: refs/heads/master@{#427402}