Refactor FileBasedPolicyProvider, introduce AsynchronousPolicyProvider.

chrome/browser/policy/file_based_policy_provider.cc

Index: chrome/browser/policy/file_based_policy_provider.cc
diff --git a/chrome/browser/policy/file_based_policy_provider.cc b/chrome/browser/policy/file_based_policy_provider.cc
index 578277d61d78f22ce4e529f8adb1f972b75cb4b8..c913c9d2801574830d816d1d663a290d5b129937 100644
--- a/chrome/browser/policy/file_based_policy_provider.cc
+++ b/chrome/browser/policy/file_based_policy_provider.cc
@@ -14,146 +14,71 @@
 #include "chrome/browser/browser_thread.h"
 #include "chrome/common/json_value_serializer.h"
 
-namespace policy {
+namespace {
 
 // Amount of time we wait for the files on disk to settle before trying to load
 // it. This alleviates the problem of reading partially written files and allows
 // to batch quasi-simultaneous changes.
 const int kSettleIntervalSeconds = 5;
 
-// The time interval for rechecking policy. This is our fallback in case the
-// file path watch fails or doesn't report a change.
-const int kReloadIntervalMinutes = 15;
-
-// FileBasedPolicyProvider implementation:
-
-FileBasedPolicyProvider::Delegate::~Delegate() {
-}
-
-FileBasedPolicyProvider::Delegate::Delegate(const FilePath& config_file_path)
-  : config_file_path_(config_file_path) {
-}
-
-FileBasedPolicyProvider::FileBasedPolicyProvider(
-    const ConfigurationPolicyProvider::PolicyDefinitionList* policy_list,
-    FileBasedPolicyProvider::Delegate* delegate)
-    : ConfigurationPolicyProvider(policy_list) {
-  loader_ = new FileBasedPolicyLoader(AsWeakPtr(),
-                                      delegate,
-                                      kSettleIntervalSeconds,
-                                      kReloadIntervalMinutes);
-  watcher_ = new FileBasedPolicyWatcher;
-  watcher_->Init(loader_.get());
-}
-
-FileBasedPolicyProvider::~FileBasedPolicyProvider() {
-  loader_->Stop();
-}
-
-bool FileBasedPolicyProvider::Provide(
-    ConfigurationPolicyStoreInterface* store) {
-  scoped_ptr<DictionaryValue> policy(loader_->GetPolicy());
-  DCHECK(policy.get());
-  DecodePolicyValueTree(policy.get(), store);
-  return true;
-}
-
-// FileBasedPolicyLoader implementation:
-
-FileBasedPolicyLoader::FileBasedPolicyLoader(
-    base::WeakPtr<ConfigurationPolicyProvider> provider,
-    FileBasedPolicyProvider::Delegate* delegate,
-    int settle_interval_seconds,
-    int reload_interval_minutes)
-    : delegate_(delegate),
-      provider_(provider),
-      origin_loop_(MessageLoop::current()),
-      reload_task_(NULL),
-      settle_interval_seconds_(settle_interval_seconds),
-      reload_interval_minutes_(reload_interval_minutes) {
-  // Force an initial load, so GetPolicy() works.
-  policy_.reset(delegate_->Load());
-  DCHECK(policy_.get());
 }
 
-void FileBasedPolicyLoader::Stop() {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
-    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(this, &FileBasedPolicyLoader::Stop));
-    return;
-  }
+namespace policy {
 
-  if (reload_task_) {
-    reload_task_->Cancel();
-    reload_task_ = NULL;
-  }
+FileBasedPolicyProvider::WatcherDelegate::WatcherDelegate(
+    const FilePath& config_file_path,
+    AsynchronousPolicyProvider::PolicyChangeObserver* observer)
+    : config_file_path_(config_file_path),
+      observer_(observer) {
 }
 
-void FileBasedPolicyLoader::Reload() {
+void FileBasedPolicyProvider::WatcherDelegate::Init() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  watcher_.reset(new FilePathWatcher());
+  if (!config_file_path_.empty() &&
+      !watcher_->Watch(config_file_path_, this))
+    OnError();
 
-  // Check the directory time in order to see whether a reload is required.
-  base::TimeDelta delay;
-  base::Time now = base::Time::Now();
-  if (!IsSafeToReloadPolicy(now, &delay)) {
-    ScheduleReloadTask(delay);
-    return;
-  }
-
-  // Load the policy definitions.
-  scoped_ptr<DictionaryValue> new_policy(delegate_->Load());
-
-  // Check again in case the directory has changed while reading it.
-  if (!IsSafeToReloadPolicy(now, &delay)) {
-    ScheduleReloadTask(delay);
-    return;
-  }
-
-  // Replace policy definition.
-  bool changed = false;
-  {
-    AutoLock lock(lock_);
-    changed = !policy_->Equals(new_policy.get());
-    policy_.reset(new_policy.release());
-  }
-
-  // There's a change, report it!
-  if (changed) {
-    VLOG(0) << "Policy reload from " << config_file_path().value()
-            << " succeeded.";
-    origin_loop_->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &FileBasedPolicyLoader::NotifyPolicyChanged));
-  }
-
-  // As a safeguard in case the file watcher fails, schedule a reload task
-  // that'll make us recheck after a reasonable interval.
-  ScheduleReloadTask(base::TimeDelta::FromMinutes(reload_interval_minutes_));
+  // There might have been changes to the directory in the time between
+  // construction of the loader and initialization of the watcher. Call reload
+  // to detect if that is the case.
+  observer_->OnPolicyChange();
 }
 
-DictionaryValue* FileBasedPolicyLoader::GetPolicy() {
-  AutoLock lock(lock_);
-  return static_cast<DictionaryValue*>(policy_->DeepCopy());
+void FileBasedPolicyProvider::WatcherDelegate::Stop() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  watcher_.reset(NULL);
+  observer_.reset(NULL);

[Mattias Nissler (ping if slow), 2010/12/02 18:16:00]

No need to pass the NULL literal, you can just use reset()

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

Done.

 }
 
-void FileBasedPolicyLoader::OnFilePathChanged(const FilePath& path) {
+void FileBasedPolicyProvider::WatcherDelegate::OnFilePathChanged(
+    const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  Reload();
+  if (observer_.get())
+    observer_->OnPolicyChange();
 }
 
-void FileBasedPolicyLoader::OnError() {
-  LOG(ERROR) << "FilePathWatcher on " << config_file_path().value()
+void FileBasedPolicyProvider::WatcherDelegate::OnError() {
+  LOG(ERROR) << "FilePathWatcher on " << config_file_path_.value()
              << " failed.";
 }
 
-FileBasedPolicyLoader::~FileBasedPolicyLoader() {
+FileBasedPolicyProvider::ProviderDelegate::ProviderDelegate(
+    const FilePath& config_file_path)
+    : config_file_path_(config_file_path),
+      settle_interval_seconds_(kSettleIntervalSeconds){
 }
 
-bool FileBasedPolicyLoader::IsSafeToReloadPolicy(const base::Time& now,
-                                                 base::TimeDelta* delay) {
+FileBasedPolicyProvider::ProviderDelegate::~ProviderDelegate() {
+}
+
+bool FileBasedPolicyProvider::ProviderDelegate::IsSafeToReloadPolicy(
+    const base::Time& now,
+    base::TimeDelta* delay) {
   DCHECK(delay);
 
   // A null modification time indicates there's no data.
-  base::Time last_modification(delegate_->GetLastModification());
+  base::Time last_modification(GetLastModification());
   if (last_modification.is_null())
     return true;
 
@@ -177,68 +102,32 @@ bool FileBasedPolicyLoader::IsSafeToReloadPolicy(const base::Time& now,
   return true;
 }
 
-void FileBasedPolicyLoader::ScheduleReloadTask(const base::TimeDelta& delay) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  if (reload_task_)
-    reload_task_->Cancel();
-
-  reload_task_ =
-      NewRunnableMethod(this, &FileBasedPolicyLoader::ReloadFromTask);
-  BrowserThread::PostDelayedTask(BrowserThread::FILE, FROM_HERE, reload_task_,
-                                 delay.InMilliseconds());
-}
-
-void FileBasedPolicyLoader::NotifyPolicyChanged() {
-  DCHECK_EQ(origin_loop_, MessageLoop::current());
-  if (provider_)
-    provider_->NotifyStoreOfPolicyChange();
-}
-
-void FileBasedPolicyLoader::ReloadFromTask() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  // Drop the reference to the reload task, since the task might be the only
-  // referer that keeps us alive, so we should not Cancel() it.
-  reload_task_ = NULL;
-
-  Reload();
-}
-
-// FileBasedPolicyWatcher implementation:
+void FileBasedPolicyProvider::ProviderDelegate::Init(
+    AsynchronousPolicyProvider::PolicyChangeObserver* observer) {
+  watcher_delegate_ = new FileBasedPolicyProvider::WatcherDelegate(
+      config_file_path_,
+      observer);
 
-FileBasedPolicyWatcher::FileBasedPolicyWatcher() {
+  // Initialization can happen early when the file thread is not
+  // yet available.  So post a task on the FILE thread which will run after
+  // threading is up and schedule watch initialization on the file thread.
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      NewRunnableMethod(watcher_delegate_.get(),
+                        &FileBasedPolicyProvider::WatcherDelegate::Init));
 }
 
-void FileBasedPolicyWatcher::Init(FileBasedPolicyLoader* loader) {
-  // Initialization can happen early when the file thread is not yet available.
-  // So post a task to ourselves on the UI thread which will run after threading
-  // is up and schedule watch initialization on the file thread.
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(this,
-                        &FileBasedPolicyWatcher::InitWatcher,
-                        scoped_refptr<FileBasedPolicyLoader>(loader)));
+void FileBasedPolicyProvider::ProviderDelegate::Stop() {
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      NewRunnableMethod(watcher_delegate_.get(),
+                        &FileBasedPolicyProvider::WatcherDelegate::Stop));
 }
 
-FileBasedPolicyWatcher::~FileBasedPolicyWatcher() {
-}
-
-void FileBasedPolicyWatcher::InitWatcher(
-    const scoped_refptr<FileBasedPolicyLoader>& loader) {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
-    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(this, &FileBasedPolicyWatcher::InitWatcher, loader));
-    return;
-  }
-
-  if (!loader->config_file_path().empty() &&
-      !watcher_.Watch(loader->config_file_path(), loader.get()))
-    loader->OnError();
-
-  // There might have been changes to the directory in the time between
-  // construction of the loader and initialization of the watcher. Call reload
-  // to detect if that is the case.
-  loader->Reload();
+FileBasedPolicyProvider::FileBasedPolicyProvider(
+    const ConfigurationPolicyProvider::PolicyDefinitionList* policy_list,
+    FileBasedPolicyProvider::ProviderDelegate* delegate)
+    : AsynchronousPolicyProvider(policy_list, delegate) {
 }
 
 }  // namespace policy