[Chromoting] Introducing refcount-based life time management of the message loops in the service (d…

[Chromoting] Introducing refcount-based life time management of the message loops in the service (daemon) and me2me host (network) processes.

This CL introduces AutoMessageLoop wrapper that provides control over life time of a message loop via scoped_refptr references. This scheme is useful in the cases when shutdown code has to run on a particular thread or when the OS requires resources (such as windows) to be freed before exiting a message loop.

The CL switches threads, owned by remoting::HostService, remoting::HostProcess and remoting::ChromotingHostContext, to refcount-based lifetime management. This change required updating tear-down sequences in remoting_me2me_host and the host plugin code.

BUG=134694
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154827

[alexeypa (please no reviews), 2012-08-21 20:25:28]

PTAL.

[alexeypa (please no reviews), 2012-08-22 23:13:46]

On 2012/08/21 20:25:28, alexeypa wrote:
> PTAL.

Ping.

[Wez, 2012-08-24 20:16:29]

On 2012/08/22 23:13:46, alexeypa wrote:
> On 2012/08/21 20:25:28, alexeypa wrote:
> > PTAL.
> 
> Ping.

Please update the CL description to clarify exactly what changes this CL makes,
e.g. which threads have been switched to this "automatic" lifetime management.

[alexeypa (please no reviews), 2012-08-24 21:04:43]

On 2012/08/24 20:16:29, Wez wrote:
> Please update the CL description to clarify exactly what changes this CL
makes,
> e.g. which threads have been switched to this "automatic" lifetime management.

Done.

[Wez, 2012-08-24 21:30:49]

http://codereview.chromium.org/10829467/diff/9002/remoting/base/auto_message_...
File remoting/base/auto_message_loop.cc (right):

http://codereview.chromium.org/10829467/diff/9002/remoting/base/auto_message_...
remoting/base/auto_message_loop.cc:36: // Deletes |this| on the message loop and
exists the loop.
typo: exits

http://codereview.chromium.org/10829467/diff/9002/remoting/base/auto_thread.cc
File remoting/base/auto_thread.cc (right):

http://codereview.chromium.org/10829467/diff/9002/remoting/base/auto_thread.c...
remoting/base/auto_thread.cc:52: thread_.Stop();
This arrangement means that once no-one needs the AutoThread any more the parent
will try to stop & join it, which may cause the parent to be blocked if the
thread is processing a long-lived task at the time; I don't think that's what we
want here.

http://codereview.chromium.org/10829467/diff/9002/remoting/base/auto_thread.h
File remoting/base/auto_thread.h (right):

http://codereview.chromium.org/10829467/diff/9002/remoting/base/auto_thread.h...
remoting/base/auto_thread.h:25: scoped_refptr<base::SingleThreadTaskRunner>
parent);
I don't really see the point of this class; why can't you start a Thread, wrap
it with an AutoMessageLoop to cause it to exit when no-one needs it any more,
and then just join it in the caller when you absolutely positively have to? 
What does this class actually provide beyond that?

http://codereview.chromium.org/10829467/diff/9002/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10829467/diff/9002/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:725: return me2me_host.Run(&message_loop);
Why pass the raw MessageLoop in to Run(), rather than the
SingleThreadTaskRunner? Or, since HostProcess already has access to the
ChromotingHostContext, have it simply pull the SingleThreadTaskRunner from
there?

http://codereview.chromium.org/10829467/diff/9002/remoting/host/win/host_serv...
File remoting/host/win/host_service.cc (right):

http://codereview.chromium.org/10829467/diff/9002/remoting/host/win/host_serv...
remoting/host/win/host_service.cc:204: new AutoThread(kIoThreadName,
main_task_runner_));
Can you instead create a normal Thread in RunMessageLoop(), start it, wrap it in
an AutoMessageLoop and pass that to DaemonProcess::Create and
WtsSessionProcessLauncher?

The IO thread will then continue to run until it's no longer needed, and
RunMessageLoop() will block until it exits, and then delete it?

[Wez, 2012-08-24 21:31:19]

On 2012/08/24 21:04:43, alexeypa wrote:
> On 2012/08/24 20:16:29, Wez wrote:
> > Please update the CL description to clarify exactly what changes this CL
> makes,
> > e.g. which threads have been switched to this "automatic" lifetime
management.
> 
> Done.

It just switches the IO thread to auto-managed lifetime, right?

[alexeypa (please no reviews), 2012-08-24 21:46:29]

On 2012/08/24 21:31:19, Wez wrote:
> It just switches the IO thread to auto-managed lifetime, right?

No, it "switches threads, owned by remoting::HostService, remoting::HostProcess
and remoting::ChromotingHostContext, to refcount-based lifetime management."

I.e. the main thread of remoting_service and remoting_me2me_host plus a bunch of
I/O threads are managed this way now.

[Wez, 2012-08-24 22:23:44]

On 2012/08/24 21:46:29, alexeypa wrote:
> On 2012/08/24 21:31:19, Wez wrote:
> > It just switches the IO thread to auto-managed lifetime, right?
> 
> No, it "switches threads, owned by remoting::HostService,
remoting::HostProcess
> and remoting::ChromotingHostContext, to refcount-based lifetime management."
> 
> I.e. the main thread of remoting_service and remoting_me2me_host plus a bunch
of
> I/O threads are managed this way now.

OK; FWIW, having that kind of information summarized in the CL description can
be really helpful from the reviewer's perspective, since it gives them a hint of
where to start and what changes to expect!

[alexeypa (please no reviews), 2012-08-27 21:19:40]

PTAL.

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/base/auto_...
File remoting/base/auto_message_loop.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/base/auto_...
remoting/base/auto_message_loop.cc:36: // Deletes |this| on the message loop and
exists the loop.
On 2012/08/24 21:30:49, Wez wrote:
> typo: exits

Done.

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/base/auto_...
File remoting/base/auto_thread.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/base/auto_...
remoting/base/auto_thread.cc:52: thread_.Stop();
On 2012/08/24 21:30:49, Wez wrote:
> This arrangement means that once no-one needs the AutoThread any more the
parent
> will try to stop & join it, which may cause the parent to be blocked if the
> thread is processing a long-lived task at the time; I don't think that's what
we
> want here.

Yeah, it is not good. Taking |thread_| owneship from |AutoThread| turned our to
be a tricky business though. I ended up hiding logic responsible for stopping a
message loop behind a callback.

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/base/auto_...
File remoting/base/auto_thread.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/base/auto_...
remoting/base/auto_thread.h:25: scoped_refptr<base::SingleThreadTaskRunner>
parent);
On 2012/08/24 21:30:49, Wez wrote:
> I don't really see the point of this class; why can't you start a Thread, wrap
> it with an AutoMessageLoop to cause it to exit when no-one needs it any more,
> and then just join it in the caller when you absolutely positively have to? 
> What does this class actually provide beyond that?

MessageLoop and Thread should be stopped differently. That's why two separate
classes were needed.

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/host/remot...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:725: return me2me_host.Run(&message_loop);
On 2012/08/24 21:30:49, Wez wrote:
> Why pass the raw MessageLoop in to Run(), rather than the
> SingleThreadTaskRunner? Or, since HostProcess already has access to the
> ChromotingHostContext, have it simply pull the SingleThreadTaskRunner from
> there?

SingleThreadTaskRunner does not expose Run() method. It only allows to post
tasks to already running loop. It does not let to start one.

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/host/win/h...
File remoting/host/win/host_service.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/host/win/h...
remoting/host/win/host_service.cc:204: new AutoThread(kIoThreadName,
main_task_runner_));
On 2012/08/24 21:30:49, Wez wrote:
> Can you instead create a normal Thread in RunMessageLoop(), start it, wrap it
in
> an AutoMessageLoop and pass that to DaemonProcess::Create and
> WtsSessionProcessLauncher?
> 
> The IO thread will then continue to run until it's no longer needed, and
> RunMessageLoop() will block until it exits, and then delete it?

Done.

[Wez, 2012-08-28 17:34:53]

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/host/remot...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:725: return me2me_host.Run(&message_loop);
On 2012/08/27 21:19:40, alexeypa wrote:
> On 2012/08/24 21:30:49, Wez wrote:
> > Why pass the raw MessageLoop in to Run(), rather than the
> > SingleThreadTaskRunner? Or, since HostProcess already has access to the
> > ChromotingHostContext, have it simply pull the SingleThreadTaskRunner from
> > there?
> 
> SingleThreadTaskRunner does not expose Run() method. It only allows to post
> tasks to already running loop. It does not let to start one.

Right, but that's an artefact of the way HostProcess::Run() works; why not
create HostProcess, post the task, and run the MessageLoop here directly?

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
File remoting/base/auto_message_loop.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.cc:23:
CHECK(task_runner_->PostDelayedTask(from_here, task, delay));
Why CHECK rather than returning the result?

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.cc:31:
CHECK(task_runner_->PostNonNestableDelayedTask(from_here, task, delay));
Ditto.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
File remoting/base/auto_message_loop.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:20: // |AutoMessageLoop| is dropped.
nit: It doesn't actually do that any more, though; it invokes a caller-supplied
callback on a supplied TaskRunner, which may or may not quit it?

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:21: class AutoMessageLoop : public
base::SingleThreadTaskRunner {
nit: AutoMessageLoop doesn't seem the right name for this any more ->
AutoThreadTaskRunner?

But do we really need this class to be so general-purpose; as a result it
doesn't actually _do_ much!

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:24: // thread.
nit: Clarify what |stop_callback| is for / when it'll be called, either here, or
in the class description.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:25:
AutoMessageLoop(base::SingleThreadTaskRunner* parent,
nit: scoped_refptr<base::SingleThreadTaskRunner>, to make ref-counting explicit.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:25:
AutoMessageLoop(base::SingleThreadTaskRunner* parent,
Why do you need the |parent| reference? The purpose is to keep it alive but
that's something that objects which need it should be doing; in what sense does
the child thread need its parent to persist while it runs?

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
File remoting/host/chromoting_host_context.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
remoting/host/chromoting_host_context.cc:42: base::Closure());
If you're going to pass a null |stop_callback|, why wrap these in
AutoMessageLoops?

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
File remoting/host/chromoting_host_context.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
remoting/host/chromoting_host_context.h:82: // Task runners wrapping the above
threads. These should be declaed after
typo: declared

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
remoting/host/chromoting_host_context.h:83: // the corresponding threads to
guarantee proper order or destruction.
typo: of

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:139: disconnected_event_.Wait();
Since is blocked on this event, the plugin thread isn't available to process
tasks at this point, so you can just block waiting on the Threads in
|host_context_| exiting, rather than keep a separate event.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:914: desktop_environment_.reset();
Is this from a DesktopEnvironment->DesktopEnvironmentFactory CL?

[alexeypa (please no reviews), 2012-08-28 19:18:51]

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/host/remot...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/9002/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:725: return me2me_host.Run(&message_loop);
On 2012/08/28 17:34:53, Wez wrote:
> Right, but that's an artefact of the way HostProcess::Run() works; why not
> create HostProcess, post the task, and run the MessageLoop here directly?

Yes, indeed. Done.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
File remoting/base/auto_message_loop.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.cc:23:
CHECK(task_runner_->PostDelayedTask(from_here, task, delay));
On 2012/08/28 17:34:53, Wez wrote:
> Why CHECK rather than returning the result?

The whole purpose of this class is to make sure that |task_runner_| exists and
running as long as |this| exists. If it CHECKs here it means something is very
wrong about our assumption how shutdown works and it is better to fix it.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.cc:31:
CHECK(task_runner_->PostNonNestableDelayedTask(from_here, task, delay));
On 2012/08/28 17:34:53, Wez wrote:
> Ditto.

See above.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
File remoting/base/auto_message_loop.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:20: // |AutoMessageLoop| is dropped.
On 2012/08/28 17:34:53, Wez wrote:
> nit: It doesn't actually do that any more, though; it invokes a
caller-supplied
> callback on a supplied TaskRunner, which may or may not quit it?

It is technically true. It is hard to figure out the purpose of the class from
the above description. The purpose is the life time management. The fact the it
is implemented via a callback it just the unfortunate result of too many ways to
stop using a message loop.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:21: class AutoMessageLoop : public
base::SingleThreadTaskRunner {
On 2012/08/28 17:34:53, Wez wrote:
> nit: AutoMessageLoop doesn't seem the right name for this any more ->
> AutoThreadTaskRunner?

Sounds like a good name. AutoMessageLoop is shorter (while still long). Let talk
offline.

> But do we really need this class to be so general-purpose; as a result it
> doesn't actually _do_ much!

Well, it provides the only guarantee we want from it - keep the loop alive while
we need it (assuming the callback is doing what it should do).

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:24: // thread.
On 2012/08/28 17:34:53, Wez wrote:
> nit: Clarify what |stop_callback| is for / when it'll be called, either here,
or
> in the class description.

Done.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:25:
AutoMessageLoop(base::SingleThreadTaskRunner* parent,
On 2012/08/28 17:34:53, Wez wrote:
> nit: scoped_refptr<base::SingleThreadTaskRunner>, to make ref-counting
explicit.

Done.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/base/auto...
remoting/base/auto_message_loop.h:25:
AutoMessageLoop(base::SingleThreadTaskRunner* parent,
On 2012/08/28 17:34:53, Wez wrote:
> Why do you need the |parent| reference? The purpose is to keep it alive but
> that's something that objects which need it should be doing; in what sense
does
> the child thread need its parent to persist while it runs?

This is a way to express parent-child relations between message loops. For
instance if the callback posts a task to the parent loop in order to stop the
child one (like it was done in the 1st iteration of this CL).

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
File remoting/host/chromoting_host_context.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
remoting/host/chromoting_host_context.cc:42: base::Closure());
On 2012/08/28 17:34:53, Wez wrote:
> If you're going to pass a null |stop_callback|, why wrap these in
> AutoMessageLoops?

AutoMessageLoop will CHECK() if the message loop will go away unexpectedly.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
File remoting/host/chromoting_host_context.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
remoting/host/chromoting_host_context.h:82: // Task runners wrapping the above
threads. These should be declaed after
On 2012/08/28 17:34:53, Wez wrote:
> typo: declared

Done.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
remoting/host/chromoting_host_context.h:83: // the corresponding threads to
guarantee proper order or destruction.
On 2012/08/28 17:34:53, Wez wrote:
> typo: of

Done. But it sounds funnier with "or". :-)

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:139: disconnected_event_.Wait();
On 2012/08/28 17:34:53, Wez wrote:
> Since is blocked on this event, the plugin thread isn't available to process
> tasks at this point, so you can just block waiting on the Threads in
> |host_context_| exiting, rather than keep a separate event.

Waiting on the event does more than waiting for the threads to stop. It also
makes sure that all objects referencing the main loop where also destroyed. An
example is |desktop_environment_| that should be destroyed because it keeps
references to the main loop.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:914: desktop_environment_.reset();
On 2012/08/28 17:34:53, Wez wrote:
> Is this from a DesktopEnvironment->DesktopEnvironmentFactory CL?

No, it references the main message loop, so it has to be destroyed.

[alexeypa (please no reviews), 2012-08-29 14:54:32]

I renamed AutoMessageLoop to AutoThreadRunner and removed |parent_| link from
it.

I also tried to modify PluginThreadTaskRunner so that we can run tasks on the
plugin thread while shutting down. It turned out to be tricky enough, so it
deserves a separate CL.

[Wez, 2012-08-30 18:20:12]

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
File remoting/host/chromoting_host_context.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
remoting/host/chromoting_host_context.cc:42: base::Closure());
On 2012/08/28 19:18:51, alexeypa wrote:
> On 2012/08/28 17:34:53, Wez wrote:
> > If you're going to pass a null |stop_callback|, why wrap these in
> > AutoMessageLoops?
> 
> AutoMessageLoop will CHECK() if the message loop will go away unexpectedly.

Please add a one-line comment to explain that that's the intent here!

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:139: disconnected_event_.Wait();
On 2012/08/28 19:18:51, alexeypa wrote:
> On 2012/08/28 17:34:53, Wez wrote:
> > Since is blocked on this event, the plugin thread isn't available to process
> > tasks at this point, so you can just block waiting on the Threads in
> > |host_context_| exiting, rather than keep a separate event.
> 
> Waiting on the event does more than waiting for the threads to stop. It also
> makes sure that all objects referencing the main loop where also destroyed. An
> example is |desktop_environment_| that should be destroyed because it keeps
> references to the main loop.

As discussed offline, we don't currently have the capability to continue pumping
messages posted to the main/plugin thread's task runner once we've been asked to
shutdown, so waiting for objects that references the plugin TaskRunner to drop
those references isn't valid; instead we should either drop posted tasks or use
an AutoThreadTaskRunner just to CHECK for tasks being posted after we've stopped
processing them?

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
File remoting/base/auto_thread_task_runner.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:18: // This class provides automatic
life time management for a single thread task
nit: ... for a SingleThreadTaskRunner.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:20: // |AutoThreadTaskRunner| is
dropped.
Looking at the implementation, I can't see anything that actually posts a Stop()
task for the thread to the parent; presumably that's something the caller has to
arrange via a suitable |stop_callback|?

What is the purpose of AutoThreadTaskRunner with no |stop_callback| or |parent|?

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:31: // TODO(alexeypa): Remove the
|parent| reference and contructors taking it
typo: constructors

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:43: // called while any objects use it.
It is achieved by keeping a reference to
nit: It -> This

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:44: // the parent loop. Since
Thread::Stop() is called after exiting the parent
nit: ... parent loop. Since Thread ... -> ... parent thread's task runner, and
posting a task to Stop() this Thread when no more references to it remain.
|parent| should itself be an AutoThreadTaskRunner managed thread, to guarantee
that it remains alive long enough to perform the Stop().

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:47:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
nit: I think it makes more sense for |parent| to come after the other
parameters, so that the first TaskRunner passed to the ctor is always the
thread's |task_runner|, even if you're using a |parent| form.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:68: // |task_runner_| is running.
nit: ... is running. -> ... is running, so that a thread Stop() task can be
posted to it.

[Wez, 2012-08-30 18:25:40]

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:139: disconnected_event_.Wait();
On 2012/08/30 18:20:12, Wez wrote:
> On 2012/08/28 19:18:51, alexeypa wrote:
> > On 2012/08/28 17:34:53, Wez wrote:
> > > Since is blocked on this event, the plugin thread isn't available to
process
> > > tasks at this point, so you can just block waiting on the Threads in
> > > |host_context_| exiting, rather than keep a separate event.
> > 
> > Waiting on the event does more than waiting for the threads to stop. It also
> > makes sure that all objects referencing the main loop where also destroyed.
An
> > example is |desktop_environment_| that should be destroyed because it keeps
> > references to the main loop.
> 
> As discussed offline, we don't currently have the capability to continue
pumping
> messages posted to the main/plugin thread's task runner once we've been asked
to
> shutdown, so waiting for objects that references the plugin TaskRunner to drop
> those references isn't valid; instead we should either drop posted tasks or
use
> an AutoThreadTaskRunner just to CHECK for tasks being posted after we've
stopped
> processing them?

Actually the plugin TaskRunner won't even return false at this point, will it,
in which case wrapping it to get CHECKs won't do anything.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/host/chro...
File remoting/host/chromoting_host_context.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/host/chro...
remoting/host/chromoting_host_context.h:24:
ChromotingHostContext(base::SingleThreadTaskRunner* ui_task_runner);
This should remain scoped_refptr<> to implicitly document the object's lifetime.

[alexeypa (please no reviews), 2012-08-30 22:08:07]

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
File remoting/host/chromoting_host_context.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/chro...
remoting/host/chromoting_host_context.cc:42: base::Closure());
On 2012/08/30 18:20:12, Wez wrote:
> On 2012/08/28 19:18:51, alexeypa wrote:
> > On 2012/08/28 17:34:53, Wez wrote:
> > > If you're going to pass a null |stop_callback|, why wrap these in
> > > AutoMessageLoops?
> > 
> > AutoMessageLoop will CHECK() if the message loop will go away unexpectedly.
> 
> Please add a one-line comment to explain that that's the intent here!

Done.

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:139: disconnected_event_.Wait();
On 2012/08/30 18:20:12, Wez wrote:
> As discussed offline, we don't currently have the capability to continue
pumping
> messages posted to the main/plugin thread's task runner once we've been asked
to
> shutdown, so waiting for objects that references the plugin TaskRunner to drop
> those references isn't valid; instead we should either drop posted tasks or
use
> an AutoThreadTaskRunner just to CHECK for tasks being posted after we've
stopped
> processing them?

I added a comment before "plugin_task_runner_->Detach();".

https://chromiumcodereview.appspot.com/10829467/diff/12003/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:139: disconnected_event_.Wait();
On 2012/08/30 18:25:40, Wez wrote:
> Actually the plugin TaskRunner won't even return false at this point, will it,
> in which case wrapping it to get CHECKs won't do anything.

PluginThreadTaskRunner returns false during shutdown.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
File remoting/base/auto_thread_task_runner.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:18: // This class provides automatic
life time management for a single thread task
On 2012/08/30 18:20:12, Wez wrote:
> nit: ... for a SingleThreadTaskRunner.

Done.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:20: // |AutoThreadTaskRunner| is
dropped.
On 2012/08/30 18:20:12, Wez wrote:
> Looking at the implementation, I can't see anything that actually posts a
Stop()
> task for the thread to the parent; presumably that's something the caller has
to
> arrange via a suitable |stop_callback|?
> 
> What is the purpose of AutoThreadTaskRunner with no |stop_callback| or
|parent|?

This is documented in ChromotingHostContext.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:31: // TODO(alexeypa): Remove the
|parent| reference and contructors taking it
On 2012/08/30 18:20:12, Wez wrote:
> typo: constructors

I don't see any typo. I rephrased the comment just in case.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:43: // called while any objects use it.
It is achieved by keeping a reference to
On 2012/08/30 18:20:12, Wez wrote:
> nit: It -> This

Done.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:44: // the parent loop. Since
Thread::Stop() is called after exiting the parent
On 2012/08/30 18:20:12, Wez wrote:
> nit: ... parent loop. Since Thread ... -> ... parent thread's task runner, and

Done.

> posting a task to Stop() this Thread when no more references to it remain.

The previous paragraph explains that Thread will not allow us to post anything
to stop the thread.

> |parent| should itself be an AutoThreadTaskRunner managed thread, to guarantee
> that it remains alive long enough to perform the Stop().

Done.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:47:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2012/08/30 18:20:12, Wez wrote:
> nit: I think it makes more sense for |parent| to come after the other
> parameters, so that the first TaskRunner passed to the ctor is always the
> thread's |task_runner|, even if you're using a |parent| form.

Done.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:68: // |task_runner_| is running.
On 2012/08/30 18:20:12, Wez wrote:
> nit: ... is running. -> ... is running, so that a thread Stop() task can be
> posted to it.

Stop() cannot be posted to it.

[Wez, 2012-08-30 23:56:43]

Hopefully one last set of review comments, but otherwise looking good.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
File remoting/base/auto_thread_task_runner.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:20: // |AutoThreadTaskRunner| is
dropped.
On 2012/08/30 22:08:07, alexeypa wrote:
> On 2012/08/30 18:20:12, Wez wrote:
> > Looking at the implementation, I can't see anything that actually posts a
> Stop()
> > task for the thread to the parent; presumably that's something the caller
has
> to
> > arrange via a suitable |stop_callback|?
> > 
> > What is the purpose of AutoThreadTaskRunner with no |stop_callback| or
> |parent|?
> 
> This is documented in ChromotingHostContext.

I can see the comment regarding wrapping the MessageLoopProxys to get CHECKs but
nothing answering the questions above. In any case, explanations of how to use
this class should be in the class header, not in calling code.

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:31: // TODO(alexeypa): Remove the
|parent| reference and contructors taking it
On 2012/08/30 22:08:07, alexeypa wrote:
> On 2012/08/30 18:20:12, Wez wrote:
> > typo: constructors
> 
> I don't see any typo. I rephrased the comment just in case.

You'd just missed the 's' in constructors. The new wording reads better anyway,
IMHO. :)

https://chromiumcodereview.appspot.com/10829467/diff/28002/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:68: // |task_runner_| is running.
On 2012/08/30 22:08:07, alexeypa wrote:
> On 2012/08/30 18:20:12, Wez wrote:
> > nit: ... is running. -> ... is running, so that a thread Stop() task can be
> > posted to it.
> 
> Stop() cannot be posted to it.

Because of the plugin shutdown case... it's a PITA that we have to make this
class confusing to code around that.

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/base/auto...
File remoting/base/auto_thread_task_runner.h (right):

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:20: // to |AutoThreadTaskRunner| is
dropped.
This still isn't true, though; the task runner is _not_ stopped when the last
reference is dropped - it may be stopped by the calling code calling Stop() on
it if it's a Thread, or by the calling code providing a |stop_callback| that
stops it, but this class doesn't actually stop it.

Suggest rewording:
"This class supports automatic lifetime management for SingleThreadTaskRunners,
by invoking a caller-supplied callback when no more references remain, that can
stop the underlying thread or message loop, n. The caller may also provide a
reference to a parent AutoThreadTaskRunner to express dependencies between child
and parent thread lifetimes."

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:32: // stopping the thread's message
loop and joining the thread separately.
nit: Add a reference to crbug.com/145856 here?

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:39: // above.
nit: You only need the first sentence of this comment, followed immediately by
the explanation of the |parent| relationship below.

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:44: // exiting the parent loop, keeping
a reference to it gives enough guaratee.
typo: guarantee

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:46: // guarantee that it remains alive
long enough to perform the Stop().
Suggest rewording:
"To work around this we require that the parent thread of an
AutoThreadTaskRunner-wrapped Thread provide an AutoThreadTaskRunner so it can
tell when all its child Threads are finished with and Stop() them."

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/base/auto...
remoting/base/auto_thread_task_runner.h:48:
scoped_refptr<base::SingleThreadTaskRunner> parent);
Do we ever need to pass a non-AutoThreadTaskRunner |parent| for our current
workaround? If not then perhaps we should restrict |parent| to be an
AutoThreadTaskRunner.

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/host/chro...
File remoting/host/chromoting_host_context.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/host/chro...
remoting/host/chromoting_host_context.cc:40: // Wrap worker threads to
|AutoThreadTaskRunner| two keep the main thread
nit: "Wrap worker threads with |AutoThreadTaskRunner| and have them reference
the main thread via |ui_task_runner_|, to ensure that it remain active to Stop()
them when no references remain. This requires that |ui_task_runner_| wraps the
main thread via AutoThreadTaskRunner or equivalent."

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/host/plug...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/10829467/diff/43001/remoting/host/plug...
remoting/host/plugin/host_script_object.cc:138: host_context_->Stop();
nit: Add comment "Release the context's TaskRunner references for the threads,
so they can exit when no objects need them."

[Sergey Ulanov, 2012-08-31 01:25:18]

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
File remoting/base/auto_thread_task_runner.h (right):

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:21: class AutoThreadTaskRunner : public
base::SingleThreadTaskRunner {
can you please add some unittests for this class?

http://codereview.chromium.org/10829467/diff/43001/remoting/host/chromoting_h...
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/10829467/diff/43001/remoting/host/chromoting_h...
remoting/host/chromoting_host_context.h:35: void Stop();
Seems wrong to call this method Stop() given that it doesn't really stop the
threads. Maybe FreeThreads() or UnrefThreads()?

You essentially tie the lifetime of all threads to the lifetime of the main UI
thread: i.e. we really stop these threads only after the main thread message
loop quits. Why not stop them in the same manner we quit the main thread?

http://codereview.chromium.org/10829467/diff/43001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10829467/diff/43001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:715: base::Unretained(&message_loop));
Do you really need Unretained here given that QuitMessageLoop is not a method?

[alexeypa (please no reviews), 2012-08-31 19:57:35]

PTAL.

http://codereview.chromium.org/10829467/diff/28002/remoting/base/auto_thread_...
File remoting/base/auto_thread_task_runner.h (right):

http://codereview.chromium.org/10829467/diff/28002/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:20: // |AutoThreadTaskRunner| is
dropped.
On 2012/08/30 23:56:43, Wez wrote:
> On 2012/08/30 22:08:07, alexeypa wrote:
> > On 2012/08/30 18:20:12, Wez wrote:
> > > Looking at the implementation, I can't see anything that actually posts a
> > Stop()
> > > task for the thread to the parent; presumably that's something the caller
> has
> > to
> > > arrange via a suitable |stop_callback|?
> > > 
> > > What is the purpose of AutoThreadTaskRunner with no |stop_callback| or
> > |parent|?
> > 
> > This is documented in ChromotingHostContext.
> 
> I can see the comment regarding wrapping the MessageLoopProxys to get CHECKs
but
> nothing answering the questions above. In any case, explanations of how to use
> this class should be in the class header, not in calling code.

I adopted a modified version of the comment describing |AutoThreadTaskRunner|
that you proposed in a later comment.

http://codereview.chromium.org/10829467/diff/28002/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:31: // TODO(alexeypa): Remove the
|parent| reference and contructors taking it
On 2012/08/30 23:56:43, Wez wrote:
> You'd just missed the 's' in constructors.

I didn't. There are two constructors that take |parent|.

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
File remoting/base/auto_thread_task_runner.h (right):

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:20: // to |AutoThreadTaskRunner| is
dropped.
On 2012/08/30 23:56:43, Wez wrote:
> This still isn't true, though; the task runner is _not_ stopped when the last
> reference is dropped - it may be stopped by the calling code calling Stop() on
> it if it's a Thread, or by the calling code providing a |stop_callback| that
> stops it, but this class doesn't actually stop it.
> 
> Suggest rewording:
> "This class supports automatic lifetime management for
SingleThreadTaskRunners,
> by invoking a caller-supplied callback when no more references remain, that
can
> stop the underlying thread or message loop, n. The caller may also provide a
> reference to a parent AutoThreadTaskRunner to express dependencies between
child
> and parent thread lifetimes."

Done.

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:21: class AutoThreadTaskRunner : public
base::SingleThreadTaskRunner {
On 2012/08/31 01:25:18, sergeyu wrote:
> can you please add some unittests for this class?

Done.

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:32: // stopping the thread's message
loop and joining the thread separately.
On 2012/08/30 23:56:43, Wez wrote:
> nit: Add a reference to crbug.com/145856 here?

Done.

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:39: // above.
On 2012/08/30 23:56:43, Wez wrote:
> nit: You only need the first sentence of this comment, followed immediately by
> the explanation of the |parent| relationship below.

I think it deserves a verbose comment, given the number of questions asked about
this specific behavior. 

PS. It also explains why we cannot just call Thread::Stop from the callback.

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:44: // exiting the parent loop, keeping
a reference to it gives enough guaratee.
On 2012/08/30 23:56:43, Wez wrote:
> typo: guarantee

Done.

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:46: // guarantee that it remains alive
long enough to perform the Stop().
On 2012/08/30 23:56:43, Wez wrote:
> Suggest rewording:
> "To work around this we require that the parent thread of an
> AutoThreadTaskRunner-wrapped Thread provide an AutoThreadTaskRunner so it can
> tell when all its child Threads are finished with and Stop() them."

This sounds confusing to me. Check out my variant.

http://codereview.chromium.org/10829467/diff/43001/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:48:
scoped_refptr<base::SingleThreadTaskRunner> parent);
On 2012/08/30 23:56:43, Wez wrote:
> Do we ever need to pass a non-AutoThreadTaskRunner |parent| for our current
> workaround? If not then perhaps we should restrict |parent| to be an
> AutoThreadTaskRunner.

Done.

http://codereview.chromium.org/10829467/diff/43001/remoting/host/chromoting_h...
File remoting/host/chromoting_host_context.cc (right):

http://codereview.chromium.org/10829467/diff/43001/remoting/host/chromoting_h...
remoting/host/chromoting_host_context.cc:40: // Wrap worker threads to
|AutoThreadTaskRunner| two keep the main thread
On 2012/08/30 23:56:43, Wez wrote:
> nit: "Wrap worker threads with |AutoThreadTaskRunner| and have them reference
> the main thread via |ui_task_runner_|, to ensure that it remain active to
Stop()
> them when no references remain. This requires that |ui_task_runner_| wraps the
> main thread via AutoThreadTaskRunner or equivalent."

Done. I removed the last sentence because it is now apparent from the
|ui_task_runner_|'s type.

http://codereview.chromium.org/10829467/diff/43001/remoting/host/chromoting_h...
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/10829467/diff/43001/remoting/host/chromoting_h...
remoting/host/chromoting_host_context.h:35: void Stop();
On 2012/08/31 01:25:18, sergeyu wrote:
> Seems wrong to call this method Stop() given that it doesn't really stop the
> threads. Maybe FreeThreads() or UnrefThreads()?

Done. ReleaseTaskRunners()

> You essentially tie the lifetime of all threads to the lifetime of the main UI
> thread: i.e. we really stop these threads only after the main thread message
> loop quits. Why not stop them in the same manner we quit the main thread?

There is a lengthy comment in the declaration of AutoThreadTaskRunner and the
corresponding bug is http://crbug.com/145856. Basically the answer is that we
can't because the plugin main loop is stopped during shutdown and Thread::Stop()
has limitations that prevent us from stopping threads the way we would want them
to be stopped.

This CL is one step closer to the model that we will be able to stop the threads
earlier.

http://codereview.chromium.org/10829467/diff/43001/remoting/host/plugin/host_...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/10829467/diff/43001/remoting/host/plugin/host_...
remoting/host/plugin/host_script_object.cc:138: host_context_->Stop();
On 2012/08/30 23:56:43, Wez wrote:
> nit: Add comment "Release the context's TaskRunner references for the threads,
> so they can exit when no objects need them."

This is now apparent from the method's name.

http://codereview.chromium.org/10829467/diff/43001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10829467/diff/43001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:715: base::Unretained(&message_loop));
On 2012/08/31 01:25:18, sergeyu wrote:
> Do you really need Unretained here given that QuitMessageLoop is not a method?

Done.

[Wez, 2012-09-04 18:09:38]

LGTM w/ nits

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
File remoting/base/auto_thread_task_runner.h (right):

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:46: // since Thread::Stop() cannot be
called from an arbirtary thread. To work
typo: arbitrary

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:47: // this around we keep a reference
to the parent task runner
typo: around this

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:49: // the worker thread is in use.
Thread::Stop() to stop the worker thread is
nit: Reword "Thread::Stop() is called to stop the worker thread ..."

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
File remoting/base/auto_thread_task_runner_unittest.cc (right):

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner_unittest.cc:19:
scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
This method is actually used to keep the TaskRunner alive until the task is
processed, not to release it; if the intent is to mimic the case where there is
an outstanding task referring to the TaskRunner & keeping it alive, call this
DummyTask, say?

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner_unittest.cc:37: FROM_HERE,
base::Bind(&ReleaseTaskRunner, task_runner));
Do you actually need this? When you NULL |task_runner|, won't that post the quit
task, which |message_loop.Run()| will process and quit?

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner_unittest.cc:40: message_loop.Run();
If your test is successful it'll exit almost immediately; if not then it'll run
indefinitely; you could PostDelayedTask() with a delay of a few seconds and set
a failing expectation in there, for example.

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner_unittest.cc:59: FROM_HERE,
base::Bind(&ReleaseTaskRunner, task_runner));
See above.

[alexeypa (please no reviews), 2012-09-04 18:42:52]

FYI

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
File remoting/base/auto_thread_task_runner.h (right):

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:46: // since Thread::Stop() cannot be
called from an arbirtary thread. To work
On 2012/09/04 18:09:38, Wez wrote:
> typo: arbitrary

Done.

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:47: // this around we keep a reference
to the parent task runner
On 2012/09/04 18:09:38, Wez wrote:
> typo: around this

Done.

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner.h:49: // the worker thread is in use.
Thread::Stop() to stop the worker thread is
On 2012/09/04 18:09:38, Wez wrote:
> nit: Reword "Thread::Stop() is called to stop the worker thread ..."

Done.

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
File remoting/base/auto_thread_task_runner_unittest.cc (right):

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner_unittest.cc:19:
scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
On 2012/09/04 18:09:38, Wez wrote:
> This method is actually used to keep the TaskRunner alive until the task is
> processed, not to release it; if the intent is to mimic the case where there
is
> an outstanding task referring to the TaskRunner & keeping it alive, call this
> DummyTask, say?

Done.

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner_unittest.cc:37: FROM_HERE,
base::Bind(&ReleaseTaskRunner, task_runner));
On 2012/09/04 18:09:38, Wez wrote:
> Do you actually need this? When you NULL |task_runner|, won't that post the
quit
> task, which |message_loop.Run()| will process and quit?

Done.

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner_unittest.cc:40: message_loop.Run();
On 2012/09/04 18:09:38, Wez wrote:
> If your test is successful it'll exit almost immediately; if not then it'll
run
> indefinitely; you could PostDelayedTask() with a delay of a few seconds and
set
> a failing expectation in there, for example.

I added a test to make sure that the posted task has been executed.

http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
remoting/base/auto_thread_task_runner_unittest.cc:59: FROM_HERE,
base::Bind(&ReleaseTaskRunner, task_runner));
On 2012/09/04 18:09:38, Wez wrote:
> See above.

Done.

[Wez, 2012-09-04 18:46:54]

On 2012/09/04 18:42:52, alexeypa wrote:
> FYI
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> File remoting/base/auto_thread_task_runner.h (right):
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> remoting/base/auto_thread_task_runner.h:46: // since Thread::Stop() cannot be
> called from an arbirtary thread. To work
> On 2012/09/04 18:09:38, Wez wrote:
> > typo: arbitrary
> 
> Done.
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> remoting/base/auto_thread_task_runner.h:47: // this around we keep a reference
> to the parent task runner
> On 2012/09/04 18:09:38, Wez wrote:
> > typo: around this
> 
> Done.
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> remoting/base/auto_thread_task_runner.h:49: // the worker thread is in use.
> Thread::Stop() to stop the worker thread is
> On 2012/09/04 18:09:38, Wez wrote:
> > nit: Reword "Thread::Stop() is called to stop the worker thread ..."
> 
> Done.
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> File remoting/base/auto_thread_task_runner_unittest.cc (right):
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> remoting/base/auto_thread_task_runner_unittest.cc:19:
> scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
> On 2012/09/04 18:09:38, Wez wrote:
> > This method is actually used to keep the TaskRunner alive until the task is
> > processed, not to release it; if the intent is to mimic the case where there
> is
> > an outstanding task referring to the TaskRunner & keeping it alive, call
this
> > DummyTask, say?
> 
> Done.
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> remoting/base/auto_thread_task_runner_unittest.cc:37: FROM_HERE,
> base::Bind(&ReleaseTaskRunner, task_runner));
> On 2012/09/04 18:09:38, Wez wrote:
> > Do you actually need this? When you NULL |task_runner|, won't that post the
> quit
> > task, which |message_loop.Run()| will process and quit?
> 
> Done.
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> remoting/base/auto_thread_task_runner_unittest.cc:40: message_loop.Run();
> On 2012/09/04 18:09:38, Wez wrote:
> > If your test is successful it'll exit almost immediately; if not then it'll
> run
> > indefinitely; you could PostDelayedTask() with a delay of a few seconds and
> set
> > a failing expectation in there, for example.
> 
> I added a test to make sure that the posted task has been executed.
> 
>
http://codereview.chromium.org/10829467/diff/41003/remoting/base/auto_thread_...
> remoting/base/auto_thread_task_runner_unittest.cc:59: FROM_HERE,
> base::Bind(&ReleaseTaskRunner, task_runner));
> On 2012/09/04 18:09:38, Wez wrote:
> > See above.
> 
> Done.

Doesn't look like the final patchset is uploaded?

[alexeypa (please no reviews), 2012-09-04 18:49:18]

On 2012/09/04 18:46:54, Wez wrote:
> Doesn't look like the final patchset is uploaded?

Still merging.

[commit-bot: I haz the power, 2012-09-04 19:33:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10829467/47003

[commit-bot: I haz the power, 2012-09-04 22:27:45]

Change committed as 154827