Remote assistance on Chrome OS Part IV - It2MeHost

remoting/host/it2me/it2me_host.h

Index: remoting/host/it2me/it2me_host.h
diff --git a/remoting/host/it2me/it2me_host.h b/remoting/host/it2me/it2me_host.h
index 349232d27a1c4505ff5b5c756f3e1ff8bb68a633..e427fc391cc0b0acaad4b6f1e7fbba51f72c365e 100644
--- a/remoting/host/it2me/it2me_host.h
+++ b/remoting/host/it2me/it2me_host.h
@@ -16,6 +16,10 @@ namespace base {
 class DictionaryValue;
 }
 
+namespace policy {
+class PolicyService;
+}  // namespace policy
+
 namespace remoting {
 
 class ChromotingHost;
@@ -28,9 +32,7 @@ class RegisterSupportHostRequest;
 class RsaKeyPair;
 
 namespace policy_hack {
-
 class PolicyWatcher;
-
 }  // namespace policy_hack
 
 // These state values are duplicated in host_session.js. Remember to update
@@ -60,8 +62,8 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   };
 
   It2MeHost(
-      ChromotingHostContext* context,
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      scoped_ptr<ChromotingHostContext> context,
+      scoped_ptr<policy_hack::PolicyWatcher> policy_watcher,
       base::WeakPtr<It2MeHost::Observer> observer,
       const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
       const std::string& directory_bot_jid);
@@ -91,7 +93,7 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
 
   ~It2MeHost() override;
 
-  ChromotingHostContext* host_context() { return host_context_; }
+  ChromotingHostContext* host_context() { return host_context_.get(); }
   scoped_refptr<base::SingleThreadTaskRunner> task_runner() {
     return task_runner_;
   }
@@ -123,6 +125,10 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   // the UI thread.
   void ShutdownOnUiThread();
 
+  // Called when |policy_watcher_| has stopped listening for changes and it is
+  // safe to delete it.
+  void OnPolicyWatcherShutdown();
+
   // Called when initial policies are read, and when they change.
   void OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies);
 
@@ -131,10 +137,7 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   void UpdateHostDomainPolicy(const std::string& host_domain);
 
   // Caller supplied fields.
-
-  // The creator of the It2MeHost object owns the the host context and is
-  // responsible for keeping it alive throughout the liftime of the host.
-  ChromotingHostContext* host_context_;
+  scoped_ptr<ChromotingHostContext> host_context_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   base::WeakPtr<It2MeHost::Observer> observer_;
   XmppSignalStrategy::XmppServerConfig xmpp_server_config_;
@@ -178,17 +181,22 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
 // implementation of the It2MeHost.
 class It2MeHostFactory {
  public:
-  It2MeHostFactory();
+  // |policy_service| is used for creating the policy watcher for new
+  // instances of It2MeHost on ChromeOS.  The caller must ensure that
+  // |policy_service| is valid throughout the lifetime of the It2MeHostFactory
+  // and each created It2MeHost object.  This is currently possible because
+  // |policy_service| is a global singleton available from the browser process.

[Wez, 2014/10/24 23:29:27]

If this is really a singleton then just pull in the dependency in the
CrOS-specific PolicyWatcher impl, directly (you can also have a Set*ForTest for
testing of course), rather than pollute this interface with impossible-to-meet
requirements on callers!

[kelvinp, 2014/10/29 01:22:52]

It is actually a global object instead of a singleton.  I can't pull it in
directly as that would introduce include dependencies from remoting to browser,
which would create a circular include dependency 

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

:(

OK... can we instead move this parameter to a set_policty_service() setter, to
avoid having to pass NULL in on non-CrOS, please?

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

Agreed.

+  It2MeHostFactory(policy::PolicyService* policy_service);
   virtual ~It2MeHostFactory();
 
   virtual scoped_refptr<It2MeHost> CreateIt2MeHost(
-      ChromotingHostContext* context,
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      scoped_ptr<ChromotingHostContext> context,
       base::WeakPtr<It2MeHost::Observer> observer,
       const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
       const std::string& directory_bot_jid);
 
  private:
+  policy::PolicyService* policy_service_;
   DISALLOW_COPY_AND_ASSIGN(It2MeHostFactory);
 };