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

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

The end goal here is to be able to use a refactored version of chromoting jni instance, having
android and iOS share a chromoting instance. But first step is to remove the jni runtime to have
the chromoting instance and all other supporting code use the common client runtime rather than this
wrapped runtime for jni.

R=sergeyu@chromium.org, yuweih@chromium.org

BUG=699788
Review-Url: https://codereview.chromium.org/2745583008
Cr-Commit-Position: refs/heads/master@{#457830}
Committed: https://chromium.googlesource.com/chromium/src/+/825f7ebf3ad0e14664fb91b80031a941747724b7

[nicholss, 2017-03-10 19:04:28]

PTAL! Thanks!

https://codereview.chromium.org/2745583008/diff/1/remoting/client/jni/chromot...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2745583008/diff/1/remoting/client/jni/chromot...
remoting/client/jni/chromoting_jni_instance.h:50:
ChromotingJniInstance(base::WeakPtr<JniClient> jni_client,
So this is a change I am not 100% sure about, I chose to pull the runtime from
the static instance method. 

The downside is this means the runtime is not injectable for tests, but we do
not do this currently. The upside is it makes the client code easier to
understand. But perhaps a little magical. No more pushing around pointers to a
runtime object, just ask the singleton for it's runtime object. 

For tests we could build them without the runtime singleton and link in our own
test version if it comes to that.

https://codereview.chromium.org/2745583008/diff/1/remoting/client/jni/jni_run...
File remoting/client/jni/jni_runtime_delegate.h (right):

https://codereview.chromium.org/2745583008/diff/1/remoting/client/jni/jni_run...
remoting/client/jni/jni_runtime_delegate.h:27: bool
RegisterJniRuntimeDelegate(JNIEnv* env);
So for this to work correctly, the JniRuntimeDelegate creates the first
ChromotingClientRuntime from the singleton and hooks it's self into the delegate
methods. All the things that are android specific happen in this delegate.

https://codereview.chromium.org/2745583008/diff/1/remoting/client/jni/jni_run...
remoting/client/jni/jni_runtime_delegate.h:29: // Houses the global resources on
which the Chromoting components run
This comment should be updated. Will do.

[nicholss, 2017-03-10 19:04:57]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 19:05:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 19:10:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 19:11:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nicholss, 2017-03-10 19:27:11]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 19:27:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 19:34:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 19:34:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nicholss, 2017-03-10 19:38:23]

Patchset #3 (id:40001) has been deleted

[nicholss, 2017-03-10 19:38:30]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 19:39:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 19:53:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 19:53:56]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

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

Thanks for cleaning up the runtime :)

https://codereview.chromium.org/2745583008/diff/1/remoting/client/jni/chromot...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2745583008/diff/1/remoting/client/jni/chromot...
remoting/client/jni/chromoting_jni_instance.h:50:
ChromotingJniInstance(base::WeakPtr<JniClient> jni_client,
On 2017/03/10 19:04:28, nicholss wrote:
> So this is a change I am not 100% sure about, I chose to pull the runtime from
> the static instance method. 
> 
> The downside is this means the runtime is not injectable for tests, but we do
> not do this currently. The upside is it makes the client code easier to
> understand. But perhaps a little magical. No more pushing around pointers to a
> runtime object, just ask the singleton for it's runtime object. 
> 
> For tests we could build them without the runtime singleton and link in our
own
> test version if it comes to that.

Maybe the real runtime object is good enough for the tests. Mocking task runners
inside the runtime is nontrivial anyways... Also given that the runtime is just
a combination of global resources, you may just write some test-only setters for
test.

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_client_runtime.cc (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.cc:50:
ui_loop_.reset(base::MessageLoopForUI::current());
I personally think the UI loop should be an implementation detail of the
delegate.

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_client_runtime.h (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.h:65: // TODO(nicholss): GetLogWriter
is not like the above methods, change to
The name comes from here:

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

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.h:92:
ChromotingClientRuntime::Delegate* delegate_;
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?

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.h:92:
ChromotingClientRuntime::Delegate* delegate_;
Maybe consider DCHECKing |delegate_| when calling functions? Looks like
SetDelegate() is the first thing to do after the instance is created.

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_runtime_delegate.cc (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_runtime_delegate.cc:95:
base::TaskScheduler::GetInstance()->Shutdown();
I feel it's weird to create the TaskScheduler in the runtime and shut it down in
the delegate.

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_runtime_delegate.h (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_runtime_delegate.h:62:
std::unique_ptr<base::MessageLoopForUI> ui_loop_;
Looks like this is not used anywhere.

But I do prefer making the delegate manage the UI loop...

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

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_client_runtime.cc (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.cc:50:
ui_loop_.reset(base::MessageLoopForUI::current());
On 2017/03/10 20:18:53, Yuwei wrote:
> I personally think the UI loop should be an implementation detail of the
> delegate.

If I move it out then every type of client needs to remember to start all of the
run loops and task schedulers. It doubles the amount of maintenance required
when chromium changes how these work. I tried both ways and I liked this way
more. I feel like it belongs to the runtime, not the runtime delegate.

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_client_runtime.h (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.h:65: // TODO(nicholss): GetLogWriter
is not like the above methods, change to
On 2017/03/10 20:18:53, Yuwei wrote:
> The name comes from here:
> 
>
https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...

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

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_runtime_delegate.cc (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_runtime_delegate.cc:95:
base::TaskScheduler::GetInstance()->Shutdown();
On 2017/03/10 20:18:53, Yuwei wrote:
> I feel it's weird to create the TaskScheduler in the runtime and shut it down
in
> the delegate.

Maybe I need to add a RuntimeDidShutdown method for the delegate to deal with
this. Not a bad idea.

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_runtime_delegate.h (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_runtime_delegate.h:62:
std::unique_ptr<base::MessageLoopForUI> ui_loop_;
On 2017/03/10 20:18:53, Yuwei wrote:
> Looks like this is not used anywhere.
> 
> But I do prefer making the delegate manage the UI loop...

Removed.

[nicholss, 2017-03-10 20:53:24]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 20:53:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 21:12:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 21:12:30]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nicholss, 2017-03-10 21:32:20]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 21:33:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 22:06:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 22:06:13]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nicholss, 2017-03-10 23:21:03]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 23:21:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 23:25:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 23:25:49]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nicholss, 2017-03-10 23:30:55]

Patchset #6 (id:120001) has been deleted

[nicholss, 2017-03-10 23:31:03]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 23:31:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 00:08:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 00:08:38]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nicholss, 2017-03-11 00:16:29]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 00:16:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 00:43:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 00:43:02]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[nicholss, 2017-03-13 15:29:50]

Patchset #6 (id:140001) has been deleted

[nicholss, 2017-03-13 15:29:58]

Patchset #7 (id:180001) has been deleted

[nicholss, 2017-03-13 15:31:30]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 15:32:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 16:37:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 16:38:00]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-03-14 19:09:38]

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_client_runtime.h (right):

https://codereview.chromium.org/2745583008/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.h:65: // TODO(nicholss): GetLogWriter
is not like the above methods, change to
On 2017/03/10 20:53:11, nicholss wrote:
> On 2017/03/10 20:18:53, Yuwei wrote:
> > The name comes from here:
> > 
> >
>
https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
> 
> I remember the comment, I just don't agree with the request.

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.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime.cc (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:46: #if defined(OS_IOS)
#elif

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:86:
delegate_->RuntimeDidShutdown();
Should we also stop all background here before this call?

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

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:88: DLOG(ERROR) << "ClientRuntime
Delegate is null.";
this message will be logged multiple times in this destruction, which is not
necessary.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime.h (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:21: struct DefaultSingletonTraits;
Do you need this, given that this file includes base/singleton.h?

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:63: // TODO(nicholss): This should
return a base::weak_ptr<TelemetryLogWriter>
This class owns the log_writer, so I think returning a pointer is appropriate.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:66: // TODO(nicholss): GetLogWriter
is not like the above methods, change to
Yes, log_writer() would be a better name

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:68: TelemetryLogWriter*
GetLogWriter();
Change return type to ChromotingEventLogWriter

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:68: TelemetryLogWriter*
GetLogWriter();
Unlike other stuff this class stores the log writer is not thread-safe. Explain
in the comments that this the log writer must be used only on network thread?

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

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime.cc (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:34: // Make sure TaskScheduler is
initialized.
Please be aware of this CL:

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

kMaxBackgroundThreads will soon be disallowed.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:76: if (delegate_) {
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?

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/jni/jn...
File remoting/client/jni/jni_runtime_delegate.cc (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/jni/jn...
remoting/client/jni/jni_runtime_delegate.cc:41: ChromotingClientRuntime* runtime
=
Looks like the runtime is created before the delegate. At exit will delegate
always be destroyed before the runtime? If that's true then RuntimeWillShutdown
etc. will never be called...

I'm not sure what's the destruction order of singletons though, but we probably
still need some documentations for the order of destruction.

[nicholss, 2017-03-14 21:47:28]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 21:48:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 21:54:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 21:54:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nicholss, 2017-03-14 22:01:48]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 22:02:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 22:10:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 22:10:12]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nicholss, 2017-03-14 22:39:17]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 22:40:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nicholss, 2017-03-14 22:56:45]

Thanks for the feedback, updated now.

PTAL

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime.cc (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:34: // Make sure TaskScheduler is
initialized.
On 2017/03/14 19:40:16, Yuwei wrote:
> Please be aware of this CL:
> 
> https://codereview.chromium.org/2729523006/
> 
> kMaxBackgroundThreads will soon be disallowed.

better land this before them!

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:46: #if defined(OS_IOS)
On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> #elif

Done.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:76: if (delegate_) {
On 2017/03/14 19:40:16, Yuwei wrote:
> 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?

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.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:86:
delegate_->RuntimeDidShutdown();
On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> Should we also stop all background here before this call?
> 
> How will the client use RuntimeWillShutdown() and RuntimeDidShutdown()?

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.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:88: DLOG(ERROR) << "ClientRuntime
Delegate is null.";
On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> this message will be logged multiple times in this destruction, which is not
> necessary.

Acknowledged.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime.h (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:21: struct DefaultSingletonTraits;
On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> Do you need this, given that this file includes base/singleton.h?

I was not suppose to have base/singleton in this h file, removed in include.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:63: // TODO(nicholss): This should
return a base::weak_ptr<TelemetryLogWriter>
On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> This class owns the log_writer, so I think returning a pointer is appropriate.

Ok thanks

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:66: // TODO(nicholss): GetLogWriter
is not like the above methods, change to
On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> Yes, log_writer() would be a better name

Done.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:68: TelemetryLogWriter*
GetLogWriter();
On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> Change return type to ChromotingEventLogWriter

Done.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:68: TelemetryLogWriter*
GetLogWriter();
On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> Unlike other stuff this class stores the log writer is not thread-safe.
Explain
> in the comments that this the log writer must be used only on network thread?

Done.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/jni/jn...
File remoting/client/jni/jni_runtime_delegate.cc (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/jni/jn...
remoting/client/jni/jni_runtime_delegate.cc:41: ChromotingClientRuntime* runtime
=
On 2017/03/14 19:40:16, Yuwei wrote:
> Looks like the runtime is created before the delegate. At exit will delegate
> always be destroyed before the runtime? If that's true then
RuntimeWillShutdown
> etc. will never be called...
> 
> I'm not sure what's the destruction order of singletons though, but we
probably
> still need some documentations for the order of destruction.

To be honest, I am not sure.

It might work out that we have to make the delegate not a singleton. I think it
will have to be looked at further after this is in, and a followup tweak CL can
tune it.

[commit-bot: I haz the power, 2017-03-14 23:04:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:04:11]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nicholss, 2017-03-14 23:08:24]

Patchset #10 (id:260001) has been deleted

[nicholss, 2017-03-14 23:08:29]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 23:09:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 23:28:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:28:57]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[nicholss, 2017-03-15 16:24:53]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 16:25:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nicholss, 2017-03-15 16:50:17]

Patchset #10 (id:280001) has been deleted

[commit-bot: I haz the power, 2017-03-15 17:37:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 17:37:20]

Dry run: This issue passed the CQ dry run.

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

lgtm, but please see my comments

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime.cc (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:86:
delegate_->RuntimeDidShutdown();
On 2017/03/14 22:56:44, nicholss wrote:
> On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> > Should we also stop all background here before this call?
> > 
> > How will the client use RuntimeWillShutdown() and RuntimeDidShutdown()?
> 
> 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.

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.

> 
> Added comments to document expectations of the delegate.

[Yuwei, 2017-03-16 08:38:53]

LGTM

Don't forget to verify that the host connection scenario doesn't crash for
unknown reason :)

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime.h (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:63: // TODO(nicholss): This should
return a base::weak_ptr<TelemetryLogWriter>
On 2017/03/14 22:56:44, nicholss wrote:
> On 2017/03/14 19:09:37, Sergey Ulanov wrote:
> > This class owns the log_writer, so I think returning a pointer is
appropriate.
> 
> Ok thanks

WeakPtr is not necessary as long as the runtime dies after anything else.

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/jni/jn...
File remoting/client/jni/jni_runtime_delegate.cc (right):

https://codereview.chromium.org/2745583008/diff/200001/remoting/client/jni/jn...
remoting/client/jni/jni_runtime_delegate.cc:41: ChromotingClientRuntime* runtime
=
On 2017/03/14 22:56:44, nicholss wrote:
> On 2017/03/14 19:40:16, Yuwei wrote:
> > Looks like the runtime is created before the delegate. At exit will delegate
> > always be destroyed before the runtime? If that's true then
> RuntimeWillShutdown
> > etc. will never be called...
> > 
> > I'm not sure what's the destruction order of singletons though, but we
> probably
> > still need some documentations for the order of destruction.
> 
> To be honest, I am not sure.
> 
> It might work out that we have to make the delegate not a singleton. I think
it
> will have to be looked at further after this is in, and a followup tweak CL
can
> tune it.

It's probably okay for now as long as the app doesn't crash at exit. Please make
a TODO here for fixing this issue.

https://codereview.chromium.org/2745583008/diff/300001/remoting/client/jni/BU...
File remoting/client/jni/BUILD.gn (right):

https://codereview.chromium.org/2745583008/diff/300001/remoting/client/jni/BU...
remoting/client/jni/BUILD.gn:55: "OpenSLES",
You don't need OpenSLES here any more since it's used only by /client

[nicholss, 2017-03-17 17:17:59]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-03-17 17:18:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 18:48:49]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1489771079212090,
"parent_rev": "db14329b13d1109723acf5528a0253d6ee8c1651", "commit_rev":
"825f7ebf3ad0e14664fb91b80031a941747724b7"}

[commit-bot: I haz the power, 2017-03-17 18:49:29]

Description was changed from

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

The end goal here is to be able to use a refactored version of chromoting jni
instance, having
android and iOS share a chromoting instance. But first step is to remove the jni
runtime to have
the chromoting instance and all other supporting code use the common client
runtime rather than this
wrapped runtime for jni.

R=sergeyu@chromium.org, yuweih@chromium.org

BUG=699788
==========

to

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

The end goal here is to be able to use a refactored version of chromoting jni
instance, having
android and iOS share a chromoting instance. But first step is to remove the jni
runtime to have
the chromoting instance and all other supporting code use the common client
runtime rather than this
wrapped runtime for jni.

R=sergeyu@chromium.org, yuweih@chromium.org

BUG=699788

Review-Url: https://codereview.chromium.org/2745583008
Cr-Commit-Position: refs/heads/master@{#457830}
Committed:
https://chromium.googlesource.com/chromium/src/+/825f7ebf3ad0e14664fb91b80031...
==========

[commit-bot: I haz the power, 2017-03-17 18:49:31]

Committed patchset #10 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/825f7ebf3ad0e14664fb91b80031...