Refactor the glib message-pump, and use it as the base for a gtk message pump and an X message pump.

Refactor the glib message-pump, and use it as the base for a gtk message pump and an X message pump.

The changes:
 * Rename MessagePumpGlibX to MessagePumpX.
 * Rename MessagePumpForUI to MessagePumpGlib.
 * Move some stuff out of MessagePumpGlib, and into MessagePumpGtk and MessagePumpX.
 * Rename MessagePumpForUI::Observer to MessageObserver, moved the platform-specific implementations into MessagePumpGtk and MessagePumpX. Ditto for MessagePumpForUI::Dispatcher.

MessagePumpX is independent of MessagePumpGtk. At the moment, MessagePumpX does process some GDK event, but once we have a complete native_widget_x, we can take out the GDK processing and things should continue to work.

BUG=none
TEST=existing message-pump tests.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90418

[sadrul, 2011-06-24 03:22:38]

Hi! This is the refactoring of the message-pump. I have kept it so MessagePumpX
is currently used only for touchui, but once native_widget_x is complete, we can
use it for ChromeOS too.

If the approach in this CL looks like what you had in mind, I will have evan@
take a look for a more thorough review.

[oshima, 2011-06-24 05:55:15]

thanks! looks mostly good. just a few minor comments.

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_glib.h
File base/message_pump_glib.h (right):

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_glib.h#new...
base/message_pump_glib.h:20: // MessagePumpObserver is notified prior to an
evebt  event being dispatched. As
an event

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_x.h
File base/message_pump_x.h (right):

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_x.h#newcode6
base/message_pump_x.h:6: #define BASE_MESSAGE_PUMP_X_H
We will need glib even after we remove gtk (for ibus/libcros), so GLIB_X still
makes sense to me. What do you think?

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_x.h#newcod...
base/message_pump_x.h:119: #define XLASTEvent 40
can you add compile time check if this value is good enough , may be at the end
of  .cc?

[sadrul, 2011-06-24 06:35:32]

@evan: Please take a look at the changes in base/.

@sky: Please take a look at the changes in chrome/, ui/ and views/ (is ui/views/
used for anything? Should it just be removed?)

@fischman: Please take a look at the change in media/

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_glib.h
File base/message_pump_glib.h (right):

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_glib.h#new...
base/message_pump_glib.h:20: // MessagePumpObserver is notified prior to an
evebt  event being dispatched. As
On 2011/06/24 05:55:15, oshima wrote:
> an event

Done.

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_x.h
File base/message_pump_x.h (right):

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_x.h#newcode6
base/message_pump_x.h:6: #define BASE_MESSAGE_PUMP_X_H
On 2011/06/24 05:55:15, oshima wrote:
> We will need glib even after we remove gtk (for ibus/libcros), so GLIB_X still
> makes sense to me. What do you think?

I changed the name because GlibX seems a bit too verbose for me, and I think
it's a good assumption that we will always be using glib, so having it in the
name doesn't convey any extra information anyway.

http://codereview.chromium.org/7250001/diff/2022/base/message_pump_x.h#newcod...
base/message_pump_x.h:119: #define XLASTEvent 40
On 2011/06/24 05:55:15, oshima wrote:
> can you add compile time check if this value is good enough , may be at the
end
> of  .cc?

Sounds like a good idea. Done!

[oshima, 2011-06-24 15:26:12]

LGTM with one nit.

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_glib.h
File base/message_pump_glib.h (right):

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_glib.h#new...
base/message_pump_glib.h:21: // Observers are notified of every change, they
have to be FAST!
Please mention that actual implementation of these class are provided in the
platform specific implementation.

[sky, 2011-06-24 15:27:12]

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_x.h
File base/message_pump_x.h (right):

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_x.h#newcode23
base/message_pump_x.h:23: virtual ~MessagePumpDispatcher() {}
If the pump doesn't own the dispatcher, make this protected. Same comment for
the Observer down below.

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_x.h#newcode35
base/message_pump_x.h:35: virtual DispatchStatus DispatchX(XEvent* xevent) = 0;
nit: DispatchX sounds pretty vague. How about DispatchXEvent? Or even just
Dispatch

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_x.h#newcode47
base/message_pump_x.h:47: virtual bool WillProcessXEvent(XEvent* xevent);
For clarity consider making this return an enum. In fact its probably worth
changing all the various observers to return an enum instead of bool. If you
agree, don't feel you need to change all observers now.

http://codereview.chromium.org/7250001/diff/1005/views/controls/menu/menu_con...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/7250001/diff/1005/views/controls/menu/menu_con...
views/controls/menu/menu_controller.cc:867: return exit_type_ != EXIT_NONE ?
How come you don't need logic like the other dispatches? Specifically handling
the key press/release?

http://codereview.chromium.org/7250001/diff/1005/views/controls/menu/nested_d...
File views/controls/menu/nested_dispatcher_gtk.cc (right):

http://codereview.chromium.org/7250001/diff/1005/views/controls/menu/nested_d...
views/controls/menu/nested_dispatcher_gtk.cc:42: if (creator_ != NULL)
nit: return creator_ && creator_->Dispatch(event);

[Ami GONE FROM CHROMIUM, 2011-06-24 15:55:02]

http://codereview.chromium.org/7250001/diff/1005/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7250001/diff/1005/media/media.gyp#newcode684
media/media.gyp:684: '../build/linux/system.gyp:gtk',
This looks strange to me, since none of the sources listed below for this target
actually explicitly use GTK/GLIB (or MessagePump, for that matter).
From a short glance through the trybots I'm guessing you made this change to
address:
http://build.chromium.org/p/tryserver.chromium/builders/linux_touch/builds/18...
wouldn't a more principled fix be to have this stanza in base/base.gypi:base,
where message_pump_glib_x.cc and friends are pulled into the build in the first
place?

(i.e. doing it the way you propose requires adding this stanza to every binary
that depends on base, no?  I'm not seeing what's special about omx_test that it
should be different in this regard)

[sadrul, 2011-06-24 17:14:01]

Addressed all comments from sky, fischman and oshima san.

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_glib.h
File base/message_pump_glib.h (right):

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_glib.h#new...
base/message_pump_glib.h:21: // Observers are notified of every change, they
have to be FAST!
On 2011/06/24 15:26:12, oshima wrote:
> Please mention that actual implementation of these class are provided in the
> platform specific implementation.

Done.

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_x.h
File base/message_pump_x.h (right):

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_x.h#newcode23
base/message_pump_x.h:23: virtual ~MessagePumpDispatcher() {}
On 2011/06/24 15:27:12, sky wrote:
> If the pump doesn't own the dispatcher, make this protected. Same comment for
> the Observer down below.

Done.

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_x.h#newcode35
base/message_pump_x.h:35: virtual DispatchStatus DispatchX(XEvent* xevent) = 0;
On 2011/06/24 15:27:12, sky wrote:
> nit: DispatchX sounds pretty vague. How about DispatchXEvent? Or even just
> Dispatch

I have changed it to Dispatch. (it used to be Dispatch, but then clang was upset
because the parent class had a 'Dispatch(GdkEvent*)')

http://codereview.chromium.org/7250001/diff/1005/base/message_pump_x.h#newcode47
base/message_pump_x.h:47: virtual bool WillProcessXEvent(XEvent* xevent);
On 2011/06/24 15:27:12, sky wrote:
> For clarity consider making this return an enum. In fact its probably worth
> changing all the various observers to return an enum instead of bool. If you
> agree, don't feel you need to change all observers now.

Sounds like a good idea. I have done it here. The GTK handlers don't have a
return value, so I have kept them as they are.

http://codereview.chromium.org/7250001/diff/1005/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7250001/diff/1005/media/media.gyp#newcode684
media/media.gyp:684: '../build/linux/system.gyp:gtk',
On 2011/06/24 15:55:02, Ami Fischman wrote:
> This looks strange to me, since none of the sources listed below for this
target
> actually explicitly use GTK/GLIB (or MessagePump, for that matter).

omx_test.cc uses the message_loop. Would it be a good idea to have this always
depend on 'base' instead? I have added this, and this also fixes the problem.

> From a short glance through the trybots I'm guessing you made this change to
> address:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_touch/builds/18...
> wouldn't a more principled fix be to have this stanza in base/base.gypi:base,
> where message_pump_glib_x.cc and friends are pulled into the build in the
first
> place?

I agree. This would definitely be the better way of doing things. (I honestly
didn't look into it too much since I noticed that omx_unittests does pull in
system.gyp:gtk explicitly, and this change will be removed anyway once we have
NativeWidgetX (soonish, http://codereview.chromium.org/7236030/))

> (i.e. doing it the way you propose requires adding this stanza to every binary
> that depends on base, no?  I'm not seeing what's special about omx_test that
it
> should be different in this regard)

http://codereview.chromium.org/7250001/diff/1005/views/controls/menu/menu_con...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/7250001/diff/1005/views/controls/menu/menu_con...
views/controls/menu/menu_controller.cc:867: return exit_type_ != EXIT_NONE ?
On 2011/06/24 15:27:12, sky wrote:
> How come you don't need logic like the other dispatches? Specifically handling
> the key press/release?

I think the other dispatches have to do something special for some events to
bypass the default behaviour of the native widgets (I do not honestly know)? In
our case, we always drop the events into a Widget/RootView, and let Views figure
out what to do with it. So we don't need (or at least, haven't needed so far)
any special handling of the events.

http://codereview.chromium.org/7250001/diff/1005/views/controls/menu/nested_d...
File views/controls/menu/nested_dispatcher_gtk.cc (right):

http://codereview.chromium.org/7250001/diff/1005/views/controls/menu/nested_d...
views/controls/menu/nested_dispatcher_gtk.cc:42: if (creator_ != NULL)
On 2011/06/24 15:27:12, sky wrote:
> nit: return creator_ && creator_->Dispatch(event);

Done.

[Ami GONE FROM CHROMIUM, 2011-06-24 17:18:36]

media/media.gyp LGTM 
(modulo that ISTM gyp style is to have local deps listed before other-directory
deps, so I'd put base after 'media' instead of before)

[sky, 2011-06-24 17:24:37]

views stuff LGTM

  -Scott

[Evan Martin, 2011-06-24 17:33:14]

base bits LGTM

http://codereview.chromium.org/7250001/diff/8027/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/7250001/diff/8027/base/base.gypi#newcode376
base/base.gypi:376: 'sources!' : [ 'message_pump_x.*', ],
I don't think gyp understands wildcards like this.

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_glib.h
File base/message_pump_glib.h (right):

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_glib.h#new...
base/message_pump_glib.h:34: // OS_LINUX platforms using GLib.
I guess to be clear it's any platform using glib, you could probably delete the
"OS_LINUX" part of this comment.

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h
File base/message_pump_gtk.h (right):

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h#newc...
base/message_pump_gtk.h:43: class MessagePumpGtk : public MessagePumpGlib {
Do these need BASE_API to not break the components build?

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h#newc...
base/message_pump_gtk.h:50: virtual void DispatchEvents(GdkEvent* event);
Why virtual? Do we actually override this anywhere?

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_x.h
File base/message_pump_x.h (right):

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_x.h#newcod...
base/message_pump_x.h:124: #endif
We now require GTK 2.18, so I think this code is obsolete.

...actually, I think we still have one buildbot not yet converted.  Don't worry
about it yet.

[sadrul, 2011-06-24 18:12:54]

Thanks!

@sky: Do I have an LGTM for chrome/ or should I add someone else for that part?

http://codereview.chromium.org/7250001/diff/8027/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/7250001/diff/8027/base/base.gypi#newcode376
base/base.gypi:376: 'sources!' : [ 'message_pump_x.*', ],
On 2011/06/24 17:33:15, Evan Martin wrote:
> I don't think gyp understands wildcards like this.

Indeed. Fixed.

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_glib.h
File base/message_pump_glib.h (right):

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_glib.h#new...
base/message_pump_glib.h:34: // OS_LINUX platforms using GLib.
On 2011/06/24 17:33:15, Evan Martin wrote:
> I guess to be clear it's any platform using glib, you could probably delete
the
> "OS_LINUX" part of this comment.

Done.

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h
File base/message_pump_gtk.h (right):

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h#newc...
base/message_pump_gtk.h:43: class MessagePumpGtk : public MessagePumpGlib {
On 2011/06/24 17:33:15, Evan Martin wrote:
> Do these need BASE_API to not break the components build?

It doesn't seem to be necessary at the moment, although it seems to be necessary
for message_pump_x. I am unsure if the difference makes sense.

http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h#newc...
base/message_pump_gtk.h:50: virtual void DispatchEvents(GdkEvent* event);
On 2011/06/24 17:33:15, Evan Martin wrote:
> Why virtual? Do we actually override this anywhere?

Not at the moment. Removed 'virtual'.

[sky, 2011-06-24 18:30:27]

On Fri, Jun 24, 2011 at 11:12 AM,  <sadrul@chromium.org> wrote:
> Thanks!
>
> @sky: Do I have an LGTM for chrome/ or should I add someone else for that
> part?

Yes.

  -Scott
>
>
> http://codereview.chromium.org/7250001/diff/8027/base/base.gypi
> File base/base.gypi (right):
>
> http://codereview.chromium.org/7250001/diff/8027/base/base.gypi#newcode376
> base/base.gypi:376: 'sources!' : [ 'message_pump_x.*', ],
> On 2011/06/24 17:33:15, Evan Martin wrote:
>>
>> I don't think gyp understands wildcards like this.
>
> Indeed. Fixed.
>
> http://codereview.chromium.org/7250001/diff/8027/base/message_pump_glib.h
> File base/message_pump_glib.h (right):
>
>
http://codereview.chromium.org/7250001/diff/8027/base/message_pump_glib.h#new...
> base/message_pump_glib.h:34: // OS_LINUX platforms using GLib.
> On 2011/06/24 17:33:15, Evan Martin wrote:
>>
>> I guess to be clear it's any platform using glib, you could probably
>
> delete the
>>
>> "OS_LINUX" part of this comment.
>
> Done.
>
> http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h
> File base/message_pump_gtk.h (right):
>
>
http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h#newc...
> base/message_pump_gtk.h:43: class MessagePumpGtk : public
> MessagePumpGlib {
> On 2011/06/24 17:33:15, Evan Martin wrote:
>>
>> Do these need BASE_API to not break the components build?
>
> It doesn't seem to be necessary at the moment, although it seems to be
> necessary for message_pump_x. I am unsure if the difference makes sense.
>
>
http://codereview.chromium.org/7250001/diff/8027/base/message_pump_gtk.h#newc...
> base/message_pump_gtk.h:50: virtual void DispatchEvents(GdkEvent*
> event);
> On 2011/06/24 17:33:15, Evan Martin wrote:
>>
>> Why virtual? Do we actually override this anywhere?
>
> Not at the moment. Removed 'virtual'.
>
> http://codereview.chromium.org/7250001/
>

[sadrul, 2011-06-24 18:36:31]

On 2011/06/24 18:30:27, sky wrote:
> On Fri, Jun 24, 2011 at 11:12 AM,  <mailto:sadrul@chromium.org> wrote:
> > Thanks!
> >
> > @sky: Do I have an LGTM for chrome/ or should I add someone else for that
> > part?
> 
> Yes.

Thanks!