Revert 56832 - Support change detection and reloading in ConfigDirPolicyProvider.

BUG=52418
TEST=unit tests

Review URL: http://codereview.chromium.org/3124025

TBR=mnissler@chromium.org
Review URL: http://codereview.chromium.org/3161035

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@56842 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 21 insertions, 414 deletions

diff --git a/chrome/browser/policy/config_dir_policy_provider.cc b/chrome/browser/policy/config_dir_policy_provider.cc
index 8c64efa..59509fc 100644
--- a/chrome/browser/policy/config_dir_policy_provider.cc
+++ b/chrome/browser/policy/config_dir_policy_provider.cc
@@ -8,109 +8,22 @@
 
 #include "base/file_util.h"
 #include "base/logging.h"
+#include "base/scoped_ptr.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/common/json_value_serializer.h"
 
-namespace {
-
-// Amount of time we wait for the files in the policy directory 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
-// directory watch fails or doesn't report a change.
-const int kReloadIntervalMinutes = 15;
-
-}  // namespace
-
-// PolicyDirLoader implementation:
-
-PolicyDirLoader::PolicyDirLoader(
-    base::WeakPtr<ConfigDirPolicyProvider> provider,
-    const FilePath& config_dir,
-    int settle_interval_seconds,
-    int reload_interval_minutes)
-    : provider_(provider),
-      origin_loop_(MessageLoop::current()),
-      config_dir_(config_dir),
-      reload_task_(NULL),
-      settle_interval_seconds_(settle_interval_seconds),
-      reload_interval_minutes_(reload_interval_minutes) {
-  // Force an initial load, so GetPolicy() works.
-  policy_.reset(Load());
-  DCHECK(policy_.get());
+ConfigDirPolicyProvider::ConfigDirPolicyProvider(const FilePath& config_dir)
+    : config_dir_(config_dir) {
 }
 
-void PolicyDirLoader::Stop() {
-  if (!ChromeThread::CurrentlyOn(ChromeThread::FILE)) {
-    ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
-                           NewRunnableMethod(this, &PolicyDirLoader::Stop));
-    return;
-  }
-
-  if (reload_task_) {
-    reload_task_->Cancel();
-    reload_task_ = NULL;
-  }
-}
-
-void PolicyDirLoader::Reload() {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-
-  // Check the directory time in order to see whether a reload is required.
-  base::Time now = base::Time::Now();
-  base::TimeDelta delay;
-  if (!IsSafeToReloadPolicy(now, &delay)) {
-    ScheduleReloadTask(delay);
-    return;
-  }
-
-  // Load the policy definitions.
-  scoped_ptr<DictionaryValue> new_policy(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) {
-    LOG(INFO) << "Policy reload from " << config_dir_.value() << " succeeded.";
-    origin_loop_->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &PolicyDirLoader::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_));
-}
-
-DictionaryValue* PolicyDirLoader::GetPolicy() {
-  AutoLock lock(lock_);
-  return static_cast<DictionaryValue*>(policy_->DeepCopy());
-}
-
-void PolicyDirLoader::OnFilePathChanged(const FilePath& path) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-  Reload();
-}
-
-void PolicyDirLoader::OnError() {
-  LOG(ERROR) << "FileWatcher on " << config_dir_.value() << " failed.";
+bool ConfigDirPolicyProvider::Provide(ConfigurationPolicyStore* store) {
+  scoped_ptr<DictionaryValue> policy(ReadPolicies());
+  DecodePolicyValueTree(policy.get(), store);
+  return true;
 }
 
-DictionaryValue* PolicyDirLoader::Load() {
+DictionaryValue* ConfigDirPolicyProvider::ReadPolicies() {
   // Enumerate the files and sort them lexicographically.
   std::set<FilePath> files;
   file_util::FileEnumerator file_enumerator(config_dir_, false,
@@ -143,105 +56,6 @@ DictionaryValue* PolicyDirLoader::Load() {
   return policy;
 }
 
-bool PolicyDirLoader::IsSafeToReloadPolicy(const base::Time& now,
-                                           base::TimeDelta* delay) {
-  DCHECK(delay);
-  file_util::FileInfo dir_info;
-
-  // Reading an empty directory or a file is always safe.
-  if (!file_util::GetFileInfo(config_dir_, &dir_info) ||
-      !dir_info.is_directory) {
-    return true;
-  }
-
-  // Check for too recent changes.
-  base::TimeDelta settleInterval(
-      base::TimeDelta::FromSeconds(settle_interval_seconds_));
-  base::TimeDelta age = now - dir_info.last_modified;
-  if (age < settleInterval) {
-    *delay = settleInterval - age;
-    return false;
-  }
-
-  return true;
-}
-
-void PolicyDirLoader::ScheduleReloadTask(const base::TimeDelta& delay) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-
-  if (reload_task_)
-    reload_task_->Cancel();
-
-  reload_task_ = NewRunnableMethod(this, &PolicyDirLoader::ReloadFromTask);
-  ChromeThread::PostDelayedTask(ChromeThread::FILE, FROM_HERE, reload_task_,
-                                delay.InMilliseconds());
-}
-
-void PolicyDirLoader::NotifyPolicyChanged() {
-  DCHECK_EQ(origin_loop_, MessageLoop::current());
-  if (provider_)
-    provider_->NotifyStoreOfPolicyChange();
-}
-
-void PolicyDirLoader::ReloadFromTask() {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::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();
-}
-
-// PolicyDirWatcher implementation:
-
-void PolicyDirWatcher::Init(PolicyDirLoader* loader) {
-  // Initialization can happen early when the file thread is not yet available.
-  // So post a task to ourselves on th UI thread which will run after threading
-  // is up and schedule watch initialization on the file thread.
-  ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
-      NewRunnableMethod(this,
-                        &PolicyDirWatcher::InitWatcher,
-                        scoped_refptr<PolicyDirLoader>(loader)));
-}
-
-void PolicyDirWatcher::InitWatcher(
-    const scoped_refptr<PolicyDirLoader>& loader) {
-  if (!ChromeThread::CurrentlyOn(ChromeThread::FILE)) {
-    ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
-        NewRunnableMethod(this, &PolicyDirWatcher::InitWatcher, loader));
-    return;
-  }
-
-  if (!Watch(loader->config_dir(), 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();
-}
-
-// ConfigDirPolicyProvider implementation:
-
-ConfigDirPolicyProvider::ConfigDirPolicyProvider(const FilePath& config_dir) {
-  loader_ = new PolicyDirLoader(AsWeakPtr(), config_dir, kSettleIntervalSeconds,
-                                kReloadIntervalMinutes);
-  watcher_ = new PolicyDirWatcher;
-  watcher_->Init(loader_.get());
-}
-
-ConfigDirPolicyProvider::~ConfigDirPolicyProvider() {
-  loader_->Stop();
-}
-
-bool ConfigDirPolicyProvider::Provide(ConfigurationPolicyStore* store) {
-  scoped_ptr<DictionaryValue> policy(loader_->GetPolicy());
-  DCHECK(policy.get());
-  DecodePolicyValueTree(policy.get(), store);
-  return true;
-}
-
 void ConfigDirPolicyProvider::DecodePolicyValueTree(
     DictionaryValue* policies,
     ConfigurationPolicyStore* store) {
@@ -257,3 +71,4 @@ void ConfigDirPolicyProvider::DecodePolicyValueTree(
   // TODO(mnissler): Handle preference overrides once |ConfigurationPolicyStore|
   // supports it.
 }
+
diff --git a/chrome/browser/policy/config_dir_policy_provider.h b/chrome/browser/policy/config_dir_policy_provider.h
index d299423..9412ff2 100644
--- a/chrome/browser/policy/config_dir_policy_provider.h
+++ b/chrome/browser/policy/config_dir_policy_provider.h
@@ -8,111 +8,9 @@
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
-#include "base/lock.h"
-#include "base/ref_counted.h"
-#include "base/scoped_ptr.h"
-#include "base/weak_ptr.h"
-#include "chrome/browser/file_path_watcher.h"
 #include "chrome/browser/policy/configuration_policy_provider.h"
 
-class ConfigDirPolicyProvider;
 class DictionaryValue;
-class MessageLoop;
-
-// FilePathWatcher delegate implementation that handles change notifications for
-// the configuration directory. It keeps the authorative version of the
-// currently effective policy dictionary and updates it as appropriate.
-class PolicyDirLoader : public FilePathWatcher::Delegate {
- public:
-  // Create a new loader that'll load its data from |config_dir|.
-  PolicyDirLoader(base::WeakPtr<ConfigDirPolicyProvider> provider,
-                  const FilePath& config_dir,
-                  int settle_interval_second,
-                  int reload_interval_minutes);
-
-  // Stops any pending reload tasks.
-  void Stop();
-
-  // Reloads the policies and sends out a notification, if appropriate. Must be
-  // called on the file thread.
-  void Reload();
-
-  // Get the current dictionary value object. Ownership of the returned value is
-  // transferred to the caller.
-  DictionaryValue* GetPolicy();
-
-  const FilePath& config_dir() { return config_dir_; }
-
-  // FilePathWatcher::Delegate implementation:
-  void OnFilePathChanged(const FilePath& path);
-  void OnError();
-
- private:
-  // Load the policy information. Ownership of the return value is transferred
-  // to the caller.
-  DictionaryValue* Load();
-
-  // Check the directory modification time to see whether reading the
-  // configuration directory is safe. If not, returns false and the delay until
-  // it is considered safe to reload in |delay|.
-  bool IsSafeToReloadPolicy(const base::Time& now, base::TimeDelta* delay);
-
-  // Post a reload task. Must be called on the file thread.
-  void ScheduleReloadTask(const base::TimeDelta& delay);
-
-  // Notifies the policy provider to send out a policy changed notification.
-  // Must be called on |origin_loop_|.
-  void NotifyPolicyChanged();
-
-  // Invoked from the reload task on the file thread.
-  void ReloadFromTask();
-
-  // The provider this loader is associated with. Access only on the thread that
-  // called the constructor. See |origin_loop_| below.
-  base::WeakPtr<ConfigDirPolicyProvider> provider_;
-
-  // The message loop on which this object was constructed and |provider_|
-  // received on. Recorded so we can call back into the non thread safe provider
-  // to fire the notification.
-  MessageLoop* origin_loop_;
-
-  // The directory in which we look for configuration files.
-  const FilePath config_dir_;
-
-  // Protects |policy_|.
-  Lock lock_;
-
-  // The current policy definition.
-  scoped_ptr<DictionaryValue> policy_;
-
-  // The reload task. Access only on the file thread. Holds a reference to the
-  // currently posted task, so we can cancel and repost it if necessary.
-  CancelableTask* reload_task_;
-
-  // Settle and reload intervals.
-  const int settle_interval_seconds_;
-  const int reload_interval_minutes_;
-
-  DISALLOW_COPY_AND_ASSIGN(PolicyDirLoader);
-};
-
-// Wraps a FilePathWatcher for the configuration directory and takes care of
-// initializing the watcher object on the file thread.
-class PolicyDirWatcher : public FilePathWatcher,
-                         public base::RefCountedThreadSafe<PolicyDirWatcher> {
- public:
-  PolicyDirWatcher() {}
-
-  // Run initialization. This is in a separate method since we need to post a
-  // task (which cannot be done from the constructor).
-  void Init(PolicyDirLoader* loader);
-
- private:
-  // Actually sets up the watch with the FilePathWatcher code.
-  void InitWatcher(const scoped_refptr<PolicyDirLoader>& loader);
-
-  DISALLOW_COPY_AND_ASSIGN(PolicyDirWatcher);
-};
 
 // A policy provider implementation backed by a set of files in a given
 // directory. The files should contain JSON-formatted policy settings. They are
@@ -120,26 +18,24 @@ class PolicyDirWatcher : public FilePathWatcher,
 // ConfigurationPolicyProvider interface. The files are consulted in
 // lexicographic file name order, so the last value read takes precedence in
 // case of preference key collisions.
-class ConfigDirPolicyProvider
-    : public ConfigurationPolicyProvider,
-      public base::SupportsWeakPtr<ConfigDirPolicyProvider> {
+class ConfigDirPolicyProvider : public ConfigurationPolicyProvider {
  public:
   explicit ConfigDirPolicyProvider(const FilePath& config_dir);
-  virtual ~ConfigDirPolicyProvider();
+  virtual ~ConfigDirPolicyProvider() { }
 
   // ConfigurationPolicyProvider implementation.
   virtual bool Provide(ConfigurationPolicyStore* store);
 
  private:
+  // Read and merge the files from the configuration directory.
+  DictionaryValue* ReadPolicies();
+
   // Decodes the value tree and writes the configuration to the given |store|.
   void DecodePolicyValueTree(DictionaryValue* policies,
                              ConfigurationPolicyStore* store);
 
-  // Watches for changes to the configuration directory.
-  scoped_refptr<PolicyDirWatcher> watcher_;
-
-  // The loader object we use internally.
-  scoped_refptr<PolicyDirLoader> loader_;
+  // The directory in which we look for configuration files.
+  const FilePath config_dir_;
 
   DISALLOW_COPY_AND_ASSIGN(ConfigDirPolicyProvider);
 };
diff --git a/chrome/browser/policy/config_dir_policy_provider_unittest.cc b/chrome/browser/policy/config_dir_policy_provider_unittest.cc
index c7a2e94..9e3d25b 100644
--- a/chrome/browser/policy/config_dir_policy_provider_unittest.cc
+++ b/chrome/browser/policy/config_dir_policy_provider_unittest.cc
@@ -9,24 +9,9 @@
 #include "chrome/browser/policy/mock_configuration_policy_store.h"
 #include "chrome/common/json_value_serializer.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "testing/gmock/include/gmock/gmock.h"
 
-using testing::Mock;
-
-namespace {
-
-// Shorter reload intervals for testing PolicyDirWatcher.
-const int kSettleIntervalSecondsForTesting = 0;
-const int kReloadIntervalMinutesForTesting = 1;
-
-}  // namespace
-
-class ConfigDirPolicyProviderTestBase : public testing::Test {
+class ConfigDirPolicyProviderTest : public testing::Test {
  protected:
-  ConfigDirPolicyProviderTestBase()
-      : ui_thread_(ChromeThread::UI, &loop_),
-        file_thread_(ChromeThread::FILE, &loop_) {}
-
   virtual void SetUp() {
     // Determine the directory to use for testing.
     ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &test_dir_));
@@ -37,10 +22,12 @@ class ConfigDirPolicyProviderTestBase : public testing::Test {
     file_util::Delete(test_dir_, true);
     file_util::CreateDirectory(test_dir_);
     ASSERT_TRUE(file_util::DirectoryExists(test_dir_));
+
+    // Create a fresh policy store mock.
+    policy_store_.reset(new MockConfigurationPolicyStore());
   }
 
   virtual void TearDown() {
-    loop_.RunAllPending();
     // Clean up test directory.
     ASSERT_TRUE(file_util::Delete(test_dir_, true));
     ASSERT_FALSE(file_util::PathExists(test_dir_));
@@ -53,101 +40,10 @@ class ConfigDirPolicyProviderTestBase : public testing::Test {
     JSONStringValueSerializer serializer(&data);
     serializer.Serialize(dict);
     FilePath file_path(test_dir_.AppendASCII(file_name));
-    ASSERT_TRUE(file_util::WriteFile(file_path, data.c_str(), data.size()));
+    file_util::WriteFile(file_path, data.c_str(), data.size());
   }
 
   FilePath test_dir_;
-  MessageLoop loop_;
-  ChromeThread ui_thread_;
-  ChromeThread file_thread_;
-};
-
-// A mock provider that allows us to capture to reload notifications.
-class MockConfigDirPolicyProvider : public ConfigDirPolicyProvider {
- public:
-  explicit MockConfigDirPolicyProvider(const FilePath& config_dir_)
-      : ConfigDirPolicyProvider(config_dir_) {}
-
-  MOCK_METHOD0(NotifyStoreOfPolicyChange, void());
-};
-
-class PolicyDirLoaderTest : public ConfigDirPolicyProviderTestBase {
- protected:
-  PolicyDirLoaderTest() {}
-
-  virtual void SetUp() {
-    ConfigDirPolicyProviderTestBase::SetUp();
-    provider_.reset(new MockConfigDirPolicyProvider(test_dir_));
-  }
-
-  virtual void TearDown() {
-    provider_.reset(NULL);
-    ConfigDirPolicyProviderTestBase::TearDown();
-  }
-
-  scoped_ptr<MockConfigDirPolicyProvider> provider_;
-};
-
-TEST_F(PolicyDirLoaderTest, BasicLoad) {
-  DictionaryValue test_dict;
-  test_dict.SetString("HomepageLocation", "http://www.google.com");
-  WriteConfigFile(test_dict, "config_file");
-
-  scoped_refptr<PolicyDirLoader> loader_(
-      new PolicyDirLoader(provider_->AsWeakPtr(), test_dir_,
-                          kSettleIntervalSecondsForTesting,
-                          kReloadIntervalMinutesForTesting));
-  scoped_ptr<DictionaryValue> policy(loader_->GetPolicy());
-  EXPECT_TRUE(policy.get());
-  EXPECT_EQ(1U, policy->size());
-
-  std::string str_value;
-  EXPECT_TRUE(policy->GetString("HomepageLocation", &str_value));
-  EXPECT_EQ("http://www.google.com", str_value);
-
-  loader_->Stop();
-}
-
-TEST_F(PolicyDirLoaderTest, TestRefresh) {
-  scoped_refptr<PolicyDirLoader> loader_(
-      new PolicyDirLoader(provider_->AsWeakPtr(), test_dir_,
-                          kSettleIntervalSecondsForTesting,
-                          kReloadIntervalMinutesForTesting));
-  scoped_ptr<DictionaryValue> policy(loader_->GetPolicy());
-  EXPECT_TRUE(policy.get());
-  EXPECT_EQ(0U, policy->size());
-
-  DictionaryValue test_dict;
-  test_dict.SetString("HomepageLocation", "http://www.google.com");
-  WriteConfigFile(test_dict, "config_file");
-
-  EXPECT_CALL(*provider_, NotifyStoreOfPolicyChange()).Times(1);
-  loader_->OnFilePathChanged(test_dir_.AppendASCII("config_file"));
-
-  // Run the loop. The refresh should be handled immediately since the settle
-  // interval has been disabled.
-  loop_.RunAllPending();
-  Mock::VerifyAndClearExpectations(provider_.get());
-
-  policy.reset(loader_->GetPolicy());
-  EXPECT_TRUE(policy.get());
-  EXPECT_EQ(1U, policy->size());
-
-  std::string str_value;
-  EXPECT_TRUE(policy->GetString("HomepageLocation", &str_value));
-  EXPECT_EQ("http://www.google.com", str_value);
-
-  loader_->Stop();
-}
-
-class ConfigDirPolicyProviderTest : public ConfigDirPolicyProviderTestBase {
- protected:
-  virtual void SetUp() {
-    ConfigDirPolicyProviderTestBase::SetUp();
-    // Create a fresh policy store mock.
-    policy_store_.reset(new MockConfigurationPolicyStore());
-  }
-
   scoped_ptr<MockConfigurationPolicyStore> policy_store_;
 };