Reland of: Change default code flag to NewAvatarMenu.

Reland of: Change default code flag to NewAvatarMenu.

Originally landed here: https://codereview.chromium.org/845373002/
Reverted here: https://codereview.chromium.org/1020863007/
because it caused issues on memory bots.

BUG=287883
TBR=rogerta@chromium.org (already reviewed in original CL, there are no changes here)

Committed: https://crrev.com/3ab32772f7c67b70ef25f0a0add62e0bbd6e5f07
Cr-Commit-Position: refs/heads/master@{#324227}

[Mike Lerman, 2015-03-30 20:43:44]

mlerman@chromium.org changed reviewers:
+ noms@chromium.org

[Mike Lerman, 2015-03-30 20:43:44]

Hey Monica,

Can you review the changes I've made since PatchSet #1? Mostly changes to
profile_info_cache and to testing_profile_manager. I was able to remove
sync_error_notifier_ash_unittest.cc and
message_center_settings_controller_unittest.cc as I no longer have a special
song and dance to do around those.

After you, I'll just get sky@ to review testing_profile_manager and TBR the
rest, as it was all done in the original patch.

[noms (inactive), 2015-03-31 20:51:27]

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

https://codereview.chromium.org/1038423004/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_downloader.cc:27:
downloader_active_(true),
What do you think about combining this with disable_avatar_download_for_testing_
into one flag? Maybe  disable_avatar_downloading_, which is set to false by
android, ios, chromeos (in the ProfileInfoCache constructor) and tests wherever
it's set now?

[Mike Lerman, 2015-04-01 13:59:51]

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

https://codereview.chromium.org/1038423004/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_downloader.cc:27:
downloader_active_(true),
On 2015/03/31 20:51:26, Monica Dinculescu wrote:
> What do you think about combining this with
disable_avatar_download_for_testing_
> into one flag? Maybe  disable_avatar_downloading_, which is set to false by
> android, ios, chromeos (in the ProfileInfoCache constructor) and tests
wherever
> it's set now?

We actually don't need this at all anymore, and can just rely on
disable_avatar_download_for_testing_. It's always paired with IsNewAvatarMenu()
which does our OS checks for us already.

[Mike Lerman, 2015-04-01 18:50:11]

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

[Mike Lerman, 2015-04-01 18:50:12]

Hi sky@,

Can you take a look at the testing_profile_manager?

The rest of the files (aside from the c/b/profiles ones Monica looked at) were
all reviewed in the original CL (https://codereview.chromium.org/845373002/),
and this is just a reland. Changes have been made only to profiles files and the
testing_profile_manager. I'll be TBR-ing the rest of the files.

Thanks!

[sky, 2015-04-01 22:26:54]

testing_profile_manager LGTM

[noms (inactive), 2015-04-07 13:55:21]

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

https://codereview.chromium.org/1038423004/diff/70001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:151:
ProfileAvatarDownloader avatar_downloader(
nit: is this still needed? The comment on L560 says this doesn't normally
happen, and you have an explicit test for the downloading, so this seems like it
isn't needed

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

https://codereview.chromium.org/1038423004/diff/70001/chrome/browser/resource...
chrome/browser/resources/options/browser_options_profile_list.js:54: iconEl.alt
= '';
nit: same comment about this holds. why aren't we putting a useful alt text
(like the avatar name or something)

[Mike Lerman, 2015-04-07 15:48:22]

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

https://codereview.chromium.org/1038423004/diff/70001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:151:
ProfileAvatarDownloader avatar_downloader(
On 2015/04/07 13:55:20, noms (slow. ping if too slow) wrote:
> nit: is this still needed? The comment on L560 says this doesn't normally
> happen, and you have an explicit test for the downloading, so this seems like
it
> isn't needed

Good point!

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

https://codereview.chromium.org/1038423004/diff/70001/chrome/browser/resource...
chrome/browser/resources/options/browser_options_profile_list.js:54: iconEl.alt
= '';
On 2015/04/07 13:55:20, noms (slow. ping if too slow) wrote:
> nit: same comment about this holds. why aren't we putting a useful alt text
> (like the avatar name or something)

Because this appears right next to the string which contains the profile name.
There's no point in having "Mike" "Mike" read out twice - might as well just
skip the first one.

If you mean the name of the Avatar (sparky, sunshine, Agent X), I don't think
that will be very useful for someone who's using a read-aloud service.

[noms (inactive), 2015-04-07 19:46:25]

LGTM

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

https://codereview.chromium.org/1038423004/diff/70001/chrome/browser/resource...
chrome/browser/resources/options/browser_options_profile_list.js:54: iconEl.alt
= '';
On 2015/04/07 15:48:21, Mike Lerman wrote:
> On 2015/04/07 13:55:20, noms (slow. ping if too slow) wrote:
> > nit: same comment about this holds. why aren't we putting a useful alt text
> > (like the avatar name or something)
> 
> Because this appears right next to the string which contains the profile name.
> There's no point in having "Mike" "Mike" read out twice - might as well just
> skip the first one.
> 
> If you mean the name of the Avatar (sparky, sunshine, Agent X), I don't think
> that will be very useful for someone who's using a read-aloud service.

Ok. Especially since dbeam@ already LGTM'ed this

[Mike Lerman, 2015-04-07 20:42:35]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-04-07 20:42:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1038423004/#ps90001
(title: "remove unneeded code from ProfileInfoCacheTest")

[Mike Lerman, 2015-04-07 20:42:53]

The CQ bit was unchecked by mlerman@chromium.org

[Mike Lerman, 2015-04-08 12:25:53]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-04-08 12:26:35]

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

[commit-bot: I haz the power, 2015-04-08 14:49:12]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-04-08 14:50:06]

Patchset 6 (id:??) landed as
https://crrev.com/3ab32772f7c67b70ef25f0a0add62e0bbd6e5f07
Cr-Commit-Position: refs/heads/master@{#324227}