Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1692011:
Implementation of managed policy: Policy Abstraction (Closed)
|
Created:
10 years, 8 months ago by danno Modified:
9 years, 7 months ago CC:
chromium-reviews, Bernhard Bauer, Paweł Hajdan Jr., ben+cc_chromium.org Visibility:
Public. |
DescriptionImplementation of managed policy abstraction on top of a preference store. This is preparation work for implementing platform-specific policy.
BUG=none
TEST=--gtest_filter=ConfigurationPolicyPrefStoreTest*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=47030
Patch Set 1 #
Total comments: 40
Patch Set 2 : incorporate reviewer feedback #Patch Set 3 : fix style nits #
Total comments: 18
Patch Set 4 : incorporate review feedback, implement tests, add comments #
Total comments: 10
Patch Set 5 : add policy builder, review feedback #Patch Set 6 : move windows implementation to another CL, cleanup #Patch Set 7 : add missing file #
Total comments: 54
Patch Set 8 : review feedback #
Total comments: 6
Patch Set 9 : fix nits #Patch Set 10 : removed windows from build that accidentally slipped in #
Total comments: 22
Patch Set 11 : review feedback #Patch Set 12 : fix style nits #
Total comments: 2
Patch Set 13 : fix nits and unit tests #Patch Set 14 : check no changes #Messages
Total messages: 17 (0 generated)
http://codereview.chromium.org/1692011/diff/1/4 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/1/4#newcode33 chrome/browser/configuration_policy_pref_store.h:33: kSettingProxy = 1, Why are we using explicit values here? I thought the point of using |enum|s was to avoid that.
http://codereview.chromium.org/1692011/diff/1/2 File chrome/browser/configuration_policy.h (right): http://codereview.chromium.org/1692011/diff/1/2#newcode30 chrome/browser/configuration_policy.h:30: }; nit: DISALLOW_COPY_AND_ASSING? http://codereview.chromium.org/1692011/diff/1/3 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/1/3#newcode39 chrome/browser/configuration_policy_pref_store.cc:39: const SimpleSettingMapEntry* current; nit: could you move this inside the loop initialization? http://codereview.chromium.org/1692011/diff/1/3#newcode41 chrome/browser/configuration_policy_pref_store.cc:41: sizeof(simple_settings_map_) / sizeof(SimpleSettingMapEntry); nit: use a macro from base/basictypes.h? maybe arraysize? http://codereview.chromium.org/1692011/diff/1/4 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/1/4#newcode18 chrome/browser/configuration_policy_pref_store.h:18: nit: remove this blank line. http://codereview.chromium.org/1692011/diff/1/4#newcode28 chrome/browser/configuration_policy_pref_store.h:28: nit: remove this blank line. http://codereview.chromium.org/1692011/diff/1/4#newcode31 chrome/browser/configuration_policy_pref_store.h:31: typedef enum { nit: why not just enum SettingType? http://codereview.chromium.org/1692011/diff/1/4#newcode53 chrome/browser/configuration_policy_pref_store.h:53: DISALLOW_COPY_AND_ASSIGN(ConfigurationPolicyPrefStore); nit: this should be in the end. http://codereview.chromium.org/1692011/diff/1/4#newcode56 chrome/browser/configuration_policy_pref_store.h:56: typedef struct SimpleSettingMapEntry_struct { nit: why not just struct SimpleSettingMapEntry? http://codereview.chromium.org/1692011/diff/1/4#newcode67 chrome/browser/configuration_policy_pref_store.h:67: #endif nit: // CHROME_BROWSER_CONFIGURATION_POLICY_PREF_STORE_H_ Also do this in all the header files. http://codereview.chromium.org/1692011/diff/1/6 File chrome/browser/configuration_policy_pref_store_linux.h (right): http://codereview.chromium.org/1692011/diff/1/6#newcode12 chrome/browser/configuration_policy_pref_store_linux.h:12: virtual PrefReadError ReadPrefs(); add a virtual destructor? http://codereview.chromium.org/1692011/diff/1/6#newcode15 chrome/browser/configuration_policy_pref_store_linux.h:15: #endif nit: // CHROME_BROWSER_CONFIGURATION_POLICY_PREF_STORE_LINUX_H_ http://codereview.chromium.org/1692011/diff/1/9 File chrome/browser/configuration_policy_pref_store_mac.h (right): http://codereview.chromium.org/1692011/diff/1/9#newcode15 chrome/browser/configuration_policy_pref_store_mac.h:15: #endif nit: // CHROME_BROWSER_CONFIGURATION_POLICY_PREF_STORE_MAC_H_ http://codereview.chromium.org/1692011/diff/1/13 File chrome/browser/configuration_policy_pref_store_win.h (right): http://codereview.chromium.org/1692011/diff/1/13#newcode15 chrome/browser/configuration_policy_pref_store_win.h:15: nit: remove this blank line
Some minor things I saw when I read through the code. http://codereview.chromium.org/1692011/diff/1/2 File chrome/browser/configuration_policy.h (right): http://codereview.chromium.org/1692011/diff/1/2#newcode1 chrome/browser/configuration_policy.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. I believe this file is a leftover from an older version of the CL is to be removed (Danno said so). http://codereview.chromium.org/1692011/diff/1/4 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/1/4#newcode15 chrome/browser/configuration_policy_pref_store.h:15: class GURL; I cannot see why you'd need this forward declaration. http://codereview.chromium.org/1692011/diff/1/4#newcode17 chrome/browser/configuration_policy_pref_store.h:17: class ConfigurationPolicyPrefStore : public PrefStore { Maybe put a class comment saying what it does and what it's used for? http://codereview.chromium.org/1692011/diff/1/6 File chrome/browser/configuration_policy_pref_store_linux.h (right): http://codereview.chromium.org/1692011/diff/1/6#newcode10 chrome/browser/configuration_policy_pref_store_linux.h:10: class LinuxConfigurationPolicyPrefStore : public ConfigurationPolicyPrefStore { Put a comment saying what the class does? http://codereview.chromium.org/1692011/diff/1/7 File chrome/browser/configuration_policy_pref_store_linux_unittest.cc (right): http://codereview.chromium.org/1692011/diff/1/7#newcode7 chrome/browser/configuration_policy_pref_store_linux_unittest.cc:7: class LinuxConfigurationPolicyPrefStoreTest : public PlatformTest { No need for PlatformTest here. I think you can just throw out the linux/mac bits anyway, we'll add them once we implement them. It's good to keep the windows bits for reference though. http://codereview.chromium.org/1692011/diff/1/11 File chrome/browser/configuration_policy_pref_store_unittest.cc (right): http://codereview.chromium.org/1692011/diff/1/11#newcode7 chrome/browser/configuration_policy_pref_store_unittest.cc:7: class ConfigurationPolicyPrefStoreTest : public PlatformTest { Why not testing::Test?
This is still a work in progress not ready to commit, but I've cleaned up a bunch of stuff and incorporated reviewer feedback. http://codereview.chromium.org/1692011/diff/1/2 File chrome/browser/configuration_policy.h (right): http://codereview.chromium.org/1692011/diff/1/2#newcode1 chrome/browser/configuration_policy.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/04/28 17:49:20, mnissler wrote: > I believe this file is a leftover from an older version of the CL is to be > removed (Danno said so). Yes, this file is no longer needed in the CL, it will be removed from the next patch set. http://codereview.chromium.org/1692011/diff/1/2#newcode30 chrome/browser/configuration_policy.h:30: }; On 2010/04/28 16:01:58, tfarina wrote: > nit: DISALLOW_COPY_AND_ASSING? See above. http://codereview.chromium.org/1692011/diff/1/3 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/1/3#newcode39 chrome/browser/configuration_policy_pref_store.cc:39: const SimpleSettingMapEntry* current; On 2010/04/28 16:01:58, tfarina wrote: > nit: could you move this inside the loop initialization? Done. http://codereview.chromium.org/1692011/diff/1/3#newcode41 chrome/browser/configuration_policy_pref_store.cc:41: sizeof(simple_settings_map_) / sizeof(SimpleSettingMapEntry); On 2010/04/28 16:01:58, tfarina wrote: > nit: use a macro from base/basictypes.h? maybe arraysize? cool. didn't know that this existed. done. http://codereview.chromium.org/1692011/diff/1/4 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/1/4#newcode15 chrome/browser/configuration_policy_pref_store.h:15: class GURL; On 2010/04/28 17:49:20, mnissler wrote: > I cannot see why you'd need this forward declaration. Removed. http://codereview.chromium.org/1692011/diff/1/4#newcode17 chrome/browser/configuration_policy_pref_store.h:17: class ConfigurationPolicyPrefStore : public PrefStore { On 2010/04/28 17:49:20, mnissler wrote: > Maybe put a class comment saying what it does and what it's used for? Absolutely. Actually, there are comments missing everywhere in the new classes, I will add them. http://codereview.chromium.org/1692011/diff/1/4#newcode18 chrome/browser/configuration_policy_pref_store.h:18: On 2010/04/28 16:01:58, tfarina wrote: > nit: remove this blank line. Done. http://codereview.chromium.org/1692011/diff/1/4#newcode28 chrome/browser/configuration_policy_pref_store.h:28: On 2010/04/28 16:01:58, tfarina wrote: > nit: remove this blank line. Done. http://codereview.chromium.org/1692011/diff/1/4#newcode31 chrome/browser/configuration_policy_pref_store.h:31: typedef enum { On 2010/04/28 16:01:58, tfarina wrote: > nit: why not just enum SettingType? Done. http://codereview.chromium.org/1692011/diff/1/4#newcode33 chrome/browser/configuration_policy_pref_store.h:33: kSettingProxy = 1, On 2010/04/28 15:42:26, Bernhard Bauer wrote: > Why are we using explicit values here? I thought the point of using |enum|s was > to avoid that. Done. http://codereview.chromium.org/1692011/diff/1/4#newcode53 chrome/browser/configuration_policy_pref_store.h:53: DISALLOW_COPY_AND_ASSIGN(ConfigurationPolicyPrefStore); On 2010/04/28 16:01:58, tfarina wrote: > nit: this should be in the end. Done. http://codereview.chromium.org/1692011/diff/1/4#newcode56 chrome/browser/configuration_policy_pref_store.h:56: typedef struct SimpleSettingMapEntry_struct { On 2010/04/28 16:01:58, tfarina wrote: > nit: why not just struct SimpleSettingMapEntry? Done. http://codereview.chromium.org/1692011/diff/1/4#newcode67 chrome/browser/configuration_policy_pref_store.h:67: #endif On 2010/04/28 16:01:58, tfarina wrote: > nit: // CHROME_BROWSER_CONFIGURATION_POLICY_PREF_STORE_H_ > > Also do this in all the header files. Done. http://codereview.chromium.org/1692011/diff/1/6 File chrome/browser/configuration_policy_pref_store_linux.h (right): http://codereview.chromium.org/1692011/diff/1/6#newcode10 chrome/browser/configuration_policy_pref_store_linux.h:10: class LinuxConfigurationPolicyPrefStore : public ConfigurationPolicyPrefStore { On 2010/04/28 17:49:20, mnissler wrote: > Put a comment saying what the class does? Done. http://codereview.chromium.org/1692011/diff/1/6#newcode12 chrome/browser/configuration_policy_pref_store_linux.h:12: virtual PrefReadError ReadPrefs(); On 2010/04/28 16:01:58, tfarina wrote: > add a virtual destructor? Done. http://codereview.chromium.org/1692011/diff/1/6#newcode15 chrome/browser/configuration_policy_pref_store_linux.h:15: #endif On 2010/04/28 16:01:58, tfarina wrote: > nit: // CHROME_BROWSER_CONFIGURATION_POLICY_PREF_STORE_LINUX_H_ Done. http://codereview.chromium.org/1692011/diff/1/7 File chrome/browser/configuration_policy_pref_store_linux_unittest.cc (right): http://codereview.chromium.org/1692011/diff/1/7#newcode7 chrome/browser/configuration_policy_pref_store_linux_unittest.cc:7: class LinuxConfigurationPolicyPrefStoreTest : public PlatformTest { On 2010/04/28 17:49:20, mnissler wrote: > No need for PlatformTest here. I think you can just throw out the linux/mac bits > anyway, we'll add them once we implement them. It's good to keep the windows > bits for reference though. Done. http://codereview.chromium.org/1692011/diff/1/9 File chrome/browser/configuration_policy_pref_store_mac.h (right): http://codereview.chromium.org/1692011/diff/1/9#newcode15 chrome/browser/configuration_policy_pref_store_mac.h:15: #endif On 2010/04/28 16:01:58, tfarina wrote: > nit: // CHROME_BROWSER_CONFIGURATION_POLICY_PREF_STORE_MAC_H_ Done. http://codereview.chromium.org/1692011/diff/1/11 File chrome/browser/configuration_policy_pref_store_unittest.cc (right): http://codereview.chromium.org/1692011/diff/1/11#newcode7 chrome/browser/configuration_policy_pref_store_unittest.cc:7: class ConfigurationPolicyPrefStoreTest : public PlatformTest { On 2010/04/28 17:49:20, mnissler wrote: > Why not testing::Test? Done. http://codereview.chromium.org/1692011/diff/1/13 File chrome/browser/configuration_policy_pref_store_win.h (right): http://codereview.chromium.org/1692011/diff/1/13#newcode15 chrome/browser/configuration_policy_pref_store_win.h:15: On 2010/04/28 16:01:58, tfarina wrote: > nit: remove this blank line Done.
http://codereview.chromium.org/1692011/diff/37001/8003 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/37001/8003#newcode13 chrome/browser/configuration_policy_pref_store.cc:13: : prefs_(new DictionaryValue()) { nit: indent 4 spaces. http://codereview.chromium.org/1692011/diff/37001/8004 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/37001/8004#newcode20 chrome/browser/configuration_policy_pref_store.h:20: virtual DictionaryValue* Prefs() { nit: this should be prefs() { return prefs_.get(); } http://codereview.chromium.org/1692011/diff/37001/8004#newcode52 chrome/browser/configuration_policy_pref_store.h:52: Value::ValueType value_type_; nit: please, could you remove the underscore of the end of these variables? The style for struct variables is different :) http://codereview.chromium.org/1692011/diff/37001/8006 File chrome/browser/configuration_policy_pref_store_win.cc (right): http://codereview.chromium.org/1692011/diff/37001/8006#newcode138 chrome/browser/configuration_policy_pref_store_win.cc:138: { Value::TYPE_BOOLEAN, kSettingHomePage, kPolicyValueNameHomepage } nit: this is ugly :) could you make it more readable? http://codereview.chromium.org/1692011/diff/37001/8006#newcode144 chrome/browser/configuration_policy_pref_store_win.cc:144: sizeof(registry_to_policy_map_) / sizeof(RegistryToPolicyMapEntry); nit: use arraysize here too? http://codereview.chromium.org/1692011/diff/37001/8006#newcode169 chrome/browser/configuration_policy_pref_store_win.cc:169: Value::CreateIntegerValue(int_value)); nit: indent 4 spaces or align with current if possible. http://codereview.chromium.org/1692011/diff/37001/8007 File chrome/browser/configuration_policy_pref_store_win.h (right): http://codereview.chromium.org/1692011/diff/37001/8007#newcode14 chrome/browser/configuration_policy_pref_store_win.h:14: virtual PrefReadError ReadPrefs(); nit: please add a comment like this: // Overridden from ConfigurationPolicyPrefStore: http://codereview.chromium.org/1692011/diff/37001/8007#newcode18 chrome/browser/configuration_policy_pref_store_win.h:18: Value::ValueType value_type_; nit: same thing here; (no need of underscore) http://codereview.chromium.org/1692011/diff/37001/8007#newcode28 chrome/browser/configuration_policy_pref_store_win.h:28: size_t* buffer_size); nit: align size_t* with LPCTSTR
some notes from the first read. http://codereview.chromium.org/1692011/diff/70001/71006 File chrome/browser/configuration_policy_provider_win.cc (right): http://codereview.chromium.org/1692011/diff/70001/71006#newcode39 chrome/browser/configuration_policy_provider_win.cc:39: DCHECK(buffer_size); incorrect indentiation these 3 lines http://codereview.chromium.org/1692011/diff/70001/71006#newcode70 chrome/browser/configuration_policy_provider_win.cc:70: wchar_t* char_buffer = reinterpret_cast<wchar_t*>(buffer.get()); shouldn't that be static_cast? http://codereview.chromium.org/1692011/diff/70001/71006#newcode76 chrome/browser/configuration_policy_provider_win.cc:76: char_buffer_size--; wraparound here if char_buffer_size = 0! I think this'd be more robust if you just checked whether there is a '\0' anywhere in the string and adjust size accordingly. WDYT? http://codereview.chromium.org/1692011/diff/70001/71006#newcode93 chrome/browser/configuration_policy_provider_win.cc:93: DCHECK(key_type == REG_DWORD); Do we really want to assert when there's a broken entry in the registry? http://codereview.chromium.org/1692011/diff/70001/71009 File chrome/browser/dummy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/70001/71009#newcode1 chrome/browser/dummy_pref_store.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. I cannot see where you use DummyPrefStore.
please review in preparation for commit. thanks! http://codereview.chromium.org/1692011/diff/37001/8003 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/37001/8003#newcode13 chrome/browser/configuration_policy_pref_store.cc:13: : prefs_(new DictionaryValue()) { On 2010/04/30 16:39:37, tfarina wrote: > nit: indent 4 spaces. Done. http://codereview.chromium.org/1692011/diff/37001/8004 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/37001/8004#newcode20 chrome/browser/configuration_policy_pref_store.h:20: virtual DictionaryValue* Prefs() { On 2010/04/30 16:39:37, tfarina wrote: > nit: this should be prefs() { return prefs_.get(); } Done, required changes in abstract superclass and its impls. http://codereview.chromium.org/1692011/diff/37001/8004#newcode52 chrome/browser/configuration_policy_pref_store.h:52: Value::ValueType value_type_; On 2010/04/30 16:39:37, tfarina wrote: > nit: please, could you remove the underscore of the end of these variables? The > style for struct variables is different :) Done. http://codereview.chromium.org/1692011/diff/37001/8006 File chrome/browser/configuration_policy_pref_store_win.cc (right): http://codereview.chromium.org/1692011/diff/37001/8006#newcode138 chrome/browser/configuration_policy_pref_store_win.cc:138: { Value::TYPE_BOOLEAN, kSettingHomePage, kPolicyValueNameHomepage } On 2010/04/30 16:39:37, tfarina wrote: > nit: this is ugly :) > > could you make it more readable? Done. http://codereview.chromium.org/1692011/diff/37001/8006#newcode144 chrome/browser/configuration_policy_pref_store_win.cc:144: sizeof(registry_to_policy_map_) / sizeof(RegistryToPolicyMapEntry); On 2010/04/30 16:39:37, tfarina wrote: > nit: use arraysize here too? Done. http://codereview.chromium.org/1692011/diff/37001/8006#newcode169 chrome/browser/configuration_policy_pref_store_win.cc:169: Value::CreateIntegerValue(int_value)); On 2010/04/30 16:39:37, tfarina wrote: > nit: indent 4 spaces or align with current if possible. Done. http://codereview.chromium.org/1692011/diff/37001/8007 File chrome/browser/configuration_policy_pref_store_win.h (right): http://codereview.chromium.org/1692011/diff/37001/8007#newcode14 chrome/browser/configuration_policy_pref_store_win.h:14: virtual PrefReadError ReadPrefs(); On 2010/04/30 16:39:37, tfarina wrote: > nit: please add a comment like this: > // Overridden from ConfigurationPolicyPrefStore: Done. http://codereview.chromium.org/1692011/diff/37001/8007#newcode18 chrome/browser/configuration_policy_pref_store_win.h:18: Value::ValueType value_type_; On 2010/04/30 16:39:37, tfarina wrote: > nit: same thing here; (no need of underscore) Done. http://codereview.chromium.org/1692011/diff/37001/8007#newcode28 chrome/browser/configuration_policy_pref_store_win.h:28: size_t* buffer_size); On 2010/04/30 16:39:37, tfarina wrote: > nit: align size_t* with LPCTSTR Done. http://codereview.chromium.org/1692011/diff/70001/71006 File chrome/browser/configuration_policy_provider_win.cc (right): http://codereview.chromium.org/1692011/diff/70001/71006#newcode39 chrome/browser/configuration_policy_provider_win.cc:39: DCHECK(buffer_size); On 2010/05/05 15:09:32, mnissler wrote: > incorrect indentiation these 3 lines Done. http://codereview.chromium.org/1692011/diff/70001/71006#newcode70 chrome/browser/configuration_policy_provider_win.cc:70: wchar_t* char_buffer = reinterpret_cast<wchar_t*>(buffer.get()); On 2010/05/05 15:09:32, mnissler wrote: > shouldn't that be static_cast? No, reinterpret_cast is correct. http://codereview.chromium.org/1692011/diff/70001/71006#newcode76 chrome/browser/configuration_policy_provider_win.cc:76: char_buffer_size--; On 2010/05/05 15:09:32, mnissler wrote: > wraparound here if char_buffer_size = 0! I think this'd be more robust if you > just checked whether there is a '\0' anywhere in the string and adjust size > accordingly. WDYT? Done. http://codereview.chromium.org/1692011/diff/70001/71006#newcode93 chrome/browser/configuration_policy_provider_win.cc:93: DCHECK(key_type == REG_DWORD); On 2010/05/05 15:09:32, mnissler wrote: > Do we really want to assert when there's a broken entry in the registry? Done. http://codereview.chromium.org/1692011/diff/70001/71009 File chrome/browser/dummy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/70001/71009#newcode1 chrome/browser/dummy_pref_store.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/05/05 15:09:32, mnissler wrote: > I cannot see where you use DummyPrefStore. I didn't actually add this file, it was already in svn, so I don't really think I should remove it.
Enjoy! - Pam http://codereview.chromium.org/1692011/diff/107001/105002 File chrome/browser/configuration_policy_builder.h (right): http://codereview.chromium.org/1692011/diff/107001/105002#newcode5 chrome/browser/configuration_policy_builder.h:5: #ifndef CHROME_BROWSER_CONFIGURATION_POLICY_H_ ..._POLICY_BUILDER_H_ http://codereview.chromium.org/1692011/diff/107001/105002#newcode9 chrome/browser/configuration_policy_builder.h:9: I'm a strong proponent of file- or class-level comments describing at a high level what the thing does, what it's used for, who is expected to create or own one, etc. Same for any of the other new class .h files that don't have one. http://codereview.chromium.org/1692011/diff/107001/105002#newcode22 chrome/browser/configuration_policy_builder.h:22: virtual void ConfigureSetting(SettingType setting, Value* value) = 0; Please expand the comment. What does this method do? http://codereview.chromium.org/1692011/diff/107001/105003 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/107001/105003#newcode11 chrome/browser/configuration_policy_pref_store.cc:11: #include "googleurl/src/gurl.h" Unused? http://codereview.chromium.org/1692011/diff/107001/105003#newcode17 chrome/browser/configuration_policy_pref_store.cc:17: return PrefStore::PREF_READ_ERROR_NONE; Surely this can fail sometimes. Should ConfigurePolicy() return a PrefReadError when it can't access whatever it's trying to read the policy from? http://codereview.chromium.org/1692011/diff/107001/105003#newcode31 chrome/browser/configuration_policy_pref_store.cc:31: }; This initialization should be up at the top, separated from the method definitions. http://codereview.chromium.org/1692011/diff/107001/105004 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/107001/105004#newcode17 chrome/browser/configuration_policy_pref_store.h:17: class ConfigurationPolicyPrefStoreTest; Unless I'm missing something, you don't need to forward-declare this class or make it a friend class. The FRIEND_TEST() macro should take care of it. http://codereview.chromium.org/1692011/diff/107001/105004#newcode26 chrome/browser/configuration_policy_pref_store.h:26: // x-platform. Nit: please write out "cross-platform". Some non-native speakers might not understand the shorthand. http://codereview.chromium.org/1692011/diff/107001/105004#newcode35 chrome/browser/configuration_policy_pref_store.h:35: // Overridden Please clarify. Overridden here from PrefStore, or should be overridden by platform-specific subclasses? Same for other "Overridden" comments below. Since this inherits from only one class, it's probably safe to simply remove the comment if what you mean is "overridden from the default version in the parent class". http://codereview.chromium.org/1692011/diff/107001/105004#newcode62 chrome/browser/configuration_policy_pref_store.h:62: // indivual policy decisions into specific preferences typo "individual"; please add a . at the end of the comment http://codereview.chromium.org/1692011/diff/107001/105004#newcode63 chrome/browser/configuration_policy_pref_store.h:63: class PolicyBuilder : public ConfigurationPolicyBuilder { Calling this a PolicyBuilder seems like a misnomer. If I understand the comment correctly, it's not so much building a policy as deconstructing a configuration policy into preferences, no? http://codereview.chromium.org/1692011/diff/107001/105004#newcode79 chrome/browser/configuration_policy_pref_store.h:79: struct SimpleSettingMapEntry { Please add a comment that this handles all the settings that map directly to preferences. http://codereview.chromium.org/1692011/diff/107001/105004#newcode85 chrome/browser/configuration_policy_pref_store.h:85: static const SimpleSettingMapEntry simple_settings_map_[]; No real objection to an array, although my inclination would be to use a vector. Saves some risk of out-of-bounds errors and is arguably clearer to use, at the expense of more complex initialization. Also a bit more memory, although I think <vector> is pretty efficient. Up to you. http://codereview.chromium.org/1692011/diff/107001/105005 File chrome/browser/configuration_policy_pref_store_unittest.cc (right): http://codereview.chromium.org/1692011/diff/107001/105005#newcode10 chrome/browser/configuration_policy_pref_store_unittest.cc:10: class ConfigurationPolicyPrefStoreTest : public testing::Test { You only need to create a Test class and use TEST_F() if you have a SetUp or TearDown method to be called before/after each test. Otherwise you can just drop this and use TEST(). http://codereview.chromium.org/1692011/diff/107001/105005#newcode20 chrome/browser/configuration_policy_pref_store_unittest.cc:20: EXPECT_EQ(result, L""); I don't understand this test. kHomePageIsNewTabPage is a boolean, but it's being retrieved as a string, which is then empty? What is it supposed to be testing? http://codereview.chromium.org/1692011/diff/107001/105005#newcode24 chrome/browser/configuration_policy_pref_store_unittest.cc:24: ConfigurationPolicyPrefStore::PolicyBuilder builder; It would be good to check the starting value and make sure it's something else before calling ConfigureSetting to change it. Oh, I see. I think you mean kHomePage in that GetString() call in TestSettingHomePageDefault() above. And in that case I'd combine that test with this one, since they're not entirely independent conditions. http://codereview.chromium.org/1692011/diff/107001/105005#newcode40 chrome/browser/configuration_policy_pref_store_unittest.cc:40: TEST_F(ConfigurationPolicyPrefStoreTest, TestSettingHomepageIsNewTabPageTrue) { I often think people try to put too much into one unit test, but in this case I'd say all three tests for HomepageIsNewTabPage should be merged into one TEST() case. Making sure it starts out with the right default and making sure you can set it both directions are closely enough related. http://codereview.chromium.org/1692011/diff/107001/105005#newcode67 chrome/browser/configuration_policy_pref_store_unittest.cc:67: TEST_F(ConfigurationPolicyPrefStoreTest, TestSettingCookiesEnabledOverride) { Yeah, here too. http://codereview.chromium.org/1692011/diff/107001/105006 File chrome/browser/configuration_policy_provider.h (right): http://codereview.chromium.org/1692011/diff/107001/105006#newcode19 chrome/browser/configuration_policy_provider.h:19: virtual void ConfigurePolicy(ConfigurationPolicyBuilder* policy) = 0; The naming here is starting to become confusing. Intuitively, it seems like a PolicyBuilder shouldn't be a |policy|. That is, should that class be called simply a ConfigurationPolicy? Picking the "main" one of these header files and adding a note about the terminology would also help. Configuration, Configure, Policy, Setting, Builder, Provider... I'm lost.
http://codereview.chromium.org/1692011/diff/107001/105004 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/107001/105004#newcode35 chrome/browser/configuration_policy_pref_store.h:35: // Overridden I've mostly seen comments of the form "<Base class> methods", i.e. "PrefStore methods" here.
http://codereview.chromium.org/1692011/diff/107001/105004 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/107001/105004#newcode35 chrome/browser/configuration_policy_pref_store.h:35: // Overridden On 2010/05/07 12:42:40, Bernhard Bauer wrote: > I've mostly seen comments of the form "<Base class> methods", i.e. "PrefStore > methods" here. (There is 4 forms of this): // Foo overrides. // Overridden from Foo: // Foo implementation(s): // Foo method(s): :)
http://codereview.chromium.org/1692011/diff/107001/105004 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/107001/105004#newcode32 chrome/browser/configuration_policy_pref_store.h:32: : provider_(provider) {} nit: I know this is just one variable. But I would prefer to move initializations to .cc file (to avoid polution of the .h file). http://codereview.chromium.org/1692011/diff/107001/105004#newcode39 chrome/browser/configuration_policy_pref_store.h:39: virtual DictionaryValue* prefs() { nit: does this fit into one line? http://codereview.chromium.org/1692011/diff/107001/105004#newcode44 chrome/browser/configuration_policy_pref_store.h:44: ConfigurationPolicyProvider* provider_; nit: this should be moved to the end, near to |prefs_|. http://codereview.chromium.org/1692011/diff/107001/105004#newcode67 chrome/browser/configuration_policy_pref_store.h:67: // Overridden nit: please add blank line above. http://codereview.chromium.org/1692011/diff/107001/105004#newcode70 chrome/browser/configuration_policy_pref_store.h:70: DictionaryValue* ReleasePrefs() { nit: would be possible to inline like below? http://codereview.chromium.org/1692011/diff/107001/105006 File chrome/browser/configuration_policy_provider.h (right): http://codereview.chromium.org/1692011/diff/107001/105006#newcode8 chrome/browser/configuration_policy_provider.h:8: #include "base/values.h" nit: is this necessary? http://codereview.chromium.org/1692011/diff/107001/105010 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/1692011/diff/107001/105010#newcode1 chrome/browser/pref_service.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. nit: 2010
I hope I got all of the feedback address, either in a comment or code. Unfortunately, there were a whole lot changes, so you may have to go over a good portion of the code again from scratch... Thanks! http://codereview.chromium.org/1692011/diff/107001/105002 File chrome/browser/configuration_policy_builder.h (right): http://codereview.chromium.org/1692011/diff/107001/105002#newcode5 chrome/browser/configuration_policy_builder.h:5: #ifndef CHROME_BROWSER_CONFIGURATION_POLICY_H_ On 2010/05/07 12:30:38, Pam wrote: > ..._POLICY_BUILDER_H_ Done. http://codereview.chromium.org/1692011/diff/107001/105002#newcode9 chrome/browser/configuration_policy_builder.h:9: On 2010/05/07 12:30:38, Pam wrote: > I'm a strong proponent of file- or class-level comments describing at a high > level what the thing does, what it's used for, who is expected to create or own > one, etc. > > Same for any of the other new class .h files that don't have one. Done. http://codereview.chromium.org/1692011/diff/107001/105002#newcode22 chrome/browser/configuration_policy_builder.h:22: virtual void ConfigureSetting(SettingType setting, Value* value) = 0; On 2010/05/07 12:30:38, Pam wrote: > Please expand the comment. What does this method do? Done. http://codereview.chromium.org/1692011/diff/107001/105003 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/107001/105003#newcode11 chrome/browser/configuration_policy_pref_store.cc:11: #include "googleurl/src/gurl.h" On 2010/05/07 12:30:38, Pam wrote: > Unused? Done. http://codereview.chromium.org/1692011/diff/107001/105003#newcode17 chrome/browser/configuration_policy_pref_store.cc:17: return PrefStore::PREF_READ_ERROR_NONE; On 2010/05/07 12:30:38, Pam wrote: > Surely this can fail sometimes. Should ConfigurePolicy() return a PrefReadError > when it can't access whatever it's trying to read the policy from? Done. http://codereview.chromium.org/1692011/diff/107001/105003#newcode31 chrome/browser/configuration_policy_pref_store.cc:31: }; On 2010/05/07 12:30:38, Pam wrote: > This initialization should be up at the top, separated from the method > definitions. I don't think so, it belongs to the PolicyBuilder class, shouldn't it be grouped with it's methods? http://codereview.chromium.org/1692011/diff/107001/105004 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/107001/105004#newcode17 chrome/browser/configuration_policy_pref_store.h:17: class ConfigurationPolicyPrefStoreTest; On 2010/05/07 12:30:38, Pam wrote: > Unless I'm missing something, you don't need to forward-declare this class or > make it a friend class. The FRIEND_TEST() macro should take care of it. Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode26 chrome/browser/configuration_policy_pref_store.h:26: // x-platform. On 2010/05/07 12:30:38, Pam wrote: > Nit: please write out "cross-platform". Some non-native speakers might not > understand the shorthand. Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode32 chrome/browser/configuration_policy_pref_store.h:32: : provider_(provider) {} On 2010/05/07 15:36:18, tfarina wrote: > nit: I know this is just one variable. But I would prefer to move > initializations to .cc file (to avoid polution of the .h file). Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode35 chrome/browser/configuration_policy_pref_store.h:35: // Overridden On 2010/05/07 12:42:40, Bernhard Bauer wrote: > I've mostly seen comments of the form "<Base class> methods", i.e. "PrefStore > methods" here. Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode39 chrome/browser/configuration_policy_pref_store.h:39: virtual DictionaryValue* prefs() { On 2010/05/07 15:36:18, tfarina wrote: > nit: does this fit into one line? Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode44 chrome/browser/configuration_policy_pref_store.h:44: ConfigurationPolicyProvider* provider_; On 2010/05/07 15:36:18, tfarina wrote: > nit: this should be moved to the end, near to |prefs_|. Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode62 chrome/browser/configuration_policy_pref_store.h:62: // indivual policy decisions into specific preferences On 2010/05/07 12:30:38, Pam wrote: > typo "individual"; please add a . at the end of the comment Done... actually class removed. http://codereview.chromium.org/1692011/diff/107001/105004#newcode63 chrome/browser/configuration_policy_pref_store.h:63: class PolicyBuilder : public ConfigurationPolicyBuilder { On 2010/05/07 12:30:38, Pam wrote: > Calling this a PolicyBuilder seems like a misnomer. If I understand the comment > correctly, it's not so much building a policy as deconstructing a configuration > policy into preferences, no? Fixed as discussed. http://codereview.chromium.org/1692011/diff/107001/105004#newcode67 chrome/browser/configuration_policy_pref_store.h:67: // Overridden On 2010/05/07 15:36:18, tfarina wrote: > nit: please add blank line above. Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode70 chrome/browser/configuration_policy_pref_store.h:70: DictionaryValue* ReleasePrefs() { On 2010/05/07 15:36:18, tfarina wrote: > nit: would be possible to inline like below? Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode79 chrome/browser/configuration_policy_pref_store.h:79: struct SimpleSettingMapEntry { On 2010/05/07 12:30:38, Pam wrote: > Please add a comment that this handles all the settings that map directly to > preferences. Done. http://codereview.chromium.org/1692011/diff/107001/105004#newcode85 chrome/browser/configuration_policy_pref_store.h:85: static const SimpleSettingMapEntry simple_settings_map_[]; On 2010/05/07 12:30:38, Pam wrote: > No real objection to an array, although my inclination would be to use a vector. > Saves some risk of out-of-bounds errors and is arguably clearer to use, at the > expense of more complex initialization. Also a bit more memory, although I think > <vector> is pretty efficient. Up to you. The array code is much conciser as it is, I will use that. http://codereview.chromium.org/1692011/diff/107001/105005 File chrome/browser/configuration_policy_pref_store_unittest.cc (right): http://codereview.chromium.org/1692011/diff/107001/105005#newcode10 chrome/browser/configuration_policy_pref_store_unittest.cc:10: class ConfigurationPolicyPrefStoreTest : public testing::Test { On 2010/05/07 12:30:38, Pam wrote: > You only need to create a Test class and use TEST_F() if you have a SetUp or > TearDown method to be called before/after each test. Otherwise you can just drop > this and use TEST(). Done. http://codereview.chromium.org/1692011/diff/107001/105005#newcode20 chrome/browser/configuration_policy_pref_store_unittest.cc:20: EXPECT_EQ(result, L""); On 2010/05/07 12:30:38, Pam wrote: > I don't understand this test. kHomePageIsNewTabPage is a boolean, but it's being > retrieved as a string, which is then empty? What is it supposed to be testing? Typo in the test. fixed. http://codereview.chromium.org/1692011/diff/107001/105005#newcode24 chrome/browser/configuration_policy_pref_store_unittest.cc:24: ConfigurationPolicyPrefStore::PolicyBuilder builder; On 2010/05/07 12:30:38, Pam wrote: > It would be good to check the starting value and make sure it's something else > before calling ConfigureSetting to change it. > > Oh, I see. I think you mean kHomePage in that GetString() call in > TestSettingHomePageDefault() above. And in that case I'd combine that test with > this one, since they're not entirely independent conditions. I disagree. One test should test one piece of functionality, not two--if possible. More smaller tests are better, IMO, especially if there is a single test that tests prerequisites once rather than again and again in every tests or by artificially merging one test. That's why there's a separate test to test the default. http://codereview.chromium.org/1692011/diff/107001/105005#newcode40 chrome/browser/configuration_policy_pref_store_unittest.cc:40: TEST_F(ConfigurationPolicyPrefStoreTest, TestSettingHomepageIsNewTabPageTrue) { On 2010/05/07 12:30:38, Pam wrote: > I often think people try to put too much into one unit test, but in this case > I'd say all three tests for HomepageIsNewTabPage should be merged into one > TEST() case. Making sure it starts out with the right default and making sure > you can set it both directions are closely enough related. I'd like to keep them separate, see above. http://codereview.chromium.org/1692011/diff/107001/105005#newcode67 chrome/browser/configuration_policy_pref_store_unittest.cc:67: TEST_F(ConfigurationPolicyPrefStoreTest, TestSettingCookiesEnabledOverride) { On 2010/05/07 12:30:38, Pam wrote: > Yeah, here too. I'd like to keep them separate, see above. http://codereview.chromium.org/1692011/diff/107001/105006 File chrome/browser/configuration_policy_provider.h (right): http://codereview.chromium.org/1692011/diff/107001/105006#newcode8 chrome/browser/configuration_policy_provider.h:8: #include "base/values.h" On 2010/05/07 15:36:18, tfarina wrote: > nit: is this necessary? Done. http://codereview.chromium.org/1692011/diff/107001/105006#newcode19 chrome/browser/configuration_policy_provider.h:19: virtual void ConfigurePolicy(ConfigurationPolicyBuilder* policy) = 0; On 2010/05/07 12:30:38, Pam wrote: > The naming here is starting to become confusing. Intuitively, it seems like a > PolicyBuilder shouldn't be a |policy|. That is, should that class be called > simply a ConfigurationPolicy? > > Picking the "main" one of these header files and adding a note about the > terminology would also help. Configuration, Configure, Policy, Setting, Builder, > Provider... I'm lost. Done. http://codereview.chromium.org/1692011/diff/107001/105010 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/1692011/diff/107001/105010#newcode1 chrome/browser/pref_service.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. On 2010/05/07 15:36:18, tfarina wrote: > nit: 2010 Done.
http://codereview.chromium.org/1692011/diff/116001/117001 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/116001/117001#newcode29 chrome/browser/configuration_policy_pref_store.cc:29: { nit: put the { in the of DictionaryValue()) http://codereview.chromium.org/1692011/diff/116001/117001#newcode33 chrome/browser/configuration_policy_pref_store.cc:33: if (!provider_->Provide(this)) { nit: no need of {} for single line statements. also this could be either: if (!provider_->Provide(this)) return PrefStore::PREF_READ_ERROR_OTHER; return PrefStore::PREF_READ_ERROR_NONE; Or: return (!provider_->Provider(this)) ? PrefStore::PREF_READ_ERROR_OTHER : PrefStore::PREF_READ_ERROR_NONE; I'd stick with the ternary one. http://codereview.chromium.org/1692011/diff/116001/117002 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/116001/117002#newcode27 chrome/browser/configuration_policy_pref_store.h:27: // PrefStore methods(s):. nit: "methods(s):." :) write it as: PrefStore methods: Same thing below for Apply.
Mostly nits, a few small things to argue about. :) - Pam http://codereview.chromium.org/1692011/diff/118002/121001 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/118002/121001#newcode23 chrome/browser/configuration_policy_pref_store.cc:23: }; These ought to fit on one line each now. http://codereview.chromium.org/1692011/diff/118002/121001#newcode33 chrome/browser/configuration_policy_pref_store.cc:33: PrefStore::PREF_READ_ERROR_OTHER; Some people really love that ternary operator. FWIW, I usually prefer to write it out in three lines, especially for a return. One advantage, for instance, is it allows easier debugging. Up to you; I just wanted to offer a counter to Thiago's recommendation. :) http://codereview.chromium.org/1692011/diff/118002/121001#newcode38 chrome/browser/configuration_policy_pref_store.cc:38: Value* value) { Does this fit on one line now? The first param looks like it does, at least, and then the second can be aligned with it. http://codereview.chromium.org/1692011/diff/118002/121002 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/118002/121002#newcode45 chrome/browser/configuration_policy_pref_store.h:45: TestSettingCookiesEnabledOverride); With no more ConfigurationPolicyPrefStoreTest class, these shouldn't be needed any longer. http://codereview.chromium.org/1692011/diff/118002/121002#newcode49 chrome/browser/configuration_policy_pref_store.h:49: // has an entry |simple_policy_map_| with the following type. ...entry in... http://codereview.chromium.org/1692011/diff/118002/121003 File chrome/browser/configuration_policy_pref_store_unittest.cc (right): http://codereview.chromium.org/1692011/diff/118002/121003#newcode13 chrome/browser/configuration_policy_pref_store_unittest.cc:13: store.prefs()->GetString(prefs::kHomePageIsNewTabPage, &result); Typo not yet fixed (should be kHomePage, not ...IsNewTabPage). http://codereview.chromium.org/1692011/diff/118002/121003#newcode42 chrome/browser/configuration_policy_pref_store_unittest.cc:42: TEST(ConfigurationPolicyPrefStoreTest, TestSettingHomepageIsNewTabPageFalse) { OK, I'm not going to argue about combining the default-value and setting-a-value tests; although it's at the end of the continuum of what's currently in our codebase, it's a valid stylistic choice. But I'm going to push back about having separate tests for setting the same policy to true and false. Actually, testing setting false when the default value is false doesn't verify what it claims to test. You need to first set true, then set false in the same test. But since setting true and setting false are running through the same code path, I'd also be fine with simply dropping TestSettingHomepageIsNewTabPageFalse and explicitly setting the policy to the opposite of the default, just in case the default should change. http://codereview.chromium.org/1692011/diff/118002/121004 File chrome/browser/configuration_policy_provider.h (right): http://codereview.chromium.org/1692011/diff/118002/121004#newcode12 chrome/browser/configuration_policy_provider.h:12: // etc.) should implement a subclass this class. ...subclass of... http://codereview.chromium.org/1692011/diff/118002/121004#newcode22 chrome/browser/configuration_policy_provider.h:22: // calls back into the provided ConfigurationPolicStore object. ...PolicyStore... I'm having trouble understanding the last clause of this comment ("which it does..."). Can you clarify? http://codereview.chromium.org/1692011/diff/118002/121004#newcode24 chrome/browser/configuration_policy_provider.h:24: virtual bool Provide(ConfigurationPolicyStore* store) = 0; Rather than "configured", would "retrieved" or "provided" be accurate? You haven't used "configured" to describe any of the other steps that I've noticed, so it's not 100% clear what it means. http://codereview.chromium.org/1692011/diff/118002/121005 File chrome/browser/configuration_policy_store.h (right): http://codereview.chromium.org/1692011/diff/118002/121005#newcode24 chrome/browser/configuration_policy_store.h:24: // through a call to |ConfigureSetting|. Comment outdated (should be "a call to |Apply|")?
Dear Chromium Gods: I have been penitent. I have been diffident. Oh, how I have sacrificed. Will you smile upon my CL and bless it with the grace of LGTM? - your humble servant, Danno http://codereview.chromium.org/1692011/diff/116001/117001 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/116001/117001#newcode29 chrome/browser/configuration_policy_pref_store.cc:29: { On 2010/05/07 16:21:05, tfarina wrote: > nit: put the { in the of DictionaryValue()) Done. http://codereview.chromium.org/1692011/diff/116001/117001#newcode33 chrome/browser/configuration_policy_pref_store.cc:33: if (!provider_->Provide(this)) { On 2010/05/07 16:21:05, tfarina wrote: > nit: no need of {} for single line statements. > > also this could be either: > > if (!provider_->Provide(this)) > return PrefStore::PREF_READ_ERROR_OTHER; > return PrefStore::PREF_READ_ERROR_NONE; > > Or: > > return (!provider_->Provider(this)) ? PrefStore::PREF_READ_ERROR_OTHER : > PrefStore::PREF_READ_ERROR_NONE; > > I'd stick with the ternary one. Done. http://codereview.chromium.org/1692011/diff/116001/117002 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/116001/117002#newcode27 chrome/browser/configuration_policy_pref_store.h:27: // PrefStore methods(s):. On 2010/05/07 16:21:05, tfarina wrote: > nit: "methods(s):." :) > > write it as: > > PrefStore methods: > > Same thing below for Apply. Done. http://codereview.chromium.org/1692011/diff/118002/121001 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/118002/121001#newcode23 chrome/browser/configuration_policy_pref_store.cc:23: }; On 2010/05/10 13:31:57, Pam wrote: > These ought to fit on one line each now. Done. http://codereview.chromium.org/1692011/diff/118002/121001#newcode33 chrome/browser/configuration_policy_pref_store.cc:33: PrefStore::PREF_READ_ERROR_OTHER; On 2010/05/10 13:31:57, Pam wrote: > Some people really love that ternary operator. FWIW, I usually prefer to write > it out in three lines, especially for a return. One advantage, for instance, is > it allows easier debugging. > > Up to you; I just wanted to offer a counter to Thiago's recommendation. :) I'll keep the ternary :-) http://codereview.chromium.org/1692011/diff/118002/121001#newcode38 chrome/browser/configuration_policy_pref_store.cc:38: Value* value) { On 2010/05/10 13:31:57, Pam wrote: > Does this fit on one line now? The first param looks like it does, at least, and > then the second can be aligned with it. Done. http://codereview.chromium.org/1692011/diff/118002/121002 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/118002/121002#newcode45 chrome/browser/configuration_policy_pref_store.h:45: TestSettingCookiesEnabledOverride); On 2010/05/10 13:31:57, Pam wrote: > With no more ConfigurationPolicyPrefStoreTest class, these shouldn't be needed > any longer. yes, they are needed, because a private method is still called. without, the tests don't compile. However, the friend class ConfiguraitonPolicyPrefStoreTest can be removed. Done. http://codereview.chromium.org/1692011/diff/118002/121002#newcode49 chrome/browser/configuration_policy_pref_store.h:49: // has an entry |simple_policy_map_| with the following type. On 2010/05/10 13:31:57, Pam wrote: > ...entry in... Done. http://codereview.chromium.org/1692011/diff/118002/121003 File chrome/browser/configuration_policy_pref_store_unittest.cc (right): http://codereview.chromium.org/1692011/diff/118002/121003#newcode13 chrome/browser/configuration_policy_pref_store_unittest.cc:13: store.prefs()->GetString(prefs::kHomePageIsNewTabPage, &result); On 2010/05/10 13:31:57, Pam wrote: > Typo not yet fixed (should be kHomePage, not ...IsNewTabPage). Done. http://codereview.chromium.org/1692011/diff/118002/121003#newcode42 chrome/browser/configuration_policy_pref_store_unittest.cc:42: TEST(ConfigurationPolicyPrefStoreTest, TestSettingHomepageIsNewTabPageFalse) { On 2010/05/10 13:31:57, Pam wrote: > OK, I'm not going to argue about combining the default-value and setting-a-value > tests; although it's at the end of the continuum of what's currently in our > codebase, it's a valid stylistic choice. But I'm going to push back about having > separate tests for setting the same policy to true and false. > > Actually, testing setting false when the default value is false doesn't verify > what it claims to test. You need to first set true, then set false in the same > test. But since setting true and setting false are running through the same code > path, I'd also be fine with simply dropping TestSettingHomepageIsNewTabPageFalse > and explicitly setting the policy to the opposite of the default, just in case > the default should change. Done. http://codereview.chromium.org/1692011/diff/118002/121004 File chrome/browser/configuration_policy_provider.h (right): http://codereview.chromium.org/1692011/diff/118002/121004#newcode12 chrome/browser/configuration_policy_provider.h:12: // etc.) should implement a subclass this class. On 2010/05/10 13:31:57, Pam wrote: > ...subclass of... Done. http://codereview.chromium.org/1692011/diff/118002/121004#newcode22 chrome/browser/configuration_policy_provider.h:22: // calls back into the provided ConfigurationPolicStore object. On 2010/05/10 13:31:57, Pam wrote: > ...PolicyStore... > > I'm having trouble understanding the last clause of this comment ("which it > does..."). Can you clarify? Done. http://codereview.chromium.org/1692011/diff/118002/121004#newcode24 chrome/browser/configuration_policy_provider.h:24: virtual bool Provide(ConfigurationPolicyStore* store) = 0; On 2010/05/10 13:31:57, Pam wrote: > Rather than "configured", would "retrieved" or "provided" be accurate? You > haven't used "configured" to describe any of the other steps that I've noticed, > so it's not 100% clear what it means. Done. http://codereview.chromium.org/1692011/diff/118002/121005 File chrome/browser/configuration_policy_store.h (right): http://codereview.chromium.org/1692011/diff/118002/121005#newcode24 chrome/browser/configuration_policy_store.h:24: // through a call to |ConfigureSetting|. On 2010/05/10 13:31:57, Pam wrote: > Comment outdated (should be "a call to |Apply|")? Done.
LGTM once the trybots pass, with one tiny nit. - Pam http://codereview.chromium.org/1692011/diff/112003/129001 File chrome/browser/configuration_policy_pref_store.cc (right): http://codereview.chromium.org/1692011/diff/112003/129001#newcode16 chrome/browser/configuration_policy_pref_store.cc:16: prefs::kHomePageIsNewTabPage }, Continuation lines generally get a 4-space indent in our C++ code.
http://codereview.chromium.org/1692011/diff/112003/129002 File chrome/browser/configuration_policy_pref_store.h (right): http://codereview.chromium.org/1692011/diff/112003/129002#newcode50 chrome/browser/configuration_policy_pref_store.h:50: const wchar_t* preference_path; nit: please, could you add a TODO(tfarina), to convert this to FilePath? |
