Refactor PolicyWatcher to allow mocking

remoting/host/policy_watcher.h

Index: remoting/host/policy_watcher.h
diff --git a/remoting/host/policy_watcher.h b/remoting/host/policy_watcher.h
index e3f54f6916bfd80c6a5dbdfc74722198e76472a8..facf7f31711e8f58ef0c52d5d6b10b55fa679c89 100644
--- a/remoting/host/policy_watcher.h
+++ b/remoting/host/policy_watcher.h
@@ -20,16 +20,13 @@ class SingleThreadTaskRunner;
 
 namespace policy {
 class AsyncPolicyLoader;
-class ConfigurationPolicyProvider;
 class Schema;
-class SchemaRegistry;
 }  // namespace policy
 
 namespace remoting {
 
 // Watches for changes to the managed remote access host policies.
-class PolicyWatcher : public policy::PolicyService::Observer,
-                      public base::NonThreadSafe {
+class PolicyWatcher : public base::NonThreadSafe {
  public:
   // Called first with all policies, and subsequently with any changed policies.
   typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)>
@@ -38,7 +35,7 @@ class PolicyWatcher : public policy::PolicyService::Observer,
   // Called after detecting malformed policies.
   typedef base::Callback<void()> PolicyErrorCallback;
 
-  ~PolicyWatcher() override;
+  virtual ~PolicyWatcher() = 0;

[Sergey Ulanov, 2017/02/23 19:18:55]

s/=0/{}/
I didn't know destructor can be pure virtual, but I don't think we use it
anywhere else, and for consistency it's better to define an empty destructor.

[rkjnsn, 2017/02/23 22:28:10]

Pure virtual destructors are interesting beasts. Unlike pure virtual methods,
you still have to define them. (There's a definition in policy_watcher.cc) Their
main effect is to make a class abstract even if all virtual methods have default
implementations.

In some code bases, the convention is to always make the destructor of abstract
classes pure virtual to make it easy to tell at a glance whether the class is
abstract or not. However, it looks like Chromium prefers the opposite, so I'll
change it.

 
   // This guarantees that the |policy_updated_callback| is called at least once
   // with the current policies.  After that, |policy_updated_callback| will be
@@ -57,7 +54,7 @@ class PolicyWatcher : public policy::PolicyService::Observer,
   // found.
   virtual void StartWatching(
       const PolicyUpdatedCallback& policy_updated_callback,
-      const PolicyErrorCallback& policy_error_callback);
+      const PolicyErrorCallback& policy_error_callback) = 0;
 
   // Specify a |policy_service| to borrow (on Chrome OS, from the browser
   // process) or specify nullptr to internally construct and use a new
@@ -78,35 +75,12 @@ class PolicyWatcher : public policy::PolicyService::Observer,
       policy::PolicyService* policy_service,
       const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner);
 
+ protected:
+  PolicyWatcher();

[Sergey Ulanov, 2017/02/23 19:18:55]

Usually for interfaces the constructor should have an empty inline definition,
or you can omit it completely.

[rkjnsn, 2017/02/23 22:28:10]

Okay, I'll move it inline.

+
  private:
   friend class PolicyWatcherTest;
 
-  // Gets Chromoting schema stored inside |owned_schema_registry_|.
-  const policy::Schema* GetPolicySchema() const;
-
-  // Simplifying wrapper around Schema::Normalize.
-  // - Returns false if |dict| is invalid (i.e. contains mistyped policy
-  // values).
-  // - Returns true if |dict| was valid or got normalized.
-  bool NormalizePolicies(base::DictionaryValue* dict);
-
-  // Stores |new_policies| into |old_policies_|.  Returns dictionary with items
-  // from |new_policies| that are different from the old |old_policies_|.
-  std::unique_ptr<base::DictionaryValue> StoreNewAndReturnChangedPolicies(
-      std::unique_ptr<base::DictionaryValue> new_policies);
-
-  // Signals policy error to the registered |PolicyErrorCallback|.
-  void SignalPolicyError();
-
-  // |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_|.
-  PolicyWatcher(policy::PolicyService* policy_service,
-                std::unique_ptr<policy::PolicyService> owned_policy_service,
-                std::unique_ptr<policy::ConfigurationPolicyProvider>
-                    owned_policy_provider,
-                std::unique_ptr<policy::SchemaRegistry> owned_schema_registry);
-
   // Creates PolicyWatcher that wraps the owned |async_policy_loader| with an
   // appropriate PolicySchema.
   //
@@ -115,27 +89,11 @@ class PolicyWatcher : public policy::PolicyService::Observer,
   static std::unique_ptr<PolicyWatcher> CreateFromPolicyLoader(
       std::unique_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;
-
-  PolicyUpdatedCallback policy_updated_callback_;
-  PolicyErrorCallback policy_error_callback_;
-
-  std::unique_ptr<base::DictionaryValue> old_policies_;
-  std::unique_ptr<base::DictionaryValue> default_values_;
-
-  policy::PolicyService* policy_service_;
+  // Gets Chromoting schema stored inside |owned_schema_registry_|.
+  virtual const policy::Schema* GetPolicySchema() const = 0;
 
-  // 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_|
-  std::unique_ptr<policy::SchemaRegistry> owned_schema_registry_;
-  std::unique_ptr<policy::ConfigurationPolicyProvider> owned_policy_provider_;
-  std::unique_ptr<policy::PolicyService> owned_policy_service_;
+  // Gets default policy values.
+  virtual const base::DictionaryValue* GetDefaultValues() const = 0;

[Sergey Ulanov, 2017/02/23 19:18:55]

I don't think these private methods need to be here. Only Impl class needs them.

https://google.github.io/styleguide/cppguide.html#Interfaces

 
   DISALLOW_COPY_AND_ASSIGN(PolicyWatcher);
 };