[NTP4] Auto-deletion of empty apps panes.

chrome/browser/resources/ntp4/apps_page.js

 // Copyright (c) 2011 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.
 
 cr.define('ntp4', function() {
   'use strict';
 
   var localStrings = new LocalStrings;
 
   var APP_LAUNCH = {
@@ skipping 17 matching lines @@
   var DRAG_SOURCE_LIMIT = DRAG_SOURCE.OUTSIDE_NTP + 1;
 
   /**
    * App context menu. The class is designed to be used as a singleton with
    * the app that is currently showing a context menu stored in this.app_.
    * @constructor
    */
   function AppContextMenu() {
     this.__proto__ = AppContextMenu.prototype;
     this.initialize();
-  };
+  }
   cr.addSingletonGetter(AppContextMenu);
 
   AppContextMenu.prototype = {
     initialize: function() {
       var menu = new cr.ui.Menu;
       cr.ui.decorate(menu, cr.ui.Menu);
       menu.classList.add('app-context-menu');
       this.menu = menu;
 
       this.launch_ = this.appendMenuItem_();
@@ skipping 219 matching lines @@
 
     /**
      * Sets the color of the favicon dominant color bar.
      * @param {string} color The css-parsable value for the color.
      */
     set stripeColor(color) {
       this.querySelector('.color-stripe').style.backgroundColor = color;
     },
 
     /**
-     * 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).
      */
     setIcon: function() {
       var src = this.useSmallIcon_ ? this.appData_.icon_small :
                                      this.appData_.icon_big;
       if (!this.appData_.enabled ||
           (!this.appData_.offline_enabled && !navigator.onLine)) {
         src += '?grayscale=true';
@@ skipping 283 matching lines @@
     canBeRemoved: function() {
       return this.appData_.can_uninstall;
     },
 
     /**
      * Uninstalls the app after it's been dropped on the trash.
      */
     removeFromChrome: function() {
       chrome.send('uninstallApp', [this.appData_.id, true]);
       this.tile.tilePage.removeTile(this.tile, true);
-
       if (this.currentBubbleShowing_)
         currentBubbleShowing_.hide();
     },
 
     /**
      * Called when a drag is starting on the tile. Updates dataTransfer with
      * data for this tile.
      */
     setDragData: function(dataTransfer) {
       dataTransfer.setData('Text', this.appData_.title);
@@ skipping 37 matching lines @@
 
   AppsPage.prototype = {
     __proto__: TilePage.prototype,
 
     initialize: function() {
       this.classList.add('apps-page');
 
       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) {
         // Select the page and scroll all the way down so the animation is
         // visible.
         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);
     },
 
     /**
      * Handler for 'cardselected' event, fired when |this| is selected. The
      * first time this is called, we load all the app icons.
      * @private
      */
     onCardSelected_: function(e) {
       var apps = this.querySelectorAll('.app.icon-loading');
       for (var i = 0; i < apps.length; i++) {
@@ skipping 18 matching lines @@
           (e.dataTransfer && e.dataTransfer.types.indexOf('url') != -1);
     },
 
     /** @inheritDoc */
     addDragData: function(dataTransfer, index) {
       var sourceId = -1;
       var currentlyDraggingTile = ntp4.getCurrentlyDraggingTile();
       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;
         }
       } else {
         this.addOutsideData_(dataTransfer, index);
         sourceId = DRAG_SOURCE.OUTSIDE_NTP;
       }
 
       assert(sourceId != -1);
@@ skipping 25 matching lines @@
       }
 
       // Make sure title is >=1 and <=45 characters for Chrome app limits.
       if (!title)
         title = url;
       if (title.length > 45)
         title = title.substring(0, 45);
       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 */
     tileMoved: function(draggedTile) {
       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++) {
         var tileContents = this.tileElements_[i].firstChild;
         if (tileContents instanceof App)
           appIds.push(tileContents.appId);
       }
 
       chrome.send('reorderApps', [draggedTile.firstChild.appId, appIds]);
     },
@@ skipping 14 matching lines @@
       store.setAppsPromoData(data);
   };
 
   /**
    * Launches the specified app using the APP_LAUNCH_NTP_APP_RE_ENABLE
    * histogram. This should only be invoked from the AppLauncherHandler.
    * @param {String} appID The ID of the app.
    */
   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,
     appNotificationChanged: appNotificationChanged,
     AppsPage: AppsPage,
     launchAppAfterEnable: launchAppAfterEnable,
   };
 });
 
 // TODO(estade): update the content handlers to use ntp namespace instead of
 // making these global.
 var appNotificationChanged = ntp4.appNotificationChanged;
 var launchAppAfterEnable = ntp4.launchAppAfterEnable;