Automatic host installation for IT2Me on Windows.

Automatic host installation for IT2Me on Windows.

With this change, the webapp will install the host component (if it is not installed) on Windows when the user tries to share an IT2Me session. Also HostInstallDialog (instead of HostSetupDialog) now displays the installation progress dialog so it can be shared by both IT2Me and Me2Me.

The code around HostDispatcher and HostIt2MeDispatcher is still a bit ugly but it should look better after the removal of the NPAPI dispatching logic, which comes next.

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

[Jamie, 2014-05-20 21:43:06]

LGTM, but please consider my comments about exposing the NPAPI interface via the
dispatcher.

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_dispatc...
File remoting/webapp/host_dispatcher.js (right):

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_dispatc...
remoting/webapp/host_dispatcher.js:92:
remoting.HostDispatcher.prototype.getNpapiHost = function() {
I don't like the host dispatcher exposing one of its underlying interfaces in
this way, but if it's going to be short-lived, I can live it for now :)

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_dispatc...
remoting/webapp/host_dispatcher.js:397: * This function installs the chromoting
host using the NPAPI plugin and should
Nit: We shouldn't refer to "chromoting" in our source. You can just use "host".

Although it doesn't look like the right comment anyway.

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_install...
File remoting/webapp/host_install_dialog.js (right):

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_install...
remoting/webapp/host_install_dialog.js:76: // their mind "Remote Deskto" (the
webapp) has already been installed.
Nit: Typo 'Deskto'.

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_it2me_d...
File remoting/webapp/host_it2me_dispatcher.js (right):

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_it2me_d...
remoting/webapp/host_it2me_dispatcher.js:60: that.npapiHost_ = null;
Is this needed because the dispatcher goes from NPAPI to NM after you install
the host components? If so, please add a comment explaining.

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_it2me_d...
remoting/webapp/host_it2me_dispatcher.js:105: return this.npapiHost_;
As before, if this is going to be long-lived, let's discuss; the dispatcher
should not be exposing the underlying interfaces.

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_screen.js
File remoting/webapp/host_screen.js (right):

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_screen....
remoting/webapp/host_screen.js:62: (/** @type {remoting.HostInstallDialog} */
hostInstallDialog).show(
I don't think you need this type-cast.

[weitao, 2014-05-20 22:09:13]

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_dispatc...
File remoting/webapp/host_dispatcher.js (right):

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_dispatc...
remoting/webapp/host_dispatcher.js:92:
remoting.HostDispatcher.prototype.getNpapiHost = function() {
I do plan to remove this in the subsequent CL's when I remove the dispatching
code.

On 2014/05/20 21:43:06, Jamie wrote:
> I don't like the host dispatcher exposing one of its underlying interfaces in
> this way, but if it's going to be short-lived, I can live it for now :)

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_dispatc...
remoting/webapp/host_dispatcher.js:397: * This function installs the chromoting
host using the NPAPI plugin and should
On 2014/05/20 21:43:06, Jamie wrote:
> Nit: We shouldn't refer to "chromoting" in our source. You can just use
"host".
> 
> Although it doesn't look like the right comment anyway.

Done.

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_install...
File remoting/webapp/host_install_dialog.js (right):

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_install...
remoting/webapp/host_install_dialog.js:76: // their mind "Remote Deskto" (the
webapp) has already been installed.
On 2014/05/20 21:43:06, Jamie wrote:
> Nit: Typo 'Deskto'.

Done.

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_it2me_d...
File remoting/webapp/host_it2me_dispatcher.js (right):

https://codereview.chromium.org/291133004/diff/1/remoting/webapp/host_it2me_d...
remoting/webapp/host_it2me_dispatcher.js:105: return this.npapiHost_;
I do plan to remove this in the subsequent CL's when I remove the dispatching
code.
On 2014/05/20 21:43:06, Jamie wrote:
> As before, if this is going to be long-lived, let's discuss; the dispatcher
> should not be exposing the underlying interfaces.

[weitao, 2014-05-20 22:09:35]

The CQ bit was checked by weitaosu@chromium.org

[commit-bot: I haz the power, 2014-05-20 22:11:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/weitaosu@chromium.org/291133004/20001

[Sergey Ulanov, 2014-05-21 03:21:58]

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_scr...
File remoting/webapp/host_screen.js (right):

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_scr...
remoting/webapp/host_screen.js:52: onInstallError(remoting.Error.CANCELLED);
Why doesn't HostInstallDialog call onInstallError() directly when installation
was cancelled? It knows that installation failed, so it doesn't look right that
it still calls onHostInstalled(). It also adds dependency on internals of the
HostInstallDialog implementation

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_scr...
remoting/webapp/host_screen.js:59: if (hostDispatcher.usingNpapi()) {
nit: This wouldn't be necessary if HostIt2MeDispatcher allowed successful
initialization with the plugin only when the plugin support It2Me and if
HostInstallDialog could instantiate NPAPI plugin directly when necessary (there
is no reason HostIt2MeDispatcher and HostInstallDialog have to use the same
instance of the plugin).

That would make this change a bit simpler, but I understand that this depends on
the direction chosen in the previous CLs, so it's not worth changing now, as we
won't need this logic for too long.

[commit-bot: I haz the power, 2014-05-21 07:27:14]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Sergey Ulanov, 2014-05-21 16:07:58]

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_ins...
File remoting/webapp/host_install_dialog.js (right):

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_ins...
remoting/webapp/host_install_dialog.js:79: hostPlugin.installHost(onDone);
This just starts installing the host silently. I past we discussed prompting the
user first, at least for It2Me. Is that still the plan?

[weitao, 2014-05-21 18:04:22]

Sergey, Thanks for your review. Many of the suggested fixes overlap with what I
plan to do in the next CL. What do you say I proceed to commit this CL and fix
them in the next CL?

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_ins...
File remoting/webapp/host_install_dialog.js (right):

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_ins...
remoting/webapp/host_install_dialog.js:79: hostPlugin.installHost(onDone);
You are right! I had forgot about this after redoing the IT2Me work (last and
this CL) and the two weeks absence. I will fix this.

On 2014/05/21 16:07:58, Sergey Ulanov wrote:
> This just starts installing the host silently. I past we discussed prompting
the
> user first, at least for It2Me. Is that still the plan?

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_scr...
File remoting/webapp/host_screen.js (right):

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_scr...
remoting/webapp/host_screen.js:52: onInstallError(remoting.Error.CANCELLED);
Good point. Will fix this.

On 2014/05/21 03:21:59, Sergey Ulanov wrote:
> Why doesn't HostInstallDialog call onInstallError() directly when installation
> was cancelled? It knows that installation failed, so it doesn't look right
that
> it still calls onHostInstalled(). It also adds dependency on internals of the
> HostInstallDialog implementation

https://codereview.chromium.org/291133004/diff/20001/remoting/webapp/host_scr...
remoting/webapp/host_screen.js:59: if (hostDispatcher.usingNpapi()) {
Yes this is planned in my next CL. :)

On 2014/05/21 03:21:59, Sergey Ulanov wrote:
> nit: This wouldn't be necessary if HostIt2MeDispatcher allowed successful
> initialization with the plugin only when the plugin support It2Me and if
> HostInstallDialog could instantiate NPAPI plugin directly when necessary
(there
> is no reason HostIt2MeDispatcher and HostInstallDialog have to use the same
> instance of the plugin).
> 
> That would make this change a bit simpler, but I understand that this depends
on
> the direction chosen in the previous CLs, so it's not worth changing now, as
we
> won't need this logic for too long.

[Sergey Ulanov, 2014-05-21 18:54:29]

On Wed, May 21, 2014 at 11:04 AM, <weitaosu@chromium.org> wrote:

> Sergey, Thanks for your review. Many of the suggested fixes overlap with
> what I
> plan to do in the next CL. What do you say I proceed to commit this CL and
> fix
> them in the next CL?


sgtm

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-05-21 20:04:54]

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

[commit-bot: I haz the power, 2014-05-21 20:04:54]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[weitao, 2014-05-22 16:46:48]

The CQ bit was checked by weitaosu@chromium.org

[commit-bot: I haz the power, 2014-05-22 16:48:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/weitaosu@chromium.org/291133004/20001

[commit-bot: I haz the power, 2014-05-22 20:08:07]

Change committed as 272294