Remote assistance on Chrome OS Part IV - It2MeHost

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 remoting {
 namespace policy_hack {
 
@@ skipping 10 matching lines @@
       scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   virtual ~PolicyWatcher();
 
   // This guarantees that the |policy_callback| is called at least once with
   // the current policies.  After that, |policy_callback| will be called
   // whenever a change to any policy is detected. It will then be called only
   // with the changed policies.
   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);
+  // should wait for |stopped_callback| to be called before deleting it.
+  virtual void StopWatching(const base::Closure& stopped_callback);
 
-  // Implemented by each platform.  This message loop should be an IO message
-  // loop.
-  static PolicyWatcher* Create(
+  // 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.
   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 43 matching lines @@
 
   // 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_;
 
   PolicyCallback policy_callback_;
 
   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_