Added a unique command buffer ID for command buffers.

Added a unique command buffer ID for command buffers.

R=piman@chromium.org
BUG=514815
Committed: https://crrev.com/12e459621d3b0a4ecb425c3ebde19aacb2545838
Cr-Commit-Position: refs/heads/master@{#349546}

[David Yen, 2015-09-15 19:22:21]

+jam for mojo changes.

For ppapi, we should be specifying the command buffer ID that it forwards the
messages to right? Is the only way to get that through a synchronous IPC call?

I'm not too sure how mojo works, are we supporting fences on
CommandBufferClientImpl?

[David Yen, 2015-09-15 19:22:40]

dyen@chromium.org changed reviewers:
+ jam@chromium.org

[David Yen, 2015-09-15 21:03:36]

On 2015/09/15 19:22:55, David Yen wrote:

Actually I think these command buffer IDs need to be better defined. Some
command buffers are proxied to one another and should all have the same command
buffer ID. Therefore the in process command buffer should probably have the same
ID as the CommandBufferService it contains.

I'm still not sure about the pepper and mojo cases though, does anyone have any
insight into this?

[David Yen, 2015-09-15 21:32:48]

dyen@chromium.org changed reviewers:
+ sievers@chromium.org

[David Yen, 2015-09-15 21:32:48]

+sievers@ for review. I pulled out the command buffer ID changes because erick@
needs it for something else.

[jam, 2015-09-15 22:13:00]

jam@chromium.org changed reviewers:
+ rjkroege@chromium.org

[jam, 2015-09-15 22:13:13]

On 2015/09/15 19:22:21, David Yen wrote:
> +jam for mojo changes.
> 
> For ppapi, we should be specifying the command buffer ID that it forwards the
> messages to right? Is the only way to get that through a synchronous IPC call?
> 
> I'm not too sure how mojo works, are we supporting fences on
> CommandBufferClientImpl?

+rjkroege

[rjkroege, 2015-09-15 22:48:41]

https://codereview.chromium.org/1345813002/diff/80001/mojo/gles2/command_buff...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/80001/mojo/gles2/command_buff...
mojo/gles2/command_buffer_client_impl.cc:123: :
CommandBuffer(gpu::kCommandBufferNamespace_Invalid, 0),
Is this right? Your goal with this CL is to give each command buffer a unique id
right? This would give the CommandBuffer instances uniform ids of 0. 

perhaps the ids could work like mojo ViewIds (src/components/mus/ids.h) and be a
combination of client-specific integer and the connect id? this seems more
aligned with mojo.

[piman, 2015-09-15 22:51:06]

https://codereview.chromium.org/1345813002/diff/100001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1345813002/diff/100001/components/mus/gles2/c...
components/mus/gles2/command_buffer_driver.cc:102:
kCommandBufferNamespace_Invalid, 0));
I guess we need a namespace for mojo too.

https://codereview.chromium.org/1345813002/diff/100001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1345813002/diff/100001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:478: uint64, /* command_buffer_id */
I don't think this is needed. You have the route id and the channel id on both
sides.

https://codereview.chromium.org/1345813002/diff/100001/gpu/command_buffer/com...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/1345813002/diff/100001/gpu/command_buffer/com...
gpu/command_buffer/common/command_buffer.h:29: class GPU_EXPORT CommandBuffer {
This is a virtual interface, it should not have members so that it can be used
for multiple interface inheritance.
We could add a (pure virtual) GetNamespaceId/GetCommandBufferId, but they need
to be implemented by concrete classes.
But I'm not really sure this belongs in this interface. On the client side, this
can go onto GpuControl. On the service side, AIUI only GLES2Decoder/ContextGroup
needs it, and can be given explicitly by its owner
(GpuCommandBufferStub/InProcessCommandBuffer).

[David Yen, 2015-09-15 23:09:03]

https://codereview.chromium.org/1345813002/diff/100001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1345813002/diff/100001/components/mus/gles2/c...
components/mus/gles2/command_buffer_driver.cc:102:
kCommandBufferNamespace_Invalid, 0));
On 2015/09/15 22:51:05, piman (slow to review) wrote:
> I guess we need a namespace for mojo too.

I'm not too familiar with mojo, does this class handle both the server and
client side? Are there more than 1 instance in a process?

https://codereview.chromium.org/1345813002/diff/100001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1345813002/diff/100001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:478: uint64, /* command_buffer_id */
On 2015/09/15 22:51:05, piman (slow to review) wrote:
> I don't think this is needed. You have the route id and the channel id on both
> sides.

I accidentally committed this before I was done. I think it is better to pass in
the command_buffer_id because we need to be sure this matches the command buffer
on the client side. My previous patch just had (client_id << 32 | route_id) on
both sides but we have no way of testing that they actually match. What do you
think?

https://codereview.chromium.org/1345813002/diff/100001/gpu/command_buffer/com...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/1345813002/diff/100001/gpu/command_buffer/com...
gpu/command_buffer/common/command_buffer.h:29: class GPU_EXPORT CommandBuffer {
On 2015/09/15 22:51:05, piman (slow to review) wrote:
> This is a virtual interface, it should not have members so that it can be used
> for multiple interface inheritance.
> We could add a (pure virtual) GetNamespaceId/GetCommandBufferId, but they need
> to be implemented by concrete classes.
> But I'm not really sure this belongs in this interface. On the client side,
this
> can go onto GpuControl. On the service side, AIUI only
GLES2Decoder/ContextGroup
> needs it, and can be given explicitly by its owner
> (GpuCommandBufferStub/InProcessCommandBuffer).

Doesn't it makes sense to put it as part of the command buffer class if this is
a way to uniquely identify command buffers? It is also convenient that the enum
is also declared here (which we wouldn't be able to do since GpuControl is only
client code).

[piman, 2015-09-15 23:17:17]

https://codereview.chromium.org/1345813002/diff/100001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1345813002/diff/100001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:478: uint64, /* command_buffer_id */
On 2015/09/15 23:09:03, David Yen wrote:
> On 2015/09/15 22:51:05, piman (slow to review) wrote:
> > I don't think this is needed. You have the route id and the channel id on
both
> > sides.
> 
> I accidentally committed this before I was done. I think it is better to pass
in
> the command_buffer_id because we need to be sure this matches the command
buffer
> on the client side. My previous patch just had (client_id << 32 | route_id) on
> both sides but we have no way of testing that they actually match. What do you
> think?

I prefer not overspecifying the message. My concern is a malicious client
sending a command_buffer_id that doesn't match the route_id/channel_id, which
could cause bugs in the service side if it's not resilient to those being
possibly mismatched.

https://codereview.chromium.org/1345813002/diff/100001/gpu/command_buffer/com...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/1345813002/diff/100001/gpu/command_buffer/com...
gpu/command_buffer/common/command_buffer.h:29: class GPU_EXPORT CommandBuffer {
On 2015/09/15 23:09:03, David Yen wrote:
> On 2015/09/15 22:51:05, piman (slow to review) wrote:
> > This is a virtual interface, it should not have members so that it can be
used
> > for multiple interface inheritance.
> > We could add a (pure virtual) GetNamespaceId/GetCommandBufferId, but they
need
> > to be implemented by concrete classes.
> > But I'm not really sure this belongs in this interface. On the client side,
> this
> > can go onto GpuControl. On the service side, AIUI only
> GLES2Decoder/ContextGroup
> > needs it, and can be given explicitly by its owner
> > (GpuCommandBufferStub/InProcessCommandBuffer).
> 
> Doesn't it makes sense to put it as part of the command buffer class if this
is
> a way to uniquely identify command buffers? It is also convenient that the
enum
> is also declared here (which we wouldn't be able to do since GpuControl is
only
> client code).

I'd strongly prefer CommandBuffer to only worry about executing commands, and
not have higher level features (channels, synchronization between them, threads
etc.) leak into it.
CommandBuffer doesn't know anything about the commands themselves, or GL or
higher-level semantics.

[David Yen, 2015-09-16 00:02:50]

PTAL. Although I am still unclear how the mojo and ppapi versions should look
like.

https://codereview.chromium.org/1345813002/diff/100001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1345813002/diff/100001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:478: uint64, /* command_buffer_id */
On 2015/09/15 23:17:17, piman (slow to review) wrote:
> On 2015/09/15 23:09:03, David Yen wrote:
> > On 2015/09/15 22:51:05, piman (slow to review) wrote:
> > > I don't think this is needed. You have the route id and the channel id on
> both
> > > sides.
> > 
> > I accidentally committed this before I was done. I think it is better to
pass
> in
> > the command_buffer_id because we need to be sure this matches the command
> buffer
> > on the client side. My previous patch just had (client_id << 32 | route_id)
on
> > both sides but we have no way of testing that they actually match. What do
you
> > think?
> 
> I prefer not overspecifying the message. My concern is a malicious client
> sending a command_buffer_id that doesn't match the route_id/channel_id, which
> could cause bugs in the service side if it's not resilient to those being
> possibly mismatched.

Ok, I've moved it back.

https://codereview.chromium.org/1345813002/diff/100001/gpu/command_buffer/com...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/1345813002/diff/100001/gpu/command_buffer/com...
gpu/command_buffer/common/command_buffer.h:29: class GPU_EXPORT CommandBuffer {
On 2015/09/15 23:17:17, piman (slow to review) wrote:
> On 2015/09/15 23:09:03, David Yen wrote:
> > On 2015/09/15 22:51:05, piman (slow to review) wrote:
> > > This is a virtual interface, it should not have members so that it can be
> used
> > > for multiple interface inheritance.
> > > We could add a (pure virtual) GetNamespaceId/GetCommandBufferId, but they
> need
> > > to be implemented by concrete classes.
> > > But I'm not really sure this belongs in this interface. On the client
side,
> > this
> > > can go onto GpuControl. On the service side, AIUI only
> > GLES2Decoder/ContextGroup
> > > needs it, and can be given explicitly by its owner
> > > (GpuCommandBufferStub/InProcessCommandBuffer).
> > 
> > Doesn't it makes sense to put it as part of the command buffer class if this
> is
> > a way to uniquely identify command buffers? It is also convenient that the
> enum
> > is also declared here (which we wouldn't be able to do since GpuControl is
> only
> > client code).
> 
> I'd strongly prefer CommandBuffer to only worry about executing commands, and
> not have higher level features (channels, synchronization between them,
threads
> etc.) leak into it.
> CommandBuffer doesn't know anything about the commands themselves, or GL or
> higher-level semantics.

Ok, I've moved it to GpuControl as you suggested.

[piman, 2015-09-16 00:50:48]

LGTM.

For ppapi, feel free to leave as a TODO... here's a copy&paste of what I
mentioned on the other CL:

The way PpapiCommandBufferProxy works is that it forwards all command
buffer IPCs to the renderer, that then sends them to the GPU process
using its own channel. So channel_id and route_id are known on the
renderer side at initialization time, and it's just a matter of sending
the FenceSyncNamespace in the reply of
PpapiHostMsg_PPBGraphics3D_Create. On the pepper side
(PPB_Graphics3D_Proxy::CreateProxyResource), it's just a matter of
wiring it to the PpapiCommandBufferProxy via Graphics3D::Init. On the
renderer side (PPB_Graphics3D_Proxy::OnMsgCreate), it's just a matter of
passing it through ResourceCreationImpl::CreateGraphics3DRaw ->
PPB_Graphics3D_Impl::CreateRaw -> PPB_Graphics3D_Impl::InitRaw and
grabbing it there from command_buffer_.

[rjkroege, 2015-09-16 18:55:22]

https://codereview.chromium.org/1345813002/diff/120001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/120001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:401: NOTIMPLEMENTED();
So, there must be a point to this CL or you wouldn't have added the code. :-)
So, what's going to go wrong in mandoline because all the command buffers are id
0? If having uniform cmd buffer ids of 0 is going to  break mandoline, I'd
really like you to implement this prior to landing.


Per my previous comment -- I think that there is a reasonably obvious namespace
id to give to a command buffer in mandoline that mirrors the way that we assign
ids to views: connection id concatenated with a per-client unique int.

[piman, 2015-09-16 20:18:07]

https://codereview.chromium.org/1345813002/diff/120001/mojo/gles2/command_buf...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/120001/mojo/gles2/command_buf...
mojo/gles2/command_buffer_client_impl.cc:401: NOTIMPLEMENTED();
On 2015/09/16 18:55:22, rjkroege wrote:
> So, there must be a point to this CL or you wouldn't have added the code. :-)
> So, what's going to go wrong in mandoline because all the command buffers are
id
> 0? If having uniform cmd buffer ids of 0 is going to  break mandoline, I'd
> really like you to implement this prior to landing.

It's not currently used, as you can see.
It will be used for memory tracing (which I don't think mandoline does), and for
the new lightweight sync points whose API can be implemented on top of existing
sync points, and will probably have to be, for mojo.

> Per my previous comment -- I think that there is a reasonably obvious
namespace
> id to give to a command buffer in mandoline that mirrors the way that we
assign
> ids to views: connection id concatenated with a per-client unique int.

What's a connection id, what's a view id? Where do they come from?
What about offscreen context that y'all just introduced for webgl?
https://codereview.chromium.org/1338433002/

Can we get help from the mandoline/mojo team to implement the mojo side of
things? It's becoming increasingly painful to make changes to command buffers
and cc when mojo gets in the way....

[rjkroege, 2015-09-16 21:01:06]

On 2015/09/16 20:18:07, piman (slow to review) wrote:
>
https://codereview.chromium.org/1345813002/diff/120001/mojo/gles2/command_buf...
> File mojo/gles2/command_buffer_client_impl.cc (right):
> 
>
https://codereview.chromium.org/1345813002/diff/120001/mojo/gles2/command_buf...
> mojo/gles2/command_buffer_client_impl.cc:401: NOTIMPLEMENTED();
> On 2015/09/16 18:55:22, rjkroege wrote:
> > So, there must be a point to this CL or you wouldn't have added the code.
:-)
> > So, what's going to go wrong in mandoline because all the command buffers
are
> id
> > 0? If having uniform cmd buffer ids of 0 is going to  break mandoline, I'd
> > really like you to implement this prior to landing.
> 
> It's not currently used, as you can see.
> It will be used for memory tracing (which I don't think mandoline does), and
for
> the new lightweight sync points whose API can be implemented on top of
existing
> sync points, and will probably have to be, for mojo.
> 
> > Per my previous comment -- I think that there is a reasonably obvious
> namespace
> > id to give to a command buffer in mandoline that mirrors the way that we
> assign
> > ids to views: connection id concatenated with a per-client unique int.
> 
> What's a connection id, what's a view id? Where do they come from?
> What about offscreen context that y'all just introduced for webgl?
> https://codereview.chromium.org/1338433002/
> 
> Can we get help from the mandoline/mojo team to implement the mojo side of
> things? It's becoming increasingly painful to make changes to command buffers
> and cc when mojo gets in the way....

Yes.

My apologies for being difficult. This was not my intent. I have communicated
very badly. I will try again. :-)

What I really want to know: does the fact that mandoline doesn't implement the
command buffer id allocation mean that mandoline will be unable to draw to the
screen in the very near future (like this week)? If so, I'd really like to see
that not happen. I'm totally fine seeing TODOs for mandoline team in mandoline
code if we have at least a few weeks to get to them.

lgtm

[David Yen, 2015-09-16 21:30:29]

dyen@chromium.org changed reviewers:
+ dcheng@chromium.org

[David Yen, 2015-09-16 21:30:30]

+dcheng@ for ppapi_messages.h changes

piman@, I implemented the changes necessary to plumb the ppapi namespace
id/command buffer id. PTAL. I wasn't sure how best to declare the shared struct,
it looks like other ones placed a new file in shared_impl so I followed that
model. Let me know if there is an easier way.

rjkroege@, what you said about client-specific integer and the connect id will
probably work, but the most important thing is that this needs to match the
server side decoder which doesn't look to be implemented yet. I don't want it to
be half implemented so I think it makes more sense to leave it as NOTIMPLEMENTED
for now.

[David Yen, 2015-09-16 21:32:46]

dyen@chromium.org changed reviewers:
+ bbudge@chromium.org, dcheng@google.com
- dcheng@chromium.org, jam@chromium.org, sievers@chromium.org

[David Yen, 2015-09-16 21:32:47]

+bbudge for ppapi changes.

[David Yen, 2015-09-16 21:33:42]

dyen@chromium.org changed reviewers:
+ jam@chromium.org

[David Yen, 2015-09-16 21:34:11]

dyen@chromium.org changed reviewers:
+ dcheng@chromium.org
- dcheng@google.com

[rjkroege, 2015-09-16 21:46:03]

On 2015/09/16 21:30:30, David Yen wrote:
> +dcheng@ for ppapi_messages.h changes
> 
> piman@, I implemented the changes necessary to plumb the ppapi namespace
> id/command buffer id. PTAL. I wasn't sure how best to declare the shared
struct,
> it looks like other ones placed a new file in shared_impl so I followed that
> model. Let me know if there is an easier way.
> 
> rjkroege@, what you said about client-specific integer and the connect id will
> probably work, but the most important thing is that this needs to match the
> server side decoder which doesn't look to be implemented yet. I don't want it
to
> be half implemented so I think it makes more sense to leave it as
NOTIMPLEMENTED
> for now.

sgtm. still lgtm. feel free to put a TODO(rjkoege): above that NOTIMPLEMENTED.

[piman, 2015-09-16 21:55:13]

lgtm

https://codereview.chromium.org/1345813002/diff/180001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/180001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:308: id_pair->namespace_id =
command_buffer_->GetNamespaceID();
Arguably you could hardcode the namespace on the ppapi side, because it has to
be a GpuIO type.

[David Yen, 2015-09-16 22:09:18]

On 2015/09/16 21:46:03, rjkroege wrote:
> On 2015/09/16 21:30:30, David Yen wrote:
> > +dcheng@ for ppapi_messages.h changes
> > 
> > piman@, I implemented the changes necessary to plumb the ppapi namespace
> > id/command buffer id. PTAL. I wasn't sure how best to declare the shared
> struct,
> > it looks like other ones placed a new file in shared_impl so I followed that
> > model. Let me know if there is an easier way.
> > 
> > rjkroege@, what you said about client-specific integer and the connect id
will
> > probably work, but the most important thing is that this needs to match the
> > server side decoder which doesn't look to be implemented yet. I don't want
it
> to
> > be half implemented so I think it makes more sense to leave it as
> NOTIMPLEMENTED
> > for now.
> 
> sgtm. still lgtm. feel free to put a TODO(rjkoege): above that NOTIMPLEMENTED.

Done.

[bbudge, 2015-09-16 22:17:19]

https://codereview.chromium.org/1345813002/diff/180001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/180001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:308: id_pair->namespace_id =
command_buffer_->GetNamespaceID();
On 2015/09/16 21:55:13, piman (slow to review) wrote:
> Arguably you could hardcode the namespace on the ppapi side, because it has to
> be a GpuIO type.

And that would simplify the patch significantly by allowing you to eliminate the
ppapi::CommandBufferIDPair struct, and just use a pointer to the
command_buffer_id.

WDYT?

[David Yen, 2015-09-16 22:25:28]

https://codereview.chromium.org/1345813002/diff/180001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/180001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:308: id_pair->namespace_id =
command_buffer_->GetNamespaceID();
On 2015/09/16 22:17:18, bbudge wrote:
> On 2015/09/16 21:55:13, piman (slow to review) wrote:
> > Arguably you could hardcode the namespace on the ppapi side, because it has
to
> > be a GpuIO type.
> 
> And that would simplify the patch significantly by allowing you to eliminate
the
> ppapi::CommandBufferIDPair struct, and just use a pointer to the
> command_buffer_id.
> 
> WDYT?

Done.

[bbudge, 2015-09-16 22:32:03]

Thanks. LGTM

[David Yen, 2015-09-17 16:45:05]

dyen@chromium.org changed reviewers:
+ sky@chromium.org
- jam@chromium.org

[David Yen, 2015-09-17 16:46:25]

+sky@ for review of
  components/mus/gles2/command_buffer_local.cc/h
  mojo/gles2/command_buffer_client_impl.cc/h

ping dcheng@ for ppapi_messages.h review

[sky, 2015-09-17 17:03:06]

https://codereview.chromium.org/1345813002/diff/250001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1345813002/diff/250001/components/mus/gles2/c...
components/mus/gles2/command_buffer_local.cc:216: NOTIMPLEMENTED();
Are these going to be spammy? What is the consequence of not implementing this?

[David Yen, 2015-09-17 17:07:04]

https://codereview.chromium.org/1345813002/diff/250001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1345813002/diff/250001/components/mus/gles2/c...
components/mus/gles2/command_buffer_local.cc:216: NOTIMPLEMENTED();
On 2015/09/17 17:03:06, sky wrote:
> Are these going to be spammy? What is the consequence of not implementing
this?

These are going to be used for a new sync point implementation, so sync points
will not work. However, since the old sync points were already NOTIMPLEMENTED()
it should be safe to assume they aren't used.

[dcheng, 2015-09-17 17:08:18]

https://codereview.chromium.org/1345813002/diff/250001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/250001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:30: uint64_t
CommandBufferProxyID(int channel_id, int32 route_id) {
Nit: wrap this in namespace { }?

https://codereview.chromium.org/1345813002/diff/250001/gpu/command_buffer/com...
File gpu/command_buffer/common/constants.h (right):

https://codereview.chromium.org/1345813002/diff/250001/gpu/command_buffer/com...
gpu/command_buffer/common/constants.h:71: kCommandBufferNamespace_Invalid = -1,
Chrome enum naming convention is to NAME_LIKE_THIS.

Also consider just using enum class.

https://codereview.chromium.org/1345813002/diff/250001/gpu/command_buffer/com...
gpu/command_buffer/common/constants.h:77: NUM_COMMAND_BUFFER_NAMESPACES
Is this actually used for anything?

[David Yen, 2015-09-17 17:26:21]

https://codereview.chromium.org/1345813002/diff/250001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/250001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:30: uint64_t
CommandBufferProxyID(int channel_id, int32 route_id) {
On 2015/09/17 17:08:18, dcheng wrote:
> Nit: wrap this in namespace { }?

Done.

https://codereview.chromium.org/1345813002/diff/250001/gpu/command_buffer/com...
File gpu/command_buffer/common/constants.h (right):

https://codereview.chromium.org/1345813002/diff/250001/gpu/command_buffer/com...
gpu/command_buffer/common/constants.h:71: kCommandBufferNamespace_Invalid = -1,
On 2015/09/17 17:08:18, dcheng wrote:
> Chrome enum naming convention is to NAME_LIKE_THIS.
> 
> Also consider just using enum class.

Done.

https://codereview.chromium.org/1345813002/diff/250001/gpu/command_buffer/com...
gpu/command_buffer/common/constants.h:77: NUM_COMMAND_BUFFER_NAMESPACES
On 2015/09/17 17:08:18, dcheng wrote:
> Is this actually used for anything?

The background of this patch is that it was moved out of another CL because
erick@ also needed to identify command buffers. The
NUM_COMMAND_BUFFER_NAMESPACES is used to declare an array for each namespace in
that patch:
(https://codereview.chromium.org/1339203002/diff/60001/gpu/command_buffer/serv...)

[sky, 2015-09-17 19:19:56]

LGTM

[dcheng, 2015-09-17 21:41:30]

lgtm

https://codereview.chromium.org/1345813002/diff/270001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/270001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:36: }  // anonymous
namespace
Nit: usually people just comment this as  "// namespace".

[David Yen, 2015-09-17 21:45:20]

https://codereview.chromium.org/1345813002/diff/270001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1345813002/diff/270001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:36: }  // anonymous
namespace
On 2015/09/17 21:41:30, dcheng wrote:
> Nit: usually people just comment this as  "// namespace".

Done.

[David Yen, 2015-09-17 21:45:25]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-09-17 21:45:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, piman@chromium.org, bbudge@chromium.org,
sky@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1345813002/#ps290001
(title: ""anonymous namespace" -> "namespace"")

[commit-bot: I haz the power, 2015-09-17 21:47:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1345813002/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1345813002/290001

[commit-bot: I haz the power, 2015-09-17 21:53:43]

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

[commit-bot: I haz the power, 2015-09-17 21:53:45]

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

[David Yen, 2015-09-17 22:42:03]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-09-17 22:42:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, bbudge@chromium.org, sky@chromium.org,
dcheng@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1345813002/#ps330001
(title: "Fix bad merge")

[commit-bot: I haz the power, 2015-09-17 22:44:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1345813002/330001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1345813002/330001

[commit-bot: I haz the power, 2015-09-18 00:13:58]

Committed patchset #18 (id:330001)

[commit-bot: I haz the power, 2015-09-18 00:14:37]

Patchset 18 (id:??) landed as
https://crrev.com/12e459621d3b0a4ecb425c3ebde19aacb2545838
Cr-Commit-Position: refs/heads/master@{#349546}