[NTP4] Auto-deletion of empty apps panes.

chrome/browser/resources/ntp4/apps_page.js

Index: chrome/browser/resources/ntp4/apps_page.js
diff --git a/chrome/browser/resources/ntp4/apps_page.js b/chrome/browser/resources/ntp4/apps_page.js
index a1952af2debe8a687c369d260ba88cbc394fa301..6cfdd961c49c2401ea981895f767a996badd4afe 100644
--- a/chrome/browser/resources/ntp4/apps_page.js
+++ b/chrome/browser/resources/ntp4/apps_page.js
@@ -35,7 +35,7 @@ cr.define('ntp4', function() {
   function AppContextMenu() {
     this.__proto__ = AppContextMenu.prototype;
     this.initialize();
-  };
+  }
   cr.addSingletonGetter(AppContextMenu);
 
   AppContextMenu.prototype = {
@@ -275,17 +275,6 @@ cr.define('ntp4', function() {
     },
 
     /**
-     * Removes the app tile from the page. Should be called after the app has
-     * been uninstalled.
-     */
-    remove: function() {
-      // Unset the ID immediately, because the app is already gone. But leave
-      // the tile on the page as it animates out.
-      this.id = '';
-      this.tile.doRemove();
-    },
-
-    /**
      * Set the URL of the icon from |appData_|. This won't actually show the
      * icon until loadIcon() is called (for performance reasons; we don't want
      * to load icons until we have to).
@@ -589,7 +578,6 @@ cr.define('ntp4', function() {
     removeFromChrome: function() {
       chrome.send('uninstallApp', [this.appData_.id, true]);
       this.tile.tilePage.removeTile(this.tile, true);
-
       if (this.currentBubbleShowing_)
         currentBubbleShowing_.hide();
     },
@@ -647,11 +635,18 @@ cr.define('ntp4', function() {
       this.addEventListener('cardselected', this.onCardSelected_);
     },
 
+    /** @inheritDoc */
+    addTileAt: function(contents, index, animate) {

[Evan Stade, 2011/12/08 02:34:56]

would rather you create an event 'tileAdded' that the TilePage fires on itself,
and AppsPage can listen to these events and do this loadIcon business.

the event seems more generally useful and its purpose is easier to understand
imo.

[Dan Beam, 2011/12/08 04:46:18]

Done.

+      if (contents instanceof App && this.classList.contains('selected-card'))

[Evan Stade, 2011/12/08 02:34:56]

when is the first half of this ever false? probably better to assert that clause

[Dan Beam, 2011/12/08 04:46:18]

Done.

Yeah, I mistakenly thought we allowed non-Apps on an AppsPage (or could possibly
in the future).  My bad.

+        contents.loadIcon();
+      TilePage.prototype.addTileAt.call(this, contents, index, animate);
+    },
+
     /**
      * Creates an app DOM element and places it at the last position on the
      * page.
      * @param {Object} appData The data object that describes the app.
-     * @param {?boolean} animate If true, the app tile plays an animation.
+     * @param {boolean=} animate If true, the app tile plays an animation.
      */
     appendApp: function(appData, animate) {
       if (animate) {
@@ -660,10 +655,7 @@ cr.define('ntp4', function() {
         ntp4.getCardSlider().selectCardByValue(this);
         this.content_.scrollTop = this.content_.scrollHeight;
       }
-      var app = new App(appData);
-      if (this.classList.contains('selected-card'))
-        app.loadIcon();
-      this.appendTile(app, animate);
+      this.appendTile(new App(appData), animate);
     },
 
     /**
@@ -702,12 +694,17 @@ cr.define('ntp4', function() {
       if (currentlyDraggingTile) {
         var tileContents = currentlyDraggingTile.firstChild;
         if (tileContents.classList.contains('app')) {
-          sourceId = currentlyDraggingTile.tilePage == this ?
-              DRAG_SOURCE.SAME_APPS_PANE : DRAG_SOURCE.OTHER_APPS_PANE;
-          this.tileGrid_.insertBefore(
-              currentlyDraggingTile,
-              this.tileElements_[index]);
+          var originalPage = currentlyDraggingTile.tilePage;
+          var samePageDrag = originalPage == this;
+          sourceId = samePageDrag ? DRAG_SOURCE.SAME_APPS_PANE :
+                                    DRAG_SOURCE.OTHER_APPS_PANE;
+          this.tileGrid_.insertBefore(currentlyDraggingTile,
+                                      this.tileElements_[index]);
           this.tileMoved(currentlyDraggingTile);
+          if (!samePageDrag)
+            originalPage.fireRemovedEvent(true);
+          if (originalPage)
+            originalPage.cleanupDrag();
         } else if (currentlyDraggingTile.querySelector('.most-visited')) {
           this.generateAppForLink(tileContents.data);
           sourceId = DRAG_SOURCE.MOST_VISITED_PANE;
@@ -753,20 +750,21 @@ cr.define('ntp4', function() {
       var data = {url: url, title: title};
 
       // Synthesize an app.
-      this.generateAppForLink(data);
+      this.generateAppForLink(data, index);
     },
 
     /**
      * Creates a new crx-less app manifest and installs it.
      * @param {Object} data The data object describing the link. Must have |url|
      *     and |title| members.
-     * TODO(estade): pass along an index.

[Evan Stade, 2011/12/08 02:34:56]

just remove the todo

[Dan Beam, 2011/12/08 02:40:54]

Done. (see newer patch set)

+     * @param {number} index The index at which to insert the newly generated
+     *     app (when it's done being created).
      */
-    generateAppForLink: function(data) {
+    generateAppForLink: function(data, index) {
       assert(data.url != undefined);
       assert(data.title != undefined);
-      var pageIndex = ntp4.getAppsPageIndex(this);
-      chrome.send('generateAppForLink', [data.url, data.title, pageIndex]);
+      chrome.send('generateAppForLink',
+                  [data.url, data.title, ntp4.getAppsPageIndex(this), index]);
     },
 
     /** @inheritDoc */
@@ -774,8 +772,8 @@ cr.define('ntp4', function() {
       if (!(draggedTile.firstChild instanceof App))
         return;
 
-      var pageIndex = ntp4.getAppsPageIndex(this);
-      chrome.send('setPageIndex', [draggedTile.firstChild.appId, pageIndex]);

[Evan Stade, 2011/12/08 02:34:56]

don't make this kind of stylistic change. You're not saving any lines and you're
arguably reducing readability. I wouldn't care one way or the other if you were
writing original code, but it's best to leave existing stuff alone.

[Dan Beam, 2011/12/08 04:46:18]

Done.

[Dan Beam, 2011/12/08 04:46:18]

OK, I didn't mean to do this I just re-wrote this code many times and it ended
up like this (slightly different from the original, wasn't sure if the one time
var was my change or not).  I'll revert these small hunks to make the diff
easier to read.

+      chrome.send('setPageIndex', [draggedTile.firstChild.appId,
+                                   ntp4.getAppsPageIndex(this)]);
 
       var appIds = [];
       for (var i = 0; i < this.tileElements_.length; i++) {
@@ -810,14 +808,14 @@ cr.define('ntp4', function() {
    */
   function launchAppAfterEnable(appId) {
     chrome.send('launchApp', [appId, APP_LAUNCH.NTP_APP_RE_ENABLE]);
-  };
+  }
 
   function appNotificationChanged(id, notification) {
     var app = $(id);
     // The app might have been uninstalled, or notifications might be disabled.
     if (app && !app.appData.notifications_disabled)
       app.setupNotification_(notification);
-  };
+  }
 
   return {
     APP_LAUNCH: APP_LAUNCH,