|
|
Created:
9 years, 3 months ago by simo Modified:
9 years, 2 months ago Reviewers:
Mattias Nissler (ping if slow) CC:
chromium-reviews Visibility:
Public. |
DescriptionConfigurationPolicyPrefStore 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
Messages
Total messages: 8 (0 generated)
first round of comments http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:27: const char* pref_path) : policy_type_(policy), please break before the : I don't think the style guide disallows what you did here, but I've never seen a multi-line initializer list in the code base that was indented like this. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:47: std::string message = "Expected " + ValueTypeToString(value_type_); Ah, so what about i18n? These errors show up in the UI, so we should be able to translate them. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:61: "dictionary", "list" }; each on one line please. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:81: if (value && CheckPolicySettings(policies, &errors)) This three-line pattern repeats a lot. Have you thought about implementing ApplyPolicySettings in CheckPolicyValueType, calling a new ApplyPolicyValue(const Value* value) function that subclasses of CheckPolicyValueType can override? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:101: value->GetAsBoolean(&disable_sync); you should check the return value of GetAsBoolean as well. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:123: value->GetAsBoolean(&auto_fill_enabled); check return value. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:158: value->GetAsString(&string_value); check return value. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:170: #endif // !defined(OS_CHROMEOS) this preprocessor guard is misplaced, should move one line down. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:190: value->GetAsString(&string_value); check return value. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:268: "range " + int_value; i18n http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:275: message = "IncognitoModeAvailability policy value could not be parsed"; i18n http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:283: // kPolicyIncognitoEnabled. could move the deprecated_enabled lookup from line 262 to here. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:287: errors->AddError(kPolicyIncognitoEnabled, i18n http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:304: Value::CreateIntegerValue(availability_enum_value_)); Ah, keeping state in the handler prevents us from reusing them. Can't we just have an internal function that checks and extracts the value which could then called by both ApplyPolicySettings() and CheckPolicySettings()? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:424: "is disabled by policy."; i18n http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:516: PrefValueMap* prefs) { Hm, should we run CheckPolicySettings() here? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:571: // Required entries are not there. Remove any related entries. It might be nicer to just update a private PrefValueMap here and then copy stuff over in the end. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:582: bool ProxyPolicyHandler::CheckPolicySettings(PolicyMap* policies, wow > 200 lines function! Can you please break it up into logical parts? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:594: LOG(WARNING) << message; It seems like we want to log the errors produced by CheckPolicySettings() when we run ApplyPolicySettings() in any case, so is there a way to move the LOG statements to a more central place? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_handler.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.h:19: class CheckPolicyValueType : public ConfigurationPolicyHandlerInterface { Maybe rename to TypeCheckingPolicyHandler? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.h:23: const char* pref_path); Why do you have the pref_path as a constructor argument here? There shouldn't be a reason for this class to require it. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.h:33: // Returns a weak reference to the value pointed to by policy_value_.get(). Comment doesn't apply? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_handler_factory.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_factory.h:21: class ConfigurationPolicyHandlerFactory { seems like there's no reason to make this a class, since all it does is declare two types and provide a static method. Remove the class and move the declarations directly into the policy namespace? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_handler_interface.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:11: #include "base/memory/scoped_ptr.h" not needed? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:12: #include "base/values.h" not needed? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:13: #include "chrome/browser/policy/policy_error_map.h" forward declaration? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:14: #include "chrome/browser/policy/policy_map.h" forward declaration? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:15: #include "chrome/browser/prefs/pref_value_map.h" forward declaration? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:30: virtual bool CheckPolicySettings(PolicyMap* map, PolicyErrorMap* errors) = 0; Make the policy map const? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:34: virtual void ApplyPolicySettings(PolicyMap* map, PrefValueMap* prefs) = 0; make the policy map const? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_pref_store.cc:105: const ConfigurationPolicyHandlerFactory::HandlerList* handlers = Please pass the handler list to the constructor in order to make this testable. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_pref_store.cc:115: Value* ConfigurationPolicyPrefKeeper::GetPolicyValue( where is this called from? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_pref_store.cc:449: } // namespace policyConfigurationPolicyHandlerFactory::HandlerList revert http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_pref_store.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_pref_store.h:85: static bool IsIncognitoModePolicy(ConfigurationPolicyType policy); I can't find why this is needed http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_provider.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_provider.cc:32: if (policies->Get(i->name, &value) && !value->IsType(Value::TYPE_NULL)) just remove the type check entirely. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... File chrome/browser/policy/policy_error_map.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... chrome/browser/policy/policy_error_map.cc:33: const_iterator it = range.first; move int loop initializer below? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... chrome/browser/policy/policy_error_map.cc:35: for ( ; it != range.second; ++it) { no need for curlies http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... File chrome/browser/policy/policy_error_map.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... chrome/browser/policy/policy_error_map.h:21: typedef std::multimap<ConfigurationPolicyType, StringValue*> PolicyMapType; Why is this a StringValue* instead of a plain string? http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... chrome/browser/policy/policy_error_map.h:45: DISALLOW_COPY_AND_ASSIGN(PolicyErrorMap); #include "base/basic_types.h"
http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:27: const char* pref_path) : policy_type_(policy), On 2011/09/20 13:12:25, Mattias Nissler wrote: > please break before the : > > I don't think the style guide disallows what you did here, but I've never seen a > multi-line initializer list in the code base that was indented like this. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:47: std::string message = "Expected " + ValueTypeToString(value_type_); On 2011/09/20 13:12:25, Mattias Nissler wrote: > Ah, so what about i18n? These errors show up in the UI, so we should be able to > translate them. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:61: "dictionary", "list" }; On 2011/09/20 13:12:25, Mattias Nissler wrote: > each on one line please. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:81: if (value && CheckPolicySettings(policies, &errors)) On 2011/09/20 13:12:25, Mattias Nissler wrote: > This three-line pattern repeats a lot. Have you thought about implementing > ApplyPolicySettings in CheckPolicyValueType, calling a new > ApplyPolicyValue(const Value* value) function that subclasses of > CheckPolicyValueType can override? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:101: value->GetAsBoolean(&disable_sync); On 2011/09/20 13:12:25, Mattias Nissler wrote: > you should check the return value of GetAsBoolean as well. Is it enough if I do this in a DCHECK? The call to CheckPolicySettings should check that the value has the right type. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:123: value->GetAsBoolean(&auto_fill_enabled); On 2011/09/20 13:12:25, Mattias Nissler wrote: > check return value. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:158: value->GetAsString(&string_value); On 2011/09/20 13:12:25, Mattias Nissler wrote: > check return value. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:170: #endif // !defined(OS_CHROMEOS) On 2011/09/20 13:12:25, Mattias Nissler wrote: > this preprocessor guard is misplaced, should move one line down. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:190: value->GetAsString(&string_value); On 2011/09/20 13:12:25, Mattias Nissler wrote: > check return value. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:268: "range " + int_value; On 2011/09/20 13:12:25, Mattias Nissler wrote: > i18n Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:275: message = "IncognitoModeAvailability policy value could not be parsed"; On 2011/09/20 13:12:25, Mattias Nissler wrote: > i18n Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:283: // kPolicyIncognitoEnabled. On 2011/09/20 13:12:25, Mattias Nissler wrote: > could move the deprecated_enabled lookup from line 262 to here. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:287: errors->AddError(kPolicyIncognitoEnabled, On 2011/09/20 13:12:25, Mattias Nissler wrote: > i18n Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:304: Value::CreateIntegerValue(availability_enum_value_)); On 2011/09/20 13:12:25, Mattias Nissler wrote: > Ah, keeping state in the handler prevents us from reusing them. Can't we just > have an internal function that checks and extracts the value which could then > called by both ApplyPolicySettings() and CheckPolicySettings()? Ah yes, true. I think adding an method isn't really going to help here though because the different conversions have to be done separately in CheckPolicySettings() to generate different error messages. I will just repeat the conversions in ApplyPolicySettings() but with DCHECKs. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:424: "is disabled by policy."; On 2011/09/20 13:12:25, Mattias Nissler wrote: > i18n Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:516: PrefValueMap* prefs) { On 2011/09/20 13:12:25, Mattias Nissler wrote: > Hm, should we run CheckPolicySettings() here? Hm well, the CheckPolicySettings() for each of the handlers in |handlers| will be run when ApplyPolicySettings is called on them. Running CheckPolicySettings() here doesn't really help because depending on the reason the checks fail, I might still have to do some work in ApplyPolicySettings(). That's why I made separate methods like DefaultSearchURLIsPresentAndValid() and DefaultSearchProviderIsDisabled(). http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:571: // Required entries are not there. Remove any related entries. On 2011/09/20 13:12:25, Mattias Nissler wrote: > It might be nicer to just update a private PrefValueMap here and then copy stuff > over in the end. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:582: bool ProxyPolicyHandler::CheckPolicySettings(PolicyMap* policies, On 2011/09/20 13:12:25, Mattias Nissler wrote: > wow > 200 lines function! Can you please break it up into logical parts? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.cc:594: LOG(WARNING) << message; On 2011/09/20 13:12:25, Mattias Nissler wrote: > It seems like we want to log the errors produced by CheckPolicySettings() when > we run ApplyPolicySettings() in any case, so is there a way to move the LOG > statements to a more central place? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_handler.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.h:19: class CheckPolicyValueType : public ConfigurationPolicyHandlerInterface { On 2011/09/20 13:12:25, Mattias Nissler wrote: > Maybe rename to TypeCheckingPolicyHandler? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.h:23: const char* pref_path); On 2011/09/20 13:12:25, Mattias Nissler wrote: > Why do you have the pref_path as a constructor argument here? There shouldn't be > a reason for this class to require it. I've removed it and I'm using PolicyStatus::GetPolicyName() instead in the log message. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler.h:33: // Returns a weak reference to the value pointed to by policy_value_.get(). On 2011/09/20 13:12:25, Mattias Nissler wrote: > Comment doesn't apply? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_handler_factory.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_factory.h:21: class ConfigurationPolicyHandlerFactory { On 2011/09/20 13:12:25, Mattias Nissler wrote: > seems like there's no reason to make this a class, since all it does is declare > two types and provide a static method. Remove the class and move the > declarations directly into the policy namespace? Okay, I've removed the class and renamed this file to configuration_policy_handler_list.h. I couldn't find a good place to include this anywhere else. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_handler_interface.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:11: #include "base/memory/scoped_ptr.h" On 2011/09/20 13:12:25, Mattias Nissler wrote: > not needed? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:12: #include "base/values.h" On 2011/09/20 13:12:25, Mattias Nissler wrote: > not needed? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:13: #include "chrome/browser/policy/policy_error_map.h" On 2011/09/20 13:12:25, Mattias Nissler wrote: > forward declaration? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:14: #include "chrome/browser/policy/policy_map.h" On 2011/09/20 13:12:25, Mattias Nissler wrote: > forward declaration? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:15: #include "chrome/browser/prefs/pref_value_map.h" On 2011/09/20 13:12:25, Mattias Nissler wrote: > forward declaration? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:30: virtual bool CheckPolicySettings(PolicyMap* map, PolicyErrorMap* errors) = 0; On 2011/09/20 13:12:25, Mattias Nissler wrote: > Make the policy map const? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_handler_interface.h:34: virtual void ApplyPolicySettings(PolicyMap* map, PrefValueMap* prefs) = 0; On 2011/09/20 13:12:25, Mattias Nissler wrote: > make the policy map const? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_pref_store.cc:105: const ConfigurationPolicyHandlerFactory::HandlerList* handlers = On 2011/09/20 13:12:25, Mattias Nissler wrote: > Please pass the handler list to the constructor in order to make this testable. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_pref_store.cc:115: Value* ConfigurationPolicyPrefKeeper::GetPolicyValue( On 2011/09/20 13:12:25, Mattias Nissler wrote: > where is this called from? Forgot to remove it. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_pref_store.cc:449: } // namespace policyConfigurationPolicyHandlerFactory::HandlerList On 2011/09/20 13:12:25, Mattias Nissler wrote: > revert Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_pref_store.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_pref_store.h:85: static bool IsIncognitoModePolicy(ConfigurationPolicyType policy); On 2011/09/20 13:12:25, Mattias Nissler wrote: > I can't find why this is needed Oh right, I forgot to remove it. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_provider.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_provider.cc:32: if (policies->Get(i->name, &value) && !value->IsType(Value::TYPE_NULL)) On 2011/09/20 13:12:25, Mattias Nissler wrote: > just remove the type check entirely. Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... File chrome/browser/policy/policy_error_map.cc (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... chrome/browser/policy/policy_error_map.cc:33: const_iterator it = range.first; On 2011/09/20 13:12:25, Mattias Nissler wrote: > move int loop initializer below? Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... chrome/browser/policy/policy_error_map.cc:35: for ( ; it != range.second; ++it) { On 2011/09/20 13:12:25, Mattias Nissler wrote: > no need for curlies Done. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... File chrome/browser/policy/policy_error_map.h (right): http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... chrome/browser/policy/policy_error_map.h:21: typedef std::multimap<ConfigurationPolicyType, StringValue*> PolicyMapType; On 2011/09/20 13:12:25, Mattias Nissler wrote: > Why is this a StringValue* instead of a plain string? For some reason I was thinking I needed to put StringValues in to return them in ListValue but obviously that's not the case. I'll change it to string. http://codereview.chromium.org/7972013/diff/1/chrome/browser/policy/policy_er... chrome/browser/policy/policy_error_map.h:45: DISALLOW_COPY_AND_ASSIGN(PolicyErrorMap); On 2011/09/20 13:12:25, Mattias Nissler wrote: > #include "base/basic_types.h" I couldn't find "base/basic_types.h" but I included "base/basictypes.h"
Moar comments http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:4302: <message name="IDS_POLICY_DEFAULT_SEARCH_DISABLED" desc="The text displayed in the status column when a policy value has been ignored because default search has been disabled"> period http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:4312: Value isn't recognized. What's the difference between PARSE_ERROR, INVALID_ERROR and NOT_RECOGNIZED_ERROR? Looking at the messages, they're all pretty much the same. Either make the messages more precise or unify all these errors. I prefer the former. http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:4315: Proxy settings details are defined without setting a proxy mode. Isn't this similar to NOT_SPECIFIED? http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:4339: Ignored because ProxyServerMode is also defined. It seems like many of these messages are "X is ignored because of Y". Maybe we can unify them? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:57: if (CheckPolicySettings(policies, &errors)) { so you now have the Check-and-report-errors-before-applying here in TypeCheckingPolicyHandler, but isn't this something we'd want for any handler? Thus, would it actually make sense to put this logic into the caller, i.e. ConfigurationPolicyPrefStore? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:60: LOG(WARNING) << "Policy: Mismatch in provided and expected policy value " Why not just GenerateLogMessages(&errors) http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:110: DCHECK(value->GetAsBoolean(&disable_sync)); DCHECK compiles to nothing in release builds, so you'd lose the GetAsBoolean :) Also, we should guard against setting anything in this case, so please make the GetAsBoolean() return value part of the condition below. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:127: DCHECK(value->GetAsBoolean(&auto_fill_enabled)); same as above http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:147: #if !defined(OS_CHROMEOS) It seems like this handler is only useful if we're not on ChromeOS. Maybe we can just not instantiate the handler for OS_CHROMEOS and remove the guards here (and below)? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:157: DCHECK(value->GetAsString(&string_value)); ditto http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:184: DCHECK(value->GetAsString(&string_value)); ditto http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:207: DCHECK(value->GetAsBoolean(&allow_file_selection_dialogs)); ditto http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:233: namespace { anonymous namespace should go to the top of the file. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:239: LOG(WARNING) << ASCIIToUTF16("Policy (") + policy + ASCIIToUTF16("): ") no need to convert to UTF16 http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:308: IDS_POLICY_OUT_OF_RANGE_ERROR, "" + int_value); Wow. I guess you're not aware of base/string_number_conversions.h and the IntToString that's declared there? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:338: DCHECK(availability->GetAsInteger(&int_value)); DCHECK compiles away http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:345: DCHECK(deprecated_enabled->GetAsBoolean(&enabled)); ditto http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:376: encodings.append(s); base/string_util.h JoinString()? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:388: current < arraysize(kDefaultSearchPolicyMap); ++current) { identation http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:393: policy_type, indentation http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:397: handlers_.push_back(new DefaultSearchEncodingsPolicyHandler()); if you need to special-case this anyway, why not just remove the entry from kDefaultSearchPolicyMap? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:414: errors->AddError(kPolicyDefaultSearchProviderName, message_id); Is it OK to add errors about these policy settings even if they're not present in policy? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:427: errors->AddError(kPolicyDefaultSearchProviderSearchURL, Am I missing something or are we adding this error unconditionally if the policy is not specified? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:431: if (DefaultSearchURLIsPresentAndValid(policies)) { We're adding an error if the URL is present and valid? That doesn't seem right to me. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:459: // Apply all default search policies. Shouldn't this block be moved into the if-block starting at 469? We shouldn't apply anything if the URL is not valid. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:463: for ( ; handler != handlers_.end(); ++handler) { No curlies required. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:483: policies->Get(kPolicyDefaultSearchProviderSearchURL); search_url is already read above. reuse? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:485: DCHECK(search_url->GetAsString(&search_url_string)); DCHECK compiles away. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:487: GURL(search_url_string).host()); indentation http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:493: std::string()); indentation http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:500: CopyTempPrefsToRealPrefs(&temp_prefs, prefs); Wait. clear-then-copy? Am I missing something or is this a very elaborate way of doing nothing? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:508: return false; I think we shouldn't abort here, since we want to collect error messages for all policies and not just the first one failing. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:528: if (search_url) { you can as well invert the checks and put early returns, so you don't get into indentation problems in 535. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:542: const std::string& path) { function parameters must line up http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:548: void DefaultSearchPolicyHandler::CopyTempPrefsToRealPrefs( Can't we just implement a MergeFrom() in PrefValueMap? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:562: remove extra newline http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:564: current < arraysize(kDefaultSearchPolicyMap); ++current) { +1 space indentation http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:602: if (mode_value == ProxyPrefs::kDirectProxyModeName) { Hm, maybe we can specify all these checks here in a more declarative way? One idea would be to make a table with each entry saying whether |server|, |pac_url|, |bypass_list| are valid for a given |mode_value| and which |message_id| to set if invalid policies are specified? Then you could loop over the table entries and do handle everything in one block. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:675: The following seems to be the same code as CheckProxyModeAndServerMode. Share? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:679: DCHECK(mode->GetAsString(&string_mode)); compiles away http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:683: DCHECK(server_mode->GetAsInteger(&int_mode)); ditto http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:705: } I'd put a newline here. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler.h (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.h:35: virtual void ApplyPolicyValue(PrefValueMap* prefs, const Value* value) = 0; Please document. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.h:63: // ConfigurationPolicyHandlerInterface method: This is actually a TypeCheckingPolicyHandler override. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.h:98: // PrefValueMap* prefs) OVERRIDE; remove old code. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.h:112: remove extra newline http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler_list.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler_list.cc:182: kSimplePolicyMap[i].policy_type, indentation (maybe break before new?) http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler_list.h (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler_list.h:21: // derived from ConfigurationPolicyHandlerInterface. What we usually do in this case is passing in a vector pointer as parameter that is then filled in by the function. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler_list.h:27: struct PolicyToPreferenceMapEntry { Do we actually need to publish this type? Can we just move it to the .cc file? http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_pref_store.cc:102: (*handler)->ApplyPolicySettings(policies, &prefs_); As mentioned before, maybe do the check-before-apply explicitly here? Thinking about it, from the code in the handlers it looks like we have a great deal of duplication between Check() and Apply() in various handlers. Maybe it would make sense to actually not have separate functions for Check() and Apply() but instead pass the error map to Apply()? If we do so, we need to make sure that Apply() can abort at any point without having to worry about clearing out settings from the passed PrefValueMap, so it'd make sense to have each handler dump stuff in a temp PrefValueMap that we merge into the main prefs if Apply was successful. Just a thought for your consideration! http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/policy... File chrome/browser/policy/policy_error_map.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/policy... chrome/browser/policy/policy_error_map.cc:72: std::pair<ConfigurationPolicyType, string16>(policy, error)); You can use std::make_pair to avoid the explicit type parameters and then remove Insert() http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/policy... File chrome/browser/policy/policy_error_map.h (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/policy... chrome/browser/policy/policy_error_map.h:39: std::string replacement_string); const ref
http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:4302: <message name="IDS_POLICY_DEFAULT_SEARCH_DISABLED" desc="The text displayed in the status column when a policy value has been ignored because default search has been disabled"> On 2011/09/26 13:30:48, Mattias Nissler wrote: > period Done. http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:4312: Value isn't recognized. On 2011/09/26 13:30:48, Mattias Nissler wrote: > What's the difference between PARSE_ERROR, INVALID_ERROR and > NOT_RECOGNIZED_ERROR? Looking at the messages, they're all pretty much the same. > Either make the messages more precise or unify all these errors. I prefer the > former. I've removed NOT_RECOGNIZED and PARSE_ERROR and used TYPE_ERROR or OUT_OF_RANGE_ERROR instead and made INVALID_ERROR more precise. http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:4315: Proxy settings details are defined without setting a proxy mode. On 2011/09/26 13:30:48, Mattias Nissler wrote: > Isn't this similar to NOT_SPECIFIED? Yes, true. http://codereview.chromium.org/7972013/diff/7001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:4339: Ignored because ProxyServerMode is also defined. On 2011/09/26 13:30:48, Mattias Nissler wrote: > It seems like many of these messages are "X is ignored because of Y". Maybe we > can unify them? Yes, there are two messages of this form but in one the reason is "ProxyServerMode is also defined" and the other one is "default search is disabled". I'm not sure how to merge these because the strings wouldn't be translatable anymore if I use placeholders for the reasons. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:60: LOG(WARNING) << "Policy: Mismatch in provided and expected policy value " On 2011/09/26 13:30:48, Mattias Nissler wrote: > Why not just GenerateLogMessages(&errors) Because the message in |errors| doesn't contain the actual type the value had because on about:policy, that is apparent. If we don't need it in the log message either, I will just change it to GeneratelogMessages(&errors). http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:110: DCHECK(value->GetAsBoolean(&disable_sync)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > DCHECK compiles to nothing in release builds, so you'd lose the GetAsBoolean :) > Also, we should guard against setting anything in this case, so please make the > GetAsBoolean() return value part of the condition below. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:127: DCHECK(value->GetAsBoolean(&auto_fill_enabled)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > same as above Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:147: #if !defined(OS_CHROMEOS) On 2011/09/26 13:30:48, Mattias Nissler wrote: > It seems like this handler is only useful if we're not on ChromeOS. Maybe we can > just not instantiate the handler for OS_CHROMEOS and remove the guards here (and > below)? Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:157: DCHECK(value->GetAsString(&string_value)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > ditto Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:184: DCHECK(value->GetAsString(&string_value)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > ditto Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:207: DCHECK(value->GetAsBoolean(&allow_file_selection_dialogs)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > ditto Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:233: namespace { On 2011/09/26 13:30:48, Mattias Nissler wrote: > anonymous namespace should go to the top of the file. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:239: LOG(WARNING) << ASCIIToUTF16("Policy (") + policy + ASCIIToUTF16("): ") On 2011/09/26 13:30:48, Mattias Nissler wrote: > no need to convert to UTF16 I can't get it to compile if I don't because |policy| is UTF16. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:308: IDS_POLICY_OUT_OF_RANGE_ERROR, "" + int_value); No I guess I wasn't. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:338: DCHECK(availability->GetAsInteger(&int_value)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > DCHECK compiles away Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:345: DCHECK(deprecated_enabled->GetAsBoolean(&enabled)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > ditto Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:376: encodings.append(s); On 2011/09/26 13:30:48, Mattias Nissler wrote: > base/string_util.h JoinString()? Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:388: current < arraysize(kDefaultSearchPolicyMap); ++current) { On 2011/09/26 13:30:48, Mattias Nissler wrote: > identation Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:393: policy_type, On 2011/09/26 13:30:48, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:397: handlers_.push_back(new DefaultSearchEncodingsPolicyHandler()); On 2011/09/26 13:30:48, Mattias Nissler wrote: > if you need to special-case this anyway, why not just remove the entry from > kDefaultSearchPolicyMap? Because I need it to be in the map in other methods like ClearDefaultSearchPreferences(). I have removed this method since but I'm now using it in CheckPolicySettings(); http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:414: errors->AddError(kPolicyDefaultSearchProviderName, message_id); On 2011/09/26 13:30:48, Mattias Nissler wrote: > Is it OK to add errors about these policy settings even if they're not present > in policy? Ah I didn't notice that. I don't think it is! http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:427: errors->AddError(kPolicyDefaultSearchProviderSearchURL, On 2011/09/26 13:30:48, Mattias Nissler wrote: > Am I missing something or are we adding this error unconditionally if the policy > is not specified? Oh right, I should check whether any other default search policies are actually set. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:431: if (DefaultSearchURLIsPresentAndValid(policies)) { On 2011/09/26 13:30:48, Mattias Nissler wrote: > We're adding an error if the URL is present and valid? That doesn't seem right > to me. Oh, that was a mistake. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:459: // Apply all default search policies. On 2011/09/26 13:30:48, Mattias Nissler wrote: > Shouldn't this block be moved into the if-block starting at 469? We shouldn't > apply anything if the URL is not valid. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:463: for ( ; handler != handlers_.end(); ++handler) { On 2011/09/26 13:30:48, Mattias Nissler wrote: > No curlies required. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:483: policies->Get(kPolicyDefaultSearchProviderSearchURL); On 2011/09/26 13:30:48, Mattias Nissler wrote: > search_url is already read above. reuse? Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:485: DCHECK(search_url->GetAsString(&search_url_string)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > DCHECK compiles away. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:487: GURL(search_url_string).host()); On 2011/09/26 13:30:48, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:493: std::string()); On 2011/09/26 13:30:48, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:500: CopyTempPrefsToRealPrefs(&temp_prefs, prefs); On 2011/09/26 13:30:48, Mattias Nissler wrote: > Wait. clear-then-copy? Am I missing something or is this a very elaborate way > of doing nothing? Oh right, of course. I obviously wasn't thinking. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:508: return false; On 2011/09/26 13:30:48, Mattias Nissler wrote: > I think we shouldn't abort here, since we want to collect error messages for all > policies and not just the first one failing. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:528: if (search_url) { On 2011/09/26 13:30:48, Mattias Nissler wrote: > you can as well invert the checks and put early returns, so you don't get into > indentation problems in 535. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:542: const std::string& path) { On 2011/09/26 13:30:48, Mattias Nissler wrote: > function parameters must line up Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:548: void DefaultSearchPolicyHandler::CopyTempPrefsToRealPrefs( On 2011/09/26 13:30:48, Mattias Nissler wrote: > Can't we just implement a MergeFrom() in PrefValueMap? Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:562: On 2011/09/26 13:30:48, Mattias Nissler wrote: > remove extra newline Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:564: current < arraysize(kDefaultSearchPolicyMap); ++current) { On 2011/09/26 13:30:48, Mattias Nissler wrote: > +1 space indentation Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:602: if (mode_value == ProxyPrefs::kDirectProxyModeName) { On 2011/09/26 13:30:48, Mattias Nissler wrote: > Hm, maybe we can specify all these checks here in a more declarative way? One > idea would be to make a table with each entry saying whether |server|, > |pac_url|, |bypass_list| are valid for a given |mode_value| and which > |message_id| to set if invalid policies are specified? Then you could loop over > the table entries and do handle everything in one block. I like that, I've done it :) http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:675: On 2011/09/26 13:30:48, Mattias Nissler wrote: > The following seems to be the same code as CheckProxyModeAndServerMode. Share? In CheckProxyModeAndServerMode the |mode_value| is a string whereas here it's using an enum value for |proxy_mode|. I'm not sure how to unify these two. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:679: DCHECK(mode->GetAsString(&string_mode)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > compiles away Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:683: DCHECK(server_mode->GetAsInteger(&int_mode)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > ditto Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:705: } On 2011/09/26 13:30:48, Mattias Nissler wrote: > I'd put a newline here. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler.h (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.h:35: virtual void ApplyPolicyValue(PrefValueMap* prefs, const Value* value) = 0; On 2011/09/26 13:30:48, Mattias Nissler wrote: > Please document. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.h:63: // ConfigurationPolicyHandlerInterface method: On 2011/09/26 13:30:48, Mattias Nissler wrote: > This is actually a TypeCheckingPolicyHandler override. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.h:98: // PrefValueMap* prefs) OVERRIDE; On 2011/09/26 13:30:48, Mattias Nissler wrote: > remove old code. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.h:112: On 2011/09/26 13:30:48, Mattias Nissler wrote: > remove extra newline Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler_list.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler_list.cc:182: kSimplePolicyMap[i].policy_type, On 2011/09/26 13:30:48, Mattias Nissler wrote: > indentation (maybe break before new?) Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler_list.h (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler_list.h:21: // derived from ConfigurationPolicyHandlerInterface. On 2011/09/26 13:30:48, Mattias Nissler wrote: > What we usually do in this case is passing in a vector pointer as parameter that > is then filled in by the function. Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler_list.h:27: struct PolicyToPreferenceMapEntry { On 2011/09/26 13:30:48, Mattias Nissler wrote: > Do we actually need to publish this type? Can we just move it to the .cc file? I'm currently using it for the DefaultSearchPolicyHandler to list all default search policies. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/policy... File chrome/browser/policy/policy_error_map.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/policy... chrome/browser/policy/policy_error_map.cc:72: std::pair<ConfigurationPolicyType, string16>(policy, error)); On 2011/09/26 13:30:48, Mattias Nissler wrote: > You can use std::make_pair to avoid the explicit type parameters and then remove > Insert() Done. http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/policy... File chrome/browser/policy/policy_error_map.h (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/policy... chrome/browser/policy/policy_error_map.h:39: std::string replacement_string); On 2011/09/26 13:30:48, Mattias Nissler wrote: > const ref Done.
getting close :) http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler.cc:60: LOG(WARNING) << "Policy: Mismatch in provided and expected policy value " On 2011/09/29 09:33:08, simo wrote: > On 2011/09/26 13:30:48, Mattias Nissler wrote: > > Why not just GenerateLogMessages(&errors) > Because the message in |errors| doesn't contain the actual type the value had > because on about:policy, that is apparent. If we don't need it in the log > message either, I will just change it to GeneratelogMessages(&errors). I'm all for GenerateLogMessages then :) http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler_list.h (right): http://codereview.chromium.org/7972013/diff/7001/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler_list.h:27: struct PolicyToPreferenceMapEntry { On 2011/09/29 09:33:08, simo wrote: > On 2011/09/26 13:30:48, Mattias Nissler wrote: > > Do we actually need to publish this type? Can we just move it to the .cc file? > I'm currently using it for the DefaultSearchPolicyHandler to list all default > search policies. Done. http://codereview.chromium.org/7972013/diff/15001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7972013/diff/15001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4296: <message name="IDS_POLICY_OUT_OF_RANGE_ERROR" desc="The text displayed in the status column when a policy value is out of range."> I guess we'll also want to communicate the actual range that is allowed, no? We can fix this in a follow up CL later. File a bug? http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/brows... File chrome/browser/policy/browser_policy_connector.cc (right): http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/brows... chrome/browser/policy/browser_policy_connector.cc:7: #include <vector> what is this needed for? 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) { I don't understand why you removed this. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:44: remove extra newline http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:154: if (CheckPolicySettings(policies, &errors)) { as discussed, can we move the if (Check) Apply to the pref store? http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:437: for (unsigned int current = 0; should be size_t instead of unsigned int, as that's what is returned by arraysize. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:582: void DefaultSearchPolicyHandler::CopyTempPrefsToRealPrefs( This is not used anymore. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:610: remove extra blank line. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:628: for (unsigned int current = 0; size_t http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:630: ProxyModeValidationEntry entry = kProxyModeValidationMap[current]; Please don't copy, use a const ref or a pointer. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:732: bypass_list && bypass_list->GetAsString(&bypass_list_string)) { break after && http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_handler_list.h (right): http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler_list.h:33: extern const PolicyToPreferenceMapEntry kSimplePolicyMap[]; OK, but even if the struct is used elsewhere, this should not need to be declared here.
http://codereview.chromium.org/7972013/diff/15001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7972013/diff/15001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4296: <message name="IDS_POLICY_OUT_OF_RANGE_ERROR" desc="The text displayed in the status column when a policy value is out of range."> On 2011/09/30 09:01:33, Mattias Nissler wrote: > I guess we'll also want to communicate the actual range that is allowed, no? We > can fix this in a follow up CL later. File a bug? Ok. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/brows... File chrome/browser/policy/browser_policy_connector.cc (right): http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/brows... chrome/browser/policy/browser_policy_connector.cc:7: #include <vector> On 2011/09/30 09:01:33, Mattias Nissler wrote: > what is this needed for? Forgot to remove it. 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 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. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_handler.cc (right): http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:44: On 2011/09/30 09:01:33, Mattias Nissler wrote: > remove extra newline Done. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:154: if (CheckPolicySettings(policies, &errors)) { On 2011/09/30 09:01:33, Mattias Nissler wrote: > as discussed, can we move the if (Check) Apply to the pref store? Done. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:437: for (unsigned int current = 0; On 2011/09/30 09:01:33, Mattias Nissler wrote: > should be size_t instead of unsigned int, as that's what is returned by > arraysize. Done. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:582: void DefaultSearchPolicyHandler::CopyTempPrefsToRealPrefs( On 2011/09/30 09:01:33, Mattias Nissler wrote: > This is not used anymore. Done. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:610: On 2011/09/30 09:01:33, Mattias Nissler wrote: > remove extra blank line. Done. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:628: for (unsigned int current = 0; On 2011/09/30 09:01:33, Mattias Nissler wrote: > size_t Done. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:630: ProxyModeValidationEntry entry = kProxyModeValidationMap[current]; On 2011/09/30 09:01:33, Mattias Nissler wrote: > Please don't copy, use a const ref or a pointer. Done. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler.cc:732: bypass_list && bypass_list->GetAsString(&bypass_list_string)) { On 2011/09/30 09:01:33, Mattias Nissler wrote: > break after && Done. http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_handler_list.h (right): http://codereview.chromium.org/7972013/diff/15001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_handler_list.h:33: extern const PolicyToPreferenceMapEntry kSimplePolicyMap[]; On 2011/09/30 09:01:33, Mattias Nissler wrote: > OK, but even if the struct is used elsewhere, this should not need to be > declared here. Done.
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. |