Change NativeBufferOzone to be an object and move the overlay calls to the surface object.

Change NativeBufferOzone to be an object and move the overlay calls to the surface object.

BUG=380861
NOTRY=true

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

[achaulk, 2014-07-03 18:13:32]

I'm going to remove the ozone stuff from that other review and do them
separately

[spang, 2014-07-03 18:29:09]

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
ui/ozone/public/surface_factory_ozone.h:33: virtual void* /* EGLClientBuffer */
native_handle() = 0;
If this is "get me the handle to pass to EGL" then I still think it should say
so. So egl_client_buffer or GetEGLClientBuffer().

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
ui/ozone/public/surface_factory_ozone.h:155: gfx::Size size, BufferFormat
format);
seems rather inconsistent. CreateNativePixmap

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
ui/ozone/public/surface_factory_ozone.h:160: virtual bool
CanShowPrimaryPlaneAsOverlay();
thanks

[dnicoara, 2014-07-03 18:30:17]

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
File ui/ozone/platform/dri/gbm_buffer.cc (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.cc:69: if (!dri_->AddFramebuffer(size_.width(),
Should skip this is the buffer isn't a scanout buffer.

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
File ui/ozone/platform/dri/gbm_buffer.h (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.h:21: class GbmBuffer : public NativePixmap,
public ScanoutSurface {
Do you need to inherit from ScanoutSurface? I'm assuming you need this for your
next changes (maybe leave it for those changes?)

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
ui/ozone/public/surface_factory_ozone.h:29: class NativePixmap : public
base::RefCounted<NativePixmap> {
Just move it in it's own file.

And add some comments describing what this is and how it is used.

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
ui/ozone/public/surface_factory_ozone.h:33: virtual void* /* EGLClientBuffer */
native_handle() = 0;
I believe these should follow the normal naming convention: NativeHandle()

[alexst (slow to review), 2014-07-03 18:30:32]

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
File ui/ozone/platform/dri/gbm_buffer.cc (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.cc:37: GbmBuffer::GbmBuffer(gbm_device* device,
DriWrapper* dri,
nit: these fit in 80chars

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.cc:109: //static
nit: space after //

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
File ui/ozone/platform/dri/gbm_buffer.h (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.h:25: bool
InitializeBuffer(SurfaceFactoryOzone::BufferFormat format,
nit: these fit

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_ozon...
File ui/ozone/public/surface_ozone_egl.h (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_ozon...
ui/ozone/public/surface_ozone_egl.h:58: ui::NativePixmap* buffer,
Since we are ref counting these, maybe raw ptr isn't the safest here. I suppose
whoever calls this should know not to delete this until after the following
flip, and if so it's worth documenting it.

[achaulk, 2014-07-03 19:49:44]

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
File ui/ozone/platform/dri/gbm_buffer.cc (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.cc:37: GbmBuffer::GbmBuffer(gbm_device* device,
DriWrapper* dri,
On 2014/07/03 18:30:32, alexst wrote:
> nit: these fit in 80chars

Done.

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.cc:69: if (!dri_->AddFramebuffer(size_.width(),
On 2014/07/03 18:30:16, dnicoara wrote:
> Should skip this is the buffer isn't a scanout buffer.

Done.

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.cc:109: //static
On 2014/07/03 18:30:32, alexst wrote:
> nit: space after //

Done.

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
File ui/ozone/platform/dri/gbm_buffer.h (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/platform/dri/gbm_bu...
ui/ozone/platform/dri/gbm_buffer.h:21: class GbmBuffer : public NativePixmap,
public ScanoutSurface {
On 2014/07/03 18:30:16, dnicoara wrote:
> Do you need to inherit from ScanoutSurface? I'm assuming you need this for
your
> next changes (maybe leave it for those changes?)

Oh, yeah those are part of the next set of changes when HDC uses them. I can
leave it out for now

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
ui/ozone/public/surface_factory_ozone.h:33: virtual void* /* EGLClientBuffer */
native_handle() = 0;
On 2014/07/03 18:29:09, spang wrote:
> If this is "get me the handle to pass to EGL" then I still think it should say
> so. So egl_client_buffer or GetEGLClientBuffer().

Done.

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_fact...
ui/ozone/public/surface_factory_ozone.h:155: gfx::Size size, BufferFormat
format);
On 2014/07/03 18:29:09, spang wrote:
> seems rather inconsistent. CreateNativePixmap

Done.

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_ozon...
File ui/ozone/public/surface_ozone_egl.h (right):

https://codereview.chromium.org/365193003/diff/1/ui/ozone/public/surface_ozon...
ui/ozone/public/surface_ozone_egl.h:58: ui::NativePixmap* buffer,
On 2014/07/03 18:30:32, alexst wrote:
> Since we are ref counting these, maybe raw ptr isn't the safest here. I
suppose
> whoever calls this should know not to delete this until after the following
> flip, and if so it's worth documenting it.

I was planning on taking references internally anyway, but users of the API
shouldn't really have to know or care about that. As far as they know, it's
"deleted" when their last reference is release

[alexst (slow to review), 2014-07-03 20:28:48]

> I was planning on taking references internally anyway, but users of the API
> shouldn't really have to know or care about that. As far as they know, it's
> "deleted" when their last reference is release

If you are going to take a ref internally, why not just use a refptr which makes
it clear that it's happening? User would still release the buffer the same way.

[achaulk, 2014-07-03 21:33:55]

On 2014/07/03 20:28:48, alexst wrote:
> > I was planning on taking references internally anyway, but users of the API
> > shouldn't really have to know or care about that. As far as they know, it's
> > "deleted" when their last reference is release
> 
> If you are going to take a ref internally, why not just use a refptr which
makes
> it clear that it's happening? User would still release the buffer the same
way.

I don't think having a refptr actually makes it more clear, but I don't really
care either way. Updated

[alexst (slow to review), 2014-07-04 01:31:49]

lgtm with nits.

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
ui/ozone/public/surface_factory_ozone.h:17: #include
"ui/ozone/public/native_pixmap.h"
Can we forward declare this instead?

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
ui/ozone/public/surface_factory_ozone.h:138: virtual
scoped_refptr<ui::NativePixmap> CreateNativePixmap(
nit: We are already in the ui namespace.

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
File ui/ozone/public/surface_ozone_egl.h (right):

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
ui/ozone/public/surface_ozone_egl.h:58: scoped_refptr<ui::NativePixmap> buffer,
already in ui

[spang, 2014-07-04 14:03:16]

lgtm

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
ui/ozone/public/surface_factory_ozone.h:17: #include
"ui/ozone/public/native_pixmap.h"
On 2014/07/04 01:31:49, alexst wrote:
> Can we forward declare this instead?

probably not

[achaulk, 2014-07-04 15:26:01]

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/365193003/diff/40001/ui/ozone/public/surface_...
ui/ozone/public/surface_factory_ozone.h:17: #include
"ui/ozone/public/native_pixmap.h"
On 2014/07/04 14:03:15, spang wrote:
> On 2014/07/04 01:31:49, alexst wrote:
> > Can we forward declare this instead?
> 
> probably not

I didn't think so either, but apparently it does work

[achaulk, 2014-07-04 16:06:03]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-07-04 16:06:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/365193003/60001

[commit-bot: I haz the power, 2014-07-04 16:07:26]

Change committed as 281411