Remote assistance on Chrome OS Part IV - It2MeHost

remoting/host/chromoting_host_context.cc

Index: remoting/host/chromoting_host_context.cc
diff --git a/remoting/host/chromoting_host_context.cc b/remoting/host/chromoting_host_context.cc
index 754ea8b5c23be1bd7643b20d9eb0a9bde81803d1..f4de9dab2fe980e8d916c48efc28549f019a97b2 100644
--- a/remoting/host/chromoting_host_context.cc
+++ b/remoting/host/chromoting_host_context.cc
@@ -4,9 +4,8 @@
 
 #include "remoting/host/chromoting_host_context.h"
 
-#include <string>
-
-#include "base/bind.h"
+#include "components/policy/core/common/policy_service.h"
+#include "content/public/browser/browser_thread.h"
 #include "remoting/base/auto_thread.h"
 #include "remoting/base/url_request_context_getter.h"
 
@@ -14,56 +13,117 @@ namespace remoting {
 
 ChromotingHostContext::ChromotingHostContext(
     AutoThreadTaskRunner* ui_task_runner)
-    : ui_task_runner_(ui_task_runner) {
+    : ui_task_runner_(ui_task_runner), policy_service_(NULL) {
+}
+
+ChromotingHostContext::~ChromotingHostContext() {
+}
+
+// static
+scoped_ptr<ChromotingHostContext> ChromotingHostContext::Create(
+    scoped_refptr<AutoThreadTaskRunner> ui_task_runner) {
+  scoped_ptr<ChromotingHostContext> context(
+      new ChromotingHostContext(ui_task_runner.get()));
+
 #if defined(OS_WIN)
   // On Windows the AudioCapturer requires COM, so we run a single-threaded
   // apartment, which requires a UI thread.
-  audio_task_runner_ =
+  context->audio_task_runner_ =
       AutoThread::CreateWithLoopAndComInitTypes("ChromotingAudioThread",
-                                                ui_task_runner_,
+                                                ui_task_runner,
                                                 base::MessageLoop::TYPE_UI,
                                                 AutoThread::COM_INIT_STA);
 #else // !defined(OS_WIN)
-  audio_task_runner_ = AutoThread::CreateWithType(
-      "ChromotingAudioThread", ui_task_runner_, base::MessageLoop::TYPE_IO);
+  context->audio_task_runner_ = AutoThread::CreateWithType(

[Jamie, 2014/10/14 01:18:41]

This code reads a lot like a ctor. Could you add extra parameters to the ctor
for the various threads? It would add a compiler check to ensure that both
Create methods are supplying the same threads etc.

I think the creation of the audio, input and encode threads could also be left
in the ctor, since the only difference is the thread they use to join. You could
either make that another ctor parameter, or perhaps use the file thread to join
on all platforms (check with someone who knows the architecture better than I do
first; it might not work if you do that).

[kelvinp, 2014/10/15 23:03:09]

I spoke to wez.  He suggested instead of joining on the FILE Thread.  We should
add a ScopedAllowIO on auto thread instead.

+      "ChromotingAudioThread", ui_task_runner, base::MessageLoop::TYPE_IO);
 #endif  // !defined(OS_WIN)
 
-  file_task_runner_ = AutoThread::CreateWithType(
-      "ChromotingFileThread", ui_task_runner_, base::MessageLoop::TYPE_IO);
-  input_task_runner_ = AutoThread::CreateWithType(
-      "ChromotingInputThread", ui_task_runner_, base::MessageLoop::TYPE_IO);
-  network_task_runner_ = AutoThread::CreateWithType(
-      "ChromotingNetworkThread", ui_task_runner_, base::MessageLoop::TYPE_IO);
-  video_capture_task_runner_ =
-      AutoThread::Create("ChromotingCaptureThread", ui_task_runner_);
-  video_encode_task_runner_ = AutoThread::Create(
-      "ChromotingEncodeThread", ui_task_runner_);
-
-  url_request_context_getter_ = new URLRequestContextGetter(
-      network_task_runner_, file_task_runner_);
+  context->file_task_runner_ = AutoThread::CreateWithType(
+      "ChromotingFileThread", ui_task_runner, base::MessageLoop::TYPE_IO);
+  context->input_task_runner_ = AutoThread::CreateWithType(
+      "ChromotingInputThread", ui_task_runner, base::MessageLoop::TYPE_IO);
+  context->network_task_runner_ = AutoThread::CreateWithType(
+      "ChromotingNetworkThread", ui_task_runner, base::MessageLoop::TYPE_IO);
+  context->video_capture_task_runner_ =
+      AutoThread::Create("ChromotingCaptureThread", ui_task_runner);
+  context->video_encode_task_runner_ = AutoThread::Create(
+      "ChromotingEncodeThread", ui_task_runner);
+
+  context->url_request_context_getter_ = new URLRequestContextGetter(
+      context->network_task_runner_, context->file_task_runner_);
+
+  if (!context->VerifyInitialized()) {
+    context.reset();
+  }
+
+  return context.Pass();
 }
 
-ChromotingHostContext::~ChromotingHostContext() {
+namespace {
+// Retrieves the task_runner from the browser thread with |id|.  Must be called
+// on the UI thread of the browser process.

[Jamie, 2014/10/14 01:18:41]

This comment implies that CreateOnChromeOS must also be called on the UI thread,
which should probably be mentioned in the header.

[kelvinp, 2014/10/15 23:03:09]

Done.

+AutoThreadTaskRunner* GetTaskRunner(content::BrowserThread::ID id) {
+#if defined(OS_CHROMEOS)
+  return new AutoThreadTaskRunner(
+      content::BrowserThread::GetMessageLoopProxyForThread(id).get(),
+      base::Bind(&base::DoNothing));
+#else
+  NOTIMPLEMENTED();
+  return nullptr;
+#endif // !defined(OS_CHROMEOS)
 }
 
-scoped_ptr<ChromotingHostContext> ChromotingHostContext::Create(
-    scoped_refptr<AutoThreadTaskRunner> ui_task_runner) {
-  DCHECK(ui_task_runner->BelongsToCurrentThread());
+}  // namespace
+
+// static
+scoped_ptr<ChromotingHostContext> ChromotingHostContext::CreateForChromeOS(
+    scoped_refptr<AutoThreadTaskRunner> ui_task_runner,
+    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
+    policy::PolicyService* policy_service) {
+  DCHECK(url_request_context_getter.get());
+  DCHECK(policy_service);
 
   scoped_ptr<ChromotingHostContext> context(
       new ChromotingHostContext(ui_task_runner.get()));
-  if (!context->audio_task_runner_.get() || !context->file_task_runner_.get() ||
-      !context->input_task_runner_.get() ||
-      !context->network_task_runner_.get() ||
-      !context->video_capture_task_runner_.get() ||
-      !context->video_encode_task_runner_.get() ||
-      !context->url_request_context_getter_.get()) {
+  // Thread assignments and re-using existing browser sources
+  // (URLRequestContextGetter) must be performed on the UI thread of the
+  // browser process.
+  context->file_task_runner_ = GetTaskRunner(content::BrowserThread::FILE);
+  context->network_task_runner_ = GetTaskRunner(content::BrowserThread::IO);
+  context->video_capture_task_runner_ =
+      GetTaskRunner(content::BrowserThread::UI);
+
+  context->url_request_context_getter_ = url_request_context_getter;
+
+  // Thread joining is blocking and it can only be done on threads that allows

[Jamie, 2014/10/14 01:18:41]

s/allows/allow/

[kelvinp, 2014/10/15 23:03:09]

Done.

+  // blocking I/O.  Uses the |file_task_runner| as the joiner.
+  context->input_task_runner_ =
+      AutoThread::CreateWithType("ChromotingInputThread",
+                                 context->file_task_runner_,
+                                 base::MessageLoop::TYPE_IO);
+  context->audio_task_runner_ =
+      AutoThread::CreateWithType("ChromotingAudioThread",
+                                 context->file_task_runner_,
+                                 base::MessageLoop::TYPE_IO);
+  context->video_encode_task_runner_ =
+      AutoThread::Create("ChromotingEncodeThread", context->file_task_runner_);
+
+  context->policy_service_ = policy_service;
+
+  if (!context->VerifyInitialized()) {
     context.reset();
   }
 
   return context.Pass();
 }
 
+bool ChromotingHostContext::VerifyInitialized() {
+  return audio_task_runner_.get() && file_task_runner_.get() &&
+         input_task_runner_.get() && network_task_runner_.get() &&
+         video_capture_task_runner_.get() && video_encode_task_runner_.get() &&
+         url_request_context_getter_.get();
+}
+
 scoped_refptr<AutoThreadTaskRunner>
 ChromotingHostContext::audio_task_runner() {
   return audio_task_runner_;
@@ -104,4 +164,8 @@ ChromotingHostContext::url_request_context_getter() {
   return url_request_context_getter_;
 }
 
+policy::PolicyService* ChromotingHostContext::policy_service() {
+  return policy_service_;
+}
+
 }  // namespace remoting