Added HostListApl implementation to connect to legacy directory and GCD.

Added HostListApl implementation to connect to legacy directory and GCD.

The new implementation (CombinedHostListApi) attempts to keep the two
directories in sync and present a merged view of the contents of the
directories.

BUG=503790
Committed: https://crrev.com/43c0916a502dc87c79e59754b37e79a4fe00acea
Cr-Commit-Position: refs/heads/master@{#339737}

[John Williams, 2015-06-27 01:48:01]

jrw@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[John Williams, 2015-06-29 19:45:06]

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/lega...
File remoting/webapp/crd/js/legacy_host_list_api.js (right):

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/lega...
remoting/webapp/crd/js/legacy_host_list_api.js:29: newHostId, hostName,
publicKey, hostClientId);
I split the implementation of this method into a separate static method so it
can be called from CombinedHostListApi.

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/mock...
File remoting/webapp/crd/js/mock_host_list_api.js (left):

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/mock...
remoting/webapp/crd/js/mock_host_list_api.js:85: resolve,
remoting.Error.handler(reject), [that.hosts]);
Many of the changes in this file involve removing calls to
remoting.mockIdentity.validateTokenAndCall.  I made this change partly because
it seemed like the easiest way to make my own unit tests work, and partly
because calling a mock API this way, as part of another mock, seems like an
anti-pattern to me.  None of the existing unit tests were affected.

[kelvinp, 2015-06-29 23:53:44]

kelvinp@chromium.org changed reviewers:
+ kelvinp@chromium.org

[kelvinp, 2015-06-29 23:53:45]

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/base/js/plu...
File remoting/webapp/base/js/plugin_settings.js (right):

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/base/js/plu...
remoting/webapp/base/js/plugin_settings.js:70: /** @const {boolean} If true, use
GCD instead of Chromoting registry. */
It seems like after your change, we would always be running using the the
combined host list, can we remove USE_CCD?

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
File remoting/webapp/crd/js/combined_host_list_api.js (right):

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
remoting/webapp/crd/js/combined_host_list_api.js:76: 'to legacy directory: ' +
error);
Nit: alignment

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
remoting/webapp/crd/js/combined_host_list_api.js:149: return
legacyPromise.then(function() {
Does other matter?  Can we do Promise.all instead?

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
File remoting/webapp/crd/js/combined_host_list_api_unittest.js (right):

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
remoting/webapp/crd/js/combined_host_list_api_unittest.js:26:
QUnit.module('combined_host_list_api', {
Nit: s/combined_host_list_api/CombinedHostListAPI

[kelvinp, 2015-07-01 01:07:23]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-07-01 01:07:23]

The CQ bit was unchecked by kelvinp@chromium.org

[John Williams, 2015-07-17 23:29:32]

jrw@chromium.org changed reviewers:
- jamiewalch@chromium.org

[John Williams, 2015-07-17 23:29:33]

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/base/js/plu...
File remoting/webapp/base/js/plugin_settings.js (right):

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/base/js/plu...
remoting/webapp/base/js/plugin_settings.js:70: /** @const {boolean} If true, use
GCD instead of Chromoting registry. */
On 2015/06/29 23:53:45, kelvinp wrote:
> It seems like after your change, we would always be running using the the
> combined host list, can we remove USE_CCD?

I removed PREFER_GCD and kept USE_GCD as the only way to quickly roll back
GCD-related functionality.

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
File remoting/webapp/crd/js/combined_host_list_api.js (right):

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
remoting/webapp/crd/js/combined_host_list_api.js:76: 'to legacy directory: ' +
error);
On 2015/06/29 23:53:45, kelvinp wrote:
> Nit: alignment

Done.

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
remoting/webapp/crd/js/combined_host_list_api.js:149: return
legacyPromise.then(function() {
On 2015/06/29 23:53:45, kelvinp wrote:
> Does other matter?  Can we do Promise.all instead?

Order doesn't matter in this version, bit I wrote it that way because
Promise.all loses type information.

In the new version, order does matter somewhat, because GCD operations are given
a limited amount of time to complete after the corresponding legacy directory
operation completes.  I made that change as a defensive mechanism, so a problem
with GCD can't totally cripple Chromoting before GCD functionality is fully
rolled out.

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
File remoting/webapp/crd/js/combined_host_list_api_unittest.js (right):

https://codereview.chromium.org/1217643002/diff/1/remoting/webapp/crd/js/comb...
remoting/webapp/crd/js/combined_host_list_api_unittest.js:26:
QUnit.module('combined_host_list_api', {
On 2015/06/29 23:53:45, kelvinp wrote:
> Nit: s/combined_host_list_api/CombinedHostListAPI

Done.

[kelvinp, 2015-07-20 18:00:51]

https://codereview.chromium.org/1217643002/diff/80001/remoting/webapp/base/js...
File remoting/webapp/base/js/base.js (right):

https://codereview.chromium.org/1217643002/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/base.js:235: base.keySet = function(map) {
Why can't we call map.keys() directly?  Is this trying to make JSCompile happy?
Or is it because we want to return a copy of the keys instead?

https://codereview.chromium.org/1217643002/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/combined_host_list_api.js (right):

https://codereview.chromium.org/1217643002/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/combined_host_list_api.js:126: hosts.push(host);
Nit: 
After we ensure that either there is no legacy host or the GCD host is more
up-to-date, maybe we can remove the legacy host from the legacyHostMap
immediately and just add all of the legacyHosts from the map at the end?

[John Williams, 2015-07-20 21:43:10]

https://codereview.chromium.org/1217643002/diff/80001/remoting/webapp/base/js...
File remoting/webapp/base/js/base.js (right):

https://codereview.chromium.org/1217643002/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/base.js:235: base.keySet = function(map) {
On 2015/07/20 18:00:51, kelvinp wrote:
> Why can't we call map.keys() directly?  Is this trying to make JSCompile
happy?
> Or is it because we want to return a copy of the keys instead?

I need a Set (so I can test for membership), but Map.keys() returns an Iterator.
 All the typecasting is necessary because the externs files don't declare that
Iterator extends Iterable.

Anyway, this function is no longer needed in my latest patch.

https://codereview.chromium.org/1217643002/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/combined_host_list_api.js (right):

https://codereview.chromium.org/1217643002/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/combined_host_list_api.js:126: hosts.push(host);
On 2015/07/20 18:00:51, kelvinp wrote:
> Nit: 
> After we ensure that either there is no legacy host or the GCD host is more
> up-to-date, maybe we can remove the legacy host from the legacyHostMap
> immediately and just add all of the legacyHosts from the map at the end?

I rewrote this method for clarity; PTAL.

[kelvinp, 2015-07-20 22:46:18]

https://codereview.chromium.org/1217643002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/combined_host_list_api.js (right):

https://codereview.chromium.org/1217643002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/combined_host_list_api.js:116: var gcdHostMap = new
Map();
Would the following be simpler?

var hostMap = new Map();

// Initialize the map with gcd host.
gcdHosts.forEach(function(host) {
  hostMap.set(host.hostId, host);
});

// Dedupe it against the legacy hosts.
legacyHosts.forEach(function(legacyHost) {
 var gcdHost = hostMap.get(legacyHost.hostId);
 if (gcdHost) {
    // The host is in both maps.
    if (gcdHost.updatedTime >= legacyHost.updatedTime) {
      hostMap.set(legacyHost.hostId, gcdHost);      
    } else {
      hostMap.set(legacyHost.hostId, legacyHost);
    }
 }
});

var hosts = [];
hostMap.forEach(function(host) {
  hosts.push(host);
});

[John Williams, 2015-07-21 04:54:55]

https://codereview.chromium.org/1217643002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/combined_host_list_api.js (right):

https://codereview.chromium.org/1217643002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/combined_host_list_api.js:116: var gcdHostMap = new
Map();
On 2015/07/20 22:46:18, kelvinp wrote:
> Would the following be simpler?
> 
> var hostMap = new Map();
> 
> // Initialize the map with gcd host.
> gcdHosts.forEach(function(host) {
>   hostMap.set(host.hostId, host);
> });
> 
> // Dedupe it against the legacy hosts.
> legacyHosts.forEach(function(legacyHost) {
>  var gcdHost = hostMap.get(legacyHost.hostId);
>  if (gcdHost) {
>     // The host is in both maps.
>     if (gcdHost.updatedTime >= legacyHost.updatedTime) {
>       hostMap.set(legacyHost.hostId, gcdHost);      
>     } else {
>       hostMap.set(legacyHost.hostId, legacyHost);
>     }
>  }
> });
> 
> var hosts = [];
> hostMap.forEach(function(host) {
>   hosts.push(host);
> });

That's not quite right, but I incorporated something similar.

[kelvinp, 2015-07-21 06:36:36]

Thank you for being flexible.  lgtm

https://codereview.chromium.org/1217643002/diff/120001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/combined_host_list_api.js (right):

https://codereview.chromium.org/1217643002/diff/120001/remoting/webapp/crd/js...
remoting/webapp/crd/js/combined_host_list_api.js:145: // TODO(jrw): Use
Array.from once it becomes available.
haha, I come across the same thing when as well :)

[John Williams, 2015-07-21 19:04:11]

On 2015/07/21 06:36:36, kelvinp wrote:
> Thank you for being flexible.  lgtm
> 
>
https://codereview.chromium.org/1217643002/diff/120001/remoting/webapp/crd/js...
> File remoting/webapp/crd/js/combined_host_list_api.js (right):
> 
>
https://codereview.chromium.org/1217643002/diff/120001/remoting/webapp/crd/js...
> remoting/webapp/crd/js/combined_host_list_api.js:145: // TODO(jrw): Use
> Array.from once it becomes available.
> haha, I come across the same thing when as well :)

There's a crbug issue for it that's marked fixed, so maybe it'll be in the next
beta.

[John Williams, 2015-07-21 19:29:51]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-07-21 19:30:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217643002/120001

[commit-bot: I haz the power, 2015-07-21 20:28:20]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-21 20:30:15]

Patchset 7 (id:??) landed as
https://crrev.com/43c0916a502dc87c79e59754b37e79a4fe00acea
Cr-Commit-Position: refs/heads/master@{#339737}