Add a factory and defines for native Linux surfaces.

Add a factory and defines for native Linux surfaces.

This CL is a first step towards supporting a build of the Aura
content shell on the native Linux framebuffer device. It sets up
the necessary defines such that an injected surface provider
dependency can vend a native surface.

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

[rjkroege, 2013-04-29 22:57:04]

backer@, piman@ please take a look.

[jonathan.backer, 2013-04-30 12:41:20]

LGTM. Just nits.

https://codereview.chromium.org/13886018/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/13886018/diff/1/build/common.gypi#newcode2637
build/common.gypi:2637: '-funwind-tables', # Emit unwind tables despite
-fno-exceptions.
IDK what this is, but I don't see it on ToT. Merge error?

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl.gyp#newcode70
ui/gl/gl.gyp:70: 'gl_context_nsl.mm',
.mm? nix this?

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl.gyp#newcode71
ui/gl/gl.gyp:71: 'gl_context_osmesa.cc',
nit: whitespace.

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:153: #if defined(USE_X11) || defined(OS_ANDROID)  ||
defined(USE_NATIVE_SURFACE_LINUX)
nit: use a \ to wrap?

[DaveMoore, 2013-04-30 17:53:42]

https://codereview.chromium.org/13886018/diff/1/third_party/khronos/EGL/eglpl...
File third_party/khronos/EGL/eglplatform.h (right):

https://codereview.chromium.org/13886018/diff/1/third_party/khronos/EGL/eglpl...
third_party/khronos/EGL/eglplatform.h:99: typedef int   EGLNativeDisplayType;
Nit: either align the type names or only one space between int and EGL

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
File ui/base/linux/native_surface_linux_factory.cc (right):

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
ui/base/linux/native_surface_linux_factory.cc:33:
delegate_->InitializeHardware();
Nit: no braces (other places in file too).

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
File ui/base/linux/native_surface_linux_factory.h (right):

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
ui/base/linux/native_surface_linux_factory.h:18: 
Nit: no blank line

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
File ui/base/linux/native_surface_linux_factory_delegate.cc (right):

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
ui/base/linux/native_surface_linux_factory_delegate.cc:12: 
Nit: no blank line

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
File ui/base/linux/native_surface_linux_factory_delegate.h (right):

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
ui/base/linux/native_surface_linux_factory_delegate.h:5: #ifndef
UI_BASE_LINUX_NATIVE_SURFACE_LNUX_FACTORY_DELEGATE_
Nit: LINUX not LNUX

https://codereview.chromium.org/13886018/diff/1/ui/surface/transport_dib_sysv...
File ui/surface/transport_dib_sysvipc.cc (right):

https://codereview.chromium.org/13886018/diff/1/ui/surface/transport_dib_sysv...
ui/surface/transport_dib_sysvipc.cc:9: #if defined(TOOLKIT_GTK) ||
(defined(OS_LINUX) && defined(USE_AURA) && defined(USE_X11))
This file now has a lot of #ifdefs. We might be better off splitting it into a
case just for X11 and another without.

[davemoore (google), 2013-04-30 23:49:25]

Ignore that...accidental byproduct of using git cl patch. I'll create another
issue

[davemoore (google), 2013-04-30 23:50:22]

On 2013/04/30 23:49:25, davemoore (google) wrote:
> Ignore that...accidental byproduct of using git cl patch. I'll create another
> issue

Or maybe this is the best way for Robert to integrate these changes into his
next upload.

[rjkroege, 2013-05-06 18:46:24]

now with use of ozone.

https://codereview.chromium.org/13886018/diff/1/third_party/khronos/EGL/eglpl...
File third_party/khronos/EGL/eglplatform.h (right):

https://codereview.chromium.org/13886018/diff/1/third_party/khronos/EGL/eglpl...
third_party/khronos/EGL/eglplatform.h:99: typedef int   EGLNativeDisplayType;
On 2013/04/30 17:53:42, DaveMoore wrote:
> Nit: either align the type names or only one space between int and EGL

Done.

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
File ui/base/linux/native_surface_linux_factory.cc (right):

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
ui/base/linux/native_surface_linux_factory.cc:33:
delegate_->InitializeHardware();
On 2013/04/30 17:53:42, DaveMoore wrote:
> Nit: no braces (other places in file too).

Done.

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
File ui/base/linux/native_surface_linux_factory.h (right):

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
ui/base/linux/native_surface_linux_factory.h:18: 
On 2013/04/30 17:53:42, DaveMoore wrote:
> Nit: no blank line

Done.

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
File ui/base/linux/native_surface_linux_factory_delegate.cc (right):

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
ui/base/linux/native_surface_linux_factory_delegate.cc:12: 
On 2013/04/30 17:53:42, DaveMoore wrote:
> Nit: no blank line

Done.

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
File ui/base/linux/native_surface_linux_factory_delegate.h (right):

https://codereview.chromium.org/13886018/diff/1/ui/base/linux/native_surface_...
ui/base/linux/native_surface_linux_factory_delegate.h:5: #ifndef
UI_BASE_LINUX_NATIVE_SURFACE_LNUX_FACTORY_DELEGATE_
On 2013/04/30 17:53:42, DaveMoore wrote:
> Nit: LINUX not LNUX

Done.

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl.gyp#newcode70
ui/gl/gl.gyp:70: 'gl_context_nsl.mm',
On 2013/04/30 12:41:20, jonathan.backer wrote:
> .mm? nix this?

bad merge. fixed.

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl.gyp#newcode71
ui/gl/gl.gyp:71: 'gl_context_osmesa.cc',
On 2013/04/30 12:41:20, jonathan.backer wrote:
> nit: whitespace.

Done.

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/13886018/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:153: #if defined(USE_X11) || defined(OS_ANDROID)  ||
defined(USE_NATIVE_SURFACE_LINUX)
On 2013/04/30 12:41:20, jonathan.backer wrote:
> nit: use a \ to wrap?

Done.

https://codereview.chromium.org/13886018/diff/13001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/13886018/diff/13001/build/common.gypi#newcode...
build/common.gypi:2637: '-funwind-tables', # Emit unwind tables despite
-fno-exceptions.
I should probably cull this

[rjkroege, 2013-05-17 22:12:00]

piman@: please take another look. I attempted to address the issues from our
discussion.

[rjkroege, 2013-05-17 22:17:06]

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl.gyp#newcode181
ui/gl/gl.gyp:181: ['OS in ("win", "android", "linux")', {
this looks like an error on my part. fixing.

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_implementation_o...
File ui/gl/gl_implementation_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_implementation_o...
ui/gl/gl_implementation_ozone.cc:11: #include
"ui/base/ozone/surface_factory_ozone.h"
I think that per discussion, I can refactor this file a bit.

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface_ozone.cc...
ui/gl/gl_surface_ozone.cc:9: #include "ui/base/ozone/surface_factory_ozone.h"
I'm not convinced that this file is worth refactoring. It's starting to diverge
more

[piman, 2013-05-18 01:47:19]

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_delegate_ozone.h (right):

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_delegate_ozone.h:6: #define
UI_BASE_LINUX_NATIVE_SURFACE_LINUX_FACTORY_DELEGATE_
nit: guard is wrong.

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.cc:49: return (gfx::AcceleratedWidget)0;
nit: no c-style cast

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.h (right):

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:6: #define
UI_BASE_LINUX_NATIVE_SURFACE_LNUX_FACTORY_
nit: guard is wrong

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:20: class SurfaceFactory {
TBH I'm not sure why we have a separate SurfaceFactory and
SurfaceFactoryDelegate. SurfaceFactory isn't functional without a delegate.
They're 1:1, and have the same methods.

Since you need initialization-time code to setup the delegate, you might as well
make SurfaceFactory the interface, and have a couple of static methods to setup
the global factory and to get it.

Lastly, can this be named SurfaceFactoryOzone or something, to indicate it's
specific to that port?

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface.h#newcode35
ui/gl/gl_surface.h:35: virtual bool Initialize(VSyncProvider* syncProvider);
Could we move this to the NativeViewGLSurfaceEGL constructor? From the usage
pattern, it looks ok to do, and that way we won't need to leak this into the
cross-platform interface.

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface_ozone.cc...
ui/gl/gl_surface_ozone.cc:9: #include "ui/base/ozone/surface_factory_ozone.h"
On 2013/05/17 22:17:06, rjkroege wrote:
> I'm not convinced that this file is worth refactoring. It's starting to
diverge
> more

TBH, with the suggested change to pass the VSyncProvider through the
constructor, it would be exactly 2 lines different (calling InitializeHardware,
and getting the VSyncProvider from the SurfaceFactory), I'm not sure it warrants
forking 85.

[rjkroege, 2013-05-21 17:36:28]

Please take another look.

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_delegate_ozone.h (right):

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_delegate_ozone.h:6: #define
UI_BASE_LINUX_NATIVE_SURFACE_LINUX_FACTORY_DELEGATE_
On 2013/05/18 01:47:19, piman wrote:
> nit: guard is wrong.

Done.

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.cc:49: return (gfx::AcceleratedWidget)0;
On 2013/05/18 01:47:19, piman wrote:
> nit: no c-style cast

Removed in implementation of other suggestion. :-)

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.h (right):

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:6: #define
UI_BASE_LINUX_NATIVE_SURFACE_LNUX_FACTORY_
On 2013/05/18 01:47:19, piman wrote:
> nit: guard is wrong

Done.

https://codereview.chromium.org/13886018/diff/22001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:20: class SurfaceFactory {
On 2013/05/18 01:47:19, piman wrote:
> TBH I'm not sure why we have a separate SurfaceFactory and
> SurfaceFactoryDelegate. SurfaceFactory isn't functional without a delegate.
> They're 1:1, and have the same methods.
> 
> Since you need initialization-time code to setup the delegate, you might as
well
> make SurfaceFactory the interface, and have a couple of static methods to
setup
> the global factory and to get it.

OK. Done.

> 
> Lastly, can this be named SurfaceFactoryOzone or something, to indicate it's
> specific to that port?

Yes. Done.

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl.gyp#newcode181
ui/gl/gl.gyp:181: ['OS in ("win", "android", "linux")', {
On 2013/05/17 22:17:06, rjkroege wrote:
> this looks like an error on my part. fixing.

on double-checking, this change is indeed necessary.

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface.h#newcode35
ui/gl/gl_surface.h:35: virtual bool Initialize(VSyncProvider* syncProvider);
On 2013/05/18 01:47:19, piman wrote:
> Could we move this to the NativeViewGLSurfaceEGL constructor? From the usage
> pattern, it looks ok to do, and that way we won't need to leak this into the
> cross-platform interface.

Done.

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/22001/ui/gl/gl_surface_ozone.cc...
ui/gl/gl_surface_ozone.cc:9: #include "ui/base/ozone/surface_factory_ozone.h"
On 2013/05/18 01:47:19, piman wrote:
> On 2013/05/17 22:17:06, rjkroege wrote:
> > I'm not convinced that this file is worth refactoring. It's starting to
> diverge
> > more
> 
> TBH, with the suggested change to pass the VSyncProvider through the
> constructor, it would be exactly 2 lines different (calling
InitializeHardware,
> and getting the VSyncProvider from the SurfaceFactory), I'm not sure it
warrants
> forking 85.

Done.

[piman, 2013-05-21 18:32:15]

LGTM + a few nits.

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.h (right):

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:29: static void
SetDelegate(SurfaceFactoryOzone* delegate);
nit: SetInstance?

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:32: // Configure display hardware. Must be
called from within the GPU process
nit: Configures

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:36: // Clean up display hardware state.
Call this from within the GPU process.
nit: Cleans

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:40: // Obtain an AcceleratedWidget backed
by a native Linux framebuffer.
nit: Obtains

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:43: const gfx::GLSurfaceHandle& handle) =
0;
Is this call used? I don't see it. It's also somewhat strange because
GLSurfaceHandle is really only used inside of content/ to represent the
renderer's output surface wrt the browser, the ui/ layer never sees it.

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:48: // Test if the given AcceleratedWidget
instance can be resized. (Native
nit: Tests

[rjkroege, 2013-05-22 17:08:30]

nits addressed plus change discussed yesterday. ptal?

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.h (right):

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:29: static void
SetDelegate(SurfaceFactoryOzone* delegate);
On 2013/05/21 18:32:15, piman wrote:
> nit: SetInstance?

Done.

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:32: // Configure display hardware. Must be
called from within the GPU process
On 2013/05/21 18:32:15, piman wrote:
> nit: Configures

Done.

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:36: // Clean up display hardware state.
Call this from within the GPU process.
On 2013/05/21 18:32:15, piman wrote:
> nit: Cleans

Done.

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:40: // Obtain an AcceleratedWidget backed
by a native Linux framebuffer.
On 2013/05/21 18:32:15, piman wrote:
> nit: Obtains

Done.

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:43: const gfx::GLSurfaceHandle& handle) =
0;
On 2013/05/21 18:32:15, piman wrote:
> Is this call used? I don't see it. It's also somewhat strange because
> GLSurfaceHandle is really only used inside of content/ to represent the
> renderer's output surface wrt the browser, the ui/ layer never sees it.

I think that your comment in https://codereview.chromium.org/14985005 makes it
clear that I am not calling this in the right place.

https://codereview.chromium.org/13886018/diff/35001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.h:48: // Test if the given AcceleratedWidget
instance can be resized. (Native
On 2013/05/21 18:32:15, piman wrote:
> nit: Tests

Done.

[piman, 2013-05-22 17:10:48]

lgtm

[commit-bot: I haz the power, 2013-05-23 18:01:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/13886018/54003

[commit-bot: I haz the power, 2013-05-23 19:12:09]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[rjkroege, 2013-05-23 19:58:07]

+sky for OWNERS in ui

[sky, 2013-05-23 21:21:32]

https://codereview.chromium.org/13886018/diff/54003/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/54003/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.cc:12: scoped_ptr<SurfaceFactoryOzone>
impl_;
style guide says no static initializers.

https://codereview.chromium.org/13886018/diff/54003/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.cc:29: return getenv("ASH_DISPLAY_SPEC") ?:
"720x1280*2";
Does this do what you want? And the spacing is off.

[rjkroege, 2013-05-23 23:34:09]

ptal.

https://codereview.chromium.org/13886018/diff/54003/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/54003/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.cc:12: scoped_ptr<SurfaceFactoryOzone>
impl_;
On 2013/05/23 21:21:32, sky wrote:
> style guide says no static initializers.

Done: now Lazy.

https://codereview.chromium.org/13886018/diff/54003/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.cc:29: return getenv("ASH_DISPLAY_SPEC") ?:
"720x1280*2";
On 2013/05/23 21:21:32, sky wrote:
> Does this do what you want? And the spacing is off.

Done.

[sky, 2013-05-23 23:47:20]

LGTM

https://codereview.chromium.org/13886018/diff/91001/ui/base/ozone/surface_fac...
File ui/base/ozone/surface_factory_ozone.cc (right):

https://codereview.chromium.org/13886018/diff/91001/ui/base/ozone/surface_fac...
ui/base/ozone/surface_factory_ozone.cc:34: else
nit: no else (the style guide actually says no else here).

[commit-bot: I haz the power, 2013-05-24 00:01:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/13886018/96001

[commit-bot: I haz the power, 2013-05-24 01:10:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-24 01:40:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/13886018/96001

[commit-bot: I haz the power, 2013-05-24 06:55:57]

Change committed as 201996