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 258 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) {
+  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;
+    });
+};
+
+/**
  * 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;
+  }
+
   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);
 };
 
 /**
  * Key name under which Me2Me hosts are cached.
  */
 remoting.HostList.HOSTS_KEY = 'me2me-cached-hosts';
 
 /** @type {remoting.HostList} */
 remoting.hostList = null;