Refactor PolicyWatcher to allow mocking

remoting/host/policy_watcher.h

 // Copyright 2015 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_WATCHER_H_
 #define REMOTING_HOST_POLICY_WATCHER_H_
 
 #include <memory>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/non_thread_safe.h"
 #include "components/policy/core/common/policy_service.h"
 
 namespace base {
 class DictionaryValue;
 class SingleThreadTaskRunner;
 }  // namespace base
 
 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>)>
       PolicyUpdatedCallback;
 
   // 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
   // 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
   // security risk of running with incorrectly formulated policies (by either
   // shutting down or locking down the host).
   // After calling |policy_error_callback| PolicyWatcher will continue watching
   // for policy changes and will call |policy_updated_callback| when the error
   // is recovered from and may call |policy_error_callback| when new errors are
   // 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
   // PolicyService (on other OS-es). PolicyWatcher must be used on the thread on
   // which it is created. |policy_service| is called on the same thread.
   //
   // When |policy_service| is null, then |file_task_runner| is used for reading
   // the policy from files / registry / preferences (which are blocking
   // operations). |file_task_runner| should be of TYPE_IO type.
   //
   // When |policy_service| is specified then |file_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 std::unique_ptr<PolicyWatcher> Create(
       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.
   //
   // |policy_service_task_runner| is passed through to the constructor of
   // PolicyWatcher.
   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;
+  // Gets Chromoting schema stored inside |owned_schema_registry_|.
+  virtual const policy::Schema* GetPolicySchema() const = 0;
 
-  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_;
-
-  // 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);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_POLICY_WATCHER_H_