NTP5: Fine tuning of Apps implementation.

chrome/browser/resources/ntp_search/apps_page.js

Index: chrome/browser/resources/ntp_search/apps_page.js
diff --git a/chrome/browser/resources/ntp_search/apps_page.js b/chrome/browser/resources/ntp_search/apps_page.js
index 2366e4c720b524e5213ddb4c30c7ea86ca970c2e..f8419178a32b1ad8bce13db4a7c15930ae66a0a8 100644
--- a/chrome/browser/resources/ntp_search/apps_page.js
+++ b/chrome/browser/resources/ntp_search/apps_page.js
@@ -576,14 +576,18 @@ cr.define('ntp', function() {
     // wants the non-default behavior.
     config: {
       // The width of a cell.
-      cellWidth: 77,
+      cellWidth: 70,
       // The start margin of a cell (left or right according to text direction).
       cellMarginStart: 12,
       // The maximum number of Tiles to be displayed.
-      maxTileCount: 20
+      maxTileCount: 20,
+      // Whether the TilePage content will be scrollable.
+      scrollable: true,
     },
 
     initialize: function() {
+      TilePage.prototype.initialize.apply(this, arguments);
+
       this.classList.add('apps-page');
 
       this.addEventListener('cardselected', this.onCardSelected_);
@@ -595,8 +599,6 @@ cr.define('ntp', function() {
                             this.onCardChangeEnded_);
 
       this.addEventListener('tilePage:tile_added', this.onTileAdded_);
-
-      this.content_.addEventListener('scroll', this.onScroll_.bind(this));
     },
 
     /**
@@ -610,22 +612,12 @@ cr.define('ntp', function() {
     },
 
     /**
-     * Similar to appendApp, but it respects the app_launch_ordinal field of
-     * |appData|.
+     * Inserts an App into the TilePage.
      * @param {Object} appData The data that describes the app.
      * @param {boolean} animate Whether to animate the insertion.
      */
     insertApp: function(appData, animate) {

[Dan Beam, 2012/11/30 21:58:03]

I think this is better named "appendApp" as insert* methods generally allow
inserting in the middle of a list.

[pedro (no code reviews), 2012/11/30 22:59:51]

We want to keep the alphabetical order, so I think the insertApp name makes
sense. I've adjusted the comment too.

-      var index = this.tiles_.length;
-      for (var i = 0; i < this.tiles_.length; i++) {
-        if (appData.app_launch_ordinal <
-            this.tiles_[i].appData.app_launch_ordinal) {
-          index = i;
-          break;
-        }
-      }
-
-      this.addTileAt(new App(appData), index);
+      this.appendTile(new App(appData));

[Evan Stade, 2012/11/30 20:52:46]

where does the alphabetization happen for new apps (not the initial set of apps)

[pedro (no code reviews), 2012/11/30 22:01:18]

Good catch. Added the sorting code back, and changed the sorting criteria to the
lowercase title.

       this.renderGrid_();
     },
 
@@ -684,14 +676,10 @@ cr.define('ntp', function() {
       }
     },
 
-    /**
-     * A handler for when the apps page is scrolled (then we need to reposition
-     * the bubbles.
-     * @private
-     */
-    onScroll_: function(e) {
-      if (!this.selected)
-        return;
+    /** @override */
+    onScroll: function() {
+      TilePage.prototype.onScroll.apply(this, arguments);
+

[Dan Beam, 2012/11/30 21:58:03]

what happened to if (!this.selected) return; ?

[pedro (no code reviews), 2012/11/30 22:59:51]

The DOM element being scrolled here is unique per page, so this means that the
page will always be selected when the onScroll handler is called, therefore,
checking whether or not the page is selected is not necessary.

       for (var i = 0; i < this.tiles_.length; i++) {
         var app = this.tiles_[i];
         assert(app instanceof App);