Mac - show user manager before opening browser.

Mac - show user manager before opening browser.

When no browsers are open and About Chrome, Preferences or
the Task Manager are selected from the menu, the guest profile is by
default used. If the guest profile preference is disabled, then first
show the user manager, then direct to the selected page.

This is somewhat of a follow up to: https://codereview.chromium.org/564453003

BUG=412862
Committed: https://crrev.com/e6e040a21c78c9097600312d761c0295fa9312f1
Cr-Commit-Position: refs/heads/master@{#302202}

[Mike Lerman, 2014-10-07 16:01:51]

mlerman@chromium.org changed reviewers:
+ mark@chromium.org, noms@chromium.org, sky@chromium.org

[Mike Lerman, 2014-10-07 16:01:52]

Hi reviewers,

PTAL!

noms - profiles and user_manager_screen_handler
mark - app_controller_mac (know things about mac)
sky - owners for app_controller_mac

Thanks!

[sky, 2014-10-07 16:22:26]

I'm not a good owner for mac related files. Can you add per-file owners for *.mm
in chrome/browser?

[Mark Mentovai, 2014-10-07 16:35:27]

So if you don’t have any windows open, you can’t get Chrome to tell you about
itself without choosing a profile? That’s a pretty rotten user experience.

[Mike Lerman, 2014-10-07 16:46:32]

On 2014/10/07 16:35:27, Mark Mentovai wrote:
> So if you don’t have any windows open, you can’t get Chrome to tell you about
> itself without choosing a profile? That’s a pretty rotten user experience.

Mark - related to sky's comment, can you recommend some good owners for
chrome/browser/*.mm? Sky, do you know who should be there?

A user would only end up in this scenario if the user has locked their profile
AND guest mode has been blocked. This definitely indicates a user who's privacy
conscious, and this is something we want to respect. The about://chrome page
currently requires a full fledged browser, so we don't really have another
avenue.

[sky, 2014-10-07 17:36:43]

On Tue, Oct 7, 2014 at 9:46 AM,  <mlerman@chromium.org> wrote:
> On 2014/10/07 16:35:27, Mark Mentovai wrote:
>>
>> So if you don't have any windows open, you can't get Chrome to tell you
>> about
>> itself without choosing a profile? That's a pretty rotten user experience.
>
>
> Mark - related to sky's comment, can you recommend some good owners for
> chrome/browser/*.mm? Sky, do you know who should be there?

avi and mark?

  -Scott
>
> A user would only end up in this scenario if the user has locked their
> profile
> AND guest mode has been blocked. This definitely indicates a user who's
> privacy
> conscious, and this is something we want to respect. The about://chrome page
> currently requires a full fledged browser, so we don't really have another
> avenue.
>
> https://codereview.chromium.org/631163004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Lerman, 2014-10-07 17:48:11]

Proposal for reviewers:
mark@ and thomasvl@, who are the reviewers for chrome/browser/mac. I'll put that
in another CL.

[Mark Mentovai, 2014-10-07 17:49:33]

thomasvl hasn’t worked on Chrome in a long time.

[Mark Mentovai, 2014-10-07 20:22:57]

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1109: case IDC_TASK_MANAGER:
How does the task manager interact with this? It’s native, not web UI.

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1414: if (lastProfile->IsGuestSession() &&
!IsGuestModeEnabled()) {
This is getting kind of repetitive. Maybe it could be refactored so that you
could have

// Returns true if it’s safe to open a new browser. Otherwise, shows the user
picker and returns false.
bool CanOpenNewBrowser();

- orderFrontStandardAboutPanel:(id)sender {
  if (ActivateBrowser(…)) {
    chrome::ShowAboutChrome(…);
  } else if (CanOpenNewBrowser()) {
    chrome::OpenAboutWindow(…);
  }
}

https://codereview.chromium.org/631163004/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/631163004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:721:
content::RecordAction(base::UserMetricsAction("TaskManager"));
Seems weird that some things have their UMA actions recorded from this file.
Shouldn’t this happen in a more centralized location, like in ShowTaskManager?

[noms (inactive), 2014-10-07 21:25:48]

https://codereview.chromium.org/631163004/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/631163004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_window.cc:159: profile_path_to_focus !=
ProfileManager::GetGuestProfilePath()) {
Not sure why this would happen. Up until now, |profile_path_to_focus| was always
the path of a legit, non guest Profile...

[Mike Lerman, 2014-10-08 15:41:37]

mark@ and noms@ comments addressed.

Sky@, there is now a small change to /ui/views which will need your fine stamp
of approval.

Thanks!

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1109: case IDC_TASK_MANAGER:
On 2014/10/07 20:22:57, Mark Mentovai wrote:
> How does the task manager interact with this? It’s native, not web UI.

Web UI can be launched from the "Stats for nerds" link. We decided, for
consistency, to gate the Task Manager the same way we do About Chrome.

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1414: if (lastProfile->IsGuestSession() &&
!IsGuestModeEnabled()) {
On 2014/10/07 20:22:57, Mark Mentovai wrote:
> This is getting kind of repetitive. Maybe it could be refactored so that you
> could have
> 
> // Returns true if it’s safe to open a new browser. Otherwise, shows the user
> picker and returns false.
> bool CanOpenNewBrowser();
> 
> - orderFrontStandardAboutPanel:(id)sender {
>   if (ActivateBrowser(…)) {
>     chrome::ShowAboutChrome(…);
>   } else if (CanOpenNewBrowser()) {
>     chrome::OpenAboutWindow(…);
>   }
> }

Done - refactored.

https://codereview.chromium.org/631163004/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/631163004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_window.cc:159: profile_path_to_focus !=
ProfileManager::GetGuestProfilePath()) {
On 2014/10/07 21:25:48, Monica Dinculescu wrote:
> Not sure why this would happen. Up until now, |profile_path_to_focus| was
always
> the path of a legit, non guest Profile...

It was! Until someone (rhymes with poms) changed code (especially on Mac, CL
585653002) to make the Guest Profile the Last Active Profile, which the
app_controller_mac passes the last active's path as the path for the User
Manager.

As per discussion offline, removing this call but adding a DCHECK in
UserManager*::Show().

https://codereview.chromium.org/631163004/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/631163004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:721:
content::RecordAction(base::UserMetricsAction("TaskManager"));
On 2014/10/07 20:22:57, Mark Mentovai wrote:
> Seems weird that some things have their UMA actions recorded from this file.
> Shouldn’t this happen in a more centralized location, like in ShowTaskManager?

This generally happens in the chrome::OpenTaskManager method, which calls
ShowTaskManager. I've changed the call to that method and removed the
RecordAction here.

[sky, 2014-10-08 16:00:05]

chrome/browser/ui/views/profiles/user_manager_view.cc LGTM

[Mark Mentovai, 2014-10-08 16:43:22]

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1109: case IDC_TASK_MANAGER:
Mike Lerman wrote:
> On 2014/10/07 20:22:57, Mark Mentovai wrote:
> > How does the task manager interact with this? It’s native, not web UI.
> 
> Web UI can be launched from the "Stats for nerds" link. We decided, for
> consistency, to gate the Task Manager the same way we do About Chrome.

As opposed to just redirecting the “stats for nerds” click to the profile
chooser? I dunno about that.

https://codereview.chromium.org/631163004/diff/20001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/631163004/diff/20001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:1329: - (bool)canOpenNewBrowser {
You need to document this.

https://codereview.chromium.org/631163004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/631163004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:36: #include
"content/public/browser/user_metrics.h"
No longer necessary.

[Mike Lerman, 2014-10-08 20:23:41]

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/631163004/diff/1/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1109: case IDC_TASK_MANAGER:
On 2014/10/08 16:43:22, Mark Mentovai wrote:
> Mike Lerman wrote:
> > On 2014/10/07 20:22:57, Mark Mentovai wrote:
> > > How does the task manager interact with this? It’s native, not web UI.
> > 
> > Web UI can be launched from the "Stats for nerds" link. We decided, for
> > consistency, to gate the Task Manager the same way we do About Chrome.
> 
> As opposed to just redirecting the “stats for nerds” click to the profile
> chooser? I dunno about that.

Ok. I can do this for Mac implementation (Windows has different dependencies
around a Browser being around to open the Task Manager). Done.

https://codereview.chromium.org/631163004/diff/20001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/631163004/diff/20001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:1329: - (bool)canOpenNewBrowser {
On 2014/10/08 16:43:22, Mark Mentovai wrote:
> You need to document this.

Done.

https://codereview.chromium.org/631163004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/631163004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:36: #include
"content/public/browser/user_metrics.h"
On 2014/10/08 16:43:22, Mark Mentovai wrote:
> No longer necessary.

Indeed.

[Mike Lerman, 2014-10-08 20:26:26]

mlerman@chromium.org changed reviewers:
+ groby@chromium.org, nick@chromium.org

[Mike Lerman, 2014-10-08 20:26:27]

More reviewers please, as this CL grows and takes form. Thanks for your time!

+nick - review task_manager
+groby - review cocoa

[groby-ooo-7-16, 2014-10-08 20:27:42]

c/b/ui/cocoa LGTM

[noms (inactive), 2014-10-08 21:06:08]

https://codereview.chromium.org/631163004/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/task_manager.cc (right):

https://codereview.chromium.org/631163004/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager.cc:1534: if (profile->IsGuestSession())
{
Hmmmm so what happens if you open the task manager from the guest profile (Like
I am legitimately in a guest profile, open the task manager, then open about
memory)?

I realize this is a problem with changing  the active profile from the locked
profile to the guest profile, but maybe we should check if there's any browser
windows open? i.e. If the active profile is guest but browser windows exist,
then maybe we are fine?

[Mike Lerman, 2014-10-09 14:02:45]

https://codereview.chromium.org/631163004/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/task_manager.cc (right):

https://codereview.chromium.org/631163004/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager.cc:1534: if (profile->IsGuestSession())
{
On 2014/10/08 21:06:08, Monica Dinculescu wrote:
> Hmmmm so what happens if you open the task manager from the guest profile
(Like
> I am legitimately in a guest profile, open the task manager, then open about
> memory)?
> 
> I realize this is a problem with changing  the active profile from the locked
> profile to the guest profile, but maybe we should check if there's any browser
> windows open? i.e. If the active profile is guest but browser windows exist,
> then maybe we are fine?

That's why there's a second condition that verifies the BrowserGuestModeEnabled
pref. If you're legitimately in guest mode, then the pref couldn't have been
set. If you're in the user manager but the pref's set to true, then we can just
directly open the chrome://memory page. If the pref is false, then you must be
in the user manager (or have nothing open), and we have no used Last Active to
use, so show the User Manager!

[noms (inactive), 2014-10-09 16:45:13]

profiles lgtm % nit

https://codereview.chromium.org/631163004/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/task_manager.cc (right):

https://codereview.chromium.org/631163004/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager.cc:1534: if (profile->IsGuestSession())
{
On 2014/10/09 14:02:45, Mike Lerman wrote:
> On 2014/10/08 21:06:08, Monica Dinculescu wrote:
> > Hmmmm so what happens if you open the task manager from the guest profile
> (Like
> > I am legitimately in a guest profile, open the task manager, then open about
> > memory)?
> > 
> > I realize this is a problem with changing  the active profile from the
locked
> > profile to the guest profile, but maybe we should check if there's any
browser
> > windows open? i.e. If the active profile is guest but browser windows exist,
> > then maybe we are fine?
> 
> That's why there's a second condition that verifies the
BrowserGuestModeEnabled
> pref. If you're legitimately in guest mode, then the pref couldn't have been
> set. If you're in the user manager but the pref's set to true, then we can
just
> directly open the chrome://memory page. If the pref is false, then you must be
> in the user manager (or have nothing open), and we have no used Last Active to
> use, so show the User Manager!

Ah, got it. Then would you mind combining the two ifs into one (if guest and
!local->state...) to make this clearer?

[Mike Lerman, 2014-10-09 17:17:20]

https://codereview.chromium.org/631163004/diff/40001/chrome/browser/task_mana...
File chrome/browser/task_manager/task_manager.cc (right):

https://codereview.chromium.org/631163004/diff/40001/chrome/browser/task_mana...
chrome/browser/task_manager/task_manager.cc:1534: if (profile->IsGuestSession())
{
On 2014/10/09 16:45:13, Monica Dinculescu wrote:
> On 2014/10/09 14:02:45, Mike Lerman wrote:
> > On 2014/10/08 21:06:08, Monica Dinculescu wrote:
> > > Hmmmm so what happens if you open the task manager from the guest profile
> > (Like
> > > I am legitimately in a guest profile, open the task manager, then open
about
> > > memory)?
> > > 
> > > I realize this is a problem with changing  the active profile from the
> locked
> > > profile to the guest profile, but maybe we should check if there's any
> browser
> > > windows open? i.e. If the active profile is guest but browser windows
exist,
> > > then maybe we are fine?
> > 
> > That's why there's a second condition that verifies the
> BrowserGuestModeEnabled
> > pref. If you're legitimately in guest mode, then the pref couldn't have been
> > set. If you're in the user manager but the pref's set to true, then we can
> just
> > directly open the chrome://memory page. If the pref is false, then you must
be
> > in the user manager (or have nothing open), and we have no used Last Active
to
> > use, so show the User Manager!
> 
> Ah, got it. Then would you mind combining the two ifs into one (if guest and
> !local->state...) to make this clearer?

Done.

[Mark Mentovai, 2014-10-09 20:11:02]

LGTM

[ncarter (slow), 2014-10-13 16:57:05]

lgtm

task_manager lgtm

[ncarter (slow), 2014-10-13 17:07:00]

On 2014/10/13 16:57:05, ncarter wrote:
> lgtm
> 
> task_manager lgtm

Oh and also: did you consider writing a unittest or browsertest to exercise
these redirect paths? It's such a corner case, and I think it would be pretty
amenable to automated testing (assuming you can coax the browsertest into the
profile-less initial state): verify that there's no active profile, verify that
!UserManager::IsShowing, trigger the menu action, verify that
UserManager::IsShowing, drive a profile selection, wait for the active tab to
load the expected chrome:// URL...

[Mike Lerman, 2014-10-30 14:26:37]

Patchset #7 (id:170001) has been deleted

[Mike Lerman, 2014-10-30 14:28:58]

On 2014/10/13 17:07:00, ncarter wrote:
> On 2014/10/13 16:57:05, ncarter wrote:
> > lgtm
> > 
> > task_manager lgtm
> 
> Oh and also: did you consider writing a unittest or browsertest to exercise
> these redirect paths? It's such a corner case, and I think it would be pretty
> amenable to automated testing (assuming you can coax the browsertest into the
> profile-less initial state): verify that there's no active profile, verify
that
> !UserManager::IsShowing, trigger the menu action, verify that
> UserManager::IsShowing, drive a profile selection, wait for the active tab to
> load the expected chrome:// URL...

Nick, I've added the tests you suggested. I had to break it in two, since
there's no way to drive a profile selection from a naturally opened User Manager
without having to friend-expose a lot of internal variables. The two tests cover
the whole flow, from Triggering the Menu to verifying UserManager::IsShowing,
then separately from the user manager driving a profile selection and verifying
the chrome:// URL.

Monica and Mark, if you'd care to review the user_manager_ui_browsertest.cc and
app_controller_mac_browsertest.mm again, then I'll send this into the CQ.
Thanks!

[Mark Mentovai, 2014-10-30 17:34:28]

LGTM still

https://codereview.chromium.org/631163004/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc (right):

https://codereview.chromium.org/631163004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc:3: // found in the
LICENSE file.
There seems to be a missing blank line after this.

https://codereview.chromium.org/631163004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc:70: std::string
userManagerURL = chrome::kChromeUIUserManagerURL;
What’s up with the naming? user_manager_url.

[Mike Lerman, 2014-10-30 17:37:25]

https://codereview.chromium.org/631163004/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc (right):

https://codereview.chromium.org/631163004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc:3: // found in the
LICENSE file.
On 2014/10/30 17:34:27, Mark Mentovai wrote:
> There seems to be a missing blank line after this.

Done.

https://codereview.chromium.org/631163004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc:70: std::string
userManagerURL = chrome::kChromeUIUserManagerURL;
On 2014/10/30 17:34:27, Mark Mentovai wrote:
> What’s up with the naming? user_manager_url.

Sorry, went on vacation and my brain reverted to Java.

Done.

[noms (inactive), 2014-10-30 17:45:11]

still lgtm % nits

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:281: 
nit: is there an extra \n here?

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:299: // Tell the browser to
open About Chrome
nit: comment should end in a .

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:307:
EXPECT_TRUE(UserManager::IsShowing());
I think you have to explicitly call UserManager::Hide(); like in the other
tests, or else the test won't die (because the UserManager messes with the
application lifecycle)

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:362: 
nit: i think there's an extra \n?

[Mike Lerman, 2014-10-30 18:06:02]

Thanks Monica and Mark!

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:281: 
On 2014/10/30 17:45:11, Monica Dinculescu wrote:
> nit: is there an extra \n here?

Done.

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:299: // Tell the browser to
open About Chrome
On 2014/10/30 17:45:11, Monica Dinculescu wrote:
> nit: comment should end in a .

Done.

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:307:
EXPECT_TRUE(UserManager::IsShowing());
On 2014/10/30 17:45:11, Monica Dinculescu wrote:
> I think you have to explicitly call UserManager::Hide(); like in the other
> tests, or else the test won't die (because the UserManager messes with the
> application lifecycle)

Done.

https://codereview.chromium.org/631163004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:362: 
On 2014/10/30 17:45:11, Monica Dinculescu wrote:
> nit: i think there's an extra \n?

Done.

[Mike Lerman, 2014-10-30 18:06:15]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-10-30 18:08:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631163004/230001

[commit-bot: I haz the power, 2014-10-31 00:10:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-31 00:10:38]

Exceeded time limit waiting for builds to trigger.

[Mike Lerman, 2014-10-31 00:13:37]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-10-31 00:14:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631163004/230001

[commit-bot: I haz the power, 2014-10-31 00:53:29]

Committed patchset #9 (id:230001)

[commit-bot: I haz the power, 2014-10-31 00:54:01]

Patchset 9 (id:??) landed as
https://crrev.com/e6e040a21c78c9097600312d761c0295fa9312f1
Cr-Commit-Position: refs/heads/master@{#302202}