Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the
native GPU buffer type and to check whether a provided
buffer is supported. Depending on GpuMemoryBufferFactory
ends up pulling in other service-only bits.

This CL addresses this issue. The two static methods
accessed by clients are pulled into a new class:
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/c27742238e78f1e2f7f6fe37f101eb0d07483416
Cr-Commit-Position: refs/heads/master@{#382987}

[Fady Samuel, 2016-03-23 18:23:31]

Description was changed from

==========
[WIP] GpuMemoryBufferSupport

BUG=
==========

to

==========
[WIP] GpuMemoryBufferSupport

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Fady Samuel, 2016-03-23 18:37:24]

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

[commit-bot: I haz the power, 2016-03-23 18:37:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831513003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831513003/20001

[commit-bot: I haz the power, 2016-03-23 18:47:36]

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

[commit-bot: I haz the power, 2016-03-23 18:47:37]

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

[Fady Samuel, 2016-03-23 19:24:01]

Description was changed from

==========
[WIP] GpuMemoryBufferSupport

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the 
native GPU buffer type and to check whether a provided 
buffer is supported. Depending on GpuMemoryBufferFactory 
ends up pulling in other service-only dependencies.

This CL addresses this issue. The two static methods 
accessed by clients is pulled into a new class: 
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Fady Samuel, 2016-03-23 19:24:09]

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

[Fady Samuel, 2016-03-23 19:24:19]

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

[commit-bot: I haz the power, 2016-03-23 19:24:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831513003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831513003/40001

[Fady Samuel, 2016-03-23 19:24:42]

PTAL David!

[reveman, 2016-03-23 20:00:43]

https://codereview.chromium.org/1831513003/diff/40001/gpu/ipc/common/gpu_memo...
File gpu/ipc/common/gpu_memory_buffer_support.h (right):

https://codereview.chromium.org/1831513003/diff/40001/gpu/ipc/common/gpu_memo...
gpu/ipc/common/gpu_memory_buffer_support.h:24: // We shouldn't be instantiating
this class.
why bother with a class at all then? remove the class and rename functions to:

static gfx::GpuMemoryBufferType GetNativeGpuMemoryBufferType();
static bool IsNativeGpuMemoryBufferConfigurationSupported(...);

?

[Fady Samuel, 2016-03-23 20:26:08]

PTAL David!

https://codereview.chromium.org/1831513003/diff/40001/gpu/ipc/common/gpu_memo...
File gpu/ipc/common/gpu_memory_buffer_support.h (right):

https://codereview.chromium.org/1831513003/diff/40001/gpu/ipc/common/gpu_memo...
gpu/ipc/common/gpu_memory_buffer_support.h:24: // We shouldn't be instantiating
this class.
On 2016/03/23 20:00:43, reveman wrote:
> why bother with a class at all then? remove the class and rename functions to:
> 
> static gfx::GpuMemoryBufferType GetNativeGpuMemoryBufferType();
> static bool IsNativeGpuMemoryBufferConfigurationSupported(...);
> 
> ?

Done.

[Fady Samuel, 2016-03-23 20:26:14]

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

[commit-bot: I haz the power, 2016-03-23 20:26:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831513003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831513003/60001

[Fady Samuel, 2016-03-23 20:51:55]

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

[Fady Samuel, 2016-03-23 20:51:56]

+piman@ for OWNERS once reveman@ is happy. Thanks!

[piman, 2016-03-23 21:01:39]

lgtm

https://codereview.chromium.org/1831513003/diff/60001/gpu/gpu_ipc_common.gypi
File gpu/gpu_ipc_common.gypi (right):

https://codereview.chromium.org/1831513003/diff/60001/gpu/gpu_ipc_common.gypi...
gpu/gpu_ipc_common.gypi:48: '../ui/ozone/ozone.gyp:ozone_platform',
Chalk one up for annoying dependencies. This one pulls skia too, and a bunch of
other things. I don't want to hold this CL, but we'll need to clean this too,
somehow.

native GpuMemoryBuffers won't be supported in NaCl, at least in the short term,
or possibly ever, so maybe we can just ifdef-out that dependency when compiling
the client-side for nacl.

[commit-bot: I haz the power, 2016-03-23 22:15:42]

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

[commit-bot: I haz the power, 2016-03-23 22:15:43]

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

[Fady Samuel, 2016-03-23 23:03:59]

On 2016/03/23 21:01:39, piman wrote:
> lgtm
> 
> https://codereview.chromium.org/1831513003/diff/60001/gpu/gpu_ipc_common.gypi
> File gpu/gpu_ipc_common.gypi (right):
> 
>
https://codereview.chromium.org/1831513003/diff/60001/gpu/gpu_ipc_common.gypi...
> gpu/gpu_ipc_common.gypi:48: '../ui/ozone/ozone.gyp:ozone_platform',
> Chalk one up for annoying dependencies. This one pulls skia too, and a bunch
of
> other things. I don't want to hold this CL, but we'll need to clean this too,
> somehow.
> 
> native GpuMemoryBuffers won't be supported in NaCl, at least in the short
term,
> or possibly ever, so maybe we can just ifdef-out that dependency when
compiling
> the client-side for nacl.

Filed a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=597438 We
will fix this :-)

[reveman, 2016-03-23 23:04:29]

lgtm

[Fady Samuel, 2016-03-23 23:04:33]

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

[Fady Samuel, 2016-03-23 23:04:34]

+dcheng@ for gpu/ipc/common OWNER

[Fady Samuel, 2016-03-23 23:16:57]

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

[commit-bot: I haz the power, 2016-03-23 23:17:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831513003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831513003/80001

[Fady Samuel, 2016-03-23 23:21:02]

Description was changed from

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the 
native GPU buffer type and to check whether a provided 
buffer is supported. Depending on GpuMemoryBufferFactory 
ends up pulling in other service-only dependencies.

This CL addresses this issue. The two static methods 
accessed by clients is pulled into a new class: 
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the 
native GPU buffer type and to check whether a provided 
buffer is supported. Depending on GpuMemoryBufferFactory 
ends up pulling in other service-only bits.

This CL addresses this issue. The two static methods 
accessed by clients is pulled into a new class: 
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Fady Samuel, 2016-03-23 23:21:27]

Description was changed from

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the 
native GPU buffer type and to check whether a provided 
buffer is supported. Depending on GpuMemoryBufferFactory 
ends up pulling in other service-only bits.

This CL addresses this issue. The two static methods 
accessed by clients is pulled into a new class: 
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the 
native GPU buffer type and to check whether a provided 
buffer is supported. Depending on GpuMemoryBufferFactory 
ends up pulling in other service-only bits.

This CL addresses this issue. The two static methods 
accessed by clients are pulled into a new class: 
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[dcheng, 2016-03-23 23:42:12]

lgtm with nits

https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
File gpu/ipc/common/gpu_memory_buffer_support.cc (right):

https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
gpu/ipc/common/gpu_memory_buffer_support.cc:20: #if defined(OS_ANDROID)
elif? Or are these not mutually exclusive?

https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
gpu/ipc/common/gpu_memory_buffer_support.cc:46: #endif
Similar comment here about #elif

[Fady Samuel, 2016-03-23 23:49:54]

CQ'ing. Thanks.

https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
File gpu/ipc/common/gpu_memory_buffer_support.cc (right):

https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
gpu/ipc/common/gpu_memory_buffer_support.cc:20: #if defined(OS_ANDROID)
On 2016/03/23 23:42:12, dcheng wrote:
> elif? Or are these not mutually exclusive?

They are mutually exclusive. Done.

https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
gpu/ipc/common/gpu_memory_buffer_support.cc:46: #endif
On 2016/03/23 23:42:12, dcheng wrote:
> Similar comment here about #elif

Done.

[Fady Samuel, 2016-03-23 23:50:05]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-03-23 23:50:06]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
reveman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1831513003/#ps100001
(title: "Addressed dcheng's nit")

[commit-bot: I haz the power, 2016-03-23 23:50:24]

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

[Fady Samuel, 2016-03-23 23:50:53]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-03-23 23:51:45]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Fady Samuel, 2016-03-23 23:51:49]

On 2016/03/23 23:49:54, Fady Samuel wrote:
> CQ'ing. Thanks.
> 
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> File gpu/ipc/common/gpu_memory_buffer_support.cc (right):
> 
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> gpu/ipc/common/gpu_memory_buffer_support.cc:20: #if defined(OS_ANDROID)
> On 2016/03/23 23:42:12, dcheng wrote:
> > elif? Or are these not mutually exclusive?
> 
> They are mutually exclusive. Done.
> 
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> gpu/ipc/common/gpu_memory_buffer_support.cc:46: #endif
> On 2016/03/23 23:42:12, dcheng wrote:
> > Similar comment here about #elif
> 
> Done.

Actually I take that bad. rjkroege@ suggested that this is a bad assumption to
make. OZONE can be made generic to run on any platform so no point in baking the
assumption in here.

[Fady Samuel, 2016-03-23 23:52:07]

On 2016/03/23 23:51:49, Fady Samuel wrote:
> On 2016/03/23 23:49:54, Fady Samuel wrote:
> > CQ'ing. Thanks.
> > 
> >
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > File gpu/ipc/common/gpu_memory_buffer_support.cc (right):
> > 
> >
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > gpu/ipc/common/gpu_memory_buffer_support.cc:20: #if defined(OS_ANDROID)
> > On 2016/03/23 23:42:12, dcheng wrote:
> > > elif? Or are these not mutually exclusive?
> > 
> > They are mutually exclusive. Done.
> > 
> >
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > gpu/ipc/common/gpu_memory_buffer_support.cc:46: #endif
> > On 2016/03/23 23:42:12, dcheng wrote:
> > > Similar comment here about #elif
> > 
> > Done.
> 
> Actually I take that bad. rjkroege@ suggested that this is a bad assumption to
> make. OZONE can be made generic to run on any platform so no point in baking
the
> assumption in here.

*take that back.

[dcheng, 2016-03-24 00:11:44]

On 2016/03/23 at 23:52:07, fsamuel wrote:
> On 2016/03/23 23:51:49, Fady Samuel wrote:
> > On 2016/03/23 23:49:54, Fady Samuel wrote:
> > > CQ'ing. Thanks.
> > > 
> > >
> >
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > > File gpu/ipc/common/gpu_memory_buffer_support.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > > gpu/ipc/common/gpu_memory_buffer_support.cc:20: #if defined(OS_ANDROID)
> > > On 2016/03/23 23:42:12, dcheng wrote:
> > > > elif? Or are these not mutually exclusive?
> > > 
> > > They are mutually exclusive. Done.
> > > 
> > >
> >
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > > gpu/ipc/common/gpu_memory_buffer_support.cc:46: #endif
> > > On 2016/03/23 23:42:12, dcheng wrote:
> > > > Similar comment here about #elif
> > > 
> > > Done.
> > 
> > Actually I take that bad. rjkroege@ suggested that this is a bad assumption
to
> > make. OZONE can be made generic to run on any platform so no point in baking
the
> > assumption in here.
> 
> *take that back.

As written, we'll never reach the Ozone bits if any of the other platform
defines are set though.

[commit-bot: I haz the power, 2016-03-24 00:27:20]

Description was changed from

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the 
native GPU buffer type and to check whether a provided 
buffer is supported. Depending on GpuMemoryBufferFactory 
ends up pulling in other service-only bits.

This CL addresses this issue. The two static methods 
accessed by clients are pulled into a new class: 
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the 
native GPU buffer type and to check whether a provided 
buffer is supported. Depending on GpuMemoryBufferFactory 
ends up pulling in other service-only bits.

This CL addresses this issue. The two static methods 
accessed by clients are pulled into a new class: 
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-03-24 00:27:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-24 00:28:36]

Description was changed from

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the 
native GPU buffer type and to check whether a provided 
buffer is supported. Depending on GpuMemoryBufferFactory 
ends up pulling in other service-only bits.

This CL addresses this issue. The two static methods 
accessed by clients are pulled into a new class: 
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Pull gpu service/client shared memory buffer code to GpuMemoryBufferSupport

GpuMemoryBufferFactory is a gpu service concept and should
not be accessed from the client.

Client code accessed GpuMemoryBufferFactory to get the
native GPU buffer type and to check whether a provided
buffer is supported. Depending on GpuMemoryBufferFactory
ends up pulling in other service-only bits.

This CL addresses this issue. The two static methods
accessed by clients are pulled into a new class:
GpuMemoryBufferSupport placed in gpu/ipc/common.

BUG=597170
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/c27742238e78f1e2f7f6fe37f101eb0d07483416
Cr-Commit-Position: refs/heads/master@{#382987}
==========

[commit-bot: I haz the power, 2016-03-24 00:28:37]

Patchset 5 (id:??) landed as
https://crrev.com/c27742238e78f1e2f7f6fe37f101eb0d07483416
Cr-Commit-Position: refs/heads/master@{#382987}

[Fady Samuel, 2016-03-24 04:59:04]

On 2016/03/24 00:11:44, dcheng wrote:
> On 2016/03/23 at 23:52:07, fsamuel wrote:
> > On 2016/03/23 23:51:49, Fady Samuel wrote:
> > > On 2016/03/23 23:49:54, Fady Samuel wrote:
> > > > CQ'ing. Thanks.
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > > > File gpu/ipc/common/gpu_memory_buffer_support.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > > > gpu/ipc/common/gpu_memory_buffer_support.cc:20: #if defined(OS_ANDROID)
> > > > On 2016/03/23 23:42:12, dcheng wrote:
> > > > > elif? Or are these not mutually exclusive?
> > > > 
> > > > They are mutually exclusive. Done.
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1831513003/diff/80001/gpu/ipc/common/gpu_memo...
> > > > gpu/ipc/common/gpu_memory_buffer_support.cc:46: #endif
> > > > On 2016/03/23 23:42:12, dcheng wrote:
> > > > > Similar comment here about #elif
> > > > 
> > > > Done.
> > > 
> > > Actually I take that bad. rjkroege@ suggested that this is a bad
assumption
> to
> > > make. OZONE can be made generic to run on any platform so no point in
baking
> the
> > > assumption in here.
> > 
> > *take that back.
> 
> As written, we'll never reach the Ozone bits if any of the other platform
> defines are set though.

Either way work has to be done. I'm fairly indifferent at the moment. If you
feel strongly about this I'll add the elifs in a separate CL.