Download the host components when user tries to enable Me2Me host.

Download the host components when user tries to enable Me2Me host.

Now when NPAPI plugin is not present the host setup wizard detects
when the host is not installed and asks the user to install it.

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

[weitao, 2014-01-31 18:37:11]

Drive-by.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_con...
File remoting/webapp/host_controller.js (right):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_con...
remoting/webapp/host_controller.js:18: if (plugin.getDaemonState === undefined)
{
Remoting.js uses a different way of detecting the plugin:

plugin.hasOwnProperty('REQUESTED_ACCESS_CODE');

Which is the preferred way?

I recommend having an isNpapiPluginSupported() function in remoting.js that
unifies all these checks.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_nat...
File remoting/webapp/host_native_messaging.js (left):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_nat...
remoting/webapp/host_native_messaging.js:68: if (!chrome.runtime.connectNative)
{
I am about to commit a CL that remove this code and add a minimum required
version to our webapp. You will need to sync and merge the conflict.

[Jamie, 2014-01-31 18:39:14]

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
File remoting/webapp/host_dispatcher.js (right):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:268: onDone({});
Why is this not onError like the other cases? Maybe add a comment explaining the
discrepancy?

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:469:
onDone(remoting.HostController.State.NOT_INSTALLED);
This is the wrong type for onDone. I think you meant to run onError. Have you
run this through jscompiler? I believe you can do it automatically now by
setting the run_jscompile GYP variable.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:517:
onDone(remoting.HostController.State.NOT_INSTALLED);
onDone take no parameters; perhaps you need to call onError?

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:548:
onDone(remoting.HostController.State.NOT_INSTALLED);
onError?

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:574:
onDone(remoting.HostController.State.NOT_INSTALLED);
onError?

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:603:
onDone(remoting.HostController.State.NOT_INSTALLED);
onError?

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_nat...
File remoting/webapp/host_native_messaging.js (right):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_nat...
remoting/webapp/host_native_messaging.js:68: console.log("NM Init");
Are these logs needed? If so, they should be a bit more descriptive than this.

In fact, it's not clear what the changes to this file have to do with this CL.
Can you be done separately?

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_set...
File remoting/webapp/host_setup_dialog.js (right):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_set...
remoting/webapp/host_setup_dialog.js:358: * @returns {string} URL of the host
package for the current platform or an
This function doesn't return anything.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_set...
remoting/webapp/host_setup_dialog.js:390: window.location = hostPackageUrl;
This always assumes the Native Messaging case (ie, it will never attempt to
install using Omaha on Windows). That seems at odds with the HostDispatcher
class, which still supports both. It also doesn't appear to agree with the CL
description, which implies that the user should not be asked to install NM if
NPAPI is supported.

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

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/remoting...
remoting/webapp/remoting.js:156: remoting.supportLinuxDistibutionDetected_ =
false;
s/support/supported/

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/remoting...
remoting/webapp/remoting.js:165: !remoting.runningOnChromeOS());
Does ChromeOS report itself as Linux? If so, then there's no need to check for
it explicitly--it will presumably be excluded by the
supportedLinuxDistributionDetected check.

[Sergey Ulanov, 2014-01-31 23:34:17]

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_con...
File remoting/webapp/host_controller.js (right):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_con...
remoting/webapp/host_controller.js:18: if (plugin.getDaemonState === undefined)
{
On 2014/01/31 18:37:11, weitaosu wrote:
> Remoting.js uses a different way of detecting the plugin:
> 
> plugin.hasOwnProperty('REQUESTED_ACCESS_CODE');
> 
> Which is the preferred way?

It's essentially the same, except we are using different properties.
hasOwnProperty() is slightly better because it doesn't look at the object's
prototype.

> 
> I recommend having an isNpapiPluginSupported() function in remoting.js that
> unifies all these checks.

I don't think it's worth worrying about it much. Hopefully we can remove all
NPAPI-related code soon. I'm working another CL to integrate download in It2Me
flow - I'll need to refactor plugin loading for It2Me, so I'll make sure the
plugin loading code is shared..

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
File remoting/webapp/host_dispatcher.js (right):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:268: onDone({});
On 2014/01/31 18:39:15, Jamie wrote:
> Why is this not onError like the other cases? Maybe add a comment explaining
the
> discrepancy?

For most other things we can'd do anything without native components installed.
But we know that the host is not configured if it's not installed.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:469:
onDone(remoting.HostController.State.NOT_INSTALLED);
On 2014/01/31 18:39:15, Jamie wrote:
> This is the wrong type for onDone. I think you meant to run onError. Have you
> run this through jscompiler? I believe you can do it automatically now by
> setting the run_jscompile GYP variable.

Done

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:517:
onDone(remoting.HostController.State.NOT_INSTALLED);
On 2014/01/31 18:39:15, Jamie wrote:
> onDone take no parameters; perhaps you need to call onError?

Yes.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:548:
onDone(remoting.HostController.State.NOT_INSTALLED);
On 2014/01/31 18:39:15, Jamie wrote:
> onError?

Done.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:574:
onDone(remoting.HostController.State.NOT_INSTALLED);
On 2014/01/31 18:39:15, Jamie wrote:
> onError?

Done.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_dis...
remoting/webapp/host_dispatcher.js:603:
onDone(remoting.HostController.State.NOT_INSTALLED);
On 2014/01/31 18:39:15, Jamie wrote:
> onError?

Done.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_nat...
File remoting/webapp/host_native_messaging.js (left):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_nat...
remoting/webapp/host_native_messaging.js:68: if (!chrome.runtime.connectNative)
{
On 2014/01/31 18:37:11, weitaosu wrote:
> I am about to commit a CL that remove this code and add a minimum required
> version to our webapp. You will need to sync and merge the conflict.

Done.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_nat...
File remoting/webapp/host_native_messaging.js (right):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_nat...
remoting/webapp/host_native_messaging.js:68: console.log("NM Init");
On 2014/01/31 18:39:15, Jamie wrote:
> Are these logs needed? If so, they should be a bit more descriptive than this.
> 
> In fact, it's not clear what the changes to this file have to do with this CL.
> Can you be done separately?

Reverted the changes in this method, but the fix in onDisconnect_() needs to be
in this CL because now pendingReplies_ may be modified while onDisconnect_()
call one of the pendingReplies_.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_set...
File remoting/webapp/host_setup_dialog.js (right):

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_set...
remoting/webapp/host_setup_dialog.js:358: * @returns {string} URL of the host
package for the current platform or an
On 2014/01/31 18:39:15, Jamie wrote:
> This function doesn't return anything.

Done.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/host_set...
remoting/webapp/host_setup_dialog.js:390: window.location = hostPackageUrl;
On 2014/01/31 18:39:15, Jamie wrote:
> This always assumes the Native Messaging case (ie, it will never attempt to
> install using Omaha on Windows). That seems at odds with the HostDispatcher
> class, which still supports both. It also doesn't appear to agree with the CL
> description, which implies that the user should not be asked to install NM if
> NPAPI is supported.

Good point. I've updated showForStartWithToken_() to skip the installation step
on windows when using the plugin.

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

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/remoting...
remoting/webapp/remoting.js:156: remoting.supportLinuxDistibutionDetected_ =
false;
On 2014/01/31 18:39:15, Jamie wrote:
> s/support/supported/

Done.

https://codereview.chromium.org/132793007/diff/20001/remoting/webapp/remoting...
remoting/webapp/remoting.js:165: !remoting.runningOnChromeOS());
On 2014/01/31 18:39:15, Jamie wrote:
> Does ChromeOS report itself as Linux? If so, then there's no need to check for
> it explicitly--it will presumably be excluded by the
> supportedLinuxDistributionDetected check.

Yes, ChromeOS reports itself as Linux (blink uses uname() for
navigator.platform, except on mac and windows). I think it's still better to
have this check explicitly here, for the future when we remove
remoting.supportLinuxDistibutionDetected_.

[Jamie, 2014-02-01 01:18:16]

lgtm

[Sergey Ulanov, 2014-02-03 20:58:16]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-02-03 20:59:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/132793007/20002

[commit-bot: I haz the power, 2014-02-03 23:18:51]

Change committed as 248604

[commit-bot: I haz the power, 2014-02-03 23:18:53]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 23:18:55]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 23:18:55]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 23:19:02]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 23:19:13]

CQ bit was unchecked on CL. Ignoring.