Removes the new-avatar-menu flags, enabling the feature for all users.

Removes the new-avatar-menu flags, enabling the feature for all users.

This CL removes the option to use the old avatar menu but doesn't fully
remove the associated code. That cleanup will happen as a follow up.

BUG=451920
Committed: https://crrev.com/84f76680c873f334c531ebddee6b9bbde43dbd0e
Cr-Commit-Position: refs/heads/master@{#330775}

[anthonyvd, 2015-05-11 20:10:11]

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

[anthonyvd, 2015-05-11 20:10:11]

Hi Roger, can you please take a quick look at this CL that removes the option to
toggle back to the old avatar menu?

Thanks!

[Roger Tawa OOO till Jul 10th, 2015-05-11 20:20:42]

lgtm

[anthonyvd, 2015-05-15 15:38:42]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-15 15:39:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124383004/1

[commit-bot: I haz the power, 2015-05-15 16:29:27]

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

[commit-bot: I haz the power, 2015-05-15 16:29:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[anthonyvd, 2015-05-15 19:20:38]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[anthonyvd, 2015-05-15 19:20:38]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1124383004/#ps20001
(title: "Removed OldAvatarMenu tests now that it can't be used.")

[commit-bot: I haz the power, 2015-05-15 19:23:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124383004/20001

[commit-bot: I haz the power, 2015-05-15 19:32:11]

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

[commit-bot: I haz the power, 2015-05-15 19:32:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[anthonyvd, 2015-05-15 19:32:46]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[anthonyvd, 2015-05-15 19:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1124383004/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-15 19:35:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124383004/40001

[anthonyvd, 2015-05-15 19:56:05]

Hello guys,

This CL removes the new-avatar-menu flags and so a few tests are now obsolete.
Could you please take a look at those files where tests were removed? Thanks!

mlerman: chrome/browser/profiles
msw: chrome/browser/ui

[anthonyvd, 2015-05-15 20:15:58]

anthonyvd@chromium.org changed reviewers:
+ mlerman@chromium.org, msw@chromium.org

[anthonyvd, 2015-05-15 20:15:58]

Hello guys,

This CL removes the new-avatar-menu flags and so a few tests are now obsolete.
Could you please take a look at those files where tests were removed? Thanks!

mlerman: chrome/browser/profiles
msw: chrome/browser/ui

[commit-bot: I haz the power, 2015-05-15 20:51:36]

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

[commit-bot: I haz the power, 2015-05-15 20:51:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2015-05-18 18:28:30]

https://codereview.chromium.org/1124383004/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/1124383004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1564: "enable-new-avatar-menu",
Can you take a look at the remaining mentions of this flag?
1) Monica's TODO at IDR_PROFILE_AVATAR_0:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/theme/t...
2) kEnableNewAvatarMenu:
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...
3) The UpdateAvatar function comment at:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
4) The source of this histograms.xml entry:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...

https://codereview.chromium.org/1124383004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1565: IDS_FLAGS_ENABLE_NEW_AVATAR_MENU_NAME,
Remove these string resources.

https://codereview.chromium.org/1124383004/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache_unittest.cc (left):

https://codereview.chromium.org/1124383004/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache_unittest.cc:673:
switches::DisableNewAvatarMenuForTesting(
Can you remove this helper, DisableNewAvatarMenuForTesting? I realize you want
to leave most cleanup in another CL, but I wonder if you should remove the flags
and behavior toggling code in this CL?

[Mike Lerman, 2015-05-19 17:15:13]

I had been expecting this to be a minimal CL, that just touches about_flags and
new_profile_management_switches. Can we not modify
new_profile_management_switches::GetProcessState() so that
DisableNewAvatarMenuForTesting() can still have effect, and make this CL as
localized as possible to simply "removing the switch"?

[anthonyvd, 2015-05-19 22:56:41]

Addressed comments.

This patchset also introduces a hacky way to achieve what Mike was asking for.
I'm not very happy about the way it's done but the way I see it there's not
really any other way to disable a command line flag for the user but not for the
test code. Happy to be proven wrong.

As for removing {Enable|Disable}NewAvatarMenuForTesting altogether, it's a
somewhat involved change, hence why I wanted to keep this for the cleanup pass.
They're used in a surprisingly high amount of places, including some test files
that *only* test the old avatar button (and therefore will just be deleted I
imagine).

At this point, I believe that's the best we can do without cleaning up all
references to the flag and since we'll be trying to get this merged to M44, I'm
not super comfortable with dramatically increasing the size of this CL.

If you guys feel strongly about this and think we should be doing more in this
CL, let me know and I'll be happy to look into it :)

Thanks for your time and your feedback, as always!

https://codereview.chromium.org/1124383004/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/1124383004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1564: "enable-new-avatar-menu",
On 2015/05/18 18:28:30, msw wrote:
> Can you take a look at the remaining mentions of this flag?
> 1) Monica's TODO at IDR_PROFILE_AVATAR_0:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/theme/t...

We appear to still be using those resources in other places in the codebase. For
example,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
references them and is called indirectly by profile_avatar_downloader and
profile_info_cache among others. We should investigate this further but I think
it's out of scope for this CL.

> 2) kEnableNewAvatarMenu:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...

I left them in to be able to make this change without changing the rest of the
logic in this pass. The plan was to remove those as part of the clean up as
well.

> 3) The UpdateAvatar function comment at:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

The comment should probably be updated as this method will now only be a
function of the type of Browser (incognito vs regular) but I'd rather leave it
as-is until my cleanup pass, this way I'll know I'll come back there when
grepping for the flag.

> 4) The source of this histograms.xml entry:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...

Since it's an histogram enum, would the correct thing to do be to leave it as is
to not break past histograms (at least until all the data that refer to it
becomes outdated) or is it safe to remove it?

https://codereview.chromium.org/1124383004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1565: IDS_FLAGS_ENABLE_NEW_AVATAR_MENU_NAME,
On 2015/05/18 18:28:30, msw wrote:
> Remove these string resources.

Good point, done.

[msw, 2015-05-19 23:10:32]

Sorry for my poor advice, if our only intent is to remove the about:flags entry,
then feel free to just touch about_flags.cc and generated_resources.grd.

Leave the commandline switches, helpers, and tests as-is. Those should only be
removed along with the code they're testing.

What's the reason for merging this to M-44?

[anthonyvd, 2015-05-19 23:38:06]

On 2015/05/19 23:10:32, msw wrote:
> Sorry for my poor advice, if our only intent is to remove the about:flags
entry,
> then feel free to just touch about_flags.cc and generated_resources.grd.
> 
> Leave the commandline switches, helpers, and tests as-is. Those should only be
> removed along with the code they're testing.
> 
> What's the reason for merging this to M-44?

Oh, that's even easier. I figured I had to remove the command line switches as
well but it's great if they can stay in. 

The idea is that we're removing the fast user switching flag and telling users
about right click profile switching in M44 to address the main concern with the
new avatar menu. We want to use that opportunity to move the users that use the
old system via the flag to the new system. We have to remove the old system at
some point anyways do we figured the introduction of a fix to the #1 reason
people were still using it was a good chance to do that.

[msw, 2015-05-20 00:28:42]

On 2015/05/19 23:38:06, anthonyvd wrote:
> On 2015/05/19 23:10:32, msw wrote:
> > Sorry for my poor advice, if our only intent is to remove the about:flags
> entry,
> > then feel free to just touch about_flags.cc and generated_resources.grd.
> > 
> > Leave the commandline switches, helpers, and tests as-is. Those should only
be
> > removed along with the code they're testing.
> > 
> > What's the reason for merging this to M-44?
> 
> Oh, that's even easier. I figured I had to remove the command line switches as
> well but it's great if they can stay in. 
> 
> The idea is that we're removing the fast user switching flag and telling users
> about right click profile switching in M44 to address the main concern with
the
> new avatar menu. We want to use that opportunity to move the users that use
the
> old system via the flag to the new system. We have to remove the old system at
> some point anyways do we figured the introduction of a fix to the #1 reason
> people were still using it was a good chance to do that.

Okay, I'm not sure how we are communicating the new right-click fast user
switcher functionality (which I think is awesome, and I originally filed Issue
403619), or why removing anything is actually important for M-44. Removing the
about:flags entry will get some attention, but some users will definitely still
be using the command line switches. I'd vote for just removing all the code in
one big CL, coordinating messaging with that release (presumably M-45), and not
worrying about hitting a particular milestone...

[Mike Lerman, 2015-05-20 01:26:22]

On 2015/05/20 00:28:42, msw wrote:
> On 2015/05/19 23:38:06, anthonyvd wrote:
> > On 2015/05/19 23:10:32, msw wrote:
> > > Sorry for my poor advice, if our only intent is to remove the about:flags
> > entry,
> > > then feel free to just touch about_flags.cc and generated_resources.grd.
> > > 
> > > Leave the commandline switches, helpers, and tests as-is. Those should
only
> be
> > > removed along with the code they're testing.
> > > 
> > > What's the reason for merging this to M-44?
> > 
> > Oh, that's even easier. I figured I had to remove the command line switches
as
> > well but it's great if they can stay in. 
> > 
> > The idea is that we're removing the fast user switching flag and telling
users
> > about right click profile switching in M44 to address the main concern with
> the
> > new avatar menu. We want to use that opportunity to move the users that use
> the
> > old system via the flag to the new system. We have to remove the old system
at
> > some point anyways do we figured the introduction of a fix to the #1 reason
> > people were still using it was a good chance to do that.
> 
> Okay, I'm not sure how we are communicating the new right-click fast user
> switcher functionality (which I think is awesome, and I originally filed Issue
> 403619), or why removing anything is actually important for M-44. Removing the
> about:flags entry will get some attention, but some users will definitely
still
> be using the command line switches. I'd vote for just removing all the code in
> one big CL, coordinating messaging with that release (presumably M-45), and
not
> worrying about hitting a particular milestone...

Are we also removing the command line switches in this CL?

I'm not 100% sure how about:flags work, but won't people who have already set a
flag keep their setting? What gets stored across executions of Chrome - the
command line flags to execute, or the set of switches? If it's the command line,
people will keep their settings. If it's the flags, then those will fall off for
most people as the flag gets removed.

[Mike Lerman, 2015-05-20 01:27:16]

Sorry, part 2. Also, can we not remove the various tests? They should be able to
be kept, and removed in another CL, no?

[anthonyvd, 2015-05-20 02:56:10]

On 2015/05/20 01:26:22, Mike Lerman wrote:
> On 2015/05/20 00:28:42, msw wrote:
> > On 2015/05/19 23:38:06, anthonyvd wrote:
> > > On 2015/05/19 23:10:32, msw wrote:
> > > > Sorry for my poor advice, if our only intent is to remove the
about:flags
> > > entry,
> > > > then feel free to just touch about_flags.cc and generated_resources.grd.
> > > > 
> > > > Leave the commandline switches, helpers, and tests as-is. Those should
> only
> > be
> > > > removed along with the code they're testing.
> > > > 
> > > > What's the reason for merging this to M-44?
> > > 
> > > Oh, that's even easier. I figured I had to remove the command line
switches
> as
> > > well but it's great if they can stay in. 
> > > 
> > > The idea is that we're removing the fast user switching flag and telling
> users
> > > about right click profile switching in M44 to address the main concern
with
> > the
> > > new avatar menu. We want to use that opportunity to move the users that
use
> > the
> > > old system via the flag to the new system. We have to remove the old
system
> at
> > > some point anyways do we figured the introduction of a fix to the #1
reason
> > > people were still using it was a good chance to do that.
> > 
> > Okay, I'm not sure how we are communicating the new right-click fast user
> > switcher functionality (which I think is awesome, and I originally filed
Issue
> > 403619), or why removing anything is actually important for M-44. Removing
the
> > about:flags entry will get some attention, but some users will definitely
> still
> > be using the command line switches. I'd vote for just removing all the code
in
> > one big CL, coordinating messaging with that release (presumably M-45), and
> not
> > worrying about hitting a particular milestone...

We're adding a blue tutorial bubble (similar to the sign in confirmation
tutorial) to the User Menu if there are potential profiles to switch to. That CL
landed today so you should be able to see it in the next Canary.

I'm not exactly sure why it's important that it lands in M44. From what I gather
it's related to how long it's been since we've been meaning to remove the flags.
I don't feel strongly enough about the timeline to make a call so I'm conveying
a decision made by our PM.

> 
> Are we also removing the command line switches in this CL?
> 
> I'm not 100% sure how about:flags work, but won't people who have already set
a
> flag keep their setting? What gets stored across executions of Chrome - the
> command line flags to execute, or the set of switches? If it's the command
line,
> people will keep their settings. If it's the flags, then those will fall off
for
> most people as the flag gets removed.

I'll test this to see what the behavior is.

RE: tests, I'll move them to the clean up CL if we still move forward with that
strategy.

I suggest the following, keeping in mind that there's a strong push to have the
removal of the new-avatar-menu flags (this CL), the removal of the
fast-user-switcher flag (landed) and the tutorial bubble (landed) all merged to
M44:
1- Modify this CL to get the minimum of work done to remove the flag from
about://flags. If that requires removing command-line switches to move existing
users it will include it, otherwise it won't even if that means some users will
still use the command-line switches to get the old behavior. Hopefully a
significant portion will adopt the new paradigm when they see the tutorial. The
others will be moved over when 3- reaches stable.
2- Merge the CL in 1- as well as the other two mentioned above (It was confirmed
that the odds of getting approval for that merge were good).
3- Create another CL that removes the rest of the code and might include the
command-line switches depending on 1-.

I'd like us to make a call about this so I can proceed with the implementation
and we can move forward. It won't be possible to get the big clean up CL in M44
but it would be possible to get just the flag removal and have the clean up in
M45 if we can land this soon.

Again, thanks a ton for the feedback and the time you guys are taking to discuss
this. It's much appreciated.

[anthonyvd, 2015-05-20 02:57:32]

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

[anthonyvd, 2015-05-20 02:57:33]

+joberbeck, our PM who might be interested in the discussion or might be better
able to explain the rationale behind wanting to merge.

[anthonyvd, 2015-05-20 02:57:53]

anthonyvd@chromium.org changed reviewers:
- joberbeck@chromium.org

[anthonyvd, 2015-05-20 14:20:44]

The latest patchset represents the minimal amount of work required to remove the
about:flags entry, as discussed in previous comments.

I tested that using the old menu and receiving this patch indeed moves you to
the new menu (ie we appear to validate that persisted flags are still in
about:flags before toggling them). Thanks msw@ for the tip!

With a patch like this, users would still be able to launch Chrome with the
command line flag to get the old menu but I think that's fine until the clean up
pass.

[Roger Tawa OOO till Jul 10th, 2015-05-20 15:45:16]

lgtm

[Mike Lerman, 2015-05-20 15:47:15]

great- thanks so much Anthony! lgtm.

[joberbeck1, 2015-05-20 16:53:34]

On 2015/05/20 02:57:33, anthonyvd wrote:
> +joberbeck, our PM who might be interested in the discussion or might be
better
> able to explain the rationale behind wanting to merge.

We saw with the removal of the 1993/NTP flags that the longer users are able to
live in a flag-enabled state, the more upset they are when those flags are
removed.  We are trying to remove the flags ASAP to minimize user angst.  In
addition, in conversations w/ Avni, she reiterated that we don't want to be
exposing "optional" functionality via flags, i.e. the fast-user-switcher.

[msw, 2015-05-20 16:57:37]

lgtm

[anthonyvd, 2015-05-20 17:01:32]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2015-05-20 17:01:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1124383004/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-20 17:01:51]

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

[commit-bot: I haz the power, 2015-05-20 18:32:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-20 18:33:28]

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