NTP5: Fix page blacklisting and remove recently closed tabs when they're clicked. Fix the styling …

chrome/browser/resources/ntp_search/thumbnail_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.Tile2;
   var TilePage = ntp.TilePage2;
 
@@ skipping 92 matching lines @@
       var favicon = thumbnailImage.querySelector('.thumbnail-favicon');
       if (favicon)
         thumbnailImage.removeChild(favicon);
 
       var self = this;
       var image = new Image();
 
       // If the thumbnail image fails to load, show the favicon and URL instead.
       // TODO(jeremycho): Move to a separate function?
       image.onerror = function() {
-        banner = self.ownerDocument.createElement('div');
+        banner = thumbnailImage.querySelector('.thumbnail-banner') ||
+            self.ownerDocument.createElement('div');
         banner.className = 'thumbnail-banner';
 
         // For now, just strip leading http://www and trailing backslash.
         // TODO(jeremycho): Consult with UX on URL truncation.
         banner.textContent = dataUrl.replace(/^(http:\/\/)?(www\.)?|\/$/gi, '');
         thumbnailImage.appendChild(banner);
 
-        favicon = self.ownerDocument.createElement('div');
+        favicon = thumbnailImage.querySelector('.thumbnail-favicon') ||
+            self.ownerDocument.createElement('div');
         favicon.className = 'thumbnail-favicon';
         favicon.style.backgroundImage =
             url('chrome://favicon/size/16/' + dataUrl);
         thumbnailImage.appendChild(favicon);
       }
 
       var thumbnailUrl = ntp.getThumbnailUrl(dataUrl);
       thumbnailImage.style.backgroundImage = url(thumbnailUrl);
       image.src = thumbnailUrl;
     },
@@ skipping 53 matching lines @@
     },
 
     /**
      * Update the tiles after a change to |data_|.
      */
     updateTiles_: function() {
       if (!this.hasBeenRendered())
         this.layout_();
 
       var maxTileCount = this.config_.maxTileCount;
-      var data = this.data_;
+      var data = this.getValidData_();
       var tiles = this.tiles;
       for (var i = 0; i < maxTileCount; i++) {
         var page = data[i];
         var tile = tiles[i];
 
         // TODO(pedrosimonetti): What do we do when there's no tile here?
         if (!tile)
           return;
 
         if (i >= data.length)
           tile.reset();
         else
           tile.updateForData(page);
       }
     },
 
     /**
-     * Array of thumbnail data objects.
-     * @type {Array}
+     * Returns all non-null data entries.  Null is used by Most Visited Page as
+     * placeholders for blacklisted entries.
+     * @private
+     * @return {Array} Non-null data entries.
      */
-    get data() {
+    getValidData_: function() {

[pedro (no code reviews), 2012/09/05 18:57:00]

I'd move this below refreshData, since getData, setData and refreshData comes
first when reading the code.

Also, I'd make this function return this.data_ here, and override this function
in MostVisited with the MV-specific logic in there.

[jeremycho, 2012/09/05 22:42:11]

Done.

+      var validData = [];
+      var data = this.data_;
+      for (var i = 0, length = data.length; i < length; i++) {
+        if (data[i])
+          validData.push(data[i]);
+      }
+      return validData;
+    },
+
+    /**
+     * Returns an array of thumbnail data objects.
+     * @return {Array} An array of thumbnail data objects.
+     */
+    getData: function() {
       return this.data_;
     },
-    set data(data) {
-      console.error('ThumbnailPage: data_ setter is not implemented.');
+
+    /**
+     * Sets the data that will be used to create Thumbnails.
+     * @param {Array} data The array of data.
+     */
+    setData: function(data) {
+      var maxTileCount = this.config_.maxTileCount;
+
+      if (!this.data_)
+        this.data_ = data.slice(0, maxTileCount);
+      else
+        this.data_ = this.refreshData_(this.data_, data, maxTileCount);
+
+      var dataLength = this.getValidData_().length;

[pedro (no code reviews), 2012/09/05 18:57:00]

nit: dataLength --> validDataLength

[jeremycho, 2012/09/05 22:42:11]

Done.

+      var tileCount = this.tileCount;
+      // Create or remove tiles if necessary.
+      if (tileCount < dataLength)
+        this.createTiles_(dataLength - tileCount);
+      else if (tileCount > dataLength) {
+        for (var i = 0; i < tileCount - dataLength; i++)
+          this.tiles_.pop();

[pedro (no code reviews), 2012/09/05 18:57:00]

Even though calling pop() will work, it feels wrong doing that, once we're
suppose to use addTile and removeTile functions. But removeTile is not quite
working as expected right now.

Either put a TODO here, or fix the removeTile function.

[jeremycho, 2012/09/05 22:42:11]

removeTile only seems necessary if we wanted to animate the repositioning of the
remaining tiles for Most Visited and Recently Closed (the currently NTP only
animates for Apps).

To call removeTile, we'd have to figure out which tile corresponds to the data
entry that's been deleted.

I'll leave a TODO for now to revisit once we've implemented Apps.

+      }
+
+      if (dataLength != tileCount && tileCount > 0)

[pedro (no code reviews), 2012/09/05 18:57:00]

Include a TODO here. I'll fix the problem in another CL.

[jeremycho, 2012/09/05 22:42:11]

Done.

+        this.renderGrid_();
+      this.updateTiles_();
+    },
+
+    /**
+     * Given an array of new data, returns the data that should replace the
+     * current array of data.  Here we simply return the new data.
+     * @param {Array} oldData The current page list.
+     * @param {Array} newData The new page list.
+     * @param {number} maxTileCount The maximum number of tiles to render.
+     * @return {Array} The merged page list that should replace the current page
+     *     list.
+     * @private
+     */
+    refreshData_: function(oldData, newData, maxTileCount) {
+      return newData.slice(0, maxTileCount);
     },
 
     /** @inheritDoc */
     shouldAcceptDrag: function(e) {
       return false;
     },
   };
 
   return {
     Thumbnail: Thumbnail,
     ThumbnailPage: ThumbnailPage
   };
 });