Using PolicyServiceWatcher instead of PolicyWatcherLinux/Win/Mac.

remoting/host/policy_hack/policy_watcher.h

Index: remoting/host/policy_hack/policy_watcher.h
diff --git a/remoting/host/policy_hack/policy_watcher.h b/remoting/host/policy_hack/policy_watcher.h
index dbbfd2310fe8104d7541da7b41b61750343ad4a1..db92c25447769fab65d429fdf5ecfba9291e6358 100644
--- a/remoting/host/policy_hack/policy_watcher.h
+++ b/remoting/host/policy_hack/policy_watcher.h
@@ -8,7 +8,6 @@
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
-#include "components/policy/core/common/policy_service.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -16,6 +15,10 @@ class TimeDelta;
 class WaitableEvent;
 }  // namespace base
 
+namespace policy {
+class PolicyService;
+}  // namespace policy
+
 namespace remoting {
 namespace policy_hack {
 
@@ -28,11 +31,22 @@ class PolicyWatcher {
   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.
+
   // 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(
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
+
   virtual ~PolicyWatcher();
 
   // This guarantees that the |policy_updated_callback| is called at least once
@@ -50,6 +64,9 @@ class PolicyWatcher {
   // 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.
+  //
+  // See |Create| method's description for comments about which thread will
+  // be used to run the callbacks.
   virtual void StartWatching(
       const PolicyUpdatedCallback& policy_updated_callback,
       const PolicyErrorCallback& policy_error_callback);
@@ -58,13 +75,22 @@ class PolicyWatcher {
   // should wait for |stopped_callback| to be called before deleting it.
   virtual void StopWatching(const base::Closure& stopped_callback);
 
-  // Implemented by each platform.  |task_runner| should be an IO message loop.
-  // |policy_service| is currently only used on ChromeOS.  The caller must
-  // ensure that |policy_service| remains valid for the lifetime of
-  // PolicyWatcher.
+  // 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).
+  //
+  // When |policy_service| is null, then |task_runner| is used for reading the
+  // policy from files / registry / preferences.  PolicyUpdatedCallback and
+  // PolicyErrorCallback will be called on the same |task_runner|.
+  // |task_runner| should be of TYPE_IO type.
+  //
+  // When |policy_service| is specified then |task_runner| argument is ignored
+  // and 1) browser's UI thread is used for PolicyUpdatedCallback and
+  // PolicyErrorCallback and 2) browser's IO thread is used for reading the

[Mattias Nissler (ping if slow), 2015/01/08 09:58:37]

nit: Infamous Chrome trivia bit: What is referred to as the IO thread in the
browser is actually used for network I/O and can't do file I/O. We're actually
doing file IO on the FILE thread or a background thread.

[Łukasz Anforowicz, 2015/01/08 23:09:27]

Errr... I changed IO to FILE in the comment above (and qualified them with
BrowserThread class to make them less ambiguous).  Does that fix the comment and
make it factually accurate?

Hmmm... I guess #2 in my comment is an implementation detail.  I'll add that
note to the comment.

+  // policy from files / registry / preferences.
   static scoped_ptr<PolicyWatcher> Create(
       policy::PolicyService* policy_service,
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
 
   // The name of the NAT traversal policy.
   static const char kNatPolicyName[];
@@ -109,7 +135,6 @@ class PolicyWatcher {
  protected:
   virtual void StartWatchingInternal() = 0;
   virtual void StopWatchingInternal() = 0;
-  virtual void Reload() = 0;
 
   // Used to check if the class is on the right thread.
   bool OnPolicyWatcherThread() const;
@@ -127,11 +152,6 @@ class PolicyWatcher {
   // The counter is reset whenever policy has been successfully read.
   void SignalTransientPolicyError();
 
-  // Used for time-based reloads in case something goes wrong with the
-  // notification system.
-  void ScheduleFallbackReloadTask();
-  void ScheduleReloadTask(const base::TimeDelta& delay);
-
   // Returns a DictionaryValue containing the default values for each policy.
   const base::DictionaryValue& Defaults() const;