Remote assistance on Chrome OS Part IV - It2MeHost

remoting/host/chromoting_host_context.cc

 // 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.
 
 #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"
 
 namespace remoting {
 
 ChromotingHostContext::ChromotingHostContext(
-    AutoThreadTaskRunner* ui_task_runner)
-    : ui_task_runner_(ui_task_runner) {
+      AutoThreadTaskRunner* ui_task_runner,
+      AutoThreadTaskRunner* file_task_runner,
+      AutoThreadTaskRunner* input_task_runner,
+      AutoThreadTaskRunner* network_task_runner,
+      AutoThreadTaskRunner* video_capture_task_runner,
+      net::URLRequestContextGetter* url_request_context_getter,
+      policy::PolicyService* policy_service)
+    : ui_task_runner_(ui_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),
+      url_request_context_getter_(url_request_context_getter),
+      policy_service_(policy_service) {
+
 #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_,
+                                                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);
+      "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_);
+  video_encode_task_runner_ =
+      AutoThread::Create("ChromotingEncodeThread", ui_task_runner);

[Wez, 2014/10/17 17:58:00]

It's strange to have some of the task runners passed in here, but to create a
few in this class; I think it'd be cleaner to move these initializations into
the Create*() functions.

[kelvinp, 2014/10/20 00:21:16]

Done.

 }
 
 ChromotingHostContext::~ChromotingHostContext() {
 }
 
+// static
 scoped_ptr<ChromotingHostContext> ChromotingHostContext::Create(
     scoped_refptr<AutoThreadTaskRunner> ui_task_runner) {
-  DCHECK(ui_task_runner->BelongsToCurrentThread());
+  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_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()) {
+      new ChromotingHostContext(ui_task_runner.get(),

[Wez, 2014/10/17 17:58:00]

Don't use .get() here.

[kelvinp, 2014/10/20 00:21:16]

Done.

+                                file_task_runner.get(),
+                                input_task_runner.get(),
+                                network_task_runner.get(),
+                                video_capture_task_runner.get(),
+                                url_request_context_getter.get(),
+                                NULL));
+
+  if (!context->VerifyInitialized()) {
     context.reset();
   }
 
   return context.Pass();
 }
 
+namespace {

[Wez, 2014/10/17 17:58:00]

Since this is a single function definition, use static rather than an anonymous
namespace.

[kelvinp, 2014/10/20 00:21:16]

What is the benefits of using a single function definition over an anonymous
namespace? 

+// Retrieves the task_runner from the browser thread with |id|.  Must be called
+// on the UI thread of the browser process.
+AutoThreadTaskRunner* GetTaskRunner(content::BrowserThread::ID id) {
+#if defined(OS_CHROMEOS)

[Wez, 2014/10/17 17:58:00]

Why not #ifdef# the entire contents of CreateForChromeOS, and this function?

[kelvinp, 2014/10/20 00:21:16]

I have moved the chromeos factory to a separate file to avoid ifdefs

+  return new AutoThreadTaskRunner(
+      content::BrowserThread::GetMessageLoopProxyForThread(id).get(),
+      base::Bind(&base::DoNothing));
+#else
+  NOTIMPLEMENTED();
+  return nullptr;
+#endif // !defined(OS_CHROMEOS)
+}
+
+}  // 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_refptr<AutoThreadTaskRunner> file_task_runner =
+     GetTaskRunner(content::BrowserThread::FILE);
+  scoped_refptr<AutoThreadTaskRunner> input_task_runner =
+      AutoThread::CreateWithType(
+          "ChromotingInputThread", ui_task_runner, base::MessageLoop::TYPE_IO);
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner =
+      GetTaskRunner(content::BrowserThread::IO);
+  scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner =
+       GetTaskRunner(content::BrowserThread::UI);
+
+  scoped_ptr<ChromotingHostContext> context(
+      new ChromotingHostContext(ui_task_runner.get(),
+                                file_task_runner.get(),
+                                input_task_runner.get(),
+                                network_task_runner.get(),
+                                video_capture_task_runner.get(),
+                                url_request_context_getter.get(),
+                                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();

[Wez, 2014/10/17 17:58:00]

The comment in the header indicates that this tests for threads being started
(hence by suggested renaming ;) but this actually tests for all must-have fields
being set, not just threads.

Perhaps just call this IsInitialized(), and update the comment.

[kelvinp, 2014/10/20 00:21:16]

Done.

+}
+
 scoped_refptr<AutoThreadTaskRunner>
 ChromotingHostContext::audio_task_runner() {
   return audio_task_runner_;
 }
 
 scoped_refptr<AutoThreadTaskRunner>
 ChromotingHostContext::file_task_runner() {
   return file_task_runner_;
 }
 
@@ skipping 20 matching lines @@
 scoped_refptr<AutoThreadTaskRunner>
 ChromotingHostContext::video_encode_task_runner() {
   return video_encode_task_runner_;
 }
 
 scoped_refptr<net::URLRequestContextGetter>
 ChromotingHostContext::url_request_context_getter() {
   return url_request_context_getter_;
 }
 
+policy::PolicyService* ChromotingHostContext::policy_service() {
+  return policy_service_;
+}
+
 }  // namespace remoting