Preference provider implementation backed by JSON files in a directory.

Preference provider implementation backed by JSON files in a directory.

BUG=42412
TEST=Unit tests in chrome/browser/value_tree_policy_decoder.cc and base/values_unittest.cc

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

[Mattias Nissler (ping if slow), 2010-05-12 08:32:44]

Pam, after reviewing danno's CL, how about more of this stuff?
Danno, could you have a quick look as well?

[Pam (message me for reviews), 2010-05-12 13:13:34]

http://codereview.chromium.org/2027010/diff/2001/3004
File chrome/browser/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3004#newcode17
chrome/browser/config_dir_policy_provider.cc:17: : config_dir_(config_dir) {
4-space indent please
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

http://codereview.chromium.org/2027010/diff/2001/3004#newcode27
chrome/browser/config_dir_policy_provider.cc:27: // Enumerate the files and
bring them into order.
I don't understand "bring them into order".

http://codereview.chromium.org/2027010/diff/2001/3004#newcode28
chrome/browser/config_dir_policy_provider.cc:28: std::set<FilePath> files;
Since a <set> is unordered, does this always process the files in lexicographic
order?

http://codereview.chromium.org/2027010/diff/2001/3004#newcode46
chrome/browser/config_dir_policy_provider.cc:46: continue;
Is this robust enough to make enterprise admins happy, or should it propagate a
real error upward? I guess at some point we need to rely on the admins
protecting their config files properly.

http://codereview.chromium.org/2027010/diff/2001/3005
File chrome/browser/config_dir_policy_provider.h (right):

http://codereview.chromium.org/2027010/diff/2001/3005#newcode14
chrome/browser/config_dir_policy_provider.h:14: // A policy provider
implementation backed by set of files in a given directory.
...a set...

http://codereview.chromium.org/2027010/diff/2001/3005#newcode29
chrome/browser/config_dir_policy_provider.h:29: DictionaryValue* ReadPolicy();
Danno's most recent patch calls a single setting a "policy", and the collection
of them... a collection of policies, I guess. So this would be clearer as
ReadPolicies() or similar.

http://codereview.chromium.org/2027010/diff/2001/3006
File chrome/browser/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3006#newcode59
chrome/browser/config_dir_policy_provider_unittest.cc:59:
TEST_F(ConfigDirPolicyProviderTest, ReadPrefsNonExistingDirectory) {
nit: "NonExistent" (or just "Missing")
also non_existent_dir below

http://codereview.chromium.org/2027010/diff/2001/3006#newcode71
chrome/browser/config_dir_policy_provider_unittest.cc:71:
ConfigDirPolicyProvider provider(test_dir_);
Please insert some blank lines where appropriate for readability. One here, for
instance, and at least one in ReadPrefsMergePrefs; maybe another one or two.

http://codereview.chromium.org/2027010/diff/2001/3006#newcode86
chrome/browser/config_dir_policy_provider_unittest.cc:86: DictionaryValue
test_dict_a;
If <set> doesn't preserve order, and this test doesn't catch that, it should be
expanded (more files) until it does.

http://codereview.chromium.org/2027010/diff/2001/3008
File chrome/browser/mock_configuration_policy_store.h (right):

http://codereview.chromium.org/2027010/diff/2001/3008#newcode14
chrome/browser/mock_configuration_policy_store.h:14: // settings as they get
set.
Is this used widely, or could it just be put in the unit test file?

http://codereview.chromium.org/2027010/diff/2001/3009
File chrome/browser/value_tree_policy_decoder.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3009#newcode13
chrome/browser/value_tree_policy_decoder.cc:13: const PolicyTypeEntry
kPolicyTypeTable[] = {
Since the policy constants aren't called "types" anymore, maybe "kPolicyMapping"
or similar?

http://codereview.chromium.org/2027010/diff/2001/3009#newcode22
chrome/browser/value_tree_policy_decoder.cc:22: void
DecodePolicyValueTree(DictionaryValue* policy,
Same as above about "policy" vs. "policies"

http://codereview.chromium.org/2027010/diff/2001/3009#newcode24
chrome/browser/value_tree_policy_decoder.cc:24: for (unsigned int i = 0; i <
arraysize(kPolicyTypeTable); ++i) {
Will this recompute the arraysize every time? Do we care?

http://codereview.chromium.org/2027010/diff/2001/3010
File chrome/browser/value_tree_policy_decoder.h (right):

http://codereview.chromium.org/2027010/diff/2001/3010#newcode13
chrome/browser/value_tree_policy_decoder.h:13: // Base functionality for all
policy providers that use a tree structure holding
"tree structure" is very generic -- specifically, a DictionaryValue tree, yes?

http://codereview.chromium.org/2027010/diff/2001/3010#newcode15
chrome/browser/value_tree_policy_decoder.h:15: // writes the configuration to
the given |ConfigurationPolicyStore|. Returns
This comment is missing the

Also, why not put this into the policy provider base class?

http://codereview.chromium.org/2027010/diff/2001/3010#newcode16
chrome/browser/value_tree_policy_decoder.h:16: void
DecodePolicyValueTree(DictionaryValue* policy,
Same comment as before about singular "policy". PolicyValueTree is OK because
Policy is an adjective there, but the DictionaryValue is a whole set of
(so-called) policies, right?

[Mattias Nissler (ping if slow), 2010-05-12 16:34:06]

Hey Pam, here's a new version, I hope it's better :-)

http://codereview.chromium.org/2027010/diff/2001/3004
File chrome/browser/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3004#newcode17
chrome/browser/config_dir_policy_provider.cc:17: : config_dir_(config_dir) {
On 2010/05/12 13:13:34, Pam wrote:
> 4-space indent please
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

Done.

http://codereview.chromium.org/2027010/diff/2001/3004#newcode27
chrome/browser/config_dir_policy_provider.cc:27: // Enumerate the files and
bring them into order.
On 2010/05/12 13:13:34, Pam wrote:
> I don't understand "bring them into order".

I was refereing to sorting them lexicographically, changed the comment
accordingly.
Done.

http://codereview.chromium.org/2027010/diff/2001/3004#newcode28
chrome/browser/config_dir_policy_provider.cc:28: std::set<FilePath> files;
On 2010/05/12 13:13:34, Pam wrote:
> Since a <set> is unordered, does this always process the files in
lexicographic
> order?

I believe std::set actually keeps all elements in ascending order
(http://www.sgi.com/tech/stl/SortedAssociativeContainer.html) w.r.t.
std::less<FilePath> which in turn uses FilePath::operator<(). Am I missing
something?

http://codereview.chromium.org/2027010/diff/2001/3004#newcode46
chrome/browser/config_dir_policy_provider.cc:46: continue;
On 2010/05/12 13:13:34, Pam wrote:
> Is this robust enough to make enterprise admins happy, or should it propagate
a
> real error upward? I guess at some point we need to rely on the admins
> protecting their config files properly.

Basically, we have two choices here when we find an error: Abort chrome startup
or tell people there is an error but continue. I wouldn't consider configuration
policy to be a reliable security measure, so it's OK to just ignore invalid
policies and continue. I think it's fair to expect admins to test their policies
after they have defined them to check whether they have the desired effect.

I'm unsure about error reporting though, what alternatives do we have here?
Admittedly logging is very minimal solution, do you think it's worth reporting
policy errors in an info bar? I think for now, logging will do it and we can
improve on that once we have indication logging is insufficient.

http://codereview.chromium.org/2027010/diff/2001/3005
File chrome/browser/config_dir_policy_provider.h (right):

http://codereview.chromium.org/2027010/diff/2001/3005#newcode14
chrome/browser/config_dir_policy_provider.h:14: // A policy provider
implementation backed by set of files in a given directory.
On 2010/05/12 13:13:34, Pam wrote:
> ...a set...

Done.

http://codereview.chromium.org/2027010/diff/2001/3005#newcode29
chrome/browser/config_dir_policy_provider.h:29: DictionaryValue* ReadPolicy();
On 2010/05/12 13:13:34, Pam wrote:
> Danno's most recent patch calls a single setting a "policy", and the
collection
> of them... a collection of policies, I guess. So this would be clearer as
> ReadPolicies() or similar.

Done.

http://codereview.chromium.org/2027010/diff/2001/3006
File chrome/browser/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3006#newcode59
chrome/browser/config_dir_policy_provider_unittest.cc:59:
TEST_F(ConfigDirPolicyProviderTest, ReadPrefsNonExistingDirectory) {
On 2010/05/12 13:13:34, Pam wrote:
> nit: "NonExistent" (or just "Missing")
> also non_existent_dir below

Done.

http://codereview.chromium.org/2027010/diff/2001/3006#newcode71
chrome/browser/config_dir_policy_provider_unittest.cc:71:
ConfigDirPolicyProvider provider(test_dir_);
On 2010/05/12 13:13:34, Pam wrote:
> Please insert some blank lines where appropriate for readability. One here,
for
> instance, and at least one in ReadPrefsMergePrefs; maybe another one or two.

Oh, I actually like spacing, but Jochen told me Chromium doesn't :) I'll add
them!
Done.

http://codereview.chromium.org/2027010/diff/2001/3006#newcode86
chrome/browser/config_dir_policy_provider_unittest.cc:86: DictionaryValue
test_dict_a;
On 2010/05/12 13:13:34, Pam wrote:
> If <set> doesn't preserve order, and this test doesn't catch that, it should
be
> expanded (more files) until it does.

See my other comment for the std::set semantics.

Adding more files here won't guarantee we detect ordering problems, since there
aren't any guarantees on the order in which the file system returns file names
when reading a directory.

I have tried locally and for my filesystem show unpredictable but deterministic
behaviour (IIRC ext2 uses hashing for dir indexes).

I changed the code nevertheless, as to increase the chance to detect problems.

http://codereview.chromium.org/2027010/diff/2001/3008
File chrome/browser/mock_configuration_policy_store.h (right):

http://codereview.chromium.org/2027010/diff/2001/3008#newcode14
chrome/browser/mock_configuration_policy_store.h:14: // settings as they get
set.
On 2010/05/12 13:13:34, Pam wrote:
> Is this used widely, or could it just be put in the unit test file?

The gconf policy provider I have already written will be using it as well, so
I've put it into a separate file. I can inline it for now and change that in a
subsequent patch if you like that better. But I guess the code would also be
useful for testing other policy providers (e.g. the windows group policy one
Danno is working on), so I'd rather keep it seperate.

http://codereview.chromium.org/2027010/diff/2001/3009
File chrome/browser/value_tree_policy_decoder.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3009#newcode13
chrome/browser/value_tree_policy_decoder.cc:13: const PolicyTypeEntry
kPolicyTypeTable[] = {
On 2010/05/12 13:13:34, Pam wrote:
> Since the policy constants aren't called "types" anymore, maybe
"kPolicyMapping"
> or similar?

Channged it to kPolicyValueTreeMapping.
Done.

http://codereview.chromium.org/2027010/diff/2001/3009#newcode22
chrome/browser/value_tree_policy_decoder.cc:22: void
DecodePolicyValueTree(DictionaryValue* policy,
On 2010/05/12 13:13:34, Pam wrote:
> Same as above about "policy" vs. "policies"

Done.

http://codereview.chromium.org/2027010/diff/2001/3009#newcode24
chrome/browser/value_tree_policy_decoder.cc:24: for (unsigned int i = 0; i <
arraysize(kPolicyTypeTable); ++i) {
On 2010/05/12 13:13:34, Pam wrote:
> Will this recompute the arraysize every time? Do we care?

It effectively boils down to a sizeof() expression which is evaluated at compile
time (see base/basictypes.h:122).

http://codereview.chromium.org/2027010/diff/2001/3010
File chrome/browser/value_tree_policy_decoder.h (right):

http://codereview.chromium.org/2027010/diff/2001/3010#newcode13
chrome/browser/value_tree_policy_decoder.h:13: // Base functionality for all
policy providers that use a tree structure holding
On 2010/05/12 13:13:34, Pam wrote:
> "tree structure" is very generic -- specifically, a DictionaryValue tree, yes?

Done.

http://codereview.chromium.org/2027010/diff/2001/3010#newcode15
chrome/browser/value_tree_policy_decoder.h:15: // writes the configuration to
the given |ConfigurationPolicyStore|. Returns
On 2010/05/12 13:13:34, Pam wrote:
> This comment is missing the
> 
> Also, why not put this into the policy provider base class?

Good idea!
Done.

http://codereview.chromium.org/2027010/diff/2001/3010#newcode16
chrome/browser/value_tree_policy_decoder.h:16: void
DecodePolicyValueTree(DictionaryValue* policy,
On 2010/05/12 13:13:34, Pam wrote:
> Same comment as before about singular "policy". PolicyValueTree is OK because
> Policy is an adjective there, but the DictionaryValue is a whole set of
> (so-called) policies, right?

Done.

[tfarina, 2010-05-12 16:34:37]

http://codereview.chromium.org/2027010/diff/2001/3001
File base/values.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3001#newcode1
base/values.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
nit: 2010

http://codereview.chromium.org/2027010/diff/2001/3003
File base/values_unittest.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3003#newcode1
base/values_unittest.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
nit: 2010

http://codereview.chromium.org/2027010/diff/2001/3004
File chrome/browser/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3004#newcode14
chrome/browser/config_dir_policy_provider.cc:14: #include
"chrome/browser/value_tree_policy_decoder.h"
nit: alphabetical order.

http://codereview.chromium.org/2027010/diff/2001/3005
File chrome/browser/config_dir_policy_provider.h (right):

http://codereview.chromium.org/2027010/diff/2001/3005#newcode24
chrome/browser/config_dir_policy_provider.h:24: // |ConfigurationPolicyProvider|
implementation.
nit: no need of | |, we use | | with member variables and parameters.

http://codereview.chromium.org/2027010/diff/2001/3008
File chrome/browser/mock_configuration_policy_store.h (right):

http://codereview.chromium.org/2027010/diff/2001/3008#newcode17
chrome/browser/mock_configuration_policy_store.h:17:
MockConfigurationPolicyStore() { }
nit: just {}, no need of spaces between them.

http://codereview.chromium.org/2027010/diff/2001/3008#newcode25
chrome/browser/mock_configuration_policy_store.h:25: //
|ConfigurationPolicyStore| implementation.
nit: no need of | | here too.

http://codereview.chromium.org/2027010/diff/2001/3009
File chrome/browser/value_tree_policy_decoder.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3009#newcode7
chrome/browser/value_tree_policy_decoder.cc:7: typedef struct {
please, could you use just struct PolicyTypeEntry? no need of typedef (we are in
c++ :) )

Can we put this struct in anonymous namespace if we are not going to use it
outside of this file scope?

http://codereview.chromium.org/2027010/diff/2001/3009#newcode24
chrome/browser/value_tree_policy_decoder.cc:24: for (unsigned int i = 0; i <
arraysize(kPolicyTypeTable); ++i) {
nit: use size_t instead?

http://codereview.chromium.org/2027010/diff/2001/3010
File chrome/browser/value_tree_policy_decoder.h (right):

http://codereview.chromium.org/2027010/diff/2001/3010#newcode9
chrome/browser/value_tree_policy_decoder.h:9: #include <base/scoped_ptr.h>
should this and basictypes.h included here? I don't see the usage. Also,
shouldn't you are using "" instead of <>?

http://codereview.chromium.org/2027010/diff/2001/3010#newcode10
chrome/browser/value_tree_policy_decoder.h:10: #include <base/values.h>
can we forward declare the DictionaryValue?

[Mattias Nissler (ping if slow), 2010-05-14 08:25:13]

http://codereview.chromium.org/2027010/diff/2001/3001
File base/values.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3001#newcode1
base/values.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
On 2010/05/12 16:34:38, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/2027010/diff/2001/3003
File base/values_unittest.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3003#newcode1
base/values_unittest.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2010/05/12 16:34:38, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/2027010/diff/2001/3004
File chrome/browser/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3004#newcode14
chrome/browser/config_dir_policy_provider.cc:14: #include
"chrome/browser/value_tree_policy_decoder.h"
On 2010/05/12 16:34:38, tfarina wrote:
> nit: alphabetical order.

Done.

http://codereview.chromium.org/2027010/diff/2001/3005
File chrome/browser/config_dir_policy_provider.h (right):

http://codereview.chromium.org/2027010/diff/2001/3005#newcode24
chrome/browser/config_dir_policy_provider.h:24: // |ConfigurationPolicyProvider|
implementation.
On 2010/05/12 16:34:38, tfarina wrote:
> nit: no need of | |, we use | | with member variables and parameters.

Done.

http://codereview.chromium.org/2027010/diff/2001/3008
File chrome/browser/mock_configuration_policy_store.h (right):

http://codereview.chromium.org/2027010/diff/2001/3008#newcode17
chrome/browser/mock_configuration_policy_store.h:17:
MockConfigurationPolicyStore() { }
On 2010/05/12 16:34:38, tfarina wrote:
> nit: just {}, no need of spaces between them.

Done.

http://codereview.chromium.org/2027010/diff/2001/3008#newcode25
chrome/browser/mock_configuration_policy_store.h:25: //
|ConfigurationPolicyStore| implementation.
On 2010/05/12 16:34:38, tfarina wrote:
> nit: no need of | | here too.

Done.

http://codereview.chromium.org/2027010/diff/2001/3009
File chrome/browser/value_tree_policy_decoder.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3009#newcode7
chrome/browser/value_tree_policy_decoder.cc:7: typedef struct {
On 2010/05/12 16:34:38, tfarina wrote:
> please, could you use just struct PolicyTypeEntry? no need of typedef (we are
in
> c++ :) )
> 
> Can we put this struct in anonymous namespace if we are not going to use it
> outside of this file scope?

Done.

http://codereview.chromium.org/2027010/diff/2001/3009#newcode24
chrome/browser/value_tree_policy_decoder.cc:24: for (unsigned int i = 0; i <
arraysize(kPolicyTypeTable); ++i) {
On 2010/05/12 16:34:38, tfarina wrote:
> nit: use size_t instead?

Done.

http://codereview.chromium.org/2027010/diff/2001/3010
File chrome/browser/value_tree_policy_decoder.h (right):

http://codereview.chromium.org/2027010/diff/2001/3010#newcode9
chrome/browser/value_tree_policy_decoder.h:9: #include <base/scoped_ptr.h>
On 2010/05/12 16:34:38, tfarina wrote:
> should this and basictypes.h included here? I don't see the usage. Also,
> shouldn't you are using "" instead of <>?

Done.

http://codereview.chromium.org/2027010/diff/2001/3010#newcode10
chrome/browser/value_tree_policy_decoder.h:10: #include <base/values.h>
On 2010/05/12 16:34:38, tfarina wrote:
> can we forward declare the DictionaryValue?

Done.

[Mattias Nissler (ping if slow), 2010-05-14 08:52:48]

Hey guys, I think this is ready for another iteration :-)

[Pam (message me for reviews), 2010-05-14 09:24:10]

Getting very close. Probably you'll point out my confusion on the two remaining
comments/requests, below, and I'll respond with an LGTM. :)

- Pam

http://codereview.chromium.org/2027010/diff/2001/3004
File chrome/browser/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3004#newcode28
chrome/browser/config_dir_policy_provider.cc:28: std::set<FilePath> files;
On 2010/05/12 16:34:06, mnissler wrote:
> I believe std::set actually keeps all elements in ascending order
> (http://www.sgi.com/tech/stl/SortedAssociativeContainer.html) w.r.t.
> std::less<FilePath> which in turn uses FilePath::operator<(). Am I missing
> something?

You're right. I was confusing it with Python's set.

http://codereview.chromium.org/2027010/diff/2001/3004#newcode46
chrome/browser/config_dir_policy_provider.cc:46: continue;
On 2010/05/12 16:34:06, mnissler wrote:
> Basically, we have two choices here when we find an error: Abort chrome
startup
> or tell people there is an error but continue. I wouldn't consider
configuration
> policy to be a reliable security measure, so it's OK to just ignore invalid
> policies and continue. I think it's fair to expect admins to test their
policies
> after they have defined them to check whether they have the desired effect.

My concern was that some user might find a way to modify or delete a policy
file, but we shouldn't hold ourselves responsible for that.

> I'm unsure about error reporting though, what alternatives do we have here?
> Admittedly logging is very minimal solution, do you think it's worth reporting
> policy errors in an info bar? I think for now, logging will do it and we can
> improve on that once we have indication logging is insufficient.

Sounds good. Emailing the admin is one wild idea, but since I don't know how
we'd do that any more reliably, nor whether it's even worth it, it's not a
serious suggestion now.

We should ping Glenn and see if he has any ideas gleaned from his talks with
enterprise admins, but that doesn't need to block this initial implementation.

http://codereview.chromium.org/2027010/diff/17001/18006
File chrome/browser/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/2027010/diff/17001/18006#newcode101
chrome/browser/config_dir_policy_provider_unittest.cc:101:
WriteConfigFile(test_dict_bar, IntToString(i));
Why two loops? Also, doesn't this overwrite the "5" file saved just above?

Consider writing some of them out of lexicographic order. It's not much of an
improvement on the test, but at least then it's meaningful if the filesystem
returns them in order of last modification.

http://codereview.chromium.org/2027010/diff/17001/18008
File chrome/browser/configuration_policy_provider.h (right):

http://codereview.chromium.org/2027010/diff/17001/18008#newcode39
chrome/browser/configuration_policy_provider.h:39: ConfigurationPolicyStore*
store);
Could this be a method of the ConfigurationPolicyProvider base class?

Thiago would tell you you don't need the || around the class name; it would be
customary if you were writing |store| (i.e., using the parameter name rather
than its type). Personally, I don't much care either way.

[Mattias Nissler (ping if slow), 2010-05-14 10:03:00]

Next round of fixes :)

http://codereview.chromium.org/2027010/diff/2001/3004
File chrome/browser/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/2027010/diff/2001/3004#newcode46
chrome/browser/config_dir_policy_provider.cc:46: continue;
On 2010/05/14 09:24:10, Pam wrote:
> On 2010/05/12 16:34:06, mnissler wrote:
> > Basically, we have two choices here when we find an error: Abort chrome
> startup
> > or tell people there is an error but continue. I wouldn't consider
> configuration
> > policy to be a reliable security measure, so it's OK to just ignore invalid
> > policies and continue. I think it's fair to expect admins to test their
> policies
> > after they have defined them to check whether they have the desired effect.
> 
> My concern was that some user might find a way to modify or delete a policy
> file, but we shouldn't hold ourselves responsible for that.

True. And if they manage to write system policy files, they'll probably also be
able to run custom chrome binaries :-)

> 
> > I'm unsure about error reporting though, what alternatives do we have here?
> > Admittedly logging is very minimal solution, do you think it's worth
reporting
> > policy errors in an info bar? I think for now, logging will do it and we can
> > improve on that once we have indication logging is insufficient.
> 
> Sounds good. Emailing the admin is one wild idea, but since I don't know how
> we'd do that any more reliably, nor whether it's even worth it, it's not a
> serious suggestion now.
> 
> We should ping Glenn and see if he has any ideas gleaned from his talks with
> enterprise admins, but that doesn't need to block this initial implementation.

I'll send him an email with chrome-enterprise@ CCed.

http://codereview.chromium.org/2027010/diff/17001/18006
File chrome/browser/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/2027010/diff/17001/18006#newcode101
chrome/browser/config_dir_policy_provider_unittest.cc:101:
WriteConfigFile(test_dict_bar, IntToString(i));
On 2010/05/14 09:24:10, Pam wrote:
> Why two loops? Also, doesn't this overwrite the "5" file saved just above?
> 
> Consider writing some of them out of lexicographic order. It's not much of an
> improvement on the test, but at least then it's meaningful if the filesystem
> returns them in order of last modification.

You're right, this code is totally broken, sorry. I meant to have it write some
of the haystack before the needle and some more after.

http://codereview.chromium.org/2027010/diff/17001/18008
File chrome/browser/configuration_policy_provider.h (right):

http://codereview.chromium.org/2027010/diff/17001/18008#newcode39
chrome/browser/configuration_policy_provider.h:39: ConfigurationPolicyStore*
store);
On 2010/05/14 09:24:10, Pam wrote:
> Could this be a method of the ConfigurationPolicyProvider base class?

It could, but I intentionally kept it seperate due to my long discussion with
Danno on whether we actually want to use common naming and structure for all
platforms. Current status of the discussion is to first implement this
infrastructure seperately and use it only for the Linux bits and eventually
refactor when we have a clear picture (i.e. the other policy provider
implementations in place).
 
> Thiago would tell you you don't need the || around the class name; it would be
> customary if you were writing |store| (i.e., using the parameter name rather
> than its type). Personally, I don't much care either way.

Done.

[Pam (message me for reviews), 2010-05-14 10:06:03]

LGTM.

- Pam

[tfarina, 2010-05-14 16:15:43]

LGTM2!

-Thiago