Modify build_gles2_cmd_buffer.py to generate a class that implements gpu::GLES2Interace and for eac…

Modify build_gles2_cmd_buffer.py to generate a class that implements gpu::GLES2Interace and for each entry point either defers to Mojo C thunk if we expose an entry point or check fails.

The code in its current form makes use of MojoGLES2GetGLES2Interface if
html_viewer makes use but we do not have a Mojo C thunk.
I'll be adding Mojo C thunks for other functions used by html_viewer and
eventually remove MojoGLES2GetGLES2Interface.

R=jamesr@chromium.org,davemoore@chromium.org
BUG=453591
Committed: https://crrev.com/24be20ca55237c93ea874d435ea19fc9a42ebb69
Cr-Commit-Position: refs/heads/master@{#321886}

[jamesr, 2015-03-19 18:22:50]

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:4059: "InsertSyncPointCHROMIUM"]
we have mojo C thunks for InsertSyncPointCHROMIUM:
https://github.com/domokit/mojo/blob/master/mojo/public/c/gles2/gles2_call_vi...

and for the mailbox entry points:
https://github.com/domokit/mojo/blob/master/mojo/public/c/gles2/gles2_call_vi...

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/client/mo...
File gpu/command_buffer/client/mojo_gles2_impl_autogen.h (right):

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/client/mo...
gpu/command_buffer/client/mojo_gles2_impl_autogen.h:21: class MojoGLES2Impl :
public gpu::gles2::GLES2Interface {
i think this file should go somewhere like mojo/gpu/.  we don't want to take a
mojo dependency in c++ files in //gpu/ just yet

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/client/mo...
gpu/command_buffer/client/mojo_gles2_impl_autogen.h:23:
MojoGLES2Impl(MojoGLES2Context context) { context_ = context; }
this should be explicit and there should be an explicitly declared dtor with an
override annotation

https://codereview.chromium.org/993253003/diff/1/third_party/mojo_services/sr...
File third_party/mojo_services/src/gpu/public/interfaces/gpu.mojom (left):

https://codereview.chromium.org/993253003/diff/1/third_party/mojo_services/sr...
third_party/mojo_services/src/gpu/public/interfaces/gpu.mojom:12:
CreateOnscreenGLES2Context(uint64 native_viewport_id,
this seems unrelated? this API change should come in via a mojo->chrome roll

[alhaad, 2015-03-19 23:14:59]

Thanks for the review. PTAL.
I think I'll need OWNERS approval as well. Who would you recommend?

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:4059: "InsertSyncPointCHROMIUM"]
On 2015/03/19 18:22:50, jamesr wrote:
> we have mojo C thunks for InsertSyncPointCHROMIUM:
>
https://github.com/domokit/mojo/blob/master/mojo/public/c/gles2/gles2_call_vi...
> 
> and for the mailbox entry points:
>
https://github.com/domokit/mojo/blob/master/mojo/public/c/gles2/gles2_call_vi...

Is it ok if I add it when I have added all the other entry points as well?

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/client/mo...
File gpu/command_buffer/client/mojo_gles2_impl_autogen.h (right):

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/client/mo...
gpu/command_buffer/client/mojo_gles2_impl_autogen.h:21: class MojoGLES2Impl :
public gpu::gles2::GLES2Interface {
On 2015/03/19 18:22:50, jamesr wrote:
> i think this file should go somewhere like mojo/gpu/.  we don't want to take a
> mojo dependency in c++ files in //gpu/ just yet

Done.

https://codereview.chromium.org/993253003/diff/1/gpu/command_buffer/client/mo...
gpu/command_buffer/client/mojo_gles2_impl_autogen.h:23:
MojoGLES2Impl(MojoGLES2Context context) { context_ = context; }
On 2015/03/19 18:22:50, jamesr wrote:
> this should be explicit and there should be an explicitly declared dtor with
an
> override annotation

Done.

https://codereview.chromium.org/993253003/diff/1/third_party/mojo_services/sr...
File third_party/mojo_services/src/gpu/public/interfaces/gpu.mojom (left):

https://codereview.chromium.org/993253003/diff/1/third_party/mojo_services/sr...
third_party/mojo_services/src/gpu/public/interfaces/gpu.mojom:12:
CreateOnscreenGLES2Context(uint64 native_viewport_id,
On 2015/03/19 18:22:50, jamesr wrote:
> this seems unrelated? this API change should come in via a mojo->chrome roll

Oops. I was using this to run my code.

[jamesr, 2015-03-19 23:18:50]

jamesr@chromium.org changed reviewers:
+ piman@chromium.org

[jamesr, 2015-03-19 23:18:51]

+piman for gpu/ owners

[piman, 2015-03-20 01:16:56]

On 2015/03/19 23:18:51, jamesr wrote:
> +piman for gpu/ owners

Can you add gpu/command_buffer OWNERS to mojo/gpu/OWNERS?
So that we can rev command buffers and GLES2Interface.

LGTM after that.

[chromium-reviews, 2015-03-20 20:41:03]

Thanks piman!
@jamesr: I need your approval as well for mojo/cc/*

On Thu, Mar 19, 2015 at 6:16 PM, <piman@chromium.org> wrote:

> On 2015/03/19 23:18:51, jamesr wrote:
>
>> +piman for gpu/ owners
>>
>
> Can you add gpu/command_buffer OWNERS to mojo/gpu/OWNERS?
> So that we can rev command buffers and GLES2Interface.
>
> LGTM after that.
>
> https://codereview.chromium.org/993253003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2015-03-23 18:03:14]

https://codereview.chromium.org/993253003/diff/60001/mojo/cc/context_provider...
File mojo/cc/context_provider_mojo.h (right):

https://codereview.chromium.org/993253003/diff/60001/mojo/cc/context_provider...
mojo/cc/context_provider_mojo.h:53: std::unique_ptr<gpu::gles2::GLES2Interface>
context_gl_;
sorry, you can't use std::unique_ptr<> as we compile this code with some
toolchains that don't have a C++11 standard library.  use base/memory/scoped_ptr
instead

https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn
File mojo/gpu/BUILD.gn (right):

https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode1
mojo/gpu/BUILD.gn:1: # Copyright (c) 2015 The Chromium Authors. All rights
reserved.
no (c)

https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode5
mojo/gpu/BUILD.gn:5: component("mojo_gles2_implementation") {
no need for this to be a component, is there? just make this a source_set

https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode13
mojo/gpu/BUILD.gn:13: defines = [ "GLES2_IMPL_IMPLEMENTATION" ]
and remove this. you don't have an export header so having a _IMPLEMENTATION
define is unnecessary (and will cause problems on windows if you #include
headers from a component that uses GLES2_IMPL_...)

[alhaad, 2015-03-23 18:15:55]

Thanks. PTAL.

https://codereview.chromium.org/993253003/diff/60001/mojo/cc/context_provider...
File mojo/cc/context_provider_mojo.h (right):

https://codereview.chromium.org/993253003/diff/60001/mojo/cc/context_provider...
mojo/cc/context_provider_mojo.h:53: std::unique_ptr<gpu::gles2::GLES2Interface>
context_gl_;
On 2015/03/23 18:03:14, jamesr wrote:
> sorry, you can't use std::unique_ptr<> as we compile this code with some
> toolchains that don't have a C++11 standard library.  use
base/memory/scoped_ptr
> instead

Thanks for pointing that out. Is this a general rule to be followed in all
Chromium / Mojo code?

https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn
File mojo/gpu/BUILD.gn (right):

https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode1
mojo/gpu/BUILD.gn:1: # Copyright (c) 2015 The Chromium Authors. All rights
reserved.
On 2015/03/23 18:03:14, jamesr wrote:
> no (c)

Done.

https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode5
mojo/gpu/BUILD.gn:5: component("mojo_gles2_implementation") {
On 2015/03/23 18:03:14, jamesr wrote:
> no need for this to be a component, is there? just make this a source_set

Done.

https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode13
mojo/gpu/BUILD.gn:13: defines = [ "GLES2_IMPL_IMPLEMENTATION" ]
On 2015/03/23 18:03:14, jamesr wrote:
> and remove this. you don't have an export header so having a _IMPLEMENTATION
> define is unnecessary (and will cause problems on windows if you #include
> headers from a component that uses GLES2_IMPL_...)

Done.

[jamesr, 2015-03-23 18:26:57]

On 2015/03/23 18:15:55, alhaad wrote:
> Thanks. PTAL.
> 
>
https://codereview.chromium.org/993253003/diff/60001/mojo/cc/context_provider...
> File mojo/cc/context_provider_mojo.h (right):
> 
>
https://codereview.chromium.org/993253003/diff/60001/mojo/cc/context_provider...
> mojo/cc/context_provider_mojo.h:53:
std::unique_ptr<gpu::gles2::GLES2Interface>
> context_gl_;
> On 2015/03/23 18:03:14, jamesr wrote:
> > sorry, you can't use std::unique_ptr<> as we compile this code with some
> > toolchains that don't have a C++11 standard library.  use
> base/memory/scoped_ptr
> > instead
> 
> Thanks for pointing that out. Is this a general rule to be followed in all
> Chromium / Mojo code?

For now, yes.  See http://chromium-cpp.appspot.com/ - this falls under the "no
C++11 library features are allowed." bit

> 
> https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn
> File mojo/gpu/BUILD.gn (right):
> 
>
https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode1
> mojo/gpu/BUILD.gn:1: # Copyright (c) 2015 The Chromium Authors. All rights
> reserved.
> On 2015/03/23 18:03:14, jamesr wrote:
> > no (c)
> 
> Done.
> 
>
https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode5
> mojo/gpu/BUILD.gn:5: component("mojo_gles2_implementation") {
> On 2015/03/23 18:03:14, jamesr wrote:
> > no need for this to be a component, is there? just make this a source_set
> 
> Done.
> 
>
https://codereview.chromium.org/993253003/diff/60001/mojo/gpu/BUILD.gn#newcode13
> mojo/gpu/BUILD.gn:13: defines = [ "GLES2_IMPL_IMPLEMENTATION" ]
> On 2015/03/23 18:03:14, jamesr wrote:
> > and remove this. you don't have an export header so having a _IMPLEMENTATION
> > define is unnecessary (and will cause problems on windows if you #include
> > headers from a component that uses GLES2_IMPL_...)
> 
> Done.

[jamesr, 2015-03-23 19:33:01]

lgtm

https://codereview.chromium.org/993253003/diff/80001/mojo/cc/context_provider...
File mojo/cc/context_provider_mojo.cc (right):

https://codereview.chromium.org/993253003/diff/80001/mojo/cc/context_provider...
mojo/cc/context_provider_mojo.cc:16: context_gl_(nullptr),
you don't need this - the default c'tor for scoped_ptr<> initializes to null

[alhaad, 2015-03-23 19:57:41]

The CQ bit was checked by alhaad@chromium.org

[alhaad, 2015-03-23 19:57:42]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/993253003/#ps80001 (title:
" ")

[commit-bot: I haz the power, 2015-03-23 19:58:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/993253003/80001

[commit-bot: I haz the power, 2015-03-23 20:06:30]

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

[commit-bot: I haz the power, 2015-03-23 20:06:31]

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

[alhaad, 2015-03-23 21:30:09]

The CQ bit was checked by alhaad@chromium.org

[alhaad, 2015-03-23 21:30:10]

The patchset sent to the CQ was uploaded after l-g-t-m from jamesr@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/993253003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-03-23 21:31:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/993253003/100001

[commit-bot: I haz the power, 2015-03-23 22:58:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-23 22:59:06]

Patchset 6 (id:??) landed as
https://crrev.com/24be20ca55237c93ea874d435ea19fc9a42ebb69
Cr-Commit-Position: refs/heads/master@{#321886}