NTP5: Fine tuning of Apps implementation.

chrome/browser/resources/ntp_search/apps_page.js

 // Copyright (c) 2012 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('ntp', function() {
   'use strict';
 
   var Tile = ntp.Tile;
   var TilePage = ntp.TilePage;
   var APP_LAUNCH = ntp.APP_LAUNCH;
@@ skipping 558 matching lines @@
      * Reference to the Tile subclass that will be used to create the tiles.
      * @constructor
      * @extends {Tile}
      */
     TileClass: App,
 
     // The config object should be defined by a TilePage subclass if it
     // 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

[Dan Beam, 2012/11/29 04:59:55]

nit: hanging "," IMO

[pedro (no code reviews), 2012/11/29 08:02:37]

Done.

     },
 
     initialize: function() {
+      TilePage.prototype.initialize.apply(this, arguments);
+
       this.classList.add('apps-page');
 
       this.addEventListener('cardselected', this.onCardSelected_);
       // Add event listeners for two events, so we can temporarily suppress
       // the app notification bubbles when the app card slides in and out of
       // view.
       this.addEventListener('carddeselected', this.onCardDeselected_);
       this.addEventListener('cardSlider:card_change_ended',
                             this.onCardChangeEnded_);
 
       this.addEventListener('tilePage:tile_added', this.onTileAdded_);
-
-      this.content_.addEventListener('scroll', this.onScroll_.bind(this));
     },
 
     /**
      * Highlight a newly installed app as it's added to the NTP.
      * @param {Object} appData The data object that describes the app.
      */
     insertAndHighlightApp: function(appData) {
       ntp.getCardSlider().selectCardByValue(this);
       this.content_.scrollTop = this.content_.scrollHeight;
       this.insertApp(appData, true);
     },
 
     /**
      * Similar to appendApp, but it respects the app_launch_ordinal field of

[Dan Beam, 2012/11/29 04:59:55]

I don't even see where appendApp exists any more...

[pedro (no code reviews), 2012/11/29 08:02:37]

Correct.

      * |appData|.

[Evan Stade, 2012/11/29 02:35:47]

is this still true?

[pedro (no code reviews), 2012/11/29 08:02:37]

Comment has been updated.

      * @param {Object} appData The data that describes the app.
      * @param {boolean} animate Whether to animate the insertion.
      */
     insertApp: function(appData, animate) {
-      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));
       this.renderGrid_();
     },
 
     /**
      * 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');
@@ skipping 46 matching lines @@
     },
 
     /**
      * A handler for when the apps page is scrolled (then we need to reposition
      * the bubbles.
      * @private
      */
     onScroll_: function(e) {
       if (!this.selected)
         return;
+
+      TilePage.prototype.onScroll_.apply(this, arguments);

[Dan Beam, 2012/11/29 04:59:55]

nit: I'd put super calls before if () returns; if possible, also make onScroll
@protected instead of @private and remove _.

[pedro (no code reviews), 2012/11/29 08:02:37]

Done. I've also removed the selection check once the element being listened is
unique per tile page.

       for (var i = 0; i < this.tiles_.length; i++) {
         var app = this.tiles_[i];
         assert(app instanceof App);
         if (app.currentBubbleShowing_)
           app.currentBubbleShowing_.resizeAndReposition();
         }
     },
 
     /** @override */
     doDragOver: function(e) {
@@ skipping 135 matching lines @@
     if (app && !app.appData.notifications_disabled)
       app.setupNotification_(notification);
   }
 
   return {
     appNotificationChanged: appNotificationChanged,
     AppsPage: AppsPage,
     launchAppAfterEnable: launchAppAfterEnable,
   };
 });