Cleanup and style guideline conformance for policy implementation

Many small changes to improve readability of configuration policy code.

BUG=none
TEST=none

[danno, 2010-10-14 11:49:22]

Please review for C++ readability.

Also please note that the code review tool for Chromium is fully public, so do
not discuss anything in this review that is not appropriate for the open-source
community (i.e. no Google internal stuff).

If you have questions about Chromium development machinery, like how to use the
code review tool or how to get blame annotations for source files, please feel
free to ask me.

Thanks!
Danno

[nadav, 2010-10-14 16:56:39]

Hi Daniel,

I made a first pass through configuration_policy_pref_store.{h,cc}. The code is
very readable, but I did find some issues I would like you to address. Please
see my inline comments. Also, for readability reviews I normally comment on any
particular issue just once, even if it is a recurring issue. I would therefore
ask that you make a pass through the whole CL with an eye towards the issues I
flagged and fix all occurrences. Part of the goal of a readability review is to
get the reviewee to develop an eye for readability issues, and so I believe such
an exercise is very valuable.

Thanks
Nadav

http://codereview.chromium.org/3774003/diff/2001/3001
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/3774003/diff/2001/3001#newcode41
chrome/browser/policy/configuration_policy_pref_store.cc:41: virtual
~ConfigurationPolicyProviderKeeper() {}
It's be nice to have consistency with how you write empty function bodies. You
use {} here, but { } (with a space) in the .h file. My personal preference would
be {}, but you can use either as long as you are consistent.

http://codereview.chromium.org/3774003/diff/2001/3001#newcode65
chrome/browser/policy/configuration_policy_pref_store.cc:65: return new
ConfigDirPolicyProvider(policy_map,
argument alignment
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Calls#Functi...

http://codereview.chromium.org/3774003/diff/2001/3001#newcode259
chrome/browser/policy/configuration_policy_pref_store.cc:259: bool success =
(provider_ == NULL || provider_->Provide(this));
const bool?
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Use_of_const#Use_of_c...

http://codereview.chromium.org/3774003/diff/2001/3001#newcode316
chrome/browser/policy/configuration_policy_pref_store.cc:316: bool found_one =
false;
Nit pick: found_one is a misleading name. Maybe call it found_any? (since you
may find more than one - you don't stop after the first)

http://codereview.chromium.org/3774003/diff/2001/3001#newcode327
chrome/browser/policy/configuration_policy_pref_store.cc:327: for (const
PolicyToPreferenceMapEntry* current = map;
We typically prefer using ints in loops. It makes the for() loop a lot easier to
read. I doubt it makes any performance difference (and not clear to me this code
is performance critical). Any reason not to loop with in index?

http://codereview.chromium.org/3774003/diff/2001/3002
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/3774003/diff/2001/3002#newcode26
chrome/browser/policy/configuration_policy_pref_store.h:26: public
ConfigurationPolicyStore {
Multiple inheritance is allowed in only very limited cases, and this isn't one
of them
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Multiple_Inheritance#...

http://codereview.chromium.org/3774003/diff/2001/3002#newcode30
chrome/browser/policy/configuration_policy_pref_store.h:30:
ConfigurationPolicyPrefStore(const CommandLine* command_line,
Why const CommandLine* and not const CommandLine&?

http://codereview.chromium.org/3774003/diff/2001/3002#newcode61
chrome/browser/policy/configuration_policy_pref_store.h:61: static const
PolicyToPreferenceMapEntry simple_policy_map_[];
Wrong naming convention for a static const
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Constant_Names#Consta...

http://codereview.chromium.org/3774003/diff/2001/3002#newcode90
chrome/browser/policy/configuration_policy_pref_store.h:90: const
PolicyToPreferenceMapEntry* FindPolicyInMap(PolicyType policy,
bad indentation
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Declarations...
(all arguments are always aligned the same!)

http://codereview.chromium.org/3774003/diff/2001/3002#newcode120
chrome/browser/policy/configuration_policy_pref_store.h:120: void
EnsureStringPrefExists(const std::string& path);
why the std:: here?

http://codereview.chromium.org/3774003/diff/2001/3002#newcode134
chrome/browser/policy/configuration_policy_pref_store.h:134: 
Please remove these blank lines

[danno, 2010-10-18 13:14:23]

Thanks for the review feedback. I made a pass over the code to fix all of the
occurrences of the issues that you identified.

I also moved some constants and an enumeration out of the
ConfigurationPolicyPrefStoreInterface into the top-level policy namespace to
shorten the code using them. I wasn't convinced that putting them inside the
class really provided any value and just made the names incredibly long and made
the code more difficult to read in my opinion. wdyt?

Looking forward to the next round!
Danno

http://codereview.chromium.org/3774003/diff/2001/3001
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/3774003/diff/2001/3001#newcode41
chrome/browser/policy/configuration_policy_pref_store.cc:41: virtual
~ConfigurationPolicyProviderKeeper() {}
On 2010/10/14 16:56:39, nadav wrote:
> It's be nice to have consistency with how you write empty function bodies. You
> use {} here, but { } (with a space) in the .h file. My personal preference
would
> be {}, but you can use either as long as you are consistent.

Done.

http://codereview.chromium.org/3774003/diff/2001/3001#newcode65
chrome/browser/policy/configuration_policy_pref_store.cc:65: return new
ConfigDirPolicyProvider(policy_map,
On 2010/10/14 16:56:39, nadav wrote:
> argument alignment
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Calls#Functi...

Done.

http://codereview.chromium.org/3774003/diff/2001/3001#newcode259
chrome/browser/policy/configuration_policy_pref_store.cc:259: bool success =
(provider_ == NULL || provider_->Provide(this));
On 2010/10/14 16:56:39, nadav wrote:
> const bool?
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Use_of_const#Use_of_c...

Done.

http://codereview.chromium.org/3774003/diff/2001/3001#newcode316
chrome/browser/policy/configuration_policy_pref_store.cc:316: bool found_one =
false;
On 2010/10/14 16:56:39, nadav wrote:
> Nit pick: found_one is a misleading name. Maybe call it found_any? (since you
> may find more than one - you don't stop after the first)

Done.

http://codereview.chromium.org/3774003/diff/2001/3001#newcode327
chrome/browser/policy/configuration_policy_pref_store.cc:327: for (const
PolicyToPreferenceMapEntry* current = map;
On 2010/10/14 16:56:39, nadav wrote:
> We typically prefer using ints in loops. It makes the for() loop a lot easier
to
> read. I doubt it makes any performance difference (and not clear to me this
code
> is performance critical). Any reason not to loop with in index?

Done.

http://codereview.chromium.org/3774003/diff/2001/3002
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/3774003/diff/2001/3002#newcode26
chrome/browser/policy/configuration_policy_pref_store.h:26: public
ConfigurationPolicyStore {
On 2010/10/14 16:56:39, nadav wrote:
> Multiple inheritance is allowed in only very limited cases, and this isn't one
> of them
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Multiple_Inheritance#...

ConfigurationPolicyStore is now ConfigurationPolicyStoreInterface and now
fulfills all of the requirements to be an interface, specifically, that the
constructor is protected. As a result, ConfigurationPolicyPrefStore fulfills the
requirements for multiple inheritance.

http://codereview.chromium.org/3774003/diff/2001/3002#newcode30
chrome/browser/policy/configuration_policy_pref_store.h:30:
ConfigurationPolicyPrefStore(const CommandLine* command_line,
On 2010/10/14 16:56:39, nadav wrote:
> Why const CommandLine* and not const CommandLine&?

Done. This has been removed, CommandLines are now handled in a different part of
the preference system.

http://codereview.chromium.org/3774003/diff/2001/3002#newcode61
chrome/browser/policy/configuration_policy_pref_store.h:61: static const
PolicyToPreferenceMapEntry simple_policy_map_[];
On 2010/10/14 16:56:39, nadav wrote:
> Wrong naming convention for a static const
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Constant_Names#Consta...

Done.

http://codereview.chromium.org/3774003/diff/2001/3002#newcode90
chrome/browser/policy/configuration_policy_pref_store.h:90: const
PolicyToPreferenceMapEntry* FindPolicyInMap(PolicyType policy,
On 2010/10/14 16:56:39, nadav wrote:
> bad indentation
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Declarations...
> (all arguments are always aligned the same!)

Done.

http://codereview.chromium.org/3774003/diff/2001/3002#newcode120
chrome/browser/policy/configuration_policy_pref_store.h:120: void
EnsureStringPrefExists(const std::string& path);
On 2010/10/14 16:56:39, nadav wrote:
> why the std:: here?

Unlike google3, Chrome/Chromium doesn't have a default using std::, it must be
explicit.

http://codereview.chromium.org/3774003/diff/2001/3002#newcode134
chrome/browser/policy/configuration_policy_pref_store.h:134: 
On 2010/10/14 16:56:39, nadav wrote:
> Please remove these blank lines

I had to add whitespace change to all of the files in the CL to force the code
review tool to consider these for the review. I will make sure they are all gone
before I commit.

[nadav, 2010-10-20 02:02:25]

Hi again,

I made a few more comments, and again, some of the issues may be recurring so
please try and correct them throughout. I must say that this CL is on the long
side, so please excuse me if my attention seems a little scattered at times and
I don't fully follow all pieces of it.

http://codereview.chromium.org/3774003/diff/13002/18003
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/3774003/diff/13002/18003#newcode44
chrome/browser/policy/config_dir_policy_provider.cc:44: DCHECK(policy_.get());
Is there any information that you may want to log with the DCHECK? My rule of
thumb is to think of the situation where I am presented with a crash in this
line. Which variable will I want to examine first? If I add it to the CHECK I
save a debugging iteration :)

http://codereview.chromium.org/3774003/diff/13002/18004
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/3774003/diff/13002/18004#newcode52
chrome/browser/policy/config_dir_policy_provider.h:52: const FilePath&
config_dir() { return config_dir_; }
this method can probably be declared const:
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Use_of_const#Use_of_c...

http://codereview.chromium.org/3774003/diff/13002/18006
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/3774003/diff/13002/18006#newcode484
chrome/browser/policy/configuration_policy_pref_store.cc:484: class
SearchTermsDataForValidation : public SearchTermsData {
A bit of a nit pick, but you may be polluting the namespace for nothing here.
Consider putting this class into an anonymous namespace, or making it a private
nested class of ConfigurationPolicyPrefStore (the former is probably easier).

http://codereview.chromium.org/3774003/diff/13002/18006#newcode523
chrome/browser/policy/configuration_policy_pref_store.cc:523: TemplateURLRef
search_url_ref(search_url, 0, 0);
can this be const?

http://codereview.chromium.org/3774003/diff/13002/18017
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/3774003/diff/13002/18017#newcode28
chrome/browser/policy/mock_configuration_policy_provider.h:28: typedef
std::map<ConfigurationPolicyType, Value*> PolicyMap;
typedefs should come before constructors/destructors:
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Declaration_Order#Dec...
That said, this typedef is not part of the API, and while this may be an
overkill for a mock, consider having the implementation in a .cc file which will
allow you to make the typedef private.

http://codereview.chromium.org/3774003/diff/13002/18019
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/3774003/diff/13002/18019#newcode100
chrome/browser/prefs/pref_service_unittest.cc:100:
prefs.RegisterStringPref(pref_name, "");
consider s/""/string()/. The argument here is of type std::string. "" is a
string literal, i.e., of type const char * const. It gets converted to an empty
string by implicit construction. However, string() is a constructor that creates
an empty string directly.

[danno, 2010-10-25 22:13:07]

I have addressed your feedback, thanks for that.

Several additional files have gotten pulled into the review due to some of the
const/naming changes that you recommended, turning this into a monster CL. I
recommend focusing on the original files I submitted in the first patch set, or
at least only the files in chrome/browser/policy/configuration*, since the new
files that have been added to the CL just touch my code tangentially, even
though I'm happy to clean them up within reason.

BTW, what do you think about renaming ConfigurationPolicyStoreInterface back to
ConfigurationPolicyStore? The C++ guidelines say that in the multiple
inheritance case, all pure interfaces must be tagged with "Interface" suffix.
However, this rule seems to be disregarded elsewhere throughout the Chrome code.
For consistency and conciseness, I would suggest keeping
ConfigurationPolicyStoreInterface a pure interface in the mechanics of its
implementation, but going back to the old name. 

Danno

http://codereview.chromium.org/3774003/diff/13002/18003
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/3774003/diff/13002/18003#newcode44
chrome/browser/policy/config_dir_policy_provider.cc:44: DCHECK(policy_.get());
I have fixed some of the DCHECKs in this code, even though it isn't part of the
original CL. They just got pulled in because of some of the const changes in the
main files. I'll fix up other DCHECKs where it makes sense, especially in the
core code from the original CL.

On 2010/10/20 02:02:25, nadav wrote:
> Is there any information that you may want to log with the DCHECK? My rule of
> thumb is to think of the situation where I am presented with a crash in this
> line. Which variable will I want to examine first? If I add it to the CHECK I
> save a debugging iteration :)

http://codereview.chromium.org/3774003/diff/13002/18004
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/3774003/diff/13002/18004#newcode52
chrome/browser/policy/config_dir_policy_provider.h:52: const FilePath&
config_dir() { return config_dir_; }
On 2010/10/20 02:02:25, nadav wrote:
> this method can probably be declared const:
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Use_of_const#Use_of_c...

Done.

http://codereview.chromium.org/3774003/diff/13002/18006
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/3774003/diff/13002/18006#newcode484
chrome/browser/policy/configuration_policy_pref_store.cc:484: class
SearchTermsDataForValidation : public SearchTermsData {
On 2010/10/20 02:02:25, nadav wrote:
> A bit of a nit pick, but you may be polluting the namespace for nothing here.
> Consider putting this class into an anonymous namespace, or making it a
private
> nested class of ConfigurationPolicyPrefStore (the former is probably easier).

Done.

http://codereview.chromium.org/3774003/diff/13002/18006#newcode523
chrome/browser/policy/configuration_policy_pref_store.cc:523: TemplateURLRef
search_url_ref(search_url, 0, 0);
On 2010/10/20 02:02:25, nadav wrote:
> can this be const?

Done.

http://codereview.chromium.org/3774003/diff/13002/18017
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/3774003/diff/13002/18017#newcode28
chrome/browser/policy/mock_configuration_policy_provider.h:28: typedef
std::map<ConfigurationPolicyType, Value*> PolicyMap;
On 2010/10/20 02:02:25, nadav wrote:
> typedefs should come before constructors/destructors:
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Declaration_Order#Dec...
> That said, this typedef is not part of the API, and while this may be an
> overkill for a mock, consider having the implementation in a .cc file which
will
> allow you to make the typedef private.

Done.

http://codereview.chromium.org/3774003/diff/13002/18019
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/3774003/diff/13002/18019#newcode100
chrome/browser/prefs/pref_service_unittest.cc:100:
prefs.RegisterStringPref(pref_name, "");
On 2010/10/20 02:02:25, nadav wrote:
> consider s/""/string()/. The argument here is of type std::string. "" is a
> string literal, i.e., of type const char * const. It gets converted to an
empty
> string by implicit construction. However, string() is a constructor that
creates
> an empty string directly.

Done.

[nadav, 2010-10-26 17:37:59]

Danno,

I actually think that keeping the "Interface" suffix on pure interfaces is
somewhat important. This helps in discouraging others from turning those
interfaces into concrete classes later on. This is a non-negligible risk because
changes to a base class are not always made with full knowledge of all derived
uses...

Sorry for getting carried away with the files I reviewed. I tried to keep to the
original files this time around :)

Nadav

http://codereview.chromium.org/3774003/diff/72001/73005
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/3774003/diff/72001/73005#newcode74
chrome/browser/policy/configuration_policy_pref_store.cc:74: static
ConfigurationPolicyProvider* CreateRecommendedProvider() {
I would actually think that the empty line here was a Good Thing. Also, the
class is long enough that you may want to consider separating the declaration
and definition (though it's not required if you think it's OK the way it is).

http://codereview.chromium.org/3774003/diff/72001/73005#newcode338
chrome/browser/policy/configuration_policy_pref_store.cc:338:
DCHECK(map[current].value_type == value->GetType())
If you use DCHECK_EQ you get the printout of the old and new values for free

http://codereview.chromium.org/3774003/diff/72001/73006
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/3774003/diff/72001/73006#newcode48
chrome/browser/policy/configuration_policy_pref_store.h:48: typedef
std::set<const char*> ProxyPreferenceSet;
Typedefs should come before (all) methods
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Declaration_Order#Dec...

http://codereview.chromium.org/3774003/diff/72001/73006#newcode104
chrome/browser/policy/configuration_policy_pref_store.h:104: const
PolicyToPreferenceMapEntry map[],
is there any reason you use const PolicyToPreferenceMapEntry map[] here but
const PolicyToPreferenceMapEntry* in the previous two methods? The "plain"
pointer notation is more common, and I wonder whether you want to be more
consistent with this.

[Mattias Nissler (ping if slow), 2010-10-27 08:30:31]

On 2010/10/26 17:37:59, nadav wrote:
> I actually think that keeping the "Interface" suffix on pure interfaces is
> somewhat important. This helps in discouraging others from turning those
> interfaces into concrete classes later on. This is a non-negligible risk
because
> changes to a base class are not always made with full knowledge of all derived
> uses...

I can see that's a useful thing to do. However, most of the chromium code
doesn't have the Interface suffix, so to most of us the suffix will look strange
and unnecessary. Note that sync and geolocation  are notable exceptions in the
chromium code that where added relatively recently (I think by Googlers  coming
in from other teams).

[nadav, 2010-10-27 15:47:20]

I understand where you're coming from, and when setting a coding style
there's always the issue of balancing consistency against other
considerations. However, a readability review is not the place to change
style guides or to discuss the merits of these changes. It only deals with
the application of an existing style to a given piece of code, and the
current style guide allows no exceptions for the multiple-inheritance rule.
I would recommend that you work with the owner of of either the C++ style
guide (c-style-arbiters@) to get an exception from this rule for Chromium or
(which is probably a lot easier) add the exception here:
http://www.chromium.org/developers/coding-style where Chromium-specific
style exceptions are already listed.
Feel free to CC me on any correspondence related to this. I actually think
your argument makes total sense. I'd just like to see it fixed where it will
actually be consistently applied and reviewed by the correct set of people.

Thanks
Nadav

On Wed, Oct 27, 2010 at 1:30 AM, <mnissler@chromium.org> wrote:

> On 2010/10/26 17:37:59, nadav wrote:
>
>> I actually think that keeping the "Interface" suffix on pure interfaces is
>> somewhat important. This helps in discouraging others from turning those
>> interfaces into concrete classes later on. This is a non-negligible risk
>>
> because
>
>> changes to a base class are not always made with full knowledge of all
>> derived
>> uses...
>>
>
> I can see that's a useful thing to do. However, most of the chromium code
> doesn't have the Interface suffix, so to most of us the suffix will look
> strange
> and unnecessary. Note that sync and geolocation  are notable exceptions in
> the
> chromium code that where added relatively recently (I think by Googlers
>  coming
> in from other teams).
>
>
> http://codereview.chromium.org/3774003/show
>

[danno, 2010-10-27 15:54:32]

OK, I'll see what chromium thinks about this. In parallel, I'll continue to work
on your other feedback, thanks for that.

Danno
On 2010/10/27 15:47:20, nadav wrote:
> I understand where you're coming from, and when setting a coding style
> there's always the issue of balancing consistency against other
> considerations. However, a readability review is not the place to change
> style guides or to discuss the merits of these changes. It only deals with
> the application of an existing style to a given piece of code, and the
> current style guide allows no exceptions for the multiple-inheritance rule.
> I would recommend that you work with the owner of of either the C++ style
> guide (c-style-arbiters@) to get an exception from this rule for Chromium or
> (which is probably a lot easier) add the exception here:
> http://www.chromium.org/developers/coding-style where Chromium-specific
> style exceptions are already listed.
> Feel free to CC me on any correspondence related to this. I actually think
> your argument makes total sense. I'd just like to see it fixed where it will
> actually be consistently applied and reviewed by the correct set of people.
> 
> Thanks
> Nadav
> 
> On Wed, Oct 27, 2010 at 1:30 AM, <mailto:mnissler@chromium.org> wrote:
> 
> > On 2010/10/26 17:37:59, nadav wrote:
> >
> >> I actually think that keeping the "Interface" suffix on pure interfaces is
> >> somewhat important. This helps in discouraging others from turning those
> >> interfaces into concrete classes later on. This is a non-negligible risk
> >>
> > because
> >
> >> changes to a base class are not always made with full knowledge of all
> >> derived
> >> uses...
> >>
> >
> > I can see that's a useful thing to do. However, most of the chromium code
> > doesn't have the Interface suffix, so to most of us the suffix will look
> > strange
> > and unnecessary. Note that sync and geolocation  are notable exceptions in
> > the
> > chromium code that where added relatively recently (I think by Googlers
> >  coming
> > in from other teams).
> >
> >
> > http://codereview.chromium.org/3774003/show
> >
>

[danno, 2010-10-28 21:02:36]

I addressed your feedback, please take another look.

Thanks, Danno

http://codereview.chromium.org/3774003/diff/72001/73005
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/3774003/diff/72001/73005#newcode74
chrome/browser/policy/configuration_policy_pref_store.cc:74: static
ConfigurationPolicyProvider* CreateRecommendedProvider() {
On 2010/10/26 17:37:59, nadav wrote:
> I would actually think that the empty line here was a Good Thing. Also, the
> class is long enough that you may want to consider separating the declaration
> and definition (though it's not required if you think it's OK the way it is).

Done.

http://codereview.chromium.org/3774003/diff/72001/73005#newcode338
chrome/browser/policy/configuration_policy_pref_store.cc:338:
DCHECK(map[current].value_type == value->GetType())
On 2010/10/26 17:37:59, nadav wrote:
> If you use DCHECK_EQ you get the printout of the old and new values for free

Done.

http://codereview.chromium.org/3774003/diff/72001/73006
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/3774003/diff/72001/73006#newcode48
chrome/browser/policy/configuration_policy_pref_store.h:48: typedef
std::set<const char*> ProxyPreferenceSet;
On 2010/10/26 17:37:59, nadav wrote:
> Typedefs should come before (all) methods
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Declaration_Order#Dec...

Done.

http://codereview.chromium.org/3774003/diff/72001/73006#newcode104
chrome/browser/policy/configuration_policy_pref_store.h:104: const
PolicyToPreferenceMapEntry map[],
On 2010/10/26 17:37:59, nadav wrote:
> is there any reason you use const PolicyToPreferenceMapEntry map[] here but
> const PolicyToPreferenceMapEntry* in the previous two methods? The "plain"
> pointer notation is more common, and I wonder whether you want to be more
> consistent with this.

Done.

[nadav, 2010-10-29 21:52:43]

LGTM
approved for C++ readability

[danno, 2010-11-02 12:56:19]

Hi Gabor. As discussed, I need a chromium reviewer's LGTM in addition to
nadav's, please take a quick look.

[gfeher, 2010-11-02 13:40:57]

LGTM with a likely typo.
(And you have two green trybots that are actually not completed.)

http://codereview.chromium.org/3774003/diff/108001/109012
File chrome/browser/policy/configuration_policy_provider_win_unittest.cc
(right):

http://codereview.chromium.org/3774003/diff/108001/109012#newcode219
chrome/browser/policy/configuration_policy_provider_win_unittest.cc:219:
key.DeleteKey(Lstd::string());
Typo?