Issue 1602173005: Add PlatformWindow/Event related code for Ozone X11.

kylechar

kylechar@chromium.org changed reviewers: + spang@chromium.org

4 years, 11 months ago (2016-01-19 21:51:24 UTC) #1

spang

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

4 years, 11 months ago (2016-01-20 19:53:30 UTC) #3

spang

Looks very promising! +sadrul especially for comments on how much code copying we can get ...

4 years, 11 months ago (2016-01-20 19:53:31 UTC) #4

kylechar

Added fixes for comments (except one) and moved XInput2/XKB event stuff into a separate CL ...

4 years, 11 months ago (2016-01-20 21:46:16 UTC) #5

Added fixes for comments (except one) and moved XInput2/XKB event stuff into a
separate CL to move shared code to ui/events/x.

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

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/o...
ui/ozone/platform/x11/ozone_platform_x11.cc:22: #include
"ui/ozone/public/ozone_platform.h"
On 2016/01/20 at 19:53:31, spang wrote:
> Keep the "// nogncheck" or "gn check" will complain about this #include (once
we fix the ime thing and get whitelisted for strict includes).

Done.

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/o...
ui/ozone/platform/x11/ozone_platform_x11.cc:72: return std::move(window);
On 2016/01/20 at 19:53:30, spang wrote:
> Return statements should ever have a std::move() because it can actually be
slower than just returning. It's not necessary anyway since there's a special
case in the language for this.

scoped_ptr<X11WindowHost> isn't converted to scoped_ptr<PlatformWindow> without
the std::move. I could store a scoped_ptr<PlatformWindow> and cast back to
X11WindowHost for SetBounds()/Create() but that seems messier?

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/x...
File ui/ozone/platform/x11/x11_event_factory.cc (right):

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_event_factory.cc:34: bool InitializeXInput2(XDisplay*
display) {
On 2016/01/20 at 19:53:31, spang wrote:
> These look really easy to share with the X11 build if we move them to a
utility file (maybe in the new //ui/events/x dir?).

Good idea! I'll move that into a different CL.

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_event_factory.cc:195: CHECK(display_);
On 2016/01/20 at 19:53:31, spang wrote:
> DCHECK for non-security assertions per chrome style guide.

Done.

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/x...
File ui/ozone/platform/x11/x11_window_host.h (right):

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_window_host.h:23: class X11WindowHost : public
PlatformWindow, public PlatformEventDispatcher {
On 2016/01/20 at 19:53:31, spang wrote:
> Suggest calling it X11WindowOzone, the "host" thing on platform/drm is to
disambiguate the two different classes that implement the window (DrmWindow in
gpu and DrmWindowHost in browser).
> 
> X11 will only have one class implementing the window so it can be consistent
with all the other PlatformWindows.
> 
> Except of course that we already have ui::X11Window. The goal should be to
just have one X11Window eventually, but having X11Window and X11WindowOzone for
now seems pragmatic for getting this landed.

Done.

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/x...
File ui/ozone/platform/x11/x11_window_host_manager.h (right):

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/x...
ui/ozone/platform/x11/x11_window_host_manager.h:23: class X11WindowHostManager :
public base::RefCounted<X11WindowHostManager> {
On 2016/01/20 at 19:53:31, spang wrote:
> X11WindowManager[Ozone] ?

Done.

spang

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/BUILD.gn File ui/ozone/platform/x11/BUILD.gn (right): https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/BUILD.gn#newcode45 ui/ozone/platform/x11/BUILD.gn:45: "//build/config/linux:glib", What's using glib? https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/ozone_platform_x11.cc File ui/ozone/platform/x11/ozone_platform_x11.cc (right): https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/ozone_platform_x11.cc#newcode72 ...

4 years, 11 months ago (2016-01-20 22:32:53 UTC) #6

kylechar

Description was changed from ========== Implement core of ozone_platform_x11. Add basic structure and event handling ...

4 years, 11 months ago (2016-01-21 00:07:33 UTC) #7

kylechar

https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/BUILD.gn File ui/ozone/platform/x11/BUILD.gn (right): https://codereview.chromium.org/1602173005/diff/20001/ui/ozone/platform/x11/BUILD.gn#newcode45 ui/ozone/platform/x11/BUILD.gn:45: "//build/config/linux:glib", On 2016/01/20 at 22:32:53, spang wrote: > What's ...

4 years, 11 months ago (2016-01-21 14:14:52 UTC) #8

sadrul

We are dup'ing way too much code. We shouldn't. https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_event_factory.cc File ui/ozone/platform/x11/x11_event_factory.cc (right): https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_event_factory.cc#newcode145 ui/ozone/platform/x11/x11_event_factory.cc:145: ...

4 years, 11 months ago (2016-01-21 14:53:01 UTC) #9

spang

https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_window_ozone.h File ui/ozone/platform/x11/x11_window_ozone.h (right): https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_window_ozone.h#newcode23 ui/ozone/platform/x11/x11_window_ozone.h:23: class X11WindowOzone : public PlatformWindow, public PlatformEventDispatcher { On ...

4 years, 11 months ago (2016-01-21 15:07:33 UTC) #10

kylechar

https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_event_factory.h File ui/ozone/platform/x11/x11_event_factory.h (right): https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_event_factory.h#newcode28 ui/ozone/platform/x11/x11_event_factory.h:28: class EVENTS_EXPORT X11EventFactory On 2016/01/21 at 14:53:01, sadrul wrote: ...

4 years, 11 months ago (2016-01-21 15:13:35 UTC) #11

spang

On 2016/01/21 15:13:35, kylechar wrote: > https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_event_factory.h > File ui/ozone/platform/x11/x11_event_factory.h (right): > > https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_event_factory.h#newcode28 > ...

4 years, 11 months ago (2016-01-21 15:27:52 UTC) #12

sadrul

On 2016/01/21 15:27:52, spang wrote: > On 2016/01/21 15:13:35, kylechar wrote: > > > https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x11_event_factory.h ...

4 years, 11 months ago (2016-01-22 15:30:54 UTC) #13

On 2016/01/21 15:27:52, spang wrote:
> On 2016/01/21 15:13:35, kylechar wrote:
> >
>
https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x...
> > File ui/ozone/platform/x11/x11_event_factory.h (right):
> > 
> >
>
https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x...
> > ui/ozone/platform/x11/x11_event_factory.h:28: class EVENTS_EXPORT
> > X11EventFactory
> > On 2016/01/21 at 14:53:01, sadrul wrote:
> > > Why can't we use X11EventSourceLibevent instead?
> > 
> > Similar X11Window the existing X11EventSourceLibevent implementation needs
> > PlatformEvent to be XEvent*. The new implementation needs PlatformEvent to
be
> > void*.
> 

Would it be possible to split out X11EventSource/X11Window in a way so we don't
have to replicate all of X11EventSource/X11Window for ozone, and instead
override just the PlatformEvent* related functions? (e.g. there could be a
X11EventSourceCommon, where 'X11EventSource : X11EventSourceCommon,
PlatformEventSource', and 'X11EventSourceOzone : X11EventSourceCommon,
PlatformEventSource'. Both of them override
X11EventSourceCommon::ProcessXEvent(XEvent*). X11EventSource directly dispatches
it using PlatformEventSource::DispatchEvent, X11EventSourceOzone creates a
ui::Event from the XEvent, and then dispatches it using
PlatformEventSource:DispatchEvent.

> It's more horrible than that, it needs it to be a ui::Event* cast to a void*;
we
> still have not cleaned this mess up. There is still some code left installing
> observers, dispatchers, and whatnot on PlatformEventSource, and expecting to
dig
> information out of PlatformEvent generically.
> 
> Unless you want to replicate the hacks we have for ozone in the x11 build, we
> have to have our own version of the window & event source.

So the plan is to dispatch ui::Events to
PlatformEventObserver/PlatformEventDispatchers. I started looking at what
PlatformEventObservers we currently have, and found these:

 1 ash::ScopedDisableInternalMouseAndKeyboardX11
 2 chromeos::SystemKeyEventListener
 3 chromeos::DeviceUMA
 4 remoting::LocalInputMonitorChromeos::Core
 5 aura::TouchEventCalibrate
 6 ui::SelectionChangeObserver
 7 ui::UserActivityDetector

The first three are chromeos-x11 specific. (there doesn't seem to be anything
equivalent for ozone. So perhaps we can just get rid of those?)

5 should just be removed. I think it can be removed right now (is there a way to
find out of the "touch-calibration" flag is used for any chromeos-x11 device?)

6 I believe is for linux-x11.

7 creates a ui::Event and processes that, and doesn't actually need to look at
the native event.

Similarly, PlatformEventDispatcher doesn't seem to be used a lot for chromeos.

So I am thinking we can probably 'use PlatformEvent = Event;' for
ozone-chromeos?

kylechar

On 2016/01/22 at 15:30:54, sadrul wrote: > On 2016/01/21 15:27:52, spang wrote: > > On ...

4 years, 11 months ago (2016-01-22 16:50:15 UTC) #14

On 2016/01/22 at 15:30:54, sadrul wrote:
> On 2016/01/21 15:27:52, spang wrote:
> > On 2016/01/21 15:13:35, kylechar wrote:
> > >
> >
https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x...
> > > File ui/ozone/platform/x11/x11_event_factory.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x...
> > > ui/ozone/platform/x11/x11_event_factory.h:28: class EVENTS_EXPORT
> > > X11EventFactory
> > > On 2016/01/21 at 14:53:01, sadrul wrote:
> > > > Why can't we use X11EventSourceLibevent instead?
> > > 
> > > Similar X11Window the existing X11EventSourceLibevent implementation needs
> > > PlatformEvent to be XEvent*. The new implementation needs PlatformEvent to
be
> > > void*.
> > 
> 
> Would it be possible to split out X11EventSource/X11Window in a way so we
don't have to replicate all of X11EventSource/X11Window for ozone, and instead
override just the PlatformEvent* related functions? (e.g. there could be a
X11EventSourceCommon, where 'X11EventSource : X11EventSourceCommon,
PlatformEventSource', and 'X11EventSourceOzone : X11EventSourceCommon,
PlatformEventSource'. Both of them override
X11EventSourceCommon::ProcessXEvent(XEvent*). X11EventSource directly dispatches
it using PlatformEventSource::DispatchEvent, X11EventSourceOzone creates a
ui::Event from the XEvent, and then dispatches it using
PlatformEventSource:DispatchEvent.

For X11Window that sounds reasonable. There is a lot of code for PlatformWindow
that is shared between X11WindowOzone and X11Window that could be pulled into a
separate class. So creating a X11WindowBase is doable. The biggest problem is
where to put X11WindowBase. The target //ui/platform_window/x11 indirectly
depends on //ui/ozone so without some major restructuring you can't put
X11WindowBase in //ui/platform_window/x11.

For X11EventFactory I'm not sure how much you'd gain by trying to factor out the
common stuff. The way X11EventSource is structured with X11EventSourceLibevent
built on top would need to be changed. There are some differences in how
X11EventFactory and X11EventSource handle things. I guess pulling some of the
common functions could work but it would be a couple of 5-10 line methods that
are pulled out.

> > It's more horrible than that, it needs it to be a ui::Event* cast to a
void*; we
> > still have not cleaned this mess up. There is still some code left
installing
> > observers, dispatchers, and whatnot on PlatformEventSource, and expecting to
dig
> > information out of PlatformEvent generically.
> > 
> > Unless you want to replicate the hacks we have for ozone in the x11 build,
we
> > have to have our own version of the window & event source.
> 
> So the plan is to dispatch ui::Events to
PlatformEventObserver/PlatformEventDispatchers. I started looking at what
PlatformEventObservers we currently have, and found these:
> 
>  1 ash::ScopedDisableInternalMouseAndKeyboardX11
>  2 chromeos::SystemKeyEventListener
>  3 chromeos::DeviceUMA
>  4 remoting::LocalInputMonitorChromeos::Core
>  5 aura::TouchEventCalibrate
>  6 ui::SelectionChangeObserver
>  7 ui::UserActivityDetector
> 
> The first three are chromeos-x11 specific. (there doesn't seem to be anything
equivalent for ozone. So perhaps we can just get rid of those?)
> 
> 5 should just be removed. I think it can be removed right now (is there a way
to find out of the "touch-calibration" flag is used for any chromeos-x11
device?)
> 
> 6 I believe is for linux-x11.
> 
> 7 creates a ui::Event and processes that, and doesn't actually need to look at
the native event.
> 
> Similarly, PlatformEventDispatcher doesn't seem to be used a lot for chromeos.
> 
> So I am thinking we can probably 'use PlatformEvent = Event;' for
ozone-chromeos?

I'll leave commenting on this to spang.

spang

On 2016/01/22 16:50:15, kylechar wrote: > On 2016/01/22 at 15:30:54, sadrul wrote: > > On ...

4 years, 11 months ago (2016-01-26 00:13:00 UTC) #15

On 2016/01/22 16:50:15, kylechar wrote:
> On 2016/01/22 at 15:30:54, sadrul wrote:
> > On 2016/01/21 15:27:52, spang wrote:
> > > On 2016/01/21 15:13:35, kylechar wrote:
> > > >
> > >
>
https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x...
> > > > File ui/ozone/platform/x11/x11_event_factory.h (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x...
> > > > ui/ozone/platform/x11/x11_event_factory.h:28: class EVENTS_EXPORT
> > > > X11EventFactory
> > > > On 2016/01/21 at 14:53:01, sadrul wrote:
> > > > > Why can't we use X11EventSourceLibevent instead?
> > > > 
> > > > Similar X11Window the existing X11EventSourceLibevent implementation
needs
> > > > PlatformEvent to be XEvent*. The new implementation needs PlatformEvent
to
> be
> > > > void*.
> > > 
> > 
> > Would it be possible to split out X11EventSource/X11Window in a way so we
> don't have to replicate all of X11EventSource/X11Window for ozone, and instead
> override just the PlatformEvent* related functions? (e.g. there could be a
> X11EventSourceCommon, where 'X11EventSource : X11EventSourceCommon,
> PlatformEventSource', and 'X11EventSourceOzone : X11EventSourceCommon,
> PlatformEventSource'. Both of them override
> X11EventSourceCommon::ProcessXEvent(XEvent*). X11EventSource directly
dispatches
> it using PlatformEventSource::DispatchEvent, X11EventSourceOzone creates a
> ui::Event from the XEvent, and then dispatches it using
> PlatformEventSource:DispatchEvent.
> 
> For X11Window that sounds reasonable. There is a lot of code for
PlatformWindow
> that is shared between X11WindowOzone and X11Window that could be pulled into
a
> separate class. So creating a X11WindowBase is doable. The biggest problem is
> where to put X11WindowBase. The target //ui/platform_window/x11 indirectly
> depends on //ui/ozone so without some major restructuring you can't put
> X11WindowBase in //ui/platform_window/x11.
> 
> For X11EventFactory I'm not sure how much you'd gain by trying to factor out
the
> common stuff. The way X11EventSource is structured with X11EventSourceLibevent
> built on top would need to be changed. There are some differences in how
> X11EventFactory and X11EventSource handle things. I guess pulling some of the
> common functions could work but it would be a couple of 5-10 line methods that
> are pulled out.
> 
> > > It's more horrible than that, it needs it to be a ui::Event* cast to a
> void*; we
> > > still have not cleaned this mess up. There is still some code left
> installing
> > > observers, dispatchers, and whatnot on PlatformEventSource, and expecting
to
> dig
> > > information out of PlatformEvent generically.
> > > 
> > > Unless you want to replicate the hacks we have for ozone in the x11 build,
> we
> > > have to have our own version of the window & event source.
> > 
> > So the plan is to dispatch ui::Events to
> PlatformEventObserver/PlatformEventDispatchers. I started looking at what
> PlatformEventObservers we currently have, and found these:
> > 
> >  1 ash::ScopedDisableInternalMouseAndKeyboardX11
> >  2 chromeos::SystemKeyEventListener
> >  3 chromeos::DeviceUMA
> >  4 remoting::LocalInputMonitorChromeos::Core
> >  5 aura::TouchEventCalibrate
> >  6 ui::SelectionChangeObserver
> >  7 ui::UserActivityDetector
> > 
> > The first three are chromeos-x11 specific. (there doesn't seem to be
anything
> equivalent for ozone. So perhaps we can just get rid of those?)

(1) is replaced by InputController:SetInternal*Disabled(), so can be ignored.
I'm not sure if nyan needs this (depends on if any of them have the touch view
mode), but if not, it can simply be removed.

(2) was not needed with platform/drm since we don't implement num lock at a low
level, so it never gets turned on in the first place. We don't have to care
about it in the ozone build, although removing it from cros-x11 would probably
regress nyan.

(3) We never implemented it, and nobody complained. So I guess we can remove it.

> > 
> > 5 should just be removed. I think it can be removed right now (is there a
way
> to find out of the "touch-calibration" flag is used for any chromeos-x11
> device?)

Only used on Pixel 1, which ships the ozone path. So yes, it can be removed.

Kyle: Want to prepare a patch that removes it? We still need the ozone version
of it but that's in a different file.

> > 
> > 6 I believe is for linux-x11.
> > 
> > 7 creates a ui::Event and processes that, and doesn't actually need to look
at
> the native event.
> > 
> > Similarly, PlatformEventDispatcher doesn't seem to be used a lot for
chromeos.
> > 
> > So I am thinking we can probably 'use PlatformEvent = Event;' for
> ozone-chromeos?
> 
> I'll leave commenting on this to spang.

NativeViewGLSurfaceGLX also requires PlatformEvent = XEvent* to listen for
expose events, which cannot be represented as a ui::Event, so we'd need to
figure that one out..

kylechar

I've uploaded a draft CL with all the small items fixed. It runs fine but ...

4 years, 11 months ago (2016-01-26 15:24:25 UTC) #16

spang

On 2016/01/26 15:24:25, kylechar wrote: > I've uploaded a draft CL with all the small ...

4 years, 11 months ago (2016-01-26 18:30:40 UTC) #17

On 2016/01/26 15:24:25, kylechar wrote:
> I've uploaded a draft CL with all the small items fixed. It runs fine but
there
> are lots of potential UB with passing around XEvents to
> PlatfromEventDispatcher/PlatfromEventObservers (which might expect ui::Events)
> that require some other solution. So that needs to be decided on first and
then
> I can make the necessary changes before event handling parts are reviewed.

We should probably keep dispatching ui::Event*, not XEvent, because the UB issue
makes this not landable; EventFromNative() will just crash. EventFromNative()
needs to work, unless we rewire everything that uses it such as
MenuEventDispatcher::DispatchEvent().

For NativeViewGLSurfaceGLX, we could either add something custom or create a new
dispatcher type that always uses XEvent* rather than PlatformEvent. Expose can't
even be represented as ui::Event, so there's really no issue with dispatching
expose in an earlier phase than PlatformEventSource.

It would mean we had XEvent* dispatchers (limited to X11Window + GLX) and
PlatformEvent (==ui::Event*) dispatchers (for everything else), and the former
runs first.

It's complicated, but I'm not sure what else to do.

Maybe we really should just do the high road path. IMO the high road is for
there to be no such thing as PlatformEventSource. There'd still be
X11EventSource, but it'd only be used called by other x11-specific code such as
X11Window and possibly NativeViewGLSurfaceGLX.

The current users of PlatformEventSource would have to instead hook into higher
level event dispatch (at the very least inside
PlatformWindowDelegate::DispatchEvent). For example menus could enable capture,
thus ensuring all events reach their owning PlatformWindow, and then the window
delegate can dispatch those events to the menu.

It seems like either way, we need a dispatcher that always uses XEvent* just to
implement X11Window, so we could view that as a step along the way.

> 
>
https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x...
> File ui/ozone/platform/x11/x11_window_manager_ozone.h (right):
> 
>
https://codereview.chromium.org/1602173005/diff/40001/ui/ozone/platform/x11/x...
> ui/ozone/platform/x11/x11_window_manager_ozone.h:23: class
X11WindowManagerOzone
> : public base::RefCounted<X11WindowManagerOzone> {
> On 2016/01/21 at 14:53:01, sadrul wrote:
> > Why does this need to be refcounted?
> 
> For the callbacks. I think X11WindowManagerOzone should live longer than any
> X11WindowOzone instances so it's probably safe to not have it ref counted.
I've
> changed that.

kylechar

I've uploaded a new patch that dispatches ui::Event using the existing infrastructure and adds a ...

4 years, 11 months ago (2016-01-26 20:27:25 UTC) #18

spang

Couple of comments, but let's get some feedback from sadrul before getting too deep into ...

4 years, 11 months ago (2016-01-26 21:37:40 UTC) #19

kylechar

https://codereview.chromium.org/1602173005/diff/80001/ui/events/platform/x11/x11_event_source.cc File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1602173005/diff/80001/ui/events/platform/x11/x11_event_source.cc#newcode98 ui/events/platform/x11/x11_event_source.cc:98: CHECK(dispatcher); On 2016/01/26 at 21:37:40, spang wrote: > CHECK ...

4 years, 11 months ago (2016-01-27 15:39:26 UTC) #20

kylechar

After talking with Sadrul I've split X11EventSource and X11Window into a two classes each. X11EventSource ...

4 years, 10 months ago (2016-02-01 16:44:28 UTC) #22

sadrul

Some comments about the X11Window changes. Looking at the X11EventSource changes now. https://codereview.chromium.org/1602173005/diff/120001/ui/platform_window/x11/x11_window_base.h File ui/platform_window/x11/x11_window_base.h ...

4 years, 10 months ago (2016-02-02 16:33:54 UTC) #23

kylechar

I've made the fixes requested. I'll wait for the rest of your comments before uploading ...

4 years, 10 months ago (2016-02-02 19:01:42 UTC) #24

kylechar

Description was changed from ========== Implement core of ozone_platform_x11. Add basic structure and event handling ...

4 years, 10 months ago (2016-02-04 16:10:53 UTC) #25

kylechar

I've uploaded some newer patches with fixes for the comments thus far and a few ...

4 years, 10 months ago (2016-02-04 16:12:10 UTC) #26

sadrul

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11/x11_event_source.cc File ui/events/platform/x11/x11_event_source.cc (left): https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11/x11_event_source.cc#oldcode89 ui/events/platform/x11/x11_event_source.cc:89: // static keep this https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11/x11_event_source_ozone.cc File ui/events/platform/x11/x11_event_source_ozone.cc (right): https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11/x11_event_source_ozone.cc#newcode148 ...

4 years, 10 months ago (2016-02-04 22:29:32 UTC) #27

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
File ui/events/platform/x11/x11_event_source.cc (left):

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
ui/events/platform/x11/x11_event_source.cc:89: // static
keep this

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
File ui/events/platform/x11/x11_event_source_ozone.cc (right):

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
ui/events/platform/x11/x11_event_source_ozone.cc:148: XEvent* xevent =
static_cast<XEvent*>(native_event);
In ozone, NativeEvent is a ui::Event, and not an XEvent, right? So this is not
right (It happens to be correct in this case since we know we are getting an
X-event here, but we shouldn't do this).

My proposal is: X11EventSource is no longer a PlatformEventSource, and it should
have a delegate:

  class X11EventSourceDelegate { void ProcessXEvent(XEvent*) = 0; }

  class X11EventSource {
    X11EventSource(XDisplay*, X11EventSourceDelegate*);
    X11EventSourceDelegate* delegate_;
  };

  X11EventSource::ExtractCookieDataDispatchEvent(...) {
    ...
    delegate_->ProcessXEvent(xevent);
    ...
  }

X11EventSourceGlib becomes the PES, and the delegate for the XES:

  class X11EventSourceGlib : PlatformEventSource, X11EventSourceDelegate {
    ...
  }

  X11EventSourceGlib::ProcessXEvent(XEvent* xevent) {
    DispatchEvent(xevent);
  }

  X11EventSourceGlib::StopCurrentEventStream() {
    x11_source_->StopCurrentEventStream();
  }

  X11EventSourceGlib::OnDispatcherListChanged() {
    x11_source_->OnDispatcherListChanged();
  }

Similarly,

  class X11EventSourceOzone : PlatformEventSource, X11EventSourceDelegate {
    ...
  }

  X11EventSourceOzone::ProcessXEvent(XEvent* xevent) {
    scoped_ptr<ui::Event> ui_event = TranslateXEventToEvent(*xevent);
    if (ui_event) {
       DispatchEvent(ui_event.get());
    } else {
       DispatchXEventToXEventDispatchers(xevent);
    }
  }

  X11EventSourceOzone::StopCurrentEventStream() {
    x11_source_->StopCurrentEventStream();
  }

  X11EventSourceOzone::OnDispatcherListChanged() {
    x11_source_->OnDispatcherListChanged();
  }

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
File ui/events/platform/x11/x11_event_source_ozone.h (right):

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
ui/events/platform/x11/x11_event_source_ozone.h:33: class EVENTS_EXPORT
X11EventSourceOzone
Actually, can you continue to call this X11EventSourceLibevent? (we should be
able to use this as long as the libevent message-pump is used, regardless of
whether ozone is turned on or not)

kylechar

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11/x11_event_source.cc File ui/events/platform/x11/x11_event_source.cc (left): https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11/x11_event_source.cc#oldcode89 ui/events/platform/x11/x11_event_source.cc:89: // static On 2016/02/04 at 22:29:32, sadrul wrote: > ...

4 years, 10 months ago (2016-02-05 18:21:46 UTC) #29

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
File ui/events/platform/x11/x11_event_source.cc (left):

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
ui/events/platform/x11/x11_event_source.cc:89: // static
On 2016/02/04 at 22:29:32, sadrul wrote:
> keep this

Done.

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
File ui/events/platform/x11/x11_event_source_ozone.cc (right):

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
ui/events/platform/x11/x11_event_source_ozone.cc:148: XEvent* xevent =
static_cast<XEvent*>(native_event);
On 2016/02/04 at 22:29:32, sadrul wrote:
> In ozone, NativeEvent is a ui::Event, and not an XEvent, right? So this is not
right (It happens to be correct in this case since we know we are getting an
X-event here, but we shouldn't do this).
> 
> My proposal is: X11EventSource is no longer a PlatformEventSource, and it
should have a delegate:
> 
>   class X11EventSourceDelegate { void ProcessXEvent(XEvent*) = 0; }
> 
>   class X11EventSource {
>     X11EventSource(XDisplay*, X11EventSourceDelegate*);
>     X11EventSourceDelegate* delegate_;
>   };
> 
>   X11EventSource::ExtractCookieDataDispatchEvent(...) {
>     ...
>     delegate_->ProcessXEvent(xevent);
>     ...
>   }
> 
> X11EventSourceGlib becomes the PES, and the delegate for the XES:
> 
>   class X11EventSourceGlib : PlatformEventSource, X11EventSourceDelegate {
>     ...
>   }
> 
>   X11EventSourceGlib::ProcessXEvent(XEvent* xevent) {
>     DispatchEvent(xevent);
>   }
> 
>   X11EventSourceGlib::StopCurrentEventStream() {
>     x11_source_->StopCurrentEventStream();
>   }
> 
>   X11EventSourceGlib::OnDispatcherListChanged() {
>     x11_source_->OnDispatcherListChanged();
>   }
> 
> Similarly,
> 
>   class X11EventSourceOzone : PlatformEventSource, X11EventSourceDelegate {
>     ...
>   }
> 
>   X11EventSourceOzone::ProcessXEvent(XEvent* xevent) {
>     scoped_ptr<ui::Event> ui_event = TranslateXEventToEvent(*xevent);
>     if (ui_event) {
>        DispatchEvent(ui_event.get());
>     } else {
>        DispatchXEventToXEventDispatchers(xevent);
>     }
>   }
> 
>   X11EventSourceOzone::StopCurrentEventStream() {
>     x11_source_->StopCurrentEventStream();
>   }
> 
>   X11EventSourceOzone::OnDispatcherListChanged() {
>     x11_source_->OnDispatcherListChanged();
>   }

I avoided this initially because of how it expands the scope/complexity of the
CL (other classes assume X11EventSource is a PlatformEventSource) but will work
fine. I've implemented the delegate now.

It required adding another singleton for X11EventSource. This is so that other
classes can call BlockUntilWindowMapped() on it (the existing static
X11EventSource::GetInstance() relied on it being a PES). I've also updated other
classes that were registering PlatformEventDispatchers with the X11EventSource
instance and changed them to register PEDs with PlatformEventSource instance
instead.

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
File ui/events/platform/x11/x11_event_source_ozone.h (right):

https://codereview.chromium.org/1602173005/diff/160001/ui/events/platform/x11...
ui/events/platform/x11/x11_event_source_ozone.h:33: class EVENTS_EXPORT
X11EventSourceOzone
On 2016/02/04 at 22:29:32, sadrul wrote:
> Actually, can you continue to call this X11EventSourceLibevent? (we should be
able to use this as long as the libevent message-pump is used, regardless of
whether ozone is turned on or not)

Done.

kylechar

Description was changed from ========== Implement core of ozone_platform_x11. Add core classes for ozone_platform_x11. These ...

4 years, 10 months ago (2016-02-08 16:43:18 UTC) #30

Description was changed from

==========
Implement core of ozone_platform_x11.

Add core classes for ozone_platform_x11. These classes are mostly
boilerplate at this point but will compile.

The bigger changes here are to X11EventSource and X11Window. For both
classes the current implementation is for PlatformEvent=XEvent* but with
Ozone PlatformEvent=void*=ui::Event*. Most of the implementation will be
the same between X11 and Ozone X11 otherwise. Both classes have been split
into a shared base class and compile specific implementation.

X11EventSource is now the base class. There were two possible listener
implementations, Glib vs Libevent, with Libevent being unused so far. The
X11 implementation is X11EventSourceGlib and uses Glib. The Ozone X11
implementation is X11EventSourceOzone and uses Libevent. Since the Ozone
X11 implementation is dispatching ui::Events there is a separate dispatcher
list for XEventDispatchers added to send XEvents where necessary.

X11WindowBase is now the base class. The X11 implementation is still called
X11Window. The X11 Ozone implementation is X11WindowOzone. The X11 Ozone
version registers itself as both a PlatformEventDispatcher and
XEventDispatcher.

Compile with the following gn args:
use_ozone = true
ozone_platform_x11 = true
target_os = "chromeos"

BUG=361137
==========

to

==========
Add PlatformWindow/Event related code for Ozone X11.

The bigger changes here are to X11EventSource and X11Window. For both
classes the current implementation is for PlatformEvent=XEvent* but with
Ozone PlatformEvent=void*=ui::Event*. Most of the implementation will be
the same between X11 and Ozone X11 otherwise. Both classes have been split
into a shared base class and compile specific implementation.

X11EventSource is now the base class. There were two possible listener
implementations, Glib vs Libevent, with Libevent being unused so far. The
X11 implementation is X11EventSourceGlib and uses Glib. The Ozone X11
implementation is X11EventSourceOzone and uses Libevent. Since the Ozone
X11 implementation is dispatching ui::Events there is a separate dispatcher
list for XEventDispatchers added to send XEvents where necessary.

X11WindowBase is now the base class. The X11 implementation is still called
X11Window. The X11 Ozone implementation is X11WindowOzone. The X11 Ozone
version registers itself as both a PlatformEventDispatcher and
XEventDispatcher.

Compile with the following gn args:
use_ozone = true
ozone_platform_x11 = true
target_os = "chromeos"

BUG=361137
==========

sadrul

I think we are really close! Just a few more comments and nits. *exciting*! https://codereview.chromium.org/1602173005/diff/200001/ui/events/platform/x11/BUILD.gn ...

4 years, 10 months ago (2016-02-08 17:28:33 UTC) #31

kylechar

I've added fixes for all of the comments in the latest round. https://codereview.chromium.org/1602173005/diff/200001/ui/events/platform/x11/BUILD.gn File ui/events/platform/x11/BUILD.gn ...

4 years, 10 months ago (2016-02-08 19:08:10 UTC) #32

sadrul

A small nit, but other than that, ui/aura/ ui/events/ ui/platform_window/ lgtm!! I will leave ui/ozone ...

4 years, 10 months ago (2016-02-09 19:34:40 UTC) #35

kylechar

Description was changed from ========== Add PlatformWindow/Event related code for Ozone X11. The bigger changes ...

4 years, 10 months ago (2016-02-09 21:30:18 UTC) #37

Description was changed from

==========
Add PlatformWindow/Event related code for Ozone X11.

The bigger changes here are to X11EventSource and X11Window. For both
classes the current implementation is for PlatformEvent=XEvent* but with
Ozone PlatformEvent=void*=ui::Event*. Most of the implementation will be
the same between X11 and Ozone X11 otherwise. Both classes have been split
into a shared base class and compile specific implementation.

X11EventSource is now the base class. There were two possible listener
implementations, Glib vs Libevent, with Libevent being unused so far. The
X11 implementation is X11EventSourceGlib and uses Glib. The Ozone X11
implementation is X11EventSourceOzone and uses Libevent. Since the Ozone
X11 implementation is dispatching ui::Events there is a separate dispatcher
list for XEventDispatchers added to send XEvents where necessary.

X11WindowBase is now the base class. The X11 implementation is still called
X11Window. The X11 Ozone implementation is X11WindowOzone. The X11 Ozone
version registers itself as both a PlatformEventDispatcher and
XEventDispatcher.

Compile with the following gn args:
use_ozone = true
ozone_platform_x11 = true
target_os = "chromeos"

BUG=361137
==========

to

==========
Add PlatformWindow/Event related code for Ozone X11.

The bigger changes here are to X11EventSource and X11Window. For both
classes the current implementation is for PlatformEvent=XEvent* but with
Ozone PlatformEvent=void*=ui::Event*. Most of the implementation will be
the same between X11 and Ozone X11 otherwise. Both classes have been split
into a shared base class and compile specific implementation.

X11EventSource is now the base class. There were two possible listener
implementations, Glib vs Libevent, with Libevent being unused so far. The
X11 implementation is X11EventSourceGlib and uses Glib. The Ozone X11
implementation is X11EventSourceLibevent and uses Libevent. Since the Ozone
X11 implementation is dispatching ui::Events there is a separate dispatcher
list for XEventDispatchers added to send XEvents where necessary.

X11WindowBase is now the base class. The X11 implementation is still called
X11Window. The X11 Ozone implementation is X11WindowOzone. The X11 Ozone
version registers itself as both a PlatformEventDispatcher and
XEventDispatcher.

Compile with the following gn args:
use_ozone = true
ozone_platform_x11 = true
target_os = "chromeos"

TBR=rockot@chromium.org
BUG=361137
==========

kylechar

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, spang@chromium.org Link to ...

4 years, 10 months ago (2016-02-09 21:30:46 UTC) #39

kylechar

Thanks! Changes made for the last comments and two more X11EventSource -> X11EventSourceGlib changes were ...

4 years, 10 months ago (2016-02-09 21:31:54 UTC) #40

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-09 21:34:51 UTC) #41

commit-bot: I haz the power

Description was changed from ========== Add PlatformWindow/Event related code for Ozone X11. The bigger changes ...

4 years, 10 months ago (2016-02-09 22:39:59 UTC) #43

Message was sent while issue was closed.

Description was changed from

==========
Add PlatformWindow/Event related code for Ozone X11.

The bigger changes here are to X11EventSource and X11Window. For both
classes the current implementation is for PlatformEvent=XEvent* but with
Ozone PlatformEvent=void*=ui::Event*. Most of the implementation will be
the same between X11 and Ozone X11 otherwise. Both classes have been split
into a shared base class and compile specific implementation.

X11EventSource is now the base class. There were two possible listener
implementations, Glib vs Libevent, with Libevent being unused so far. The
X11 implementation is X11EventSourceGlib and uses Glib. The Ozone X11
implementation is X11EventSourceLibevent and uses Libevent. Since the Ozone
X11 implementation is dispatching ui::Events there is a separate dispatcher
list for XEventDispatchers added to send XEvents where necessary.

X11WindowBase is now the base class. The X11 implementation is still called
X11Window. The X11 Ozone implementation is X11WindowOzone. The X11 Ozone
version registers itself as both a PlatformEventDispatcher and
XEventDispatcher.

Compile with the following gn args:
use_ozone = true
ozone_platform_x11 = true
target_os = "chromeos"

TBR=rockot@chromium.org
BUG=361137
==========

to

==========
Add PlatformWindow/Event related code for Ozone X11.

The bigger changes here are to X11EventSource and X11Window. For both
classes the current implementation is for PlatformEvent=XEvent* but with
Ozone PlatformEvent=void*=ui::Event*. Most of the implementation will be
the same between X11 and Ozone X11 otherwise. Both classes have been split
into a shared base class and compile specific implementation.

X11EventSource is now the base class. There were two possible listener
implementations, Glib vs Libevent, with Libevent being unused so far. The
X11 implementation is X11EventSourceGlib and uses Glib. The Ozone X11
implementation is X11EventSourceLibevent and uses Libevent. Since the Ozone
X11 implementation is dispatching ui::Events there is a separate dispatcher
list for XEventDispatchers added to send XEvents where necessary.

X11WindowBase is now the base class. The X11 implementation is still called
X11Window. The X11 Ozone implementation is X11WindowOzone. The X11 Ozone
version registers itself as both a PlatformEventDispatcher and
XEventDispatcher.

Compile with the following gn args:
use_ozone = true
ozone_platform_x11 = true
target_os = "chromeos"

TBR=rockot@chromium.org
BUG=361137

Committed: https://crrev.com/456fe8e2a64b676531e9d4f24a5268e2523a4d22
Cr-Commit-Position: refs/heads/master@{#374502}
==========

commit-bot: I haz the power

Patchset 12 (id:??) landed as https://crrev.com/456fe8e2a64b676531e9d4f24a5268e2523a4d22 Cr-Commit-Position: refs/heads/master@{#374502}

4 years, 10 months ago (2016-02-09 22:40:00 UTC) #44

kylechar

kylechar@chromium.org changed reviewers: + rockot@chromium.org

4 years, 10 months ago (2016-02-09 22:41:45 UTC) #45

kylechar

Hi rockot@, can you look at the 3 modified lines in chrome/browser/extensions/global_shortcut_listener_x11.cc as TBR? They ...

4 years, 10 months ago (2016-02-09 22:44:32 UTC) #46

Message was sent while issue was closed.

lgtm

Issue 1602173005: Add PlatformWindow/Event related code for Ozone X11. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Small fixes. #

Patch Set 3 : Fixes for comments from spang. #

Patch Set 4 : Intermediate CL. #

Patch Set 5 : XEventDispatcher added. #

Patch Set 6 : Split X11EventSource and X11Window. #

Patch Set 7 : Fixes for comments. #

Patch Set 8 : Fix deps problem. #

Patch Set 9 : Fixes for comments. #

Patch Set 10 : More fixes for comments. #

Patch Set 11 : Rebase. #

Patch Set 12 : Final fixes for comments + tests. #

Messages