Implemented on-demand installation of the Chromoting Host on Windows.

Implemented on-demand installation of the Chromoting Host on Windows.

This CL also updates the Google Update interface definition file with v3 interfaces.

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

[alexeypa (please no reviews), 2012-04-06 23:12:38]

Please take a look.

grt -> google_update/*
sergeyu -> remoting/*

[Wez, 2012-04-06 23:18:43]

Would it make more sense to split these into two CLs?

[alexeypa (please no reviews), 2012-04-06 23:20:59]

On 2012/04/06 23:18:43, Wez wrote:
> Would it make more sense to split these into two CLs?

I'm not sure. It will slow down checking it in and reviewers of google_update
changes will have less context on why it is needed.

[alexeypa (please no reviews), 2012-04-07 02:25:19]

+pkasting for chrome/browser/google/*

[Peter Kasting, 2012-04-07 05:57:00]

LGTM

[Sergey Ulanov, 2012-04-07 18:09:21]

http://codereview.chromium.org/10021003/diff/1010/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_installer_win.cc (right):

http://codereview.chromium.org/10021003/diff/1010/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:24: };
don't need semicolon here. add // namespace omaha.

http://codereview.chromium.org/10021003/diff/1010/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:120: bundle_.ReceiveVoid());
nit: this parameter can probably fit on the previous line

http://codereview.chromium.org/10021003/diff/1010/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:326: bind_options.dwClassContext  =
CLSCTX_LOCAL_SERVER;
nit: remove extra space

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:109: CompletionCallback
done_callback_;
Since this is used only during installation maybe call it
install_done_callback_?

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:342: void
DaemonControllerWin::DoInstall(scoped_ptr<base::DictionaryValue> config,
Since this method installs the host only if it isn't installed and then starts
the host, then maybe call it EnsureInstalledAndStart()

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:361: config_ = config.Pass();
Do we really need to store these values as class members? I think we should be
able to pass them directly to OnInstallationComplete().
base::Bind supports currying (http://en.wikipedia.org/wiki/Currying). You can
define OnInstallationComplete as this:
 void OnInstallationComplete(scoped_ptr<DictionaryValue> config,
                             const CompletionCallback& callback,
                             HRESULT hr);
And then when you Bind() it you specify the first two parameters, but not the
last one - that will produce Callback<void(HRESULT)>.

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:369: base::Unretained(this)));
nit: not indented properly

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_installer_win.cc (right):

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:86: public:
nit: indent with one space.

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:134: ScopedBstr
empty(ASCIIToUTF16(kOmahaEmpty).c_str());
Do we need to convert and empty string? Can this be empty(L"")?

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:249:
MessageLoop::current()->PostDelayedTask(
use base::RepeatingTimer instead of posting repeated tasks

http://codereview.chromium.org/10021003/diff/6005/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:252: base::Unretained(this), done),
not sure if base::Unretained() is safe here. This can be fixed by using
RepeatingTimer

[grt (UTC plus 2), 2012-04-08 14:57:04]

google_update changes LGTM.  please send a tryjob to the "win" bot, as it builds
the whole world and will suss out compile breaks.

https://chromiumcodereview.appspot.com/10021003/diff/6005/google_update/READM...
File google_update/README.chromium (right):

https://chromiumcodereview.appspot.com/10021003/diff/6005/google_update/READM...
google_update/README.chromium:7: with the following two coclasses removed:
why?

[alexeypa (please no reviews), 2012-04-09 20:24:11]

Addressed CR feedback. PTAL.

https://chromiumcodereview.appspot.com/10021003/diff/1010/remoting/host/plugi...
File remoting/host/plugin/daemon_installer_win.cc (right):

https://chromiumcodereview.appspot.com/10021003/diff/1010/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.cc:24: };
On 2012/04/07 18:09:21, sergeyu wrote:
> don't need semicolon here. add // namespace omaha.

Done.

https://chromiumcodereview.appspot.com/10021003/diff/1010/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.cc:120: bundle_.ReceiveVoid());
On 2012/04/07 18:09:21, sergeyu wrote:
> nit: this parameter can probably fit on the previous line

Done.

https://chromiumcodereview.appspot.com/10021003/diff/1010/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.cc:326: bind_options.dwClassContext  =
CLSCTX_LOCAL_SERVER;
On 2012/04/07 18:09:21, sergeyu wrote:
> nit: remove extra space

Done.

https://chromiumcodereview.appspot.com/10021003/diff/6005/google_update/READM...
File google_update/README.chromium (right):

https://chromiumcodereview.appspot.com/10021003/diff/6005/google_update/READM...
google_update/README.chromium:7: with the following two coclasses removed:
On 2012/04/08 14:57:05, grt wrote:
> why?

Because their GUIDs are auto-generated, meaning that they are internal to Omaha.

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:109: CompletionCallback
done_callback_;
On 2012/04/07 18:09:21, sergeyu wrote:
> Since this is used only during installation maybe call it
> install_done_callback_?

It was removed from the class.

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:342: void
DaemonControllerWin::DoInstall(scoped_ptr<base::DictionaryValue> config,
On 2012/04/07 18:09:21, sergeyu wrote:
> Since this method installs the host only if it isn't installed and then starts
> the host, then maybe call it EnsureInstalledAndStart()

Renamed to DoInstallAsNeededAndStart to preserve "Do" as a way of marking all
worker methods.

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:361: config_ = config.Pass();
On 2012/04/07 18:09:21, sergeyu wrote:
> Do we really need to store these values as class members? I think we should be
> able to pass them directly to OnInstallationComplete().
> base::Bind supports currying (http://en.wikipedia.org/wiki/Currying). You can
> define OnInstallationComplete as this:
>  void OnInstallationComplete(scoped_ptr<DictionaryValue> config,
>                              const CompletionCallback& callback,
>                              HRESULT hr);
> And then when you Bind() it you specify the first two parameters, but not the
> last one - that will produce Callback<void(HRESULT)>.

Done.

I kept installer_ as a class member (instead of passing it to the callback)
though to make sure that the installer will be safely deleted when
DaemonController is destroyed prematurely.

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:369: base::Unretained(this)));
On 2012/04/07 18:09:21, sergeyu wrote:
> nit: not indented properly

Done.

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
File remoting/host/plugin/daemon_installer_win.cc (right):

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.cc:86: public:
On 2012/04/07 18:09:21, sergeyu wrote:
> nit: indent with one space.

Done.

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.cc:134: ScopedBstr
empty(ASCIIToUTF16(kOmahaEmpty).c_str());
On 2012/04/07 18:09:21, sergeyu wrote:
> Do we need to convert and empty string? Can this be empty(L"")?

This was needed because of the presubmit check frowning at wchar_t and L"". I
found a way around it using char16 and TO_L_STRING macro.

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.cc:249:
MessageLoop::current()->PostDelayedTask(
On 2012/04/07 18:09:21, sergeyu wrote:
> use base::RepeatingTimer instead of posting repeated tasks

Done.

https://chromiumcodereview.appspot.com/10021003/diff/6005/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.cc:252: base::Unretained(this), done),
On 2012/04/07 18:09:21, sergeyu wrote:
> not sure if base::Unretained() is safe here. This can be fixed by using
> RepeatingTimer

Done.

[Sergey Ulanov, 2012-04-09 22:32:57]

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:112: base::Lock lock_;
Don't think you need this lock anymore - |installer_| is accessed only on
worker_thread_. But please add DCHECKS to verify it.

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:270: if (SUCCEEDED(result)) {
add DCHECK(worker_thread.message_loop_proxy()->BelongsToCurrentThread()) here.

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:346: IDaemonControl* control =
NULL;
DCHECK(worker_thread.message_loop_proxy()->BelongsToCurrentThread()) and in
other methods that are supposed to run on the worker thread.

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_installer_win.cc (right):

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:25: } // namespace omaha
nit: two spaces before the comment

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:84:
base::DelayTimer<DaemonComInstallerWin> polling_timer_;
Why DelayTimer instead of RepeatingTimer?
Or you can use OneShotTimer and restart it every time in
PollInstallationStatus().

[alexeypa (please no reviews), 2012-04-10 00:20:05]

FYI

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:112: base::Lock lock_;
On 2012/04/09 22:32:58, sergeyu wrote:
> Don't think you need this lock anymore - |installer_| is accessed only on
> worker_thread_. But please add DCHECKS to verify it.

Done.

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:270: if (SUCCEEDED(result)) {
On 2012/04/09 22:32:58, sergeyu wrote:
> add DCHECK(worker_thread.message_loop_proxy()->BelongsToCurrentThread()) here.

Done.

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_controller_win.cc:346: IDaemonControl* control =
NULL;
On 2012/04/09 22:32:58, sergeyu wrote:
> DCHECK(worker_thread.message_loop_proxy()->BelongsToCurrentThread()) and in
> other methods that are supposed to run on the worker thread.

Done.

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_installer_win.cc (right):

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:25: } // namespace omaha
On 2012/04/09 22:32:58, sergeyu wrote:
> nit: two spaces before the comment

Done.

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:84:
base::DelayTimer<DaemonComInstallerWin> polling_timer_;
On 2012/04/09 22:32:58, sergeyu wrote:
> Why DelayTimer instead of RepeatingTimer?
> Or you can use OneShotTimer and restart it every time in
> PollInstallationStatus().

RepeatingTimer has to be stopped explicitly, so I'll have to add eigth
polling_timer_.Stop() calls to PollInstallationStatus.

OneShotTimer requires passing all timer parameters (delay, method pointer, etc)
every time it is started.

[Sergey Ulanov, 2012-04-10 01:17:14]

LGTM, but please consider replacing base::DelayTimer with base::Timer

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
File remoting/host/plugin/daemon_installer_win.cc (right):

http://codereview.chromium.org/10021003/diff/9001/remoting/host/plugin/daemon...
remoting/host/plugin/daemon_installer_win.cc:84:
base::DelayTimer<DaemonComInstallerWin> polling_timer_;
On 2012/04/10 00:20:06, alexeypa wrote:
> On 2012/04/09 22:32:58, sergeyu wrote:
> > Why DelayTimer instead of RepeatingTimer?
> > Or you can use OneShotTimer and restart it every time in
> > PollInstallationStatus().
> 
> RepeatingTimer has to be stopped explicitly, so I'll have to add eigth
> polling_timer_.Stop() calls to PollInstallationStatus.
> 
> OneShotTimer requires passing all timer parameters (delay, method pointer,
etc)
> every time it is started.
> 

Hm, then maybe use base::Timer? It's not a template, so the code would be
cleaner than with DelayTimer. 
Problem with DelayTimer is that this is not really the use case it is designed
for.

[commit-bot: I haz the power, 2012-04-10 16:42:21]

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

[commit-bot: I haz the power, 2012-04-10 18:12:36]

Change committed as 131587