Delay the response to ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer via a callback to allow for b…

Delay the response to ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer via a callback to allow for buffer allocation on the gpu process.

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

[alexst (slow to review), 2014-05-17 02:44:31]

Please have a look. Thanks.

[reveman, 2014-05-19 05:51:06]

https://codereview.chromium.org/290573011/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/290573011/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1255:
gfx::GpuMemoryBufferHandle* handle = new gfx::GpuMemoryBufferHandle;
I prefer if you don't heap allocate this. Seems unnecessary and ownership passed
to the callback using base::Owned is a bit too subtle for my taste.

https://codereview.chromium.org/290573011/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1262:
ScopedCallbackRunner callback_runner(&reply_callback);
I prefer if you called ::GpuMemoryBufferAllocated directly instead. And just
used base::Bind below when you call 
GpuMemoryBufferImpl::AllocateForChildProcess. Basically replacing "handle->type
= gfx::EMPTY_BUFFER" below with "GpuMemoryBufferAllocated(reply,
gfx::GpuMemoryBufferHandle())".

https://codereview.chromium.org/290573011/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.h (right):

https://codereview.chromium.org/290573011/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.h:283:
gfx::GpuMemoryBufferHandle* handle);
Can we change this to:

void GpuMemoryBufferAllocated(
    IPC::Message* reply,
    const gfx::GpuMemoryBufferHandle& handle);

That way it would all line up nicely when moving it off the IO thread by adding
a AllocateGpuMemoryBufferOnUI/GpuMemoryBufferAllocatedOnUI pair of functions.
I'd like to do that as a follow up.

https://codereview.chromium.org/290573011/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/290573011/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:34: const base::Closure&
allocate_callback);
Instead of passing a heap allocated gfx::GpuMemoryBufferHandle to this function,
can make the callback:

typedef base::Callback<void(const gfx::GpuMemoryBufferHandle& handle)>
    AllocationCallback;

https://codereview.chromium.org/290573011/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_android.cc (right):

https://codereview.chromium.org/290573011/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:45:
allocate_callback.Run();
I think it'd be better to forward the responsibility to run this callback to the
implementation and make it transparent to this function whether it's sync or
async.

[alexst (slow to review), 2014-05-19 15:31:00]

New patch to address your comments is up, ptal.

[reveman, 2014-05-19 18:57:56]

lgtm with nits

https://codereview.chromium.org/290573011/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/290573011/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.h:16: AllocationCallback;
nit: I think this should be in the GpuMemoryBuffer namespace instead of just
content. Please move it into GpuMemoryBufferImpl class just before "virtual
~GpuMemoryB.."

https://codereview.chromium.org/290573011/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_win.cc (right):

https://codereview.chromium.org/290573011/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_win.cc:36: handle->type =
gfx::EMPTY_BUFFER;
nit: think you forgot to remove this line

[alexst (slow to review), 2014-05-19 21:34:31]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-05-19 21:36:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alexst@chromium.org/290573011/120001

[alexst (slow to review), 2014-05-19 21:46:50]

The CQ bit was unchecked by alexst@chromium.org

[alexst (slow to review), 2014-05-19 21:47:24]

+piman for owner review.

[danakj, 2014-05-20 19:37:47]

https://codereview.chromium.org/290573011/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/290573011/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:447:
ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer,
Should this message be renamed to not say "Sync"?

[alexst (slow to review), 2014-05-20 19:51:34]

https://codereview.chromium.org/290573011/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/290573011/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:447:
ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer,
On 2014/05/20 19:37:47, danakj wrote:
> Should this message be renamed to not say "Sync"?

It's still a sync message from the caller's point of view, the fact that its
reply is delayed is transparent to the sender.

Naming convention from ipc_message_macros.h:

// Use the IPC_MESSAGE_HANDLER_DELAY_REPLY macro instead of IPC_MESSAGE_HANDLER
//     IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_SyncMessageName,
//                                     OnSyncMessageName)

[piman, 2014-05-20 20:31:00]

lgtm

[alexst (slow to review), 2014-05-20 20:39:54]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-05-20 20:41:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alexst@chromium.org/290573011/120001

[alexst (slow to review), 2014-05-21 04:12:19]

The CQ bit was unchecked by alexst@chromium.org

[alexst (slow to review), 2014-05-21 04:12:29]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-05-21 04:13:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alexst@chromium.org/290573011/120001

[commit-bot: I haz the power, 2014-05-21 04:30:16]

Change committed as 271861