Refactor the daemon controller so that the callbacks are called on the caller thread.

Refactor the daemon controller so that the callbacks are called on the caller thread.

This CL switches DaemonController to the same model that is used by PairingRegistry. DaemonController becomes a ref-counted object responsible for dispatching calls between threads. Platform-specific logic is implemented in non thread-safe delegate class. Most of the delegate's methods are synchronous and are running on a backgorund thread.

BUG=173509
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222162

[alexeypa (please no reviews), 2013-09-06 22:37:25]

PTAL.

PS. I'm sure you can do this one in an hour. :-)

[Sergey Ulanov, 2013-09-09 18:12:31]

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller.cc (right):

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller.cc:126: if
(!delegate_task_runner_->BelongsToCurrentThread()) {
Here and in other Do*() methods you can replace it with a DCHECK.

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller_delegate_linux.cc (right):

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller_delegate_linux.cc:47: void Dummy() {}
not used anywhere

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller_delegate_linux.h (right):

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller_delegate_linux.h:8: #include
"base/memory/ref_counted.h"
nit: not used in this file

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller_delegate_win.cc (right):

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller_delegate_win.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
This shows up as a new file, so it's hard to review it. Can you try reuploading
this change so that it's shown as a moved file. --similarity option for "git-cl
upload" might be useful.

[alexeypa (please no reviews), 2013-09-09 19:30:56]

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller.cc (right):

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller.cc:126: if
(!delegate_task_runner_->BelongsToCurrentThread()) {
On 2013/09/09 18:12:31, Sergey Ulanov wrote:
> Here and in other Do*() methods you can replace it with a DCHECK.

Only if DaemonController::ServiceNextRequest() posts a tasks instead of running
closure directly. Done.

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller_delegate_linux.cc (right):

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller_delegate_linux.cc:47: void Dummy() {}
On 2013/09/09 18:12:31, Sergey Ulanov wrote:
> not used anywhere

Done.

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller_delegate_linux.h (right):

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller_delegate_linux.h:8: #include
"base/memory/ref_counted.h"
On 2013/09/09 18:12:31, Sergey Ulanov wrote:
> nit: not used in this file

Done.

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller_delegate_win.cc (right):

https://codereview.chromium.org/23606019/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller_delegate_win.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2013/09/09 18:12:31, Sergey Ulanov wrote:
> This shows up as a new file, so it's hard to review it. Can you try
reuploading
> this change so that it's shown as a moved file. --similarity option for
"git-cl
> upload" might be useful. 

Done.

[Sergey Ulanov, 2013-09-09 20:19:50]

lgtm

https://codereview.chromium.org/23606019/diff/9001/remoting/host/setup/daemon...
File remoting/host/setup/daemon_controller_delegate_mac.mm (right):

https://codereview.chromium.org/23606019/diff/9001/remoting/host/setup/daemon...
remoting/host/setup/daemon_controller_delegate_mac.mm:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
I think this counts as renamed file, so you should keep 2012 here.

[alexeypa (please no reviews), 2013-09-09 20:39:59]

https://chromiumcodereview.appspot.com/23606019/diff/9001/remoting/host/setup...
File remoting/host/setup/daemon_controller_delegate_mac.mm (right):

https://chromiumcodereview.appspot.com/23606019/diff/9001/remoting/host/setup...
remoting/host/setup/daemon_controller_delegate_mac.mm:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/09/09 20:19:51, Sergey Ulanov wrote:
> I think this counts as renamed file, so you should keep 2012 here.

Done.

[commit-bot: I haz the power, 2013-09-09 20:40:44]

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

[commit-bot: I haz the power, 2013-09-09 20:50:11]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-09-09 21:28:37]

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

[commit-bot: I haz the power, 2013-09-09 21:33:52]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-09 21:54:45]

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

[commit-bot: I haz the power, 2013-09-10 00:54:10]

Change committed as 222162

[Wez, 2013-09-10 09:05:36]

Sorry, have only just started reviewing this; some initial comments below.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.cc:1560: if
(!plugin_task_runner_->BelongsToCurrentThread()) {
nit: This can be replaced by
DCHECK(plugin_task_runner_->BelongsToCurrentThread()), I think, since
PairingRegistry always calls back on the caller thread.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.h (right):

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.h:259: const ScopedRefNPObject&
callback,
nit: This, and any other methods that get bound to run off the plugin thread,
should be passed scoped_ptr<ScopedRefNPObject>&

(NPObject ref-counting isn't thread-safe)

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
File remoting/host/setup/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:88: struct UsageStatsConsent {
nit: UsageStatsConsentFlags?

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:97: // information whether the crash
dump reporting is controlled by policy.
Move the member descriptions to document the struct members, above.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:122: // finished or fails.
nit: Indicate which thread |done| is invoked upon.  (PairingRegistry should do
so, too, but doesn't, I notice...)

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:247: // task runner is used to guarantee
that one one method of the delegate is
typo: one one

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:248: // called at a time.
nit: Couldn't any SequenceTaskRunner provide the necessary ordering/exclusion
guarantee on method calls?

[alexeypa (please no reviews), 2013-09-10 16:59:22]

This CL has landed. I uploaded the fixes for you comments to
https://codereview.chromium.org/23578017. I haven't tested it much and I,
probably, will not have to to do this. Feel free to take over.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.cc:1560: if
(!plugin_task_runner_->BelongsToCurrentThread()) {
On 2013/09/10 09:05:37, Wez wrote:
> nit: This can be replaced by
> DCHECK(plugin_task_runner_->BelongsToCurrentThread()), I think, since
> PairingRegistry always calls back on the caller thread.

Done.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.h (right):

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.h:259: const ScopedRefNPObject&
callback,
On 2013/09/10 09:05:37, Wez wrote:
> nit: This, and any other methods that get bound to run off the plugin thread,
> should be passed scoped_ptr<ScopedRefNPObject>&
> 
> (NPObject ref-counting isn't thread-safe)

Done.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
File remoting/host/setup/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:88: struct UsageStatsConsent {
On 2013/09/10 09:05:37, Wez wrote:
> nit: UsageStatsConsentFlags?

It is long enough already.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:97: // information whether the crash
dump reporting is controlled by policy.
On 2013/09/10 09:05:37, Wez wrote:
> Move the member descriptions to document the struct members, above.

Done.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:122: // finished or fails.
On 2013/09/10 09:05:37, Wez wrote:
> nit: Indicate which thread |done| is invoked upon.  (PairingRegistry should do
> so, too, but doesn't, I notice...)

Done.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:247: // task runner is used to guarantee
that one one method of the delegate is
On 2013/09/10 09:05:37, Wez wrote:
> typo: one one

Done.

https://chromiumcodereview.appspot.com/23606019/diff/1/remoting/host/setup/da...
remoting/host/setup/daemon_controller.h:248: // called at a time.
On 2013/09/10 09:05:37, Wez wrote:
> nit: Couldn't any SequenceTaskRunner provide the necessary ordering/exclusion
> guarantee on method calls?

No. |delegate_thread_| can be used to invoke COM methods that requires
|delegate_thread_| to be a concrete UI thread, not a bunch of random threads.