Add a sign-in promo message to the Other Devices menu.

chrome/browser/resources/ntp4/other_sessions.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 The menu that shows tabs from sessions on other devices.
  */
 
 cr.define('ntp', function() {
   'use strict';
@@ skipping 18 matching lines @@
 
     decorate: function() {
       MenuButton.prototype.decorate.call(this);
       this.menu = new Menu;
       cr.ui.decorate(this.menu, Menu);
       this.menu.classList.add('footer-menu');
       this.menu.addEventListener('contextmenu',
                                  this.onContextMenu_.bind(this), true);
       document.body.appendChild(this.menu);
 
+      this.promoMessage_ = $('other-sessions-promo-message');

[Dan Beam, 2012/03/27 01:21:06]

this would be much more loosely coupled if you passed:

 a) the node
 b) the ID
 c) a selector

to decorate somehow (but I don't think that's how it works) or added a node with
a class (not ID) inside of this menu and .querySelector()'d for it.

[Patrick Dubroy, 2012/03/27 13:44:21]

What is the advantage of that? Currently new_tab.js has no knowledge of the
promo message, because it's an implementation detail of this element. I don't
see why it would be better for new_tab.js to have more knowledge about the inner
workings of this element.
That sounds like _tighter_ coupling of the promo and the menu. That sounds like
a better idea, but as mentioned in my other comment, I don't think it makes
sense to make the promo message a child of the button.

+
+      this.resetMenu_();

[Dan Beam, 2012/03/27 01:21:06]

you're probably better off doing all this in initialize()

[Patrick Dubroy, 2012/03/27 13:44:21]

It's not clear what you're talking about here. What do you mean by "all this"?
Why would it be better?

+
       this.sessions_ = [];
       this.anchorType = cr.ui.AnchorType.ABOVE;
       this.invertLeftRight = true;
 
-      chrome.send('getForeignSessions');
       this.recordUmaEvent_(HISTOGRAM_EVENT.INITIALIZED);
     },
 
     /**
+     * Initialize this element.
+     * @param {boolean} signedIn Is the current user signed in?
+     */
+    initialize: function(signedIn) {
+      this.updateSignInState(signedIn);
+    },
+
+    /**
      * Record an event in the UMA histogram.
      * @param {Number} eventId The id of the event to be recorded.
      */
     recordUmaEvent_: function(eventId) {
       chrome.send('metricsHandler:recordInHistogram',
           ['NewTabPage.OtherSessionsMenu', eventId, HISTOGRAM_EVENT_LIMIT]);
     },
 
     /**
      * Handle a context menu event for an object in the menu's DOM subtree.
@@ skipping 11 matching lines @@
      * @override
      */
     showMenu: function() {
       if (this.sessions_.length == 0)
         chrome.send('getForeignSessions');
       this.recordUmaEvent_(HISTOGRAM_EVENT.SHOW_MENU);
       MenuButton.prototype.showMenu.call(this);
     },
 
     /**
+     * Reset the menu to the default state.
+     * @private
+     */
+    resetMenu_: function() {
+      this.menu.innerHTML = '';
+      this.menu.appendChild(this.promoMessage_);

[Dan Beam, 2012/03/27 01:21:06]

also, if you never need more than 1 and/or you move the markup, you can remove
this appendChild() line

[Patrick Dubroy, 2012/03/27 13:44:21]

What is the purpose of "also" in this sentence? Is it referring to a previous
comment?

The reason for appendChild() here is that all the children of the menu,
including the promo message, get removed in the previous line
(this.menu.innerHTML = ''). The reason for this is that the menu might contain
all the elements that were added by addSession_(). Rather than try to remove
those elements individually, it's simpler to clear all the children and then
re-insert the promo.

+      this.promoMessage_.hidden = true;

[Dan Beam, 2012/03/27 01:21:06]

this should probably be using setPromoVisible_() if you really want reset to
change visiblity, but I see why it's not (it'd be circular)

[Patrick Dubroy, 2012/03/27 13:44:21]

I've removed this line altogether. It's not really necessary.

+    },
+
+    /**
      * Create a custom click handler for a link, so that clicking on a link
      * restores the session (including back stack) rather than just opening
      * the URL.
      */
     makeClickHandler_: function(sessionTag, windowId, tabId) {
       var self = this;
       return function(e) {
         self.recordUmaEvent_(HISTOGRAM_EVENT.LINK_CLICKED);
         chrome.send('openForeignSession', [sessionTag, windowId, tabId,
             e.button, e.altKey, e.ctrlKey, e.metaKey, e.shiftKey]);
@@ skipping 26 matching lines @@
           a.style.backgroundImage = 'url(chrome://favicon/' + tab.url + ')';
           var clickHandler = this.makeClickHandler_(
               session.tag, String(window.sessionId), String(tab.sessionId));
           a.addEventListener('click', clickHandler);
           section.appendChild(a);
         }
       }
     },
 
     /**
-     * Create the UI for the promo and place it inside the menu.
-     * The promo is shown instead of foreign session data when tab sync is
-     * not enabled for a profile.
+     * Show or hide the promo message shown in place of the session list.
+     * The promo is shown when the user is signed in, but there is no foreign
+     * session data available.
+     * @param {boolean} visible Whether the message should be shown.
+     * @private
      */
-    showPromo_: function() {
-      var message = localStrings.getString('otherSessionsEmpty');
-      this.menu.appendChild(this.ownerDocument.createTextNode(message));
+    setPromoVisible_: function(visible) {
+      this.resetMenu_();

[Dan Beam, 2012/03/27 01:21:06]

should setting it |visible| remove the contents?

[Patrick Dubroy, 2012/03/27 13:44:21]

No, probably not. Fixed.

+      this.promoMessage_.hidden = !visible;
     },
 
     /**
-     * Sets the menu model data.
+     * Sets the menu model data. An empty list means that either there are no
+     * foreign sessions, or tab sync is disabled for this profile.
+     * |isTabSyncEnabled| makes it possible to distinguish between the cases.
+     *
      * @param {Array} sessionList Array of objects describing the sessions
-     * from other devices.
+     *     from other devices.
+     * @param {boolean} isTabSyncEnabled Is tab sync enabled for this profile?
      */
-    set sessions(sessionList) {
-      // Clear the current contents of the menu.
-      this.menu.innerHTML = '';
+    setForeignSessions: function(sessionList, isTabSyncEnabled) {
+      this.sessions_ = sessionList;
+      this.setPromoVisible_(sessionList.length == 0);

[Dan Beam, 2012/03/27 01:21:06]

shouldn't this be done after you rebuild the session list?

[Patrick Dubroy, 2012/03/27 13:44:21]

In this version of the code, it needed to be done *before* building the session
list, because setPromoVisible_() also called resetMenu_().

Now that I've changed setPromoVisible_() to not call resetMenu_(), it's not
_necessary_ for setPromoVisible_() to happen before building the session list,
but there's no reason that it can't. There is no ordering dependency between the
two operations.

+      if (sessionList.length > 0) {
+        // Rebuild the menu with the new data.
+        for (var i = 0; i < sessionList.length; i++) {
+          this.addSession_(sessionList[i]);
+        }
+      }
+      // The menu button is shown iff tab sync is enabled.
+      if (isTabSyncEnabled)
+        this.classList.remove('invisible');
+      else
+        this.classList.add('invisible');
+    },
 
-      // Rebuild the menu with the new data.
-      for (var i = 0; i < sessionList.length; i++) {
-        this.addSession_(sessionList[i]);
-      }
-
-      if (sessionList.length == 0)
+    /**
+     * Called when this element is initialized, and from the new tab page when
+     * the user's signed in state changes,
+     * @param {boolean} signedIn Is the user currently signed in?
+     */
+    updateSignInState: function(signedIn) {
+      if (signedIn)
+        chrome.send('getForeignSessions');
+      else
         this.classList.add('invisible');
-      else
-        this.classList.remove('invisible');
-
-      this.sessions_ = sessionList;
     },
   };
 
   return {
     OtherSessionsMenuButton: OtherSessionsMenuButton,
   };
 });