Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551373002:
Disable supervised user creation when force sign in is enabled. (Closed)
DescriptionDisable supervised user creation when force sign in is enabled.
BUG=642059
Committed: https://crrev.com/bedce4c8743078985338ee4461a74aecee17e830
Cr-Commit-Position: refs/heads/master@{#437092}
Patch Set 1 #
Total comments: 11
Patch Set 2 : treib's comments #
Total comments: 4
Patch Set 3 : treib's comments #Patch Set 4 : merge from master #
Messages
Total messages: 37 (23 generated)
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zmin@chromium.org changed reviewers: + pam@chromium.org, pastarmovj@chromium.org
pastarmovj@chromium.org: Please review changes in chrome/browser/policy/configuration_policy_handler_list_factory.cc pam@chromium.org: Please review changes in chrome/browser/supervised_user/* chrome/browser/BUILD.gn chrome/test/BUILD.gn Thanks, Owen
Description was changed from ========== Disable supervised user creation when force sign in is enabled. BUG=642059 ========== to ========== Disable supervised user creation when force sign in is enabled. BUG=642059 ==========
zmin@chromium.org changed reviewers: + treib@chromium.org - pam@chromium.org
-pam@ +treib@ Hi Marc, Can you please review changes in chrome/browser/supervised_user/* chrome/browser/BUILD.gn chrome/test/BUILD.gn Thanks, Owen
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Looking good, just a bunch of nits. https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_creation_policy_handler.h (right): https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler.h:13: namespace policy { This probably shouldn't be in the policy namespace if it's in the supervised_user folder. https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc (right): https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:21: policies_.Clear(); I don't think this is necessary, these shouldn't survive across tests anyway? https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:37: handler_.ApplyPolicySettings(policies_, &prefs_); Maybe make a helper method for this, and make policies_ and prefs_ private? https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:38: EXPECT_FALSE(prefs_.GetValue(prefs::kSupervisedUserCreationAllowed, nullptr)); Should this also test that setting the key::kSupervisedUserCreationEnabled policy works? https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:68: EXPECT_FALSE(value); IMO this deserves a comment.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_creation_policy_handler.h (right): https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler.h:13: namespace policy { On 2016/12/06 09:28:36, Marc Treib wrote: > This probably shouldn't be in the policy namespace if it's in the > supervised_user folder. I have checked other policy handlers that are not located in policy/ folder. About half of them are using the policy namespace while the rest of them are either in its own namespace or no namespace at all. So, even there isn't very strong consistency, most of handlers are using policy namespace. In the other hand, I think the folder and the namespace is able to show that this class is associated with "supervised user" and "policy" at the same time. https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc (right): https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:21: policies_.Clear(); On 2016/12/06 09:28:37, Marc Treib wrote: > I don't think this is necessary, these shouldn't survive across tests anyway? Removed. https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:37: handler_.ApplyPolicySettings(policies_, &prefs_); On 2016/12/06 09:28:36, Marc Treib wrote: > Maybe make a helper method for this, and make policies_ and prefs_ private? Done. https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:38: EXPECT_FALSE(prefs_.GetValue(prefs::kSupervisedUserCreationAllowed, nullptr)); On 2016/12/06 09:28:36, Marc Treib wrote: > Should this also test that setting the key::kSupervisedUserCreationEnabled > policy works? Done. https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:68: EXPECT_FALSE(value); On 2016/12/06 09:28:37, Marc Treib wrote: > IMO this deserves a comment. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! LGTM https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_creation_policy_handler.h (right): https://codereview.chromium.org/2551373002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_creation_policy_handler.h:13: namespace policy { On 2016/12/06 17:04:55, zmin wrote: > On 2016/12/06 09:28:36, Marc Treib wrote: > > This probably shouldn't be in the policy namespace if it's in the > > supervised_user folder. > > I have checked other policy handlers that are not located in policy/ folder. > About half of them are using the policy namespace while the rest of them are > either in its own namespace or no namespace at all. So, even there isn't very > strong consistency, most of handlers are using policy namespace. In the other > hand, I think the folder and the namespace is able to show that this class is > associated with "supervised user" and "policy" at the same time. Hm alright, fair enough. I still think this kind of "scattered namespace" is very uncommon outside of policy, but oh well. https://codereview.chromium.org/2551373002/diff/20001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc (right): https://codereview.chromium.org/2551373002/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:26: handler_.ApplyPolicySettings(policies_, &prefs_); optional: Looks like every call to SetUpPolicy is immediately followed by a call to ApplyPolicySettings, so SetUpPolicy could just call ApplyPolicySettings. https://codereview.chromium.org/2551373002/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:85: // disabled even the policy is set to true. nit: s/even/even if/
https://codereview.chromium.org/2551373002/diff/20001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc (right): https://codereview.chromium.org/2551373002/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:26: handler_.ApplyPolicySettings(policies_, &prefs_); On 2016/12/06 17:13:40, Marc Treib wrote: > optional: Looks like every call to SetUpPolicy is immediately followed by a call > to ApplyPolicySettings, so SetUpPolicy could just call ApplyPolicySettings. Done. And I rename the SetUpPolicy to SetUpPolicyAndApply https://codereview.chromium.org/2551373002/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_creation_policy_handler_unittest.cc:85: // disabled even the policy is set to true. On 2016/12/06 17:13:40, Marc Treib wrote: > nit: s/even/even if/ Done.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2551373002/#ps40001 (title: "treib's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, pastarmovj@chromium.org Link to the patchset: https://codereview.chromium.org/2551373002/#ps60001 (title: "merge from master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481144561017200,
"parent_rev": "fbf2bc925874ec0867c058db0bfa5a91c5862921", "commit_rev":
"dc19ad60b82078bb19d4aa9873d9c25d0792d46b"}
Message was sent while issue was closed.
Description was changed from ========== Disable supervised user creation when force sign in is enabled. BUG=642059 ========== to ========== Disable supervised user creation when force sign in is enabled. BUG=642059 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Disable supervised user creation when force sign in is enabled. BUG=642059 ========== to ========== Disable supervised user creation when force sign in is enabled. BUG=642059 Committed: https://crrev.com/bedce4c8743078985338ee4461a74aecee17e830 Cr-Commit-Position: refs/heads/master@{#437092} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bedce4c8743078985338ee4461a74aecee17e830 Cr-Commit-Position: refs/heads/master@{#437092} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
