Remote assistance on Chrome OS Part IV - It2MeHost

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 0ced8f23a4a7c55437e8fb0566dbb359a8c8adef..fe140502ad657fc3a832153735508085a51d166e 100644
--- a/remoting/host/policy_hack/policy_watcher.h
+++ b/remoting/host/policy_hack/policy_watcher.h
@@ -8,6 +8,7 @@
 #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;
@@ -38,12 +39,16 @@ class PolicyWatcher {
   virtual void StartWatching(const PolicyCallback& policy_callback);
 
   // Should be called after StartWatching() before the object is deleted. Calls
-  // just wait for |done| to be signaled before deleting the object.
-  virtual void StopWatching(base::WaitableEvent* done);
-
-  // Implemented by each platform.  This message loop should be an IO message
-  // loop.
-  static PolicyWatcher* Create(
+  // should wait for |stopped_callback| to be called before deleting it.
+  virtual void StopWatching(const base::Closure& stopped_callback);
+
+  // Implemented by each platform.  This |task_runner| should be an IO message

[Wez, 2014/10/29 18:27:51]

Remove "This" - it doesn't make sense grammatically.

[kelvinp, 2014/10/29 22:20:17]

Done.

+  // loop.  The policy watcher re-posts any calls to its public interface on the
+  // |task_runner| so it can be called from any thread.  |policy_service| is

[Wez, 2014/10/29 18:27:51]

This isn't true under ChromeOS, though. Is it even a property we care about - we
seem to only use it from the same thread?

[Wez, 2014/10/29 18:27:51]

It's also the task runner on which the watcher will call callbacks.

[kelvinp, 2014/10/29 22:20:17]

To be honest, I don't know the historical context of why the policy_watcher
reposts on the existing thread.  I merely document it off the existing behavior
of the code.  In order to avoid documenting unnecessary rationale, I am going to
remove the comment regarding the task_runner.  No information is better than
information that converting existing behavior into a rationale :)

[Wez, 2014/10/30 01:08:59]

Acknowledged.

+  // currently only used on ChromeOS.  The caller must ensure that
+  // |policy_service| remains valid for the lifetime of PolicyWatcherChromeOS.

[Wez, 2014/10/29 18:27:51]

The caller does not see PolicyWatcherChromeOS - it's an implementation detail -
so don't mention it in the comment.

[Wez, 2014/10/29 18:27:51]

This would be cleaner with separate CrOS and non-CrOS creator functions, IMO.

[kelvinp, 2014/10/29 22:20:17]

Totally, but that would also introduce ifdef's on the call site.  In the long
run, we would want to get rid of policy hack (our custom cross platform
implementation) and move towards the policy_service in the policy component
anyways, so leaving the policy_service in the Ctor is not as bad as it seems.  

[kelvinp, 2014/10/29 22:20:17]

Done.

[Wez, 2014/10/30 01:08:59]

Acknowledged.

+  static scoped_ptr<PolicyWatcher> Create(
+      policy::PolicyService* policy_service,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
   // The name of the NAT traversal policy.
@@ -107,6 +112,7 @@ class PolicyWatcher {
   const base::DictionaryValue& Defaults() const;
 
  private:
+  void StopWatchingOnPolicyWatcherThread();
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   PolicyCallback policy_callback_;