[Mac] User manager should show up as a standalone window.

chrome/browser/ui/cocoa/user_manager_mac.mm

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#import <Cocoa/Cocoa.h>
+#include "chrome/browser/ui/cocoa/user_manager_mac.h"
 
+#include "base/strings/string_number_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser_dialogs.h"
+#include "chrome/common/url_constants.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_view.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util_mac.h"
+
+namespace {
+

[groby-ooo-7-16, 2013/12/04 01:59:48]

No need for anon namespaces if it's constants only

+// Default window size. Taken from the views implementation in
+// chrome/browser/ui/views/user_manager_view.cc.
+const int kWindowWidth = 900;
+const int kWindowHeight = 700;

[groby-ooo-7-16, 2013/12/04 01:59:48]

Hm. You might want to dynamically size this, maybe based on browser window.
900x700 will look humongous on e.g. an 11" MBA

+
+}  // namespace
 
 namespace chrome {
 
 // Declared in browser_dialogs.h so others don't have to depend on this header.
-// TODO(noms): Add implementation when the User Manager dialog is implemented.
 void ShowUserManager(const base::FilePath& profile_path_to_focus) {
-  NOTIMPLEMENTED();
+  UserManagerMac::Show(profile_path_to_focus);
 }
 
 void HideUserManager() {
-  NOTIMPLEMENTED();
+  UserManagerMac::Hide();
 }
 
 }  // namespace chrome
+
+@interface UserManagerWindowController (Private)
+- (void)windowWillClose:(NSNotification*)notification;
+- (void)windowDidResize:(NSNotification*)notification;
+@end
+
+@implementation UserManagerWindowController
+
+- (id)initWithProfile:(Profile*) profile

[groby-ooo-7-16, 2013/12/04 01:59:48]

No space after (Profile*) and other types in methods.

[noms (inactive), 2013/12/05 17:55:06]

Done.

+         withObserver:(UserManagerMac*) userManagerObserver {
+
+  userManagerObserver_ = userManagerObserver;
+  NSWindow* window = [[NSWindow alloc]
+      initWithContentRect:NSMakeRect(0, 0, kWindowWidth, kWindowHeight)

[noms (inactive), 2013/12/03 21:58:28]

I think this shouldn't be 0, but I don't know what other values I can give.
Ideally, it should be on top of the existing browser window, I think.

[groby-ooo-7-16, 2013/12/04 01:59:48]

This sounds oddly like a constrained dialog. (Like e.g. print preview). Do you
have mocks?

[noms (inactive), 2013/12/05 17:55:06]

Addressed by separate reply.

 On 2013/12/04 01:59:48, groby wrote:

+                styleMask:NSTitledWindowMask |
+                          NSClosableWindowMask |
+                          NSResizableWindowMask
+                  backing:NSBackingStoreBuffered
+                    defer:NO];
+  [window setTitle:l10n_util::GetNSString(IDS_USER_MANAGER_SCREEN_TITLE)];
+
+  // Initialize the web view.
+  webContents_.reset(content::WebContents::Create(
+      content::WebContents::CreateParams(profile)));
+  NSView* webContentView = webContents_->GetView()->GetNativeView();
+  DCHECK(webContentView);
+
+  NSView* contentView = [window contentView];

[groby-ooo-7-16, 2013/12/04 01:59:48]

Why not make the webContentView the contentView? That'll get auto-resized, so
you can kill windowDidResize:

[noms (inactive), 2013/12/05 17:55:06]

Oh cool. Done!
On 2013/12/04 01:59:48, groby wrote:

+  [webContentView setFrameSize:[contentView frame].size];
+  [contentView addSubview:webContentView];
+
+  // Initialize, but don't show the window until the web contents have loaded.
+  self = [super initWithWindow:window];
+
+  [[NSNotificationCenter defaultCenter] addObserver:self

[groby-ooo-7-16, 2013/12/04 01:59:48]

Missing cleanup - if you register yourself as an observer for the window, you
must remove yourself before either |self| or |window| are deallocated.

|self|, you can handle in -dealloc. |window| is usually handled in
|windowWillClose|

[noms (inactive), 2013/12/05 17:55:06]

Done.

+                                         selector:@selector(windowWillClose:)
+                                             name:NSWindowWillCloseNotification
+                                           object:self.window];
+  [[NSNotificationCenter defaultCenter] addObserver:self
+                                         selector:@selector(windowDidResize:)
+                                             name:NSWindowDidResizeNotification
+                                           object:self.window];
+  return self;
+}
+
+- (void)showURL:(GURL)url {
+  webContents_->GetController().LoadURL(url, content::Referrer(),
+                                        content::PAGE_TRANSITION_AUTO_TOPLEVEL,
+                                        std::string());
+  [self show];
+}
+
+- (void)show {
+  [[self window] makeKeyAndOrderFront:self];

[groby-ooo-7-16, 2013/12/04 01:59:48]

Just inline this into showURL

[noms (inactive), 2013/12/05 17:55:06]

Hmm, it's also used on line 135 (to focus a window that's already showing).
On 2013/12/04 01:59:48, groby wrote:

+}
+
+- (void)hide {
+  [[self window] close];

[groby-ooo-7-16, 2013/12/04 01:59:48]

Be aware that -close can delete the window. You might not want to call this
-hide. People might expect a subsequent -show to work.

[noms (inactive), 2013/12/05 17:55:06]

Renamed to -close.
On 2013/12/04 01:59:48, groby wrote:

+}
+
+-(BOOL)isVisible {
+  return [[self window] isVisible];
+}
+
+- (void)windowWillClose:(NSNotification*)notification {
+  if (userManagerObserver_) {

[groby-ooo-7-16, 2013/12/04 01:59:48]

all WindowWasClosed will do is delete the UserManager - not needed with
BrowserContextKeyedService, since it'll be deleted when the profile goes away.

[noms (inactive), 2013/12/05 17:55:06]

Addressed with separate reply.
On 2013/12/04 01:59:48, groby wrote:

+    userManagerObserver_->WindowWasClosed();
+    userManagerObserver_ = nil;
+  }
+  // Reset the web contents so that the guest profile doesn't have any
+  // renderer hosts, so that the profile can be destroyed cleanly in case the
+  // process is force-killed.
+  webContents_.reset();
+  [self autorelease];

[groby-ooo-7-16, 2013/12/04 01:59:48]

Oy. Self-deleting is never fun. Can you have UserManagerMac own this?

[noms (inactive), 2013/12/05 17:55:06]

Done.

+}
+
+- (void)windowDidResize:(NSNotification*)notification {
+  NSView* webContentView = webContents_->GetView()->GetNativeView();
+  DCHECK(webContentView);
+  [webContentView setFrameSize:[[[self window] contentView] frame].size];
+}
+
+@end
+
+// static
+UserManagerMac* UserManagerMac::instance_ = NULL;

[groby-ooo-7-16, 2013/12/04 01:59:48]

If you _must_ do the instance_ thing (see below re: keyed services), move into
anon namespace.

[noms (inactive), 2013/12/05 17:55:06]

instance_ is in the UserManagerMac namespace (it's a static member). I don't
think I can define it in the anon namespace...can I?
On 2013/12/04 01:59:48, groby wrote:

+
+UserManagerMac::UserManagerMac(Profile* profile) {

[groby-ooo-7-16, 2013/12/04 01:59:48]

I assume the user manager is tied to a specific browser (via
ProfileChooserController, which is per-browser), I'd suggest making it a
BrowserContextKeyedService - automatically ties lifetime to the BrowserContext.

Yes, you _could_ probably live with one instance, but that should simplify
things. You'll just need to either pass BrowserContext to
HideUserManager/ShowUserManager, or get rid of the static functions and just
directly obtain the service when you want to show the dialog.

[noms (inactive), 2013/12/05 17:55:06]

Addressed by separate reply.

On 2013/12/04 01:59:48, groby wrote:

+  window_controller_ = [[UserManagerWindowController alloc]
+      initWithProfile: profile withObserver:this];
+}
+
+UserManagerMac::~UserManagerMac() {
+}
+
+// static
+void UserManagerMac::Show(const base::FilePath& profile_path_to_focus) {
+  if (instance_) {
+    [instance_->window_controller_ show];
+    return;
+  }
+
+  // Create the guest profile, if necessary, and open the User Manager
+  // from the guest profile.
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  profile_manager->CreateProfileAsync(
+      ProfileManager::GetGuestProfilePath(),
+      base::Bind(&UserManagerMac::OnGuestProfileCreated,
+                 profile_path_to_focus),
+      string16(),
+      string16(),
+      std::string());
+}
+
+// static
+void UserManagerMac::Hide() {
+  if (instance_)
+    [instance_->window_controller_ hide];
+}
+
+// static
+bool UserManagerMac::IsShowing() {
+  return instance_ ? [instance_->window_controller_ isVisible]: false;

[groby-ooo-7-16, 2013/12/04 01:59:48]

Seems like this is not called anywhere.

[noms (inactive), 2013/12/05 17:55:06]

Done.

+}
+
+void UserManagerMac::WindowWasClosed() {
+  delete this;
+  instance_ = NULL;
+}
+
+void UserManagerMac::OnGuestProfileCreated(
+    const base::FilePath& profile_path_to_focus,
+    Profile* guest_profile,
+    Profile::CreateStatus status) {
+  if (status != Profile::CREATE_STATUS_INITIALIZED)
+    return;
+
+  instance_ = new UserManagerMac(guest_profile);
+
+  // Tell the webui which user pod should be focused.
+  std::string page = chrome::kChromeUIUserManagerURL;
+
+  if (!profile_path_to_focus.empty()) {
+    ProfileInfoCache& cache =
+        g_browser_process->profile_manager()->GetProfileInfoCache();
+    size_t index = cache.GetIndexOfProfileWithPath(profile_path_to_focus);
+    if (index != std::string::npos) {

[groby-ooo-7-16, 2013/12/04 01:59:48]

Personal nit: I prefer building GURLs via GURLs, not by string concatenation -
but that's really just my preference, so up to you.

GURL url(kChromeUI..);
url_canon::StdStringReplacements<std::string> replacements;

if (!path.empty()) {
  ...
  if (index!=...)
    replacements.SetRefStr(base::IntToString(index));
}

[... showURL:url.ReplaceComponents(replacements)];

[noms (inactive), 2013/12/05 17:55:06]

I'll leave it like this, for now, to be consistent with the views class.

On 2013/12/04 01:59:48, groby wrote:

+      page += "#";
+      page += base::IntToString(index);
+    }
+  }
+  [instance_->window_controller_ showURL:GURL(page)];
+}
+