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

chrome/browser/resources/ntp_search/thumbnail_page.js

Index: chrome/browser/resources/ntp_search/thumbnail_page.js
diff --git a/chrome/browser/resources/ntp_search/thumbnail_page.js b/chrome/browser/resources/ntp_search/thumbnail_page.js
index 5213864e84d0258940ebe2a54d0d444e8c6f9ddc..5cf5156ca387cba14876cb35c0ceb5bbc76431b9 100644
--- a/chrome/browser/resources/ntp_search/thumbnail_page.js
+++ b/chrome/browser/resources/ntp_search/thumbnail_page.js
@@ -110,7 +110,8 @@ cr.define('ntp', function() {
       // 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.
@@ -118,7 +119,8 @@ cr.define('ntp', function() {
         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);
@@ -192,7 +194,7 @@ cr.define('ntp', function() {
         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];
@@ -210,14 +212,68 @@ cr.define('ntp', function() {
     },
 
     /**
-     * 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 */