Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

This class is just a stub to hold dmabuf in order to be passed to
GLImageNativePixmap::Initialize(NativePixmap*).

This complete enablement of glCreateImageCHROMIUM with linux dma buf on linux.

BUG=584248
R=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

[Julien Isorce, 2017-02-23 14:12:56]

Description was changed from

==========
Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

This class is just a stub to hold dmabuf in order to be passed to
GLImageNativePixmap::Initialize(NativePixmap*).

This complete enablement of glCreateImageCHROMIUM with linux dma buf on linux.

BUG=584248
R=reveman@chromium.org
==========

to

==========
Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

This class is just a stub to hold dmabuf in order to be passed to
GLImageNativePixmap::Initialize(NativePixmap*).

This complete enablement of glCreateImageCHROMIUM with linux dma buf on linux.

BUG=584248
R=reveman@chromium.org
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
==========

[Julien Isorce, 2017-04-05 08:21:26]

Description was changed from

==========
Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

This class is just a stub to hold dmabuf in order to be passed to
GLImageNativePixmap::Initialize(NativePixmap*).

This complete enablement of glCreateImageCHROMIUM with linux dma buf on linux.

BUG=584248
R=reveman@chromium.org
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
==========

to

==========
Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

This class is just a stub to hold dmabuf in order to be passed to
GLImageNativePixmap::Initialize(NativePixmap*).

This complete enablement of glCreateImageCHROMIUM with linux dma buf on linux.

BUG=584248
R=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[tonikitoo, 2017-04-05 18:46:59]

Description was changed from

==========
Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

This class is just a stub to hold dmabuf in order to be passed to
GLImageNativePixmap::Initialize(NativePixmap*).

This complete enablement of glCreateImageCHROMIUM with linux dma buf on linux.

BUG=584248
R=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

to

==========
Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

This class is just a stub to hold dmabuf in order to be passed to
GLImageNativePixmap::Initialize(NativePixmap*).

This complete enablement of glCreateImageCHROMIUM with linux dma buf on linux.

BUG=584248
R=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[reveman, 2017-04-12 18:21:37]

https://codereview.chromium.org/2705213005/diff/20001/gpu/ipc/service/gpu_mem...
File gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc (right):

https://codereview.chromium.org/2705213005/diff/20001/gpu/ipc/service/gpu_mem...
gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc:106: pixmap = new
ui::NativePixmapDmaBufStub(size, format,
should we be using make_scoped_refptr here to make it more explicit that this is
ref counted?

https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/BUILD.gn#newcode496
ui/gfx/BUILD.gn:496: "//gpu/ipc/service:ipc_service_sources",
why is this needed? seems bad to have references to //gpu/ here in ui/gfx/..

https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/linux/native_pix...
File ui/gfx/linux/native_pixmap_dmabuf_stub.h (right):

https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/linux/native_pix...
ui/gfx/linux/native_pixmap_dmabuf_stub.h:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
do we need to add this class/file? Can we instead refactor
client_native_pixmap_dmabuf so we can use that on linux?

[Julien Isorce, 2017-04-13 12:11:46]

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

[commit-bot: I haz the power, 2017-04-13 12:12:03]

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, 2017-04-13 12:21:57]

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

[commit-bot: I haz the power, 2017-04-13 12:21:57]

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

[Julien Isorce, 2017-04-13 12:46:02]

Thx for the review, here are my replies:

https://codereview.chromium.org/2705213005/diff/20001/gpu/ipc/service/gpu_mem...
File gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc (right):

https://codereview.chromium.org/2705213005/diff/20001/gpu/ipc/service/gpu_mem...
gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc:106: pixmap = new
ui::NativePixmapDmaBufStub(size, format,
On 2017/04/12 18:21:36, reveman wrote:
> should we be using make_scoped_refptr here to make it more explicit that this
is
> ref counted?

Sure, I will do it.

https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/BUILD.gn#newcode496
ui/gfx/BUILD.gn:496: "//gpu/ipc/service:ipc_service_sources",
On 2017/04/12 18:21:36, reveman wrote:
> why is this needed? seems bad to have references to //gpu/ here in ui/gfx/..

I agree, this is because of
https://codereview.chromium.org/2705213005/patch/40001/50001 otherwise I get:

ERROR at //gpu/ipc/service/BUILD.gn:28:1: Dependency not allowed.
target(link_target_type, "ipc_service_sources") {
^------------------------------------------------
The item //gpu/ipc/service:ipc_service_sources
can not depend on //ui/gfx:memory_buffer_sources
because it is not in //ui/gfx:memory_buffer_sources's visibility list: [
  //ui/gfx:*
]

If I remove the change in gpu/ipc/service/BUILD.gn then gn passes but I get
undefined reference to 'gfx::NativePixmapDmaBufStub::NativePixmapDmaBufStub when
compiling for
obj/gpu/ipc/service/ipc_service_sources/gpu_memory_buffer_factory_native_pixmap.o

https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/linux/native_pix...
File ui/gfx/linux/native_pixmap_dmabuf_stub.h (right):

https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/linux/native_pix...
ui/gfx/linux/native_pixmap_dmabuf_stub.h:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/04/12 18:21:36, reveman wrote:
> do we need to add this class/file? Can we instead refactor
> client_native_pixmap_dmabuf so we can use that on linux?

Could you elaborate ? Because here this is the service side, not client so I am
not sure to follow. Thx!

Currently the implementations of gfx::NativePixmap are CastPixmap, GbmPixmap and
(Headless)TestPixmap. This new NativePixmapDmaBufStub is close to GbmPixmap
though the later is really dependent of other Gbm classes like for example
GbmBuffer, GbmSurfaceFactory, GbmDevice. 
Maybe one could later make GbmPixmap inherits this new NativePixmapDmaBufStub
but would require to make GbmBuffer generic too.

[Julien Isorce, 2017-04-13 12:54:02]

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

[commit-bot: I haz the power, 2017-04-13 12:54:18]

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, 2017-04-13 14:57:47]

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

[commit-bot: I haz the power, 2017-04-13 14:57:48]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-04-14 09:52:52]

julien.isorce@chromium.org changed reviewers:
+ spang@chromium.org

[Julien Isorce, 2017-04-14 09:59:42]

>
https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/BUILD.gn#newcode496
> ui/gfx/BUILD.gn:496: "//gpu/ipc/service:ipc_service_sources",
> On 2017/04/12 18:21:36, reveman wrote:
> > why is this needed? seems bad to have references to //gpu/ here in ui/gfx/..
> 
> I agree, this is because of
> https://codereview.chromium.org/2705213005/patch/40001/50001 otherwise I get:
> 
> ERROR at //gpu/ipc/service/BUILD.gn:28:1: Dependency not allowed.
> target(link_target_type, "ipc_service_sources") {
> ^------------------------------------------------
> The item //gpu/ipc/service:ipc_service_sources
> can not depend on //ui/gfx:memory_buffer_sources
> because it is not in //ui/gfx:memory_buffer_sources's visibility list: [
>   //ui/gfx:*
> ]
> 
> If I remove the change in gpu/ipc/service/BUILD.gn then gn passes but I get
> undefined reference to 'gfx::NativePixmapDmaBufStub::NativePixmapDmaBufStub
when
> compiling for
>
obj/gpu/ipc/service/ipc_service_sources/gpu_memory_buffer_factory_native_pixmap.o
>

Hi @spang, any idea how to avoid the wrong visibility change in ui/gfx/BUILD.gn
? See above. Thx you !

[spang1, 2017-04-18 18:59:44]

On 2017/04/14 09:59:42, Julien Isorce wrote:
> >
>
https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/BUILD.gn#newcode496
> > ui/gfx/BUILD.gn:496: "//gpu/ipc/service:ipc_service_sources",
> > On 2017/04/12 18:21:36, reveman wrote:
> > > why is this needed? seems bad to have references to //gpu/ here in
ui/gfx/..
> > 
> > I agree, this is because of
> > https://codereview.chromium.org/2705213005/patch/40001/50001 otherwise I
get:
> > 
> > ERROR at //gpu/ipc/service/BUILD.gn:28:1: Dependency not allowed.
> > target(link_target_type, "ipc_service_sources") {
> > ^------------------------------------------------
> > The item //gpu/ipc/service:ipc_service_sources
> > can not depend on //ui/gfx:memory_buffer_sources
> > because it is not in //ui/gfx:memory_buffer_sources's visibility list: [
> >   //ui/gfx:*
> > ]
> > 
> > If I remove the change in gpu/ipc/service/BUILD.gn then gn passes but I get
> > undefined reference to 'gfx::NativePixmapDmaBufStub::NativePixmapDmaBufStub
> when
> > compiling for
> >
>
obj/gpu/ipc/service/ipc_service_sources/gpu_memory_buffer_factory_native_pixmap.o
> >
> 
> Hi @spang, any idea how to avoid the wrong visibility change in
ui/gfx/BUILD.gn
> ? See above. Thx you !

The comment says to depend on "//ui/gfx:memory_buffer" from gpu/, please do
that. You should not depend directly on :memory_buffer_sources; it'd cause
issues for the component build.

[Julien Isorce, 2017-04-19 10:15:40]

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

[commit-bot: I haz the power, 2017-04-19 10:16:15]

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, 2017-04-19 12:04:06]

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

[commit-bot: I haz the power, 2017-04-19 12:04:08]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-04-19 12:06:23]

On 2017/04/18 18:59:44, spang1 wrote:
> On 2017/04/14 09:59:42, Julien Isorce wrote:
> > 
> > Hi @spang, any idea how to avoid the wrong visibility change in
> ui/gfx/BUILD.gn
> > ? See above. Thx you !
> 
> The comment says to depend on "//ui/gfx:memory_buffer" from gpu/, please do
> that. You should not depend directly on :memory_buffer_sources; it'd cause
> issues for the component build.

Oh great thx ! I was also missing GFX_EXPORT in native_pixmap_dmabuf_stub.h

[Julien Isorce, 2017-04-19 12:07:56]

Please take a look to patch set 5, thx!

[spang, 2017-04-21 15:11:49]

lgtm

https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pix...
File ui/gfx/linux/native_pixmap_dmabuf_stub.cc (right):

https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pix...
ui/gfx/linux/native_pixmap_dmabuf_stub.cc:79: {
Wrong whitespace, please run clang-format / git cl format.

https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pix...
File ui/gfx/linux/native_pixmap_dmabuf_stub.h (right):

https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pix...
ui/gfx/linux/native_pixmap_dmabuf_stub.h:20: class GFX_EXPORT
NativePixmapDmaBufStub : public gfx::NativePixmap {
Why is it called stub?

It holds the actual dmabufs right? Why not just NativePixmapDmabuf ?

[Julien Isorce, 2017-04-24 12:26:00]

Thx for the review. Remarks addressed in Patch Set 6.

https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pix...
File ui/gfx/linux/native_pixmap_dmabuf_stub.cc (right):

https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pix...
ui/gfx/linux/native_pixmap_dmabuf_stub.cc:79: {
On 2017/04/21 15:11:49, spang wrote:
> Wrong whitespace, please run clang-format / git cl format.

Done.

https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pix...
File ui/gfx/linux/native_pixmap_dmabuf_stub.h (right):

https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pix...
ui/gfx/linux/native_pixmap_dmabuf_stub.h:20: class GFX_EXPORT
NativePixmapDmaBufStub : public gfx::NativePixmap {
On 2017/04/21 15:11:49, spang wrote:
> Why is it called stub?
> 
> It holds the actual dmabufs right? Why not just NativePixmapDmabuf ?

I agree the stub suffix is not valuable.

[Julien Isorce, 2017-04-24 12:26:12]

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

[commit-bot: I haz the power, 2017-04-24 12:26: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, 2017-04-24 14:59:05]

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

[commit-bot: I haz the power, 2017-04-24 14:59:06]

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

[Julien Isorce, 2017-04-25 07:27:31]

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

[commit-bot: I haz the power, 2017-04-25 07:27:47]

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, 2017-04-25 09:15:30]

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

[commit-bot: I haz the power, 2017-04-25 09:15:32]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-04-27 13:49:43]

Hi @reveman, please take a look. Thx!

[Julien Isorce, 2017-05-11 08:57:59]

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

[commit-bot: I haz the power, 2017-05-11 08:58:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julien Isorce, 2017-05-11 08:59:07]

julien.isorce@chromium.org changed reviewers:
+ sadrul@chromium.org

[Julien Isorce, 2017-05-11 09:00:22]

Hi sadrul, PTAL at ui/gfx/*. Thank you!

[commit-bot: I haz the power, 2017-05-11 11:37:17]

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

[commit-bot: I haz the power, 2017-05-11 11:37:18]

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

[sadrul, 2017-05-12 17:10:20]

On 2017/05/11 09:00:22, Julien Isorce wrote:
> Hi sadrul, PTAL at ui/gfx/*. Thank you!

reveman@ should review.

[Julien Isorce, 2017-05-18 15:01:40]

On 2017/05/12 17:10:20, sadrul wrote:
> On 2017/05/11 09:00:22, Julien Isorce wrote:
> > Hi sadrul, PTAL at ui/gfx/*. Thank you!
> 
> reveman@ should review.

Hi David, gentle ping for review. Thx.

[Julien Isorce, 2017-05-19 07:55:39]

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

[commit-bot: I haz the power, 2017-05-19 07:56:15]

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, 2017-05-19 08:49:35]

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

[commit-bot: I haz the power, 2017-05-19 08:49:37]

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_...)

[Julien Isorce, 2017-05-19 14:13:22]

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

[commit-bot: I haz the power, 2017-05-19 14:13:43]

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, 2017-05-19 15:49:09]

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

[commit-bot: I haz the power, 2017-05-19 15:49:10]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-06-01 07:26:41]

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

[commit-bot: I haz the power, 2017-06-01 07:26: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, 2017-06-01 08:11:00]

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

[commit-bot: I haz the power, 2017-06-01 08:11:01]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Julien Isorce, 2017-06-01 12:16:37]

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

[commit-bot: I haz the power, 2017-06-01 12:16:53]

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, 2017-06-01 13:44:07]

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

[commit-bot: I haz the power, 2017-06-01 13:44:08]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-06-05 14:49:07]

Hi @reveman, gentle ping to review this CL. Thx.

[reveman, 2017-06-05 17:44:42]

https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pi...
File ui/gfx/linux/native_pixmap_dmabuf.h (right):

https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pi...
ui/gfx/linux/native_pixmap_dmabuf.h:1: // Copyright 2017 The Chromium Authors.
All rights reserved.
Can we share this code with ozone somehow? Maybe ozone impl can inherit from
this?

[Julien Isorce, 2017-06-05 20:23:58]

https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pi...
File ui/gfx/linux/native_pixmap_dmabuf.h (right):

https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pi...
ui/gfx/linux/native_pixmap_dmabuf.h:1: // Copyright 2017 The Chromium Authors.
All rights reserved.
On 2017/06/05 17:44:42, reveman wrote:
> Can we share this code with ozone somehow? Maybe ozone impl can inherit from
> this?

Thx for the suggestions. I just had another look and it might possible to do it
in a less intrusive way than I thought initially. By re-using
gfx::NativePixmapHandle 

(https://chromium.googlesource.com/chromium/src.git/+/master/ui/gfx/native_pix...).
The new gfx::NativePixmapDmabuf will own a gfx::NativePixmapHandle.
And ozone::GbmBuffer will inherit from gfx::NativePixmapHandle
(https://chromium.googlesource.com/chromium/src.git/+/master/ui/ozone/platform...)
Which will make possible for ozone::GbmPixmap to inherit from the new
gfx::NativePixmapDmabuf.
Something like that, I need to try and see how it goes.

[Julien Isorce, 2017-06-06 16:38:06]

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

[commit-bot: I haz the power, 2017-06-06 16:38:19]

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, 2017-06-06 17:01:37]

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

[commit-bot: I haz the power, 2017-06-06 17:01:38]

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

[Julien Isorce, 2017-06-07 07:15:08]

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

[commit-bot: I haz the power, 2017-06-07 07:15:24]

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, 2017-06-07 07:54:38]

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

[commit-bot: I haz the power, 2017-06-07 07:54:39]

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

[Julien Isorce, 2017-06-07 09:55:34]

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

[commit-bot: I haz the power, 2017-06-07 09:55:42]

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, 2017-06-07 12:08:37]

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

[commit-bot: I haz the power, 2017-06-07 12:08:39]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-06-07 13:55:12]

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

[commit-bot: I haz the power, 2017-06-07 13:55: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, 2017-06-07 14:05:54]

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

[commit-bot: I haz the power, 2017-06-07 14:05:56]

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

[Julien Isorce, 2017-06-07 15:06:59]

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

[commit-bot: I haz the power, 2017-06-07 15:07:21]

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, 2017-06-07 16:43:52]

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

[commit-bot: I haz the power, 2017-06-07 16:43:53]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-06-07 20:10:49]

https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pi...
File ui/gfx/linux/native_pixmap_dmabuf.h (right):

https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pi...
ui/gfx/linux/native_pixmap_dmabuf.h:1: // Copyright 2017 The Chromium Authors.
All rights reserved.
On 2017/06/05 17:44:42, reveman wrote:
> Can we share this code with ozone somehow? Maybe ozone impl can inherit from
> this?

Done.

[reveman, 2017-06-07 21:02:37]

https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pi...
File ui/gfx/linux/native_pixmap_dmabuf.h (right):

https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pi...
ui/gfx/linux/native_pixmap_dmabuf.h:21: class GFX_EXPORT
NativePixmapDmabufHelper
why a helper instead of NativePixmapDmabufBase that we derive from or just
include all functionality in NativePixmapDmabuf and have GbmBuffer just derive
from that?

https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pi...
ui/gfx/linux/native_pixmap_dmabuf.h:22: : public
base::RefCountedThreadSafe<NativePixmapDmabufHelper> {
why does this need to be thread-safe refcounted?

[Julien Isorce, 2017-06-07 21:29:53]

https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pi...
File ui/gfx/linux/native_pixmap_dmabuf.h (right):

https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pi...
ui/gfx/linux/native_pixmap_dmabuf.h:21: class GFX_EXPORT
NativePixmapDmabufHelper
On 2017/06/07 21:02:37, reveman wrote:
> why a helper instead of NativePixmapDmabufBase that we derive from or just
> include all functionality in NativePixmapDmabuf and have GbmBuffer just derive
> from that?

I did something like that in Patch Set 13. I will come back to this solution, I
think I now see how to make it nicer.

https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pi...
ui/gfx/linux/native_pixmap_dmabuf.h:22: : public
base::RefCountedThreadSafe<NativePixmapDmabufHelper> {
On 2017/06/07 21:02:37, reveman wrote:
> why does this need to be thread-safe refcounted?

By symmetry with scoped_refptr<GbmBuffer> in GbmPixmap. It is not present in the
other solution.

[Julien Isorce, 2017-06-08 09:49:54]

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

[commit-bot: I haz the power, 2017-06-08 09:50:03]

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, 2017-06-08 10:56:24]

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

[commit-bot: I haz the power, 2017-06-08 10:56:25]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-06-08 11:07:33]

On 2017/06/07 21:02:37, reveman wrote:
> why a helper instead of NativePixmapDmabufBase that we derive from or just
> include all functionality in NativePixmapDmabuf and have GbmBuffer just derive
> from that?

So I uploaded back the previous version (Patch Set 13) into Patch Set 18. 
But I do not see how to get ride of this horrible:

  std::unique_ptr<NativePixmapDmabufHelper> holder_;
  const NativePixmapDmabufHelper& helper_;

Any idea ?

[reveman, 2017-06-08 13:40:32]

https://codereview.chromium.org/2705213005/diff/310002/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):

https://codereview.chromium.org/2705213005/diff/310002/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/gbm_buffer.h:24: class GbmBuffer : public
GbmBufferBase, public gfx::NativePixmapDmabufHelper {
why do we need to inherit this here? can't we just remove most of the functions
now that gfx::NativePixmapDmaBuf implement them?

[Julien Isorce, 2017-06-08 14:01:31]

On 2017/06/08 13:40:32, reveman wrote:
>
https://codereview.chromium.org/2705213005/diff/310002/ui/ozone/platform/drm/...
> ui/ozone/platform/drm/gpu/gbm_buffer.h:24: class GbmBuffer : public
> GbmBufferBase, public gfx::NativePixmapDmabufHelper {
> why do we need to inherit this here? can't we just remove most of the
functions
> now that gfx::NativePixmapDmaBuf implement them?

Hi David, thx for your comments. But I am a bit lost, it looks to me that
yesterday you said the opposite :) ?
"why a helper instead of NativePixmapDmabufBase that we derive from"
"why do we need to inherit this here? "
I guess I have miss understood your point so let me clarify:

Prior this CL here is the existing design:

DrmBuffer inherits DrmBufferBase which inherits ozone::Refcounted<ScanoutBuffer>
DrmBuffer is a scoped_refptr member of DrmPixmap which implements the interface
RefCounted<gfx::NativePixmap>

Then this CL introduces gfx::NativePixmapDmabuf which implements the interface
RefCounted<gfx::NativePixmap>.

Now making DrmPixmap inherits gfx::NativePixmapDmabuf is not straight forward to
me. It is actually all about the relation
between NativePixmapHelper and DrmBuffer.
Currently I have 2 solutions for this relation: Patch Set 16 (member) or Patch
Set 18 (parent).

[Julien Isorce, 2017-06-09 15:19:41]

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

[commit-bot: I haz the power, 2017-06-09 15:20:02]

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, 2017-06-09 16:38:51]

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

[commit-bot: I haz the power, 2017-06-09 16:38:52]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-06-16 14:16:44]

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

[commit-bot: I haz the power, 2017-06-16 14:16:55]

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, 2017-06-16 15:58:10]

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

[commit-bot: I haz the power, 2017-06-16 15:58:12]

Dry run: This issue passed the CQ dry run.

[Julien Isorce, 2017-08-21 10:23:07]

Moved to https://chromium-review.googlesource.com/c/623307