Improve apps v2 upgrade UX

remoting/webapp/crd/js/host_list.js

Index: remoting/webapp/crd/js/host_list.js
diff --git a/remoting/webapp/crd/js/host_list.js b/remoting/webapp/crd/js/host_list.js
index d75224f7f1fdaacd459579699f39a3389af9160c..0ecb1e9fb1b5f4562f68f0f7f28cd92265f94a96 100644
--- a/remoting/webapp/crd/js/host_list.js
+++ b/remoting/webapp/crd/js/host_list.js
@@ -276,10 +276,115 @@ remoting.HostList.prototype.display = function() {
   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) {
+  this.showHostMigrationTips_().then(
+    /**
+     * @param {boolean} migrationTipsShown
+     * @this {remoting.HostList}
+     */
+    function(migrationTipsShown) {
+      if (migrationTipsShown) {
+        return;

[Jamie, 2015/01/14 03:55:29]

This doesn't seem right. migrationTipShown==true suggests to me that migration
tips are (already) shown, which seems to be backed up by the early return, but
that's not what the docs for showMigrationTips states. It seems the wrong way
round.

[kelvinp, 2015/01/15 01:10:14]

I have change the structure of the code now.  Let me know if the new structure
doesn't address your concerns.

+      }
+
+      var localize = l10n.getTranslationOrError;

[Jamie, 2015/01/14 03:55:29]

Please don't assign these shortened aliases if you can avoid it. It took me a
while to work out where localize was defined, and once you've added in the
annotation, having a shorter function name isn't going to save you a line-break
anyway,

[kelvinp, 2015/01/15 01:10:13]

Done.

+      var buttonLabel = localize('HOME_DAEMON_START_BUTTON');

[Jamie, 2015/01/14 03:55:29]

Please annotate these strings with /**i18n-content*/ in-keeping with other
strings in JS (look elsewhere for the exact syntax), otherwise the
verify_resources script won't know the strings are in use (and won't protect you
against typos).

[kelvinp, 2015/01/15 01:10:14]

Done.

+      this.noHosts_.innerText =
+          (hostingSupported) ?
+              localize('HOST_LIST_EMPTY_HOSTING_SUPPORTED', [buttonLabel]) :
+              localize('HOST_LIST_EMPTY_HOSTING_NOT_SUPPORTED', [buttonLabel]);
+
+    }.bind(this));
+};
+
+/**
+ * Checks whether the user has hosts registered to a different account and
+ * informs the user to sign-in with that account if necessary.
+ *
+ * @return {Promise} A Promise object that would resolve to true if the
+ *   migration tips is shown to the user.
+ * @private
+ */
+remoting.HostList.prototype.showHostMigrationTips_ = function() {
+  if (!base.isAppsV2()) {
+    return Promise.resolve(false);

[Jamie, 2015/01/14 03:55:29]

I don't have a strong opinion either way, but since this method is called
showHostMigrationTips (ie, the sort of thing one might call a Boolean variable),
might it be better to resolve or reject, rather than resolving with true or
false? In other words:

showHostMigrationTip().then(
  <yes>,
  <no>
);

[kelvinp, 2015/01/15 01:10:14]

I have renamed the function to WasSignedWithDifferentAcount 

+  }
+
+  var getCachedInfo = new Promise(
+    /** @param {function(*) : void} resolve */
+    function(resolve) {
+      chrome.storage.local.get(
+        ['remoting-email','remoting-fullname',remoting.HostList.HOSTS_KEY],
+        /** @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);
+    }
+  );
+
+  var noHosts = this.noHosts_;
+
+  return Promise.all([getCachedInfo, getCurrentEmail]).then(
+    /** @param {Object.<string>} results */
+    function(results){
+      var cached = /** @type {Object} */ results[0];
+      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');
+
+      /**
+       * @param {string} email
+       * @param {string} fullName
+       * @return {string}
+       */
+      function buildMigrationTips(email, fullName) {
+        var localize = l10n.getTranslationOrError;
+        var params = [
+          fullName,
+          email,
+          '<a href="https://support.google.com/chrome/answer/2364824?hl=en" ' +
+             'target="_blank">',
+          '</a>'];
+        return localize('HOST_LIST_EMPTY_V2_MIGRATION', params);
+      }
+
+      if (cachedEmail && cachedEmail !== currentEmail &&
+          cachedHosts && cachedHosts.length !== 0) {
+        noHosts.innerHTML = buildMigrationTips(cachedEmail, cachedName);

[Jamie, 2015/01/14 03:55:29]

I don't think this is the right way to do structure this. The naming of this
function suggests that it tells the caller whether or not to display the
migration tips, whereas what it actually does is show them itself. I prefer the
former, since the code in the then() function also sets this text and it would
be cleaner to have it collocated with this code.

[kelvinp, 2015/01/15 01:10:13]

Done.

+        return true;
+      }
+      return false;
+    });
 };
 
 /**
@@ -446,8 +551,15 @@ remoting.HostList.prototype.onErrorClick_ = function() {
  * Save the host list to local storage.
  */
 remoting.HostList.prototype.save_ = function() {
+  if (this.hosts_.length === 0) {
+    return;
+  }
+
   var items = {};
   items[remoting.HostList.HOSTS_KEY] = JSON.stringify(this.hosts_);
+  if (base.isAppsV2()) {
+    chrome.storage.local.remove('remoting-email');
+  }
   chrome.storage.local.set(items);
 };