content: Use type-parameterized tests for GpuMemoryBuffer implementations.

content: Use type-parameterized tests for GpuMemoryBuffer implementations.

This makes it easier to add type specific tests and cleans up a lot
of code as a factory implementation is not longer needed for
shared memory.

BUG=538325
Committed: https://crrev.com/fb8c8e10d21c0f784084fd043c4179ff94457d9b
Cr-Commit-Position: refs/heads/master@{#352930}

[reveman, 2015-10-06 22:56:19]

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

[reveman, 2015-10-06 22:56:20]

Some refactoring and cleanup of GpuMemoryBuffer code in content/. This makes
some follow up changes related to importing of foreign GMBs easier implement and
test. Please take a look.

[piman, 2015-10-07 01:20:17]

LGTM + a bunch of nits.

https://codereview.chromium.org/1389133002/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/1389133002/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_memory_buffer_manager.h:19: using
GpuMemoryBufferConfigurationSet = std::set<GpuMemoryBufferConfiguration>;
nit: hash_set?

https://codereview.chromium.org/1389133002/diff/1/content/common/gpu/client/g...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1389133002/diff/1/content/common/gpu/client/g...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:17:
native_pixmap = nullptr;
nit: that doesn't really do anything.

https://codereview.chromium.org/1389133002/diff/1/content/common/gpu/client/g...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory_unittest.cc
(right):

https://codereview.chromium.org/1389133002/diff/1/content/common/gpu/client/g...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory_unittest.cc:38:
EXPECT_TRUE(destroyed);
nit: ASSERT_TRUE. If not true, the callback has a reference to a dangling stack
object, you definitely want to abort at that point.

https://codereview.chromium.org/1389133002/diff/1/content/test/gpu_memory_buf...
File content/test/gpu_memory_buffer_impl_test_template.h (right):

https://codereview.chromium.org/1389133002/diff/1/content/test/gpu_memory_buf...
content/test/gpu_memory_buffer_impl_test_template.h:67: EXPECT_TRUE(destroyed);
nit: ASSERT_TRUE

https://codereview.chromium.org/1389133002/diff/1/content/test/gpu_memory_buf...
content/test/gpu_memory_buffer_impl_test_template.h:106:
EXPECT_GT(row_size_in_bytes, 0u);
Consider adding EXPECT_GT(strides[plane], row_size_in_bytes);

https://codereview.chromium.org/1389133002/diff/1/ui/ozone/platform/drm/commo...
File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1389133002/diff/1/ui/ozone/platform/drm/commo...
ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: return
false;
nit: return format == gfx::BufferFormat::BGRA_8888 || format ==
gfx::BufferFormat::BGRX_8888;

That seems simpler and no less maintainable.

https://codereview.chromium.org/1389133002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface_unittest.cc
(right):

https://codereview.chromium.org/1389133002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface_unittest.cc:13:
GpuMemoryBufferImplIOSurface);
nit: I was under the impression that you could use INSTANTIATE_TYPED_TEST_CASE_P
with a list of types to instantiate with, so you could list all types in a
single place without having to create new files for every new type.
I don't feel strongly, just wanted to point out.

[reveman, 2015-10-07 09:05:00]

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

[reveman, 2015-10-07 09:05:01]

+spang for ui/ozone

https://codereview.chromium.org/1389133002/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/1389133002/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_memory_buffer_manager.h:19: using
GpuMemoryBufferConfigurationSet = std::set<GpuMemoryBufferConfiguration>;
On 2015/10/07 at 01:20:17, piman (slow to review) wrote:
> nit: hash_set?

Done.

https://codereview.chromium.org/1389133002/diff/1/content/common/gpu/client/g...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1389133002/diff/1/content/common/gpu/client/g...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:17:
native_pixmap = nullptr;
On 2015/10/07 at 01:20:17, piman (slow to review) wrote:
> nit: that doesn't really do anything.

Replaced it with a comment.

https://codereview.chromium.org/1389133002/diff/1/content/common/gpu/client/g...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory_unittest.cc
(right):

https://codereview.chromium.org/1389133002/diff/1/content/common/gpu/client/g...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory_unittest.cc:38:
EXPECT_TRUE(destroyed);
On 2015/10/07 at 01:20:17, piman (slow to review) wrote:
> nit: ASSERT_TRUE. If not true, the callback has a reference to a dangling
stack object, you definitely want to abort at that point.

Done.

https://codereview.chromium.org/1389133002/diff/1/content/test/gpu_memory_buf...
File content/test/gpu_memory_buffer_impl_test_template.h (right):

https://codereview.chromium.org/1389133002/diff/1/content/test/gpu_memory_buf...
content/test/gpu_memory_buffer_impl_test_template.h:67: EXPECT_TRUE(destroyed);
On 2015/10/07 at 01:20:17, piman (slow to review) wrote:
> nit: ASSERT_TRUE

Done.

https://codereview.chromium.org/1389133002/diff/1/content/test/gpu_memory_buf...
content/test/gpu_memory_buffer_impl_test_template.h:106:
EXPECT_GT(row_size_in_bytes, 0u);
On 2015/10/07 at 01:20:17, piman (slow to review) wrote:
> Consider adding EXPECT_GT(strides[plane], row_size_in_bytes);

I'd rather not as that would break support for negative stride.

https://codereview.chromium.org/1389133002/diff/1/ui/ozone/platform/drm/commo...
File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right):

https://codereview.chromium.org/1389133002/diff/1/ui/ozone/platform/drm/commo...
ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:57: return
false;
On 2015/10/07 at 01:20:17, piman (slow to review) wrote:
> nit: return format == gfx::BufferFormat::BGRA_8888 || format ==
gfx::BufferFormat::BGRX_8888;
> 
> That seems simpler and no less maintainable.

Done here and in places where I had a similar pattern.

https://codereview.chromium.org/1389133002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface_unittest.cc
(right):

https://codereview.chromium.org/1389133002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface_unittest.cc:13:
GpuMemoryBufferImplIOSurface);
On 2015/10/07 at 01:20:17, piman (slow to review) wrote:
> nit: I was under the impression that you could use
INSTANTIATE_TYPED_TEST_CASE_P with a list of types to instantiate with, so you
could list all types in a single place without having to create new files for
every new type.
> I don't feel strongly, just wanted to point out.

I'm aware of that but not 100% sure which way I prefer to do this. Keeping a
separate file is nice as it means all GpuMemoryBufferImplSharedMemory/* tests
can be found in one file and you can add a new impl that can be tested without
touching any existing files but it does add a few extra files right now. I'm
happy to change this in a follow up but will keep the separate file pattern for
now as I'd like to change the GLImage test too in that case.

[reveman, 2015-10-07 09:05:09]

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

[commit-bot: I haz the power, 2015-10-07 09:05:58]

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

[commit-bot: I haz the power, 2015-10-07 09:19:57]

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

[commit-bot: I haz the power, 2015-10-07 09:19:57]

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

[reveman, 2015-10-07 09:51:06]

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

[reveman, 2015-10-07 09:51:06]

+sky for ui/gfx

[reveman, 2015-10-07 09:51:17]

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

[commit-bot: I haz the power, 2015-10-07 09:51:57]

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

[commit-bot: I haz the power, 2015-10-07 12:17:17]

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

[commit-bot: I haz the power, 2015-10-07 12:17:18]

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

[reveman, 2015-10-07 12:33:23]

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

[commit-bot: I haz the power, 2015-10-07 12:34:02]

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

[dshwang, 2015-10-07 14:58:59]

On 2015/10/07 09:05:01, reveman wrote:
> +spang for ui/ozone

lgtm ui/ozone/ as non-owner

[reveman, 2015-10-07 15:06:18]

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

[commit-bot: I haz the power, 2015-10-07 15:07:26]

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

[spang, 2015-10-07 15:39:17]

lgtm

[commit-bot: I haz the power, 2015-10-07 16:19:15]

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

[commit-bot: I haz the power, 2015-10-07 16:19:16]

Dry run: This issue passed the CQ dry run.

[sky, 2015-10-07 17:35:13]

https://codereview.chromium.org/1389133002/diff/100001/ui/gfx/OWNERS
File ui/gfx/OWNERS (right):

https://codereview.chromium.org/1389133002/diff/100001/ui/gfx/OWNERS#newcode23
ui/gfx/OWNERS:23: per-file buffer*=reveman@chromium.org
I'm all for more owners, but AFAICT this is your first patch to
buffer_format_util.cc. Am I only seeing that because
https://chromium.googlesource.com/chromium/src/+log/master/ui/gfx/buffer_form...
isn't showing history before the move?

https://codereview.chromium.org/1389133002/diff/100001/ui/gfx/buffer_format_u...
File ui/gfx/buffer_format_util.cc (right):

https://codereview.chromium.org/1389133002/diff/100001/ui/gfx/buffer_format_u...
ui/gfx/buffer_format_util.cc:21: static_assert(arraysize(kBufferFormats) ==
nit: a newline would be nice between 20 and 21.

[reveman, 2015-10-07 18:12:56]

reveman@chromium.org changed reviewers:
+ dongseong.hwang@intel.com

[reveman, 2015-10-07 18:12:57]

https://codereview.chromium.org/1389133002/diff/100001/ui/gfx/OWNERS
File ui/gfx/OWNERS (right):

https://codereview.chromium.org/1389133002/diff/100001/ui/gfx/OWNERS#newcode23
ui/gfx/OWNERS:23: per-file buffer*=reveman@chromium.org
On 2015/10/07 at 17:35:13, sky wrote:
> I'm all for more owners, but AFAICT this is your first patch to
buffer_format_util.cc. Am I only seeing that because
https://chromium.googlesource.com/chromium/src/+log/master/ui/gfx/buffer_form...
isn't showing history before the move?

That code used to be in gpu_memory_buffer.h + other files that I'm the owner of
but as part of some refactoring [1] done earlier it was moved to a separate file
so it can be shared instead of duplicated between components. I forgot to ask
that I was kept as owner back then.

[1] https://codereview.chromium.org/1281043006

https://codereview.chromium.org/1389133002/diff/100001/ui/gfx/buffer_format_u...
File ui/gfx/buffer_format_util.cc (right):

https://codereview.chromium.org/1389133002/diff/100001/ui/gfx/buffer_format_u...
ui/gfx/buffer_format_util.cc:21: static_assert(arraysize(kBufferFormats) ==
On 2015/10/07 at 17:35:13, sky wrote:
> nit: a newline would be nice between 20 and 21.

Done.

[reveman, 2015-10-07 18:13:01]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-07 18:13:01]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
dongseong.hwang@intel.com, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1389133002/#ps120001
(title: "add blankline")

[reveman, 2015-10-07 18:13:03]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2015-10-07 18:13:09]

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

[commit-bot: I haz the power, 2015-10-07 18:15:08]

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

[sky, 2015-10-07 18:23:02]

Ok, LGTM

[commit-bot: I haz the power, 2015-10-07 19:40:16]

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

[commit-bot: I haz the power, 2015-10-07 19:40:17]

Dry run: This issue passed the CQ dry run.

[reveman, 2015-10-07 20:11:14]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-10-07 20:14:16]

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

[commit-bot: I haz the power, 2015-10-07 20:26:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-07 20:28:04]

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