Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
378693003:
[Mac] Cmd+W should close the User Manager (Closed)
|
Created:
6 years, 5 months ago by noms (inactive) Modified:
6 years, 5 months ago CC:
chromium-reviews, jdduke (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Description[Mac] Cmd+W should close the User Manager
This implementation is similar to that in ui/cocoa/autofill/autofill_sign_in_container.mm.
BUG=364644
TEST=Start Chrome with --new-profile-management turned on. From the avatar
menu, select "Not <name>". The User Manager should show up. Cmd+W should
close it, but other accelerators (eg. Cmd+t) should not work.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282390
Patch Set 1 #
Total comments: 11
Patch Set 2 : rebase #Patch Set 3 : rachel comments #
Messages
Total messages: 13 (0 generated)
I've accelerated ALL the things. Please take a look & thanks for helping me sort it out! :)
jdduke for insights into RenderWidget keyboard event handling. AFAICT, we should be save to forward all WebContents keyboard events to the containing window in HandleKeyboardEvents, but I'm not 100% certain. https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... File chrome/browser/ui/cocoa/profiles/user_manager_mac.h (left): https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.h:15: class UserManagerMac; +1 - Kill ALL THE THINGS! ;) https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:48: UserManagerWebContentsDelegate(NSWindow* window) : window_(window) {} Make the ctor take a ChromeEventProcessingWindow... https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:55: // Since the User Manager is a "top level" window, only handle close events. Really? No other shortcuts? Quit? https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:57: [BrowserWindowUtils getCommandId:event] != IDC_CLOSE_WINDOW) getCommandId != -1, maybe? Or skip this entirely, and just redispatch all key events? (I'm not sure if that'll work. jdduke touched the RenderWidgetHostImpl keyboard handling last, so I'll loop him in) https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:63: ChromeEventProcessingWindow* event_window = Which kills the DCHECK here.
On 2014/07/08 23:31:28, groby wrote: > jdduke for insights into RenderWidget keyboard event handling. AFAICT, we should > be save to forward all WebContents keyboard events to the containing window in > HandleKeyboardEvents, but I'm not 100% certain. My understanding of keyboard event flow outside of content/ is fairly limited, so I don't have a lot to say here (my abuse of the keyboard-related code in RenderWidgetHostImpl was mostly mechanical rearrangement =/). Adding jochen@ who might be of more help.
I punt to Avi
What groby says, mostly. I'm not really sure of how the event handling is working here, but if you're aping autofill_sign_in_container, then do so and avoid making it more complicated than it need be.
https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:48: UserManagerWebContentsDelegate(NSWindow* window) : window_(window) {} On 2014/07/08 23:31:28, groby wrote: > Make the ctor take a ChromeEventProcessingWindow... Done. https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:55: // Since the User Manager is a "top level" window, only handle close events. I guess that would make sense. No other shortcuts though! The user manager can be the only top level window, and it doesn't look like a normal browser window, so people shouldn't think of it as being able to do anything in it other than launch a profile. I will add Quit, though. On 2014/07/08 23:31:28, groby wrote: > Really? No other shortcuts? Quit? > https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:57: [BrowserWindowUtils getCommandId:event] != IDC_CLOSE_WINDOW) Hmm, I may be misreading this, but wouldn't redispatching all the events allow all sort of Cmd+t and their brethren through? Currently the User Manager handles (by design) no accelerators -- I'd just like people to be able to close it like a tab, but not do anything else... I refactored this a bit to be more readable (same functionality) On 2014/07/08 23:31:28, groby wrote: > getCommandId != -1, maybe? Or skip this entirely, and just redispatch all key > events? > > (I'm not sure if that'll work. jdduke touched the RenderWidgetHostImpl keyboard > handling last, so I'll loop him in) https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:63: ChromeEventProcessingWindow* event_window = On 2014/07/08 23:31:28, groby wrote: > Which kills the DCHECK here. Done.
This looks reasonable to me. LGTM if groby is happy.
Code per se is LGTM - if app-modal dialog is really the status we desire here. https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:57: [BrowserWindowUtils getCommandId:event] != IDC_CLOSE_WINDOW) On 2014/07/09 18:06:55, Monica Dinculescu wrote: > Hmm, I may be misreading this, but wouldn't redispatching all the events allow > all sort of Cmd+t and their brethren through? Currently the User Manager handles > (by design) no accelerators -- I'd just like people to be able to close it like > a tab, but not do anything else... That's really odd UI behavior. You're essentially making this into an app-modal dialog. I don't believe there are _any_ app-modal dialogs in Chrome right now. Why does the user manager qualify for that status? > > I refactored this a bit to be more readable (same functionality) > > On 2014/07/08 23:31:28, groby wrote: > > getCommandId != -1, maybe? Or skip this entirely, and just redispatch all key > > events? > > > > (I'm not sure if that'll work. jdduke touched the RenderWidgetHostImpl > keyboard > > handling last, so I'll loop him in) > I don't think so. |window_| is a top-level window. Accelerator events it doesn't handle are dispatched to the menu. That means Cmd-T will do nothing
https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right): https://codereview.chromium.org/378693003/diff/1/chrome/browser/ui/cocoa/prof... chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:57: [BrowserWindowUtils getCommandId:event] != IDC_CLOSE_WINDOW) Trying to soothe some worries: It's not app modal. It basically looks like a browser window that's also opened (doesn't steal focus, doesn't close any existing browser windows), but it has a separate, special underlying profile, which can't really be used. The user manager is only used to select different profiles (think of the cros login screen), so it makes no sense to allow tab opening/bookmarks/etc, since it's not meant to be a browser window... Also, in particular, this CL doesn't take away functionality from anything existent, it adds a little bit :) On 2014/07/09 19:48:13, groby wrote: > On 2014/07/09 18:06:55, Monica Dinculescu wrote: > > Hmm, I may be misreading this, but wouldn't redispatching all the events allow > > all sort of Cmd+t and their brethren through? Currently the User Manager > handles > > (by design) no accelerators -- I'd just like people to be able to close it > like > > a tab, but not do anything else... > > That's really odd UI behavior. You're essentially making this into an app-modal > dialog. I don't believe there are _any_ app-modal dialogs in Chrome right now. > Why does the user manager qualify for that status? > > > > > I refactored this a bit to be more readable (same functionality) > > > > On 2014/07/08 23:31:28, groby wrote: > > > getCommandId != -1, maybe? Or skip this entirely, and just redispatch all > key > > > events? > > > > > > (I'm not sure if that'll work. jdduke touched the RenderWidgetHostImpl > > keyboard > > > handling last, so I'll loop him in) > > > > I don't think so. |window_| is a top-level window. Accelerator events it doesn't > handle are dispatched to the menu. That means Cmd-T will do nothing
SLGTM
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/378693003/40001
Message was sent while issue was closed.
Change committed as 282390 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
