Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5043002:
A workaround to close currently opened menu. Send escape key if screen locker fails to grab input. (Closed)
|
Created:
10 years, 1 month ago by oshima Modified:
9 years, 6 months ago CC:
chromium-reviews, nkostylev+cc_chromium.org, davemoore+watch_chromium.org, ben+cc_chromium.org, Alpha Left Google, Sergey Ulanov, dmac, garykac Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionA workaround to close currently opened menu. Send escape key if screen locker fails to grab input.
* added new xtst target in build/linux/system.gyp
BUG=chromium-os:5902
TEST=goto youtube, open flash menu then kick off the screen locker. It used to show spinner and eventually crash. It should close the menu and lock, and will not crash.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66662
Patch Set 1 #Patch Set 2 : " #
Total comments: 12
Patch Set 3 : " #
Total comments: 9
Patch Set 4 : use grab_server #
Total comments: 6
Patch Set 5 : " #Patch Set 6 : " #
Total comments: 4
Patch Set 7 : fix comments #Patch Set 8 : include xtst in gyp #Patch Set 9 : update gyp #
Messages
Total messages: 15 (0 generated)
http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:13: #include "app/x11_util.h" nit: sort http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:291: // This happen when a plugin has a menu opened and sending s/happen/happens/ http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:301: XTestGrabControl(display, True); I don't think that you need this. It lets you sidestep server grabs (not keyboard or pointer grabs), which you probably don't need to do -- server grabs are uncommon and usually brief, and if the server was grabbed, you'll still block on your next X request after XTestGrabControl(display, False). http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:304: XSync(display, False); This can just be XFlush(display) instead. http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:313: while ((current_grab_window = gtk_grab_get_current()) != NULL) Should this come before the XTest code? I guess that none of the GTK grab stuff matters as much after the first time that this method is called (since we'll have released the GTK grab the first time through).
http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:288: if (mouse_grab_status_ != GDK_GRAB_INVALID_TIME || This is wrong: if _only_ the pointer is grabbed, you'll end up sending an Escape keypress to some unrelated client that has the input focus. The only-pointer-is-grabbed state is actually easy to get into; I think that it happens whenever you hold the mouse button down (an implicit pointer grab gets added in this case). I think that the only-keyboard-is-grabbed state is unlikely to happen (see e.g. the end of http://mail.gnome.org/archives/gtk-devel-list/1999-January/msg00158.html). One thing that's a bit weird is that this method will try to send the key events even if we were successful in grabbing the pointer and keyboard earlier -- you're testing != GDK_GRAB_INVALID_TIME, which will also match the success code. This might not be a big deal since I don't think it gets called again after we're successful, but it'd probably be safer to check == GDK_GRAB_ALREADY_GRABBED. Also, there's a race here, in that the client could've released its grab between the point where we failed to get the grab and here. You could use something like this instead: gdk_x11_grab_server(); if (gdk_pointer_is_grabbed()) { // use XTEST to send events } gdk_x11_ungrab_server(); There's no gdk_keyboard_is_grabbed() method, unfortunately. You could probably just attempt to grab the keyboard and see if you're successful or not to test if it's already grabbed.
PTAL. I changed the xtest part so that it uses escape when kdb is grabbed by other client, and uses mouse click when pointer is grabbed. Not perfect but this is probably best guess we can make. http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:13: #include "app/x11_util.h" On 2010/11/16 15:38:13, Daniel Erat wrote: > nit: sort Done. http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:288: if (mouse_grab_status_ != GDK_GRAB_INVALID_TIME || On 2010/11/16 17:26:50, Daniel Erat wrote: > This is wrong: if _only_ the pointer is grabbed, you'll end up sending an Escape > keypress to some unrelated client that has the input focus. BTW, this *was* how view menu was implemented. Only pointer grab, but not keyboard grab and it looked working fine on desktop (by accident i think). The > only-pointer-is-grabbed state is actually easy to get into; I think that it > happens whenever you hold the mouse button down (an implicit pointer grab gets > added in this case). I think that the only-keyboard-is-grabbed state is > unlikely to happen (see e.g. the end of > http://mail.gnome.org/archives/gtk-devel-list/1999-January/msg00158.html). > Hmm, I'm not sure if I'm convinced. 1) I thought grabbing key without grabbing pointer happens when wm grab keyboard to process modifier keys? Sorry if I'm mistaken. 2) If a client can do that, the screen locker should deal with it. It should not misbehave because there is bad client. But I agree we should not send escape if keyboard is not grabbed. > One thing that's a bit weird is that this method will try to send the key events > even if we were successful in grabbing the pointer and keyboard earlier -- > you're testing != GDK_GRAB_INVALID_TIME, which will also match the success code. > This might not be a big deal since I don't think it gets called again after > we're successful, but it'd probably be safer to check == > GDK_GRAB_ALREADY_GRABBED. > Yes, if both are successful, this method will not be called, and there is GDK_GRAB_FROZEN, so I thought this is safer. In any case, the condition has been updated to send escape only when keyboard grab failed. > Also, there's a race here, in that the client could've released its grab between > the point where we failed to get the grab and here. You could use something > like this instead: > > gdk_x11_grab_server(); > if (gdk_pointer_is_grabbed()) { > // use XTEST to send events > } > gdk_x11_ungrab_server(); > > There's no gdk_keyboard_is_grabbed() method, unfortunately. You could probably > just attempt to grab the keyboard and see if you're successful or not to test if > it's already grabbed. Can you elaborate a bit? Are you suggesting that gdk_xxxx_ungrab may be called in between? http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:291: // This happen when a plugin has a menu opened and sending On 2010/11/16 15:38:13, Daniel Erat wrote: > s/happen/happens/ Done. http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:301: XTestGrabControl(display, True); On 2010/11/16 15:38:13, Daniel Erat wrote: > I don't think that you need this. It lets you sidestep server grabs (not > keyboard or pointer grabs), which you probably don't need to do -- server grabs > are uncommon and usually brief, and if the server was grabbed, you'll still > block on your next X request after XTestGrabControl(display, False). Done. http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:304: XSync(display, False); On 2010/11/16 15:38:13, Daniel Erat wrote: > This can just be XFlush(display) instead. Done. http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:313: while ((current_grab_window = gtk_grab_get_current()) != NULL) On 2010/11/16 15:38:13, Daniel Erat wrote: > Should this come before the XTest code? I guess that none of the GTK grab stuff > matters as much after the first time that this method is called (since we'll > have released the GTK grab the first time through). I believe the order isn't important and it was arbitrary. Moved.
On Tue, Nov 16, 2010 at 1:06 PM, <oshima@chromium.org> wrote: > PTAL. > > I changed the xtest part so that it uses escape when kdb is grabbed by other > client, and uses mouse click when pointer is grabbed. Not perfect but this > is > probably best guess we can make. > > > http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/screen_locker.cc (right): > > http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screen_locker.cc:13: #include > "app/x11_util.h" > On 2010/11/16 15:38:13, Daniel Erat wrote: >> >> nit: sort > > Done. > > http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screen_locker.cc:288: if > (mouse_grab_status_ != GDK_GRAB_INVALID_TIME || > On 2010/11/16 17:26:50, Daniel Erat wrote: >> >> This is wrong: if _only_ the pointer is grabbed, you'll end up sending > > an Escape >> >> keypress to some unrelated client that has the input focus. > > BTW, this *was* how view menu was implemented. Only pointer grab, but > not keyboard grab and it > looked working fine on desktop (by accident i think). > > The >> >> only-pointer-is-grabbed state is actually easy to get into; I think > > that it >> >> happens whenever you hold the mouse button down (an implicit pointer > > grab gets >> >> added in this case). > > I think that the only-keyboard-is-grabbed state is >> >> unlikely to happen (see e.g. the end of > > http://mail.gnome.org/archives/gtk-devel-list/1999-January/msg00158.html). > > > Hmm, I'm not sure if I'm convinced. > 1) I thought grabbing key without grabbing pointer happens when wm grab > keyboard to process > modifier keys? Sorry if I'm mistaken. > 2) If a client can do that, the screen locker should deal with it. It > should not misbehave > because there is bad client. 1) I thought so too, but I wasn't able to repro this by running a little program that tries to do grabs while I have an accelerator held. 2) Yeah, I agree that it's good to handle this. > But I agree we should not send escape if keyboard is not grabbed. > >> One thing that's a bit weird is that this method will try to send the > > key events >> >> even if we were successful in grabbing the pointer and keyboard > > earlier -- >> >> you're testing != GDK_GRAB_INVALID_TIME, which will also match the > > success code. >> >> This might not be a big deal since I don't think it gets called again > > after >> >> we're successful, but it'd probably be safer to check == >> GDK_GRAB_ALREADY_GRABBED. > > > Yes, if both are successful, this method will not be called, and there > is GDK_GRAB_FROZEN, > so I thought this is safer. In any case, the condition has been updated > to send escape only when > keyboard grab failed. > >> Also, there's a race here, in that the client could've released its > > grab between >> >> the point where we failed to get the grab and here. You could use > > something >> >> like this instead: > >> gdk_x11_grab_server(); >> if (gdk_pointer_is_grabbed()) { >> // use XTEST to send events >> } >> gdk_x11_ungrab_server(); > >> There's no gdk_keyboard_is_grabbed() method, unfortunately. You could > > probably >> >> just attempt to grab the keyboard and see if you're successful or not > > to test if >> >> it's already grabbed. > > Can you elaborate a bit? Are you suggesting that gdk_xxxx_ungrab may be > called in between? Yes. If you don't have the server grabbed, another client could've released its grab between the point where your attempt to grab failed and where you're trying to break the grab. > http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screen_locker.cc:291: // This happen when > a plugin has a menu opened and sending > On 2010/11/16 15:38:13, Daniel Erat wrote: >> >> s/happen/happens/ > > Done. > > http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screen_locker.cc:301: > XTestGrabControl(display, True); > On 2010/11/16 15:38:13, Daniel Erat wrote: >> >> I don't think that you need this. It lets you sidestep server grabs > > (not >> >> keyboard or pointer grabs), which you probably don't need to do -- > > server grabs >> >> are uncommon and usually brief, and if the server was grabbed, you'll > > still >> >> block on your next X request after XTestGrabControl(display, False). > > Done. > > http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screen_locker.cc:304: XSync(display, > False); > On 2010/11/16 15:38:13, Daniel Erat wrote: >> >> This can just be XFlush(display) instead. > > Done. > > http://codereview.chromium.org/5043002/diff/2001/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screen_locker.cc:313: while > ((current_grab_window = gtk_grab_get_current()) != NULL) > On 2010/11/16 15:38:13, Daniel Erat wrote: >> >> Should this come before the XTest code? I guess that none of the GTK > > grab stuff >> >> matters as much after the first time that this method is called (since > > we'll >> >> have released the GTK grab the first time through). > > I believe the order isn't important and it was arbitrary. Moved. > > http://codereview.chromium.org/5043002/ >
http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:13: #include "app/x11_util.h" still needs to be sorted http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:286: // Make sure there is no gtl grab widget so that gtk simply propagates s/gtl/gtk/ http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:311: Display* display = x11_util::GetXDisplay(); So the reason for adding a server grab around this code (and checking the keyboard grab status again while you have the server grabbed) is that the grab could've already been released at this point, in which case you'll send an event to some other window. http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:316: } else if (mouse_grab_status_ == GDK_GRAB_ALREADY_GRABBED || why 'else if' instead of keeping this as an independent if statement? http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:317: mouse_grab_status_ == GDK_GRAB_FROZEN) { nit: indent this to line up with the mouse_grab_status_ on the previous line http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:329: XTestFakeMotionEvent(display, -1, -1, -1, CurrentTime); I'm a bit nervous about (-1, -1) being used; there could be a window there. It'd be better if you used something farther offscreen.
PTAL * Separated gtk grab and keyboard/pointer grab code. * keyboard/pointer grab is now done inside gdk_x11_grab_server() so that grab/send event happens synchronously. I'm still not 100% sure if this does what's intended as the grab has been release by the time the events are handled. http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:13: #include "app/x11_util.h" On 2010/11/16 21:58:06, Daniel Erat wrote: > still needs to be sorted Done. http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:286: // Make sure there is no gtl grab widget so that gtk simply propagates On 2010/11/16 21:58:06, Daniel Erat wrote: > s/gtl/gtk/ Done. http://codereview.chromium.org/5043002/diff/7001/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:329: XTestFakeMotionEvent(display, -1, -1, -1, CurrentTime); On 2010/11/16 21:58:06, Daniel Erat wrote: > I'm a bit nervous about (-1, -1) being used; there could be a window there. > It'd be better if you used something farther offscreen. changed to -10000, -10000
LGTM after a few comments are addressed. http://codereview.chromium.org/5043002/diff/14001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/14001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screen_locker.cc:307: // client by sending events that will hopefullly close menu or window nit: s/menu or window/menus or windows/ http://codereview.chromium.org/5043002/diff/14001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screen_locker.cc:341: gdk_x11_grab_server(); Do you mean for this to appear before the call to gdk_keyboard_grab? Seems strange that it happens after the keyboard grab but before the pointer grab. http://codereview.chromium.org/5043002/diff/14001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screen_locker.cc:372: gdk_x11_ungrab_server(); Server grabs block all other clients from doing anything, so I'm a bit worried about this interrupting window manager animations. Can you move this up so we call it before the CHECK_EQ() calls and OnGrabInputs() (and add another copy of it to the end of the failed-grab path)?
http://codereview.chromium.org/5043002/diff/14001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/14001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screen_locker.cc:307: // client by sending events that will hopefullly close menu or window On 2010/11/17 01:24:23, Daniel Erat wrote: > nit: s/menu or window/menus or windows/ Done. http://codereview.chromium.org/5043002/diff/14001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screen_locker.cc:341: gdk_x11_grab_server(); On 2010/11/17 01:24:23, Daniel Erat wrote: > Do you mean for this to appear before the call to gdk_keyboard_grab? Seems > strange that it happens after the keyboard grab but before the pointer grab. Doh, yes, that's what I meant. http://codereview.chromium.org/5043002/diff/14001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screen_locker.cc:372: gdk_x11_ungrab_server(); On 2010/11/17 01:24:23, Daniel Erat wrote: > Server grabs block all other clients from doing anything, so I'm a bit worried > about this interrupting window manager animations. Can you move this up so we > call it before the CHECK_EQ() calls and OnGrabInputs() (and add another copy of > it to the end of the failed-grab path)? Done.
LGTM! http://codereview.chromium.org/5043002/diff/8002/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/8002/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:307: // client by sending events that will hopefullly close menus or windows nit: s/hopefullly/hopefully/ http://codereview.chromium.org/5043002/diff/8002/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:679: // Call this after lock_window_->Show() otherwise the 1st invocation nit: add a semicolon after "Show()", e.g. "Call this after ...Show(); otherwise the first invocation..."
will wait for green trybots http://codereview.chromium.org/5043002/diff/8002/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/5043002/diff/8002/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:307: // client by sending events that will hopefullly close menus or windows On 2010/11/17 19:00:55, Daniel Erat wrote: > nit: s/hopefullly/hopefully/ Done. http://codereview.chromium.org/5043002/diff/8002/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screen_locker.cc:679: // Call this after lock_window_->Show() otherwise the 1st invocation On 2010/11/17 19:00:55, Daniel Erat wrote: > nit: add a semicolon after "Show()", e.g. "Call this after ...Show(); otherwise > the first invocation..." Done.
evan, ajwong I need Xtst for chromeos screen locker and want to add new xtst target to build/linux/system.gyp. Could you please take a look? Thanks,
LGTM on the gyp changes, but you should make sure evan, or one of the other core linux devs okay it as well. On 2010/11/18 00:49:19, oshima wrote: > evan, ajwong > > I need Xtst for chromeos screen locker and want to add > new xtst target to build/linux/system.gyp. Could you please take a look? > > Thanks,
evan, mmoss could you please take a look at system.gyp change? Thanks, - oshima On 2010/11/18 01:28:29, awong wrote: > LGTM on the gyp changes, but you should make sure evan, or one of the other core > linux devs okay it as well. > > On 2010/11/18 00:49:19, oshima wrote: > > evan, ajwong > > > > I need Xtst for chromeos screen locker and want to add > > new xtst target to build/linux/system.gyp. Could you please take a look? > > > > Thanks,
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
