[Mac, Win] Show a user manager tutorial once per profile.

[Mac, Win] Show a user manager tutorial once per profile.

This tutorial is only shown when the user explicitly brings up the
user manager, and not, for example, as a result of locking the profile.
This preference can be made syncable in the future.

WebUI change: if there is only one pod, show the tutorial to the
right side of the pod, rather than centered (which is how it is displayed
for multiple pods).

BUG=324052
TEST=Start Chrome with --new-profile-management enabled. Press "View
all users". You should see the tutorial. Close the user manager and
repeat the previous step. You shouldn't see the tutorial again.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258483

[noms (inactive), 2014-03-14 15:50:58]

Hi,

I've added a profile preference to control when the user manager tutorial is
shown. Please take a look. Thanks!

+ Nikita: c/b/resources/user_manager/*
+ Alexei: c/b/ui/cocoa/*
+ Mike: c/b/ui/views/*
+ Gab: c/b/prefs/browser_prefs.cc, c/common/pref_names.*

[gab, 2014-03-14 16:43:57]

https://codereview.chromium.org/199533004/diff/70001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/199533004/diff/70001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:513: registry->RegisterBooleanPref(
The |registry| isn't typically invoked directly from this file, this file
delegates to all the various registrars for their features (it would be a
mega-mess if every feature had it's method here and registered it's prefs
directly in here).

Please move this to a Foo::RegisterProfilePrefs() call where "Foo" is relevant
to your feature (like all the other users of this file do).

https://codereview.chromium.org/199533004/diff/70001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/199533004/diff/70001/chrome/common/pref_names...
chrome/common/pref_names.cc:1944: "profile.user_manager_tutorial_shown";
This is the LOCAL STATE section (see line 1305) is this pref (and the
surrounding prefs) meant to be saved in local state; from this CL it seems like
it's a per profile pref.

[Nikita (slow), 2014-03-14 17:30:34]

c/b/resources/user_manager/* lgtm

https://codereview.chromium.org/199533004/diff/70001/chrome/browser/resources...
File chrome/browser/resources/user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/199533004/diff/70001/chrome/browser/resources...
chrome/browser/resources/user_manager/user_manager_tutorial.js:116:
$('inner-container').style.opacity = '0.4';
nit: Please define a separate CSS class rule and just set it here.

[noms (inactive), 2014-03-14 18:24:36]

Nikita & Gab: addressed your comments.

https://codereview.chromium.org/199533004/diff/70001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/199533004/diff/70001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:513: registry->RegisterBooleanPref(
Done. Moved to profiles::RegisterTutorialPrefs()

On 2014/03/14 16:43:58, gab wrote:
> The |registry| isn't typically invoked directly from this file, this file
> delegates to all the various registrars for their features (it would be a
> mega-mess if every feature had it's method here and registered it's prefs
> directly in here).
> 
> Please move this to a Foo::RegisterProfilePrefs() call where "Foo" is relevant
> to your feature (like all the other users of this file do).

https://codereview.chromium.org/199533004/diff/70001/chrome/browser/resources...
File chrome/browser/resources/user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/199533004/diff/70001/chrome/browser/resources...
chrome/browser/resources/user_manager/user_manager_tutorial.js:116:
$('inner-container').style.opacity = '0.4';
Done. Added a style to screen-container.css
On 2014/03/14 17:30:35, Nikita Kostylev wrote:
> nit: Please define a separate CSS class rule and just set it here.

https://codereview.chromium.org/199533004/diff/70001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/199533004/diff/70001/chrome/common/pref_names...
chrome/common/pref_names.cc:1944: "profile.user_manager_tutorial_shown";
Done. Moved these 4 preferences next to their profiles brethren.
On 2014/03/14 16:43:58, gab wrote:
> This is the LOCAL STATE section (see line 1305) is this pref (and the
> surrounding prefs) meant to be saved in local state; from this CL it seems
like
> it's a per profile pref.

[gab, 2014-03-14 20:52:26]

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:432:
profiles::RegisterTutorialPrefs(registry);
Typically this method is named RegisterProfilePrefs (see all other method calls
above) and it's the class that is more specifically named, e.g.
ProfileTutorials::RegisterProfilePrefs().

Please consider a different place/name for this call for consistency with every
other users of this API.

[Alexei Svitkine (slow), 2014-03-14 21:04:40]

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:606:
browser_->profile()->GetPrefs()->SetBoolean(
Nit: Cache a pointer to the pref store above to avoid calling this chain twice.

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/user_manager_mac.h (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/user_manager_mac.h:29: bool show_tutorial);
Can this be an enum instead so that it's obvious what the true/false means at
the call sites? Alternatively, have the method name be more meaningful - e.g.
ShowWithTutorial().

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/user_manager_mac.mm (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/user_manager_mac.mm:188: page += "#tutorial";
So this case ignores the |profile_path_to_focus| param? Can you document the
behaviour in the header comment?

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/profile_chooser_view.cc:527: bool tutorial_shown =
browser_->profile()->GetPrefs()->GetBoolean(
Same comment about caching profile prefs pointer. Also, add a comment mentioning
the identical logic in the Mac code (and point back to this code from a comment
there).

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/user_manager_view.cc (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/user_manager_view.cc:87: show_tutorial),
I do wonder if there's a way to design this so that this much logic isn't
repeated between the Mac and Windows implementations....

https://codereview.chromium.org/199533004/diff/80001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:1213: // Time when the user's GAIA info was last
updated (represented as an int64).
Is this local machine time or something more authoritative like server time?

What does "represented as an int64" mean? Is it milliseconds? Is it the
ToInternalRepresentation() value? Expand the comment.

[Nikita (slow), 2014-03-17 12:22:15]

lgtm

[msw, 2014-03-17 19:06:18]

Please make corresponding changes to the Views implementation when you address
Alexei's Cocoa comments.

https://codereview.chromium.org/199533004/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:527: bool tutorial_shown =
browser_->profile()->GetPrefs()->GetBoolean(
optional nit: cache a PrefService* for use here and below.

[noms (inactive), 2014-03-17 21:52:22]

Alright. I've shifted and refactored a whole bunch of code around. Please take a
look again :)

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:432:
profiles::RegisterTutorialPrefs(registry);
I've moved the preferences to Profile::RegisterProfilePrefs
On 2014/03/14 20:52:27, gab wrote:
> Typically this method is named RegisterProfilePrefs (see all other method
calls
> above) and it's the class that is more specifically named, e.g.
> ProfileTutorials::RegisterProfilePrefs().
> 
> Please consider a different place/name for this call for consistency with
every
> other users of this API.

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:606:
browser_->profile()->GetPrefs()->SetBoolean(
On 2014/03/14 21:04:40, Alexei Svitkine wrote:
> Nit: Cache a pointer to the pref store above to avoid calling this chain
twice.

Done.

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/user_manager_mac.h (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/user_manager_mac.h:29: bool show_tutorial);
Added an enum as in the future we might (very likely) have a second kind of
tutorial to show. I've put it under profiles:: as it needs to be accessed form
all the places.

On 2014/03/14 21:04:40, Alexei Svitkine wrote:
> Can this be an enum instead so that it's obvious what the true/false means at
> the call sites? Alternatively, have the method name be more meaningful - e.g.
> ShowWithTutorial().

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/user_manager_mac.mm (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/user_manager_mac.mm:188: page += "#tutorial";
On 2014/03/14 21:04:40, Alexei Svitkine wrote:
> So this case ignores the |profile_path_to_focus| param? Can you document the
> behaviour in the header comment?

Done.

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/profile_chooser_view.cc:527: bool tutorial_shown =
browser_->profile()->GetPrefs()->GetBoolean(
On 2014/03/14 21:04:40, Alexei Svitkine wrote:
> Same comment about caching profile prefs pointer. Also, add a comment
mentioning
> the identical logic in the Mac code (and point back to this code from a
comment
> there).

Done.

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/user_manager_view.cc (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/user_manager_view.cc:87: show_tutorial),
Added a helper function in profile_window.cc that does all this magic and runs a
callback for the platform specific code.

On 2014/03/14 21:04:40, Alexei Svitkine wrote:
> I do wonder if there's a way to design this so that this much logic isn't
> repeated between the Mac and Windows implementations....

https://codereview.chromium.org/199533004/diff/80001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/199533004/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:1213: // Time when the user's GAIA info was last
updated (represented as an int64).
On 2014/03/14 21:04:40, Alexei Svitkine wrote:
> Is this local machine time or something more authoritative like server time?
> 
> What does "represented as an int64" mean? Is it milliseconds? Is it the
> ToInternalRepresentation() value? Expand the comment.

Done.

[gab, 2014-03-17 22:13:59]

chrome/browser/prefs/* and chrome/common/pref_names.* lgtm -- much better,
thanks!

[Alexei Svitkine (slow), 2014-03-18 15:50:32]

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:123: void
OnUserManagerGuestProfileCreated(
Add a comment.

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:138: ProfileInfoCache& cache =
Nit: const

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:266: base::string16(),
Nit: Can you add /* */ comments with the name of these params?

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.h (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:72: // pod should be focused.
|callback| is ran once the custom url for the
 What's a user pod?

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:73: // user manager has been decided.
Can you document what the second param of the callback is?

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:536:
profiles::USER_MANAGER_TUTORIAL_OVERVIEW);
I think it would be better to keep the entry point as chrome::ShowUserManager()
and have this logic contained in that function that will be shared between
platforms.

I'm thinking you can have a cross-platform implementation of
chrome::ShowUserManager() that has the above logic and that has some a short set
of of ifdefs that then calls either UserManagerView::Show() or
UserManagerMac::Show() or whichever.

This allows to keep this logic in one place.

(You can probably change chrome::ShowUserManager() to
profiles::ShowUserManager() and put that function in profile_window.h like
you've done for the other code.)

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/user_manager_view.h (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/user_manager_view.h:39: // |status|), creates a new
UserManagerView instance and shows the |url|.
Nit: Bad indentation of comment?

[noms (inactive), 2014-03-18 17:39:08]

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:536:
profiles::USER_MANAGER_TUTORIAL_OVERVIEW);
The problem with that is that ShowUserManager() can be called by code that
doesn't have a valid Profile* pointer (for example, trying to switch to a locked
profile from the fast user switcher), so you can't assume that you can always
read the preference in there. I can add the tutorial_mode to ShowUserManager(),
though, so that all of this logic can go into a helper function which can be
called from both mac and windows (since they each implement
chrome::ShowUserManager in their own way). Sounds good?

On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> I think it would be better to keep the entry point as
chrome::ShowUserManager()
> and have this logic contained in that function that will be shared between
> platforms.
> 
> I'm thinking you can have a cross-platform implementation of
> chrome::ShowUserManager() that has the above logic and that has some a short
set
> of of ifdefs that then calls either UserManagerView::Show() or
> UserManagerMac::Show() or whichever.
> 
> This allows to keep this logic in one place.
> 
> (You can probably change chrome::ShowUserManager() to
> profiles::ShowUserManager() and put that function in profile_window.h like
> you've done for the other code.)

[Alexei Svitkine (slow), 2014-03-18 17:45:40]

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:536:
profiles::USER_MANAGER_TUTORIAL_OVERVIEW);
On 2014/03/18 17:39:09, Monica Dinculescu wrote:
> The problem with that is that ShowUserManager() can be called by code that
> doesn't have a valid Profile* pointer (for example, trying to switch to a
locked
> profile from the fast user switcher), so you can't assume that you can always
> read the preference in there. I can add the tutorial_mode to
ShowUserManager(),
> though, so that all of this logic can go into a helper function which can be
> called from both mac and windows (since they each implement
> chrome::ShowUserManager in their own way). Sounds good?

I see - okay, sgtm.

> 
> On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> > I think it would be better to keep the entry point as
> chrome::ShowUserManager()
> > and have this logic contained in that function that will be shared between
> > platforms.
> > 
> > I'm thinking you can have a cross-platform implementation of
> > chrome::ShowUserManager() that has the above logic and that has some a short
> set
> > of of ifdefs that then calls either UserManagerView::Show() or
> > UserManagerMac::Show() or whichever.
> > 
> > This allows to keep this logic in one place.
> > 
> > (You can probably change chrome::ShowUserManager() to
> > profiles::ShowUserManager() and put that function in profile_window.h like
> > you've done for the other code.)
>

[noms (inactive), 2014-03-19 15:58:19]

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:123: void
OnUserManagerGuestProfileCreated(
On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> Add a comment.

Done.

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:138: ProfileInfoCache& cache =
On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> Nit: const

Done.

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:266: base::string16(),
This is a pretty standard call for this function (used in 3 other places in the
file). I think the comments wouldn't add a whole lot of meaning.

On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> Nit: Can you add /* */ comments with the name of these params?

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.h (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:72: // pod should be focused.
|callback| is ran once the custom url for the
On 2014/03/18 15:50:33, Alexei Svitkine wrote:
>  What's a user pod?

Done.

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:73: // user manager has been decided.
I thought it was already documented, but I've expanded the comment.
On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> Can you document what the second param of the callback is?

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:536:
profiles::USER_MANAGER_TUTORIAL_OVERVIEW);
On 2014/03/18 17:45:40, Alexei Svitkine wrote:
> On 2014/03/18 17:39:09, Monica Dinculescu wrote:
> > The problem with that is that ShowUserManager() can be called by code that
> > doesn't have a valid Profile* pointer (for example, trying to switch to a
> locked
> > profile from the fast user switcher), so you can't assume that you can
always
> > read the preference in there. I can add the tutorial_mode to
> ShowUserManager(),
> > though, so that all of this logic can go into a helper function which can be
> > called from both mac and windows (since they each implement
> > chrome::ShowUserManager in their own way). Sounds good?
> 
> I see - okay, sgtm.
> 
> > 
> > On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> > > I think it would be better to keep the entry point as
> > chrome::ShowUserManager()
> > > and have this logic contained in that function that will be shared between
> > > platforms.
> > > 
> > > I'm thinking you can have a cross-platform implementation of
> > > chrome::ShowUserManager() that has the above logic and that has some a
short
> > set
> > > of of ifdefs that then calls either UserManagerView::Show() or
> > > UserManagerMac::Show() or whichever.
> > > 
> > > This allows to keep this logic in one place.
> > > 
> > > (You can probably change chrome::ShowUserManager() to
> > > profiles::ShowUserManager() and put that function in profile_window.h like
> > > you've done for the other code.)
> > 
> 

Done.

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/user_manager_view.h (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/user_manager_view.h:39: // |status|), creates a new
UserManagerView instance and shows the |url|.
On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> Nit: Bad indentation of comment?

Done.

[Alexei Svitkine (slow), 2014-03-19 16:37:00]

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:138: ProfileInfoCache& cache =
On 2014/03/19 15:58:20, Monica Dinculescu wrote:
> On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> > Nit: const
> 
> Done.

Optional nit: I think in this case you should be able to do a const& since
GetIndexOfProfileWithPath() is const.

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.h (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:73: // user manager has been decided.
I meant the 2nd param of |callback|, not the 2nd param of
|CreateGuestProfileForUserManager|. Make the comment explicitly say that its the
URL somewhere.

On 2014/03/19 15:58:20, Monica Dinculescu wrote:
> I thought it was already documented, but I've expanded the comment.
> On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> > Can you document what the second param of the callback is?
>

https://codereview.chromium.org/199533004/diff/200002/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/200002/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:520: if (end_guest_button_)
Can you preserve some comment explaining this if statement?

https://codereview.chromium.org/199533004/diff/200002/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:520: if (end_guest_button_)
Can you avoid having the logic to call the different functions by just passing a
NULL profile in the guest case?

i.e.

profiles::ShowUserManagerMaybeWithTutorial(
    end_guest_button_ ? NULL : browser_->profile());

[noms (inactive), 2014-03-19 17:10:37]

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.h (right):

https://codereview.chromium.org/199533004/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:73: // user manager has been decided.
On 2014/03/19 16:37:01, Alexei Svitkine wrote:
> I meant the 2nd param of |callback|, not the 2nd param of
> |CreateGuestProfileForUserManager|. Make the comment explicitly say that its
the
> URL somewhere.
> 
> On 2014/03/19 15:58:20, Monica Dinculescu wrote:
> > I thought it was already documented, but I've expanded the comment.
> > On 2014/03/18 15:50:33, Alexei Svitkine wrote:
> > > Can you document what the second param of the callback is?
> > 
> 

Done.

https://codereview.chromium.org/199533004/diff/200002/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/200002/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:520: if (end_guest_button_)
On 2014/03/19 16:37:01, Alexei Svitkine wrote:
> Can you avoid having the logic to call the different functions by just passing
a
> NULL profile in the guest case?
> 
> i.e.
> 
> profiles::ShowUserManagerMaybeWithTutorial(
>     end_guest_button_ ? NULL : browser_->profile());

Done.

https://codereview.chromium.org/199533004/diff/200002/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:520: if (end_guest_button_)
On 2014/03/19 16:37:01, Alexei Svitkine wrote:
> Can you preserve some comment explaining this if statement?

Done.

[Alexei Svitkine (slow), 2014-03-19 17:19:22]

LGTM, thanks!

[noms (inactive), 2014-03-19 17:21:42]

+ Rachel for profiles owners review
Mike: ping! :)

[rpetterson, 2014-03-19 17:58:49]

profiles lgtm

[rpetterson, 2014-03-19 17:59:01]

and a few nits :)

https://codereview.chromium.org/199533004/diff/230001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.h (right):

https://codereview.chromium.org/199533004/diff/230001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:25: USER_MANAGER_TUTORIAL_LOCK       //
Not yet implemented.
Not yet implement -- maybe TODO(noms) would work better for the comment?

https://codereview.chromium.org/199533004/diff/230001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:73: // focused. |callback| is ran with
the custom url to be displayed, as well as
typo: |callback| is *run* with the . . .

[noms (inactive), 2014-03-20 14:15:58]

https://codereview.chromium.org/199533004/diff/230001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.h (right):

https://codereview.chromium.org/199533004/diff/230001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:25: USER_MANAGER_TUTORIAL_LOCK       //
Not yet implemented.
On 2014/03/19 17:59:01, rpetterson wrote:
> Not yet implement -- maybe TODO(noms) would work better for the comment?

Done.

https://codereview.chromium.org/199533004/diff/230001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:73: // focused. |callback| is ran with
the custom url to be displayed, as well as
On 2014/03/19 17:59:01, rpetterson wrote:
> typo: |callback| is *run* with the . . .

Done.

[msw, 2014-03-20 18:51:25]

Thanks for the code consolidation; I have some more nits.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:170: #if !defined(OS_ANDROID) &&
!defined(OS_CHROMEOS)
nit: should this also exclude OS_IOS?

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:152: if (!callback.is_null())
nit: none of the above is necessary if callback.is_null(), move this check up to
the top if statement with the early return.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:261: 
nit: remove blank line

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:264: ProfileManager* profile_manager =
g_browser_process->profile_manager();
nit: inline this below.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.h (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:25: USER_MANAGER_TUTORIAL_LOCK       //
TODO(noms): To be implemented.
nit: include a comma after the terminal enum value.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.h:15: namespace user_prefs { class
PrefRegistrySyncable; }
nit: remove this; it's no longer needed in this file.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:596: // Only
non-guest users appear in the User Manager, or can display a
nit: consider "Guest users cannot appear in the User Manager nor display a
tutorial."

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/user_manager_mac.mm (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/user_manager_mac.mm:149: // Create the guest profile, if
necessary, and open the user manager
nit: if you are trying to make these consistently not capitalized, note that
there are numerous other capitalized instances added by this patch.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/user_manager_mac.mm:169: void
UserManagerMac::OnGuestProfileCreated(Profile* guest_profile,
nit: the ordering of these functions still doesn't match their declarations, but
you don't need to fix that in this patch...

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:520: // Only non-guest users
appear in the User Manager, or can display a
nit: consider "Guest users cannot appear in the User Manager nor display a
tutorial."

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/user_manager_view.h (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/user_manager_view.h:39: // |status|), creates a new
UserManagerView instance and shows the |url|.
nit: |status| no longer exists, please update the comment.

[noms (inactive), 2014-03-20 20:28:19]

Done! Unfortunately a rebase sneaked in there because I had to download the
patch on my windows machine...

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:170: #if !defined(OS_ANDROID) &&
!defined(OS_CHROMEOS)
Good call. Done.
On 2014/03/20 18:51:26, msw wrote:
> nit: should this also exclude OS_IOS?

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:152: if (!callback.is_null())
On 2014/03/20 18:51:26, msw wrote:
> nit: none of the above is necessary if callback.is_null(), move this check up
to
> the top if statement with the early return.

Done.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:261: 
On 2014/03/20 18:51:26, msw wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.cc:264: ProfileManager* profile_manager =
g_browser_process->profile_manager();
On 2014/03/20 18:51:26, msw wrote:
> nit: inline this below.

Done.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profile_window.h (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_window.h:25: USER_MANAGER_TUTORIAL_LOCK       //
TODO(noms): To be implemented.
On 2014/03/20 18:51:26, msw wrote:
> nit: include a comma after the terminal enum value.

Done.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.h:15: namespace user_prefs { class
PrefRegistrySyncable; }
On 2014/03/20 18:51:26, msw wrote:
> nit: remove this; it's no longer needed in this file.

Done.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:596: // Only
non-guest users appear in the User Manager, or can display a
On 2014/03/20 18:51:26, msw wrote:
> nit: consider "Guest users cannot appear in the User Manager nor display a
> tutorial."

Done.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/user_manager_mac.mm (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/user_manager_mac.mm:149: // Create the guest profile, if
necessary, and open the user manager
I don't think I was on purpose. Recapitalized.
On 2014/03/20 18:51:26, msw wrote:
> nit: if you are trying to make these consistently not capitalized, note that
> there are numerous other capitalized instances added by this patch.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/profile_chooser_view.cc (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/profile_chooser_view.cc:520: // Only non-guest users
appear in the User Manager, or can display a
On 2014/03/20 18:51:26, msw wrote:
> nit: consider "Guest users cannot appear in the User Manager nor display a
> tutorial."

Done.

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/user_manager_view.h (right):

https://codereview.chromium.org/199533004/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/user_manager_view.h:39: // |status|), creates a new
UserManagerView instance and shows the |url|.
Done here and in the mac version. On 2014/03/20 18:51:26, msw wrote:
> nit: |status| no longer exists, please update the comment.

[msw, 2014-03-20 20:36:49]

lgtm

[noms (inactive), 2014-03-20 20:38:06]

Sweet! Thanks everyone! Sending it over the CQ.

[noms (inactive), 2014-03-20 20:38:12]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:40:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/199533004/250001

[commit-bot: I haz the power, 2014-03-20 21:51:48]

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

[commit-bot: I haz the power, 2014-03-20 21:51:48]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[noms (inactive), 2014-03-20 21:52:29]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-03-20 21:56:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/199533004/250001

[commit-bot: I haz the power, 2014-03-21 01:04:20]

Change committed as 258483