Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
519643002:
Default values for enterprise users are supported. (Closed)
|
Created:
6 years, 3 months ago by merkulova Modified:
6 years, 3 months ago Reviewers:
Joao da Silva CC:
chromium-reviews, joaodasilva+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org, xiyuan Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDefault values for enterprise users are supported.
BUG=402800
Committed: https://crrev.com/5078a919832261d425de571d171d74e091882e5c
Cr-Commit-Position: refs/heads/master@{#293480}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Test added. #
Total comments: 12
Patch Set 3 : Nits. #Patch Set 4 : EasyUnlock merge. #Patch Set 5 : Test fixed. #Patch Set 6 : Bug fixed. #
Messages
Total messages: 26 (7 generated)
merkulova@chromium.org changed reviewers: + joaodasilva@chromium.org
This is right on, just a couple of small changes before it's ready. There is a unittest for the generated code at components/policy/core/common/generate_policy_source_unittest.cc. Can you add a test there that verified that we set the default for ChromeOsMultiProfileUserBehavior on an empty PolicyMap, and that we don't override the PolicyMap if it already contains that policy? https://codereview.chromium.org/519643002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc (right): https://codereview.chromium.org/519643002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:286: SetEnterpriseUsersDefaults(policy_map); Add a comment here explaining what we're doing. Suggestion: "If the store has a verified policy blob received from the server then apply the defaults for policies that haven't been configured by the administrator, given that this is an enterprise user." https://codereview.chromium.org/519643002/diff/1/components/policy/resources/... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/519643002/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:3146: 'default_for_enterprise_users': 'primary_only', Document what this field does in the big comment at the top of this file https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:241: '\n') Add a forward declaration for SetEnterpriseUsersDefaults here, wrapped in #ifdef OS_CHROMEOS https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:579: '#include "components/policy/core/common/policy_map.h"\n' Includes in alphabetical order, so this comes before schema_internal.h https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:641: if policy.has_enterprise_default: Use 2 spaces for indentation instead of 4 (below too) https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:643: creation_expression = 'new bool(%s)' % policy.enterprise_default new base::FundamentalValue() Also, the |policy.enterprise_default| value is a python boolean, so it's True or False. It needs to be converted to the C++ keywords in lowercase. Suggestion: 'new base::FundamentalValue(%s)' % (policy.enterprise_default and 'true' else 'false') https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:645: creation_expression = 'new int(%s)' % policy.enterprise_default new base::FundamentalValue https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:654: ' policy_map->Set("%s",\n' These 2 lines are duplicating the string for the policy name. We can use the constant in the policy::key namespace instead (these are generated below, in line 694) You can just replace Get("%s") and Set("%s" with something like Get(key::k%s) and Set(key::k%s https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:661: f.write('}\n\n' Single newline here, so that the #endif comes right after the end of the function (the opening #ifdef is also right next to the start)
The bug number is 402800, can you fix that? :-)
Updated the test and bug number. https://codereview.chromium.org/519643002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc (right): https://codereview.chromium.org/519643002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:286: SetEnterpriseUsersDefaults(policy_map); On 2014/08/29 09:11:42, Joao da Silva wrote: > Add a comment here explaining what we're doing. Suggestion: > > "If the store has a verified policy blob received from the server then apply the > defaults for policies that haven't been configured by the administrator, given > that this is an enterprise user." Done. https://codereview.chromium.org/519643002/diff/1/components/policy/resources/... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/519643002/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:3146: 'default_for_enterprise_users': 'primary_only', On 2014/08/29 09:11:42, Joao da Silva wrote: > Document what this field does in the big comment at the top of this file Done. https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:241: '\n') On 2014/08/29 09:11:42, Joao da Silva wrote: > Add a forward declaration for SetEnterpriseUsersDefaults here, wrapped in #ifdef > OS_CHROMEOS Done. https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:579: '#include "components/policy/core/common/policy_map.h"\n' On 2014/08/29 09:11:42, Joao da Silva wrote: > Includes in alphabetical order, so this comes before schema_internal.h Done. https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:641: if policy.has_enterprise_default: On 2014/08/29 09:11:42, Joao da Silva wrote: > Use 2 spaces for indentation instead of 4 (below too) Done. https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:643: creation_expression = 'new bool(%s)' % policy.enterprise_default On 2014/08/29 09:11:43, Joao da Silva wrote: > new base::FundamentalValue() > > Also, the |policy.enterprise_default| value is a python boolean, so it's True or > False. It needs to be converted to the C++ keywords in lowercase. Suggestion: > > 'new base::FundamentalValue(%s)' % (policy.enterprise_default and 'true' else > 'false') Done. https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:645: creation_expression = 'new int(%s)' % policy.enterprise_default On 2014/08/29 09:11:42, Joao da Silva wrote: > new base::FundamentalValue Done. https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:654: ' policy_map->Set("%s",\n' On 2014/08/29 09:11:42, Joao da Silva wrote: > These 2 lines are duplicating the string for the policy name. We can use the > constant in the policy::key namespace instead (these are generated below, in > line 694) > > You can just replace Get("%s") and Set("%s" with something like Get(key::k%s) > and Set(key::k%s Done. https://codereview.chromium.org/519643002/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:661: f.write('}\n\n' On 2014/08/29 09:11:42, Joao da Silva wrote: > Single newline here, so that the #endif comes right after the end of the > function (the opening #ifdef is also right next to the start) Done.
Please hold off this CL, say for about a week. This allows me to get https://codereview.chromium.org/478773002/ in and merge into M38. Thanks.
To Xiyuan: sure, added myself as cc to your CL.
lgtm after fixing the remaining nits. Let's wait for Xiyuan's CL before landing this then. It should only require a new 'default_for_enterprise_users' after the rebase. https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... File components/policy/core/common/generate_policy_source_unittest.cc (right): https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:177: #if defined(OS_CHROMEOS) Move this one line above, so that this test doesn't exist on non-ChromeOS platforms. https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:183: const base::Value *multiprof_behavior = base::Value* multiprof_behavior (move the '*') https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:195: const base::Value *multiprof_behavior = Same here https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:199: #endif Move this one line below, after the } https://codereview.chromium.org/519643002/diff/20001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/519643002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:235: f.write('#ifdef OS_CHROMEOS\n' #if defined(OS_CHROMEOS) https://codereview.chromium.org/519643002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:642: f.write('#ifdef OS_CHROMEOS\n' #if defined(OS_CHROMEOS)
Waiting for xyiuan@ CL to land. https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... File components/policy/core/common/generate_policy_source_unittest.cc (right): https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:177: #if defined(OS_CHROMEOS) On 2014/09/01 09:09:11, Joao da Silva wrote: > Move this one line above, so that this test doesn't exist on non-ChromeOS > platforms. Done. https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:183: const base::Value *multiprof_behavior = On 2014/09/01 09:09:11, Joao da Silva wrote: > base::Value* multiprof_behavior (move the '*') Done. https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:195: const base::Value *multiprof_behavior = On 2014/09/01 09:09:11, Joao da Silva wrote: > Same here Done. https://codereview.chromium.org/519643002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:199: #endif On 2014/09/01 09:09:11, Joao da Silva wrote: > Move this one line below, after the } Done. https://codereview.chromium.org/519643002/diff/20001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/519643002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:235: f.write('#ifdef OS_CHROMEOS\n' On 2014/09/01 09:09:11, Joao da Silva wrote: > #if defined(OS_CHROMEOS) Done. https://codereview.chromium.org/519643002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:642: f.write('#ifdef OS_CHROMEOS\n' On 2014/09/01 09:09:11, Joao da Silva wrote: > #if defined(OS_CHROMEOS) Done.
Xiyuan's CL has landed, so this is ready to go after the rebase.
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/519643002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/519643002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/519643002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/519643002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 3fc260d55ef50884a56c2b8a6507f176cac04f93
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5078a919832261d425de571d171d74e091882e5c Cr-Commit-Position: refs/heads/master@{#293480} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
