Hangouts remote desktop part IV - Host installer

Hangouts remote desktop part IV - Host installer

This CL implements the HostInstaller, a class that enables the caller to download the host binary and monitor the install progress of the host by pinging the host periodically via native messaging.

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

[Jamie, 2014-08-13 02:50:39]

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
File remoting/webapp/host_installer.js (right):

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:40: * @type {number|null}
I think this is equivalent to ?number

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:55: return new Promise(function(resolve) {
Why resolve(bool) instead of resolve/reject? I think the latter would make for
clearer code because you wouldn't need as many conditionals later on.

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:82: * @throws {Error}  Throws when the there
is not matching host binary for the
s/when/if/
s/the//
s/not/no/

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:83: *     current platform.
@private

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:129: if
(that.downloadAndWaitForInstallPromise_ !== null) {
Why is it important to save this promise rather than returning a new one each
time? I can't quite get my head round what it means to call then() on a promise
more than once; what will happen?

[kelvinp, 2014-08-14 00:09:34]

PTAL

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
File remoting/webapp/host_installer.js (right):

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:40: * @type {number|null}
On 2014/08/13 02:50:39, Jamie wrote:
> I think this is equivalent to ?number

Done.

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:55: return new Promise(function(resolve) {
On 2014/08/13 02:50:39, Jamie wrote:
> Why resolve(bool) instead of resolve/reject? I think the latter would make for
> clearer code because you wouldn't need as many conditionals later on.

I think as a pattern, reject should be used for handling exception instead of
passing return value.

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:82: * @throws {Error}  Throws when the there
is not matching host binary for the
On 2014/08/13 02:50:39, Jamie wrote:
> s/when/if/
> s/the//
> s/not/no/

Done.

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:83: *     current platform.
On 2014/08/13 02:50:39, Jamie wrote:
> @private

Done.

https://codereview.chromium.org/466143003/diff/40001/remoting/webapp/host_ins...
remoting/webapp/host_installer.js:129: if
(that.downloadAndWaitForInstallPromise_ !== null) {
On 2014/08/13 02:50:38, Jamie wrote:
> Why is it important to save this promise rather than returning a new one each
> time? I can't quite get my head round what it means to call then() on a
promise
> more than once; what will happen?

The downloadAndWaitForInstallPromise_ is quite expensive (due to the underlying
setInterval call), so I want to make sure there is only one active setInterval
timer at any given moment.

Calling myPromise.then(onResolve) is like hooking an one off event.  The promise
engine will simply invoke |onResolve| when the |myPromise| fulfills.

  promise.then(function() {console.log('hello1')})
  promise.then(function() {console.log('hello2')})
when the promise fulfills
hello1
hello2

[Jamie, 2014-08-14 00:38:50]

lgtm

[kelvinp, 2014-08-14 04:47:24]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-08-14 04:51:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/466143003/60001

[commit-bot: I haz the power, 2014-08-14 10:56:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-14 14:16:05]

Committed patchset #2 (60001) as 289555