[NTP4] Auto-deletion of empty apps panes.

chrome/browser/resources/ntp4/tile_page.js

Index: chrome/browser/resources/ntp4/tile_page.js
diff --git a/chrome/browser/resources/ntp4/tile_page.js b/chrome/browser/resources/ntp4/tile_page.js
index b5ab15d0da0b41a68dd75cc9b1a97e31b9056570..a0fe5de53edaab1383159388f66d2cd567e1dd4e 100644
--- a/chrome/browser/resources/ntp4/tile_page.js
+++ b/chrome/browser/resources/ntp4/tile_page.js
@@ -290,10 +290,23 @@ cr.define('ntp4', function() {
     },
 
     /**
-     * Called when an app is removed from Chrome. Animates its disappearance.
+     * Called when a tile is removed from a tilePage. Animates its disappearance
+     * if |opt_animate| is truthy.
+     * @param {=boolean} opt_animate Whether the removal should be animated.
+     * @param {=boolean} opt_dontDelete Whether we should auto-delete an empty
+     *     apps page if the last tile is removed.
+     * @return {Element} The tile being removed.
      */
-    doRemove: function() {
-      this.firstChild.classList.add('removing-tile-contents');
+    remove: function(opt_animate, opt_dontDelete) {
+      var owningPage = this.tilePage;
+      if (opt_animate)
+        owningPage.classList.add('animating-tile-page');
+      this.parentNode.removeChild(this);
+      owningPage.calculateLayoutValues_();
+      owningPage.cleanupDrag();
+      if (owningPage instanceof ntp4.AppsPage && !opt_dontDelete)

[Evan Stade, 2011/12/02 00:53:24]

don't do this, use inheritance or whatever (same applies to all the other places
you used instanceof)

[Dan Beam, 2011/12/02 01:25:11]

So do something like .removeIfEmpty() on TilePage and override for just
AppsPage?  That's cool with me.

Does this mean that I should also fix the other occurrences of the use of this
operator, like:

  if (typeof ntp4.MostVisitedPage != 'undefined' &&
      page instanceof ntp4.MostVisitedPage) {
    assert(this.tilePages.length == 1,
           'MostVisitedPage should be added as first tile page');

or

  var animate = page instanceof ntp4.AppsPage &&
                page.classList.contains('temporary');

to be something like

  if (page.shouldBeFirst())
    assert(this.tilePages.length == 1);

and

  var animate = page.shouldAnimateInsertion();

?

[Evan Stade, 2011/12/02 02:39:49]

no, I just meant in this context. Superclass is not supposed to know about
subclasses.

[Dan Beam, 2011/12/05 18:07:13]

Done. (still ironing out the issues with this small re-design)

+        ntp4.removeAppsPageIfEmpty(owningPage, opt_animate);
+      return this;
     },
 
     /**
@@ -304,7 +317,7 @@ cr.define('ntp4', function() {
       if (this.firstChild.classList.contains('new-tile-contents'))
         this.firstChild.classList.remove('new-tile-contents');
       if (this.firstChild.classList.contains('removing-tile-contents'))
-        this.tilePage.removeTile(this, true);
+        this.remove(true);
     },
   };
 
@@ -498,10 +511,26 @@ cr.define('ntp4', function() {
     },
 
     /**
+     * Removes the tilePage from the DOM and cleans up event handlers.
+     */
+    remove: function() {
+      // This checks arguments.length as most remove functions have a boolean
+      // |opt_animate| argument, but that's not necesarilly applicable to
+      // removing a tilePage. Selecting a different card in an animated way and
+      // deleting the card afterward is probably a better choice.
+      assert(typeof arguments[0] != 'boolean',
+             'This function takes no |opt_animate| argument.');
+      assert(this.tileCount == 0, 'Only removal of an empty page is allowed.');
+      this.tearDown_();
+      this.parentNode.removeChild(this);
+    },
+
+    /**
      * Cleans up resources that are no longer needed after this TilePage
      * instance is removed from the DOM.
+     * @private
      */
-    tearDown: function() {
+    tearDown_: function() {
       this.eventTracker.removeAll();
     },
 
@@ -541,21 +570,6 @@ cr.define('ntp4', function() {
     },
 
     /**
-     * Removes the given tile and animates the respositioning of the other
-     * tiles.
-     * @param {HTMLElement} tile The tile to remove from |tileGrid_|.
-     * @param {?boolean} animate If true, remaining tiles will animate.
-     */
-    removeTile: function(tile, animate) {
-      if (animate)
-        this.classList.add('animating-tile-page');
-      var index = tile.index;
-      tile.parentNode.removeChild(tile);
-      this.calculateLayoutValues_();
-      this.cleanupDrag();
-    },
-
-    /**
      * Removes all tiles from the page.
      */
     removeAllTiles: function() {
@@ -1118,8 +1132,11 @@ cr.define('ntp4', function() {
           var originalPage = currentlyDraggingTile ?
               currentlyDraggingTile.tilePage : null;
           this.addDragData(e.dataTransfer, adjustedIndex);
-          if (originalPage)
+          if (originalPage) {
             originalPage.cleanupDrag();
+            if (originalPage instanceof ntp4.AppsPage)
+              ntp4.removeAppsPageIfEmpty(originalPage, true);
+          }
         }
 
         // Dropping the icon may cause topMargin to change, but changing it
@@ -1142,7 +1159,10 @@ cr.define('ntp4', function() {
         return;
 
       this.addDragData(null, this.tileElements_.length);
-      originalPage.cleanupDrag();
+      if (originalPage instanceof ntp4.AppsPage)
+        ntp4.removeAppsPageIfEmpty(originalPage, true);
+      if (originalPage)
+        originalPage.cleanupDrag();
     },
 
     /**