Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8773002:
Added 'can_be_recommended' flag to the relevant policies. (Closed)
|
Created:
9 years ago by Joao da Silva Modified:
9 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded 'can_be_recommended' flag to the relevant policies.
This flag determines which policies will be included in the recommended policies
templates.
BUG=49941
TEST=Everything works as before
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113411
Patch Set 1 #
Total comments: 18
Patch Set 2 : Removed non-working policies; added some others; changed dynamic_refresh to boolean #
Total comments: 6
Patch Set 3 : Added TODO tags #Messages
Total messages: 7 (0 generated)
This is the set of policies that IMO make sense as "recommended". This flag will only affect template generation; chrome will still accept any policy as recommended. Please review, thanks!
I put some comments in the CL. As a general comment do we really want to bother marking policies as recommended or not or is it fine if we just allow all and leave it to the judgement of the admin to not put stupid things in the recommended policy. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:190: 'name': 'ApplicationLocaleValue', I can imagine some cases where this can be useful as recommended policy too. If the windows locale is not the proper one for some reason people can still be guided to the right Chrome locale through a recommended policy but be allowed to override it if they want to. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:728: 'name': 'Proxy', Why don't we make proxy settings recommended too? http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1988: 'name': 'ImportHistory', For all the import policies I think those might be recommended as well. The idea is that if the admin set those to false it will skip initial import but the import option in the settings will work so that users can still do it from "preferences" if they feel like they want to.
This is clearly a case of http://goto.google.com/gjdbr, see comments :) http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:177: 'can_be_recommended': True, What's the point here? http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:519: 'can_be_recommended': True, I don't think this makes sense, since the user doesn't have a way to toggle this AFAIK. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:728: 'name': 'Proxy', On 2011/12/01 09:43:19, pastarmovj wrote: > Why don't we make proxy settings recommended too? Doesn't make sense on desktop platforms, since system-level configuration would override recommended. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1319: 'can_be_recommended': True, Not sure whether this would actually do the right thing. Have you tested? We might want to ask Markus. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1780: 'name': 'ChromeOsLockOnIdleSuspend', why does this not have the flag? http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1988: 'name': 'ImportHistory', On 2011/12/01 09:43:19, pastarmovj wrote: > For all the import policies I think those might be recommended as well. The idea > is that if the admin set those to false it will skip initial import but the > import option in the settings will work so that users can still do it from > "preferences" if they feel like they want to. seconded. These should actually work fine, but please test :)
I've tested the policies individually and opened a couple of bugs for those that don't work as expected. The 'can_be_recommended' flag has been moved to the 'features' dictionary, so that it appears in the documentation. The 'dynamic_refresh' flag has been converted to a boolean too (the new grit code handles both boolean & ints here, but a boolean type makes more sense for this flag). Please have another look, thanks! http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:177: 'can_be_recommended': True, On 2011/12/01 10:11:35, Mattias Nissler wrote: > What's the point here? Removed. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:190: 'name': 'ApplicationLocaleValue', On 2011/12/01 09:43:19, pastarmovj wrote: > I can imagine some cases where this can be useful as recommended policy too. If > the windows locale is not the proper one for some reason people can still be > guided to the right Chrome locale through a recommended policy but be allowed to > override it if they want to. I initially skipped this one because chrome_frame reads it directly from the registry, and I thought a recommended setting wouldn't ever apply. However, I just verified that there's a --lang command line flag that the user can use, so it makes sense as recommended after all. Good catch! http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:263: 'can_be_recommended': True, I've removed this one, since there's no knob for the user to toggle this. The description text has been updated too. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:519: 'can_be_recommended': True, On 2011/12/01 10:11:35, Mattias Nissler wrote: > I don't think this makes sense, since the user doesn't have a way to toggle this > AFAIK. Right, removed. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1140: 'can_be_recommended': True, This has the same effect as the mandatory policy, when used as recommended (the user can't change the URLs). Removed for now and added a bug to track this. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1198: 'can_be_recommended': True, These turned out not to work as recommended policies. Removed for now and added a bug to track this. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1319: 'can_be_recommended': True, On 2011/12/01 10:11:35, Mattias Nissler wrote: > Not sure whether this would actually do the right thing. Have you tested? We > might want to ask Markus. It DCHECKs on debug and is ignored on release. Removed for now and opened a bug to track this. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1780: 'name': 'ChromeOsLockOnIdleSuspend', On 2011/12/01 10:11:35, Mattias Nissler wrote: > why does this not have the flag? Good catch, added. http://codereview.chromium.org/8773002/diff/1/chrome/app/policy/policy_templa... chrome/app/policy/policy_templates.json:1988: 'name': 'ImportHistory', On 2011/12/01 10:11:35, Mattias Nissler wrote: > On 2011/12/01 09:43:19, pastarmovj wrote: > > For all the import policies I think those might be recommended as well. The > idea > > is that if the admin set those to false it will skip initial import but the > > import option in the settings will work so that users can still do it from > > "preferences" if they feel like they want to. > > seconded. These should actually work fine, but please test :) Tested and works as expected (sets the default for the checkboxes on the import dialog).
LGTM with nits. http://codereview.chromium.org/8773002/diff/4001/chrome/app/policy/policy_tem... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/8773002/diff/4001/chrome/app/policy/policy_tem... chrome/app/policy/policy_templates.json:1161: # Flag this policy with 'can_be_recommended' after fixing nit: Put a TODO tag. http://codereview.chromium.org/8773002/diff/4001/chrome/app/policy/policy_tem... chrome/app/policy/policy_templates.json:1196: # Flag these policies with 'can_be_recommended' after fixing ditto http://codereview.chromium.org/8773002/diff/4001/chrome/app/policy/policy_tem... chrome/app/policy/policy_templates.json:1320: # Flag these policies with 'can_be_recommended' after fixing ditto
@pastarmovj: will land if it LGTY http://codereview.chromium.org/8773002/diff/4001/chrome/app/policy/policy_tem... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/8773002/diff/4001/chrome/app/policy/policy_tem... chrome/app/policy/policy_templates.json:1161: # Flag this policy with 'can_be_recommended' after fixing On 2011/12/07 15:21:04, Mattias Nissler wrote: > nit: Put a TODO tag. Done. http://codereview.chromium.org/8773002/diff/4001/chrome/app/policy/policy_tem... chrome/app/policy/policy_templates.json:1196: # Flag these policies with 'can_be_recommended' after fixing On 2011/12/07 15:21:04, Mattias Nissler wrote: > ditto Done. http://codereview.chromium.org/8773002/diff/4001/chrome/app/policy/policy_tem... chrome/app/policy/policy_templates.json:1320: # Flag these policies with 'can_be_recommended' after fixing On 2011/12/07 15:21:04, Mattias Nissler wrote: > ditto Done.
lgtm |
