Refactoring out the chromoting jni runtime class in favor of chromoting client runtime.

remoting/client/chromoting_client_runtime.cc

 // Copyright 2016 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/client/chromoting_client_runtime.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/singleton.h"
 #include "base/message_loop/message_loop.h"
+#include "base/task_scheduler/task_scheduler.h"
+#include "remoting/base/chromium_url_request.h"
+#include "remoting/base/telemetry_log_writer.h"
 #include "remoting/base/url_request_context_getter.h"
 
+namespace {
+
+const char kTelemetryBaseUrl[] = "https://remoting-pa.googleapis.com/v1/events";
+
+}  // namespace
+
 namespace remoting {
 
-std::unique_ptr<ChromotingClientRuntime> ChromotingClientRuntime::Create(
-    base::MessageLoopForUI* ui_loop) {
-  DCHECK(ui_loop);
+// static
+ChromotingClientRuntime* ChromotingClientRuntime::GetInstance() {
+  return base::Singleton<ChromotingClientRuntime>::get();
+}
+
+ChromotingClientRuntime::ChromotingClientRuntime() {
+  // TODO(sergeyu): Consider adding separate pools for different task classes.
+  const int kMaxBackgroundThreads = 5;
+  if (!base::TaskScheduler::GetInstance()) {
+    // Make sure TaskScheduler is initialized.

[Yuwei, 2017/03/14 19:40:16]

Please be aware of this CL:

https://codereview.chromium.org/2729523006/

kMaxBackgroundThreads will soon be disallowed.

[nicholss, 2017/03/14 22:56:44]

better land this before them!

+    base::TaskScheduler::CreateAndSetSimpleTaskScheduler(kMaxBackgroundThreads);
+  }
+
+  if (!base::MessageLoop::current()) {
+    VLOG(1) << "Starting main message loop";
+    ui_loop_.reset(new base::MessageLoopForUI());
+#if defined(OS_ANDROID)
+    // On Android, the UI thread is managed by Java, so we need to attach and
+    // start a special type of message loop to allow Chromium code to run tasks.
+    ui_loop_->Start();
+#endif  // OS_ANDROID
+#if defined(OS_IOS)

[Sergey Ulanov, 2017/03/14 19:09:37]

#elif

[nicholss, 2017/03/14 22:56:44]

Done.

+    base::MessageLoopForUI::current()->Attach();
+#endif  // OS_IOS
+  } else {
+    ui_loop_.reset(base::MessageLoopForUI::current());
+  }
+
+#if defined(DEBUG)
+  net::URLFetcher::SetIgnoreCertificateRequests(true);
+#endif  // DEBUG
 
   // |ui_loop_| runs on the main thread, so |ui_task_runner_| will run on the
   // main thread.  We can not kill the main thread when the message loop becomes
   // idle so the callback function does nothing (as opposed to the typical
   // base::MessageLoop::QuitClosure())
-  scoped_refptr<AutoThreadTaskRunner> ui_task_runner = new AutoThreadTaskRunner(
-      ui_loop->task_runner(), base::Bind(&base::DoNothing));
+  ui_task_runner_ = new AutoThreadTaskRunner(ui_loop_->task_runner(),
+                                             base::Bind(&base::DoNothing));
 
-  scoped_refptr<AutoThreadTaskRunner> display_task_runner =
-      AutoThread::Create("native_disp", ui_task_runner);
-  scoped_refptr<AutoThreadTaskRunner> network_task_runner =
-      AutoThread::CreateWithType("native_net", ui_task_runner,
-                                 base::MessageLoop::TYPE_IO);
-  scoped_refptr<AutoThreadTaskRunner> file_task_runner =
-      AutoThread::CreateWithType("native_file", ui_task_runner,
-                                 base::MessageLoop::TYPE_IO);
-  scoped_refptr<net::URLRequestContextGetter> url_requester =
-      new URLRequestContextGetter(network_task_runner, file_task_runner);
+  display_task_runner_ = AutoThread::Create("native_disp", ui_task_runner_);
+  network_task_runner_ = AutoThread::CreateWithType(
+      "native_net", ui_task_runner_, base::MessageLoop::TYPE_IO);
+  file_task_runner_ = AutoThread::CreateWithType("native_file", ui_task_runner_,
+                                                 base::MessageLoop::TYPE_IO);
+  url_requester_ =
+      new URLRequestContextGetter(network_task_runner_, file_task_runner_);
 
-  return base::WrapUnique(new ChromotingClientRuntime(
-      ui_task_runner, display_task_runner, network_task_runner,
-      file_task_runner, url_requester));
+  CreateLogWriter();
 }
 
-ChromotingClientRuntime::ChromotingClientRuntime(
-    scoped_refptr<AutoThreadTaskRunner> ui_task_runner,
-    scoped_refptr<AutoThreadTaskRunner> display_task_runner,
-    scoped_refptr<AutoThreadTaskRunner> network_task_runner,
-    scoped_refptr<AutoThreadTaskRunner> file_task_runner,
-    scoped_refptr<net::URLRequestContextGetter> url_requester)
-    : ui_task_runner_(ui_task_runner),
-      display_task_runner_(display_task_runner),
-      network_task_runner_(network_task_runner),
-      file_task_runner_(file_task_runner),
-      url_requester_(url_requester) {}
+ChromotingClientRuntime::~ChromotingClientRuntime() {
+  if (delegate_) {

[Yuwei, 2017/03/14 19:40:16]

Do we need to support the case when delegate_ is not set? If not I think simply
DCHECKing it should be fine.

Also why is delegate_ checked multiple times in the destructor?

[nicholss, 2017/03/14 22:56:44]

delegate is checked twice because it is accessed twice. and work happens between
the checks. The delegate is not required if the client does not need the
delegate to be assigned.

+    delegate_->RuntimeWillShutdown();
+  } else {
+    DLOG(ERROR) << "ClientRuntime Delegate is null.";
+  }
 
-ChromotingClientRuntime::~ChromotingClientRuntime() {}
+  // Block until tasks blocking shutdown have completed their execution.
+  base::TaskScheduler::GetInstance()->Shutdown();
+
+  if (delegate_) {
+    delegate_->RuntimeDidShutdown();

[Sergey Ulanov, 2017/03/14 19:09:37]

Should we also stop all background here before this call?

How will the client use RuntimeWillShutdown() and RuntimeDidShutdown()?

[nicholss, 2017/03/14 22:56:44]

Not sure, if you tell me how we do that we can try it, I copied what was in
place before and this was what the jni side was doing.

Added comments to document expectations of the delegate.

[Sergey Ulanov, 2017/03/16 07:23:50]

Ah, all other threads are are AutoThreads, and they are essentially being leaked
because they depend on main thread to be destroyed (see 
AutoThread::QuitThread()). This is one of the issues with AutoThread.
I think potentially this class can be updated to use regular thread instead of
AutoThread (e.g. client plugin doesn't use AutoThread). Maybe add a TODO?

Longer term we could migrate most background tasks (except maybe network thread)
to the TaskScheduler and then we won't need these extra threads.

+  } else {
+    DLOG(ERROR) << "ClientRuntime Delegate is null.";

[Sergey Ulanov, 2017/03/14 19:09:37]

this message will be logged multiple times in this destruction, which is not
necessary.

[nicholss, 2017/03/14 22:56:44]

Acknowledged.

+  }
+}
+
+void ChromotingClientRuntime::SetDelegate(
+    ChromotingClientRuntime::Delegate* delegate) {
+  delegate_ = delegate;
+  delegate_->RequestAuthTokenForLogger();
+}
+
+void ChromotingClientRuntime::CreateLogWriter() {
+  if (!network_task_runner()->BelongsToCurrentThread()) {
+    network_task_runner()->PostTask(
+        FROM_HERE, base::Bind(&ChromotingClientRuntime::CreateLogWriter,
+                              base::Unretained(this)));
+    return;
+  }
+  log_writer_.reset(new TelemetryLogWriter(
+      kTelemetryBaseUrl,
+      base::MakeUnique<ChromiumUrlRequestFactory>(url_requester())));
+  log_writer_->SetAuthClosure(
+      base::Bind(&ChromotingClientRuntime::RequestAuthTokenForLogger,
+                 base::Unretained(this)));
+}
+
+void ChromotingClientRuntime::RequestAuthTokenForLogger() {
+  if (delegate_) {
+    delegate_->RequestAuthTokenForLogger();
+  } else {
+    DLOG(ERROR) << "ClientRuntime Delegate is null.";
+  }
+}
+
+TelemetryLogWriter* ChromotingClientRuntime::GetLogWriter() {
+  DCHECK(network_task_runner()->BelongsToCurrentThread());
+  return log_writer_.get();
+}
+
 
 }  // namespace remoting