[NTP4] Make TilePage and CardSlider evented to simplify code and fix page switcher bug

chrome/browser/resources/shared/js/cr/ui/card_slider.js

Index: chrome/browser/resources/shared/js/cr/ui/card_slider.js
diff --git a/chrome/browser/resources/shared/js/cr/ui/card_slider.js b/chrome/browser/resources/shared/js/cr/ui/card_slider.js
index 72951ffc815b334a4bc895aee6cc9a1b05c50b55..cc72d998e3c169ff99a906f185b79ee4270d6b77 100644
--- a/chrome/browser/resources/shared/js/cr/ui/card_slider.js
+++ b/chrome/browser/resources/shared/js/cr/ui/card_slider.js
@@ -78,8 +78,11 @@ cr.define('cr.ui', function() {
    * Events are fired at the container.
    */
   CardSlider.EventType = {
+    CARD_ADDED: 'cardSlider:card_added',
+    CARD_REMOVED: 'cardSlider:card_removed',
     // Fired when the user slides to another card.
-    CARD_CHANGED: 'cardSlider:card_changed'
+    CARD_CHANGED: 'cardSlider:card_changed',
+    CARD_CHANGE_ENDED: 'cardSlider:card_change_ended',

[Evan Stade, 2012/01/17 19:45:29]

as long as you're changing this, I'm unsure of its utility. Putting event name
strings in this struct won't generate compile time errors; will it even generate
runtime errors if you type, say, CARD_CHANGE_END instead of _ENDED? does
AddEventListener complaing about undefined input?

[Dan Beam, 2012/01/17 21:33:09]

Nope.
Nope.

It would allow us to change the strange_use_of_underscores_in_js without any
changes other than here and would minify better (force of habit I guess...we
don't need to in Chrome).  I can remove if you want, I just don't see the
*problem* with doing this, so I just followed the precedent.

[Evan Stade, 2012/01/17 22:33:05]

I believe we've had this conversation before, and concluded no it wouldn't allow
us to make most changes only here.

[Dan Beam, 2012/01/17 23:33:17]

Done. (I know you'd rather change the name, *however* in this case of [quite
feasibly] changing from under_scores to camelCase it'd actually work.)

   };
 
 
@@ -132,7 +135,7 @@ cr.define('cr.ui', function() {
       this.frame_.addEventListener('mousewheel',
                                    this.onMouseWheel_.bind(this));
       this.container_.addEventListener(
-          'webkitTransitionEnd', this.onAnimationTransitioned_.bind(this));
+          'webkitTransitionEnd', this.onWebkitTransitionEnd_.bind(this));
 
       // Also support touch events in case a touch screen happens to be
       // available.  Ideally we would support touch events whenever they
@@ -299,14 +302,126 @@ cr.define('cr.ui', function() {
     },
 
     /**
-     * A handler for the animations ending their transition.
+     * Handles the ends of -webkit-transitions on -webkit-transform (animated
+     * card switches).
+     * @param {Event} e The webkitTransitionEnd event.
      * @private
      */
-    onAnimationTransitioned_: function(event) {
-      if (event.target.id == 'page-list') {
-        cr.dispatchSimpleEvent(this.currentCardValue, 'cardSelectionCompleted',
-                               true, true);
+    onWebkitTransitionEnd_: function(e) {
+      // Ignore irrelevant transitions that might bubble up.
+      if (e.target !== this.container_ ||
+          e.propertyName != '-webkit-transform') {

[Evan Stade, 2012/01/17 19:45:29]

why !== then !=

[Dan Beam, 2012/01/17 21:33:09]

I tend to use strict equal/does-not-equal operator when you have 2 unknown types
(the second comparison is to string literal) and when comparing to something on
this.  In general I don't like possibly accidentally comparing references as
null == undefined (or undefined == false), as you could possibly get into
something like:

  var shouldHide = false;
  window.setTimeout(function() {
    // If this node belongs to us.
    if ($('blah') == this.blah_) {
      // $('blah') might be null, this.blah_ probably undefined
      this.doBlah_();
    }
    // Only show if we're not already.
    if (shouldHide == this.isHidden_) {
      // false != undefined, true, this gets called
      return;
    } else {
      // Would throw, but never get here.
      this.hide_();
    }
  }, 100); // forget to .bind()

In this case e.target is always non-null (I believe) if you want me to change.

+        return;
       }
+      this.fireChangeEndedEvent_(true);
+    },
+
+    /**
+     * Dispatches a simple event to tell subscribers we're done moving to the
+     * newly selected card.
+     * @param {boolean} wasAnimated whether or not the change was animated.
+     * @private
+     */
+    fireChangeEndedEvent_: function(wasAnimated) {
+      var e = document.createEvent('Event');
+      e.initEvent(CardSlider.EventType.CARD_CHANGE_ENDED, true, true);
+      e.cardSlider = this;
+      e.changedTo = this.currentCard_;
+      e.wasAnimated = wasAnimated;
+      this.container_.dispatchEvent(e);
+    },
+
+    /**
+     * Add a card to the card slider at a particular index. If the card being
+     * added is inserted in front of the current card, cardSlider.currentCard
+     * will be adjusted accordingly (to current card + 1).
+     * @param {!Node} card A card that will be added to the card slider.
+     * @param {number} index An index at which the given |card| should be
+     *     inserted. Must be positive and less than the number of cards.
+     */
+    addCardAtIndex: function(card, index) {
+      assert(card instanceof Node, '|card| isn\'t a Node');
+      this.assertValidIndex_(index);
+      this.cards_ = Array.prototype.concat.call(
+          this.cards_.slice(0, index), card, this.cards_.slice(index));
+      if (index <= this.currentCard_)
+        this.selectCard(this.currentCard_ + 1, false, true);
+      this.fireAddedEvent_(card, index);
+    },
+
+    /**
+     * Append a card to the end of the list.
+     * @param {!Node} card A card to add at the end of the card slider.
+     */
+    appendCard: function(card) {
+      assert(card instanceof Node, '|card| isn\'t a Node');
+      this.cards_.push(card);
+      this.fireAddedEvent_(card, this.cards_.length - 1);
+    },
+
+    /**
+     * Dispatches a simple event to tell interested subscribers that a card was
+     * added to this card slider.
+     * @param {Node} card The recently added card.
+     * @param {number} index The position of the newly added card.
+     * @private
+     */
+    fireAddedEvent_: function(card, index) {
+      this.assertValidIndex_(index);
+      var e = document.createEvent('Event');
+      e.initEvent(CardSlider.EventType.CARD_ADDED, true, true);
+      e.addedIndex = index;
+      e.addedCard = card;
+      this.container_.dispatchEvent(e);
+    },
+
+    /**
+     * Removes a card by index from the card slider. If the card to be removed
+     * is the current card or in front of the current card, the current card
+     * will be updated (to current card - 1).
+     * @param {!Node} card A card to be removed.
+     */
+    removeCard: function(card) {
+      assert(card instanceof Node, '|card| isn\'t a Node');
+      this.removeCardAtIndex(this.cards_.indexOf(card));
+    },
+
+    /**
+     * Removes a card by index from the card slider. If the card to be removed
+     * is the current card or in front of the current card, the current card
+     * will be updated (to current card - 1).
+     * @param {number} index The index of the tile that should be removed.
+     */
+    removeCardAtIndex: function(index) {
+      this.assertValidIndex_(index);
+      var removed = this.cards_.splice(index, 1).pop();
+      if (index < this.currentCard_)
+        this.selectCard(this.currentCard_ - 1, false, true);
+      this.fireRemovedEvent_(removed, index);
+    },
+
+    /**
+     * Dispatches a CARD_REMOVED event so interested subscribers know when a
+     * card was removed from this card slider.
+     * @param {Node} card The recently removed card.
+     * @param {number} index The index of the card before it was removed.
+     * @private
+     */
+    fireRemovedEvent_: function(card, index) {
+      var e = document.createEvent('Event');
+      e.initEvent(CardSlider.EventType.CARD_REMOVED, true, true);
+      e.removedCard = card;
+      e.removedIndex = index;
+      this.container_.dispatchEvent(e);
+    },
+
+    /**
+     * Checks the the given |index| exists in this.cards_.
+     * @param {number} index An index to check.
+     * @private
+     */
+    assertValidIndex_: function(index) {
+      assert(index >= 0 && index < this.cards_.length);
     },
 
     /**
@@ -315,27 +430,30 @@ cr.define('cr.ui', function() {
      * @param {number} newCardIndex Index of card to show.
      * @param {boolean=} opt_animate If true will animate transition from
      *     current position to new position.
+     * @param {boolean=} opt_dontNotify If true, don't tell subscribers that
+     *     we've changed cards.
      */
-    selectCard: function(newCardIndex, opt_animate) {
-      var previousCard = this.currentCardValue;
+    selectCard: function(newCardIndex, opt_animate, opt_dontNotify) {
+      this.assertValidIndex_(newCardIndex);
 
+      var previousCard = this.currentCardValue;
       var isChangingCard =
           !this.cards_[newCardIndex].classList.contains('selected-card');
 
       if (isChangingCard) {
-        previousCard.classList.remove('selected-card');
-        // If we have a new card index and it is valid then update the left
-        // position and current card index.
+        if (previousCard)
+          previousCard.classList.remove('selected-card');
         this.currentCard_ = newCardIndex;
         this.currentCardValue.classList.add('selected-card');
       }
 
-      this.transformToCurrentCard_(opt_animate);
+      var willTransitionHappen = this.transformToCurrentCard_(opt_animate);
 
-      if (isChangingCard) {
+      if (isChangingCard && !opt_dontNotify) {
         var event = document.createEvent('Event');
         event.initEvent(CardSlider.EventType.CARD_CHANGED, true, true);
         event.cardSlider = this;
+        event.wasAnimated = !!opt_animate;
         this.container_.dispatchEvent(event);
 
         // We also dispatch an event on the cards themselves.
@@ -346,6 +464,13 @@ cr.define('cr.ui', function() {
         cr.dispatchSimpleEvent(this.currentCardValue, 'cardselected',
                                true, true);
       }
+
+      // If we're not changing, animated, or transitioning, fire a
+      // CARD_CHANGE_ENDED event right away.
+      if ((!isChangingCard || !opt_animate || !willTransitionHappen) &&
+          !opt_dontNotify) {
+        this.fireChangeEndedEvent_(false);
+      }
     },
 
     /**
@@ -364,13 +489,20 @@ cr.define('cr.ui', function() {
      * animate to that card or snap to it.
      * @param {boolean=} opt_animate If true will animate transition from
      *     current position to new position.
+     * @return {boolean} Whether or not a transformation was necessary.
      * @private
      */
     transformToCurrentCard_: function(opt_animate) {
+      var prevLeft = this.currentLeft_;
       this.currentLeft_ = -this.cardWidth_ *
           (isRTL() ? this.cards_.length - this.currentCard - 1 :
                      this.currentCard);
 
+      // If there's no change, return something to let the caller know there
+      // won't be a transition occuring.
+      if (prevLeft == this.currentLeft_)
+        return false;
+
       // Animate to the current card, which will either transition if the
       // current card is new, or reset the existing card if we didn't drag
       // enough to change cards.
@@ -381,6 +513,8 @@ cr.define('cr.ui', function() {
       }
       this.container_.style.WebkitTransition = transition;
       this.translateTo_(this.currentLeft_);
+
+      return true;
     },
 
     /**