Ozone X11 platform

Ozone X11 platform

It uses the existing X11 platform event source and converts to ui::Event before dispatching.
use_x11 is false for the entire build, a new flag use_x11_backend is introduced to enable the x11 event processing.

BUG=361137

[achaulk, 2014-11-21 21:53:42]

achaulk@chromium.org changed reviewers:
+ sadrul@chromium.org, spang@chromium.org

[achaulk, 2014-11-21 21:59:05]

New version for ozone X11

This one uses the X event code, but translates to ui::Event just before running
the platform event dispatch code. Some X events need to be processed by the
window, so I created a new event type to wrap the XEvent pointer when needed

Created use_x11_backend variable to replace use_x11 in the cases where we still
want x even without x.

[spang, 2014-11-24 18:56:54]

https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni
File build/config/ui.gni (right):

https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni#newcode65
build/config/ui.gni:65: use_ozone_evdev = use_ozone && !use_ozone_x11
shouldn't disable evdev if x11 is set

https://codereview.chromium.org/750593003/diff/1/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/750593003/diff/1/ui/events/BUILD.gn#newcode140
ui/events/BUILD.gn:140: "OZONE_X11"
can we do without this ifdef?

https://codereview.chromium.org/750593003/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/750593003/diff/1/ui/events/event.cc#newcode80
ui/events/event.cc:80: CASE_TYPE(ET_X_RAW_EVENT);
This is essentially just native_event() spelled differently. We've been trying
to take a principled stance on removing that.

If did want to break that principle, the right thing to do would be to allow
NativeEvent to be an opaque pointer in ozone builds and add a generic way to
build a ui::Event out of it.

However my sense is still that we should concentrate on removing it.

https://codereview.chromium.org/750593003/diff/1/ui/events/x/events_x.h
File ui/events/x/events_x.h (right):

https://codereview.chromium.org/750593003/diff/1/ui/events/x/events_x.h#newco...
ui/events/x/events_x.h:31: EVENTS_EXPORT EventType EventTypeFromNative(const
XEvent* native_event);
I don't think we should have these X overloads in our build. It is very
confusing to have both

EventTypeFromNative(const XEvent* native_event);

and

EventTypeFromNative(const base::NativeEvent& aka. void* const &);

in the same build.

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

https://codereview.chromium.org/750593003/diff/1/ui/gl/gl_surface_ozone.cc#ne...
ui/gl/gl_surface_ozone.cc:73: #if defined(OZONE_X11)
please don't use an #ifdef for this, it is very wrong if we build both x11 and
!x11 into the same binary.

https://codereview.chromium.org/750593003/diff/1/ui/ozone/platform/x11/BUILD.gn
File ui/ozone/platform/x11/BUILD.gn (right):

https://codereview.chromium.org/750593003/diff/1/ui/ozone/platform/x11/BUILD....
ui/ozone/platform/x11/BUILD.gn:5:
import("//tools/generate_library_loader/generate_library_loader.gni")
unused

https://codereview.chromium.org/750593003/diff/1/ui/ozone/platform/x11/x11_su...
File ui/ozone/platform/x11/x11_surface_factory.cc (right):

https://codereview.chromium.org/750593003/diff/1/ui/ozone/platform/x11/x11_su...
ui/ozone/platform/x11/x11_surface_factory.cc:56: SetGLGetProcAddressProcCallback
set_gl_get_proc_address) {
I think we have copied this enough times that it deserves a utility function.

[achaulk, 2014-11-24 19:28:39]

https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni
File build/config/ui.gni (right):

https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni#newcode65
build/config/ui.gni:65: use_ozone_evdev = use_ozone && !use_ozone_x11
On 2014/11/24 18:56:54, spang wrote:
> shouldn't disable evdev if x11 is set

Why not? I guess it doesn't matter either way, but that config doesn't make any
sense

https://codereview.chromium.org/750593003/diff/1/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/750593003/diff/1/ui/events/BUILD.gn#newcode140
ui/events/BUILD.gn:140: "OZONE_X11"
On 2014/11/24 18:56:54, spang wrote:
> can we do without this ifdef?

Not easily, events_ozone.cc and events_x.cc both define the global void
UpdateDeviceList() and we need the X version

https://codereview.chromium.org/750593003/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/750593003/diff/1/ui/events/event.cc#newcode80
ui/events/event.cc:80: CASE_TYPE(ET_X_RAW_EVENT);
On 2014/11/24 18:56:54, spang wrote:
> This is essentially just native_event() spelled differently. We've been trying
> to take a principled stance on removing that.
> 
> If did want to break that principle, the right thing to do would be to allow
> NativeEvent to be an opaque pointer in ozone builds and add a generic way to
> build a ui::Event out of it.
> 
> However my sense is still that we should concentrate on removing it.

Yes, but there are some messages that need to be passed up to the window
implementation as-is. I could add ET_FOCUS_OUT, ET_EXPOSE, ET_CONFIGURE,
ET_CLIENT_MESSAGE and appropriate structs... but those are still X specific
abstractions anyway, and CLIENT_MESSAGE is just another name for NativeEvent too

https://codereview.chromium.org/750593003/diff/1/ui/events/x/events_x.h
File ui/events/x/events_x.h (right):

https://codereview.chromium.org/750593003/diff/1/ui/events/x/events_x.h#newco...
ui/events/x/events_x.h:31: EVENTS_EXPORT EventType EventTypeFromNative(const
XEvent* native_event);
On 2014/11/24 18:56:54, spang wrote:
> I don't think we should have these X overloads in our build. It is very
> confusing to have both
> 
> EventTypeFromNative(const XEvent* native_event);
> 
> and
> 
> EventTypeFromNative(const base::NativeEvent& aka. void* const &);
> 
> in the same build.

Well we still need the logic to determine EventType given an XEvent, along with
maybe about half of these to be able to generate the ui::Event objects. I could
copy-paste most of the cc file and rename everything, but that doesn't seem any
better

Besides, when NativeEvent is removed, all the NativeEvent& versions will go away

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

https://codereview.chromium.org/750593003/diff/1/ui/gl/gl_surface_ozone.cc#ne...
ui/gl/gl_surface_ozone.cc:73: #if defined(OZONE_X11)
On 2014/11/24 18:56:54, spang wrote:
> please don't use an #ifdef for this, it is very wrong if we build both x11 and
> !x11 into the same binary.

Well it won't work on non-X builds since XGetWindowAttributes won't be linked in
and that attribute list might be wrong too

https://codereview.chromium.org/750593003/diff/1/ui/ozone/platform/x11/BUILD.gn
File ui/ozone/platform/x11/BUILD.gn (right):

https://codereview.chromium.org/750593003/diff/1/ui/ozone/platform/x11/BUILD....
ui/ozone/platform/x11/BUILD.gn:5:
import("//tools/generate_library_loader/generate_library_loader.gni")
On 2014/11/24 18:56:54, spang wrote:
> unused

Done.

https://codereview.chromium.org/750593003/diff/1/ui/ozone/platform/x11/x11_su...
File ui/ozone/platform/x11/x11_surface_factory.cc (right):

https://codereview.chromium.org/750593003/diff/1/ui/ozone/platform/x11/x11_su...
ui/ozone/platform/x11/x11_surface_factory.cc:56: SetGLGetProcAddressProcCallback
set_gl_get_proc_address) {
On 2014/11/24 18:56:54, spang wrote:
> I think we have copied this enough times that it deserves a utility function.

Acknowledged.

[spang, 2014-11-24 20:04:24]

https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni
File build/config/ui.gni (right):

https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni#newcode65
build/config/ui.gni:65: use_ozone_evdev = use_ozone && !use_ozone_x11
On 2014/11/24 19:28:39, achaulk wrote:
> On 2014/11/24 18:56:54, spang wrote:
> > shouldn't disable evdev if x11 is set
> 
> Why not? I guess it doesn't matter either way, but that config doesn't make
any
> sense

It does make sense. All of the existing platforms can be part of the same build,
and x11 should too.

The buildbots should do a build like

  use_ozone = true
  ozone_platform_test = true
  ozone_platform_dri = true
  ozone_platform_x11 = true

  // caca example is currently off because it is not installed on the builders
  // gbm is currently off due to library versioning issues

.. so that they compile test as much as possible.

[achaulk, 2014-11-24 20:16:26]

https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni
File build/config/ui.gni (right):

https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni#newcode65
build/config/ui.gni:65: use_ozone_evdev = use_ozone && !use_ozone_x11
On 2014/11/24 20:04:24, spang wrote:
> On 2014/11/24 19:28:39, achaulk wrote:
> > On 2014/11/24 18:56:54, spang wrote:
> > > shouldn't disable evdev if x11 is set
> > 
> > Why not? I guess it doesn't matter either way, but that config doesn't make
> any
> > sense
> 
> It does make sense. All of the existing platforms can be part of the same
build,
> and x11 should too.
> 
> The buildbots should do a build like
> 
>   use_ozone = true
>   ozone_platform_test = true
>   ozone_platform_dri = true
>   ozone_platform_x11 = true
> 
>   // caca example is currently off because it is not installed on the builders
>   // gbm is currently off due to library versioning issues
> 
> .. so that they compile test as much as possible.

Oh just for sanity checking? It would link against X libraries so you couldn't
use these builds on devices without X libs in place, so I figured this would be
built separately

[spang, 2014-11-24 20:30:30]

On 2014/11/24 20:16:26, achaulk wrote:
> https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni
> File build/config/ui.gni (right):
> 
> https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni#newcode65
> build/config/ui.gni:65: use_ozone_evdev = use_ozone && !use_ozone_x11
> On 2014/11/24 20:04:24, spang wrote:
> > On 2014/11/24 19:28:39, achaulk wrote:
> > > On 2014/11/24 18:56:54, spang wrote:
> > > > shouldn't disable evdev if x11 is set
> > > 
> > > Why not? I guess it doesn't matter either way, but that config doesn't
make
> > any
> > > sense
> > 
> > It does make sense. All of the existing platforms can be part of the same
> build,
> > and x11 should too.
> > 
> > The buildbots should do a build like
> > 
> >   use_ozone = true
> >   ozone_platform_test = true
> >   ozone_platform_dri = true
> >   ozone_platform_x11 = true
> > 
> >   // caca example is currently off because it is not installed on the
builders
> >   // gbm is currently off due to library versioning issues
> > 
> > .. so that they compile test as much as possible.
> 
> Oh just for sanity checking? It would link against X libraries so you couldn't
> use these builds on devices without X libs in place, so I figured this would
be
> built separately

Right, buildbots will have everything. So this type of (turn off X things if Y)
conditional will break that.

CrOS device builds likely won't build x11 for the reason you mentioned (although
we can and might fix that with library_loader trickery to allow you to build a
single binary that works everywhere - some people (think that they) want this).

[achaulk, 2014-11-28 19:39:17]

On 2014/11/24 20:30:30, spang wrote:
> On 2014/11/24 20:16:26, achaulk wrote:
> > https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni
> > File build/config/ui.gni (right):
> > 
> >
https://codereview.chromium.org/750593003/diff/1/build/config/ui.gni#newcode65
> > build/config/ui.gni:65: use_ozone_evdev = use_ozone && !use_ozone_x11
> > On 2014/11/24 20:04:24, spang wrote:
> > > On 2014/11/24 19:28:39, achaulk wrote:
> > > > On 2014/11/24 18:56:54, spang wrote:
> > > > > shouldn't disable evdev if x11 is set
> > > > 
> > > > Why not? I guess it doesn't matter either way, but that config doesn't
> make
> > > any
> > > > sense
> > > 
> > > It does make sense. All of the existing platforms can be part of the same
> > build,
> > > and x11 should too.
> > > 
> > > The buildbots should do a build like
> > > 
> > >   use_ozone = true
> > >   ozone_platform_test = true
> > >   ozone_platform_dri = true
> > >   ozone_platform_x11 = true
> > > 
> > >   // caca example is currently off because it is not installed on the
> builders
> > >   // gbm is currently off due to library versioning issues
> > > 
> > > .. so that they compile test as much as possible.
> > 
> > Oh just for sanity checking? It would link against X libraries so you
couldn't
> > use these builds on devices without X libs in place, so I figured this would
> be
> > built separately
> 
> Right, buildbots will have everything. So this type of (turn off X things if
Y)
> conditional will break that.
> 
> CrOS device builds likely won't build x11 for the reason you mentioned
(although
> we can and might fix that with library_loader trickery to allow you to build a
> single binary that works everywhere - some people (think that they) want
this).

x11 can be built with other platforms now, and the ifdefs have been removed

[achaulk, 2014-12-05 21:19:04]

PTAL, I think with the xevent message type removed all of the initial concerns
have been addressed

[spang, 2014-12-06 00:31:23]

https://codereview.chromium.org/750593003/diff/100001/build/config/ui.gni
File build/config/ui.gni (right):

https://codereview.chromium.org/750593003/diff/100001/build/config/ui.gni#new...
build/config/ui.gni:61: # specifies if X11 is used as a backend even if use_x11
is off.
So if we just say 

 use_ozone=1 ozone_platform_x11=1

it won't work right? That's unfortunate because all the other platforms work
that way.

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

https://codereview.chromium.org/750593003/diff/100001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:70: if (!config_) {
We may want to delegate this whole config-choosing process and remove
GetEGLSurfaceProperties. It's clearly platform-specific in nontrivial ways.

https://codereview.chromium.org/750593003/diff/100001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:77: ~0,
I think this was overridden in the old code? Probably not legal to pass ~0.

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
File ui/ozone/platform/x11/ozone_platform_x11.cc (right):

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
ui/ozone/platform/x11/ozone_platform_x11.cc:49:
window_manager_->CreatePlatformWindow(delegate).Pass();
I don't think you need the .Pass() here.

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
ui/ozone/platform/x11/ozone_platform_x11.cc:79: X11InputController
input_controller_;
I don't think it makes sense to have an input controller in the GPU process.

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
File ui/ozone/platform/x11/x11_input_controller.cc (right):

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_input_controller.cc:10: }
add a newline between definitions.

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
File ui/ozone/platform/x11/x11_surface_factory.cc (right):

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_surface_factory.cc:25: NOTIMPLEMENTED();
This will be noisy, probably just "return true".

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_surface_factory.cc:35: bool SupportsPartialSwap()
override { return false; }
Not sure we should do this. Doing it this way breaks partial swap for everyone,
even if you have a working GLES driver.

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_surface_factory.cc:72: static int32 config_attribs[] =
{EGL_BUFFER_SIZE,
We should fix this broken interface rather than make this static.

[achaulk, 2014-12-08 16:33:43]

https://codereview.chromium.org/750593003/diff/100001/build/config/ui.gni
File build/config/ui.gni (right):

https://codereview.chromium.org/750593003/diff/100001/build/config/ui.gni#new...
build/config/ui.gni:61: # specifies if X11 is used as a backend even if use_x11
is off.
On 2014/12/06 00:31:23, spang wrote:
> So if we just say 
> 
>  use_ozone=1 ozone_platform_x11=1
> 
> it won't work right? That's unfortunate because all the other platforms work
> that way.

That configuration will use ozone-x11. What do you mean by "won't work"?

[achaulk, 2014-12-08 16:44:41]

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

https://codereview.chromium.org/750593003/diff/100001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:70: if (!config_) {
On 2014/12/06 00:31:23, spang wrote:
> We may want to delegate this whole config-choosing process and remove
> GetEGLSurfaceProperties. It's clearly platform-specific in nontrivial ways.

It is, but it involves calling egl from within ozone, and I thought we were
avoiding that?

https://codereview.chromium.org/750593003/diff/100001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:77: ~0,
On 2014/12/06 00:31:23, spang wrote:
> I think this was overridden in the old code? Probably not legal to pass ~0.

It's not, but GetEGLSurfaceProperties is doing the replacement

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
File ui/ozone/platform/x11/ozone_platform_x11.cc (right):

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
ui/ozone/platform/x11/ozone_platform_x11.cc:79: X11InputController
input_controller_;
On 2014/12/06 00:31:23, spang wrote:
> I don't think it makes sense to have an input controller in the GPU process.

It's just a stub for now. If it really is as simple as "return true for
HasMouse" then it doesn't matter where it lives, otherwise it'll get moved into
the events tree

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
File ui/ozone/platform/x11/x11_surface_factory.cc (right):

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_surface_factory.cc:25: NOTIMPLEMENTED();
On 2014/12/06 00:31:23, spang wrote:
> This will be noisy, probably just "return true".

Done.

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_surface_factory.cc:35: bool SupportsPartialSwap()
override { return false; }
On 2014/12/06 00:31:23, spang wrote:
> Not sure we should do this. Doing it this way breaks partial swap for
everyone,
> even if you have a working GLES driver.

I don't know how to determine if it is broken or not. The extension is reported
as present, it just doesn't work

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_surface_factory.cc:72: static int32 config_attribs[] =
{EGL_BUFFER_SIZE,
On 2014/12/06 00:31:23, spang wrote:
> We should fix this broken interface rather than make this static.

Sure

[spang, 2014-12-09 00:58:40]

On 2014/12/08 16:33:43, achaulk wrote:
> https://codereview.chromium.org/750593003/diff/100001/build/config/ui.gni
> File build/config/ui.gni (right):
> 
>
https://codereview.chromium.org/750593003/diff/100001/build/config/ui.gni#new...
> build/config/ui.gni:61: # specifies if X11 is used as a backend even if
use_x11
> is off.
> On 2014/12/06 00:31:23, spang wrote:
> > So if we just say 
> > 
> >  use_ozone=1 ozone_platform_x11=1
> > 
> > it won't work right? That's unfortunate because all the other platforms work
> > that way.
> 
> That configuration will use ozone-x11. What do you mean by "won't work"?

That it won't compile:

gn gen out_gn_ozone --args='os="chromeos" use_ozone=true ozone_platform_x11=true
use_goma=true'
ninja -C out_gn_ozone -j1000 -k50 chrome

[2/2] LINK ./chrome
FAILED: /usr/local/google/home/spang/goma/gomacc
../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings -m64
-fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -pthread
-B../third_party/binutils/Linux_x64/Release/bin -fuse-ld=gold -Wl,--icf=safe
-Wl,-rpath=\$ORIGIN/ -Wl,-rpath-link= -Wl,--disable-new-dtags -o ./chrome
-Wl,--start-group @./chrome.rsp ./libffmpegsumo.so -Wl,--end-group  -ldl
-lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0 -lglib-2.0 -ldbus-1
-lfontconfig -lgmodule-2.0 -lgthread-2.0 -lrt -lfreetype -lharfbuzz -lresolv
-lnss3 -lnssutil3 -lsmime3 -lplds4 -lplc4 -lnspr4 -lexpat -lX11 -lXcomposite
-lXcursor -lXdamage -lXext -lXfixes -lXi -lXrender -lXtst -ldrm -lasound -lcap
-lpam 
../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/ui/events/platform/x11/x11_events_platform.x11_event_source_glib.o: multiple
definition of 'ui::PlatformEventSource::CreateDefault()'
../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/ui/events/platform/platform.platform_event_source_stub.o: previous
definition here
../ui/platform_window/x11/x11_window.cc:77: error: undefined reference to
'ui::TouchFactory::SetTouchDeviceListFromCommandLine()'
../ui/events/platform/x11/x11_event_source.cc:82: error: undefined reference to
'ui::DeviceDataManagerX11::CreateInstance()'
../ui/events/platform/x11/x11_event_source.cc:134: error: undefined reference to
'ui::TranslateXEventToNativeEvent(_XEvent*)'
../ui/events/platform/x11/x11_event_source.cc:140: error: undefined reference to
'ui::UpdateDeviceList()'
../ui/events/platform/x11/x11_hotplug_event_handler.cc:353: error: undefined
reference to 'ui::DeviceListCacheX11::GetInstance()'
../ui/events/platform/x11/x11_hotplug_event_handler.cc:353: error: undefined
reference to 'ui::DeviceListCacheX11::GetXI2DeviceList(_XDisplay*)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: cannot make progress due to previous errors.

[achaulk, 2014-12-09 15:26:07]

On 2014/12/09 00:58:40, spang wrote:
> On 2014/12/08 16:33:43, achaulk wrote:
> > https://codereview.chromium.org/750593003/diff/100001/build/config/ui.gni
> > File build/config/ui.gni (right):
> > 
> >
>
https://codereview.chromium.org/750593003/diff/100001/build/config/ui.gni#new...
> > build/config/ui.gni:61: # specifies if X11 is used as a backend even if
> use_x11
> > is off.
> > On 2014/12/06 00:31:23, spang wrote:
> > > So if we just say 
> > > 
> > >  use_ozone=1 ozone_platform_x11=1
> > > 
> > > it won't work right? That's unfortunate because all the other platforms
work
> > > that way.
> > 
> > That configuration will use ozone-x11. What do you mean by "won't work"?
> 
> That it won't compile:
> 
> gn gen out_gn_ozone --args='os="chromeos" use_ozone=true
ozone_platform_x11=true
> use_goma=true'
> ninja -C out_gn_ozone -j1000 -k50 chrome
> 
> [2/2] LINK ./chrome
> FAILED: /usr/local/google/home/spang/goma/gomacc
> ../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings
-m64
> -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -pthread
> -B../third_party/binutils/Linux_x64/Release/bin -fuse-ld=gold -Wl,--icf=safe
> -Wl,-rpath=\$ORIGIN/ -Wl,-rpath-link= -Wl,--disable-new-dtags -o ./chrome
> -Wl,--start-group @./chrome.rsp ./libffmpegsumo.so -Wl,--end-group  -ldl
> -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0 -lglib-2.0 -ldbus-1
> -lfontconfig -lgmodule-2.0 -lgthread-2.0 -lrt -lfreetype -lharfbuzz -lresolv
> -lnss3 -lnssutil3 -lsmime3 -lplds4 -lplc4 -lnspr4 -lexpat -lX11 -lXcomposite
> -lXcursor -lXdamage -lXext -lXfixes -lXi -lXrender -lXtst -ldrm -lasound -lcap
> -lpam 
> ../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
> obj/ui/events/platform/x11/x11_events_platform.x11_event_source_glib.o:
multiple
> definition of 'ui::PlatformEventSource::CreateDefault()'
> ../third_party/binutils/Linux_x64/Release/bin/ld.gold:
> obj/ui/events/platform/platform.platform_event_source_stub.o: previous
> definition here
> ../ui/platform_window/x11/x11_window.cc:77: error: undefined reference to
> 'ui::TouchFactory::SetTouchDeviceListFromCommandLine()'
> ../ui/events/platform/x11/x11_event_source.cc:82: error: undefined reference
to
> 'ui::DeviceDataManagerX11::CreateInstance()'
> ../ui/events/platform/x11/x11_event_source.cc:134: error: undefined reference
to
> 'ui::TranslateXEventToNativeEvent(_XEvent*)'
> ../ui/events/platform/x11/x11_event_source.cc:140: error: undefined reference
to
> 'ui::UpdateDeviceList()'
> ../ui/events/platform/x11/x11_hotplug_event_handler.cc:353: error: undefined
> reference to 'ui::DeviceListCacheX11::GetInstance()'
> ../ui/events/platform/x11/x11_hotplug_event_handler.cc:353: error: undefined
> reference to 'ui::DeviceListCacheX11::GetXI2DeviceList(_XDisplay*)'
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)
> ninja: build stopped: cannot make progress due to previous errors.

Oh, I hadn't tried doing that before. I could make including x11 depend on
ozone_platform_x11 being set. Though that's kind of ugly to do at the top level
file

[spang, 2014-12-09 19:47:33]

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

https://codereview.chromium.org/750593003/diff/100001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:70: if (!config_) {
On 2014/12/08 16:44:41, achaulk wrote:
> On 2014/12/06 00:31:23, spang wrote:
> > We may want to delegate this whole config-choosing process and remove
> > GetEGLSurfaceProperties. It's clearly platform-specific in nontrivial ways.
> 
> It is, but it involves calling egl from within ozone, and I thought we were
> avoiding that?

We were mostly worried about #incuding chrome's version of the khronos headers
from ui/ozone/public/*.h because some platforms may need a modified header.

Since EGLConfig is just void* we could still do

class SurfaceOzoneEGL {
  virtual void* /* EGLConfig */ GetConfig() = 0;
};

We don't need to #include any headers for that.

It's definitely ugly but should work.. its a pain that EGL has these
platform-specific typedefs..

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
File ui/ozone/platform/x11/ozone_platform_x11.cc (right):

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
ui/ozone/platform/x11/ozone_platform_x11.cc:79: X11InputController
input_controller_;
On 2014/12/08 16:44:41, achaulk wrote:
> On 2014/12/06 00:31:23, spang wrote:
> > I don't think it makes sense to have an input controller in the GPU process.
> 
> It's just a stub for now. If it really is as simple as "return true for
> HasMouse" then it doesn't matter where it lives, otherwise it'll get moved
into
> the events tree

Would still prefer you put it in the scoped_ptr<> for consistency with all the
other parts that are only used from some processes.

I think eventually we should split the process-specific stuff into separate
objects altogether but for now we are just conditionally initializing parts of
this one.

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
File ui/ozone/platform/x11/x11_surface_factory.cc (right):

https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_surface_factory.cc:35: bool SupportsPartialSwap()
override { return false; }
On 2014/12/08 16:44:41, achaulk wrote:
> On 2014/12/06 00:31:23, spang wrote:
> > Not sure we should do this. Doing it this way breaks partial swap for
> everyone,
> > even if you have a working GLES driver.
> 
> I don't know how to determine if it is broken or not. The extension is
reported
> as present, it just doesn't work

Fixing this @ https://codereview.chromium.org/792623002/

[achaulk, 2014-12-15 18:23:40]

On 2014/12/09 19:47:33, spang wrote:
>
https://codereview.chromium.org/750593003/diff/100001/ui/gl/gl_surface_ozone.cc
> File ui/gl/gl_surface_ozone.cc (right):
> 
>
https://codereview.chromium.org/750593003/diff/100001/ui/gl/gl_surface_ozone....
> ui/gl/gl_surface_ozone.cc:70: if (!config_) {
> On 2014/12/08 16:44:41, achaulk wrote:
> > On 2014/12/06 00:31:23, spang wrote:
> > > We may want to delegate this whole config-choosing process and remove
> > > GetEGLSurfaceProperties. It's clearly platform-specific in nontrivial
ways.
> > 
> > It is, but it involves calling egl from within ozone, and I thought we were
> > avoiding that?
> 
> We were mostly worried about #incuding chrome's version of the khronos headers
> from ui/ozone/public/*.h because some platforms may need a modified header.
> 
> Since EGLConfig is just void* we could still do
> 
> class SurfaceOzoneEGL {
>   virtual void* /* EGLConfig */ GetConfig() = 0;
> };
> 
> We don't need to #include any headers for that.
> 
> It's definitely ugly but should work.. its a pain that EGL has these
> platform-specific typedefs..

Added callbacks and moved init to inside ozone

> 
>
https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
> File ui/ozone/platform/x11/ozone_platform_x11.cc (right):
> 
>
https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/o...
> ui/ozone/platform/x11/ozone_platform_x11.cc:79: X11InputController
> input_controller_;
> On 2014/12/08 16:44:41, achaulk wrote:
> > On 2014/12/06 00:31:23, spang wrote:
> > > I don't think it makes sense to have an input controller in the GPU
process.
> > 
> > It's just a stub for now. If it really is as simple as "return true for
> > HasMouse" then it doesn't matter where it lives, otherwise it'll get moved
> into
> > the events tree
> 
> Would still prefer you put it in the scoped_ptr<> for consistency with all the
> other parts that are only used from some processes.
> 
> I think eventually we should split the process-specific stuff into separate
> objects altogether but for now we are just conditionally initializing parts of
> this one.

Done
> 
>
https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
> File ui/ozone/platform/x11/x11_surface_factory.cc (right):
> 
>
https://codereview.chromium.org/750593003/diff/100001/ui/ozone/platform/x11/x...
> ui/ozone/platform/x11/x11_surface_factory.cc:35: bool SupportsPartialSwap()
> override { return false; }
> On 2014/12/08 16:44:41, achaulk wrote:
> > On 2014/12/06 00:31:23, spang wrote:
> > > Not sure we should do this. Doing it this way breaks partial swap for
> > everyone,
> > > even if you have a working GLES driver.
> > 
> > I don't know how to determine if it is broken or not. The extension is
> reported
> > as present, it just doesn't work
> 
> Fixing this @ https://codereview.chromium.org/792623002/

Removed extra API