[Mac] Cmd+W should close the User Manager

chrome/browser/ui/cocoa/profiles/user_manager_mac.mm

Index: chrome/browser/ui/cocoa/profiles/user_manager_mac.mm
diff --git a/chrome/browser/ui/cocoa/profiles/user_manager_mac.mm b/chrome/browser/ui/cocoa/profiles/user_manager_mac.mm
index 3087ac4b94c39afc5047ac53e0b36a3f2ebde880..d42a960ba6b53c7a008a6ac2ddaf9d4ad24f3bd3 100644
--- a/chrome/browser/ui/cocoa/profiles/user_manager_mac.mm
+++ b/chrome/browser/ui/cocoa/profiles/user_manager_mac.mm
@@ -4,10 +4,16 @@
 
 #include "chrome/browser/ui/cocoa/profiles/user_manager_mac.h"
 
+#include "base/mac/foundation_util.h"
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser_dialogs.h"
+#import "chrome/browser/ui/cocoa/browser_window_utils.h"
+#include "chrome/browser/ui/cocoa/chrome_event_processing_window.h"
+#include "content/public/browser/native_web_keyboard_event.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_delegate.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 
@@ -36,10 +42,38 @@ void HideUserManager() {
 
 }  // namespace chrome
 
+// Custom WebContentsDelegate that allows handling of hotkeys.
+class UserManagerWebContentsDelegate : public content::WebContentsDelegate {
+ public:
+  UserManagerWebContentsDelegate(NSWindow* window) : window_(window) {}

[groby-ooo-7-16, 2014/07/08 23:31:28]

Make the ctor take a ChromeEventProcessingWindow...

[noms (inactive), 2014/07/09 18:06:55]

Done.

+
+  // WebContentsDelegate implementation. Forwards all unhandled keyboard events
+  // to the current window.
+  virtual void HandleKeyboardEvent(
+      content::WebContents* source,
+      const content::NativeWebKeyboardEvent& event) OVERRIDE {
+    // Since the User Manager is a "top level" window, only handle close events.

[groby-ooo-7-16, 2014/07/08 23:31:28]

Really? No other shortcuts? Quit?

[noms (inactive), 2014/07/09 18:06:55]

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:

+    if (![BrowserWindowUtils shouldHandleKeyboardEvent:event] ||
+        [BrowserWindowUtils getCommandId:event] != IDC_CLOSE_WINDOW)

[groby-ooo-7-16, 2014/07/08 23:31:28]

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)

[noms (inactive), 2014/07/09 18:06:55]

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:

[groby-ooo-7-16, 2014/07/09 19:48:13]

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 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

[noms (inactive), 2014/07/09 21:08:14]

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:

+      return;
+
+    // Not invoking +[BrowserWindowUtils handleKeyboardEvent here], since the
+    // window in question is a ConstrainedWindowCustomWindow, not a
+    // BrowserWindow.
+    ChromeEventProcessingWindow* event_window =

[groby-ooo-7-16, 2014/07/08 23:31:28]

Which kills the DCHECK here.

[noms (inactive), 2014/07/09 18:06:55]

Done.

+        base::mac::ObjCCastStrict<ChromeEventProcessingWindow>(window_);
+    [event_window redispatchKeyEvent:event.os_event];
+  }
+
+ private:
+  NSWindow* window_;  // Used to redispatch key events.
+};
+
 // Window controller for the User Manager view.
 @interface UserManagerWindowController : NSWindowController <NSWindowDelegate> {
  @private
   scoped_ptr<content::WebContents> webContents_;
+  scoped_ptr<UserManagerWebContentsDelegate> webContentsDelegate_;
   UserManagerMac* userManagerObserver_;  // Weak.
 }
 - (void)windowWillClose:(NSNotification*)notification;
@@ -66,7 +100,7 @@ void HideUserManager() {
   NSRect contentRect = NSMakeRect((screenWidth - kWindowWidth) / 2,
                                   (screenHeight - kWindowHeight) / 2,
                                   kWindowWidth, kWindowHeight);
-  NSWindow* window = [[NSWindow alloc]
+  ChromeEventProcessingWindow* window = [[ChromeEventProcessingWindow alloc]
       initWithContentRect:contentRect
                 styleMask:NSTitledWindowMask |
                           NSClosableWindowMask |
@@ -83,6 +117,9 @@ void HideUserManager() {
     webContents_.reset(content::WebContents::Create(
         content::WebContents::CreateParams(profile)));
     window.contentView = webContents_->GetNativeView();
+    webContentsDelegate_.reset(
+        new UserManagerWebContentsDelegate([self window]));
+    webContents_->SetDelegate(webContentsDelegate_.get());
     DCHECK(window.contentView);
 
     [[NSNotificationCenter defaultCenter]