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..6c378f737cdf7b09d0a80ef796d851b2e370bfea 100644
--- a/remoting/host/chromoting_host_context.cc
+++ b/remoting/host/chromoting_host_context.cc
@@ -4,64 +4,39 @@
 
 #include "remoting/host/chromoting_host_context.h"
 
-#include <string>
-
-#include "base/bind.h"
+#include "content/public/browser/browser_thread.h"
 #include "remoting/base/auto_thread.h"
 #include "remoting/base/url_request_context_getter.h"
 
 namespace remoting {
 
 ChromotingHostContext::ChromotingHostContext(
-    AutoThreadTaskRunner* ui_task_runner)
-    : ui_task_runner_(ui_task_runner) {
-#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_ =
-      AutoThread::CreateWithLoopAndComInitTypes("ChromotingAudioThread",
-                                                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);
-#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_);
+    scoped_refptr<AutoThreadTaskRunner> ui_task_runner,
+    scoped_refptr<AutoThreadTaskRunner> audio_task_runner,
+    scoped_refptr<AutoThreadTaskRunner> file_task_runner,
+    scoped_refptr<AutoThreadTaskRunner> input_task_runner,
+    scoped_refptr<AutoThreadTaskRunner> network_task_runner,
+    scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner,
+    scoped_refptr<AutoThreadTaskRunner> video_encode_task_runner,
+    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter)
+    : ui_task_runner_(ui_task_runner),
+      audio_task_runner_(audio_task_runner),
+      file_task_runner_(file_task_runner),
+      input_task_runner_(input_task_runner),
+      network_task_runner_(network_task_runner),
+      video_capture_task_runner_(video_capture_task_runner),
+      video_encode_task_runner_(video_encode_task_runner),
+      url_request_context_getter_(url_request_context_getter) {
 }
 
 ChromotingHostContext::~ChromotingHostContext() {
 }
 
-scoped_ptr<ChromotingHostContext> ChromotingHostContext::Create(
-    scoped_refptr<AutoThreadTaskRunner> ui_task_runner) {
-  DCHECK(ui_task_runner->BelongsToCurrentThread());
-
-  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()) {
-    context.reset();
-  }
-
-  return context.Pass();
+scoped_ptr<ChromotingHostContext> ChromotingHostContext::Copy() {
+  return make_scoped_ptr(new ChromotingHostContext(
+      ui_task_runner_, audio_task_runner_, file_task_runner_,
+      input_task_runner_, network_task_runner_, video_capture_task_runner_,
+      video_encode_task_runner_, url_request_context_getter_));
 }
 
 scoped_refptr<AutoThreadTaskRunner>
@@ -104,4 +79,86 @@ ChromotingHostContext::url_request_context_getter() {
   return url_request_context_getter_;
 }
 
+scoped_ptr<ChromotingHostContext> ChromotingHostContext::Create(
+    scoped_refptr<AutoThreadTaskRunner> ui_task_runner) {
+#if defined(OS_WIN)
+  // On Windows the AudioCapturer requires COM, so we run a single-threaded
+  // apartment, which requires a UI thread.
+  scoped_refptr<AutoThreadTaskRunner> audio_task_runner =
+      AutoThread::CreateWithLoopAndComInitTypes(
+          "ChromotingAudioThread", ui_task_runner, base::MessageLoop::TYPE_UI,
+          AutoThread::COM_INIT_STA);
+#else   // !defined(OS_WIN)
+  scoped_refptr<AutoThreadTaskRunner> audio_task_runner =
+      AutoThread::CreateWithType("ChromotingAudioThread", ui_task_runner,
+                                 base::MessageLoop::TYPE_IO);
+#endif  // !defined(OS_WIN)
+  scoped_refptr<AutoThreadTaskRunner> file_task_runner =
+      AutoThread::CreateWithType("ChromotingFileThread", ui_task_runner,
+                                 base::MessageLoop::TYPE_IO);
+  scoped_refptr<AutoThreadTaskRunner> input_task_runner =
+      AutoThread::CreateWithType("ChromotingInputThread", ui_task_runner,
+                                 base::MessageLoop::TYPE_IO);
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner =
+      AutoThread::CreateWithType("ChromotingNetworkThread", ui_task_runner,
+                                 base::MessageLoop::TYPE_IO);
+  scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner =
+      AutoThread::Create("ChromotingCaptureThread", ui_task_runner);
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
+      new URLRequestContextGetter(network_task_runner, file_task_runner);
+  scoped_refptr<AutoThreadTaskRunner> video_encode_task_runner =
+      AutoThread::Create("ChromotingEncodeThread", ui_task_runner);
+
+  return make_scoped_ptr(new ChromotingHostContext(
+      ui_task_runner, audio_task_runner, file_task_runner, input_task_runner,
+      network_task_runner, video_capture_task_runner, video_encode_task_runner,
+      url_request_context_getter));

[Wez, 2014/10/24 00:28:48]

nit: Rather than create them all in temporaries and then pass them, it may read
more cleanly to create the non-ifdef'd ones in the ctor i.e:

new ChromotingHostContext(
    ...,
    AutoThread::CreateWithType(...),
    ...);

[kelvinp, 2014/10/24 21:39:41]

Done.

+}
+
+#if defined(OS_CHROMEOS)
+namespace {
+// Retrieves the task_runner from the browser thread with |id|.  Must be called
+// on the UI thread of the browser process.

[Wez, 2014/10/24 00:28:47]

nit: the latter is a req of the GetMessageLoopProxyForThread API, so do you need
to re-state it here?

[kelvinp, 2014/10/24 21:39:41]

Done.

+AutoThreadTaskRunner* GetTaskRunner(content::BrowserThread::ID id) {
+  return new AutoThreadTaskRunner(
+      content::BrowserThread::GetMessageLoopProxyForThread(id).get(),
+      base::Bind(&base::DoNothing));

[Wez, 2014/10/24 00:28:47]

nit: add brief comment explaining the issue w/ using DoNothing here.

[kelvinp, 2014/10/24 21:39:41]

Done.

+}
+
+}  // namespace
+
+// static
+scoped_ptr<ChromotingHostContext> ChromotingHostContext::CreateForChromeOS(
+    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
+  DCHECK(url_request_context_getter.get());
+
+  scoped_refptr<AutoThreadTaskRunner> ui_task_runner =
+      GetTaskRunner(content::BrowserThread::UI);
+  scoped_refptr<AutoThreadTaskRunner> file_task_runner =
+      GetTaskRunner(content::BrowserThread::FILE);
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner =
+      GetTaskRunner(content::BrowserThread::IO);
+  scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner =
+      GetTaskRunner(content::BrowserThread::UI);
+
+  // Use the |file_task_runner| as the joiner as it is the only browser-thread
+  // that allows blocking I/O, which is required by thread joining.

[Wez, 2014/10/24 00:28:48]

I thought we had fixed AutoThread?

[kelvinp, 2014/10/24 21:39:41]

Close but not yet.  We agreed to separate that in a separate CL :)

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

I meant land that separate CL so you don't need to hack around it in this one...

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

I will create a separate CL.

+  scoped_refptr<AutoThreadTaskRunner> audio_task_runner =
+      AutoThread::CreateWithType("ChromotingAudioThread", file_task_runner,
+                                 base::MessageLoop::TYPE_IO);
+  scoped_refptr<AutoThreadTaskRunner> input_task_runner =
+      AutoThread::CreateWithType("ChromotingInputThread", file_task_runner,
+                                 base::MessageLoop::TYPE_IO);
+  scoped_refptr<AutoThreadTaskRunner> video_encode_task_runner =
+      AutoThread::CreateWithType("ChromotingEncodeThread", file_task_runner,
+                                 base::MessageLoop::TYPE_IO);
+
+  return make_scoped_ptr(new ChromotingHostContext(
+      ui_task_runner, audio_task_runner, file_task_runner, input_task_runner,
+      network_task_runner,
+      ui_task_runner,  // |video_capture_task_runner|
+      video_encode_task_runner, url_request_context_getter));

[Wez, 2014/10/24 00:28:48]

More readable to have one line per task-runner here. You could also create all
but the joiner task runner in-line in the ctor params.

[kelvinp, 2014/10/24 21:39:41]

Done.

+}
+#endif  // defined(OS_CHROMEOS)
+
 }  // namespace remoting