Local NTP: Render two rows of Most Visited tiles.

Local NTP: Render two rows of Most Visited tiles.

BUG=196498
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196429

[jeremycho, 2013-04-25 00:03:16]

Hi Dan - this is ready for review.  We'd like to get this in for M28 if
possible.  Thanks!

[Dan Beam, 2013-04-25 00:09:49]

generally lg, lots of red code == good

https://codereview.chromium.org/14261017/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/local_ntp.js (left):

https://codereview.chromium.org/14261017/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:280: } else if (isUndoing) {
^ so what happened to this?

https://codereview.chromium.org/14261017/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14261017/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:421: if (typeof
lastBlacklistedRID != 'undefined') {
nit: no curlies

[jeremycho, 2013-04-25 00:15:03]

Thanks for the quick reply!

https://codereview.chromium.org/14261017/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/local_ntp.js (left):

https://codereview.chromium.org/14261017/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:280: } else if (isUndoing) {
On 2013/04/25 00:09:49, Dan Beam wrote:
> ^ so what happened to this?
We're getting rid of the undo animation - the new set of tiles are rendered
immediately.

https://codereview.chromium.org/14261017/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14261017/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:421: if (typeof
lastBlacklistedRID != 'undefined') {
On 2013/04/25 00:09:49, Dan Beam wrote:
> nit: no curlies

Done.

[Dan Beam, 2013-04-25 00:33:18]

lgtm

[commit-bot: I haz the power, 2013-04-25 01:02:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/14261017/11001

[commit-bot: I haz the power, 2013-04-25 01:03:00]

Failed to apply patch for chrome/browser/resources/local_ntp/local_ntp.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/resources/local_ntp/local_ntp.js
  Hunk #1 FAILED at 36.
  Hunk #2 succeeded at 129 (offset 8 lines).
  Hunk #3 succeeded at 137 (offset 8 lines).
  Hunk #4 succeeded at 169 (offset 8 lines).
  Hunk #5 succeeded at 262 (offset 8 lines).
  Hunk #6 succeeded at 280 (offset 8 lines).
  Hunk #7 succeeded at 371 (offset 8 lines).
  Hunk #8 succeeded at 403 (offset 8 lines).
  Hunk #9 succeeded at 425 (offset 8 lines).
  Hunk #10 succeeded at 440 (offset 8 lines).
  Hunk #11 succeeded at 450 (offset 8 lines).
  Hunk #12 succeeded at 1351 (offset 419 lines).
  Hunk #13 succeeded at 1416 with fuzz 2 (offset 421 lines).
  Hunk #14 succeeded at 1433 with fuzz 1 (offset 421 lines).
  1 out of 14 hunks FAILED -- saving rejects to file
chrome/browser/resources/local_ntp/local_ntp.js.rej

Patch:       chrome/browser/resources/local_ntp/local_ntp.js
Index: chrome/browser/resources/local_ntp/local_ntp.js
diff --git a/chrome/browser/resources/local_ntp/local_ntp.js
b/chrome/browser/resources/local_ntp/local_ntp.js
index
8610518c190a9de19e7a1d190da4795d251c590b..450fc51f1ca04f3575d2be9b759d79ed7572f35b
100644
--- a/chrome/browser/resources/local_ntp/local_ntp.js
+++ b/chrome/browser/resources/local_ntp/local_ntp.js
@@ -36,6 +36,7 @@ var CLASSES = {
   HIDE_NOTIFICATION: 'mv-notice-hide',
   HIDE_TILE: 'mv-tile-hide', // hides tiles on small browser width
   PAGE: 'mv-page', // page tiles
+  ROW: 'mv-row',  // tile row
   SELECTED: 'selected', // a selected suggestion (if any)
   THUMBNAIL: 'mv-thumb',
   TILE: 'mv-tile',
@@ -121,13 +122,6 @@ var tiles = [];
 var lastBlacklistedTile = null;
 
 /**
- * The index of the last blacklisted tile, if any. Used to determine where to
- * re-insert a tile on undo.
- * @type {number}
- */
-var lastBlacklistedIndex = -1;
-
-/**
  * True if a page has been blacklisted and we're waiting on the
  * onmostvisitedchange callback. See onMostVisitedChange() for how this
  * is used.
@@ -136,18 +130,11 @@ var lastBlacklistedIndex = -1;
 var isBlacklisting = false;
 
 /**
- * True if a blacklist has been undone and we're waiting on the
- * onmostvisitedchange callback. See onMostVisitedChange() for how this
- * is used.
- * @type {boolean}
- */
-var isUndoing = false;
-
-/**
- * Current number of tiles shown based on the window width, including filler.
+ * Current number of tiles columns shown based on the window width, including
+ * those that just contain filler.
  * @type {number}
  */
-var numTilesShown = 0;
+var numColumnsShown = 0;
 
 /**
  * The browser embeddedSearch.newTabPage object.
@@ -175,13 +162,19 @@ var TILE_WIDTH = 160;
  * @type {number}
  * @const
  */
-var MOST_VISITED_HEIGHT = 156;
+var MOST_VISITED_HEIGHT = 296;
+
+/** @type {number} @const */
+var MAX_NUM_TILES_TO_SHOW = 8;
 
 /** @type {number} @const */
-var MAX_NUM_TILES_TO_SHOW = 4;
+var MIN_NUM_COLUMNS = 2;
 
 /** @type {number} @const */
-var MIN_NUM_TILES_TO_SHOW = 2;
+var MAX_NUM_COLUMNS = 4;
+
+/** @type {number} @const */
+var NUM_ROWS = 2;
 
 /**
  * Minimum total padding to give to the left and right of the most visited
@@ -262,35 +255,13 @@ function onMostVisitedChange() {
   var pages = ntpApiHandle.mostVisited;
 
   if (isBlacklisting) {
-    // If this was called as a result of a blacklist, add a new replacement
-    // (possibly filler) tile at the end and trigger the blacklist animation.
-    var replacementTile = createTile(pages[MAX_NUM_TILES_TO_SHOW - 1]);
-
-    tiles.push(replacementTile);
-    tilesContainer.appendChild(replacementTile.elem);
-
+    // Trigger the blacklist animation and re-render the tiles when it
+    // completes.
     var lastBlacklistedTileElement = lastBlacklistedTile.elem;
     lastBlacklistedTileElement.addEventListener(
         'webkitTransitionEnd', blacklistAnimationDone);
     lastBlacklistedTileElement.classList.add(CLASSES.BLACKLIST);
-    // In order to animate the replacement tile sliding into place, it must
-    // be made visible.
-    updateTileVisibility(numTilesShown + 1);
-
-  } else if (isUndoing) {
-    // If this was called as a result of an undo, re-insert the last
blacklisted
-    // tile in its old location and trigger the undo animation.
-    tiles.splice(
-        lastBlacklistedIndex, 0, lastBlacklistedTile);
-    var lastBlacklistedTileElement = lastBlacklistedTile.elem;
-    tilesContainer.insertBefore(
-        lastBlacklistedTileElement,
-        tilesContainer.childNodes[lastBlacklistedIndex]);
-    lastBlacklistedTileElement.addEventListener(
-        'webkitTransitionEnd', undoAnimationDone);
-    // Force the removal to happen synchronously.
-    lastBlacklistedTileElement.scrollTop;
-    lastBlacklistedTileElement.classList.remove(CLASSES.BLACKLIST);
+
   } else {
     // Otherwise render the tiles using the new data without animation.
     tiles = [];
@@ -302,12 +273,17 @@ function onMostVisitedChange() {
 }
 
 /**
- * Renders the current set of tiles without animation.
+ * Renders the current set of tiles.
  */
 function renderTiles() {
-  removeChildren(tilesContainer);
-  for (var i = 0, length = tiles.length; i < length; ++i) {
-    tilesContainer.appendChild(tiles[i].elem);
+  var rows = tilesContainer.children;
+  for (var i = 0; i < rows.length; ++i) {
+    removeChildren(rows[i]);
+  }
+
+  for (var i = 0, length = tiles.length;
+       i < Math.min(length, numColumnsShown * NUM_ROWS); ++i) {
+    rows[Math.floor(i / numColumnsShown)].appendChild(tiles[i].elem);
   }
 }
 
@@ -388,19 +364,16 @@ function createTile(page) {
  * is blacklisted.
  * @param {number} rid The RID of the page being blacklisted.
  * @return {function(Event)} A function which handles the blacklisting of the
- *     page by displaying the notification, updating state variables, and
- *     notifying Chrome.
+ *     page by updating state variables and notifying Chrome.
  */
 function generateBlacklistFunction(rid) {
   return function(e) {
     // Prevent navigation when the page is being blacklisted.
     e.stopPropagation();
 
-    showNotification();
     isBlacklisting = true;
     tilesContainer.classList.add(CLASSES.HIDE_BLACKLIST_BUTTON);
     lastBlacklistedTile = getTileByRid(rid);
-    lastBlacklistedIndex = tiles.indexOf(lastBlacklistedTile);
     ntpApiHandle.deleteMostVisitedItem(rid);
   };
 }
@@ -423,16 +396,19 @@ function hideNotification() {
 }
 
 /**
- * Handles the end of the blacklist animation by removing the blacklisted tile.
+ * Handles the end of the blacklist animation by showing the notification and
+ * re-rendering the new set of tiles.
  */
 function blacklistAnimationDone() {
-  tiles.splice(lastBlacklistedIndex, 1);
-  removeNode(lastBlacklistedTile.elem);
-  updateTileVisibility(numTilesShown);
+  showNotification();
   isBlacklisting = false;
   tilesContainer.classList.remove(CLASSES.HIDE_BLACKLIST_BUTTON);
   lastBlacklistedTile.elem.removeEventListener(
       'webkitTransitionEnd', blacklistAnimationDone);
+  // Need to call explicitly to re-render the tiles, since the initial
+  // onmostvisitedchange issued by the blacklist function only triggered
+  // the animation.
+  onMostVisitedChange();
 }
 
 /**
@@ -442,22 +418,8 @@ function blacklistAnimationDone() {
 function onUndo() {
   hideNotification();
   var lastBlacklistedRID = lastBlacklistedTile.rid;
-  if (typeof lastBlacklistedRID != 'undefined') {
-    isUndoing = true;
+  if (typeof lastBlacklistedRID != 'undefined')
     ntpApiHandle.undoMostVisitedDeletion(lastBlacklistedRID);
-  }
-}
-
-/**
- * Handles the end of the undo animation by removing the extraneous end tile.
- */
-function undoAnimationDone() {
-  isUndoing = false;
-  tiles.splice(tiles.length - 1, 1);
-  removeNode(tilesContainer.lastElementChild);
-  updateTileVisibility(numTilesShown);
-  lastBlacklistedTile.elem.removeEventListener(
-      'webkitTransitionEnd', undoAnimationDone);
 }
 
 /**
@@ -471,7 +433,7 @@ function onRestoreAll() {
 
 /**
  * Handles a resize by vertically centering the most visited section
- * and triggering the tile show/hide animation if necessary.
+ * and re-rendering the tiles if the number of columns has changed.
  */
 function onResize() {
   // The Google page uses a fixed layout instead.
@@ -481,23 +443,13 @@ function onResize() {
         Math.max(0, (clientHeight - MOST_VISITED_HEIGHT) / 2) + 'px';
   }
   var clientWidth = document.documentElement.clientWidth;
-  var numTilesToShow = Math.floor(
+  var numColumnsToShow = Math.floor(
       (clientWidth - MIN_TOTAL_HORIZONTAL_PADDING) / TILE_WIDTH);
-  numTilesToShow = Math.max(MIN_NUM_TILES_TO_SHOW, numTilesToShow);
-  if (numTilesToShow != numTilesShown) {
-    updateTileVisibility(numTilesToShow);
-    numTilesShown = numTilesToShow;
-  }
-}
-
-/**
- * Triggers an animation to show the first numTilesToShow tiles and hide the
- * remaining.
- * @param {number} numTilesToShow The number of tiles to show.
- */
-function updateTileVisibility(numTilesToShow) {
-  for (var i = 0, length = tiles.length; i < length; ++i) {
-    tiles[i].elem.classList.toggle(CLASSES.HIDE_TILE, i >= numTilesToShow);
+  numColumnsToShow = Math.max(MIN_NUM_COLUMNS,
+                              Math.min(MAX_NUM_COLUMNS, numColumnsToShow));
+  if (numColumnsToShow != numColumnsShown) {
+    numColumnsShown = numColumnsToShow;
+    renderTiles();
   }
 }
 
@@ -981,6 +933,12 @@ function init() {
   attribution = $(IDS.ATTRIBUTION);
   ntpContents = $(IDS.NTP_CONTENTS);
 
+  for (var i = 0; i < NUM_ROWS; i++) {
+    var row = document.createElement('div');
+    row.classList.add(CLASSES.ROW);
+    tilesContainer.appendChild(row);
+  }
+
   if (isGooglePage) {
     document.body.classList.add(CLASSES.GOOGLE_PAGE);
     var logo = document.createElement('div');
@@ -1038,11 +996,6 @@ function init() {
 
   $(IDS.SUGGESTIONS_CONTAINER).dir = searchboxApiHandl…
(message too large)

[commit-bot: I haz the power, 2013-04-25 01:35:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/14261017/18001

[commit-bot: I haz the power, 2013-04-25 04:13:21]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-25 16:16:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/14261017/18001

[commit-bot: I haz the power, 2013-04-25 17:48:38]

Change committed as 196429