Cosmetic cleanups in chrome/browser/policy/

Cosmetic cleanups in chrome/browser/policy/

BUG=None
TEST=Everything works as before, compiles cleanly


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

[Joao da Silva, 2011-09-20 11:43:07]

Please review. It's not as bad as it looks :-)

[pastarmovj, 2011-09-20 13:14:08]

LGTM with a few nits below.

There was one issue a few times that I am not sure I am the right one but I saw
both examples and code and just opted for the one I prefer more. When you have a
namespace only to declare some forwards sometimes you have an empty line between
the namespace declaration and its contents and sometimes you don't. You should
consider unifying this at least in the code you touched.

Kudos for doing this cleanup.

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/file_b...
File chrome/browser/policy/file_based_policy_loader.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/file_b...
chrome/browser/policy/file_based_policy_loader.h:18: class FilePathWatcher;
To be consistent with the rest why don't you put a new line around the forward
declaration here?

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/mock_c...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/mock_c...
chrome/browser/policy/mock_configuration_policy_provider.h:33:
ConfigurationPolicyProvider::Observer* observer) OVERRIDE {}
Don't we put a single space between { } when the body is empty?

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/url_bl...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/url_bl...
chrome/browser/policy/url_blacklist_manager.h:88: void AddFilter(const
std::string& filter, bool block);
I hope rearranging the functions here was followed by the same rearrange in the
source file.

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_cache.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_cache.h:20: class CachedCloudPolicyResponse;
Again one empty line between the namespace and its contents.

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_disk_cache.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_disk_cache.h:15: class
CachedCloudPolicyResponse;
Ditto space here.

[Joao da Silva, 2011-09-20 18:51:17]

Thanks for looking at this monster :-)

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/file_b...
File chrome/browser/policy/file_based_policy_loader.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/file_b...
chrome/browser/policy/file_based_policy_loader.h:18: class FilePathWatcher;
On 2011/09/20 13:14:08, pastarmovj wrote:
> To be consistent with the rest why don't you put a new line around the forward
> declaration here?

Done.

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/mock_c...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/mock_c...
chrome/browser/policy/mock_configuration_policy_provider.h:33:
ConfigurationPolicyProvider::Observer* observer) OVERRIDE {}
On 2011/09/20 13:14:08, pastarmovj wrote:
> Don't we put a single space between { } when the body is empty?

I'm not sure about this, so I ran a quick grep job:

chrome/browser# grep '{ }' | wc -l 
207
chrome/browser# grep '{}' | wc -l 
3553
chrome/browser/policy# grep '{ }' | wc -l 
0
chrome/browser# grep '{}' | wc -l 
105

I'd stick with the common pattern for now, if that's ok with you too :-)

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/url_bl...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/url_bl...
chrome/browser/policy/url_blacklist_manager.h:88: void AddFilter(const
std::string& filter, bool block);
On 2011/09/20 13:14:08, pastarmovj wrote:
> I hope rearranging the functions here was followed by the same rearrange in
the
> source file.

Done.

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_cache.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_cache.h:20: class CachedCloudPolicyResponse;
On 2011/09/20 13:14:08, pastarmovj wrote:
> Again one empty line between the namespace and its contents.

Done.

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_disk_cache.h (right):

http://codereview.chromium.org/7941008/diff/2001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_disk_cache.h:15: class
CachedCloudPolicyResponse;
On 2011/09/20 13:14:08, pastarmovj wrote:
> Ditto space here.

Done.

[commit-bot: I haz the power, 2011-09-21 08:14:54]

Can't process patch for file
chrome/browser/policy/configuration_policy_provider_delegate_win.h.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-09-21 12:25:40]

Change committed as 102100