Disable UI flags for non-desktop OSes

Disable UI flags for non-desktop OSes

This is mostly to prevent the high res avatars from being downloaded when not necessary, and will also prevent any other non-UI side effects which are undesired on non-desktop OSes.

BUG=404066, 411428
Committed: https://chromium.googlesource.com/chromium/src/+/8003a95bb88e1d25ad59251b5d7e8609d76e5669

Committed: https://crrev.com/9496620249d7bf393a541b78add51f2999f2c6ea
Cr-Commit-Position: refs/heads/master@{#293723}

[Mike Lerman, 2014-08-15 13:26:34]

Here's the CL to disable flags. PTAL, thanks!

[noms (inactive), 2014-08-15 13:29:30]

lgtm w maybe a nit

https://codereview.chromium.org/480453002/diff/1/components/signin/core/commo...
File components/signin/core/common/profile_management_switches.cc (right):

https://codereview.chromium.org/480453002/diff/1/components/signin/core/commo...
components/signin/core/common/profile_management_switches.cc:147: // Non-desktop
OSes never have NewAvatarMenu or related functionality.
nit: I would rephrase this as "NewAvatarMenu functionality is only available on
desktop"
(same below for NewProfileManagement)

[Mike Lerman, 2014-08-15 13:35:57]

https://codereview.chromium.org/480453002/diff/1/components/signin/core/commo...
File components/signin/core/common/profile_management_switches.cc (right):

https://codereview.chromium.org/480453002/diff/1/components/signin/core/commo...
components/signin/core/common/profile_management_switches.cc:147: // Non-desktop
OSes never have NewAvatarMenu or related functionality.
On 2014/08/15 13:29:30, Monica Dinculescu wrote:
> nit: I would rephrase this as "NewAvatarMenu functionality is only available
on
> desktop"
> (same below for NewProfileManagement) 

Done like Dunn's.

[Roger Tawa OOO till Jul 10th, 2014-08-15 14:05:03]

lgtm

[noms (inactive), 2014-08-15 14:16:08]

The CQ bit was checked by noms@chromium.org

[noms (inactive), 2014-08-15 14:16:11]

The CQ bit was unchecked by noms@chromium.org

[Mike Lerman, 2014-08-15 14:22:45]

Hi,

Please verify that the flag changes I'm making for android and chromeos are
correct.

Thanks,
Mike

[Nikita (slow), 2014-08-15 14:56:13]

+Pavel

https://codereview.chromium.org/480453002/diff/40001/components/signin/core/c...
File components/signin/core/common/profile_management_switches.cc (right):

https://codereview.chromium.org/480453002/diff/40001/components/signin/core/c...
components/signin/core/common/profile_management_switches.cc:156: //
NewProfileManagement is only available on desktop.
That's not correct.

On Chrome OS "new profile management" also works.
Uber tray has "Add account" item when this flag (and account consistency) is
enabled.

Maybe Chrome OS should only rely on "account consistency" flag.

Pavel should know better.

[dzhioev (left Google), 2014-08-15 15:31:33]

https://codereview.chromium.org/480453002/diff/40001/components/signin/core/c...
File components/signin/core/common/profile_management_switches.cc (right):

https://codereview.chromium.org/480453002/diff/40001/components/signin/core/c...
components/signin/core/common/profile_management_switches.cc:156: //
NewProfileManagement is only available on desktop.
On 2014/08/15 14:56:13, Nikita Kostylev wrote:
> That's not correct.
> 
> On Chrome OS "new profile management" also works.
> Uber tray has "Add account" item when this flag (and account consistency) is
> enabled.
> 
> Maybe Chrome OS should only rely on "account consistency" flag.
> 
> Pavel should know better.
I don't know for sure. Chrome OS checks only IsEnableAccountConsistency to
decide whether Mirror UI should be shown or not. It used IsNewProfileManagement
before, but this changed after https://codereview.chromium.org/372033002 where
IsNewProfileManagement was renamed to IsEnableAccountConsistency. Mike, can you
confirm that such behavior is correct?

[Mike Lerman, 2014-08-15 16:44:32]

On 2014/08/15 15:31:33, dzhioev wrote:
>
https://codereview.chromium.org/480453002/diff/40001/components/signin/core/c...
> File components/signin/core/common/profile_management_switches.cc (right):
> 
>
https://codereview.chromium.org/480453002/diff/40001/components/signin/core/c...
> components/signin/core/common/profile_management_switches.cc:156: //
> NewProfileManagement is only available on desktop.
> On 2014/08/15 14:56:13, Nikita Kostylev wrote:
> > That's not correct.
> > 
> > On Chrome OS "new profile management" also works.
> > Uber tray has "Add account" item when this flag (and account consistency) is
> > enabled.
> > 
> > Maybe Chrome OS should only rely on "account consistency" flag.
> > 
> > Pavel should know better.
> I don't know for sure. Chrome OS checks only IsEnableAccountConsistency to
> decide whether Mirror UI should be shown or not. It used
IsNewProfileManagement
> before, but this changed after https://codereview.chromium.org/372033002 where
> IsNewProfileManagement was renamed to IsEnableAccountConsistency. Mike, can
you
> confirm that such behavior is correct?

The rename in the CL mentioned was to move all functionality related to
secondary accounts and Mirror functionality under the IsEnableAccountConsistency
flag. For Desktop, the flags have the following high level semantics:
new-avatar-menu includes the new desktop UI and Guest Mode on Desktop;
new-profile-management enables Lock on desktop; enable-account-consistency
enables secondary accounts for profiles, associated UI and Mirror (account
reconciliation). These three bundles of functionality, for desktop, are being
rolled out in different releases (M38 - NewAvatarMenu, M39 -
NewProfileManagement, ??? - AccountConsistency).
I assume that ChromeOS, like Android and iOS, will be releasing all
Mirror-related functionality in one go, under the flag of
EnableAccountConsistency. My change above, in the present CL, reflects that
assumption. If there's a different plan, let me know!

[dzhioev (left Google), 2014-08-18 15:25:39]

On 2014/08/15 16:44:32, Mike Lerman wrote:
> On 2014/08/15 15:31:33, dzhioev wrote:
> >
>
https://codereview.chromium.org/480453002/diff/40001/components/signin/core/c...
> > File components/signin/core/common/profile_management_switches.cc (right):
> > 
> >
>
https://codereview.chromium.org/480453002/diff/40001/components/signin/core/c...
> > components/signin/core/common/profile_management_switches.cc:156: //
> > NewProfileManagement is only available on desktop.
> > On 2014/08/15 14:56:13, Nikita Kostylev wrote:
> > > That's not correct.
> > > 
> > > On Chrome OS "new profile management" also works.
> > > Uber tray has "Add account" item when this flag (and account consistency)
is
> > > enabled.
> > > 
> > > Maybe Chrome OS should only rely on "account consistency" flag.
> > > 
> > > Pavel should know better.
> > I don't know for sure. Chrome OS checks only IsEnableAccountConsistency to
> > decide whether Mirror UI should be shown or not. It used
> IsNewProfileManagement
> > before, but this changed after https://codereview.chromium.org/372033002
where
> > IsNewProfileManagement was renamed to IsEnableAccountConsistency. Mike, can
> you
> > confirm that such behavior is correct?
> 
> The rename in the CL mentioned was to move all functionality related to
> secondary accounts and Mirror functionality under the
IsEnableAccountConsistency
> flag. For Desktop, the flags have the following high level semantics:
> new-avatar-menu includes the new desktop UI and Guest Mode on Desktop;
> new-profile-management enables Lock on desktop; enable-account-consistency
> enables secondary accounts for profiles, associated UI and Mirror (account
> reconciliation). These three bundles of functionality, for desktop, are being
> rolled out in different releases (M38 - NewAvatarMenu, M39 -
> NewProfileManagement, ??? - AccountConsistency).
> I assume that ChromeOS, like Android and iOS, will be releasing all
> Mirror-related functionality in one go, under the flag of
> EnableAccountConsistency. My change above, in the present CL, reflects that
> assumption. If there's a different plan, let me know!

Thanks, sounds right. LGTM.

[newt (away), 2014-08-18 17:29:37]

aruslan: could you take a look at this?

[aruslan, 2014-08-18 21:24:08]

Mike, if you land this CL you will turn off Mirror on Android, is it the intent?

Drive-by review: this multilayered flags set up is quite bizarre and
error-prone, and it is not consistently reflected elsewhere.

For example, Android is set to be STATE_ACCOUNT_CONSISTENCY by default, yet
Android-specific code checks for switches::IsNewProfileManagement(), and to make
things more fun, the rest of cross-platform code does #if ANDROID to "return
false" on random code paths etc.

I'm sure we don't have as many configuration as profile_management_switches.cc
attempts to show.
It would be terrific if we had some extremely simple rules to turn things on and
off on a feature-basis instead of platform-basis.

[aruslan, 2014-08-18 21:25:21]

https://codereview.chromium.org/480453002/diff/40001/chrome/browser/android/s...
File chrome/browser/android/signin/signin_manager_android.cc (right):

https://codereview.chromium.org/480453002/diff/40001/chrome/browser/android/s...
chrome/browser/android/signin/signin_manager_android.cc:289: return
switches::IsEnableAccountConsistency();
This is a deadly way of dealing with flags.
It is really hard to tell if there is a difference between
"IsNewProfileManagementEnabled" and "IsNewProfileManagement".

[Mike Lerman, 2014-08-19 14:58:57]

On 2014/08/18 21:25:21, aruslan wrote:
>
https://codereview.chromium.org/480453002/diff/40001/chrome/browser/android/s...
> File chrome/browser/android/signin/signin_manager_android.cc (right):
> 
>
https://codereview.chromium.org/480453002/diff/40001/chrome/browser/android/s...
> chrome/browser/android/signin/signin_manager_android.cc:289: return
> switches::IsEnableAccountConsistency();
> This is a deadly way of dealing with flags.
> It is really hard to tell if there is a difference between
> "IsNewProfileManagementEnabled" and "IsNewProfileManagement".

The goal of this CL is not to turn off Mirror on Android; the goal is that all
of Mirror on Android should be keyed off the AccountConsistency flag, and
NewProfileManagement should have no impact on Android.

[Mike Lerman, 2014-08-19 14:59:07]

https://codereview.chromium.org/480453002/diff/40001/chrome/browser/android/s...
File chrome/browser/android/signin/signin_manager_android.cc (right):

https://codereview.chromium.org/480453002/diff/40001/chrome/browser/android/s...
chrome/browser/android/signin/signin_manager_android.cc:289: return
switches::IsEnableAccountConsistency();
On 2014/08/18 21:25:21, aruslan wrote:
> This is a deadly way of dealing with flags.
> It is really hard to tell if there is a difference between
> "IsNewProfileManagementEnabled" and "IsNewProfileManagement".

I don't see a IsNewProfileManagementEnabled flag, except for this method here in
signin_manager_android. I assume this is the method that controls/passes the
relevant flag through to the Java-side of Clank? It should (I imagine) be called
IsAccountConsistencyEnabled, since NewProfileManagement has no meaning on
Android.

[aruslan, 2014-08-19 18:08:25]

On 2014/08/19 14:59:07, Mike Lerman wrote:
> It should (I imagine) be called
> IsAccountConsistencyEnabled, since NewProfileManagement has no meaning on
> Android.

Whoa, cool, I haven't realized we _changed_ the semantics of the flag instead of
just deleting it.
NewProfileManagement was the umbrella flag for everything Mirror-related.

Looks like https://codereview.chromium.org/372033002 changed the semantics of
the new-profile-management flag in public Chromium, but left Chrome for Android
internal repo "as is".
I think you should finish what you've started with http://crbug.com/391497 and
update the Chrome for Android repo to use account-consistency instead of
new-profile-management.
(That includes tests etc.)

This would make it unnecessary to have two similarly named functions
  switches::IsNewProfileManagement()
and
  SigninManager::IsNewProfileManagementEnabled()
to return different values.

I think you have all necessary visas to land this change.

[Mike Lerman, 2014-08-20 13:05:14]

Thanks, Ruslan!

Xiyuan, can I get an owners review for chromeos, given Pavel's comments?
Nikita's OOO.

[Nikita (slow), 2014-08-20 13:11:05]

lgtm

[xiyuan, 2014-08-20 15:36:47]

lgtm

[Mike Lerman, 2014-08-29 19:30:27]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-08-29 19:30:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/480453002/60001

[commit-bot: I haz the power, 2014-08-29 23:01:03]

Committed patchset #4 (id:60001) as 8003a95bb88e1d25ad59251b5d7e8609d76e5669

[Mike Lerman, 2014-08-30 01:10:31]

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

The reason for reverting is: This will cause android to lose account
consistency. I should not have removed the file! .

[Mike Lerman, 2014-09-05 20:43:07]

Patchset #4 (id:60001) has been deleted

[Mike Lerman, 2014-09-05 20:46:36]

Patchset #4 (id:80001) has been deleted

[Mike Lerman, 2014-09-08 13:48:28]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-08 13:48:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/480453002/100001

[commit-bot: I haz the power, 2014-09-08 14:51:34]

Committed patchset #4 (id:100001) as aff15e6f6e87c132943b43892779d1487ff56cb9

[commit-bot: I haz the power, 2014-09-10 03:11:22]

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

[commit-bot: I haz the power, 2014-09-10 03:46:14]

Patchset 4 (id:??) landed as
https://crrev.com/9496620249d7bf393a541b78add51f2999f2c6ea
Cr-Commit-Position: refs/heads/master@{#293723}