Merged PolicyServiceWatcher into PolicyWatcher.

remoting/host/policy_hack/policy_watcher.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef REMOTING_HOST_POLICY_HACK_POLICY_WATCHER_H_
 #define REMOTING_HOST_POLICY_HACK_POLICY_WATCHER_H_
 
 #include "base/callback.h"
-#include "base/memory/weak_ptr.h"
-#include "base/values.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "components/policy/core/common/policy_service.h"
 
 namespace base {
+class DictionaryValue;
 class SingleThreadTaskRunner;
-class TimeDelta;
-class WaitableEvent;
 }  // namespace base
 
 namespace policy {
-class PolicyService;
+class AsyncPolicyLoader;
+class ConfigurationPolicyProvider;
+class SchemaRegistry;
 }  // namespace policy
 
 namespace remoting {
 namespace policy_hack {
 
-// Watches for changes to the managed remote access host policies.
-// If StartWatching() has been called, then before this object can be deleted,
-// StopWatching() have completed (the provided |done| event must be signaled).
-class PolicyWatcher {
+// Watches for changes to the managed remote access host policies.  If
+// StartWatching() has been called, then before this object can be deleted,
+// StopWatching() has to be completed (the provided |done| event must be
+// signaled).
+class PolicyWatcher : public policy::PolicyService::Observer {
  public:
   // Called first with all policies, and subsequently with any changed policies.
   typedef base::Callback<void(scoped_ptr<base::DictionaryValue>)>
       PolicyUpdatedCallback;
 
-  // TODO(lukasza): PolicyErrorCallback never gets called by
-  // PolicyServiceWatcher.  Need to either 1) remove error-handling from
-  // PolicyWatcher or 2) add error-handling around PolicyService
-  // 2a) Add policy name/type validation via policy::Schema::Normalize.
-  // 2b) Consider exposing parsing errors from policy::ConfigDirPolicyLoader.
+  // TODO(lukasza): PolicyErrorCallback never gets called by PolicyWatcher.
+  // Need to either 1) remove error-handling from PolicyWatcher or 2) add
+  // error-handling around PolicyService 2a) Add policy name/type validation via
+  // policy::Schema::Normalize.  2b) Consider exposing parsing errors from
+  // policy::ConfigDirPolicyLoader.
 
   // Called after detecting malformed policies.
   typedef base::Callback<void()> PolicyErrorCallback;
 
-  // Derived classes specify which |task_runner| should be used for calling
-  // their StartWatchingInternal and StopWatchingInternal methods.
-  // Derived classes promise back to call UpdatePolicies and other instance
-  // methods on the same |task_runner|.
-  explicit PolicyWatcher(
-      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
-
-  virtual ~PolicyWatcher();
+  ~PolicyWatcher() override;
 
   // This guarantees that the |policy_updated_callback| is called at least once
   // with the current policies.  After that, |policy_updated_callback| will be
   // called whenever a change to any policy is detected. It will then be called
   // only with the changed policies.
   //
   // |policy_error_callback| will be called when malformed policies are detected
   // (i.e. wrong type of policy value, or unparseable files under
   // /etc/opt/chrome/policies/managed).
   // When called, the |policy_error_callback| is responsible for mitigating the
@@ skipping 26 matching lines @@
   // When |policy_service| is specified then |task_runner| argument is ignored
   // and 1) BrowserThread::UI is used for PolicyUpdatedCallback and
   // PolicyErrorCallback and 2) BrowserThread::FILE is used for reading the
   // policy from files / registry / preferences (although (2) is just an
   // implementation detail and should likely be ignored outside of
   // PolicyWatcher).
   static scoped_ptr<PolicyWatcher> Create(
       policy::PolicyService* policy_service,
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
 
- protected:
-  virtual void StartWatchingInternal() = 0;
-  virtual void StopWatchingInternal() = 0;
+ private:
+  friend class PolicyWatcherTest;
+
+  // TODO(lukasza): s/Internal/OnWatcherThread/g

[Sergey Ulanov, 2015/01/26 18:20:01]

maybe rename them in this CL?

[Łukasz Anforowicz, 2015/01/26 19:24:20]

Done.  Initially I wanted to just make this changelist a dumb merge with little
changes of content being merged.  OTOH, with the merging of the constructors and
getting rid of "using namespace policy", it is already a quite busy changelist,
so it is probably better to make this as complete as possible.  OTOH, I probably
want to remove the type-checking in a separate changelist (as this is not simple
refactoring as the other things within the current changelist).

+  virtual void StartWatchingInternal();
+  virtual void StopWatchingInternal();
 
   // Used to check if the class is on the right thread.
   bool OnPolicyWatcherThread() const;
 
   // Takes the policy dictionary from the OS specific store and extracts the
   // relevant policies.
   void UpdatePolicies(const base::DictionaryValue* new_policy);
 
   // Signals policy error to the registered |PolicyErrorCallback|.
   void SignalPolicyError();
 
   // Called whenever a transient error occurs during reading of policy files.
   // This will increment a counter, and will trigger a call to
   // SignalPolicyError() only after a threshold count is reached.
   // The counter is reset whenever policy has been successfully read.
   void SignalTransientPolicyError();
 
-  friend class PolicyWatcherTest;
+  // |policy_service_task_runner| is the task runner where it is safe
+  // to call |policy_service_| methods and where we expect to get callbacks
+  // from |policy_service_|.
+  explicit PolicyWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&
+                             policy_service_task_runner);
 
-  // Returns a DictionaryValue containing the default values for each policy.
-  const base::DictionaryValue& Defaults() const;
+  // Constructor for the case when |policy_service| is borrowed.
+  PolicyWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&
+                    policy_service_task_runner,
+                policy::PolicyService* policy_service);
 
- private:
+  // Constructor for the case when |policy_service| is owned (and uses also
+  // owned |owned_policy_provider| and |owned_schema_registry|.
+  PolicyWatcher(
+      const scoped_refptr<base::SingleThreadTaskRunner>&
+          policy_service_task_runner,
+      scoped_ptr<policy::PolicyService> owned_policy_service,
+      scoped_ptr<policy::ConfigurationPolicyProvider> owned_policy_provider,
+      scoped_ptr<policy::SchemaRegistry> owned_schema_registry);
+
+  // Creates PolicyWatcher that wraps the owned |async_policy_loader| with an
+  // appropriate PolicySchema.
+  //
+  // |policy_service_task_runner| is passed through to the constructor of
+  // PolicyWatcher.
+  static scoped_ptr<PolicyWatcher> CreateFromPolicyLoader(
+      const scoped_refptr<base::SingleThreadTaskRunner>&
+          policy_service_task_runner,
+      scoped_ptr<policy::AsyncPolicyLoader> async_policy_loader);
+
+  // PolicyService::Observer interface.
+  void OnPolicyUpdated(const policy::PolicyNamespace& ns,
+                       const policy::PolicyMap& previous,
+                       const policy::PolicyMap& current) override;
+  void OnPolicyServiceInitialized(policy::PolicyDomain domain) override;
+
+  // TODO(lukasza): Remove and merge with StopWatchingInternal.
   void StopWatchingOnPolicyWatcherThread();
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  scoped_refptr<base::SingleThreadTaskRunner> policy_service_task_runner_;
 
   PolicyUpdatedCallback policy_updated_callback_;
   PolicyErrorCallback policy_error_callback_;
   int transient_policy_error_retry_counter_;
 
   scoped_ptr<base::DictionaryValue> old_policies_;
   scoped_ptr<base::DictionaryValue> default_values_;
+
+  // TODO(lukasza): Remove - components/policy filters out mistyped values.
   scoped_ptr<base::DictionaryValue> bad_type_values_;
 
-  // Allows us to cancel any inflight FileWatcher events or scheduled reloads.
-  base::WeakPtrFactory<PolicyWatcher> weak_factory_;
+  policy::PolicyService* policy_service_;
+
+  // Order of fields below is important to ensure destruction takes object
+  // dependencies into account:
+  // - |owned_policy_service_| uses |owned_policy_provider_|
+  // - |owned_policy_provider_| uses |owned_schema_registry_|
+  scoped_ptr<policy::SchemaRegistry> owned_schema_registry_;
+  scoped_ptr<policy::ConfigurationPolicyProvider> owned_policy_provider_;
+  scoped_ptr<policy::PolicyService> owned_policy_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(PolicyWatcher);
 };
 
 }  // namespace policy_hack
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_POLICY_HACK_POLICY_WATCHER_H_