Improve apps v2 upgrade UX

remoting/webapp/crd/js/host_list.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview
  * Class representing the host-list portion of the home screen UI.
  */
 
 'use strict';
@@ skipping 271 matching lines @@
   var canChangeLocalHostState =
       (state != remoting.HostController.State.NOT_IMPLEMENTED) &&
       (state != remoting.HostController.State.UNKNOWN) &&
       (state != remoting.HostController.State.NOT_INSTALLED ||
        remoting.isMe2MeInstallable()) &&
       (enabled || this.lastError_ == '');
 
   remoting.updateModalUi(enabled ? 'enabled' : 'disabled', 'data-daemon-state');
   var element = document.getElementById('daemon-control');
   element.hidden = !canChangeLocalHostState;
-  element = document.getElementById('host-list-empty-hosting-supported');
-  element.hidden = !canChangeLocalHostState;
-  element = document.getElementById('host-list-empty-hosting-unsupported');
-  element.hidden = canChangeLocalHostState;
+
+  if (noHostsRegistered) {
+    this.showEmptyText_(canChangeLocalHostState);
+  }
 };
 
 /**
+ * Displays a message to the user when the host list is empty.
+ *
+ * @param {boolean} hostingSupported
+ * @return {void}
+ * @private
+ */
+remoting.HostList.prototype.showEmptyText_ = function(hostingSupported) {

[Jamie, 2015/01/15 20:51:30]

"Empty text" suggests that the text is empty, not that it's what we show when
the host-list is empty. I suggest calling this showEmptyHostListMessage_.

[kelvinp, 2015/01/20 19:41:00]

Done.

+  /**
+   * @param {string} email
+   * @param {string} fullName
+   * @return {string}
+   */
+  function buildMigrationTips(email, fullName) {
+    var params = [
+      fullName,
+      email,
+      '<a href="https://support.google.com/chrome/answer/2364824?hl=en" ' +
+         'target="_blank">',
+      '</a>'];
+    return l10n.getTranslationOrError(
+        /*i18n-content*/'HOST_LIST_EMPTY_V2_MIGRATION', params);
+  }
+
+  var emptyTextDiv = this.noHosts_;

[Jamie, 2015/01/15 20:51:30]

I'm not sure what the value of this assignment is. If you think that the member
variable is mis-named (which I think is a good call), then it should be renamed
everywhere, not aliased here. I'd also suggest something more explicit if you're
planning on changing it, in keeping with the method name above; something like
emptyHostListMessageDiv_, perhaps?

[kelvinp, 2015/01/20 19:41:00]

This variable is necessary for the annonymous function in the then block to
access it on line 334.  I can rename it to var that = this to make the intention
clearer.

+
+  this.isSignedInWithDifferentIdentity_().then(
+    /**
+     * @param {{ email: string, fullName: string}} previousIdentity
+     * @this {remoting.HostList}
+     */
+    function(previousIdentity) {
+      var buttonLabel = l10n.getTranslationOrError(
+          /*i18n-content*/'HOME_DAEMON_START_BUTTON');
+
+      if (Boolean(previousIdentity)) {
+        emptyTextDiv.innerHTML = buildMigrationTips(previousIdentity.email,
+                                                    previousIdentity.fullName);
+      } else if (hostingSupported) {
+        emptyTextDiv.innerText = l10n.getTranslationOrError(
+            /*i18n-content*/'HOST_LIST_EMPTY_HOSTING_SUPPORTED',
+            [buttonLabel]);
+      } else {
+        emptyTextDiv.innerText = l10n.getTranslationOrError(
+            /*i18n-content*/'HOST_LIST_EMPTY_HOSTING_UNSUPPORTED',
+            [buttonLabel]);
+      }
+    }
+  );
+};
+
+/**
+ * @return {Promise} A Promise object that would resolve to
+ *   { email: string, fullName: string} if the user has signed in with a
+ *   different account that has hosts registered to it.  Otherwise, resolves to
+ *   null.
+ * @private
+ */
+remoting.HostList.prototype.isSignedInWithDifferentIdentity_ = function() {
+  if (!base.isAppsV2()) {
+    return Promise.resolve(null);

[Jamie, 2015/01/15 20:51:31]

I think my original objection still applies. For the function name you've
chosen, I would expect a Promise that resolves if the user is signed in with a
different identity and rejects if not. It would also make the then() function a
bit cleaner, because you wouldn't need to cast the previous information for
Boolean for the conditional, and you wouldn't need to translate
HOME_DAEMON_START_BUTTON for the "yes" case, where it's not needed.

[kelvinp, 2015/01/20 19:41:00]

Done.

+  }
+
+  var getCachedInfo = new Promise(

[Jamie, 2015/01/15 20:51:30]

s/Info/UserInfo/

You could also consider s/Cached/AppsV1/ since the fact that it's cached is an
implementation detail--what we're really interested in is the fact that it's the
previous (apps v1) identity.

[kelvinp, 2015/01/20 19:41:00]

Done.

+    /** @param {function(*) : void} resolve */
+    function(resolve) {
+      chrome.storage.local.get(
+        ['remoting-email','remoting-fullname',remoting.HostList.HOSTS_KEY],

[Jamie, 2015/01/15 20:51:30]

Nit: Spaces between list elements.

[Jamie, 2015/01/15 20:51:30]

These strings are duplicated. You should define them as constants, and I think
it would be better if they were more explicit about their purpose; perhaps
'remoting-appsv2-migration-email'. If you prefer, you could store them under a
single key:

remoting-appsv2-migration: { email: ..., fullname: ... }

[kelvinp, 2015/01/20 19:41:00]

Done.

+        /** @param {*} results */
+        function(results) { resolve(results); }
+      );
+    }
+  );
+
+  var getCurrentEmail = new Promise(
+    /**
+      * @param {function(*) : void} resolve
+      * @param {function(*) : void} reject
+      */
+    function(resolve, reject) {
+      /**
+       * @param {string} email
+       * @param {string} name
+       */
+      remoting.identity.getUserInfo(function(email, name) {
+        resolve(email);
+      }, reject);
+    }
+  );
+
+  return Promise.all([getCachedInfo, getCurrentEmail]).then(
+    /** @param {Object} results */
+    function(results){
+      var cached = /** @type {Object} */ (results[0]);

[Jamie, 2015/01/15 20:51:31]

s/cached/cachedUserInfo/

+      var currentEmail = /** @type {string} */ (results[1]);
+      var cachedHosts = /** @type {Array} */
+          (base.jsonParseSafe(cached[remoting.HostList.HOSTS_KEY]));
+      var cachedEmail = getStringAttr(cached, 'remoting-email', '');
+      var cachedName = getStringAttr(cached, 'remoting-fullname', '');
+
+      if (cachedEmail && cachedEmail !== currentEmail &&
+          Array.isArray(cachedHosts) && cachedHosts.length !== 0) {
+        return {
+          email: cachedEmail,
+          fullName: cachedName
+        };
+      }
+      return null;
+    });
+};
+
+/**
  * Remove a host from the list, and deregister it.
  * @param {remoting.HostTableEntry} hostTableEntry The host to be removed.
  * @return {void} Nothing.
  * @private
  */
 remoting.HostList.prototype.deleteHost_ = function(hostTableEntry) {
   this.table_.removeChild(hostTableEntry.tableRow);
   var index = this.hostTableEntries_.indexOf(hostTableEntry);
   if (index != -1) {
     this.hostTableEntries_.splice(index, 1);
@@ skipping 143 matching lines @@
     });
   } else {
     this.refresh(remoting.updateLocalHostState);
   }
 };
 
 /**
  * Save the host list to local storage.
  */
 remoting.HostList.prototype.save_ = function() {
+  if (this.hosts_.length === 0) {
+    return;
+  }

[Jamie, 2015/01/15 20:51:30]

I don't think this logic is correct. It will prevent the app from overwriting a
previous non-empty host-list with an empty one. I assume you're doing it to make
the "different account" message persistent, but I think that can be done less
invasively, for example by not loading the host-list until that check has
completed, or storing a separate Boolean to indicate whether or not there were
hosts that only the v1 app sets.

[kelvinp, 2015/01/20 19:41:00]

Done.

+
   var items = {};
   items[remoting.HostList.HOSTS_KEY] = JSON.stringify(this.hosts_);
+  if (base.isAppsV2()) {
+    chrome.storage.local.remove('remoting-email');

[Jamie, 2015/01/15 20:51:31]

Clearing this here seems a bit ad-hoc. I think you've added enough code to this
class, and with sufficiently self-contained functionality that it would be
cleaner to pull it out into a separate class. Off the top of my head (i.e, don't
feel bound by this), the API would be something like this:

remoting.AppsV2Migration = {
  userHasHosts: function() {
    // apps v1 would save the required state to support migration.
    // apps v2 would delete the state.
  },
  getEmptyHostListMessage: function() {
    // This would be pretty what you've already written.
  }
};

With a suitable storage mock, this would be very simple to write a unit-test for
completely isolated from any tests for host-list.

WDYT?

[kelvinp, 2015/01/20 19:41:00]

Done.

+  }
   chrome.storage.local.set(items);
 };
 
 /**
  * Key name under which Me2Me hosts are cached.
  */
 remoting.HostList.HOSTS_KEY = 'me2me-cached-hosts';
 
 /** @type {remoting.HostList} */
 remoting.hostList = null;