Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400703002:
Fix policy checks for chrome://policy (Closed)
DescriptionFix policy checks for chrome://policy
chrome://policy is supposed to display errors in policies. There were,
however, two problems:
1 - When run without a pref argument (as it is when constructing this
page) ConfigurationPolicyHandlerList::ApplyPolicySettings was skipping
some checks.
2 - The policy checks for the search policies were exiting on the first
error, and hence were only reporting at most one error.
BUG=637372
Committed: https://crrev.com/236e80aa72907e388dd1dc1e962939ad61b5514d
Cr-Commit-Position: refs/heads/master@{#425056}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Add comment so that this doesnt get broken again #
Total comments: 4
Patch Set 3 : Fix nits in default_search_policy_handler.cc #
Messages
Total messages: 23 (12 generated)
aberent@chromium.org changed reviewers: + pkasting@chromium.org, tnagel@chromium.org
pkasting@chromium.org: Please review changes in default_search_policy_handler.cc tnagel@chromium.org: Please review changes in configuration_policy_handler_list.cc
The CQ bit was checked by aberent@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...
LGTM configuration_policy_handler_list.cc
Some optional comments. https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... File components/policy/core/browser/configuration_policy_handler_list.cc (right): https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:35: PolicyErrorMap scoped_errors; Optional: Add DCHECK(prefs || errors). https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:36: if (!errors) Very optional: That looks like an ugly hack. Probably it would be better to refactor CheckPolicySettings() to accept nullptr? https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:43: for (handler = handlers_.begin(); handler != handlers_.end(); ++handler) { Optional: What would you think about converting to range-based loop and using "auto" to cut down on the noise at the head of the loop? https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:44: if ((*handler)->CheckPolicySettings(policies, errors) && prefs) { I guess I broke this. :\ Do you think there's a need to refactor this a bit and/or add a comment to prevent people like me from breaking it again? I could imagine something like below, but maybe it's too verbose? // Check and collect errors. if (!(*handler)->CheckPolicySettings(policies, errors)) { continue; } if (prefs) { (*handler) ->ApplyPolicySettings.... }
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... File components/policy/core/browser/configuration_policy_handler_list.cc (right): https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:36: if (!errors) Thinking some more about this: Since we'll probably want to merge that CL to 55, including some refactoring is maybe not a great idea.
https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... File components/policy/core/browser/configuration_policy_handler_list.cc (right): https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:35: PolicyErrorMap scoped_errors; On 2016/10/06 17:05:43, Thiemo Nagel (slow) wrote: > Optional: Add DCHECK(prefs || errors). Not done. Minimising changes for an easy merge. https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:36: if (!errors) On 2016/10/10 11:28:43, Thiemo Nagel (slow) wrote: > Thinking some more about this: Since we'll probably want to merge that CL to 55, > including some refactoring is maybe not a great idea. Agreed. We should maybe create a separate CL to refactor this. My feeling is that this should be split into two functions; one to get the errors, and a second one to actually apply the policies. Note that this function only seems to be called in two places, one of which sets the policies, and the other just gets the errors. https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:43: for (handler = handlers_.begin(); handler != handlers_.end(); ++handler) { On 2016/10/06 17:05:43, Thiemo Nagel (slow) wrote: > Optional: What would you think about converting to range-based loop and using > "auto" to cut down on the noise at the head of the loop? Yes, when we refactor. https://codereview.chromium.org/2400703002/diff/1/components/policy/core/brow... components/policy/core/browser/configuration_policy_handler_list.cc:44: if ((*handler)->CheckPolicySettings(policies, errors) && prefs) { On 2016/10/06 17:05:43, Thiemo Nagel (slow) wrote: > I guess I broke this. :\ > > Do you think there's a need to refactor this a bit and/or add a comment to > prevent people like me from breaking it again? I could imagine something like > below, but maybe it's too verbose? > > // Check and collect errors. > if (!(*handler)->CheckPolicySettings(policies, errors)) { > continue; > } > > if (prefs) { > (*handler) > ->ApplyPolicySettings.... > } For now I have just added a comment, with a TODO to refactor. For the refactoring CL we need to decide what the correct behaviour is when there are errors. The current code, and your code, skips all the policies owned by a handler if that handler reports any errors. This seems wrong, since it means that, for example, an error in the DefaultSearchProviderAlternateURLs causes Chrome to ignore the DefaultSearchProviderSearchURL (see http://crbug/637372)
Thank you! configuration_policy_handler_list.cc still LGTM.
The CQ bit was checked by aberent@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 https://codereview.chromium.org/2400703002/diff/20001/components/search_engin... File components/search_engines/default_search_policy_handler.cc (right): https://codereview.chromium.org/2400703002/diff/20001/components/search_engin... components/search_engines/default_search_policy_handler.cc:304: bool all_ok = true; Nit: I had to look at several things to understand why you were making this change, so I suggest documenting this with an added comment, e.g.: It's important to call CheckPolicySettings() on all handlers and not just exit on the first error, so we report all policy errors. https://codereview.chromium.org/2400703002/diff/20001/components/search_engin... components/search_engines/default_search_policy_handler.cc:307: all_ok = false; Nit: Shorter: for (const auto& handler : handlers_) all_ok &= handler->CheckPolicySettings(policies, errors);
https://codereview.chromium.org/2400703002/diff/20001/components/search_engin... File components/search_engines/default_search_policy_handler.cc (right): https://codereview.chromium.org/2400703002/diff/20001/components/search_engin... components/search_engines/default_search_policy_handler.cc:304: bool all_ok = true; On 2016/10/10 19:39:33, Peter Kasting (very slow) wrote: > Nit: I had to look at several things to understand why you were making this > change, so I suggest documenting this with an added comment, e.g.: > > It's important to call CheckPolicySettings() on all handlers and not just exit > on the first error, so we report all policy errors. Done. https://codereview.chromium.org/2400703002/diff/20001/components/search_engin... components/search_engines/default_search_policy_handler.cc:307: all_ok = false; On 2016/10/10 19:39:33, Peter Kasting (very slow) wrote: > Nit: Shorter: > > for (const auto& handler : handlers_) > all_ok &= handler->CheckPolicySettings(policies, errors); Done.
The CQ bit was checked by aberent@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2400703002/#ps40001 (title: "Fix nits in default_search_policy_handler.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix policy checks for chrome://policy chrome://policy is supposed to display errors in policies. There were, however, two problems: 1 - When run without a pref argument (as it is when constructing this page) ConfigurationPolicyHandlerList::ApplyPolicySettings was skipping some checks. 2 - The policy checks for the search policies were exiting on the first error, and hence were only reporting at most one error. BUG=637372 ========== to ========== Fix policy checks for chrome://policy chrome://policy is supposed to display errors in policies. There were, however, two problems: 1 - When run without a pref argument (as it is when constructing this page) ConfigurationPolicyHandlerList::ApplyPolicySettings was skipping some checks. 2 - The policy checks for the search policies were exiting on the first error, and hence were only reporting at most one error. BUG=637372 Committed: https://crrev.com/236e80aa72907e388dd1dc1e962939ad61b5514d Cr-Commit-Position: refs/heads/master@{#425056} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/236e80aa72907e388dd1dc1e962939ad61b5514d Cr-Commit-Position: refs/heads/master@{#425056} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
