gpu: A common GpuMemoryBufferManager implemenetation for clients.

gpu: A common GpuMemoryBufferManager implemenetation for clients.

This CL introduces gpu::ClientGpuMemoryBufferManager, a common
implementation for gpu::GpuMemoryBufferManager. This is used by
content-renderers (ChildGpuMemoryBufferManager) and mus clients
(MojoGpuMemoryBufferManager), both of which gets the shared memory
allocation from a host process. These IPC related tasks used to allocate
the shared memory are performed through a delegate interface, so that
code in //content can continue to use chrome-ipc, while the mus client
lib can use mojo.

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[sadrul, 2016-11-08 06:23:37]

Description was changed from

==========
gpu: A common GpuMemoryBufferManager implemenetation for clients.

This CL introduces gpu::ClientGpuMemoryBufferManager, a common
implementation for gpu::GpuMemoryBufferManager. This is used by
content-renderers (ChildGpuMemoryBufferManager) and mus clients
(MojoGpuMemoryBufferManager), both of which gets the shared memory
allocation from a host process. These IPC related tasks used to allocate
the shared memory are performed through a delegate interface, so that
code in //content can continue to use chrome-ipc, while the mus client
lib can use mojo.

BUG=643746
==========

to

==========
gpu: A common GpuMemoryBufferManager implemenetation for clients.

This CL introduces gpu::ClientGpuMemoryBufferManager, a common
implementation for gpu::GpuMemoryBufferManager. This is used by
content-renderers (ChildGpuMemoryBufferManager) and mus clients
(MojoGpuMemoryBufferManager), both of which gets the shared memory
allocation from a host process. These IPC related tasks used to allocate
the shared memory are performed through a delegate interface, so that
code in //content can continue to use chrome-ipc, while the mus client
lib can use mojo.

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sadrul, 2016-11-08 06:23:43]

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

[commit-bot: I haz the power, 2016-11-08 06:23:58]

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-11-08 06:50:03]

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

[commit-bot: I haz the power, 2016-11-08 06:50:04]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[sadrul, 2016-11-08 07:06:16]

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

[commit-bot: I haz the power, 2016-11-08 07:06:29]

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-11-08 08:06:09]

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

[commit-bot: I haz the power, 2016-11-08 08:06:10]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sadrul, 2016-11-08 16:07:18]

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

[commit-bot: I haz the power, 2016-11-08 16:07:35]

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

[sadrul, 2016-11-08 17:29:44]

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

[commit-bot: I haz the power, 2016-11-08 17:30:28]

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

[sadrul, 2016-11-08 17:30:31]

sadrul@chromium.org changed reviewers:
+ reveman@chromium.org

[commit-bot: I haz the power, 2016-11-08 19:20:57]

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

[commit-bot: I haz the power, 2016-11-08 19:20:58]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-11-08 19:54:50]

hm, I'm not sure how useful this common implementation is. What's the code that
is shared after this change? Can we instead move that code into
GpuMemoryBufferImpl?

[sadrul, 2016-11-08 20:21:25]

On 2016/11/08 19:54:50, reveman wrote:
> hm, I'm not sure how useful this common implementation is. What's the code
that
> is shared after this change? Can we instead move that code into
> GpuMemoryBufferImpl?

The GpuMemoryBufferManager implementation is the one that is shared. With this
change, the clients don't need to manage the GpuMemoryBufferImpl instances
themselves; ClientGMBManager takes care of that. The content/mus-specific code
would simply be responsible for getting the shm handles.

I am not sure we can turn GpuMemoryBufferImpl itself into a GMBManager?

[reveman, 2016-11-09 00:47:59]

On 2016/11/08 at 20:21:25, sadrul wrote:
> On 2016/11/08 19:54:50, reveman wrote:
> > hm, I'm not sure how useful this common implementation is. What's the code
that
> > is shared after this change? Can we instead move that code into
> > GpuMemoryBufferImpl?
> 
> The GpuMemoryBufferManager implementation is the one that is shared. With this
change, the clients don't need to manage the GpuMemoryBufferImpl instances
themselves; ClientGMBManager takes care of that. The content/mus-specific code
would simply be responsible for getting the shm handles.

There isn't really any management going on in the ClientGpuMemoryBufferManager
case. It's just forwarding allocation requests to a manager in a more privilege
process and that's the part that we need to be different anyhow.

> 
> I am not sure we can turn GpuMemoryBufferImpl itself into a GMBManager?

The idea is to do something similar to GpuMemoryBufferImpl::CreateFromHandle.
Add static functions to GMBImpl so the logic can easily be shared between
different manager impls. However, I'm still failing to see what exact logic
we're able to share thanks to this patch. It just looks like we're adding
another layer of indirection through the delegate interface but maybe I'm
missing something.

[sadrul, 2016-11-09 02:56:22]

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

[commit-bot: I haz the power, 2016-11-09 02:56:56]

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

[sadrul, 2016-11-09 03:44:38]

On 2016/11/09 00:47:59, reveman wrote:
> On 2016/11/08 at 20:21:25, sadrul wrote:
> > On 2016/11/08 19:54:50, reveman wrote:
> > > hm, I'm not sure how useful this common implementation is. What's the code
> that
> > > is shared after this change? Can we instead move that code into
> > > GpuMemoryBufferImpl?
> > 
> > The GpuMemoryBufferManager implementation is the one that is shared. With
this
> change, the clients don't need to manage the GpuMemoryBufferImpl instances
> themselves; ClientGMBManager takes care of that. The content/mus-specific code
> would simply be responsible for getting the shm handles.
> 
> There isn't really any management going on in the ClientGpuMemoryBufferManager
> case. It's just forwarding allocation requests to a manager in a more
privilege
> process and that's the part that we need to be different anyhow.

Ah. It sounds like you are suggesting the callers of
GpuMemoryBufferManager::AllocateGpuMemoryBuffer() should just directly create
the GpuMemoryBufferImpl using some static function in GpuMemoryBufferImpl. Is
that correct? We could inject some static factory/delegate into
GpuMemoryBufferImpl (where the factory is responsible for sending the allocation
requests) so that all the callers wouldn't need to know about it. One difficulty
here is that that would introduce a dependency on //gpu/ipc/client to some
components that currently do not have that dependency (//cc, for example).

> 
> > 
> > I am not sure we can turn GpuMemoryBufferImpl itself into a GMBManager?
> 
> The idea is to do something similar to GpuMemoryBufferImpl::CreateFromHandle.
> Add static functions to GMBImpl so the logic can easily be shared between
> different manager impls. However, I'm still failing to see what exact logic
> we're able to share thanks to this patch. It just looks like we're adding
> another layer of indirection through the delegate interface but maybe I'm
> missing something.

[sadrul, 2016-11-09 03:44:38]

On 2016/11/09 00:47:59, reveman wrote:
> On 2016/11/08 at 20:21:25, sadrul wrote:
> > On 2016/11/08 19:54:50, reveman wrote:
> > > hm, I'm not sure how useful this common implementation is. What's the code
> that
> > > is shared after this change? Can we instead move that code into
> > > GpuMemoryBufferImpl?
> > 
> > The GpuMemoryBufferManager implementation is the one that is shared. With
this
> change, the clients don't need to manage the GpuMemoryBufferImpl instances
> themselves; ClientGMBManager takes care of that. The content/mus-specific code
> would simply be responsible for getting the shm handles.
> 
> There isn't really any management going on in the ClientGpuMemoryBufferManager
> case. It's just forwarding allocation requests to a manager in a more
privilege
> process and that's the part that we need to be different anyhow.

Ah. It sounds like you are suggesting the callers of
GpuMemoryBufferManager::AllocateGpuMemoryBuffer() should just directly create
the GpuMemoryBufferImpl using some static function in GpuMemoryBufferImpl. Is
that correct? We could inject some static factory/delegate into
GpuMemoryBufferImpl (where the factory is responsible for sending the allocation
requests) so that all the callers wouldn't need to know about it. One difficulty
here is that that would introduce a dependency on //gpu/ipc/client to some
components that currently do not have that dependency (//cc, for example).

> 
> > 
> > I am not sure we can turn GpuMemoryBufferImpl itself into a GMBManager?
> 
> The idea is to do something similar to GpuMemoryBufferImpl::CreateFromHandle.
> Add static functions to GMBImpl so the logic can easily be shared between
> different manager impls. However, I'm still failing to see what exact logic
> we're able to share thanks to this patch. It just looks like we're adding
> another layer of indirection through the delegate interface but maybe I'm
> missing something.

[reveman, 2016-11-09 03:56:23]

Let's take a step back. I think there might be a misunderstanding here. Why do
we need a common client gmb manager? I assumed it was to avoid code duplication.
Is there another reason?

[commit-bot: I haz the power, 2016-11-09 04:06:43]

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

[commit-bot: I haz the power, 2016-11-09 04:06:44]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-11-09 04:58:24]

On 2016/11/09 03:56:23, reveman wrote:
> Let's take a step back. I think there might be a misunderstanding here. Why do
> we need a common client gmb manager? I assumed it was to avoid code
duplication.
> Is there another reason?

Nope, that is the only reason. Admittedly, the duplicated code is small enough
(and some of it can probably be removed:
https://codereview.chromium.org/2487783003/) that this probably doesn't make
things much better.

[sadrul, 2016-12-09 04:33:25]

Abandoning this in favour of https://codereview.chromium.org/2514923002/