Refactor FileBasedPolicyProvider, introduce AsynchronousPolicyProvider.

Refactor FileBasedPolicyProvider, introduce AsynchronousPolicyProvider.

Create a superclass of FileBasedPolicyProvider that abstracts how a provider can provide policy on the UI thread that is loaded from the FILE thread.

BUG=65094
TEST=AsynchronousPolicyProvider.*

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

[danno, 2010-12-02 15:49:05]

please take a look.

[battre (please use the other), 2010-12-02 17:40:24]

Thanks for sharing. I learned a lot.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:23: int
reload_interval_minutes);
Should we make |reload_interval_minutes| a TimeDelta?

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:41: // FileBasedPolicyLoader
objects should only be deleted by
Is this a copy and paste error? The header file contains no second occurrence of
FileBasedPolicyLoader.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:84: const int
reload_interval_minutes_;
Should we make this a TimeDelta?

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:21: // Policy provider that
loads policy asynchronously. Providers subclass should
Nit: singular/plural in "Providers subclass"?

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:45: // it was passed during
Init at the very latest during this call.
Is it guaranteed that Stop() is called, i.e. the observer gets deleted?

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:53: // false and the delay
until it is considered safe to reload in |delay|.
Nit: "then" "delays"

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:86: const int
settle_interval_seconds_;
TimeDelta?

http://codereview.chromium.org/5562002/diff/17001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/5562002/diff/17001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:7: 'pyautolib_sources': [
Nit: I think this is a mistake

[Mattias Nissler (ping if slow), 2010-12-02 18:15:59]

some initial comments, will make a more thorough pass tomorrow morning.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:38: // Check the directory
time in order to see whether a reload is required.
Outdated comment?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:41: if
(!delegate_->IsSafeToReloadPolicy(now, &delay)) {
Does IsSafeToReloadPolicy make sense for group policy?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:42:
ScheduleReloadTask(delay);
Does the periodic reload task make sense for group policy?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:21:
base::WeakPtr<ConfigurationPolicyProvider> provider,
I thought the idea was to change the notification interface between provider and
ConfigurationPolicyPrefStore into a observer-based mechanism, so we wouldn't
need the WeakPtr hack here. Is that planned for a different CL?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:41: // FileBasedPolicyLoader
objects should only be deleted by
newline before comment?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:67: // Protects |policy_|.
Is that still necessary? I thought we decided to access |policy_| only from the
UI thread, and the lock should go away in that case.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider.cc (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.cc:15: const int
kReloadIntervalMinutes = 15;
Is the hardcoded interval appropriate for this very generic interface?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:22: // subclass from this
class if loading the policy requires disk access or must
subclass should subclass?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:64: const
PolicyDefinitionList* policy_list,
fits previous line?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider.cc (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.cc:51: observer_.reset(NULL);
No need to pass the NULL literal, you can just use reset()

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:31: class WatcherDelegate :
public FilePathWatcher::Delegate {
Seems like you can declare this in the implementation file.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:60: void
Init(AsynchronousPolicyProvider::PolicyChangeObserver* observer);
Missing a marker what is overriden from the Delegate interface. Also make them
virtual (doesn't make a difference but seems to be common style)?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:79: FilePathWatcher watcher_;
Another watcher?

[danno, 2010-12-03 17:05:38]

Please have another look.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:23: int
reload_interval_minutes);
On 2010/12/02 17:40:25, battre wrote:
> Should we make |reload_interval_minutes| a TimeDelta?

Internally, it's now a TimeDelta, but it's much nicer with the constants if the
passed in value is an int.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:41: // FileBasedPolicyLoader
objects should only be deleted by
On 2010/12/02 17:40:25, battre wrote:
> Is this a copy and paste error? The header file contains no second occurrence
of
> FileBasedPolicyLoader.

Done.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:84: const int
reload_interval_minutes_;
On 2010/12/02 17:40:25, battre wrote:
> Should we make this a TimeDelta?

Done.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:21: // Policy provider that
loads policy asynchronously. Providers subclass should
On 2010/12/02 17:40:25, battre wrote:
> Nit: singular/plural in "Providers subclass"?

Done.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:45: // it was passed during
Init at the very latest during this call.
On 2010/12/02 17:40:25, battre wrote:
> Is it guaranteed that Stop() is called, i.e. the observer gets deleted?

Done.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:53: // false and the delay
until it is considered safe to reload in |delay|.
On 2010/12/02 17:40:25, battre wrote:
> Nit: "then" "delays"

Done.

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/17001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:86: const int
settle_interval_seconds_;
On 2010/12/02 17:40:25, battre wrote:
> TimeDelta?

Done.

http://codereview.chromium.org/5562002/diff/17001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/5562002/diff/17001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:7: 'pyautolib_sources': [
On 2010/12/02 17:40:25, battre wrote:
> Nit: I think this is a mistake

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:38: // Check the directory
time in order to see whether a reload is required.
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> Outdated comment?

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:41: if
(!delegate_->IsSafeToReloadPolicy(now, &delay)) {
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> Does IsSafeToReloadPolicy make sense for group policy?

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:42:
ScheduleReloadTask(delay);
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> Does the periodic reload task make sense for group policy?

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:21:
base::WeakPtr<ConfigurationPolicyProvider> provider,
There's plenty left to do, this work is only the first step. Refactoring the
windows provider to work like the others is the next, and then changing the
notification interface. I'll add a TODO.
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> I thought the idea was to change the notification interface between provider
and
> ConfigurationPolicyPrefStore into a observer-based mechanism, so we wouldn't
> need the WeakPtr hack here. Is that planned for a different CL?

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:41: // FileBasedPolicyLoader
objects should only be deleted by
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> newline before comment?

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:67: // Protects |policy_|.
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> Is that still necessary? I thought we decided to access |policy_| only from
the
> UI thread, and the lock should go away in that case.

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider.cc (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.cc:15: const int
kReloadIntervalMinutes = 15;
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> Is the hardcoded interval appropriate for this very generic interface?

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:22: // subclass from this
class if loading the policy requires disk access or must
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> subclass should subclass?

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:64: const
PolicyDefinitionList* policy_list,
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> fits previous line?

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider.cc (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.cc:51: observer_.reset(NULL);
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> No need to pass the NULL literal, you can just use reset()

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:31: class WatcherDelegate :
public FilePathWatcher::Delegate {
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> Seems like you can declare this in the implementation file.

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:60: void
Init(AsynchronousPolicyProvider::PolicyChangeObserver* observer);
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> Missing a marker what is overriden from the Delegate interface. Also make them
> virtual (doesn't make a difference but seems to be common style)?

Done.

http://codereview.chromium.org/5562002/diff/22001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:79: FilePathWatcher watcher_;
On 2010/12/02 18:16:00, Mattias Nissler wrote:
> Another watcher?

Done.

[Mattias Nissler (ping if slow), 2010-12-06 10:26:20]

overall approach looks good, but lots of nits.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:16:
origin_loop_(MessageLoop::current()) {}
Move closing brace to next line for scanability :)

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:32: // Manages the life
cycle of a new policy while is pending on the message
What is pending?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:17: // Used by the
implementation of asynchronous policy provider to managed the
to manage

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:18: // tasks on the file
thread that do the heavy lifting of loading policies on the
"on the file thread" twice?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:26: // Triggers initial
policy load and schedules fallback reload.
In the base implementation, it doesn't schedule fallback reload.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:38: void
SetProvider(base::WeakPtr<AsynchronousPolicyProvider> provider);
If there is SetProvider(), you don't need to pass a WeakPtr, since you can just
do SetProvider(NULL) when the provider goes away.

What's the plan for the next iteration? Remove this? If so, please put a TODO.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:54: // Replaces the existing
policy to value map with a new once, sending
s/once/one/

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:56: // the ui thread.
s/ui/UI/ (you say FILE thread above)

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:79: MessageLoop*
origin_loop_;
Do we still need this? If yes, the comment above concerning the UI thread is not
precise.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader_unittest.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader_unittest.cc:5: #include
"chrome/browser/browser_thread.h"
Do you need that header?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader_unittest.cc:21: const int
kReloadIntervalMinutesForTesting = 1;
This is unused.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:10: #include "base/time.h"
do you need this header?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:14: class CancelableTask;
not needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:15: class MessageLoop;
not needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:39:
DISALLOW_COPY_AND_ASSIGN(Delegate);
I think technically you don't need DISALLOW_COPY_AND_ASSIGN in pure interfaces.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:42: // Assumes ownership of
|delegate|.
s/delegate/loader/

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:51: // For tests to trigger
reloads
missing period. If tests construct the provider themselves, they have a loader
pointer, so why do we need this?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:55: // The loader object
used internally.
Indentation.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider_unittest.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:5: #include
"base/message_loop.h"
Don't need this header, since you get loop_ from the parent class.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_test_base.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:10: #include "base/time.h"
do you need this header?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:13: #include
"chrome/browser/policy/configuration_policy_provider.h"
isn't this one included in asynchronous_policy_provider.h anyway?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:54:
scoped_ptr<ConfigurationPolicyProvider> provider_;
Seems like the provider_ is not really needed here. So why not declare it in the
subclasses?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:64: bool
run_pending_during_tear_down_;
But there's no harm in always running it? So why not just get rid of the flag?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:72:
MessageLoop::current()->QuitNow();
Do you really want to call QuitNow() instead of Quit()?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider.h:25: // directory.  The files
should contain JSON-formatted policy settings. They are
nit: Decide on whether you want one or two spaces after a period and be
consistent.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider.h:26: // merged together and
the result is returned via the PolicyLoader
s/PolicyLoader/ProviderDelegate/

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_mac.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_mac.h:16: // A provider
delegate implementation that read Mac OS X's managed preferences.
s/read/reads/

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_loader.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.cc:10: // it. This alleviates the
problem of reading partially written files and allows
s/it/them/

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.cc:57: // threading is up and
schedule watch initialization on the file thread.
Didn't we post on the UI thread before? That was intentional, since if we post
before the FILE thread exists, the task gets discarded.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.cc:113: OnError();
need curlies here.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_loader.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.h:68: scoped_ptr<FilePathWatcher>
watcher_;
Can you comment on why we need a scoped_ptr to wrap this and note the ownership
subtleties here?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:11: #include "base/lock.h"
not needed.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:12: #include
"base/ref_counted.h"
not needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:13: #include
"base/scoped_ptr.h"
not Needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:15: #include
"base/weak_ptr.h"
not needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:16: #include
"chrome/browser/file_path_watcher.h"
not needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:18: #include
"chrome/browser/policy/configuration_policy_provider.h"
don't need that header.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:5: #include
"base/file_util.h"
not needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:6: #include
"base/scoped_temp_dir.h"
not needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:8: #include
"chrome/browser/policy/asynchronous_policy_provider.h"
Needed? You get that include from file_based_policy_provider anyways.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:11: #include
"chrome/browser/policy/configuration_policy_store_interface.h"
needed? You include the mock below and AFAICS, you only use that.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:21: using
testing::Mock;
you don't use this.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:46:
EXPECT_CALL(*provider_delegate, Load()).WillOnce(Return(policies));
Why do you install two expected calls on provider_delegate? Can you put a
comment explaining that?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:76:
provider_->Provide(store_.get());
Does this trigger the second EXPECT_CALL(*provider_delegate, ...) from above? If
so, can we move it here so it's easier to understand our expectations?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/mock_...
chrome/browser/policy/mock_configuration_policy_provider.h:16: #include
"testing/gtest/include/gtest/gtest.h"
Is this include really needed?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/mock_...
chrome/browser/policy/mock_configuration_policy_provider.h:24: public
base::SupportsWeakPtr<MockConfigurationPolicyProvider> {
You can probably remove the SupportsWeakPtr declaration here if we end up not
using it because of SetProvider().

http://codereview.chromium.org/5562002/diff/57001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1415:
'browser/policy/asynchronous_policy_provider_unittest.cc',
how about the asynchronous_policy_test_base.h?

[danno, 2010-12-06 14:05:12]

addressed feedback, please take another look.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:16:
origin_loop_(MessageLoop::current()) {}
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Move closing brace to next line for scanability :)
Actually, in my readability review, my reviewer suggested being consistent
everywhere with null bodies, recommending {} over {\n}. In headers {} is used
frequently. So, upshot: I'm not sure that I agree that adding the newline makes
it more readable.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:32: // Manages the life
cycle of a new policy while is pending on the message
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> What is pending?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:17: // Used by the
implementation of asynchronous policy provider to managed the
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> to manage

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:18: // tasks on the file
thread that do the heavy lifting of loading policies on the
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> "on the file thread" twice?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:26: // Triggers initial
policy load and schedules fallback reload.
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> In the base implementation, it doesn't schedule fallback reload.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:38: void
SetProvider(base::WeakPtr<AsynchronousPolicyProvider> provider);
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> If there is SetProvider(), you don't need to pass a WeakPtr, since you can
just
> do SetProvider(NULL) when the provider goes away.
> 
> What's the plan for the next iteration? Remove this? If so, please put a TODO.

Done. TODO already in .cc file.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:54: // Replaces the existing
policy to value map with a new once, sending
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> s/once/one/

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:56: // the ui thread.
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> s/ui/UI/ (you say FILE thread above)

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:79: MessageLoop*
origin_loop_;
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Do we still need this? If yes, the comment above concerning the UI thread is
not
> precise.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader_unittest.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader_unittest.cc:5: #include
"chrome/browser/browser_thread.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Do you need that header?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader_unittest.cc:21: const int
kReloadIntervalMinutesForTesting = 1;
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> This is unused.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:10: #include "base/time.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> do you need this header?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:14: class CancelableTask;
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:15: class MessageLoop;
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:39:
DISALLOW_COPY_AND_ASSIGN(Delegate);
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> I think technically you don't need DISALLOW_COPY_AND_ASSIGN in pure
interfaces.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:42: // Assumes ownership of
|delegate|.
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> s/delegate/loader/

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:51: // For tests to trigger
reloads
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> missing period. If tests construct the provider themselves, they have a loader
> pointer, so why do we need this?

One the AsynchronousPolicyProvider allows the loader to be passed in. Subclasses
do not and their tests need access.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider.h:55: // The loader object
used internally.
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Indentation.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider_unittest.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:5: #include
"base/message_loop.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Don't need this header, since you get loop_ from the parent class.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_test_base.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:10: #include "base/time.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> do you need this header?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:13: #include
"chrome/browser/policy/configuration_policy_provider.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> isn't this one included in asynchronous_policy_provider.h anyway?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:54:
scoped_ptr<ConfigurationPolicyProvider> provider_;
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Seems like the provider_ is not really needed here. So why not declare it in
the
> subclasses?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:64: bool
run_pending_during_tear_down_;
This is dead code now.

On 2010/12/06 10:26:20, Mattias Nissler wrote:
> But there's no harm in always running it? So why not just get rid of the flag?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:72:
MessageLoop::current()->QuitNow();
Yes. But this functionality isn't used anymore after the refactor. 
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Do you really want to call QuitNow() instead of Quit()?

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider.h:25: // directory.  The files
should contain JSON-formatted policy settings. They are
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> nit: Decide on whether you want one or two spaces after a period and be
> consistent.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider.h:26: // merged together and
the result is returned via the PolicyLoader
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> s/PolicyLoader/ProviderDelegate/

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_mac.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_mac.h:16: // A provider
delegate implementation that read Mac OS X's managed preferences.
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> s/read/reads/

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_loader.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.cc:10: // it. This alleviates the
problem of reading partially written files and allows
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> s/it/them/

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.cc:57: // threading is up and
schedule watch initialization on the file thread.
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Didn't we post on the UI thread before? That was intentional, since if we post
> before the FILE thread exists, the task gets discarded.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.cc:113: OnError();
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> need curlies here.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_loader.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.h:68: scoped_ptr<FilePathWatcher>
watcher_;
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Can you comment on why we need a scoped_ptr to wrap this and note the
ownership
> subtleties here?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:11: #include "base/lock.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not needed.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:12: #include
"base/ref_counted.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:13: #include
"base/scoped_ptr.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not Needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:15: #include
"base/weak_ptr.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:16: #include
"chrome/browser/file_path_watcher.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider.h:18: #include
"chrome/browser/policy/configuration_policy_provider.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> don't need that header.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:5: #include
"base/file_util.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:6: #include
"base/scoped_temp_dir.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> not needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:8: #include
"chrome/browser/policy/asynchronous_policy_provider.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Needed? You get that include from file_based_policy_provider anyways.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:11: #include
"chrome/browser/policy/configuration_policy_store_interface.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> needed? You include the mock below and AFAICS, you only use that.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:21: using
testing::Mock;
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> you don't use this.

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:46:
EXPECT_CALL(*provider_delegate, Load()).WillOnce(Return(policies));
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Why do you install two expected calls on provider_delegate? Can you put a
> comment explaining that?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:76:
provider_->Provide(store_.get());
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Does this trigger the second EXPECT_CALL(*provider_delegate, ...) from above?
If
> so, can we move it here so it's easier to understand our expectations?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/mock_...
chrome/browser/policy/mock_configuration_policy_provider.h:16: #include
"testing/gtest/include/gtest/gtest.h"
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> Is this include really needed?

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/browser/policy/mock_...
chrome/browser/policy/mock_configuration_policy_provider.h:24: public
base::SupportsWeakPtr<MockConfigurationPolicyProvider> {
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> You can probably remove the SupportsWeakPtr declaration here if we end up not
> using it because of SetProvider().

Done.

http://codereview.chromium.org/5562002/diff/57001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/5562002/diff/57001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1415:
'browser/policy/asynchronous_policy_provider_unittest.cc',
On 2010/12/06 10:26:20, Mattias Nissler wrote:
> how about the asynchronous_policy_test_base.h?

Done.

[Mattias Nissler (ping if slow), 2010-12-06 14:26:06]

LGTM.

[danno, 2010-12-09 08:54:59]

please review deltas, I had to make some changes to get the tests to work again.

[Mattias Nissler (ping if slow), 2010-12-09 09:27:17]

LGTM with my two comments addressed.

http://codereview.chromium.org/5562002/diff/61002/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/5562002/diff/61002/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:23:
DCHECK_EQ(MessageLoop::current(), origin_loop_);
Why did you remove this?

http://codereview.chromium.org/5562002/diff/72002/chrome/browser/policy/confi...
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/5562002/diff/72002/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider_unittest.cc:31:
file_util::Delete(test_dir_.path(), true);
This is not necessary, since ScopedTempDir deletes itself automagically.

[danno, 2010-12-09 10:23:01]

http://codereview.chromium.org/5562002/diff/61002/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/5562002/diff/61002/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:23:
DCHECK_EQ(MessageLoop::current(), origin_loop_);
On 2010/12/09 09:27:17, Mattias Nissler wrote:
> Why did you remove this?

For the singleton policy provider in the real app, the last reference to the
load is released after the message loop is already destroyed and when this
erroneously fires.

http://codereview.chromium.org/5562002/diff/72002/chrome/browser/policy/confi...
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/5562002/diff/72002/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider_unittest.cc:31:
file_util::Delete(test_dir_.path(), true);
You're looking at an old version. This code is not in there anymore.
 On 2010/12/09 09:27:17, Mattias Nissler wrote:
> This is not necessary, since ScopedTempDir deletes itself automagically.

[Mattias Nissler (ping if slow), 2010-12-09 10:23:49]

LGTM then.