Add differentiation between owner only and common flags on ChromeOS.

Add differentiation between owner only and common flags on ChromeOS.

Some flags require to be active on the login screen and those can only be
activated from the owner account.

BUG=chromium-os:39248
TEST=Open about:flags from non-owner account and check that "ChromeOS (owner only)" flags are grayed out.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194890

[pastarmovj, 2013-04-08 15:35:46]

Hey Nico,
can you please review this rather minimal change to the flags UI to
differentiate between owner only and generic chromeos flags like we discussed
when you were in MUC.
I have since then synced with Glen and got UX blessing for that change.
Thanks!
Julian

[Nico, 2013-04-08 19:24:10]

* Mixing the owners flags with the OS field seems fairly heavy-handed. Is there
no better way to do this?

* In general, prefer enum parameters over bool parameters (even if the new enum
has only two values), it makes for much more readable call sites.

https://codereview.chromium.org/13467023/diff/1/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/13467023/diff/1/chrome/browser/about_flags.h#...
chrome/browser/about_flags.h:129: // by the OS enum above.
This comment is now no longer true.

[pastarmovj, 2013-04-09 09:17:49]

Thanks for the quick response!

I can see your point but also I believe this is the cleanest way to augment the
structure without increasing the amount of effort needed to add new flags to it.

Please have a look at this second alternative implementation. I personally
prefer the first one but I would be glad to use the two variants as a base to
discuss further implementation changes that you believe need to be done.

https://codereview.chromium.org/13467023/diff/1/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/13467023/diff/1/chrome/browser/about_flags.h#...
chrome/browser/about_flags.h:129: // by the OS enum above.
On 2013/04/08 19:24:10, Nico wrote:
> This comment is now no longer true.

It still is. I never added a new call to this directly. The new parameter added
is forwarded through the call site - GetFlagsExperimentsData.

I could make the function decide for itself but I decided that it will be
cleaner to forward this value - both for easier mocking and for the fact that we
can't actually decide the ownership synchronously (see flags_ui.cc for details
about that).

[Nico, 2013-04-09 15:38:26]

https://codereview.chromium.org/13467023/diff/1/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/13467023/diff/1/chrome/browser/about_flags.h#...
chrome/browser/about_flags.h:129: // by the OS enum above.
On 2013/04/09 09:17:49, pastarmovj wrote:
> On 2013/04/08 19:24:10, Nico wrote:
> > This comment is now no longer true.
> 
> It still is. I never added a new call to this directly. The new parameter
added
> is forwarded through the call site - GetFlagsExperimentsData.

Right, but if the parameter is true this returns "kOsCrOS | kOsCrOSOwnerOnly",
which is not "one of the values defined by the OS enum above" but a bit-or of
two of those. Which means all call sites now need to do bit tests instead of
equality testing.
> 
> I could make the function decide for itself but I decided that it will be
> cleaner to forward this value - both for easier mocking and for the fact that
we
> can't actually decide the ownership synchronously (see flags_ui.cc for details
> about that).

[pastarmovj, 2013-04-11 11:38:33]

So what do you think about the new version which doesn't mix ownership checks
with platform checks?

[Nico, 2013-04-12 04:29:45]

I like patch set 2 more. I'd still use an enum instead of a bool for the
is_owner flag.

[pastarmovj, 2013-04-15 15:30:16]

On 2013/04/12 04:29:45, Nico wrote:
> I like patch set 2 more. I'd still use an enum instead of a bool for the
> is_owner flag.

I changed it as you suggested. PTAL.

[Nico, 2013-04-16 21:40:24]

lgtm with non-nit comments addressed (nits are optional).

Sorry for the delayed review comments.

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1595: !!(experiment.supported_platforms &
kOsCrOSOwnerOnly))
nit: `!= 0` reads nicer than !! imo

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/about_flag...
chrome/browser/about_flags.h:115: 
nit: I'd move the enum right above this function, as that's where it's used.

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:112: bool owner_;
Use your new enum for this too, instead of having a bool.

[pastarmovj, 2013-04-17 16:03:21]

Thanks for the review! 

I will land once the CQ is running again.

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1595: !!(experiment.supported_platforms &
kOsCrOSOwnerOnly))
On 2013/04/16 21:40:24, Nico wrote:
> nit: `!= 0` reads nicer than !! imo

Changed here and on 1592.

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/about_flag...
chrome/browser/about_flags.h:115: 
On 2013/04/16 21:40:24, Nico wrote:
> nit: I'd move the enum right above this function, as that's where it's used.

Done.

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/13467023/diff/15001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:112: bool owner_;
On 2013/04/16 21:40:24, Nico wrote:
> Use your new enum for this too, instead of having a bool.

Done.

[commit-bot: I haz the power, 2013-04-18 08:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/13467023/26002

[commit-bot: I haz the power, 2013-04-18 11:53:59]

Change committed as 194890