Change default code flag to NewAvatarMenu.

Change default code flag to NewAvatarMenu. Flags or Finch Configs still take priority, but all else being equal we should direct the user to NewAvatarMenu.

This CL fixes a few issues that arise when the flag's on:
- Missing alt text on chrome://settings
- Unit tests for old components (OldAvatarMenu) need to force that state
- The ProfileAvatarDownloader works automatically with NewAvatarMenu on - this needs to be accounted for so it doesn't leave images hanging around.
- BookmarkBubbleSigninDelegate now triggers the User Menu, not a Sign in tab, on Desktop. (note this can't be easily tested in Cocoa)

BUG=287883
Committed: https://crrev.com/e4d285b050ab3429f7d937fde6234fcba07a360e
Cr-Commit-Position: refs/heads/master@{#321770}

[Mike Lerman, 2015-03-06 20:34:22]

mlerman@chromium.org changed reviewers:
+ dbeam@chromium.org, jcivelli@chromium.org, noms@chromium.org,
rogerta@chromium.org, shess@chromium.org, sky@chromium.org

[Mike Lerman, 2015-03-06 20:34:23]

Hi all,

I'm trying to move the Chrome codebase to use NewAvatarMenu as default (still
overrideable), so that what runs on the bots matches stable. Had some fix-up to
do along the way. PTAL.

noms - c/b/profiles, the testing_profile_manager and all the ui/* tests? And
overall?
dbeam - c/b/resources/options
shess - c/b/ui/cocoa
sky - c/b/ui/views/bookmarks, c/b/ui/bookmarks and c/b/ui/views/profiles
rogerta - components/signin
jcivelli - c/test

[Dan Beam, 2015-03-06 20:35:13]

lgtm

[Scott Hess - ex-Googler, 2015-03-07 04:33:07]

lgtm for cocoa/

https://codereview.chromium.org/845373002/diff/220001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/220001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm:37:
testing_profile_manager()->CreateTestingProfile(
Why are you going to the point of making this change?  The other unit test in
this CL uses testing_profile_manager_., so this doesn't make things consistent
(or maybe it does and that one should also change to make things consistent).

[Roger Tawa OOO till Jul 10th, 2015-03-09 14:27:18]

lgtm

Should you remove explicit calls to EnableNewAvatarMenuForTesting() in tests?

[Mike Lerman, 2015-03-09 14:28:34]

Roger - I was going to remove EnableNewAvatarMenuForTesting in another CL.

https://codereview.chromium.org/845373002/diff/220001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/220001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm:37:
testing_profile_manager()->CreateTestingProfile(
On 2015/03/07 04:33:07, Scott Hess wrote:
> Why are you going to the point of making this change?  The other unit test in
> this CL uses testing_profile_manager_., so this doesn't make things consistent
> (or maybe it does and that one should also change to make things consistent).

You're right. I had made this change as part of an alternate solution and didn't
revert it back. I should, though - no need to make unnecessary changes. Makes
git blame harder to follow.

[Scott Hess - ex-Googler, 2015-03-09 15:36:28]

LGTM with cocoa, but slight indent fix.

https://codereview.chromium.org/845373002/diff/220001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/220001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm:37:
testing_profile_manager()->CreateTestingProfile(
On 2015/03/09 14:28:34, Mike Lerman wrote:
> On 2015/03/07 04:33:07, Scott Hess wrote:
> > Why are you going to the point of making this change?  The other unit test
in
> > this CL uses testing_profile_manager_., so this doesn't make things
consistent
> > (or maybe it does and that one should also change to make things
consistent).
> 
> You're right. I had made this change as part of an alternate solution and
didn't
> revert it back. I should, though - no need to make unnecessary changes. Makes
> git blame harder to follow.

OK - that's kind of what I figured, but wondered if maybe it was a long-term
adjustment you were aiming for ...

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm:40:
TestingProfile::TestingFactories());
This and the next call are one space too indented, now :-).

[sky, 2015-03-09 15:54:20]

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/views...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:27:

nit: remove this newline.

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:54:
base::MessageLoop::current()->RunUntilIdle();
We do you need the RunUntilIdle?

The pattern you have here is rather confusing. I think it'll be easier to read
if you change this to something like IsProfilerChooserShowing() (no args), and
move the assertion to the call site, eg instead of:

  ExpectProfileChooserShows(true);

you have:

  EXPECT_TRUE(IsProfileChooserShowing());

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:71:
#if defined(OS_CHROMEOS)
All these ifdefs make it hard to follow. I think it would be easier if the
condition was around a constant, eg:

if (kHasProfileChooser) {
  ...
}

[noms (inactive), 2015-03-09 19:26:18]

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:125: // A bitmap for the
high res avatar cache.  2x2 bitmap of green 32 bit pixels.
nit: i don't think the comment matches the actual number of pixels :)

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:152:
GetCache()->GetAvatarIconIndexOfProfileAtIndex(i),
This should only run on desktop. Isn't AddProfiles a generic method used in
non-desktop tests as well?

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:618: base::FilePath
icon_path2 =
isn't icon_path2 == icon_path, that you've already defined on L566? Why do we
need a second one?

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller_unittest.mm:26:
base::CommandLine::ForCurrentProcess());
But don't we get this bubble with or without the flag? Why do we need to disable
it here?

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm:36:
switches::DisableNewAvatarMenuForTesting(
Why are we disabling the flag? This is the os menu bar, not the old profile
bubble, right?

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/views...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:152:
EXPECT_EQ(starting_tab_count + 1, tab_count);
I don't think this will compile (given that starting_tab_count isn't always
defined)

https://codereview.chromium.org/845373002/diff/280001/chrome/test/base/testin...
File chrome/test/base/testing_profile_manager.cc (right):

https://codereview.chromium.org/845373002/diff/280001/chrome/test/base/testin...
chrome/test/base/testing_profile_manager.cc:105: ProfileAvatarDownloader
avatar_downloader(
nit: same question as before. Doesn't this run on non-desktop platforms too?

[Mike Lerman, 2015-03-10 15:58:59]

Comments addressed.

Note I'm still working to resolve the ASAN issues, which will require changes to
testing_profile_manager and/or profile_info_cache.

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_menu_bubble_controller_unittest.mm:40:
TestingProfile::TestingFactories());
On 2015/03/09 15:36:27, Scott Hess wrote:
> This and the next call are one space too indented, now :-).

Done.

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/views...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:27:

On 2015/03/09 15:54:20, sky wrote:
> nit: remove this newline.

Done.

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:54:
base::MessageLoop::current()->RunUntilIdle();
On 2015/03/09 15:54:20, sky wrote:
> We do you need the RunUntilIdle?
> 
> The pattern you have here is rather confusing. I think it'll be easier to read
> if you change this to something like IsProfilerChooserShowing() (no args), and
> move the assertion to the call site, eg instead of:
> 
>   ExpectProfileChooserShows(true);
> 
> you have:
> 
>   EXPECT_TRUE(IsProfileChooserShowing());

We don't need the Runloop();. I thought we didn't, but hadn't tested that.

Ok - pattern changed. I had that from before I had changed certain things about
this test.

https://codereview.chromium.org/845373002/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:71:
#if defined(OS_CHROMEOS)
On 2015/03/09 15:54:20, sky wrote:
> All these ifdefs make it hard to follow. I think it would be easier if the
> condition was around a constant, eg:
> 
> if (kHasProfileChooser) {
>   ...
> }

Done.

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:125: // A bitmap for the
high res avatar cache.  2x2 bitmap of green 32 bit pixels.
On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> nit: i don't think the comment matches the actual number of pixels :)

Done.

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:152:
GetCache()->GetAvatarIconIndexOfProfileAtIndex(i),
On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> This should only run on desktop. Isn't AddProfiles a generic method used in
> non-desktop tests as well?

As discussed offline - the ProfileAvatarDownloader now won't do anything on
non-desktop platforms.

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:618: base::FilePath
icon_path2 =
On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> isn't icon_path2 == icon_path, that you've already defined on L566? Why do we
> need a second one?

Maybe? I suppose so.

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_sync_promo_controller_unittest.mm:26:
base::CommandLine::ForCurrentProcess());
On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> But don't we get this bubble with or without the flag? Why do we need to
disable
> it here?

The SyncPromo calls chrome::ShowBrowserSignin, which shows either a bubble or a
tab based on IsNewAvatarMenu(), and we can't really detect if the
ProfileChooserController's showing like we can in Views, so we just test the old
flow here.

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm:36:
switches::DisableNewAvatarMenuForTesting(
On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> Why are we disabling the flag? This is the os menu bar, not the old profile
> bubble, right?

Is it? Oh. Trying to count the numbers of items in the ProfileMenuController was
reporting wrong numbers. I'll change this so it works properly with the NewMenu
rather than sticking to the old ones.

Please can you verify that it's supposed to always be showing (even with just
one profile)?

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/views...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:152:
EXPECT_EQ(starting_tab_count + 1, tab_count);
On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> I don't think this will compile (given that starting_tab_count isn't always
> defined)

Done.

https://codereview.chromium.org/845373002/diff/280001/chrome/test/base/testin...
File chrome/test/base/testing_profile_manager.cc (right):

https://codereview.chromium.org/845373002/diff/280001/chrome/test/base/testin...
chrome/test/base/testing_profile_manager.cc:105: ProfileAvatarDownloader
avatar_downloader(
On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> nit: same question as before. Doesn't this run on non-desktop platforms too?

Same answer as before. The ProfileAvatarDownloader will self-regulate to not do
anything on non-desktop platforms.

[Mike Lerman, 2015-03-11 16:04:40]

Patchset #22 (id:420001) has been deleted

[noms (inactive), 2015-03-17 14:26:31]

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm:36:
switches::DisableNewAvatarMenuForTesting(
On 2015/03/10 15:58:58, Mike Lerman wrote:
> On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> > Why are we disabling the flag? This is the os menu bar, not the old profile
> > bubble, right?
> 
> Is it? Oh. Trying to count the numbers of items in the ProfileMenuController
was
> reporting wrong numbers. I'll change this so it works properly with the
NewMenu
> rather than sticking to the old ones.
> 
> Please can you verify that it's supposed to always be showing (even with just
> one profile)?

At the moment for one profile, it's showing up in the menu bar (it also has the
additional Edit | New options which are useful) but not in the dock menu (I
think this class controls both).

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_downloader.cc (right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_downloader.cc:41: if (downloader_active_)
{
nit: i would return early if !downloader_active

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:575: base::string16(),
kIconIndex, std::string());
nit: alignment here is whack :)

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options_profile_list.js (right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/resource...
chrome/browser/resources/options/browser_options_profile_list.js:54: iconEl.alt
= '';
nit: can we/should we put something useful in here? like the profile name and
"avatar" or something?

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_error_notifier_ash_unittest.cc (right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/sync/syn...
chrome/browser/sync/sync_error_notifier_ash_unittest.cc:108:
base::MessageLoop::current()->RunUntilIdle();
Should you have an assert after the MessageLoop bit that the avatar actually got
"downloaded"?

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/ui/views...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:109:
int tab_count_incognito = incognito_browser->tab_strip_model()->count();
nit: L108-109 are the same as L117-L120

[Mike Lerman, 2015-03-18 19:29:48]

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/845373002/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_menu_controller_unittest.mm:36:
switches::DisableNewAvatarMenuForTesting(
On 2015/03/17 14:26:30, Monica Dinculescu wrote:
> On 2015/03/10 15:58:58, Mike Lerman wrote:
> > On 2015/03/09 19:26:17, noms (OOO-slow to respond) wrote:
> > > Why are we disabling the flag? This is the os menu bar, not the old
profile
> > > bubble, right?
> > 
> > Is it? Oh. Trying to count the numbers of items in the ProfileMenuController
> was
> > reporting wrong numbers. I'll change this so it works properly with the
> NewMenu
> > rather than sticking to the old ones.
> > 
> > Please can you verify that it's supposed to always be showing (even with
just
> > one profile)?
> 
> At the moment for one profile, it's showing up in the menu bar (it also has
the
> additional Edit | New options which are useful) but not in the dock menu (I
> think this class controls both). 

Okay, good. I'll leave it Enabled then.

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_downloader.cc (right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_downloader.cc:41: if (downloader_active_)
{
On 2015/03/17 14:26:30, Monica Dinculescu wrote:
> nit: i would return early if !downloader_active

Done.

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache_unittest.cc (right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:575: base::string16(),
kIconIndex, std::string());
On 2015/03/17 14:26:30, Monica Dinculescu wrote:
> nit: alignment here is whack :)

Done.

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options_profile_list.js (right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/resource...
chrome/browser/resources/options/browser_options_profile_list.js:54: iconEl.alt
= '';
On 2015/03/17 14:26:30, Monica Dinculescu wrote:
> nit: can we/should we put something useful in here? like the profile name and
> "avatar" or something?

It's right next to the profile's name, if you can't see the image adds no value.
For accessibility, when this is being read out loud, it's probably more useful
just to not read the name twice, but only once.

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_error_notifier_ash_unittest.cc (right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/sync/syn...
chrome/browser/sync/sync_error_notifier_ash_unittest.cc:108:
base::MessageLoop::current()->RunUntilIdle();
On 2015/03/17 14:26:30, Monica Dinculescu wrote:
> Should you have an assert after the MessageLoop bit that the avatar actually
got
> "downloaded"?

Done.

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/ui/views...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/845373002/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:109:
int tab_count_incognito = incognito_browser->tab_strip_model()->count();
On 2015/03/17 14:26:30, Monica Dinculescu wrote:
> nit: L108-109 are the same as L117-L120

Ooh, nice! Done.

[Mike Lerman, 2015-03-19 15:53:32]

mlerman@chromium.org changed reviewers:
+ mukai@chromium.org
- jcivelli@chromium.org

[Mike Lerman, 2015-03-19 15:53:33]

Ping reviewers! Thanks :)

sky - c/b/ui/views/bookmarks, c/b/ui/bookmarks and c/b/ui/views/profiles, c/test
too
noms - c/b/profiles
mukai - c/b/notifications

(jcivelli removed, since sky's now a c/test reviewer)

[Jun Mukai, 2015-03-19 16:51:20]

https://codereview.chromium.org/845373002/diff/540001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller_unittest.cc
(right):

https://codereview.chromium.org/845373002/diff/540001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:45:
ProfileInfoCache& cache = *testing_profile_manager_.profile_info_cache();
Why not
ProfileInfoCache* cache = testing_profile_manager_.profile_info_cache();
cache->GetAvatar...(cache->GetIndex...())

?

[sky, 2015-03-19 16:51:55]

LGTM

[noms (inactive), 2015-03-19 16:53:16]

lgtm % nit

https://codereview.chromium.org/845373002/diff/540001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller_unittest.cc
(right):

https://codereview.chromium.org/845373002/diff/540001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:45:
ProfileInfoCache& cache = *testing_profile_manager_.profile_info_cache();
nit: const?

[Mike Lerman, 2015-03-19 17:23:56]

https://codereview.chromium.org/845373002/diff/540001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller_unittest.cc
(right):

https://codereview.chromium.org/845373002/diff/540001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:45:
ProfileInfoCache& cache = *testing_profile_manager_.profile_info_cache();
On 2015/03/19 16:53:16, Monica Dinculescu wrote:
> nit: const?

I'll go with const &, the standard way of referencing the ProfileInfoCache. Is
that okay, mukai@?

[Jun Mukai, 2015-03-19 18:27:45]

c/b/notifications lgtm, with a comment.

https://codereview.chromium.org/845373002/diff/560001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller_unittest.cc
(right):

https://codereview.chromium.org/845373002/diff/560001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:49:
cache.GetIndexOfProfileWithPath(profile->GetPath()));
Personally I think it would be better to be a pointer.
Invoking a method for some 'preloading' doesn't look like a task of a const
method. Indeed GetAvatarIconOfProfileAtIndex() is const method for some reasons,
but if you want to make a side effect like preparing data for this method, I
think a mutable pointer would be matching.

Anyways I don't have strong opinions. It's up to you.

[Jun Mukai, 2015-03-19 18:34:25]

https://codereview.chromium.org/845373002/diff/560001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller_unittest.cc
(right):

https://codereview.chromium.org/845373002/diff/560001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:49:
cache.GetIndexOfProfileWithPath(profile->GetPath()));
Ah, could you also add more comments for why this is necessary?
If you remove this new code, what kind of bad thing do we have?

[Mike Lerman, 2015-03-19 19:27:30]

Monica, can you also take a look at the changes I made to ProfileInfoCache and
the UserManagerScreenHandler, re: making GetAvatarIconOfProfileAtIndex not
const? Thanks.

https://codereview.chromium.org/845373002/diff/560001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller_unittest.cc
(right):

https://codereview.chromium.org/845373002/diff/560001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:49:
cache.GetIndexOfProfileWithPath(profile->GetPath()));
On 2015/03/19 18:34:24, Jun Mukai wrote:
> Ah, could you also add more comments for why this is necessary?
> If you remove this new code, what kind of bad thing do we have?

Changed to a pointer. Comment added.

[Jun Mukai, 2015-03-19 19:45:58]

lgtm. thanks!

[noms (inactive), 2015-03-19 19:46:53]

still lgtm

[Mike Lerman, 2015-03-23 12:51:09]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-03-23 12:51:09]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
rogerta@chromium.org, shess@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/845373002/#ps580001
(title: "Test comment. GetAvatarIcon's no longer const.")

[commit-bot: I haz the power, 2015-03-23 12:51:25]

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

[commit-bot: I haz the power, 2015-03-23 14:26:29]

Committed patchset #29 (id:580001)

[commit-bot: I haz the power, 2015-03-23 14:27:36]

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

[Mike Lerman, 2015-03-23 16:27:07]

A revert of this CL (patchset #29 id:580001) has been created in
https://codereview.chromium.org/1020863007/ by mlerman@chromium.org.

The reason for reverting is: Broke Linux Valgrind tests on the waterfall. I'll
need to do more testing / work on this CL before it's ready to land.

http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v....