Improve apps v2 upgrade UX

remoting/webapp/crd/js/apps_v2_migration.js

+// Copyright 2014 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
+* The current v1 web-app allows users to sign in as any user. Some users may
+* be signed in using a different account than their chrome profile.  When these
+* users upgrade to the v2 app, their host list will be empty and it is not
+* obvious why.  remoting.AppsV2Migration shows a migration tip to the user to
+* sign in to their previous accounts if necessary.
+*/
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+
+(function() {
+
+// Storage key used for the migration settings.
+var MIGRATION_KEY_ = 'remoting-v2-migration';
+
+/** @constructor */
+remoting.MigrationSettings = function() {};
+remoting.MigrationSettings.prototype.email = /** @type {string}*/ ('');
+remoting.MigrationSettings.prototype.fullName = /** @type {string}*/ ('');
+remoting.MigrationSettings.prototype.hasHosts = /** @type {boolean}*/ (false);

[Jamie, 2015/01/20 22:32:01]

These should be member variables, not prototype variables; ie, assign them in
the ctor.

[Jamie, 2015/01/20 22:32:01]

Is it necessary to save hasHosts, or could we just make the existence of the
MigrationSettings object an implicit indicator that there were hosts?

[kelvinp, 2015/01/21 21:16:20]

Done.

[kelvinp, 2015/01/21 21:16:21]

Done.

+
+
+/**
+ * @return {Promise} A Promise object that would resolve to the
+ *   remoting.MigrationSettings
+ */
+function loadPreferences_() {
+  return new Promise(
+    /** @param {function(*) : void} resolve */
+    function(resolve) {
+      /** @param {Object<string, remoting.MigrationSettings>} result */
+      chrome.storage.local.get(MIGRATION_KEY_, function(result) {
+        resolve(result[MIGRATION_KEY_]);
+      });
+    }
+  );
+}
+
+/**
+ * @return {Promise} A Promise object that would resolve to the email of the

[Jamie, 2015/01/20 22:32:01]

s/email/email and full name/ (or change it so that it only returns the email if
that's all you need).

[kelvinp, 2015/01/21 21:16:20]

Done.

+ *   current user.
+ */
+function getEmail_() {
+  return new Promise(
+    /**
+      * @param {function(*) : void} resolve
+      * @param {function(*) : void} reject
+      */
+    function(resolve, reject) {
+      remoting.identity.getUserInfo(resolve, reject);
+    }
+  );
+}
+
+remoting.AppsV2Migration = function() {};
+
+/**
+ * @return {Promise} A Promise object that would resolve to
+ *   { email: string, fullName: string} if the user has signed in with a

[Jamie, 2015/01/20 22:32:01]

s/has signed in /has previously signed-in to the v1 app/

[Jamie, 2015/01/20 22:32:01]

Nit: No space after "{".

[kelvinp, 2015/01/21 21:16:20]

Done.

+ *   different account that has hosts registered to it.  Otherwise, the promise
+ *   will be rejected.
+ */
+remoting.AppsV2Migration.hasHostsInV1App = function() {
+  if (!base.isAppsV2()) {
+    return Promise.reject(false);
+  }
+
+  return Promise.all([loadPreferences_(), getEmail_()]).then(
+    /** @param {Object} results */
+    function(results){
+      var v1 = /**@type{remoting.MigrationSettings}*/ (results[0]);
+      var currentEmail = /** @type {string} */ (results[1]);
+
+      if (v1 && v1.email && v1.email !== currentEmail && v1.hasHosts) {
+        return {

[Jamie, 2015/01/20 22:32:01]

Is this equivalent to return Promise.resolve(...)? If so, then I think it would
be clearer.

[kelvinp, 2015/01/21 21:16:21]

Done.

+          email: v1.email,
+          fullName: v1.fullName
+        };
+      }
+      return Promise.reject(false);
+    });
+};
+
+/**
+ * @param {string} email
+ * @param {string} fullName
+ * @return {string}
+ */
+remoting.AppsV2Migration.buildMigrationTips = function(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);
+};
+
+/** @param {boolean} hasHosts */

[Jamie, 2015/01/20 22:32:01]

The behaviour of this function is quite subtle, so I think it's worth
enumerating the possibilities of (hasHost, isAppsV2); what it does, and why.

[kelvinp, 2015/01/21 21:16:20]

Done.

+remoting.AppsV2Migration.savePreferences = function(hasHosts) {
+  if (base.isAppsV2()) {
+    if (hasHosts) {
+      chrome.storage.local.remove(MIGRATION_KEY_);
+    }
+    return;
+  }
+
+  var preference = /**@type{remoting.MigrationSettings}*/ ({});

[Jamie, 2015/01/20 22:32:01]

This would be clearer if the MigrationSettings ctor took email, fullName and
hasHosts as parameters.

+  preference.email = window.localStorage['remoting-email'];
+  preference.fullName = window.localStorage['remoting-fullname'];

[Jamie, 2015/01/20 22:32:01]

Can these be passed in to the function? Using window.localStorage and
chrome.storage.local in the same function looks odd, and it doesn't seem right
that this function needs to know where they are stored. It would also make your
unit test simpler, I think.

If you don't want to pass them as function parameters, I think it would be
better to use the getUserInfo API to retrieve them.

[kelvinp, 2015/01/21 21:16:21]

Done.

+  preference.hasHosts = hasHosts;
+
+  var container = {};
+  container[MIGRATION_KEY_] = preference;
+  chrome.storage.local.set(container);
+};
+
+}());
+