Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(48)

Issue 7972013: ConfigurationPolicyPrefStore refactoring to surface error messages. (Closed)

Created:
9 years, 3 months ago by simo
Modified:
9 years, 2 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

ConfigurationPolicyPrefStore refactoring to surface error messages. Created ConfigurationPolicyHandlerInterface and instances of it to check and apply policy settings. These are stored in the BrowserPolicyConnector. BUG= TEST=

Patch Set 1 #

Total comments: 78

Patch Set 2 : . #

Total comments: 104

Patch Set 3 : . #

Total comments: 25

Patch Set 4 : . #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+1608 lines, -879 lines) Patch
M chrome/app/generated_resources.grd View 1 2 1 chunk +43 lines, -0 lines 0 comments Download
M chrome/browser/policy/browser_policy_connector.h View 1 2 3 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/policy/browser_policy_connector.cc View 1 2 3 4 chunks +11 lines, -1 line 0 comments Download
M chrome/browser/policy/config_dir_policy_provider_unittest.cc View 1 2 1 chunk +0 lines, -12 lines 0 comments Download
A chrome/browser/policy/configuration_policy_handler.h View 1 2 3 1 chunk +271 lines, -0 lines 1 comment Download
A chrome/browser/policy/configuration_policy_handler.cc View 1 2 3 1 chunk +806 lines, -0 lines 1 comment Download
A chrome/browser/policy/configuration_policy_handler_interface.h View 1 2 3 1 chunk +42 lines, -0 lines 0 comments Download
A chrome/browser/policy/configuration_policy_handler_list.h View 1 2 3 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/browser/policy/configuration_policy_handler_list.cc View 1 2 3 1 chunk +204 lines, -0 lines 0 comments Download
M chrome/browser/policy/configuration_policy_pref_store.cc View 1 2 3 5 chunks +34 lines, -865 lines 1 comment Download
M chrome/browser/policy/configuration_policy_provider.cc View 1 1 chunk +1 line, -1 line 0 comments Download
A chrome/browser/policy/policy_error_map.h View 1 2 1 chunk +62 lines, -0 lines 0 comments Download
A chrome/browser/policy/policy_error_map.cc View 1 2 1 chunk +71 lines, -0 lines 0 comments Download
M chrome/browser/prefs/pref_value_map.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/prefs/pref_value_map.cc View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 chunks +7 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
simo
9 years, 3 months ago (2011-09-20 09:46:18 UTC) #1
Mattias Nissler (ping if slow)
first round of comments http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configuration_policy_handler.cc File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configuration_policy_handler.cc#newcode27 chrome/browser/policy/configuration_policy_handler.cc:27: const char* pref_path) : policy_type_(policy), ...
9 years, 3 months ago (2011-09-20 13:12:25 UTC) #2
simo
http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configuration_policy_handler.cc File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configuration_policy_handler.cc#newcode27 chrome/browser/policy/configuration_policy_handler.cc:27: const char* pref_path) : policy_type_(policy), On 2011/09/20 13:12:25, Mattias ...
9 years, 3 months ago (2011-09-22 11:43:26 UTC) #3
Mattias Nissler (ping if slow)
Moar comments http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resources.grd#newcode4302 chrome/app/generated_resources.grd:4302: <message name="IDS_POLICY_DEFAULT_SEARCH_DISABLED" desc="The text displayed in the ...
9 years, 2 months ago (2011-09-26 13:30:48 UTC) #4
simo
http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resources.grd#newcode4302 chrome/app/generated_resources.grd:4302: <message name="IDS_POLICY_DEFAULT_SEARCH_DISABLED" desc="The text displayed in the status column ...
9 years, 2 months ago (2011-09-29 09:33:08 UTC) #5
Mattias Nissler (ping if slow)
getting close :) http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/configuration_policy_handler.cc File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/configuration_policy_handler.cc#newcode60 chrome/browser/policy/configuration_policy_handler.cc:60: LOG(WARNING) << "Policy: Mismatch in provided ...
9 years, 2 months ago (2011-09-30 09:01:33 UTC) #6
simo
http://codereview.chromium.org/7972013/diff/15001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7972013/diff/15001/chrome/app/generated_resources.grd#newcode4296 chrome/app/generated_resources.grd:4296: <message name="IDS_POLICY_OUT_OF_RANGE_ERROR" desc="The text displayed in the status column ...
9 years, 2 months ago (2011-09-30 13:00:41 UTC) #7
Mattias Nissler (ping if slow)
9 years, 2 months ago (2011-09-30 13:33:35 UTC) #8
almost there :)

http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi...
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (left):

http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider_unittest.cc:196:
TEST_P(ConfigDirPolicyProviderValueTest, NullValue) {
On 2011/09/30 13:00:41, simo wrote:
> On 2011/09/30 09:01:33, Mattias Nissler wrote:
> > I don't understand why you removed this.
> Because as far as I can see it, this test was meant to make sure that if you
> pass in a NullValue to a provider, it will not be added to the PolicyMap. This
> isn't the case anymore since I add any value to the map.

Ah, I see. This is correct, the test is obsolete.

http://codereview.chromium.org/7972013/diff/19005/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/7972013/diff/19005/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:438:
errors->AddError(kPolicyDefaultSearchProviderSearchURL,
Indentation

http://codereview.chromium.org/7972013/diff/19005/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.h (right):

http://codereview.chromium.org/7972013/diff/19005/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.h:32: PolicyErrorMap* errors)
OVERRIDE;
Hm, why did the simpler Apply disappear from here? I actually liked it, since it
makes writing the simple handlers easier, so can we have it back? Note that this
still works after moving the Check-and-Apply to the pref store.

http://codereview.chromium.org/7972013/diff/19005/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7972013/diff/19005/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:104: PolicyMap*
policies, const HandlerList* policy_handlers) {
Each argument on a separate line if the function declaration doesn't fit on a
single line.

Powered by Google App Engine
This is Rietveld 408576698