ozone: unify GpuMemoryBufferFactoryOzoneNativePixmap in content/

ozone: unify GpuMemoryBufferFactoryOzoneNativePixmap in content/

ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h/cc is merged into
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h/cc now that
that class changed name and it's not supposed to manage native "buffers" anymore
but instead native "pixmaps". The reason we introduced it in the past was to
avoid exposing the "pixmap" concept to content/ but as part of giving up on that
we should move that code to content/. content/common/gpu/media/ already uses the
"pixmap" concept.

Now ui/ozone/gpu/ is not needed anymore, so remove it.

Introduce GLImageOzoneNativePixmap like GLImageIOSurface and
GLImageSurfaceTexture.

TEST=run ozone_demo
BUG=475633
Committed: https://crrev.com/161c7a23d99119c7acf7f9faf0ae794ab4d62b62
Cr-Commit-Position: refs/heads/master@{#340927}

[dshwang, 2015-07-24 18:07:22]

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

[dshwang, 2015-07-24 18:07:23]

reveman, could you review?
submit this CL, as you requested unifying
GpuMemoryBufferFactoryOzoneNativePixmap in crrev.com/1128113011

[spang, 2015-07-24 18:14:18]

ui/ozone lgtm

https://codereview.chromium.org/1258713002/diff/1/ui/ozone/gpu/README
File ui/ozone/gpu/README (left):

https://codereview.chromium.org/1258713002/diff/1/ui/ozone/gpu/README#oldcode1
ui/ozone/gpu/README:1: This component contains support code for
content/common/gpu.
:)

[dshwang, 2015-07-24 18:21:36]

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

[dshwang, 2015-07-24 18:23:02]

On 2015/07/24 18:14:18, spang wrote:
> ui/ozone lgtm

Thank you for rapid reviewing! 

reveman, could you review overall change and
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.* ?
piman, could you review content/common/gpu/media/vaapi_drm_picture.cc,
ui/gl/gl.gyp, ui/gl/BUILD.gn ?

[piman, 2015-07-24 18:41:25]

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:141:
ui::NativePixmap* pixmap = nullptr;
You need to keep a reference here. Otherwise, as soon as you release the lock,
another thread could remove the entry and release the last reference, and you
have a dangling pointer.

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h:52: typedef
std::map<std::pair<uint32_t, uint32_t>,
nit: hash_map.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:21: bool Initialize(ui::NativePixmap*
pixmap) {
Can you avoid overloading? It makes the code confusing, and is discouraged by
the style guide

My suggestion is to provide a factory function instead:
static scoped_ptr<GLImageOzoneNativePixmap> Create(const Size& size,
ui::NativePixmap* pixmap), which does both the allocation and initialization.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:57: bool Initialize(ui::NativePixmap*
pixmap, GpuMemoryBuffer::Format format) {
Same here, no overloading please.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:131: NOTREACHED();
If NOTREACHED(), can you replace this by a DCHECK(!pixmap) ?

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:132: return scoped_refptr<GLImage>();
nit: return nullptr should work

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:139: return scoped_refptr<GLImage>();
nit: here too.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:143: if (pixmap->GetDmaBufFd() > 0) {
0 is a valid fd.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:147: return scoped_refptr<GLImage>();
nit: return nullptr should work

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.h:14: GL_EXPORT scoped_refptr<GLImage>
CreateImageForNativePixmap(
There should be an "Ozone" in this name, I think.

[reveman, 2015-07-24 19:13:17]

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:60: }
nit: remove this function in favor of just using "NativePixmapMapKey key(id,
client_id)" below for consistency with other implementations

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:114:
native_pixmap_map_[GetIndex(id, client_id)] = pixmap;
nit: add DCHECK to ensure that pixmap with this key doesn't already exist

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:126:
native_pixmap_map_.erase(GetIndex(id, client_id));
nit: add DCHECK to ensure that pixmap with this key exist

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:141:
ui::NativePixmap* pixmap = nullptr;
On 2015/07/24 at 18:41:25, piman (Very slow to review) wrote:
> You need to keep a reference here. Otherwise, as soon as you release the lock,
another thread could remove the entry and release the last reference, and you
have a dangling pointer.

Maybe fix this by keeping the lock while we create the image as that's what
other implementations do.

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h:55:
base::Lock native_pixmap_map_lock_;
nit: make all this consistent with
GpuMemoryBufferFactoryIOSurface/SurfaceTexture:

using NativePixmapMapKey = std::pair<int, int>;
using NativePixmapMap = base::hash_map<NativePixmapMapKey,
scoped_refptr<ui::NativePixmap>>;
NativePixmapMap native_pixmaps_;
base::Lock native_pixmaps_lock_;

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.h:18: unsigned internalformat);
If we're going to add a ui/gl/gl_image_ozone_native_pixmap.h/cc file then we
should keep this consistent with other ui/gl/gl_image_* implemenations. e.g. one
class per file and no hidden factory in the .cc file.

Alternatively, to avoid a lot of refactoring in this CL you can just inline all
this ozone GLImage code in
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc and save the
exercise of moving the implementation to ui/gl as a follow up.

[dshwang, 2015-07-24 19:56:24]

Patchset #2 (id:20001) has been deleted

[dshwang, 2015-07-24 19:57:21]

thank you for quick reviewing!
I addressed all comments. could you review again?

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:60: }
On 2015/07/24 19:13:17, reveman wrote:
> nit: remove this function in favor of just using "NativePixmapMapKey key(id,
> client_id)" below for consistency with other implementations

Done.

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:114:
native_pixmap_map_[GetIndex(id, client_id)] = pixmap;
On 2015/07/24 19:13:17, reveman wrote:
> nit: add DCHECK to ensure that pixmap with this key doesn't already exist

Done.

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:126:
native_pixmap_map_.erase(GetIndex(id, client_id));
On 2015/07/24 19:13:17, reveman wrote:
> nit: add DCHECK to ensure that pixmap with this key exist

Done.

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:141:
ui::NativePixmap* pixmap = nullptr;
On 2015/07/24 19:13:17, reveman wrote:
> On 2015/07/24 at 18:41:25, piman (Very slow to review) wrote:
> > You need to keep a reference here. Otherwise, as soon as you release the
lock,
> another thread could remove the entry and release the last reference, and you
> have a dangling pointer.

good point. changed to "scoped_refptr<ui::NativePixmap> pixmap;"

> Maybe fix this by keeping the lock while we create the image as that's what
> other implementations do.

scoped_refptr<ui::NativePixmap> is enough because "class NativePixmap : public
base::RefCountedThreadSafe<NativePixmap>"
|native_pixmap_map_lock_| is for |native_pixmap_map_|, not NativePixmap.

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h:52: typedef
std::map<std::pair<uint32_t, uint32_t>,
On 2015/07/24 18:41:25, piman (Very slow to review) wrote:
> nit: hash_map.

Done.

https://codereview.chromium.org/1258713002/diff/1/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h:55:
base::Lock native_pixmap_map_lock_;
On 2015/07/24 19:13:17, reveman wrote:
> nit: make all this consistent with
> GpuMemoryBufferFactoryIOSurface/SurfaceTexture:
> 
> using NativePixmapMapKey = std::pair<int, int>;
> using NativePixmapMap = base::hash_map<NativePixmapMapKey,
> scoped_refptr<ui::NativePixmap>>;
> NativePixmapMap native_pixmaps_;
> base::Lock native_pixmaps_lock_;

cool. Done.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:21: bool Initialize(ui::NativePixmap*
pixmap) {
On 2015/07/24 18:41:25, piman (Very slow to review) wrote:
> Can you avoid overloading? It makes the code confusing, and is discouraged by
> the style guide
> 
> My suggestion is to provide a factory function instead:
> static scoped_ptr<GLImageOzoneNativePixmap> Create(const Size& size,
> ui::NativePixmap* pixmap), which does both the allocation and initialization.

Good suggestion! Done.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:57: bool Initialize(ui::NativePixmap*
pixmap, GpuMemoryBuffer::Format format) {
On 2015/07/24 18:41:25, piman (Very slow to review) wrote:
> Same here, no overloading please.

Done.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:131: NOTREACHED();
On 2015/07/24 18:41:25, piman (Very slow to review) wrote:
> If NOTREACHED(), can you replace this by a DCHECK(!pixmap) ?

Yes, DCHECK is better. Done.

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:143: if (pixmap->GetDmaBufFd() > 0) {
On 2015/07/24 18:41:25, piman (Very slow to review) wrote:
> 0 is a valid fd.

change to "pixmap->GetDmaBufFd() >= 0"

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.h:14: GL_EXPORT scoped_refptr<GLImage>
CreateImageForNativePixmap(
On 2015/07/24 18:41:25, piman (Very slow to review) wrote:
> There should be an "Ozone" in this name, I think.

change to CreateImageForOzoneNativePixmap

https://codereview.chromium.org/1258713002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.h:18: unsigned internalformat);
On 2015/07/24 19:13:17, reveman wrote:
> If we're going to add a ui/gl/gl_image_ozone_native_pixmap.h/cc file then we
> should keep this consistent with other ui/gl/gl_image_* implemenations. e.g.
one
> class per file and no hidden factory in the .cc file.
> 
> Alternatively, to avoid a lot of refactoring in this CL you can just inline
all
> this ozone GLImage code in
> content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc and save
the
> exercise of moving the implementation to ui/gl as a follow up.

ui/ozone/demo and content/common/gpu/media needs this also. Both should not
depend on content/common/gpu
I think factory is practical solution in our situation. WDYT?

[piman, 2015-07-24 21:56:26]

LGTM with a bunch of nits. Make sure you get reveman@'s LGTM before landing.

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:106: return
gfx::GpuMemoryBufferHandle();
nit: return nullptr

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:114:
native_pixmaps_[BufferToPixmapMapKey(id, client_id)] = pixmap;
nit: a better way to write this is:
auto result = native_pixmaps_.insert(BufferToPixmapMapKey(id, client_id),
pixmap);
DCHECK(result.second);

That way:
- you don't need to default-construct + copy-assign (as [] does on a map),
instead copy-constructing in place.
- you don't need 2 lookups
- it's shorter

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:130:
native_pixmaps_.erase(BufferToPixmapMapKey(id, client_id));
nit: similarly, you can do:

auto it = native_pixmaps_.find(BufferToPixmapMapKey(id, client_id));
DCHECK(it != native_pixmaps_.end()) << "pixmap with this key must exist";
native_pixmaps_.erase(it);

That way you don't have the double lookup, and it's shorter.

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:151: return
scoped_refptr<gfx::GLImage>();
nit: return nullptr

https://codereview.chromium.org/1258713002/diff/40001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:18: new GLImageOzoneNativePixmap(size);
nit: you can pass pixmap to the constructor here

https://codereview.chromium.org/1258713002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:49: using GLImageEGL::Initialize;
nit: You shouldn't need this any more because you don't have an overload that
hides the base class one.

https://codereview.chromium.org/1258713002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:61: new
GLImageOzoneNativePixmapDmaBuf(size, internalformat);
nit: same here, you can pass pixmap to the constructor

[dshwang, 2015-07-25 05:28:15]

On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> LGTM with a bunch of nits. Make sure you get reveman@'s LGTM before landing.

Thank you for detailed reviewing. I resolved all nits.

reveman, could you review again?

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:106: return
gfx::GpuMemoryBufferHandle();
On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> nit: return nullptr

thx, but this method return instance, not pointer :)

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:114:
native_pixmaps_[BufferToPixmapMapKey(id, client_id)] = pixmap;
On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> nit: a better way to write this is:
> auto result = native_pixmaps_.insert(BufferToPixmapMapKey(id, client_id),
> pixmap);
> DCHECK(result.second);
> 
> That way:
> - you don't need to default-construct + copy-assign (as [] does on a map),
> instead copy-constructing in place.
> - you don't need 2 lookups
> - it's shorter

we want to check if the pixmap already exist, instead of if insertion succeeds.

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:130:
native_pixmaps_.erase(BufferToPixmapMapKey(id, client_id));
On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> nit: similarly, you can do:
> 
> auto it = native_pixmaps_.find(BufferToPixmapMapKey(id, client_id));
> DCHECK(it != native_pixmaps_.end()) << "pixmap with this key must exist";
> native_pixmaps_.erase(it);
> 
> That way you don't have the double lookup, and it's shorter.

got it. cool.

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:151: return
scoped_refptr<gfx::GLImage>();
On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> nit: return nullptr

Done.

https://codereview.chromium.org/1258713002/diff/40001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:18: new GLImageOzoneNativePixmap(size);
On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> nit: you can pass pixmap to the constructor here

Done.

https://codereview.chromium.org/1258713002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:49: using GLImageEGL::Initialize;
On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> nit: You shouldn't need this any more because you don't have an overload that
> hides the base class one.

Done.

https://codereview.chromium.org/1258713002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:61: new
GLImageOzoneNativePixmapDmaBuf(size, internalformat);
On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> nit: same here, you can pass pixmap to the constructor

Done.

[piman, 2015-07-27 19:07:51]

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:114:
native_pixmaps_[BufferToPixmapMapKey(id, client_id)] = pixmap;
On 2015/07/25 05:28:15, dshwang wrote:
> On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> > nit: a better way to write this is:
> > auto result = native_pixmaps_.insert(BufferToPixmapMapKey(id, client_id),
> > pixmap);
> > DCHECK(result.second);
> > 
> > That way:
> > - you don't need to default-construct + copy-assign (as [] does on a map),
> > instead copy-constructing in place.
> > - you don't need 2 lookups
> > - it's shorter
> 
> we want to check if the pixmap already exist, instead of if insertion
succeeds.

Insertion fails if the pixmap already exists.

[piman, 2015-07-27 19:07:51]

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:114:
native_pixmaps_[BufferToPixmapMapKey(id, client_id)] = pixmap;
On 2015/07/25 05:28:15, dshwang wrote:
> On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> > nit: a better way to write this is:
> > auto result = native_pixmaps_.insert(BufferToPixmapMapKey(id, client_id),
> > pixmap);
> > DCHECK(result.second);
> > 
> > That way:
> > - you don't need to default-construct + copy-assign (as [] does on a map),
> > instead copy-constructing in place.
> > - you don't need 2 lookups
> > - it's shorter
> 
> we want to check if the pixmap already exist, instead of if insertion
succeeds.

Insertion fails if the pixmap already exists.

[reveman, 2015-07-27 19:28:29]

Re-factor ui/gl/gl_image_ozone_native_pixmap.h/cc so there's only one
GLImageOzoneNativePixmap class (probably means not using GLImageLinuxDMABuffer)
consistent with other GLImage implementations ui/gl/ and this looks good to me.

[dshwang, 2015-07-28 12:15:19]

On 2015/07/27 19:28:29, reveman wrote:
> Re-factor ui/gl/gl_image_ozone_native_pixmap.h/cc so there's only one
> GLImageOzoneNativePixmap class (probably means not using
GLImageLinuxDMABuffer)
> consistent with other GLImage implementations ui/gl/ and this looks good to
me.

Ok, I merged three class into GLImageOzoneNativePixmap.
Could you review again?

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:114:
native_pixmaps_[BufferToPixmapMapKey(id, client_id)] = pixmap;
On 2015/07/27 19:07:51, piman (Very slow to review) wrote:
> On 2015/07/25 05:28:15, dshwang wrote:
> > On 2015/07/24 21:56:26, piman (Very slow to review) wrote:
> > > nit: a better way to write this is:
> > > auto result = native_pixmaps_.insert(BufferToPixmapMapKey(id, client_id),
> > > pixmap);
> > > DCHECK(result.second);
> > > 
> > > That way:
> > > - you don't need to default-construct + copy-assign (as [] does on a map),
> > > instead copy-constructing in place.
> > > - you don't need 2 lookups
> > > - it's shorter
> > 
> > we want to check if the pixmap already exist, instead of if insertion
> succeeds.
> 
> Insertion fails if the pixmap already exists.

aha, thank you for advice. Done.

[reveman, 2015-07-28 14:40:57]

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.h:14: class GL_EXPORT
GLImageOzoneNativePixmap : public GLImage {
While it might not be ideal we currently use inheritance rather than composition
to share code between GLImage implementations so we should do the same here for
consistency. GLImageOzoneNativePixmap is a GLImageEGL.

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.h:20: unsigned internalformat);
Some consistency requests:

Please remove this Create function and make the ctor public like this:

GLImageOzoneNativePixmap(const gfx::Size& size, unsigned internalformat);

and a public Initialize function that looks like this:

bool Initialize(ui::NativePixmap* pixmap, gfx::GpuMemoryBuffer::Format format);

Initialize should set the pixmap and Destroy should reset it. Take a look at
other GLImage implementations if it's not clear exactly what this should look
like. 

Be keeping all this GLImage code consistent it's easier to maintain and easier
to make large changes to the GLImage system.

[dshwang, 2015-07-28 16:21:55]

reveman, Thank you for reviewing. Could you review again?

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.h:14: class GL_EXPORT
GLImageOzoneNativePixmap : public GLImage {
On 2015/07/28 14:40:57, reveman wrote:
> While it might not be ideal we currently use inheritance rather than
composition
> to share code between GLImage implementations so we should do the same here
for
> consistency. GLImageOzoneNativePixmap is a GLImageEGL.

GLImageOzoneNativePixmap inherits GLImageLinuxDMABuffer, which inherits
GLImageEGL.
It's because GLImageLinuxDMABuffer can handle both cases;
"pixmap_->GetEGLClientBuffer()" and "pixmap_->GetDmaBufFd() >= 0"

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.h:20: unsigned internalformat);
On 2015/07/28 14:40:57, reveman wrote:
> Some consistency requests:
> 
> Please remove this Create function and make the ctor public like this:
> 
> GLImageOzoneNativePixmap(const gfx::Size& size, unsigned internalformat);
> 
> and a public Initialize function that looks like this:
> 
> bool Initialize(ui::NativePixmap* pixmap, gfx::GpuMemoryBuffer::Format
format);
> 
> Initialize should set the pixmap and Destroy should reset it. Take a look at
> other GLImage implementations if it's not clear exactly what this should look
> like. 
> 
> Be keeping all this GLImage code consistent it's easier to maintain and easier
> to make large changes to the GLImage system.

Thank you for detailed explanation. Done.

[dshwang, 2015-07-28 16:25:25]

Patchset #5 (id:100001) has been deleted

[reveman, 2015-07-28 19:11:17]

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:13:
GLImageOzoneNativePixmap::~GLImageOzoneNativePixmap() {}
DCHECK(!pixmap_) in dtor

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:18: DCHECK(pixmap);
DCHECK(!pixmap_) instead

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:19: pixmap_ = pixmap;
pixmap_ should not be set if we return false

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:45: return pixmap_ &&
can we DCHECK(pixmap_) instead?

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:14: class GL_EXPORT
GLImageOzoneNativePixmap : public GLImageLinuxDMABuffer {
It's confusing to inherit from GLImageLinuxDMABuffer when this is not always
backed by a linux DMA buf. Can you inherit from GLImageEGL instead and move what
you need from GLImageLinuxDMABuffer to the GLImageOzoneNativePixmap class. I
think you can remove the GLImageLinuxDMABuffer class as it's not used by anyone
else.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:17: bool
Initialize(scoped_refptr<ui::NativePixmap> pixmap,
nit: blank line after ctor and ui::NativePixmap* instead of scoped_refptr to
avoid extra refcount churn

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:20: void Destroy(bool have_context)
override;
Above this line, add:
// Overridden from GLImage:

[spang, 2015-07-28 19:34:27]

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.h:14: class GL_EXPORT
GLImageOzoneNativePixmap : public GLImage {
On 2015/07/28 16:21:55, dshwang wrote:
> On 2015/07/28 14:40:57, reveman wrote:
> > While it might not be ideal we currently use inheritance rather than
> composition
> > to share code between GLImage implementations so we should do the same here
> for
> > consistency. GLImageOzoneNativePixmap is a GLImageEGL.
> 
> GLImageOzoneNativePixmap inherits GLImageLinuxDMABuffer, which inherits
> GLImageEGL.
> It's because GLImageLinuxDMABuffer can handle both cases;
> "pixmap_->GetEGLClientBuffer()" and "pixmap_->GetDmaBufFd() >= 0"

This is an abuse of inheritance.

I don't think we should be stacking this class on top of GLImageLinuxDMABuffer,
since it might not be a dmabuf.

If we need to support both EGL_LINUX_DMA_BUF_EXT nad EGL_NATIVE_PIXMAP_KHR in a
unified GLImageOzoneNativePixmap class, we should merge GLImageLinuxDMABuffer
directly into the class rather than use inheritance as a way to import
functionality.

One reason this is wrong is that in order to support a 3rd kind of image using
this pattern, we'd need multiple inheritance, but it's not allowed by the style
guide.

[dshwang, 2015-07-29 13:10:19]

Patchset #7 (id:150001) has been deleted

[dshwang, 2015-07-29 13:23:10]

thx for reviewing. I resolved your comments. could you review again?

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/80001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.h:14: class GL_EXPORT
GLImageOzoneNativePixmap : public GLImage {
On 2015/07/28 19:34:27, spang wrote:
> On 2015/07/28 16:21:55, dshwang wrote:
> > On 2015/07/28 14:40:57, reveman wrote:
> > > While it might not be ideal we currently use inheritance rather than
> > composition
> > > to share code between GLImage implementations so we should do the same
here
> > for
> > > consistency. GLImageOzoneNativePixmap is a GLImageEGL.
> > 
> > GLImageOzoneNativePixmap inherits GLImageLinuxDMABuffer, which inherits
> > GLImageEGL.
> > It's because GLImageLinuxDMABuffer can handle both cases;
> > "pixmap_->GetEGLClientBuffer()" and "pixmap_->GetDmaBufFd() >= 0"
> 
> This is an abuse of inheritance.
> 
> I don't think we should be stacking this class on top of
GLImageLinuxDMABuffer,
> since it might not be a dmabuf.
> 
> If we need to support both EGL_LINUX_DMA_BUF_EXT nad EGL_NATIVE_PIXMAP_KHR in
a
> unified GLImageOzoneNativePixmap class, we should merge GLImageLinuxDMABuffer
> directly into the class rather than use inheritance as a way to import
> functionality.
> 
> One reason this is wrong is that in order to support a 3rd kind of image using
> this pattern, we'd need multiple inheritance, but it's not allowed by the
style
> guide.

Got it. It's same opinion to reveman's. I did make this class inherit GLImageEGL
and remove GLImageLinuxDMABuffer.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:13:
GLImageOzoneNativePixmap::~GLImageOzoneNativePixmap() {}
On 2015/07/28 19:11:17, reveman wrote:
> DCHECK(!pixmap_) in dtor

Done.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:18: DCHECK(pixmap);
On 2015/07/28 19:11:17, reveman wrote:
> DCHECK(!pixmap_) instead

Take both. Done.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:19: pixmap_ = pixmap;
On 2015/07/28 19:11:17, reveman wrote:
> pixmap_ should not be set if we return false

Done.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:45: return pixmap_ &&
On 2015/07/28 19:11:17, reveman wrote:
> can we DCHECK(pixmap_) instead?

Yes, Done.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:14: class GL_EXPORT
GLImageOzoneNativePixmap : public GLImageLinuxDMABuffer {
On 2015/07/28 19:11:17, reveman wrote:
> It's confusing to inherit from GLImageLinuxDMABuffer when this is not always
> backed by a linux DMA buf. Can you inherit from GLImageEGL instead and move
what
> you need from GLImageLinuxDMABuffer to the GLImageOzoneNativePixmap class. I
> think you can remove the GLImageLinuxDMABuffer class as it's not used by
anyone
> else.

Good point! This class inherits GLImageEGL and remove GLImageLinuxDMABuffer

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:17: bool
Initialize(scoped_refptr<ui::NativePixmap> pixmap,
On 2015/07/28 19:11:17, reveman wrote:
> nit: blank line after ctor and ui::NativePixmap* instead of scoped_refptr to
> avoid extra refcount churn

blank line after ctor: Done.
ui::NativePixmap* instead of scoped_refptr: I think scoped_refptr is better
because scoped_refptr has intention that this class keeps the pixmap reference
like how this class work. This method is not hot-spot, so I take readability
rather than optimization.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:20: void Destroy(bool have_context)
override;
On 2015/07/28 19:11:17, reveman wrote:
> Above this line, add:
> // Overridden from GLImage:

Done.

[reveman, 2015-07-29 14:16:41]

lgtm with nits

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:18: DCHECK(pixmap);
On 2015/07/29 at 13:23:09, dshwang wrote:
> On 2015/07/28 19:11:17, reveman wrote:
> > DCHECK(!pixmap_) instead
> 
> Take both. Done.

Checking that the first param to a function is non-null is overkill imo.
Especially when you're about to dereference it, which will cause an easy to
diagnose crash on all platforms.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:17: bool
Initialize(scoped_refptr<ui::NativePixmap> pixmap,
On 2015/07/29 at 13:23:09, dshwang wrote:
> On 2015/07/28 19:11:17, reveman wrote:
> > nit: blank line after ctor and ui::NativePixmap* instead of scoped_refptr to
> > avoid extra refcount churn
> 
> blank line after ctor: Done.
> ui::NativePixmap* instead of scoped_refptr: I think scoped_refptr is better
because scoped_refptr has intention that this class keeps the pixmap reference
like how this class work. This method is not hot-spot, so I take readability
rather than optimization.

We don't use scoped_refptr for that purpose in chromium. Please change to
ui::NativePixmap* as that's the most common in content/ and consistently used
within the GLImage framework. The alternative to avoid refcount churn is "const
scoped_refptr<ui::NativePixmap>& pixmap" but raw pointer is preferred here.

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:111:
DCHECK(result.second) << "pixmap with this key must not exist";
This is fine but I typically prefer to isolate DCHECK code so that it would
compile without "unused variable" errors if the DCHECK was removed. Mostly
because it makes the code easier to read. An extra lookup in a DCHECK is not a
problem as it's only used when running in debug mode. This code would then
instead look like:

NativePixmapMapKey key(id, client_id);                                          
DCHECK(natives_pixmaps_.find(key) == native_pixmaps_.end());                    
   
natives_pixmaps_[key] = pixmap;

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h:53: using
BufferToPixmapMapKey = std::pair<int, int>;
nit: for consistency use just the value type to describe the map. using
NativePixmapMapKey = std::pair<int, int>

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h:54: using
BufferToPixmapMap =
nit: NativePixmapMap

https://codereview.chromium.org/1258713002/diff/170001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/170001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:87: bool result = false;
Would this function be easier to read if you set the pixmap_ and early out on
success?

https://codereview.chromium.org/1258713002/diff/170001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:93: base::FileDescriptor
handle(pixmap->GetDmaBufFd(), false);
Is this temporary |handle| variable necessary?

https://codereview.chromium.org/1258713002/diff/170001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:121: static_cast<EGLClientBuffer>(NULL),
attrs);
Indent looks wierd here. Please clang format the patch.

[dshwang, 2015-07-29 15:15:04]

thx for careful reviewing. addressed all nits.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:18: DCHECK(pixmap);
On 2015/07/29 14:16:41, reveman wrote:
> On 2015/07/29 at 13:23:09, dshwang wrote:
> > On 2015/07/28 19:11:17, reveman wrote:
> > > DCHECK(!pixmap_) instead
> > 
> > Take both. Done.
> 
> Checking that the first param to a function is non-null is overkill imo.
> Especially when you're about to dereference it, which will cause an easy to
> diagnose crash on all platforms.

Done. relying on natural crash.

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/140001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.h:17: bool
Initialize(scoped_refptr<ui::NativePixmap> pixmap,
On 2015/07/29 14:16:41, reveman wrote:
> On 2015/07/29 at 13:23:09, dshwang wrote:
> > On 2015/07/28 19:11:17, reveman wrote:
> > > nit: blank line after ctor and ui::NativePixmap* instead of scoped_refptr
to
> > > avoid extra refcount churn
> > 
> > blank line after ctor: Done.
> > ui::NativePixmap* instead of scoped_refptr: I think scoped_refptr is better
> because scoped_refptr has intention that this class keeps the pixmap reference
> like how this class work. This method is not hot-spot, so I take readability
> rather than optimization.
> 
> We don't use scoped_refptr for that purpose in chromium. Please change to
> ui::NativePixmap* as that's the most common in content/ and consistently used
> within the GLImage framework. The alternative to avoid refcount churn is
"const
> scoped_refptr<ui::NativePixmap>& pixmap" but raw pointer is preferred here.

I see. Done.

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:111:
DCHECK(result.second) << "pixmap with this key must not exist";
On 2015/07/29 14:16:41, reveman wrote:
> This is fine but I typically prefer to isolate DCHECK code so that it would
> compile without "unused variable" errors if the DCHECK was removed. Mostly
> because it makes the code easier to read. An extra lookup in a DCHECK is not a
> problem as it's only used when running in debug mode. This code would then
> instead look like:
> 
> NativePixmapMapKey key(id, client_id);                                        
 
> DCHECK(natives_pixmaps_.find(key) == native_pixmaps_.end());                  
 
>    
> natives_pixmaps_[key] = pixmap;

oh, that's good idea. Done.

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h (right):

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h:53: using
BufferToPixmapMapKey = std::pair<int, int>;
On 2015/07/29 14:16:41, reveman wrote:
> nit: for consistency use just the value type to describe the map. using
> NativePixmapMapKey = std::pair<int, int>

Done.

https://codereview.chromium.org/1258713002/diff/170001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h:54: using
BufferToPixmapMap =
On 2015/07/29 14:16:41, reveman wrote:
> nit: NativePixmapMap

Done.

https://codereview.chromium.org/1258713002/diff/170001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/1258713002/diff/170001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:87: bool result = false;
On 2015/07/29 14:16:41, reveman wrote:
> Would this function be easier to read if you set the pixmap_ and early out on
> success?

current is more succinct, because otherwise we need to handle pixmap_ in every
return code (4 places in this function).

In addition, I changed this to "bool result = true;" to reserve
GLImageOzoneOverlayOnlyPassThrough logic.

https://codereview.chromium.org/1258713002/diff/170001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:93: base::FileDescriptor
handle(pixmap->GetDmaBufFd(), false);
On 2015/07/29 14:16:41, reveman wrote:
> Is this temporary |handle| variable necessary? 

no, removed.

https://codereview.chromium.org/1258713002/diff/170001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:121: static_cast<EGLClientBuffer>(NULL),
attrs);
On 2015/07/29 14:16:41, reveman wrote:
> Indent looks wierd here. Please clang format the patch.

Done.

[dshwang, 2015-07-29 15:16:21]

The CQ bit was checked by dongseong.hwang@intel.com

[dshwang, 2015-07-29 15:16:22]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org,
piman@chromium.org, reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1258713002/#ps190001
(title: "resolve nits")

[commit-bot: I haz the power, 2015-07-29 15:16:53]

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

[reveman, 2015-07-29 15:28:14]

https://codereview.chromium.org/1258713002/diff/190001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/190001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:125:
DCHECK(it != native_pixmaps_.end()) << "pixmap with this key must exist";
nit: maybe do the same as above here:
NativePixmapMapKey key(id, client_id);
DCHECK(native_pixmaps_.find(key) != native_pixmaps_.end());
native_pixmaps_.erase(key);

[dshwang, 2015-07-29 15:35:28]

https://codereview.chromium.org/1258713002/diff/190001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1258713002/diff/190001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:125:
DCHECK(it != native_pixmaps_.end()) << "pixmap with this key must exist";
On 2015/07/29 15:28:14, reveman wrote:
> nit: maybe do the same as above here:
> NativePixmapMapKey key(id, client_id);
> DCHECK(native_pixmaps_.find(key) != native_pixmaps_.end());
> native_pixmaps_.erase(key);

Thx, but I think before and after is identical. and |it| variable is used, no
matter DCHECK_ON

[commit-bot: I haz the power, 2015-07-29 15:40:30]

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

[commit-bot: I haz the power, 2015-07-29 15:40:31]

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

[dshwang, 2015-07-29 15:48:51]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2015-07-29 15:49:04]

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

[commit-bot: I haz the power, 2015-07-29 16:24:08]

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

[commit-bot: I haz the power, 2015-07-29 16:24:09]

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

[dshwang, 2015-07-29 17:41:40]

The CQ bit was checked by dongseong.hwang@intel.com

[dshwang, 2015-07-29 17:41:40]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
reveman@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1258713002/#ps210001
(title: "fix linux_chromium_gn_dgb build")

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

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

[commit-bot: I haz the power, 2015-07-29 18:28:28]

Committed patchset #9 (id:210001)

[commit-bot: I haz the power, 2015-07-29 18:29:43]

Patchset 9 (id:??) landed as
https://crrev.com/161c7a23d99119c7acf7f9faf0ae794ab4d62b62
Cr-Commit-Position: refs/heads/master@{#340927}