Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
102913002:
[Mac] User manager should show up as a standalone window. (Closed)
|
Created:
7 years ago by noms (inactive) Modified:
7 years ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Mac] User manager should show up as a standalone window.
BUG=324040
TEST=With the --new-profile-management flag on, start Chrome and click the avatar button. Select "View All Users". This should bring up the User Manager in a new, top-level window.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241336
Patch Set 1 : #
Total comments: 29
Patch Set 2 : rebase #Patch Set 3 : rachel comments #
Total comments: 18
Patch Set 4 : rebase #Patch Set 5 : rachel comments #
Total comments: 16
Patch Set 6 : rebase #Patch Set 7 : upload error #Patch Set 8 : fix bad rebase #Patch Set 9 : nico comments + a wild test appears! #Patch Set 10 : . #Patch Set 11 : s/browser_test/unit_test/ #
Total comments: 2
Patch Set 12 : rebase #Patch Set 13 : nit #Messages
Total messages: 29 (0 generated)
Hi Rachel, I've made the user manager be a standalone window with a webview, as discussed. Would you mind taking a look, please? Thanks!
https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:51: initWithContentRect:NSMakeRect(0, 0, kWindowWidth, kWindowHeight) 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.
Youch. Did you just delete the patchset? All my comments vanished :(
:( I did. I was getting a lot of 500s cleaned up intermediary patchsets, assuming you hadn't got a chance to look at it. Moar :( On 2013/12/03 21:59:02, groby wrote: > Youch. Did you just delete the patchset? All my comments vanished :(
Many of the comments are about moving to a BrowserContextKeyedService. And then I realized you modelled this closely after the Views implementation :) Not sure if you maybe want to do a later refactor, or try out the BCKS here. Or maybe I'm missing something major, and this is a really bad suggestion? Also, there's common platform-agnostic code. You might want to figure this out into a base class? And finally, this misses the StartKeepAlive/EndKeepAlive stuff views does. (Do you need that? Isn't that just for dialogs that can outlive the browser? Is this supposed to outlive the browser, in which case BKCS is indeed a bad idea?) https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:109: chrome::ShowUserManager(profile_path); Is ShowUserManager OK with an empty path? Should it even be invoked for a guest profile? https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:18: No need for anon namespaces if it's constants only https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:22: const int kWindowHeight = 700; Hm. You might want to dynamically size this, maybe based on browser window. 900x700 will look humongous on e.g. an 11" MBA https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:46: - (id)initWithProfile:(Profile*) profile No space after (Profile*) and other types in methods. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:51: initWithContentRect:NSMakeRect(0, 0, kWindowWidth, kWindowHeight) On 2013/12/03 21:58:28, Monica Dinculescu wrote: > 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. This sounds oddly like a constrained dialog. (Like e.g. print preview). Do you have mocks? https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:65: NSView* contentView = [window contentView]; Why not make the webContentView the contentView? That'll get auto-resized, so you can kill windowDidResize: https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:72: [[NSNotificationCenter defaultCenter] addObserver:self 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| https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:91: [[self window] makeKeyAndOrderFront:self]; Just inline this into showURL https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:95: [[self window] close]; Be aware that -close can delete the window. You might not want to call this -hide. People might expect a subsequent -show to work. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:103: if (userManagerObserver_) { all WindowWasClosed will do is delete the UserManager - not needed with BrowserContextKeyedService, since it'll be deleted when the profile goes away. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:111: [self autorelease]; Oy. Self-deleting is never fun. Can you have UserManagerMac own this? https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:123: UserManagerMac* UserManagerMac::instance_ = NULL; If you _must_ do the instance_ thing (see below re: keyed services), move into anon namespace. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:125: UserManagerMac::UserManagerMac(Profile* profile) { 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. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:160: return instance_ ? [instance_->window_controller_ isVisible]: false; Seems like this is not called anywhere. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:184: if (index != std::string::npos) { 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)];
Before I actually go in an address the comments, here's some general answers that might provide some context. Here is a mock of what it should look like: https://drive.google.com/a/google.com/folderview?id=0B6x6iYCtKinEdENmTkZxZGVS... And a screenshot of what it looks like on Windows (it has since gotten way prettier): https://drive.google.com/file/d/0B1B1Up4p2NRMM0NtMXpWWmJ5czg/edit?usp=sharing The idea is that this User Manager should come up to authenticate users back into Chrome. So if a user signs out of Chrome, they can't go back to their profile unless they unlock it, which can only be through a non-profile-y type of window, which is the User Manager. This also means that the User Manager needs to live off the Guest profile, and not any regular other profile, so that it can be displayed by locked and unlocked profiles. I've actually modelled it closely to the TaskManager widget (with the window controller and the singleton instance), as they're both very similar in look, which is that of a "top level" Chrome window. Unless I have this very wrong, this means it couldn't be a constrained dialog like the print view, as that has an underlying browser window, and just looks like a modal dialog on top. For example, when the User Manager comes up there's no browser window that's already opened for the guest profile. This is also why I'm not sure why the BCKS would help. This is just a standalone-type dialog, and it is *not* tied to a specific browser. It is true that any browser can bring it up through the ProfileChooserController, but the dialog itself always belongs to the Guest profile. This is also why it's designed as a singleton -- you can only have one such User Manager window open at a given time -- they all always look the same, so many of the same-looking-thing is weird. Finally, the reason why the views implementation needed the StartKeepAlive/EndKeepAlive stuff is because on Windows, the browser process dies if there are no open browsers. Since the User Manager can be the only "dialog" displayed, with no other browsers, without the lifecycle stuff the process would be killed. So we're just telling the browser process that while the dialog is being displayed, it shouldn't try to kill the process. The reason why I didn't add it to the Mac code is that the browser process is always alive, and doesn't actually die if all browser windows are closed. Is this incorrect? Hope this sort of explains why the code looks like the way it does! On 2013/12/04 01:59:48, groby wrote: > Many of the comments are about moving to a BrowserContextKeyedService. And then > I realized you modelled this closely after the Views implementation :) > > Not sure if you maybe want to do a later refactor, or try out the BCKS here. Or > maybe I'm missing something major, and this is a really bad suggestion? > > Also, there's common platform-agnostic code. You might want to figure this out > into a base class? > > And finally, this misses the StartKeepAlive/EndKeepAlive stuff views does. (Do > you need that? Isn't that just for dialogs that can outlive the browser? Is this > supposed to outlive the browser, in which case BKCS is indeed a bad idea?) > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:109: > chrome::ShowUserManager(profile_path); > Is ShowUserManager OK with an empty path? Should it even be invoked for a guest > profile? > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/user_manager_mac.mm (right): > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:18: > No need for anon namespaces if it's constants only > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:22: const int kWindowHeight = 700; > Hm. You might want to dynamically size this, maybe based on browser window. > 900x700 will look humongous on e.g. an 11" MBA > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:46: - (id)initWithProfile:(Profile*) > profile > No space after (Profile*) and other types in methods. > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:51: > initWithContentRect:NSMakeRect(0, 0, kWindowWidth, kWindowHeight) > On 2013/12/03 21:58:28, Monica Dinculescu wrote: > > 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. > > This sounds oddly like a constrained dialog. (Like e.g. print preview). Do you > have mocks? > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:65: NSView* contentView = [window > contentView]; > Why not make the webContentView the contentView? That'll get auto-resized, so > you can kill windowDidResize: > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:72: [[NSNotificationCenter > defaultCenter] addObserver:self > 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| > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:91: [[self window] > makeKeyAndOrderFront:self]; > Just inline this into showURL > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:95: [[self window] close]; > Be aware that -close can delete the window. You might not want to call this > -hide. People might expect a subsequent -show to work. > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:103: if (userManagerObserver_) { > all WindowWasClosed will do is delete the UserManager - not needed with > BrowserContextKeyedService, since it'll be deleted when the profile goes away. > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:111: [self autorelease]; > Oy. Self-deleting is never fun. Can you have UserManagerMac own this? > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:123: UserManagerMac* > UserManagerMac::instance_ = NULL; > If you _must_ do the instance_ thing (see below re: keyed services), move into > anon namespace. > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:125: > UserManagerMac::UserManagerMac(Profile* profile) { > 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. > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:160: return instance_ ? > [instance_->window_controller_ isVisible]: false; > Seems like this is not called anywhere. > > https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/user_manager_mac.mm:184: if (index != std::string::npos) > { > 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)];
Thank you for the detailed explanation - that makes indeed a whole lot of sense, and my suggestions around BKCS/ConstrainedWindow are indeed completely wrong. (I'm also quite surprised we don't have a framework for top level windows :)
I think I've addressed most of the comments, minus the one about the initial size being potentially too gigantic. Still looking into alternatives there. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:109: chrome::ShowUserManager(profile_path); Yes. If the path is non empty, the User Manager focuses (makes bigger) the pod whose profile has that path, and if the path is empty, it does nothing. The idea is that if Profile X opens the UM, it should see itself as focused. The guest profile should be allowed to open the user manager, but since it does not appear as a user, it has nothing to focus. On 2013/12/04 01:59:48, groby wrote: > Is ShowUserManager OK with an empty path? Should it even be invoked for a guest > profile? https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:46: - (id)initWithProfile:(Profile*) profile On 2013/12/04 01:59:48, groby wrote: > No space after (Profile*) and other types in methods. Done. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:51: initWithContentRect:NSMakeRect(0, 0, kWindowWidth, kWindowHeight) Addressed by separate reply. On 2013/12/04 01:59:48, groby wrote: > On 2013/12/03 21:58:28, Monica Dinculescu wrote: > > 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. > > This sounds oddly like a constrained dialog. (Like e.g. print preview). Do you > have mocks? https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:65: NSView* contentView = [window contentView]; Oh cool. Done! On 2013/12/04 01:59:48, groby wrote: > Why not make the webContentView the contentView? That'll get auto-resized, so > you can kill windowDidResize: https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:72: [[NSNotificationCenter defaultCenter] addObserver:self On 2013/12/04 01:59:48, groby wrote: > 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| Done. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:91: [[self window] makeKeyAndOrderFront:self]; 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: > Just inline this into showURL https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:95: [[self window] close]; Renamed to -close. On 2013/12/04 01:59:48, groby wrote: > Be aware that -close can delete the window. You might not want to call this > -hide. People might expect a subsequent -show to work. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:103: if (userManagerObserver_) { Addressed with separate reply. On 2013/12/04 01:59:48, groby wrote: > all WindowWasClosed will do is delete the UserManager - not needed with > BrowserContextKeyedService, since it'll be deleted when the profile goes away. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:111: [self autorelease]; On 2013/12/04 01:59:48, groby wrote: > Oy. Self-deleting is never fun. Can you have UserManagerMac own this? Done. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:123: UserManagerMac* UserManagerMac::instance_ = NULL; 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: > If you _must_ do the instance_ thing (see below re: keyed services), move into > anon namespace. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:125: UserManagerMac::UserManagerMac(Profile* profile) { Addressed by separate reply. On 2013/12/04 01:59:48, groby wrote: > 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. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:160: return instance_ ? [instance_->window_controller_ isVisible]: false; On 2013/12/04 01:59:48, groby wrote: > Seems like this is not called anywhere. Done. https://codereview.chromium.org/102913002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:184: if (index != std::string::npos) { I'll leave it like this, for now, to be consistent with the views class. On 2013/12/04 01:59:48, groby wrote: > 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)];
ping! :)
Almost there - and once more, you need to summon the gods of Mac land for the actual LGTM :) https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:111: size_t active_index = avatarMenu_->GetActiveProfileIndex(); I assume the active profile cannot change after buttons have been tagged? https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/user_manager_mac.h (right): https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.h:30: - (void)showURL:(GURL)url; Please pass as const ref, not a copy https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:21: const int kWindowWidth = 900; I'd leave at least a TODO to adjust to smaller screens https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:49: // Center the window on the primary screen. What do you mean by "primary" screen? The one with the menu bar, or the one with keyboard focus? https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:114: // process is force-killed. I still don't understand the reference to force-killing here. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:115: webContents_.reset(); Do you still need the reset? Doesn't the observer destroy this anyways? https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:117: if (userManagerObserver_) { Just DCHECK the observer - should never be NULL. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:119: userManagerObserver_ = nil; There's a tiny potential for conflict here - WindowWasClosed will dealloc the controller, so this technically is dead memory already. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:163: instance_ = NULL; instance_ is technically already freed memory - don't access after delete.
https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:111: size_t active_index = avatarMenu_->GetActiveProfileIndex(); Correct. The buttons are drawn for a bubble that's attached to a browser window. The owner of that browser window is the active profile. If it changes, then we're looking at a different browser window. On 2013/12/10 00:41:06, groby wrote: > I assume the active profile cannot change after buttons have been tagged? https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/user_manager_mac.h (right): https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.h:30: - (void)showURL:(GURL)url; On 2013/12/10 00:41:06, groby wrote: > Please pass as const ref, not a copy Done. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:21: const int kWindowWidth = 900; On 2013/12/10 00:41:06, groby wrote: > I'd leave at least a TODO to adjust to smaller screens Done. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:49: // Center the window on the primary screen. The reference says primary means the one that contains the menu bar. On 2013/12/10 00:41:06, groby wrote: > What do you mean by "primary" screen? The one with the menu bar, or the one with > keyboard focus? > https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:114: // process is force-killed. I meant when you Ctrl-C the process in a terminal. But it's been fixed by the observer destroying this nicely. On 2013/12/10 00:41:06, groby wrote: > I still don't understand the reference to force-killing here. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:115: webContents_.reset(); On 2013/12/10 00:41:06, groby wrote: > Do you still need the reset? Doesn't the observer destroy this anyways? Done. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:117: if (userManagerObserver_) { On 2013/12/10 00:41:06, groby wrote: > Just DCHECK the observer - should never be NULL. Done. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:119: userManagerObserver_ = nil; On 2013/12/10 00:41:06, groby wrote: > There's a tiny potential for conflict here - WindowWasClosed will dealloc the > controller, so this technically is dead memory already. Done. https://codereview.chromium.org/102913002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/user_manager_mac.mm:163: instance_ = NULL; Hmm, instance is static. Freeing the 'this' object shouldn't affect it, right? On 2013/12/10 00:41:06, groby wrote: > instance_ is technically already freed memory - don't access after delete.
Hi Nico, This CL implements the user manager view (already happily running on Windows) as a standalone webview window. Would you mind doing an owner's review, please? I've also added an extra call to close this dialog in BrowserProcessImpl::StartTearDown(), so that if you Ctrl-C or Command-Q the process, the guest profile (which owns the user manager) is cleaned up correctly.
Sorry about the delay, I was out yesterday. Is there any test coverage for the new code? Like, a test that just opens and closes the dialog again for example (I think that would've found one bug; at least on a valgrind bot, but probably also locally) https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/user_manager_mac.h (right): https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.h:23: @private nit: I think we indent @private by 1 https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.h:28: - (id)initWithProfile:(Profile*) profile nit: no space between *) and p https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.h:29: withObserver:(UserManagerMac*) userManagerObserver; (same here) https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.h:33: @end nit: It looks like this class is only used in the implementation file, maybe it can be forward-declared (just `@class UMWC` here and define it in the .mm) https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.mm:17: namespace { nit: const has implicit internal linkage, no need for the unnamed namespace https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.mm:72: webContents_.reset(content::WebContents::Create( This is technically not correct (the best kind of not correct?). webContents_ is an instance variable, so this line really is self->webContents_… and self isn't really initialized yet – or at least the call to [super initWithWindow:] could change it. The usual pattern for object initialization we use in chrome is - (id)init… { // Initialize stack-local variables that are required to call [super init…] // (in this case, |window| if ((self = [super init…])) { // initialize instance variables, do other initialization stuff } return self; } (It probably doesn't make a huge difference in practice in this case, but it's consistent with other classes) https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.mm:157: instance_ = NULL; This too writes to a member variable, so this is really this->instance_ = NULL; Doing this right after deleting this seems optimistic :-)
I think I've addressed all of the comments + added a (sadly, browser) test. Sorry about the use-after-delete sins -- Rachel had even pointed them out and I somehow still managed to reason them away. :( https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/user_manager_mac.h (right): https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.h:23: @private On 2013/12/12 18:05:40, Nico wrote: > nit: I think we indent @private by 1 Done. https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.h:28: - (id)initWithProfile:(Profile*) profile Gah. Done. On 2013/12/12 18:05:40, Nico wrote: > nit: no space between *) and p https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.h:29: withObserver:(UserManagerMac*) userManagerObserver; On 2013/12/12 18:05:40, Nico wrote: > (same here) Done. https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.h:33: @end On 2013/12/12 18:05:40, Nico wrote: > nit: It looks like this class is only used in the implementation file, maybe it > can be forward-declared (just `@class UMWC` here and define it in the .mm) Done. https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.mm:17: namespace { On 2013/12/12 18:05:40, Nico wrote: > nit: const has implicit internal linkage, no need for the unnamed namespace Done. https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.mm:72: webContents_.reset(content::WebContents::Create( Eeeek. Thank you. Fixed and won't do again. On 2013/12/12 18:05:40, Nico wrote: > This is technically not correct (the best kind of not correct?). webContents_ is > an instance variable, so this line really is > > self->webContents_… > > and self isn't really initialized yet – or at least the call to [super > initWithWindow:] could change it. The usual pattern for object initialization we > use in chrome is > > - (id)init… { > // Initialize stack-local variables that are required to call [super init…] > // (in this case, |window| > if ((self = [super init…])) { > // initialize instance variables, do other initialization stuff > } > return self; > } > > > (It probably doesn't make a huge difference in practice in this case, but it's > consistent with other classes) https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.mm:157: instance_ = NULL; OH I see. I herp derped this one. Also, the TaskManager where I copied this from does the same thing, so I'll fix it there too. On 2013/12/12 18:05:40, Nico wrote: > This too writes to a member variable, so this is really > > this->instance_ = NULL; > > Doing this right after deleting this seems optimistic :-)
https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.mm:157: instance_ = NULL; On 2013/12/12 22:59:23, Monica Dinculescu wrote: > OH I see. I herp derped this one. Also, the TaskManager where I copied this from > does the same thing, so I'll fix it there too. The Mac TaskManager was written by someone who had no idea what they were doing, don't trust that code. (But chrome/browser/ui/cocoa/task_manager_mac_unittest.mm has an example of a unit test for this type of code, maybe that could be used as inspiration?)
The User Manager unfortunately does profile magic (needs to create a guest profile, and makes a lot of assumptions on the way), which I can't get to work with unit tests and the testing_profile_manager alone.
Hm. browser_command_controller_unittest.cc has an example how to build Guest TestingProfiles. TestingProfileManager and TestingBrowserProcess should give you the remaining scaffolding. (See avatar_menu_bubble_controller_unittest.mm for example use) Are you sure that's not enough?
Ah! I forgot about that, and I *just* wrote that test. Will give it a try, thanks!
I've converted the basic "can the user manager be shown" test to a unit test. I think that if I add IsShowing() to the UserManager api (in chrome_dialogs), then this can be a cross-platform test, but also increase that api by 50%. I'm not sure it's worth it. Also, the LockProfile test didn't disappear -- it is already covered by ProfileListDesktopBrowserTest::SignOut and didn't need to be rewritten.
Nico: ping! :) On 2013/12/13 20:58:32, Monica Dinculescu wrote: > I've converted the basic "can the user manager be shown" test to a unit test. I > think that if I add IsShowing() to the UserManager api (in chrome_dialogs), then > this can be a cross-platform test, but also increase that api by 50%. I'm not > sure it's worth it. > > Also, the LockProfile test didn't disappear -- it is already covered by > ProfileListDesktopBrowserTest::SignOut and didn't need to be rewritten.
lgtm Sorry. https://codereview.chromium.org/102913002/diff/330001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/102913002/diff/330001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:97: - (NSButton*)makeLinkButtonWithTitle:(NSString*)title (nit: just "linkButtonWithTitle:…" is probably more idiomatic for a function returning an autoreleased object)
On Mon, Dec 16, 2013 at 1:11 PM, <thakis@chromium.org> wrote: > lgtm > > Sorry. > (about the delay, that is) > > > https://codereview.chromium.org/102913002/diff/330001/ > chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm > File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm > (right): > > https://codereview.chromium.org/102913002/diff/330001/ > chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm#newcode97 > chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:97: - > (NSButton*)makeLinkButtonWithTitle:(NSString*)title > (nit: just "linkButtonWithTitle:…" is probably more idiomatic for a > function returning an autoreleased object) > > https://codereview.chromium.org/102913002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Wooohoo thanks! Sending it over to the CQ. https://codereview.chromium.org/102913002/diff/330001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/102913002/diff/330001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:97: - (NSButton*)makeLinkButtonWithTitle:(NSString*)title On 2013/12/16 21:11:27, Nico wrote: > (nit: just "linkButtonWithTitle:…" is probably more idiomatic for a function > returning an autoreleased object) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/102913002/370001
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/102913002/370001
https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/user_manager_mac.mm (right): https://codereview.chromium.org/102913002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/user_manager_mac.mm:157: instance_ = NULL; On 2013/12/12 23:04:09, Nico wrote: > On 2013/12/12 22:59:23, Monica Dinculescu wrote: > > OH I see. I herp derped this one. Also, the TaskManager where I copied this > from > > does the same thing, so I'll fix it there too. > > The Mac TaskManager was written by someone who had no idea what they were doing, > don't trust that code. The person who wrote the mac task manager reached out to me yesterday and politely reminded me that "instance_" is a static variable, not a member, so assigning to it after deleting |this| is perfectly safe and this was ok as it was (but so is assigning to it before deleting |this|, so as it is now is ok too).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/102913002/370001
Message was sent while issue was closed.
Change committed as 241336 |
