Fixing the issue of GDK discarding unsupported XInput events.

Fixing the issue of GDK discarding unsupported XInput events.

Current implementation of message_pump_glib_x uses both X event queue and GDK's g_main_context_iteration to read event messages.

If the first X event in the X event queue can be dispatched directly, it will be extracted and dispatched.
If the first X event in the X event queue can't be dispatched directly, g_main_context_iteration will be called to translate X events into GDK evnets.
If the first X event can't be dispatched directly and is followed by X events that can be dispatched directly in the event queue,
those events that can be dispatched directly will also go through GDK and may get lost if GDK does not recognize them (e.g. XInput2 events). 

To fix this, a GDK event filter is setup to intercept all the X events before they are translated into GDK events.
In the filter if the X events are found to be wanted, they will get dispatched directly instead of translated into GDK events. 

BUG=
TEST=

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

[sadrul, 2011-05-20 00:32:36]

Aha, so using the GDK filter function does fix the menu-cancellation bug?

[sadrul, 2011-05-24 15:34:18]

Some nits. Please add evan@ when you address them.

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:36: gdk_window_add_filter(NULL, GdkEventFilter,
this);
To keep with the existing code, '&GdkEventFilter'

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:104: should_quit = ProcessXEvent(&xev);
Using should_quit isn't necessary anymore: 
  if (ProcessXEvent(&xev))
    return true;

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:145: pump->ProcessXEvent(xev);
This looks good as is. But should this do anything with the return value
(probably not)?

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:152: 
Too many new lines!

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:179: DLOG(WARNING) << "GDK received an event it
shouldn't have";
Can you please replace the DLOG with a NOTREACHED here, and see if that causes
any problems? If not, get rid of the TODO too. From what I understand, such an
event should never reach here anymore?

[Yufeng Shen (Slow to review), 2011-05-24 18:30:50]

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:36: gdk_window_add_filter(NULL, GdkEventFilter,
this);
On 2011/05/24 15:34:18, sadrul wrote:
> To keep with the existing code, '&GdkEventFilter'

Done.

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:104: should_quit = ProcessXEvent(&xev);
On 2011/05/24 15:34:18, sadrul wrote:
> Using should_quit isn't necessary anymore: 
>   if (ProcessXEvent(&xev))
>     return true;

Done.

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:145: pump->ProcessXEvent(xev);
On 2011/05/24 15:34:18, sadrul wrote:
> This looks good as is. But should this do anything with the return value
> (probably not)?

Since it is in the GDK's message processing loop, I don't know if there is a way
to break the loop right here if the ProcessXEvent(xev) return true (meaning
'should quit'). I change it to call pump->Quit() if ProcessXEvent() return true
so that the quit signal will be handled soon after the GDK's message loop is
finished.

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:152: 
On 2011/05/24 15:34:18, sadrul wrote:
> Too many new lines!

Done.

http://codereview.chromium.org/7050030/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:179: DLOG(WARNING) << "GDK received an event it
shouldn't have";
On 2011/05/24 15:34:18, sadrul wrote:
> Can you please replace the DLOG with a NOTREACHED here, and see if that causes
> any problems? If not, get rid of the TODO too. From what I understand, such an
> event should never reach here anymore?

Done.

[sadrul, 2011-05-24 18:59:32]

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.cc#...
base/message_pump_glib_x.cc:143: pump->Quit();
ProcessXEvent already calls Quit when appropriate. So this isn't necessary here.
I was just wondering if anything can be done here, but probably not. Let's
revert this change to what you had before.

[Evan Martin, 2011-05-24 19:12:00]

Am I here for OWNERS approval?  LGTM in that case; otherwise, let me know and I
will actually review the code.

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.h
File base/message_pump_glib_x.h (right):

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.h#n...
base/message_pump_glib_x.h:35: // through GDK's dispatching process and may get
discarded. Setting up
s/Setting/This function sets/

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.h#n...
base/message_pump_glib_x.h:37: // and dispatch them so that they won't get lost.
s/dispatch/dispatches/

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.h#n...
base/message_pump_glib_x.h:47: // Dispatching the XEvent and return true if we
should exit the current loop
s/Dispatching/Dispatches/

[Yufeng Shen (Slow to review), 2011-05-24 19:19:26]

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.cc#...
base/message_pump_glib_x.cc:143: pump->Quit();
On 2011/05/24 18:59:33, sadrul wrote:
> ProcessXEvent already calls Quit when appropriate. So this isn't necessary
here.
> I was just wondering if anything can be done here, but probably not. Let's
> revert this change to what you had before.

Done.

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.h
File base/message_pump_glib_x.h (right):

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.h#n...
base/message_pump_glib_x.h:35: // through GDK's dispatching process and may get
discarded. Setting up
On 2011/05/24 19:12:00, Evan Martin wrote:
> s/Setting/This function sets/

Done.

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.h#n...
base/message_pump_glib_x.h:37: // and dispatch them so that they won't get lost.
On 2011/05/24 19:12:00, Evan Martin wrote:
> s/dispatch/dispatches/

Done.

http://codereview.chromium.org/7050030/diff/6001/base/message_pump_glib_x.h#n...
base/message_pump_glib_x.h:47: // Dispatching the XEvent and return true if we
should exit the current loop
On 2011/05/24 19:12:00, Evan Martin wrote:
> s/Dispatching/Dispatches/

Done.

[sadrul, 2011-05-24 19:58:16]

LGTM from me.

@Evan: If you can take a look too, that'd be great. Thanks!

[Evan Martin, 2011-05-24 20:38:32]

LGTM++

[rjkroege, 2011-05-24 21:06:41]

in the interests of completeness: LGTM

[commit-bot: I haz the power, 2011-05-24 21:40:57]

Try job failure for 7050030-7004 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-05-24 23:57:36]

Try job failure for 7050030-7004 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...