Settings to control Add Person and Browse as Guest.

Settings to control Add Person and Browse as Guest.

The settings page will now have two options near the profile management,
which will control whether Browse As Guest and Add Person will be
available from the User Manager. These settings will not be available to supervised users, and will appear only on Desktop OSes.

Also, add a new enterprise policy for the Enable Add Person preference (Browse As Guest policy already exists).
BUG=406473
NOTE: I will remove PRESUBMIT.py from this CL before landing it. It needs to be modified to upload to rietveld from my Windows machine, see crbug.com/409029

Committed: https://crrev.com/46a8366faec20a16c85109f480bfa7b348480cac
Cr-Commit-Position: refs/heads/master@{#293763}

[Mike Lerman, 2014-08-29 20:41:57]

mlerman@chromium.org changed reviewers:
+ asvitkine@chromium.org, estade@chromium.org, joaodasilva@chromium.org,
treib@chromium.org

[Mike Lerman, 2014-08-29 20:41:58]

Kind and generous reviewers,

This CL gently requests your attention. Specifically:

asvitkine - histograms.xml
estade- c/b/resources and c/b/ui/webui
joaodasilva - policy_templates and configuration_policy_handler_list_factory
treib - Overall review
Thanks!

[Mike Lerman, 2014-08-29 20:43:09]

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

[Mike Lerman, 2014-08-29 20:43:09]

Also adding sky@ for review, for c/b/browser_process_impl.cc.

Thanks everyone!

[Alexei Svitkine (slow), 2014-08-29 22:17:50]

histograms lgtm

[Evan Stade, 2014-08-30 01:15:37]

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:216: <label
for="browser-guest-enable"
structure should be

<label> // no for=
  <input>
  <span>
</label>

The way you currently have it, clicking the space between checkbox and text
doesn't toggle the setting.

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:309: if
(IsAddPersonEnabled()) {
nit: invert this check and return early

[Marc Treib, 2014-09-01 08:49:53]

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.css:465: display: none;
Should these be hidden also when guestMode=true?

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:214:
pref="profile.browser_guest_enabled" type="checkbox">
Hm, I think a supervised user could change the prefs by using the inspector and
un-hiding the checkboxes. Is there another line of defense apart from hiding the
UI?

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/user_manager/user_manager.js (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/user_manager/user_manager.js:35: * @param {Object}
screen Screen params dict, e.g. {id: screenId, data: data}
nit: The comment is inaccurate/out-of-date (not your fault of course, but would
still be nice to fix)

[Joao da Silva, 2014-09-01 08:56:18]

policy lgtm

https://codereview.chromium.org/518063002/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/518063002/diff/40001/PRESUBMIT.py#newcode1121
PRESUBMIT.py:1121:
'^components[\\\/]policy[\\\/]resources[\\\/]policy_templates.json$',
This has been fixed in https://codereview.chromium.org/518863002

[Mike Lerman, 2014-09-02 16:58:29]

Thanks for the comments.

https://codereview.chromium.org/518063002/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/518063002/diff/40001/PRESUBMIT.py#newcode1121
PRESUBMIT.py:1121:
'^components[\\\/]policy[\\\/]resources[\\\/]policy_templates.json$',
On 2014/09/01 08:56:18, Joao da Silva wrote:
> This has been fixed in https://codereview.chromium.org/518863002

Perfect, just rebased. Thanks.

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.css:465: display: none;
On 2014/09/01 08:49:53, Marc Treib wrote:
> Should these be hidden also when guestMode=true?

Not necessary, the parent container is already hidden when guestMode=true.

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:214:
pref="profile.browser_guest_enabled" type="checkbox">
On 2014/09/01 08:49:53, Marc Treib wrote:
> Hm, I think a supervised user could change the prefs by using the inspector
and
> un-hiding the checkboxes. Is there another line of defense apart from hiding
the
> UI?

Yes, found one! Done.

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:216: <label
for="browser-guest-enable"
On 2014/08/30 01:15:36, Evan Stade wrote:
> structure should be
> 
> <label> // no for=
>   <input>
>   <span>
> </label>
> 
> The way you currently have it, clicking the space between checkbox and text
> doesn't toggle the setting.

Done.

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/user_manager/user_manager.js (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/user_manager/user_manager.js:35: * @param {Object}
screen Screen params dict, e.g. {id: screenId, data: data}
On 2014/09/01 08:49:53, Marc Treib wrote:
> nit: The comment is inaccurate/out-of-date (not your fault of course, but
would
> still be nice to fix)

Done.

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/518063002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:309: if
(IsAddPersonEnabled()) {
On 2014/08/30 01:15:37, Evan Stade wrote:
> nit: invert this check and return early

Done.

[Evan Stade, 2014-09-02 18:18:18]

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:198: <input
id="profile-browser-guest-enable"
where is this id used?

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:201: </span>
nit: can go on the line above

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:206: <input
id="profile-add-person-enable"
where is this id used?

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:209: </span>
nit: can go on the line above

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/core_options_handler.cc:666: bool
CoreOptionsHandler::AllowedByUnsupervisedUserOnly(
can you just name this "IsUserUnsupervised"

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:151: bool
IsAddPersonEnabled() {
wasn't this already added in some previous cl

[sky, 2014-09-02 18:19:12]

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:786:
registry->RegisterBooleanPref(prefs::kBrowserAddPersonEnabled, true);
Seems a bit arbitrary that the pref is registered here and not in a location
closer to where this is used. Is there a better place for the registration to
live?

[Mike Lerman, 2014-09-04 14:37:08]

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:786:
registry->RegisterBooleanPref(prefs::kBrowserAddPersonEnabled, true);
On 2014/09/02 18:19:12, sky wrote:
> Seems a bit arbitrary that the pref is registered here and not in a location
> closer to where this is used. Is there a better place for the registration to
> live?

I can put it in c/b/ui/browser_ui_prefs. There doesn't seem to be anything
closer to the settings page. (Done in this patch)

There could be a case for c/b/profiles/profile_state.cc, although these really
are UI and Settings related Prefs, moreso than profiles.

Otherwise I can create a new RegistryPrefs method within c/b/ui/webui/options,
likely within OptionsUI. I'll defer to you as to when it's warranted to split
off a new home for registering prefs.

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:198: <input
id="profile-browser-guest-enable"
On 2014/09/02 18:18:18, Evan Stade wrote:
> where is this id used?

It's not. Removing.

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:201: </span>
On 2014/09/02 18:18:18, Evan Stade wrote:
> nit: can go on the line above

Done.

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:206: <input
id="profile-add-person-enable"
On 2014/09/02 18:18:18, Evan Stade wrote:
> where is this id used?

Removed.

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:209: </span>
On 2014/09/02 18:18:18, Evan Stade wrote:
> nit: can go on the line above

Done.

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/core_options_handler.cc:666: bool
CoreOptionsHandler::AllowedByUnsupervisedUserOnly(
On 2014/09/02 18:18:18, Evan Stade wrote:
> can you just name this "IsUserUnsupervised"

Done.

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:151: bool
IsAddPersonEnabled() {
On 2014/09/02 18:18:18, Evan Stade wrote:
> wasn't this already added in some previous cl

The IsGuestModeEnabled (function directly above) was added elsewhere. Add Person
being disable-able is introduced newly in this CL (AFAIK).

[sky, 2014-09-04 17:43:43]

LGTM

[Evan Stade, 2014-09-04 17:55:36]

mostly lg

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.css:459: #appearance-section,
I know I keep vacillating on this -- sorry for that. But it strikes me that for
both this rule block and the one you're now adding, the list of IDs indicates
you should probably be using a class. It would make sense to introduce a new
class, e.g. exclude-in-guest-mode, so your rule here is pretty simple:

div[guestmode=true] .exclude-in-guest-mode {
  display: none;
}

This also makes the html more readable --- you don't have to trace through
multiple files to figure out when this thing is shown, because the class name is
more descriptive for this purpose than an ID.

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/core_options_handler.cc:667: const base::Value*
to_value) {
nit: will this fit on one line?

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:308: // The UI
should have prevented the user from adding a user.
nit: "The 'Add user' UI should not be showing."

[Evan Stade, 2014-09-04 18:10:13]

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:786:
registry->RegisterBooleanPref(prefs::kBrowserAddPersonEnabled, true);
On 2014/09/04 14:37:08, Mike Lerman wrote:
> On 2014/09/02 18:19:12, sky wrote:
> > Seems a bit arbitrary that the pref is registered here and not in a location
> > closer to where this is used. Is there a better place for the registration
to
> > live?
> 
> I can put it in c/b/ui/browser_ui_prefs. There doesn't seem to be anything
> closer to the settings page. (Done in this patch)
> 
> There could be a case for c/b/profiles/profile_state.cc, although these really
> are UI and Settings related Prefs, moreso than profiles.
> 
> Otherwise I can create a new RegistryPrefs method within c/b/ui/webui/options,
> likely within OptionsUI. I'll defer to you as to when it's warranted to split
> off a new home for registering prefs.

I think you should put them here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

[Mike Lerman, 2014-09-04 20:58:51]

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/518063002/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:786:
registry->RegisterBooleanPref(prefs::kBrowserAddPersonEnabled, true);
On 2014/09/04 18:10:13, Evan Stade wrote:
> On 2014/09/04 14:37:08, Mike Lerman wrote:
> > On 2014/09/02 18:19:12, sky wrote:
> > > Seems a bit arbitrary that the pref is registered here and not in a
location
> > > closer to where this is used. Is there a better place for the registration
> to
> > > live?
> > 
> > I can put it in c/b/ui/browser_ui_prefs. There doesn't seem to be anything
> > closer to the settings page. (Done in this patch)
> > 
> > There could be a case for c/b/profiles/profile_state.cc, although these
really
> > are UI and Settings related Prefs, moreso than profiles.
> > 
> > Otherwise I can create a new RegistryPrefs method within
c/b/ui/webui/options,
> > likely within OptionsUI. I'll defer to you as to when it's warranted to
split
> > off a new home for registering prefs.
> 
> I think you should put them here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

Chatted with noms - she agreed.

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.css:459: #appearance-section,
On 2014/09/04 17:55:36, Evan Stade wrote:
> I know I keep vacillating on this -- sorry for that. But it strikes me that
for
> both this rule block and the one you're now adding, the list of IDs indicates
> you should probably be using a class. It would make sense to introduce a new
> class, e.g. exclude-in-guest-mode, so your rule here is pretty simple:
> 
> div[guestmode=true] .exclude-in-guest-mode {
>   display: none;
> }
> 
> This also makes the html more readable --- you don't have to trace through
> multiple files to figure out when this thing is shown, because the class name
is
> more descriptive for this purpose than an ID.

I'm not sure I want to do this change for this CL. For the current list, I'd
have a difficulty with the chromeos exclusion, which means I'd either need to
create a .exclude-in-guest-mode and a .exclude-in-guest-mode-except-chromeos or
have separate code for #searchBox, which then means I have a class for just
three objects, when the current OS tagged list conveys the objective nicely.

The other big reason is there's already a guest-visibility="disabled" tag, which
from what I could gather only take effect in ChromeOS. Adding a similarly named
class would be confusing. What would be better (and I can open a bug to do in a
later CL) would be to have a more coherent design and flag/class to control
visibility. Currently, some things start hidden, and others don't; some are
hidden in CSS, some in JS, and ChromeOS has its own solution. I'd be happy (in
M40/41) to take a stab at figuring out the half dozen use cases, coming up with
nice syntax, and implementing that.

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/core_options_handler.cc:667: const base::Value*
to_value) {
On 2014/09/04 17:55:36, Evan Stade wrote:
> nit: will this fit on one line?

Yes :)

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/518063002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:308: // The UI
should have prevented the user from adding a user.
On 2014/09/04 17:55:36, Evan Stade wrote:
> nit: "The 'Add user' UI should not be showing."

ok.

[Evan Stade, 2014-09-04 21:48:09]

lgtm

[Marc Treib, 2014-09-05 07:21:43]

lgtm too (just for completeness' sake, not that it makes any difference :) )

[Mike Lerman, 2014-09-05 10:37:27]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-05 10:39:25]

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

[commit-bot: I haz the power, 2014-09-05 11:33:47]

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

[commit-bot: I haz the power, 2014-09-05 11:33:48]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Mike Lerman, 2014-09-05 16:44:12]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:46:38]

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

[commit-bot: I haz the power, 2014-09-05 17:13:54]

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

[commit-bot: I haz the power, 2014-09-05 17:13:55]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Mike Lerman, 2014-09-08 14:46:28]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-08 14:46:57]

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

[commit-bot: I haz the power, 2014-09-08 15:25:34]

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

[commit-bot: I haz the power, 2014-09-08 15:25:35]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike Lerman, 2014-09-08 16:26:14]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-08 16:30:00]

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

[commit-bot: I haz the power, 2014-09-08 17:40:31]

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

[commit-bot: I haz the power, 2014-09-08 17:40:32]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Mike Lerman, 2014-09-08 18:10:12]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-08 18:27:05]

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

[commit-bot: I haz the power, 2014-09-08 18:53:52]

Committed patchset #6 (id:160001) as bac4ee9d03955cdad29ee4f7667e14cc02067429

[commit-bot: I haz the power, 2014-09-10 03:47:28]

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