Readability review.

Fix style issues with ConfigDirPolicyProvider.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=59086

[Mattias Nissler (ping if slow), 2010-09-01 12:52:58]

OK, seems we're ready to go :) Let me know if you have any questions or trouble
with the chromium codereview tool. Thanks.

[Mattias Nissler (ping if slow), 2010-09-07 10:18:57]

Matthew, I'm putting you as the reviewer in place of Alice. Please let me now if
you have any issues with the chromium code review tool. Thanks!

[mharris, 2010-09-08 19:48:58]

Thanks for applying for readability. Remember that the purpose of a readability
review is not to judge your coding ability but to confirm that you've
internalized the style guide and are aware of appropriate conventions and
utilities. Because this is my first readability review in Chromium, I'm learning
about the Chromium code base as I go, and my ability to point those things out
is limited.

I generally try to point out only the first instance of each issue. It is your
responsibility to fix all of the instances in this change.

This looks like very clean code, and I don't have much to say. I don't expect
the review to last more than a few iterations.

http://codereview.chromium.org/3331001/diff/1/2
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/3331001/diff/1/2#newcode181
chrome/browser/policy/config_dir_policy_provider.cc:181: if (reload_task_)
FYI, braces are not required for single-line statements, but some projects
require them. You appear to be following the Chromium convention, but I thought
I should point it out.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

http://codereview.chromium.org/3331001/diff/1/3
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/3331001/diff/1/3#newcode7
chrome/browser/policy/config_dir_policy_provider.h:7: #pragma once
The Google style guide says you should not use #pragma once, and the Chromium
style guide doesn't contradict that. Is this allowed? (I see it in every other
header in this directory too.)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Window...

http://codereview.chromium.org/3331001/diff/1/3#newcode25
chrome/browser/policy/config_dir_policy_provider.h:25: class PolicyDirLoader :
public FilePathWatcher::Delegate {
I notice that you don't place your classes into a namespace. The Google style
guide encourages the use of namespaces (wrapping the entire source file after
the #includes) to prevent unintentional naming conflicts. Is there a reason you
don't use them here?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

http://codereview.chromium.org/3331001/diff/1/3#newcode40
chrome/browser/policy/config_dir_policy_provider.h:40: // Get the current
dictionary value object. Ownership of the returned value is
Function comments should be descriptive ("Gets the current object") rather than
imperative ("Get the current object").

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/3331001/diff/1/3#newcode83
chrome/browser/policy/config_dir_policy_provider.h:83: base::Time
last_modification_file_;
You should #include "base/time.h" for this instead of depending on another
header to provide it. Otherwise your code is at risk of breaking mysteriously
when somebody else stops #including it.

http://codereview.chromium.org/3331001/diff/1/3#newcode111
chrome/browser/policy/config_dir_policy_provider.h:111: public
base::RefCountedThreadSafe<PolicyDirWatcher> {
The Google style guide forbids multiple inheritance except when all
superclasses, with the possible exception of the first one, are pure interfaces,
which must end with the Interface suffix.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Multip...

The class comment for RefCounted says, "you should always make your destructor
private, to avoid any code deleting the object accidentally while there are
references to it." I don't see a private destructor here.

http://codereview.chromium.org/3331001/diff/1/4
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/3331001/diff/1/4#newcode12
chrome/browser/policy/config_dir_policy_provider_unittest.cc:12: #include
"testing/gmock/include/gmock/gmock.h"
Did you intend to alphabetize these #includes? These last two don't seem to
follow the pattern of the ones above them.

[Mattias Nissler (ping if slow), 2010-09-09 09:43:42]

Here is a new version in which I addressed your comments.

http://codereview.chromium.org/3331001/diff/1/2
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/3331001/diff/1/2#newcode181
chrome/browser/policy/config_dir_policy_provider.cc:181: if (reload_task_)
On 2010/09/08 19:48:58, mharris wrote:
> FYI, braces are not required for single-line statements, but some projects
> require them. You appear to be following the Chromium convention, but I
thought
> I should point it out.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

Right. Most of the chromium code base doesn't have them if they're not required
(i.e. when both the conditional and the statement fit their line).

http://codereview.chromium.org/3331001/diff/1/3
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/3331001/diff/1/3#newcode7
chrome/browser/policy/config_dir_policy_provider.h:7: #pragma once
On 2010/09/08 19:48:58, mharris wrote:
> The Google style guide says you should not use #pragma once, and the Chromium
> style guide doesn't contradict that. Is this allowed? (I see it in every other
> header in this directory too.)
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Window...

This is the result of a recent discussion on the chromium-dev list
(http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
decision was to put them since the can buy us in the order of 10% faster
compilation on Windows.

http://codereview.chromium.org/3331001/diff/1/3#newcode25
chrome/browser/policy/config_dir_policy_provider.h:25: class PolicyDirLoader :
public FilePathWatcher::Delegate {
On 2010/09/08 19:48:58, mharris wrote:
> I notice that you don't place your classes into a namespace. The Google style
> guide encourages the use of namespaces (wrapping the entire source file after
> the #includes) to prevent unintentional naming conflicts. Is there a reason
you
> don't use them here?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

There is no good reason I guess. There are currently some subsystems that live
in a separate namespace (sync is one example) but others don't. I think you have
a point and we should put all the policy-related stuff into its own namespace.
Are you OK if I take care of that in a separate CL? I don't think it's a good
idea to include all the other policy code in this CL. WDYT?

http://codereview.chromium.org/3331001/diff/1/3#newcode40
chrome/browser/policy/config_dir_policy_provider.h:40: // Get the current
dictionary value object. Ownership of the returned value is
On 2010/09/08 19:48:58, mharris wrote:
> Function comments should be descriptive ("Gets the current object") rather
than
> imperative ("Get the current object").
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done for all function comments.

http://codereview.chromium.org/3331001/diff/1/3#newcode83
chrome/browser/policy/config_dir_policy_provider.h:83: base::Time
last_modification_file_;
On 2010/09/08 19:48:58, mharris wrote:
> You should #include "base/time.h" for this instead of depending on another
> header to provide it. Otherwise your code is at risk of breaking mysteriously
> when somebody else stops #including it.

Done. Also fixed a few more instances of this.

http://codereview.chromium.org/3331001/diff/1/3#newcode111
chrome/browser/policy/config_dir_policy_provider.h:111: public
base::RefCountedThreadSafe<PolicyDirWatcher> {
On 2010/09/08 19:48:58, mharris wrote:
> The Google style guide forbids multiple inheritance except when all
> superclasses, with the possible exception of the first one, are pure
interfaces,
> which must end with the Interface suffix.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Multip...
> 
> The class comment for RefCounted says, "you should always make your destructor
> private, to avoid any code deleting the object accidentally while there are
> references to it." I don't see a private destructor here.

I switched FileWatcher to a private data member and put the private destructor.

http://codereview.chromium.org/3331001/diff/1/4
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/3331001/diff/1/4#newcode12
chrome/browser/policy/config_dir_policy_provider_unittest.cc:12: #include
"testing/gmock/include/gmock/gmock.h"
On 2010/09/08 19:48:58, mharris wrote:
> Did you intend to alphabetize these #includes? These last two don't seem to
> follow the pattern of the ones above them.

Doh! All chromium includes should be alphabetized. Fixed.

[mharris, 2010-09-10 01:41:48]

On 2010/09/09 09:43:42, Mattias Nissler wrote:
> This is the result of a recent discussion on the chromium-dev list
>
(http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> decision was to put them since the can buy us in the order of 10% faster
> compilation on Windows.

Okay. I just followed up on that thread with a request to add it to the style
guide. It doesn't matter to me whether you guys require or forbid it, but it
should be consistent with the style guide.

[mharris, 2010-09-10 01:47:25]

Ugh. Rietveld seems to have discarded the rest of my comment. Here's a second
try:

Yes, it's fine to add namespaces in a separate change, since it's a broader
change than just readability fixes.

I like your change to move the FileWatcher to a member variable.

This has been a ridiculously short readability review, but your code meets the
requirements. Go ahead and submit, after removing the blank lines you added if
necessary. I've marked the readability bug as fixed, and you should receive
readability shortly after you submit. Congratulations!

Good luck on your future C++ coding at Google. Remember that the main Google C++
Style Guide differs in some ways from the Chromium style.

[Mattias Nissler (ping if slow), 2010-09-10 07:39:35]

On 2010/09/10 01:47:25, mharris wrote:
> Ugh. Rietveld seems to have discarded the rest of my comment. Here's a second
> try:
> 
> Yes, it's fine to add namespaces in a separate change, since it's a broader
> change than just readability fixes.
> 
> I like your change to move the FileWatcher to a member variable.
> 
> This has been a ridiculously short readability review, but your code meets the
> requirements. Go ahead and submit, after removing the blank lines you added if
> necessary. I've marked the readability bug as fixed, and you should receive
> readability shortly after you submit. Congratulations!

Great, thanks!

> 
> Good luck on your future C++ coding at Google. Remember that the main Google
C++
> Style Guide differs in some ways from the Chromium style.

I'll keep this in mind.