Fix ChromotingHost and DesktopEnvironmentFactory references to TaskRunners.

Fix ChromotingHost and DesktopEnvironmentFactory references to TaskRunners.

These classes previously referenced TaskRunners indirectly via a supplied
ChromotingHostContext, causing them to fail unexpectedly when trying to use the
TaskRunners after they had been detached from the ChromotingHostContext during
shutdown.  This CL has them take their own references to the TaskRunners they
need.

This CL also renames the "desktop" TaskRunner to "input" TaskRunner, reflecting
its current purpose.

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

[Wez, 2012-09-29 03:27:02]

PTAL; this is necessary before I can land the AutoThread CL.

[alexeypa (please no reviews), 2012-10-01 18:31:34]

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.h:11: #include "base/memory/scoped_ptr.h"
Add base/memory/ref_counted.h

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.h:67: // The caller must ensure that
|signal_strategy| and |environment| out-live
nit: environment -> desktop_environment_factory

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.h:67: // The caller must ensure that
|signal_strategy| and |environment| out-live
How about:

The caller must ensure that ... are alive until the host is completely stopped.

Then you will not need TODO, just to make sure that the pointers are cleared
before running |shutdown_task|.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host_context.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host_context.cc:22:
input_thread_("ChromotingInputThread"),
nit: I believe these names will be more readable if we used spaces: i.e.
"Chromoting input thread".

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host_context.h (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host_context.h:61: virtual
base::SingleThreadTaskRunner* input_task_runner();
nit: Make sure that this renaming is mentioned in the CL description.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host_unittest.cc:238: desktop_environment.Pass(),
nit: I wonder how it worked before?

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/desktop_...
File remoting/host/desktop_environment_factory.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/desktop_...
remoting/host/desktop_environment_factory.cc:18: :
input_task_runner_(input_task_runner), ui_task_runner_(ui_task_runner) {
nit: I believe it will look better if you put
input_task_runner_(input_task_runner) and ui_task_runner_(ui_task_runner) on two
separate lines.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/desktop_...
File remoting/host/desktop_environment_factory.h (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/desktop_...
remoting/host/desktop_environment_factory.h:34:
scoped_refptr<base::SingleThreadTaskRunner> input_task_runner_;
Now that you have these pointers here, you will need to make sure that
DesktopEnvironmentFactory is deleted as part of the host shutdown. There are at
least three different places: me2me host, simple host, and it2me host. I'm not
sure about unit tests.

Passing the task runners to Create() is less elegant but it helps to avoid this
problem, because the references will be released when a desktop environment
instance if deleted.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/remoting...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:131: desktop_environment_factory_(new
SessionDesktopEnvironmentFactory()),
SessionDesktopEnvironmentFactory is derived from DesktopEnvironmentFactory. You
will need to pass both task runners to SessionDesktopEnvironmentFactory as well.

[Wez, 2012-10-02 05:19:44]

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.h:11: #include "base/memory/scoped_ptr.h"
On 2012/10/01 18:31:34, alexeypa wrote:
> Add base/memory/ref_counted.h

Done.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.h:67: // The caller must ensure that
|signal_strategy| and |environment| out-live
On 2012/10/01 18:31:34, alexeypa wrote:
> nit: environment -> desktop_environment_factory

Done.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.h:67: // The caller must ensure that
|signal_strategy| and |environment| out-live
On 2012/10/01 18:31:34, alexeypa wrote:
> How about:
> 
> The caller must ensure that ... are alive until the host is completely
stopped.
> 
> Then you will not need TODO, just to make sure that the pointers are cleared
> before running |shutdown_task|.

Done.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host_context.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host_context.cc:22:
input_thread_("ChromotingInputThread"),
On 2012/10/01 18:31:34, alexeypa wrote:
> nit: I believe these names will be more readable if we used spaces: i.e.
> "Chromoting input thread".

Perhaps, but let's rename them in a separate CL.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host_context.h (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host_context.h:61: virtual
base::SingleThreadTaskRunner* input_task_runner();
On 2012/10/01 18:31:34, alexeypa wrote:
> nit: Make sure that this renaming is mentioned in the CL description.

Done.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host_unittest.cc:238: desktop_environment.Pass(),
On 2012/10/01 18:31:34, alexeypa wrote:
> nit: I wonder how it worked before?

Good question; I think we just don't inject any input, so the set_input_stub()
call is redundant.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/desktop_...
File remoting/host/desktop_environment_factory.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/desktop_...
remoting/host/desktop_environment_factory.cc:18: :
input_task_runner_(input_task_runner), ui_task_runner_(ui_task_runner) {
On 2012/10/01 18:31:34, alexeypa wrote:
> nit: I believe it will look better if you put
> input_task_runner_(input_task_runner) and ui_task_runner_(ui_task_runner) on
two
> separate lines.

Done.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/desktop_...
File remoting/host/desktop_environment_factory.h (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/desktop_...
remoting/host/desktop_environment_factory.h:34:
scoped_refptr<base::SingleThreadTaskRunner> input_task_runner_;
On 2012/10/01 18:31:34, alexeypa wrote:
> Now that you have these pointers here, you will need to make sure that
> DesktopEnvironmentFactory is deleted as part of the host shutdown. There are
at
> least three different places: me2me host, simple host, and it2me host. I'm not
> sure about unit tests.
> 
> Passing the task runners to Create() is less elegant but it helps to avoid
this
> problem, because the references will be released when a desktop environment
> instance if deleted.

Good point; I've updated things to destroy the DesktopEnvironmentFactory when
the shutdown task is invoked, for the most part.  ChromotingHost can actually
queue up multiple callbacks, though, which is something else I think we should
clean up.

The ordering constraint will go away once we move the threads into
DesktopEnvironmentFactory itself.

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/remoting...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:131: desktop_environment_factory_(new
SessionDesktopEnvironmentFactory()),
On 2012/10/01 18:31:34, alexeypa wrote:
> SessionDesktopEnvironmentFactory is derived from DesktopEnvironmentFactory.
You
> will need to pass both task runners to SessionDesktopEnvironmentFactory as
well.

Done.

[alexeypa (please no reviews), 2012-10-02 17:00:12]

lgtm

https://chromiumcodereview.appspot.com/11018004/diff/13001/remoting/host/chro...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/11018004/diff/13001/remoting/host/chro...
remoting/host/chromoting_host.h:70: // |shutdown_task| supplied to Shutdown()
has been notified.
nit: notified -> called/invoked/executed?

https://chromiumcodereview.appspot.com/11018004/diff/13001/remoting/host/remo...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/11018004/diff/13001/remoting/host/remo...
remoting/host/remoting_me2me_host.cc:131: desktop_environment_factory_(new
SessionDesktopEnvironmentFactory()),
SessiondesktopEnviromentFactory's constructor takes two parameters.

[commit-bot: I haz the power, 2012-10-02 18:21:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/11018004/19001

[commit-bot: I haz the power, 2012-10-02 21:26:43]

Change committed as 159771