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

remoting/client/chromoting_client_runtime.h

Index: remoting/client/chromoting_client_runtime.h
diff --git a/remoting/client/chromoting_client_runtime.h b/remoting/client/chromoting_client_runtime.h
index 69efb0dfc2843472a20b879d686420c6a69274aa..dafaa7105e0a885a80922c30b33d565efad0b961 100644
--- a/remoting/client/chromoting_client_runtime.h
+++ b/remoting/client/chromoting_client_runtime.h
@@ -8,11 +8,17 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/singleton.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "remoting/base/auto_thread.h"
+#include "remoting/base/telemetry_log_writer.h"
 
 namespace base {
 class MessageLoopForUI;
+
+template <typename T>
+struct DefaultSingletonTraits;
 }  // namespace base
 
 // Houses the global resources on which the Chromoting components run
@@ -21,28 +27,17 @@ namespace remoting {
 
 class ChromotingClientRuntime {
  public:
-  // Caller to create is responsible for creating and attaching a new ui thread
-  // for use. Example:
-  //
-  // 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.
-  //
-  //  base::MessageLoopForUI *ui_loop = new base::MessageLoopForUI();
-  //  ui_loop_->Start();
-  //  std::unique_ptr<ChromotingClientRuntime> runtime =
-  //    ChromotingClientRuntime::Create(ui_loop);
-  //
-  // On iOS we created a new message loop and now attach it.
-  //
-  //  base::MessageLoopForUI *ui_loop = new base::MessageLoopForUI();
-  //  ui_loop_->Attach();
-  //  std::unique_ptr<ChromotingClientRuntime> runtime =
-  //    ChromotingClientRuntime::Create(ui_loop);
-  //
-  static std::unique_ptr<ChromotingClientRuntime> Create(
-      base::MessageLoopForUI* ui_loop);
-
-  ~ChromotingClientRuntime();
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    virtual void RuntimeWillShutdown() = 0;
+    virtual void RequestAuthTokenForLogger() = 0;
+  };
+
+  static ChromotingClientRuntime* GetInstance();
+
+  void SetDelegate(ChromotingClientRuntime::Delegate* delegate);
 
   scoped_refptr<AutoThreadTaskRunner> network_task_runner() {
     return network_task_runner_;
@@ -64,14 +59,23 @@ class ChromotingClientRuntime {
     return url_requester_;
   }
 
+  // TODO(nicholss): This should return a base::weak_ptr<TelemetryLogWriter>
+  // At some point the application should set the auth closure when auth is
+  // possible.
+  // TODO(nicholss): GetLogWriter is not like the above methods, change to

[Yuwei, 2017/03/10 20:18:53]

The name comes from here:

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...

[nicholss, 2017/03/10 20:53:11]

I remember the comment, I just don't agree with the request.

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

In that CL GetLogWriter() was calling GetWeakPtr(), that was the reason
GetLogWriter() was a better name. It looks like the style guide was changed
recently and is now more ambiguous in this regard. Currently it says:
  Accessors and mutators (get and set functions) may be named like variables.
These 
  often correspond to actual member variables, but this is not required. For
example,
  int count() and void set_count(int count).

The previous version  used to say the following
(https://github.com/google/styleguide/commit/db0a26320f3e930c6ea7225ed53539b4f...):
  Functions that are very cheap to call may instead follow the style for 
  variable names (all lower-case, with underscores between words). The 
  rule of thumb is that such a function should be so cheap that you normally
  wouldn't bother caching its return value when calling it in a loop. A 
  canonical example is an inline method that just returns one of the 
  class's member variables.

I liked the old wording better, as it was less ambiguous. But in either case,
according to the new wording both names would be acceptable (and GetWeakPtr() is
irrelevant). I think log_writer() may be better for consistency.

+  // log_writer?
+  TelemetryLogWriter* GetLogWriter();
+
  private:
   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);
+  virtual ~ChromotingClientRuntime();
+
+  void CreateLogWriter();
+  void RequestAuthTokenForLogger();
+
+  // Chromium code's connection to the app message loop. Once created the
+  // MessageLoop will live for the life of the program.
+  std::unique_ptr<base::MessageLoopForUI> ui_loop_;
 
   // References to native threads.
   scoped_refptr<AutoThreadTaskRunner> ui_task_runner_;
@@ -82,6 +86,13 @@ class ChromotingClientRuntime {
 
   scoped_refptr<net::URLRequestContextGetter> url_requester_;
 
+  // For logging session stage changes and stats.
+  std::unique_ptr<TelemetryLogWriter> log_writer_;
+
+  ChromotingClientRuntime::Delegate* delegate_;

[Yuwei, 2017/03/10 20:18:53]

Maybe consider DCHECKing |delegate_| when calling functions? Looks like
SetDelegate() is the first thing to do after the instance is created.

[Yuwei, 2017/03/10 20:18:53]

Looks like both the runtime and the delegate store the raw pointers of each
other.

They are both singletons so I'm not sure which will be destroyed first?

+
+  friend struct base::DefaultSingletonTraits<ChromotingClientRuntime>;
+
   DISALLOW_COPY_AND_ASSIGN(ChromotingClientRuntime);
 };