Remove CommandBuffer::GetTransferBuffer.

Remove CommandBuffer::GetTransferBuffer.

The only time we call GetTransferBuffer is on Pepper, right after
CreateTransferBuffer, so combine the two. This lets us dramatically remove all
the maintenance burden (maps) and extra IPCs.

Also separate service-only calls out of CommandBuffer, into CommandBufferServiceBase

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260806

[piman, 2014-03-27 23:23:03]

sievers: please review
jschuh: IPC changes (ppapi_messages.h, gpu_messages.h)
darin: mojo/ OWNER
yzshen: ppapi/ OWNER

[jschuh, 2014-03-27 23:32:44]

ipc security lgtm (notes: message removal and consolidation, minor attack
surface reduction from two-step to one-step ipc)

[darin (slow to review), 2014-03-28 00:03:38]

LGTM for src/mojo/

[piman, 2014-03-28 03:37:54]

-yzshen (forgot he was out)
+bbudge for ppapi/

[bbudge, 2014-03-28 03:45:47]

One comment, otherwise ppapi LGTM.

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
ppapi/proxy/ppapi_command_buffer_proxy.cc:126: if ((*id) <= 0 ||
!handle.is_shmem())
Isn't id an int here?

[piman, 2014-03-28 04:47:54]

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
ppapi/proxy/ppapi_command_buffer_proxy.cc:126: if ((*id) <= 0 ||
!handle.is_shmem())
On 2014/03/28 03:45:47, bbudge wrote:
> Isn't id an int here?

id is a pointer to a signed int, yes?
PpapiHostMsg_PPBGraphics3D_CreateTransferBuffer will return a negative value for
id on failure, that's what we're checking here (note: code was already existing,
l.127 on the old file)

I'm not sure what is your concern?

[bbudge, 2014-03-28 05:43:29]

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
ppapi/proxy/ppapi_command_buffer_proxy.cc:126: if ((*id) <= 0 ||
!handle.is_shmem())
On 2014/03/28 04:47:55, piman wrote:
> On 2014/03/28 03:45:47, bbudge wrote:
> > Isn't id an int here?
> 
> id is a pointer to a signed int, yes?
> PpapiHostMsg_PPBGraphics3D_CreateTransferBuffer will return a negative value
for
> id on failure, that's what we're checking here (note: code was already
existing,
> l.127 on the old file)
> 
> I'm not sure what is your concern?

I got confused by the function signature of GetTransferBuffer, which is deleted.
Now I see this is part of CreateTransferBuffer. LGTM

[bbudge, 2014-03-28 05:51:30]

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
ppapi/proxy/ppapi_command_buffer_proxy.cc:126: if ((*id) <= 0 ||
!handle.is_shmem())
On 2014/03/28 05:43:30, bbudge wrote:
> On 2014/03/28 04:47:55, piman wrote:
> > On 2014/03/28 03:45:47, bbudge wrote:
> > > Isn't id an int here?
> > 
> > id is a pointer to a signed int, yes?
> > PpapiHostMsg_PPBGraphics3D_CreateTransferBuffer will return a negative value
> for
> > id on failure, that's what we're checking here (note: code was already
> existing,
> > l.127 on the old file)
> > 
> > I'm not sure what is your concern?
> 
> I got confused by the function signature of GetTransferBuffer, which is
deleted.
> Now I see this is part of CreateTransferBuffer. LGTM

nit: the parens aren't needed here, are they?

[piman, 2014-03-28 05:54:02]

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
ppapi/proxy/ppapi_command_buffer_proxy.cc:126: if ((*id) <= 0 ||
!handle.is_shmem())
On 2014/03/28 05:51:31, bbudge wrote:
> On 2014/03/28 05:43:30, bbudge wrote:
> > On 2014/03/28 04:47:55, piman wrote:
> > > On 2014/03/28 03:45:47, bbudge wrote:
> > > > Isn't id an int here?
> > > 
> > > id is a pointer to a signed int, yes?
> > > PpapiHostMsg_PPBGraphics3D_CreateTransferBuffer will return a negative
value
> > for
> > > id on failure, that's what we're checking here (note: code was already
> > existing,
> > > l.127 on the old file)
> > > 
> > > I'm not sure what is your concern?
> > 
> > I got confused by the function signature of GetTransferBuffer, which is
> deleted.
> > Now I see this is part of CreateTransferBuffer. LGTM
> 
> nit: the parens aren't needed here, are they?

Probably not, I can remove them.

[no sievers, 2014-03-28 21:01:02]

LGTM. Liking the cmdbuffer service interface split.

https://codereview.chromium.org/215803002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/215803002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/ppb_graphics_3d_impl.cc:122: if (id < 0)
Why not 'if (!buffer) return id'?

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (left):

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
ppapi/proxy/ppapi_command_buffer_proxy.cc:128: return NULL;
Ah, that's why.

[piman, 2014-03-28 23:23:40]

PTAL, I had messed up pepper, because I was destroying the shm before having a
chance to send it to the other side.

I simplified the internal API to forward the gpu::Buffer to the pepper proxy to
take care of that.

https://codereview.chromium.org/215803002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/215803002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/ppb_graphics_3d_impl.cc:122: if (id < 0)
On 2014/03/28 21:01:03, sievers wrote:
> Why not 'if (!buffer) return id'?

Either way, doesn't matter. Either buffer is set and id is >0, or buffer is NULL
and id is -1.
So test one and DCHECK the other.

Actually, this code changed in the latest version.

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/215803002/diff/20001/ppapi/proxy/ppapi_comman...
ppapi/proxy/ppapi_command_buffer_proxy.cc:126: if ((*id) <= 0 ||
!handle.is_shmem())
On 2014/03/28 05:54:02, piman wrote:
> On 2014/03/28 05:51:31, bbudge wrote:
> > On 2014/03/28 05:43:30, bbudge wrote:
> > > On 2014/03/28 04:47:55, piman wrote:
> > > > On 2014/03/28 03:45:47, bbudge wrote:
> > > > > Isn't id an int here?
> > > > 
> > > > id is a pointer to a signed int, yes?
> > > > PpapiHostMsg_PPBGraphics3D_CreateTransferBuffer will return a negative
> value
> > > for
> > > > id on failure, that's what we're checking here (note: code was already
> > > existing,
> > > > l.127 on the old file)
> > > > 
> > > > I'm not sure what is your concern?
> > > 
> > > I got confused by the function signature of GetTransferBuffer, which is
> > deleted.
> > > Now I see this is part of CreateTransferBuffer. LGTM
> > 
> > nit: the parens aren't needed here, are they?
> 
> Probably not, I can remove them.

Done.

[piman, 2014-03-28 23:23:43]

PTAL, I had messed up pepper, because I was destroying the shm before having a
chance to send it to the other side.

I simplified the internal API to forward the gpu::Buffer to the pepper proxy to
take care of that.

[piman, 2014-03-31 20:55:20]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:57:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/215803002/80001

[epenner, 2014-03-31 21:13:05]

The CQ bit was unchecked by epenner@chromium.org

[epenner, 2014-03-31 21:15:52]

On 2014/03/31 21:13:05, epenner wrote:
> The CQ bit was unchecked by mailto:epenner@chromium.org

If it's okay can I land this one first. My apologies as I wasn't aware the
branch cut was eminent. I will hit CQ on this again as soon as it lands.
https://codereview.chromium.org/217593004

[epenner, 2014-04-01 00:42:32]

The CQ bit was checked by epenner@chromium.org

[commit-bot: I haz the power, 2014-04-01 00:42:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/215803002/80001

[commit-bot: I haz the power, 2014-04-01 00:43:27]

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

[commit-bot: I haz the power, 2014-04-01 00:43:28]

Failed to apply patch for content/renderer/pepper/ppb_graphics_3d_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/pepper/ppb_graphics_3d_impl.cc
  Hunk #1 FAILED at 38.
  Hunk #2 succeeded at 112 (offset -1 lines).
  Hunk #3 FAILED at 124.
  2 out of 3 hunks FAILED -- saving rejects to file
content/renderer/pepper/ppb_graphics_3d_impl.cc.rej

Patch:       content/renderer/pepper/ppb_graphics_3d_impl.cc
Index: content/renderer/pepper/ppb_graphics_3d_impl.cc
diff --git a/content/renderer/pepper/ppb_graphics_3d_impl.cc
b/content/renderer/pepper/ppb_graphics_3d_impl.cc
index
18a8e90e0b63d61478acecaa2cfed2d7056d0f6a..3ad09d4eee47c9462d90f3def0fbf6beee360f35
100644
--- a/content/renderer/pepper/ppb_graphics_3d_impl.cc
+++ b/content/renderer/pepper/ppb_graphics_3d_impl.cc
@@ -38,23 +38,6 @@ namespace {
 const int32 kCommandBufferSize = 1024 * 1024;
 const int32 kTransferBufferSize = 1024 * 1024;
 
-PP_Bool ShmToHandle(base::SharedMemory* shm,
-                    size_t size,
-                    int* shm_handle,
-                    uint32_t* shm_size) {
-  if (!shm || !shm_handle || !shm_size)
-    return PP_FALSE;
-#if defined(OS_POSIX)
-  *shm_handle = shm->handle().fd;
-#elif defined(OS_WIN)
-  *shm_handle = reinterpret_cast<int>(shm->handle());
-#else
-  #error "Platform not supported."
-#endif
-  *shm_size = size;
-  return PP_TRUE;
-}
-
 }  // namespace.
 
 PPB_Graphics3D_Impl::PPB_Graphics3D_Impl(PP_Instance instance)
@@ -113,10 +96,10 @@ gpu::CommandBuffer::State PPB_Graphics3D_Impl::GetState() {
   return GetCommandBuffer()->GetState();
 }
 
-int32_t PPB_Graphics3D_Impl::CreateTransferBuffer(uint32_t size) {
-  int32_t id = -1;
-  GetCommandBuffer()->CreateTransferBuffer(size, &id);
-  return id;
+scoped_refptr<gpu::Buffer> PPB_Graphics3D_Impl::CreateTransferBuffer(
+    uint32_t size,
+    int32_t* id) {
+  return GetCommandBuffer()->CreateTransferBuffer(size, id);
 }
 
 PP_Bool PPB_Graphics3D_Impl::DestroyTransferBuffer(int32_t id) {
@@ -124,14 +107,6 @@ PP_Bool PPB_Graphics3D_Impl::DestroyTransferBuffer(int32_t
id) {
   return PP_TRUE;
 }
 
-PP_Bool PPB_Graphics3D_Impl::GetTransferBuffer(int32_t id,
-                                               int* shm_handle,
-                                               uint32_t* shm_size) {
-  scoped_refptr<gpu::Buffer> buffer =
GetCommandBuffer()->GetTransferBuffer(id);
-  return ShmToHandle(
-      buffer->shared_memory(), buffer->size(), shm_handle, shm_size);
-}
-
 PP_Bool PPB_Graphics3D_Impl::Flush(int32_t put_offset) {
   GetCommandBuffer()->Flush(put_offset);
   return PP_TRUE;

[epenner, 2014-04-01 01:14:02]

And... That's my fault. Sorry about that. I don't know if my patch needs a
merge, but if so it helps a great deal to go in first.

[piman, 2014-04-01 02:51:00]

On 2014/04/01 01:14:02, epenner wrote:
> And... That's my fault. Sorry about that. I don't know if my patch needs a
> merge, but if so it helps a great deal to go in first.

no worries, land your patch first.

[piman, 2014-04-01 02:52:45]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-04-01 02:53:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/215803002/100001

[commit-bot: I haz the power, 2014-04-01 09:06:46]

Change committed as 260806