Adding container class for chromoting client runtimes.

Adding container class for chromoting client runtimes.

R=lambroslambrou@chromium.org

Committed: https://crrev.com/b34c0f6877af54e8bdd6052bf8934dd46a3b5899
Cr-Commit-Position: refs/heads/master@{#380979}

[nicholss, 2016-03-03 00:55:16]

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
File remoting/client/chromoting_client_runtime.cc (right):

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.cc:23:
ChromotingClientRuntime::ChromotingClientRuntime() {
I am going to be using this in the iOS app and am thinking of this being the
parent of remote/client/jni/chromoting_jni_runtime

[nicholss, 2016-03-03 18:58:35]

Description was changed from

==========
Adding base class for chromoting client runtimes.

R=lambroslambrou@chromium.org
==========

to

==========
Adding base class for chromoting client runtimes.

R=lambroslambrou@chromium.org
==========

[nicholss, 2016-03-03 18:58:36]

nicholss@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Lambros, 2016-03-03 19:15:08]

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
File remoting/client/chromoting_client_runtime.cc (right):

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.cc:10: #include
"base/synchronization/waitable_event.h"
not used?

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.cc:11: #include
"net/url_request/url_request_context.h"
not used?

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.cc:23:
ChromotingClientRuntime::ChromotingClientRuntime() {
On 2016/03/03 00:55:16, nicholss wrote:
> I am going to be using this in the iOS app and am thinking of this being the
> parent of remote/client/jni/chromoting_jni_runtime

We usually prefer aggregation instead of (non-interface) inheritance. Is there
some reason to prefer inheritance in this case?
https://google.github.io/styleguide/cppguide.html#Inheritance

The normal pattern is to have the cross-platform code call out to
platform-specific code via an interface. See for example:
remoting/host/setup/daemon_controller.h
and
remoting/host/setup/daemon_controller_delegate_{linux,mac.win}.h

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.cc:26: ui_loop_ = new
base::MessageLoop(base::MessageLoop::TYPE_UI);
Why are you starting a message-loop in this new code? Are you going to be
refactoring all the existing client code to use this new class?

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.cc:33: VLOG(1) << "Spawning additional
threads";
Remove this log?

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.cc:48: new
URLRequestContextGetter(network_task_runner_, file_task_runner_);
Don't you need
#include "remoting/base/url_request_context_getter.h"
for this?

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
File remoting/client/chromoting_client_runtime.h (right):

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.h:5: #ifndef
REMOTING_CLIENT_CHROMOTING_CLIENT_CONTEXT_H_
Fix header guard.

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.h:11: #include
"base/memory/scoped_ptr.h"
not used?

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.h:12: #include
"base/memory/weak_ptr.h"
not used?

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.h:16: #include
"remoting/base/url_request_context_getter.h"
I think this should be "net/url_request/url_request_context_getter.h"
and put this remoting-specific header in the .cc file.

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.h:17: #include
"remoting/client/chromoting_client.h"
not used? (this, and all the ones below as well)

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.h:32: // we close. Its components are
reused across |ChromotingJniInstance|s.
Jni is specific to Android.

https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
remoting/client/chromoting_client_runtime.h:64: // MessageLoop requires ARC to
be disabled.
ARC is Mac-specific, but this file is cross-platform code?

[nicholss, 2016-03-03 19:36:45]

On 2016/03/03 19:15:08, Lambros wrote:
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> File remoting/client/chromoting_client_runtime.cc (right):
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.cc:10: #include
> "base/synchronization/waitable_event.h"
> not used?
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.cc:11: #include
> "net/url_request/url_request_context.h"
> not used?
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.cc:23:
> ChromotingClientRuntime::ChromotingClientRuntime() {
> On 2016/03/03 00:55:16, nicholss wrote:
> > I am going to be using this in the iOS app and am thinking of this being the
> > parent of remote/client/jni/chromoting_jni_runtime
> 
> We usually prefer aggregation instead of (non-interface) inheritance. Is there
> some reason to prefer inheritance in this case?
> https://google.github.io/styleguide/cppguide.html#Inheritance
> 
> The normal pattern is to have the cross-platform code call out to
> platform-specific code via an interface. See for example:
> remoting/host/setup/daemon_controller.h
> and
> remoting/host/setup/daemon_controller_delegate_{linux,mac.win}.h
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.cc:26: ui_loop_ = new
> base::MessageLoop(base::MessageLoop::TYPE_UI);
> Why are you starting a message-loop in this new code? Are you going to be
> refactoring all the existing client code to use this new class?
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.cc:33: VLOG(1) << "Spawning
additional
> threads";
> Remove this log?
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.cc:48: new
> URLRequestContextGetter(network_task_runner_, file_task_runner_);
> Don't you need
> #include "remoting/base/url_request_context_getter.h"
> for this?
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> File remoting/client/chromoting_client_runtime.h (right):
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.h:5: #ifndef
> REMOTING_CLIENT_CHROMOTING_CLIENT_CONTEXT_H_
> Fix header guard.
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.h:11: #include
> "base/memory/scoped_ptr.h"
> not used?
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.h:12: #include
> "base/memory/weak_ptr.h"
> not used?
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.h:16: #include
> "remoting/base/url_request_context_getter.h"
> I think this should be "net/url_request/url_request_context_getter.h"
> and put this remoting-specific header in the .cc file.
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.h:17: #include
> "remoting/client/chromoting_client.h"
> not used? (this, and all the ones below as well)
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.h:32: // we close. Its components
are
> reused across |ChromotingJniInstance|s.
> Jni is specific to Android.
> 
>
https://codereview.chromium.org/1764503002/diff/1/remoting/client/chromoting_...
> remoting/client/chromoting_client_runtime.h:64: // MessageLoop requires ARC to
> be disabled.
> ARC is Mac-specific, but this file is cross-platform code?


Thank you very much for the feedback. I have cleaned up the includes and did
some minor trimming based on comments. PTAL.

[Lambros, 2016-03-03 21:57:22]

I'm not sure about the inheritance model, but everything else LG. Defer to
Sergey for final LG.

[Sergey Ulanov, 2016-03-04 22:50:17]

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

https://codereview.chromium.org/1764503002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.cc:14: void DoNothing() {
Please use base::DoNothing().

https://codereview.chromium.org/1764503002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.cc:49: return
base::Singleton<ChromotingClientRuntime>::get();
Does this need to be a singleton?
Potentially there may be multiple concurrent sessions and ideally we'd want to
use separate objects for each of them (e.g. to avoid sharing threads).

https://codereview.chromium.org/1764503002/diff/40001/remoting/client/ios/chr...
File remoting/client/ios/chromoting_client_runtime_ios.cc (right):

https://codereview.chromium.org/1764503002/diff/40001/remoting/client/ios/chr...
remoting/client/ios/chromoting_client_runtime_ios.cc:19:
ChromotingClientRuntimeIos::ChromotingClientRuntime() {
There is nothing in this class that's specific to iOS. Why can't it be shared
with Android?

https://codereview.chromium.org/1764503002/diff/40001/remoting/client/ios/chr...
remoting/client/ios/chromoting_client_runtime_ios.cc:50:
ChromotingClientRuntimeIos::~ChromotingClientRuntime() {}
You are missing Ios suffix in the name, so this code won't compile.

[nicholss, 2016-03-07 18:32:36]

I am about to push a new change that uses this runtime in the jni code

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

https://codereview.chromium.org/1764503002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.cc:14: void DoNothing() {
On 2016/03/04 22:50:17, Sergey Ulanov wrote:
> Please use base::DoNothing().

Done.

https://codereview.chromium.org/1764503002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.cc:49: return
base::Singleton<ChromotingClientRuntime>::get();
On 2016/03/04 22:50:17, Sergey Ulanov wrote:
> Does this need to be a singleton?
> Potentially there may be multiple concurrent sessions and ideally we'd want to
> use separate objects for each of them (e.g. to avoid sharing threads).

It is very convenient if this is a singleton. 

It gives a way to get access to the runtime threads without having to pass a
pointer to the container object everywhere. If someone wanted to manage multiple
sessions, potentially the client viewer could be new process or we make a new
kind of runtime, but for the case of mobile it is easier to have the runtime as
a singleton.

https://codereview.chromium.org/1764503002/diff/40001/remoting/client/ios/chr...
File remoting/client/ios/chromoting_client_runtime_ios.cc (right):

https://codereview.chromium.org/1764503002/diff/40001/remoting/client/ios/chr...
remoting/client/ios/chromoting_client_runtime_ios.cc:19:
ChromotingClientRuntimeIos::ChromotingClientRuntime() {
On 2016/03/04 22:50:17, Sergey Ulanov wrote:
> There is nothing in this class that's specific to iOS. Why can't it be shared
> with Android?

The Android runtime has a bunch of jni connections and I will remove this
version when I am able to merge them. I think I will have to create a new thing
that manages just threads and the JniRuntime will have one of those objects.
Originally I was going to extend this class but Lambros said we should not use
inheritance, we should use composition instead. Agree?

https://codereview.chromium.org/1764503002/diff/40001/remoting/client/ios/chr...
remoting/client/ios/chromoting_client_runtime_ios.cc:50:
ChromotingClientRuntimeIos::~ChromotingClientRuntime() {}
On 2016/03/04 22:50:17, Sergey Ulanov wrote:
> You are missing Ios suffix in the name, so this code won't compile.

Acknowledged.

[nicholss, 2016-03-07 18:41:56]

https://codereview.chromium.org/1764503002/diff/60001/remoting/client/jni/chr...
File remoting/client/jni/chromoting_jni_runtime.h (right):

https://codereview.chromium.org/1764503002/diff/60001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_runtime.h:37: return
runtime_-ui_task_runner();
Doing this to be backwards compatible.

[nicholss, 2016-03-07 21:56:37]

Description was changed from

==========
Adding base class for chromoting client runtimes.

R=lambroslambrou@chromium.org
==========

to

==========
Adding container class for chromoting client runtimes.

R=lambroslambrou@chromium.org
==========

[nicholss, 2016-03-08 16:44:29]

PTAL.

I have moved my simple thread containing class into a container that is used by
jni runtime to grab threads to run with.

[Sergey Ulanov, 2016-03-08 18:53:46]

I still don't think we want to add any singletons.

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

https://codereview.chromium.org/1764503002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.cc:49: return
base::Singleton<ChromotingClientRuntime>::get();
On 2016/03/07 18:32:36, nicholss wrote:
> On 2016/03/04 22:50:17, Sergey Ulanov wrote:
> > Does this need to be a singleton?
> > Potentially there may be multiple concurrent sessions and ideally we'd want
to
> > use separate objects for each of them (e.g. to avoid sharing threads).
> 
> It is very convenient if this is a singleton. 
> 
> It gives a way to get access to the runtime threads without having to pass a
> pointer to the container object everywhere. If someone wanted to manage
multiple
> sessions, potentially the client viewer could be new process or we make a new
> kind of runtime, but for the case of mobile it is easier to have the runtime
as
> a singleton. 

There are cases when we will need to have multiple concurrent sessions, so it's
better to support this case from the start. We already pass all threads both in
the client and in the host without singletons (except on Android, but Lambros is
working on cleaning it up: crbug.com/526336). I don't think that it introduced
any extra complexity. Passing task_runner makes dependency between dependencies
on threads very explicit.

https://codereview.chromium.org/1764503002/diff/220001/remoting/client/BUILD.gn
File remoting/client/BUILD.gn (right):

https://codereview.chromium.org/1764503002/diff/220001/remoting/client/BUILD....
remoting/client/BUILD.gn:14: if (!is_nacl) {
there is if(is_nacl) below. You can move this to else case there?

[nicholss, 2016-03-08 21:20:25]

I think that make it good?

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

https://codereview.chromium.org/1764503002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_client_runtime.cc:49: return
base::Singleton<ChromotingClientRuntime>::get();
On 2016/03/08 18:53:46, Sergey Ulanov wrote:
> On 2016/03/07 18:32:36, nicholss wrote:
> > On 2016/03/04 22:50:17, Sergey Ulanov wrote:
> > > Does this need to be a singleton?
> > > Potentially there may be multiple concurrent sessions and ideally we'd
want
> to
> > > use separate objects for each of them (e.g. to avoid sharing threads).
> > 
> > It is very convenient if this is a singleton. 
> > 
> > It gives a way to get access to the runtime threads without having to pass a
> > pointer to the container object everywhere. If someone wanted to manage
> multiple
> > sessions, potentially the client viewer could be new process or we make a
new
> > kind of runtime, but for the case of mobile it is easier to have the runtime
> as
> > a singleton. 
> 
> There are cases when we will need to have multiple concurrent sessions, so
it's
> better to support this case from the start. We already pass all threads both
in
> the client and in the host without singletons (except on Android, but Lambros
is
> working on cleaning it up: crbug.com/526336). I don't think that it introduced
> any extra complexity. Passing task_runner makes dependency between
dependencies
> on threads very explicit.

Done.

https://codereview.chromium.org/1764503002/diff/220001/remoting/client/BUILD.gn
File remoting/client/BUILD.gn (right):

https://codereview.chromium.org/1764503002/diff/220001/remoting/client/BUILD....
remoting/client/BUILD.gn:14: if (!is_nacl) {
On 2016/03/08 18:53:46, Sergey Ulanov wrote:
> there is if(is_nacl) below. You can move this to else case there?

Done.

[nicholss, 2016-03-08 23:36:20]

PTAL

[Lambros, 2016-03-09 00:06:18]

https://codereview.chromium.org/1764503002/diff/300001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime_unittest.cc (right):

https://codereview.chromium.org/1764503002/diff/300001/remoting/client/chromo...
remoting/client/chromoting_client_runtime_unittest.cc:22: EXPECT_TRUE(runtime);
EXPECT_TRUE(x); if (!x) return;
can be replaced with
ASSERT_TRUE(x);

https://codereview.chromium.org/1764503002/diff/300001/remoting/client/chromo...
remoting/client/chromoting_client_runtime_unittest.cc:31: runtime.reset();
This line is a no-op, |runtime| goes out of scope here anyway?

https://codereview.chromium.org/1764503002/diff/300001/remoting/remoting_test...
File remoting/remoting_test.gypi (right):

https://codereview.chromium.org/1764503002/diff/300001/remoting/remoting_test...
remoting/remoting_test.gypi:235: 'client/chromoting_client_runtime_unittest.cc',
nit: swap with previous line

[nicholss, 2016-03-09 16:54:28]

https://codereview.chromium.org/1764503002/diff/300001/remoting/client/chromo...
File remoting/client/chromoting_client_runtime_unittest.cc (right):

https://codereview.chromium.org/1764503002/diff/300001/remoting/client/chromo...
remoting/client/chromoting_client_runtime_unittest.cc:22: EXPECT_TRUE(runtime);
On 2016/03/09 00:06:18, Lambros wrote:
> EXPECT_TRUE(x); if (!x) return;
> can be replaced with
> ASSERT_TRUE(x);

Done.

https://codereview.chromium.org/1764503002/diff/300001/remoting/client/chromo...
remoting/client/chromoting_client_runtime_unittest.cc:31: runtime.reset();
On 2016/03/09 00:06:18, Lambros wrote:
> This line is a no-op, |runtime| goes out of scope here anyway?

Done.

https://codereview.chromium.org/1764503002/diff/300001/remoting/remoting_test...
File remoting/remoting_test.gypi (right):

https://codereview.chromium.org/1764503002/diff/300001/remoting/remoting_test...
remoting/remoting_test.gypi:235: 'client/chromoting_client_runtime_unittest.cc',
On 2016/03/09 00:06:18, Lambros wrote:
> nit: swap with previous line

Done.

[nicholss, 2016-03-10 00:13:51]

PTAL?

[Lambros, 2016-03-10 00:52:15]

lgtm, please see comments

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

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:23:
base::MessageLoopForUI::current()->Attach();
ui_loop_->Attach(); maybe?

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:29: NOTREACHED();
This file seems to be specific to mobile clients.
I'm not sure it makes sense to compile this code on a platform where it will be
NOTREACHED() ?

It feels like this should be:
#error Not implemented.
instead.

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:46: file_task_runner_ =
AutoThread::CreateWithType("native_file", ui_task_runner_,
Adding a new thread (and passing it to URLRequestContextGetter) is a material
change on Android. It looks OK, but please make sure to test your changes on
that platform (at least a couple of connect/disconnect cycles).

https://codereview.chromium.org/1764503002/diff/340001/remoting/remoting_clie...
File remoting/remoting_client.gypi (right):

https://codereview.chromium.org/1764503002/diff/340001/remoting/remoting_clie...
remoting/remoting_client.gypi:23: '<@(remoting_client_standalone_sources)',
Should we compile these files for non-mobile platforms?

[Sergey Ulanov, 2016-03-11 01:22:34]

LGTM, when my comments are addressed.

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

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:18: if
(!base::MessageLoop::current()) {
Maybe just replace it with a DCHECK and make the calling code responsible for
MessageLoop creation. That way you can avoid the ifdefs below.

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

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:10: #include
"base/message_loop/message_loop.h"
Don't need to include this here. Just forward-declare MessageLoopForUI.

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:12: #include
"remoting/base/auto_thread.h"
forward-declare AutoThreadTaskRunner

[nicholss, 2016-03-11 21:23:02]

I think this is ready.

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

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:18: if
(!base::MessageLoop::current()) {
On 2016/03/11 01:22:34, Sergey Ulanov wrote:
> Maybe just replace it with a DCHECK and make the calling code responsible for
> MessageLoop creation. That way you can avoid the ifdefs below.

Ok, I moved this out, it is starting to look more and more like
ChromotingHostContext. Maybe we could make a ChromotingContext in the future
with some options how to set the threads.

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:23:
base::MessageLoopForUI::current()->Attach();
On 2016/03/10 00:52:15, Lambros wrote:
> ui_loop_->Attach(); maybe?

Done.

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:29: NOTREACHED();
On 2016/03/10 00:52:15, Lambros wrote:
> This file seems to be specific to mobile clients.
> I'm not sure it makes sense to compile this code on a platform where it will
be
> NOTREACHED() ?
> 
> It feels like this should be:
> #error Not implemented.
> instead.

This is fixed with the moving of the main thread generation. #if's are moved
out.

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:46: file_task_runner_ =
AutoThread::CreateWithType("native_file", ui_task_runner_,
On 2016/03/10 00:52:15, Lambros wrote:
> Adding a new thread (and passing it to URLRequestContextGetter) is a material
> change on Android. It looks OK, but please make sure to test your changes on
> that platform (at least a couple of connect/disconnect cycles).

Testing now.

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

https://codereview.chromium.org/1764503002/diff/340001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.h:10: #include
"base/message_loop/message_loop.h"
On 2016/03/11 01:22:34, Sergey Ulanov wrote:
> Don't need to include this here. Just forward-declare MessageLoopForUI.

Done.

[Sergey Ulanov, 2016-03-11 21:41:45]

lgtm

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

https://codereview.chromium.org/1764503002/diff/420001/remoting/client/chromo...
remoting/client/chromoting_client_runtime.cc:14: // Caller to create is
responsible for creating and attaching a new ui thread
nit: move this comment to the .cc file please.

[nicholss, 2016-03-11 22:51:30]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2016-03-11 22:51:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1764503002/#ps440001
(title: "Moving usage comment to .h file. Now more simple.")

[commit-bot: I haz the power, 2016-03-11 22:52:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1764503002/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1764503002/440001

[commit-bot: I haz the power, 2016-03-12 00:53:37]

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

[commit-bot: I haz the power, 2016-03-12 00:53:38]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[nicholss, 2016-03-14 14:46:20]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2016-03-14 14:46:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1764503002/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1764503002/440001

[commit-bot: I haz the power, 2016-03-14 15:41:12]

Description was changed from

==========
Adding container class for chromoting client runtimes.

R=lambroslambrou@chromium.org
==========

to

==========
Adding container class for chromoting client runtimes.

R=lambroslambrou@chromium.org
==========

[commit-bot: I haz the power, 2016-03-14 15:41:13]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-03-14 15:42:22]

Description was changed from

==========
Adding container class for chromoting client runtimes.

R=lambroslambrou@chromium.org
==========

to

==========
Adding container class for chromoting client runtimes.

R=lambroslambrou@chromium.org

Committed: https://crrev.com/b34c0f6877af54e8bdd6052bf8934dd46a3b5899
Cr-Commit-Position: refs/heads/master@{#380979}
==========

[commit-bot: I haz the power, 2016-03-14 15:42:24]

Patchset 23 (id:??) landed as
https://crrev.com/b34c0f6877af54e8bdd6052bf8934dd46a3b5899
Cr-Commit-Position: refs/heads/master@{#380979}

[Lambros, 2016-03-21 20:39:49]

A revert of this CL (patchset #23 id:440001) has been created in
https://codereview.chromium.org/1821623003/ by lambroslambrou@chromium.org.

The reason for reverting is: Broke Android client and tests - see bug 596240..