MD History: add sign in promo in synced tabs when user isn't logged in

chrome/browser/resources/md_history/synced_device_manager.js

 // Copyright 2016 The Chromium Authors. All rights reserved.

[tsergeant, 2016/06/17 06:22:03]

I won't review this file until after calamity@ is done.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @typedef {{device: string,
  *           lastUpdateTime: string,
  *           separatorIndexes: !Array<number>,
  *           timestamp: number,
  *           tabs: !Array<!ForeignSessionTab>,
  *           tag: string}}
@@ skipping 17 matching lines @@
       observer: 'searchTermChanged'
     },
 
     /**
      * An array of synced devices with synced tab data.
      * @type {!Array<!ForeignDeviceInternal>}
      */
     syncedDevices_: {
       type: Array,
       value: function() { return []; }
+    },
+
+    signInState_: {
+      type: Boolean,
+      value: loadTimeData.getBoolean('isUserSignedIn'),
+    },
+
+    showNoSyncedMessage_: {
+      type: Boolean,
+      value: false,
     }
   },
 
   /**
    * @param {!ForeignSession} session
    * @return {!ForeignDeviceInternal}
    */
   createInternalDevice_: function(session) {
     var tabs = [];
     var separatorIndexes = [];
@@ skipping 25 matching lines @@
     return {
       device: session.name,
       lastUpdateTime: '– ' + session.modifiedTime,
       separatorIndexes: separatorIndexes,
       timestamp: session.timestamp,
       tabs: tabs,
       tag: session.tag,
     };
   },
 
+
+  onSignInTap_: function() {
+    chrome.send('SyncSetupShowSetupUI');
+    chrome.send('SyncSetupStartSignIn', [false]);
+  },
+
+  /**
+   * Change the message showing when user is logged in and there is no synced
+   * tabs. Also decide whether or not should display the message.
+   * @param {boolean} fetchingSyncedTabs
+   * @private
+   */
+  updateSyncedTabMessage_: function(fetchingSyncedTabs) {
+    var messageId = fetchingSyncedTabs ? 'loading' : 'noSyncedResults';
+    this.$['no-synced-tabs'].textContent = loadTimeData.getString(messageId);
+    this.showNoSyncedMessage_ = this.signInState_ &&
+                                (this.syncedDevices_.length == 0);

[calamity, 2016/06/17 06:25:45]

It would be 'more Polymer' to use a computed binding to change the text.

The two things you want to compute are 'whether the message should be displayed
at all' and 'what the message should be'.

So it should be:

noSyncedTabsMessage: function(signInState) {
  return loadTimeData.getString(signInState ? 'noSyncedResults' : 'loading');
}

showNoSyncedTabsMessage: function(signInState, syncedDevicesLength) {
  return signInState && syncedDevicesLength == 0;
}

then in the html,

...
hidden="[[!showNoSyncedTabsMessage(signInState_, syncedDevices_.length]]">
  [[noSyncedTabsMessage(signInState_)]]
</div>

[lshang, 2016/06/21 03:00:26]

Done.
It's really cool that computed binding will be recalculated whenever any of the
params changes.

+  },
+
   /**
    * Replaces the currently displayed synced tabs with |sessionList|. It is
    * common for only a single session within the list to have changed, We try to
    * avoid doing extra work in this case. The logic could be more intelligent
    * about updating individual tabs rather than replacing whole sessions, but
    * this approach seems to have acceptable performance.
    * @param {?Array<!ForeignSession>} sessionList
    */
   updateSyncedDevices: function(sessionList) {
-    if (!sessionList)
+    if (!sessionList) {
+      this.updateSyncedTabMessage_(false);
       return;
+    }
 
     // First, update any existing devices that have changed.
     var updateCount = Math.min(sessionList.length, this.syncedDevices_.length);
     for (var i = 0; i < updateCount; i++) {
       var oldDevice = this.syncedDevices_[i];
       if (oldDevice.tag != sessionList[i].tag ||
           oldDevice.timestamp != sessionList[i].timestamp) {
         this.splice(
             'syncedDevices_', i, 1, this.createInternalDevice_(sessionList[i]));
       }
     }
 
     // Then, append any new devices.
     for (var i = updateCount; i < sessionList.length; i++) {
       this.push('syncedDevices_', this.createInternalDevice_(sessionList[i]));
     }
+
+    this.updateSyncedTabMessage_(false);
+  },
+
+  updateSignInState: function(isUserSignedIn) {
+    // If user's sign in state didn't change, then don't change message or
+    // update synced devices.
+    if (this.signInState_ == isUserSignedIn)
+      return;

[calamity, 2016/06/17 06:25:45]

You can remove this if you do the above since notifications won't propagate
through when the value is the same.

[lshang, 2016/06/21 03:00:26]

When user changes state from signed out to signed in, then there is a short
period of time when it's fetching synced tabs and message 'Loading' is
displayed, then after the data returns, message will be 'no synced tabs'. We
want to make sure that 'Loading' only be displayed when user's state is changed
from signed out to signed in.

I did this because there are occasions when user has two Chrome windows with
different accounts A and B, A is logged in and B is not logged in. when user
also signs in account B, both A and B will receive updateSignInState
notifications from backend, but we would only want B to show the 'Loading'
message.

+    this.signInState_ = isUserSignedIn;
+
+    // User signed out, clear session list and show the sign in promo.
+    if (!isUserSignedIn) {
+      this.sessionList = [];
+      this.syncedDevices_ = [];

[calamity, 2016/06/17 06:25:45]

You should make a method called clearDisplayedSyncedDevices that just runs
this.syncedDevices = [].

It's getting confusing to remember why we need to clear both the session list
_and_ the syncedDevices array.

[lshang, 2016/06/21 03:00:26]

Done.
Yeah I think we just want to clear the UI here.

+      this.updateSyncedDevices(this.sessionList);

[calamity, 2016/06/17 06:25:45]

I don't think you'll need this line either.

[lshang, 2016/06/21 03:00:26]

Done.

+    } else {
+      // User signed in, show the loading message when querying for synced
+      // devices.
+      this.updateSyncedTabMessage_(true);
+    }

[calamity, 2016/06/17 06:25:45]

All of this is fairly nuanced, we should have a test in
history_synced_tabs_test.js that makes sure the right things are being hidden
and shown for different configurations of signin and sessions.

[lshang, 2016/06/21 03:00:26]

Done.

   },
 
   searchTermChanged: function(searchedTerm) {
     this.syncedDevices_ = [];
     this.updateSyncedDevices(this.sessionList);
   }
 });