Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1324913002:
Fix out of order key events in Chrome on Linux. (Closed)
DescriptionFix out of order key events in Chrome on Linux.
The ibus-gtk layer was using XPutBackEvent which pushes an event to the
top of the X event queue. The problem with this is that if the gtk event
loop processes more than one event before the X event loop executes the
message pump will see the events in reverse order.
Fix this by processing the mapped X11 event immediately. Since the gtk
event loop and the X11 event loop are on the same thread this makes
sense as the events stay in the proper order.
BUG=524084
Committed: https://crrev.com/a8bae713f4d0a4598443f1279d2fd595cbae8d18
Cr-Commit-Position: refs/heads/master@{#412607}
Patch Set 1 #Patch Set 2 : Fix component build #
Total comments: 1
Patch Set 3 : Rebase #
Total comments: 2
Patch Set 4 : Fix nits #
Total comments: 2
Patch Set 5 : Fix spelling error #
Dependent Patchsets: Messages
Total messages: 30 (10 generated)
dtapuska@chromium.org changed reviewers: + erg@chromium.org, sadrul@chromium.org, shuchen@chromium.org
This is a second attempt at the re-ordering issue. The first attempt (via XSendEvent) caused an issue with the Save Dialog not responding.
(fyi, I'm waiting for comments from the others and will owners stamp afterwards.)
https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc (right): https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc:79: XPutBackEvent(x_event.xkey.display, &x_event); Can we send this key-event directly to the InputMethod instead of going through the event-source?
On 2015/09/02 17:18:52, sadrul wrote: > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > File chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc (right): > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc:79: > XPutBackEvent(x_event.xkey.display, &x_event); > Can we send this key-event directly to the InputMethod instead of going through > the event-source? @shuchen; Do you have any idea if Sadrul's comment is possible?
On 2015/09/14 19:12:21, dtapuska wrote: > On 2015/09/02 17:18:52, sadrul wrote: > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > File chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc (right): > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc:79: > > XPutBackEvent(x_event.xkey.display, &x_event); > > Can we send this key-event directly to the InputMethod instead of going > through > > the event-source? > > @shuchen; Do you have any idea if Sadrul's comment is possible? It is tricky to dispatch key event directly into InputMethod. For example, for Ash & non-Ash, the means of integrating InputMethod into the key event flow are different. It is also tricky to dispatch key event directly to WindowTreeHost, in terms of finding the correct target to dispatch. So IMHO, dispatching key event directly into ui/base/ime & ui/aura would be risky and easy to cause regressions. What's more, I'm not sure about how GTK event queue works, but will there be such cases where some times the key events goes to GTK event queue but sometimes not? If there are such cases, then directly dispatching key event would potentially broken the key event order again.
On 2015/09/15 05:17:49, Shu Chen wrote: > On 2015/09/14 19:12:21, dtapuska wrote: > > On 2015/09/02 17:18:52, sadrul wrote: > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > File chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc (right): > > > > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc:79: > > > XPutBackEvent(x_event.xkey.display, &x_event); > > > Can we send this key-event directly to the InputMethod instead of going > > through > > > the event-source? > > > > @shuchen; Do you have any idea if Sadrul's comment is possible? > > It is tricky to dispatch key event directly into InputMethod. > For example, for Ash & non-Ash, the means of integrating InputMethod into the > key event flow are different. > It is also tricky to dispatch key event directly to WindowTreeHost, in terms of > finding the correct target to dispatch. > > So IMHO, dispatching key event directly into ui/base/ime & ui/aura would be > risky and easy to cause regressions. > > What's more, I'm not sure about how GTK event queue works, but will there be > such cases where some times the key events goes to GTK event queue but sometimes > not? > If there are such cases, then directly dispatching key event would potentially > broken the key event order again. So then is the approach I took in this CL an approach that is suggested?
On 2015/09/15 05:17:49, Shu Chen wrote: > On 2015/09/14 19:12:21, dtapuska wrote: > > On 2015/09/02 17:18:52, sadrul wrote: > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > File chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc (right): > > > > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc:79: > > > XPutBackEvent(x_event.xkey.display, &x_event); > > > Can we send this key-event directly to the InputMethod instead of going > > through > > > the event-source? > > > > @shuchen; Do you have any idea if Sadrul's comment is possible? > > It is tricky to dispatch key event directly into InputMethod. > For example, for Ash & non-Ash, the means of integrating InputMethod into the > key event flow are different. Can you explain a little bit? I am not all that familiar with this code, but for chromeos (ash), InputMethodEngine::SendKeyEvents() seems to directly send the event(s) to the EventSource (through the WindowTreeHost). I am suggesting we do something similar here, by either injecting event into the InputMethod (through the WindowTreeHost), or to the EventProcessor (also through WindowTreeHost), instead of putting the event back in the X11/PlatformEventSource queue. > It is also tricky to dispatch key event directly to WindowTreeHost, in terms of > finding the correct target to dispatch. We should be able to find the target here (DesktopWindowTreeHostX11::GetHostForXID() gives us the aura::WindowTreeHost, which has a GetInputMethod() we can use). > > So IMHO, dispatching key event directly into ui/base/ime & ui/aura would be > risky and easy to cause regressions. I share your concern, but ui/events/platform is an even lower layer (and x11 server still lower than that), and so I would like to keep this at the aura and/or ime layer if possible. (ideally, this would be taken care of by something in libgtk2ui/, but I do not have a suggestion that would work). > > What's more, I'm not sure about how GTK event queue works, but will there be > such cases where some times the key events goes to GTK event queue but sometimes > not? > If there are such cases, then directly dispatching key event would potentially > broken the key event order again. Interesting point. I think as far as the chrome-managed windows are concerned, we should be fine. I do not know about windows that gtk+ manages (e.g. open/save dialogs). Does IME work correctly in those dialogs?
On 2015/09/15 14:01:01, dtapuska wrote: > On 2015/09/15 05:17:49, Shu Chen wrote: > > On 2015/09/14 19:12:21, dtapuska wrote: > > > On 2015/09/02 17:18:52, sadrul wrote: > > > > > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > > File chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > > chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc:79: > > > > XPutBackEvent(x_event.xkey.display, &x_event); > > > > Can we send this key-event directly to the InputMethod instead of going > > > through > > > > the event-source? > > > > > > @shuchen; Do you have any idea if Sadrul's comment is possible? > > > > It is tricky to dispatch key event directly into InputMethod. > > For example, for Ash & non-Ash, the means of integrating InputMethod into the > > key event flow are different. > > It is also tricky to dispatch key event directly to WindowTreeHost, in terms > of > > finding the correct target to dispatch. > > > > So IMHO, dispatching key event directly into ui/base/ime & ui/aura would be > > risky and easy to cause regressions. > > > > What's more, I'm not sure about how GTK event queue works, but will there be > > such cases where some times the key events goes to GTK event queue but > sometimes > > not? > > If there are such cases, then directly dispatching key event would potentially > > broken the key event order again. > > So then is the approach I took in this CL an approach that is suggested? Yes. I cannot think of a better way than this cl. Sadrul, can you please let us know whether you're convinced.
On 2015/09/15 16:52:00, sadrul wrote: > On 2015/09/15 05:17:49, Shu Chen wrote: > > On 2015/09/14 19:12:21, dtapuska wrote: > > > On 2015/09/02 17:18:52, sadrul wrote: > > > > > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > > File chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > > chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc:79: > > > > XPutBackEvent(x_event.xkey.display, &x_event); > > > > Can we send this key-event directly to the InputMethod instead of going > > > through > > > > the event-source? > > > > > > @shuchen; Do you have any idea if Sadrul's comment is possible? > > > > It is tricky to dispatch key event directly into InputMethod. > > For example, for Ash & non-Ash, the means of integrating InputMethod into the > > key event flow are different. > > Can you explain a little bit? I am not all that familiar with this code, but for > chromeos (ash), InputMethodEngine::SendKeyEvents() seems to directly send the > event(s) to the EventSource (through the WindowTreeHost). I am suggesting we do > something similar here, by either injecting event into the InputMethod (through > the WindowTreeHost), or to the EventProcessor (also through WindowTreeHost), > instead of putting the event back in the X11/PlatformEventSource queue. For desktop, InputMethod is the first guy to handle key event, and then the key events go to WTH. For Ash, WTH is the first guy to handle key event, and then the key events go to InputMethod (through InputMethodEventHandler). That is the tricky part. > > > It is also tricky to dispatch key event directly to WindowTreeHost, in terms > of > > finding the correct target to dispatch. > > We should be able to find the target here > (DesktopWindowTreeHostX11::GetHostForXID() gives us the aura::WindowTreeHost, > which has a GetInputMethod() we can use). I think we need to use Shell::GetPrimaryRootWindow()->GetHost()->GetInputMethod() for Ash. I'm not sure whether it is ok to include ash/... from chrome/browser/ui/libgtk2ui/.... > > > > > So IMHO, dispatching key event directly into ui/base/ime & ui/aura would be > > risky and easy to cause regressions. > > I share your concern, but ui/events/platform is an even lower layer (and x11 > server still lower than that), and so I would like to keep this at the aura > and/or ime layer if possible. (ideally, this would be taken care of by something > in libgtk2ui/, but I do not have a suggestion that would work). > > > > > What's more, I'm not sure about how GTK event queue works, but will there be > > such cases where some times the key events goes to GTK event queue but > sometimes > > not? > > If there are such cases, then directly dispatching key event would potentially > > broken the key event order again. > > Interesting point. I think as far as the chrome-managed windows are concerned, > we should be fine. I do not know about windows that gtk+ manages (e.g. open/save > dialogs). Does IME work correctly in those dialogs? Yes, I think IME works correctly in those dialogs. I guess GTK expert like yukishiino@ or suzhe@ can give us some advices.
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dtapuska@chromium.org changed reviewers: + suzhe@chromium.org, yukishiino@chromium.org
On 2015/09/15 17:50:44, Shu Chen (OOO Jul 31 - Aug 8) wrote: > On 2015/09/15 16:52:00, sadrul wrote: > > On 2015/09/15 05:17:49, Shu Chen wrote: > > > On 2015/09/14 19:12:21, dtapuska wrote: > > > > On 2015/09/02 17:18:52, sadrul wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > > > File chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1324913002/diff/20001/chrome/browser/ui/libgt... > > > > > chrome/browser/ui/libgtk2ui/gtk2_event_loop.cc:79: > > > > > XPutBackEvent(x_event.xkey.display, &x_event); > > > > > Can we send this key-event directly to the InputMethod instead of going > > > > through > > > > > the event-source? > > > > > > > > @shuchen; Do you have any idea if Sadrul's comment is possible? > > > > > > It is tricky to dispatch key event directly into InputMethod. > > > For example, for Ash & non-Ash, the means of integrating InputMethod into > the > > > key event flow are different. > > > > Can you explain a little bit? I am not all that familiar with this code, but > for > > chromeos (ash), InputMethodEngine::SendKeyEvents() seems to directly send the > > event(s) to the EventSource (through the WindowTreeHost). I am suggesting we > do > > something similar here, by either injecting event into the InputMethod > (through > > the WindowTreeHost), or to the EventProcessor (also through WindowTreeHost), > > instead of putting the event back in the X11/PlatformEventSource queue. > For desktop, InputMethod is the first guy to handle key event, and then the key > events go to WTH. > For Ash, WTH is the first guy to handle key event, and then the key events go to > InputMethod (through InputMethodEventHandler). > That is the tricky part. > > > > > > It is also tricky to dispatch key event directly to WindowTreeHost, in terms > > of > > > finding the correct target to dispatch. > > > > We should be able to find the target here > > (DesktopWindowTreeHostX11::GetHostForXID() gives us the aura::WindowTreeHost, > > which has a GetInputMethod() we can use). > I think we need to use > Shell::GetPrimaryRootWindow()->GetHost()->GetInputMethod() for Ash. > I'm not sure whether it is ok to include ash/... from > chrome/browser/ui/libgtk2ui/.... > > > > > > > > > So IMHO, dispatching key event directly into ui/base/ime & ui/aura would be > > > risky and easy to cause regressions. > > > > I share your concern, but ui/events/platform is an even lower layer (and x11 > > server still lower than that), and so I would like to keep this at the aura > > and/or ime layer if possible. (ideally, this would be taken care of by > something > > in libgtk2ui/, but I do not have a suggestion that would work). > > > > > > > > What's more, I'm not sure about how GTK event queue works, but will there be > > > such cases where some times the key events goes to GTK event queue but > > sometimes > > > not? > > > If there are such cases, then directly dispatching key event would > potentially > > > broken the key event order again. > > > > Interesting point. I think as far as the chrome-managed windows are concerned, > > we should be fine. I do not know about windows that gtk+ manages (e.g. > open/save > > dialogs). Does IME work correctly in those dialogs? > Yes, I think IME works correctly in those dialogs. I guess GTK expert like > yukishiino@ or suzhe@ can give us some advices. I'm digging up this old pending patch because I was trying to fix a flaky browser test and I saw the key events were re-ordered causing the flakiness... yukishiino@ or suzhe@ Can you comment on this approach that I took (11 months ago :-)) I've rebased it and it still seems to work for me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. My thought is that ibus-gtk eventually moves a key event from X event loop to GDK event loop, so we'd like to move it back. The most straight forward way is to put the event in X event loop. Dispatching the X event right away is probably the best option to keep the event order. Another option is to use XSendEvent (i.e. queue) instead of XPutBackEvent (i.e. stack). IIUC, IME is designed to be one of key event handlers, and not designed to be the first key event taker (owner of event dispatching). So, I'd prefer dispatching the raw X event to the X event handler to redirecting the event to IME. I vaguely remember that there is a case that a shortcut-key handler or someone else first takes a key event before IME. https://codereview.chromium.org/1324913002/diff/40001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/1324913002/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:70: void DispatchEventNow(XEvent* event); nit: We should declare this member function between DispatchXEvents and BlockUntilWindowMapped. nit: s/DispatchEventNow/DispatchXEventNow/ because we already have DispatchXEvents.
I'm sorry that I had missed this review 11 months ago.
shuchen@ are you good with this given yuki's comments? https://codereview.chromium.org/1324913002/diff/40001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/1324913002/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:70: void DispatchEventNow(XEvent* event); On 2016/08/16 09:26:11, Yuki wrote: > nit: We should declare this member function between DispatchXEvents and > BlockUntilWindowMapped. > > nit: s/DispatchEventNow/DispatchXEventNow/ because we already have > DispatchXEvents. Done.
lgtm from IME perspective.
ui/events lgtm https://codereview.chromium.org/1324913002/diff/60001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/1324913002/diff/60001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:54: // Dispatches a given event immediately. This is to faciliate sequential *facilitate
erg@ can you stamp this now that the others have signed off on it? https://codereview.chromium.org/1324913002/diff/60001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/1324913002/diff/60001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:54: // Dispatches a given event immediately. This is to faciliate sequential On 2016/08/17 01:03:10, sadrul wrote: > *facilitate Done.
lgtm
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, shuchen@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1324913002/#ps80001 (title: "Fix spelling error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix out of order key events in Chrome on Linux. The ibus-gtk layer was using XPutBackEvent which pushes an event to the top of the X event queue. The problem with this is that if the gtk event loop processes more than one event before the X event loop executes the message pump will see the events in reverse order. Fix this by processing the mapped X11 event immediately. Since the gtk event loop and the X11 event loop are on the same thread this makes sense as the events stay in the proper order. BUG=524084 ========== to ========== Fix out of order key events in Chrome on Linux. The ibus-gtk layer was using XPutBackEvent which pushes an event to the top of the X event queue. The problem with this is that if the gtk event loop processes more than one event before the X event loop executes the message pump will see the events in reverse order. Fix this by processing the mapped X11 event immediately. Since the gtk event loop and the X11 event loop are on the same thread this makes sense as the events stay in the proper order. BUG=524084 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix out of order key events in Chrome on Linux. The ibus-gtk layer was using XPutBackEvent which pushes an event to the top of the X event queue. The problem with this is that if the gtk event loop processes more than one event before the X event loop executes the message pump will see the events in reverse order. Fix this by processing the mapped X11 event immediately. Since the gtk event loop and the X11 event loop are on the same thread this makes sense as the events stay in the proper order. BUG=524084 ========== to ========== Fix out of order key events in Chrome on Linux. The ibus-gtk layer was using XPutBackEvent which pushes an event to the top of the X event queue. The problem with this is that if the gtk event loop processes more than one event before the X event loop executes the message pump will see the events in reverse order. Fix this by processing the mapped X11 event immediately. Since the gtk event loop and the X11 event loop are on the same thread this makes sense as the events stay in the proper order. BUG=524084 Committed: https://crrev.com/a8bae713f4d0a4598443f1279d2fd595cbae8d18 Cr-Commit-Position: refs/heads/master@{#412607} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a8bae713f4d0a4598443f1279d2fd595cbae8d18 Cr-Commit-Position: refs/heads/master@{#412607} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
