Using PolicyServiceWatcher instead of PolicyWatcherLinux/Win/Mac.

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 "components/policy/core/common/policy_service.h"
 
 namespace base {
 class SingleThreadTaskRunner;
 class TimeDelta;
 class WaitableEvent;
 }  // namespace base
 
+namespace policy {
+class PolicyService;
+}  // 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 {
  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.
+
   // 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
   // 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.
+  //
+  // 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);
 
   // Should be called after StartWatching() before the object is deleted. Calls
   // 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[];
 
   // The name of the policy for requiring 2-factor authentication.
   static const char kHostRequireTwoFactorPolicyName[];
 
   // The name of the host domain policy.
   static const char kHostDomainPolicyName[];
 
@@ skipping 24 matching lines @@
 
   // The name of the policy that restricts the range of host UDP ports.
   static const char kUdpPortRangePolicyName[];
 
   // The name of the policy for overriding policies, for use in testing.
   static const char kHostDebugOverridePoliciesName[];
 
  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;
 
   // 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();
 
-  // 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;
 
  private:
   void StopWatchingOnPolicyWatcherThread();
   scoped_refptr<base::SingleThreadTaskRunner> 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_;
   scoped_ptr<base::DictionaryValue> bad_type_values_;
 
   // Allows us to cancel any inflight FileWatcher events or scheduled reloads.
   base::WeakPtrFactory<PolicyWatcher> weak_factory_;
 };
 
 }  // namespace policy_hack
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_POLICY_HACK_POLICY_WATCHER_H_