Remove references to IsNewAvatarMenu since the flag was removed.

Remove references to IsNewAvatarMenu since the flag was removed.

This CL removes all references of the new-avatar-menu flag since it's
no longer used. The new avatar menu is always shown on desktop and never
on Mobile and ChromeOS.

BUG=517582
Committed: https://crrev.com/66aa95ddaf6ddf1f731fee87f25148051d770bfa
Cr-Commit-Position: refs/heads/master@{#352041}

[anthonyvd, 2015-09-25 22:10:18]

anthonyvd@chromium.org changed reviewers:
+ estade@chromium.org, mlerman@chromium.org, pkasting@chromium.org,
rogerta@chromium.org, rsesek@chromium.org

[anthonyvd, 2015-09-25 22:10:19]

Hello everyone! This CL rips out most of what was left in the code that cared
about the --{enable|disable}-new-avatar-menu command line flag. After this
change there will still be some clean up and refactoring to do but I feel this
is already massive enough to be a good first step.

Mostly, this just removes switches::IsNewAvatarMenu() checks where they would be
redundant and adds some compile time checks in the other places (ChromeOS is the
only platform where the old icon can still be shown outside of Incognito mode).
The #ifdef'd sites will be cleaned up in a follow up CL through some
refactoring.

Can you please take a look at the following files: 

mlerman@: c/b/profiles
rogerta@: c/b/signin, components/signin
pkasting@: c/b/ui, c/b/ui/views, c/b/ui/startup
estade@: c/b/ui/webui
rsesek@: c/b/ui/cocoa

Thanks a lot for your time!

[Evan Stade, 2015-09-25 22:49:11]

webui lgtm, one question on manage_profile_overlay.js

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/options/manage_profile_overlay.js:116:
PageManager.closeOverlay();
shouldn't this line be deleted too

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:502: if (url.host()
== chrome::kChromeUIUserManagerHost) {
nit no curlies

[Peter Kasting, 2015-09-25 23:42:52]

LGTM.

Random grouse: I hope in the future ShouldShowAvatar() gets deleted or changed
to be less confusing -- I can't figure out what that function means today.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (left):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:183:
ASSERT_FALSE(switches::IsNewAvatarMenu());
It's not a problem that we used to force-disable this?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:209:
TEST_F(BrowserCommandControllerTest, NewAvatarMenuEnabledWhenOnlyOneProfile) {
Nit: Remove "New"?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:249: // Both the old
style and the new style avatar menu should be disabled.
Nit: "The avatar menu should be disabled."?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_pages.cc:373: bool should_show_in_avatar_bubble =
Nit: |show_avatar_bubble| is shorter and seems just as clear.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_pages.cc:376: // ChromeOS doesn't have the Avatar
Bubble.
Nit: Don't capitalize Avatar Bubble

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:261: #endif
This can trigger an unreachable code warning.  Instead put the remainder of this
function in an "#else" section.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:290: }
Nit: Compared to the implementation above, this is both slightly shorter and
slightly more efficient.  (Also fixes a non-const ref to be a const ref.)

  const ProfileInfoCache& profile_info =
      g_browser_process->profile_manager()->GetProfileInfoCache();
  size_t profile_index =
      profile_info.GetIndexOfProfileWithPath(last_used_profile->GetPath());
  if ((profile_index == std::string::npos) ||
      !profile_info.ProfileIsSigninRequiredAtIndex(profile_index)) {
    // Signin is not required.  However, guest, system or locked profiles cannot
    // be re-opened on startup. The only exception is if there's already a Guest
    // window open in a separate process (for example, launching a new browser
    // after clicking on a downloaded file in Guest mode).
    if ((!last_used_profile->IsGuestSession() &&
         !last_used_profile->IsSystemProfile()) ||
        (chrome::GetTotalBrowserCountForProfile(
             last_used_profile->GetOffTheRecordProfile()) > 0))
      return false;
  }

  // Show the User Manager.
  profiles::UserManagerProfileSelected action =
      command_line.HasSwitch(switches::kShowAppList) ?
          profiles::USER_MANAGER_SELECT_PROFILE_APP_LAUNCHER :
          profiles::USER_MANAGER_SELECT_PROFILE_NO_ACTION;
  UserManager::Show(base::FilePath(), profiles::USER_MANAGER_NO_TUTORIAL,
                    action);
  return true;
}

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:713: if
(ShowUserManagerOnStartupIfNeeded(last_used_profile, command_line)) {
Nit: Or just:

  if (last_opened_profiles.empty() &&
      ShowUserManagerOnStartupIfNeeded(last_used_profile, command_line))
    return true;

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:79:
UpdateOldAvatarButton();
Nit: Hopefully one of your subsequent cleanup changes will rename all these
things so we don't ever use "old" and "new" in reference to anything...

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:82: // On
desktop, the old avatar button only shows up when in incognito mode.
Nit: I'd combine this comment with the one above and move both above the #ifdef.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:86: // again once
the window is visible.
It's not obvious to me how this comment is related to the next lines of code. 
It seems like maybe this belongs somewhere else, I dunno.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:92: //
|OnProfileAvatarChanged()| in this case, but only for non guest profiles.
Nit: Fix this comment's grammar while here?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:93: if
(!browser_view_->IsGuestSession()) {
Nit: No {}

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/avatar_menu_button.cc:124: #if
!defined(OS_CHROMEOS)
Nit: Remove "!" and reverse arms

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/avatar_menu_button.cc:125: *avatar =
cache.GetAvatarIconOfProfileAtIndex(index);
Nit: Unindent

[Evan Stade, 2015-09-25 23:48:53]

> Random grouse: I hope in the future ShouldShowAvatar() gets deleted or changed
> to be less confusing -- I can't figure out what that function means today.

not that random: I had a similar sentiment and filed crbug.com/534850

[Robert Sesek, 2015-09-28 15:14:48]

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/cocoa_profile_test.h (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/cocoa_profile_test.h:36: CocoaProfileTest(bool
incognito);
Is this change to CocoaProfileTest really necessary? Why can't you just create a
new one-off browser in the one test where you use this ctor?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm:32: 
nit: extra blank line

[Mike Lerman, 2015-09-28 15:23:02]

Hey,

I took a look at more than just profiles :)

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/avatar_menu.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
chrome/browser/profiles/avatar_menu.cc:116: // ChromeOS doesn't have the User
Manager, it can't open it.
ChromeOS, also, I don't think will ever set signin_required for the item to
true, because "lock" is set on a ChromeOS user not a ChromeOS profile. So I
don't think the #if is really necessary, but please verify.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_list_desktop_unittest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_list_desktop_unittest.cc:285: #if
defined(OS_CHROMEOS)
ChromeOS doesn't have an old avatar menu anyways, I don't think. You can remove
this?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_list_desktop_unittest.cc:309:
EXPECT_TRUE(AvatarMenu::ShouldShowAvatarMenu());
Given this method implementation just does an OS check... I don't think it's
useful or necessary?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_window_browsertest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_window_browsertest.cc:89: void
SetUpCommandLine(base::CommandLine* command_line) override {
don't need to override this method anymore.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (left):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:183:
ASSERT_FALSE(switches::IsNewAvatarMenu());
On 2015/09/25 23:42:51, Peter Kasting wrote:
> It's not a problem that we used to force-disable this?

I think we can delete this test now. it tested behaviour specific to the old
avatar menu (where we'd only show the icon when there were 2+ profiles), so the
test can be removed.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:209:
TEST_F(BrowserCommandControllerTest, NewAvatarMenuEnabledWhenOnlyOneProfile) {
On 2015/09/25 23:42:51, Peter Kasting wrote:
> Nit: Remove "New"?

I don't think this test is necessary anymore; there's no logic we're testing,
the BrowserCommandController just always allowed AvatarMenu to be enabled.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (left):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/manage_profile_handler.cc:134:
localized_strings->SetBoolean("newAvatarMenuEnabled",
What about ChromeOS?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc:32:
InProcessBrowserTest::SetUp();
don't need to override SetUp() at all.

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
File components/signin/core/common/profile_management_switches.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
components/signin/core/common/profile_management_switches.cc:20:
STATE_OLD_AVATAR_MENU,
I think we can remove this state now?

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
components/signin/core/common/profile_management_switches.cc:28: bool
is_new_avatar_menu = base::CommandLine::ForCurrentProcess()->HasSwitch(
remove this bool... and remove switches::kEnableNewAvatarMenu?

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
File components/signin/core/common/profile_management_switches.h (right):

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
components/signin/core/common/profile_management_switches.h:39: //void
EnableNewAvatarMenuForTesting(base::CommandLine* command_line);
Add a TODO to remove these methods... or just remove 'em completely!

[Roger Tawa OOO till Jul 10th, 2015-09-28 20:01:29]

lgtm

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
File components/signin/core/common/profile_management_switches.h (right):

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
components/signin/core/common/profile_management_switches.h:39: //void
EnableNewAvatarMenuForTesting(base::CommandLine* command_line);
On 2015/09/28 15:23:02, Mike Lerman wrote:
> Add a TODO to remove these methods... or just remove 'em completely!

Should just remove it all now.

[anthonyvd, 2015-09-29 20:23:14]

anthonyvd@chromium.org changed reviewers:
+ sky@chromium.org

[anthonyvd, 2015-09-29 20:23:16]

Thanks everyone for the feedback, here's a new patchset that should address your
comments :)

sky@ can you please review the two files under chrome/browser/ ?

Thanks a lot!

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/avatar_menu.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
chrome/browser/profiles/avatar_menu.cc:116: // ChromeOS doesn't have the User
Manager, it can't open it.
On 2015/09/28 15:23:01, Mike Lerman wrote:
> ChromeOS, also, I don't think will ever set signin_required for the item to
> true, because "lock" is set on a ChromeOS user not a ChromeOS profile. So I
> don't think the #if is really necessary, but please verify.

It looks like it does here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
unfortunately.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_list_desktop_unittest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_list_desktop_unittest.cc:285: #if
defined(OS_CHROMEOS)
On 2015/09/28 15:23:01, Mike Lerman wrote:
> ChromeOS doesn't have an old avatar menu anyways, I don't think. You can
remove
> this?

True, it can show the icon but never the menu.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_list_desktop_unittest.cc:309:
EXPECT_TRUE(AvatarMenu::ShouldShowAvatarMenu());
On 2015/09/28 15:23:01, Mike Lerman wrote:
> Given this method implementation just does an OS check... I don't think it's
> useful or necessary?

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_window_browsertest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_window_browsertest.cc:89: void
SetUpCommandLine(base::CommandLine* command_line) override {
On 2015/09/28 15:23:01, Mike Lerman wrote:
> don't need to override this method anymore.

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/options/manage_profile_overlay.js:116:
PageManager.closeOverlay();
On 2015/09/25 22:49:10, Evan Stade wrote:
> shouldn't this line be deleted too

You're right, and same thing for the one at line 101 as well. Those links can
never be shown on ChromeOS because the flow to create users is started only from
the lock screen. Because of this, there's no scenarion where we need to close
the overlay.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (left):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:183:
ASSERT_FALSE(switches::IsNewAvatarMenu());
On 2015/09/28 15:23:01, Mike Lerman wrote:
> On 2015/09/25 23:42:51, Peter Kasting wrote:
> > It's not a problem that we used to force-disable this?
> 
> I think we can delete this test now. it tested behaviour specific to the old
> avatar menu (where we'd only show the icon when there were 2+ profiles), so
the
> test can be removed.

It doesn't seem like it's a problem because the test only asserts that the
number of profiles are what we'd expect and that Ctrl+Shift+M is enabled on
Desktop and disabled on ChromeOS. All those things are independant of the
Old/New avatar state so the force disable was just superflous.

As for removing the test, I'd prefer leaving it in, since it does test that
platform-dependant behavior is WAI. I'll rename it to better reflect what it's
actually testing.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:209:
TEST_F(BrowserCommandControllerTest, NewAvatarMenuEnabledWhenOnlyOneProfile) {
On 2015/09/28 15:23:01, Mike Lerman wrote:
> On 2015/09/25 23:42:51, Peter Kasting wrote:
> > Nit: Remove "New"?
> 
> I don't think this test is necessary anymore; there's no logic we're testing,
> the BrowserCommandController just always allowed AvatarMenu to be enabled.

Good point, it's redundant with the test right above so I'll just remove it.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:249: // Both the old
style and the new style avatar menu should be disabled.
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: "The avatar menu should be disabled."?

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_pages.cc:373: bool should_show_in_avatar_bubble =
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: |show_avatar_bubble| is shorter and seems just as clear.

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_pages.cc:376: // ChromeOS doesn't have the Avatar
Bubble.
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: Don't capitalize Avatar Bubble

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/cocoa_profile_test.h (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/cocoa_profile_test.h:36: CocoaProfileTest(bool
incognito);
On 2015/09/28 15:14:47, Robert Sesek wrote:
> Is this change to CocoaProfileTest really necessary? Why can't you just create
a
> new one-off browser in the one test where you use this ctor?

I would end up pretty much copy-pasting the CocoaProfileTest and the CocoaTest
browser creation logic so I figured that it might as well be added here since it
reduces duplication and allows other eventual tests to benefit from it. If you
feel like it doesn't belong I'll be happy to move it to be specific to my test
:)

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm:32: 
On 2015/09/28 15:14:47, Robert Sesek wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:261: #endif
On 2015/09/25 23:42:52, Peter Kasting wrote:
> This can trigger an unreachable code warning.  Instead put the remainder of
this
> function in an "#else" section.

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:290: }
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: Compared to the implementation above, this is both slightly shorter and
> slightly more efficient.  (Also fixes a non-const ref to be a const ref.)
> 
>   const ProfileInfoCache& profile_info =
>       g_browser_process->profile_manager()->GetProfileInfoCache();
>   size_t profile_index =
>       profile_info.GetIndexOfProfileWithPath(last_used_profile->GetPath());
>   if ((profile_index == std::string::npos) ||
>       !profile_info.ProfileIsSigninRequiredAtIndex(profile_index)) {
>     // Signin is not required.  However, guest, system or locked profiles
cannot
>     // be re-opened on startup. The only exception is if there's already a
Guest
>     // window open in a separate process (for example, launching a new browser
>     // after clicking on a downloaded file in Guest mode).
>     if ((!last_used_profile->IsGuestSession() &&
>          !last_used_profile->IsSystemProfile()) ||
>         (chrome::GetTotalBrowserCountForProfile(
>              last_used_profile->GetOffTheRecordProfile()) > 0))
>       return false;
>   }
> 
>   // Show the User Manager.
>   profiles::UserManagerProfileSelected action =
>       command_line.HasSwitch(switches::kShowAppList) ?
>           profiles::USER_MANAGER_SELECT_PROFILE_APP_LAUNCHER :
>           profiles::USER_MANAGER_SELECT_PROFILE_NO_ACTION;
>   UserManager::Show(base::FilePath(), profiles::USER_MANAGER_NO_TUTORIAL,
>                     action);
>   return true;
> }

I feel like your version is a little easier to follow for the reader as well so
done. Thanks!

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:713: if
(ShowUserManagerOnStartupIfNeeded(last_used_profile, command_line)) {
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: Or just:
> 
>   if (last_opened_profiles.empty() &&
>       ShowUserManagerOnStartupIfNeeded(last_used_profile, command_line))
>     return true;

I'm not sure that's equivalent, the rest of the block (line 717-724) is included
under if (last_opened_profiles.empty()). (Unless I'm missing something)

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:79:
UpdateOldAvatarButton();
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: Hopefully one of your subsequent cleanup changes will rename all these
> things so we don't ever use "old" and "new" in reference to anything...

I absolutely agree with that. Along with functional refactors to clean up the
logic, I was thinking about using "Avatar icon" for the "old" avatar button
since it's only shown on ChromeOS teleported windows and it's just the icon
without a menu attached.

As for the "new" button and menu, I was thinking about Identity Chip (which is
what I've heard UX people call that button) and User Menu which is what we've
been calling it internally.

Imho, this should make it clearer. Thoughts?

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:82: // On
desktop, the old avatar button only shows up when in incognito mode.
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: I'd combine this comment with the one above and move both above the
#ifdef.

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:86: // again once
the window is visible.
On 2015/09/25 23:42:52, Peter Kasting wrote:
> It's not obvious to me how this comment is related to the next lines of code. 
> It seems like maybe this belongs somewhere else, I dunno.

I take it as an explanation of why we're calling that function again after the
frame has been made visible instead of while it's being laid out. Not sure where
it would fit better.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:92: //
|OnProfileAvatarChanged()| in this case, but only for non guest profiles.
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: Fix this comment's grammar while here?

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:93: if
(!browser_view_->IsGuestSession()) {
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/avatar_menu_button.cc:124: #if
!defined(OS_CHROMEOS)
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: Remove "!" and reverse arms

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/avatar_menu_button.cc:125: *avatar =
cache.GetAvatarIconOfProfileAtIndex(index);
On 2015/09/25 23:42:52, Peter Kasting wrote:
> Nit: Unindent

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:502: if (url.host()
== chrome::kChromeUIUserManagerHost) {
On 2015/09/25 22:49:10, Evan Stade wrote:
> nit no curlies

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (left):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/manage_profile_handler.cc:134:
localized_strings->SetBoolean("newAvatarMenuEnabled",
On 2015/09/28 15:23:01, Mike Lerman wrote:
> What about ChromeOS?

For context, you can take a look at the comments in the corresponding JS file
but basically the JS code that cares about this value is not accessible on
ChromeOS since they create users from the lock screen rather than the settings
page.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc:32:
InProcessBrowserTest::SetUp();
On 2015/09/28 15:23:01, Mike Lerman wrote:
> don't need to override SetUp() at all.

I swear I know how virtuals work even if it's not apparent in this CL :P

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
File components/signin/core/common/profile_management_switches.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
components/signin/core/common/profile_management_switches.cc:20:
STATE_OLD_AVATAR_MENU,
On 2015/09/28 15:23:01, Mike Lerman wrote:
> I think we can remove this state now?

Done.

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
components/signin/core/common/profile_management_switches.cc:28: bool
is_new_avatar_menu = base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/09/28 15:23:01, Mike Lerman wrote:
> remove this bool... and remove switches::kEnableNewAvatarMenu?

Done.

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
File components/signin/core/common/profile_management_switches.h (right):

https://codereview.chromium.org/1307093004/diff/200001/components/signin/core...
components/signin/core/common/profile_management_switches.h:39: //void
EnableNewAvatarMenuForTesting(base::CommandLine* command_line);
On 2015/09/28 20:01:28, Roger Tawa wrote:
> On 2015/09/28 15:23:02, Mike Lerman wrote:
> > Add a TODO to remove these methods... or just remove 'em completely!
> 
> Should just remove it all now.

Oops! Good catch :)

[Robert Sesek, 2015-09-29 20:31:11]

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/cocoa_profile_test.h (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/cocoa_profile_test.h:36: CocoaProfileTest(bool
incognito);
On 2015/09/29 20:23:15, anthonyvd wrote:
> On 2015/09/28 15:14:47, Robert Sesek wrote:
> > Is this change to CocoaProfileTest really necessary? Why can't you just
create
> a
> > new one-off browser in the one test where you use this ctor?
> 
> I would end up pretty much copy-pasting the CocoaProfileTest and the CocoaTest
> browser creation logic so I figured that it might as well be added here since
it
> reduces duplication and allows other eventual tests to benefit from it. If you
> feel like it doesn't belong I'll be happy to move it to be specific to my test
> :)

You can still inherit from CocoaProfileTest, but you just need to create a new
browser using the fixture's profile(). I don't think this belongs in a common
superclass if there's only one user.

[Peter Kasting, 2015-09-29 20:34:16]

LGTM

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:713: if
(ShowUserManagerOnStartupIfNeeded(last_used_profile, command_line)) {
On 2015/09/29 20:23:15, anthonyvd wrote:
> On 2015/09/25 23:42:52, Peter Kasting wrote:
> > Nit: Or just:
> > 
> >   if (last_opened_profiles.empty() &&
> >       ShowUserManagerOnStartupIfNeeded(last_used_profile, command_line))
> >     return true;
> 
> I'm not sure that's equivalent, the rest of the block (line 717-724) is
included
> under if (last_opened_profiles.empty()). (Unless I'm missing something)

You're right, my suggestion is wrong.  Sorry!

I suggest removing the braces here, though, as seeing that close brace is part
of what led my eye to misread the outer conditional as being closed.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:79:
UpdateOldAvatarButton();
On 2015/09/29 20:23:16, anthonyvd wrote:
> On 2015/09/25 23:42:52, Peter Kasting wrote:
> > Nit: Hopefully one of your subsequent cleanup changes will rename all these
> > things so we don't ever use "old" and "new" in reference to anything...
> 
> I absolutely agree with that. Along with functional refactors to clean up the
> logic, I was thinking about using "Avatar icon" for the "old" avatar button
> since it's only shown on ChromeOS teleported windows and it's just the icon
> without a menu attached.
> 
> As for the "new" button and menu, I was thinking about Identity Chip (which is
> what I've heard UX people call that button) and User Menu which is what we've
> been calling it internally.
> 
> Imho, this should make it clearer. Thoughts?

Seems OK.  It would be nice if it were possible to refer to things as
"incognito" rather than "avatar" if they were in places that could only ever
apply to incognito mode.

I feel like there ought to be some better name than "avatar" that encompasses
both the avatar and incognito functions, like "profile image", but I worry that
anything I think of still sounds confusable with the new User Menu.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:86: // again once
the window is visible.
On 2015/09/29 20:23:15, anthonyvd wrote:
> On 2015/09/25 23:42:52, Peter Kasting wrote:
> > It's not obvious to me how this comment is related to the next lines of
code. 
> > It seems like maybe this belongs somewhere else, I dunno.
> 
> I take it as an explanation of why we're calling that function again after the
> frame has been made visible instead of while it's being laid out. Not sure
where
> it would fit better.

If it's a comment about how this containing function gets called, maybe above
the function, or in the caller?

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:90: // Call
|OnProfileAvatarChanged()| in this case to make sure the task bar icon
Nit: No || on function names

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:91: // is
correctly updated, but only for non guest profiles.
Nit: Is this because guest profiles don't badge the icon?  If so then maybe
instead say "...updated.  Guest profiles don't badge the icon and thus don't
need this." or similar.

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right):

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/avatar_menu_button.cc:127: is_rectangle);
Nit: All fits on one line, I think:

    AvatarMenu::GetImageForMenuButton(profile->GetPath(), avatar, is_rectangle);

[sky, 2015-09-29 22:19:18]

LGTM

[anthonyvd, 2015-09-30 18:34:03]

Thanks again for the comments, the latest patchsets should address the remaining
comments!

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/cocoa_profile_test.h (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/cocoa_profile_test.h:36: CocoaProfileTest(bool
incognito);
On 2015/09/29 20:31:11, Robert Sesek wrote:
> On 2015/09/29 20:23:15, anthonyvd wrote:
> > On 2015/09/28 15:14:47, Robert Sesek wrote:
> > > Is this change to CocoaProfileTest really necessary? Why can't you just
> create
> > a
> > > new one-off browser in the one test where you use this ctor?
> > 
> > I would end up pretty much copy-pasting the CocoaProfileTest and the
CocoaTest
> > browser creation logic so I figured that it might as well be added here
since
> it
> > reduces duplication and allows other eventual tests to benefit from it. If
you
> > feel like it doesn't belong I'll be happy to move it to be specific to my
test
> > :)
> 
> You can still inherit from CocoaProfileTest, but you just need to create a new
> browser using the fixture's profile(). I don't think this belongs in a common
> superclass if there's only one user.

Fair point, done. I don't have access to my mac so I'll run some trybots to see
if I did it correctly.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:713: if
(ShowUserManagerOnStartupIfNeeded(last_used_profile, command_line)) {
On 2015/09/29 20:34:16, Peter Kasting wrote:
> On 2015/09/29 20:23:15, anthonyvd wrote:
> > On 2015/09/25 23:42:52, Peter Kasting wrote:
> > > Nit: Or just:
> > > 
> > >   if (last_opened_profiles.empty() &&
> > >       ShowUserManagerOnStartupIfNeeded(last_used_profile, command_line))
> > >     return true;
> > 
> > I'm not sure that's equivalent, the rest of the block (line 717-724) is
> included
> > under if (last_opened_profiles.empty()). (Unless I'm missing something)
> 
> You're right, my suggestion is wrong.  Sorry!
> 
> I suggest removing the braces here, though, as seeing that close brace is part
> of what led my eye to misread the outer conditional as being closed.

Done.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:79:
UpdateOldAvatarButton();
On 2015/09/29 20:34:16, Peter Kasting wrote:
> On 2015/09/29 20:23:16, anthonyvd wrote:
> > On 2015/09/25 23:42:52, Peter Kasting wrote:
> > > Nit: Hopefully one of your subsequent cleanup changes will rename all
these
> > > things so we don't ever use "old" and "new" in reference to anything...
> > 
> > I absolutely agree with that. Along with functional refactors to clean up
the
> > logic, I was thinking about using "Avatar icon" for the "old" avatar button
> > since it's only shown on ChromeOS teleported windows and it's just the icon
> > without a menu attached.
> > 
> > As for the "new" button and menu, I was thinking about Identity Chip (which
is
> > what I've heard UX people call that button) and User Menu which is what
we've
> > been calling it internally.
> > 
> > Imho, this should make it clearer. Thoughts?
> 
> Seems OK.  It would be nice if it were possible to refer to things as
> "incognito" rather than "avatar" if they were in places that could only ever
> apply to incognito mode.
> 
> I feel like there ought to be some better name than "avatar" that encompasses
> both the avatar and incognito functions, like "profile image", but I worry
that
> anything I think of still sounds confusable with the new User Menu.

There should be a few places at least where specifying incognito directly will
make sense. As for the "Avatar" name, I'll try to find something better. I agree
that it's not a perfect fit.

https://codereview.chromium.org/1307093004/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:86: // again once
the window is visible.
On 2015/09/29 20:34:16, Peter Kasting wrote:
> On 2015/09/29 20:23:15, anthonyvd wrote:
> > On 2015/09/25 23:42:52, Peter Kasting wrote:
> > > It's not obvious to me how this comment is related to the next lines of
> code. 
> > > It seems like maybe this belongs somewhere else, I dunno.
> > 
> > I take it as an explanation of why we're calling that function again after
the
> > frame has been made visible instead of while it's being laid out. Not sure
> where
> > it would fit better.
> 
> If it's a comment about how this containing function gets called, maybe above
> the function, or in the caller?

Good point. Since it's pertinent only to this specific call I'll move it right
above the call site.

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:90: // Call
|OnProfileAvatarChanged()| in this case to make sure the task bar icon
On 2015/09/29 20:34:16, Peter Kasting wrote:
> Nit: No || on function names

Done.

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:91: // is
correctly updated, but only for non guest profiles.
On 2015/09/29 20:34:16, Peter Kasting wrote:
> Nit: Is this because guest profiles don't badge the icon?  If so then maybe
> instead say "...updated.  Guest profiles don't badge the icon and thus don't
> need this." or similar.

Done.

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right):

https://codereview.chromium.org/1307093004/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/avatar_menu_button.cc:127: is_rectangle);
On 2015/09/29 20:34:16, Peter Kasting wrote:
> Nit: All fits on one line, I think:
> 
>     AvatarMenu::GetImageForMenuButton(profile->GetPath(), avatar,
is_rectangle);

Done.

[Mike Lerman, 2015-09-30 19:06:35]

profiles (and the rest) LGTM!

[Robert Sesek, 2015-09-30 21:25:08]

https://chromiumcodereview.appspot.com/1307093004/diff/280001/chrome/browser/...
File chrome/browser/ui/cocoa/profiles/avatar_icon_controller_unittest.mm
(right):

https://chromiumcodereview.appspot.com/1307093004/diff/280001/chrome/browser/...
chrome/browser/ui/cocoa/profiles/avatar_icon_controller_unittest.mm:33: //
Incognito, which is the only way to show the icon
Is this comment still relevant?

https://chromiumcodereview.appspot.com/1307093004/diff/280001/chrome/browser/...
chrome/browser/ui/cocoa/profiles/avatar_icon_controller_unittest.mm:47:
BrowserWindowCocoa* window = (BrowserWindowCocoa*)browser()->window();
nit: C-style casts are banned. static_cast<> instead.

https://chromiumcodereview.appspot.com/1307093004/diff/280001/chrome/browser/...
chrome/browser/ui/cocoa/profiles/avatar_icon_controller_unittest.mm:56:
BrowserWindowCocoa* window = (BrowserWindowCocoa*)browser->window();
No C-style casts.

[anthonyvd, 2015-10-01 14:16:54]

https://chromiumcodereview.appspot.com/1307093004/diff/280001/chrome/browser/...
File chrome/browser/ui/cocoa/profiles/avatar_icon_controller_unittest.mm
(right):

https://chromiumcodereview.appspot.com/1307093004/diff/280001/chrome/browser/...
chrome/browser/ui/cocoa/profiles/avatar_icon_controller_unittest.mm:33: //
Incognito, which is the only way to show the icon
On 2015/09/30 21:25:08, Robert Sesek wrote:
> Is this comment still relevant?

Done.

https://chromiumcodereview.appspot.com/1307093004/diff/280001/chrome/browser/...
chrome/browser/ui/cocoa/profiles/avatar_icon_controller_unittest.mm:47:
BrowserWindowCocoa* window = (BrowserWindowCocoa*)browser()->window();
On 2015/09/30 21:25:08, Robert Sesek wrote:
> nit: C-style casts are banned. static_cast<> instead.

Done.

https://chromiumcodereview.appspot.com/1307093004/diff/280001/chrome/browser/...
chrome/browser/ui/cocoa/profiles/avatar_icon_controller_unittest.mm:56:
BrowserWindowCocoa* window = (BrowserWindowCocoa*)browser->window();
On 2015/09/30 21:25:08, Robert Sesek wrote:
> No C-style casts.

Done.

[Robert Sesek, 2015-10-01 14:29:55]

cocoa/ LGTM

[anthonyvd, 2015-10-02 14:23:29]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2015-10-02 14:23:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, estade@chromium.org, sky@chromium.org,
pkasting@chromium.org, mlerman@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1307093004/#ps300001 (title: "Address
review feedback")

[commit-bot: I haz the power, 2015-10-02 14:24:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307093004/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307093004/300001

[commit-bot: I haz the power, 2015-10-02 15:40:37]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-10-02 15:41:19]

Patchset 16 (id:??) landed as
https://crrev.com/66aa95ddaf6ddf1f731fee87f25148051d770bfa
Cr-Commit-Position: refs/heads/master@{#352041}