Local NTP: prevent tiles from reloading on resize by moving them into single <div>.

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 fb8b873216619a004c19a623ce170a4cb6d0796b..f0a92ebbf27abcaaba8e990cf481178c761b53f0 100644
--- a/chrome/browser/resources/local_ntp/local_ntp.js
+++ b/chrome/browser/resources/local_ntp/local_ntp.js
@@ -14,6 +14,7 @@
  */
 function LocalNTP() {
 <include src="../../../../ui/webui/resources/js/assert.js">
+<include src="local_ntp_util.js">
 <include src="window_disposition_util.js">
 
 
@@ -39,7 +40,6 @@ var CLASSES = {
   NON_GOOGLE_PAGE: 'non-google-page',
   PAGE: 'mv-page', // page tiles
   PAGE_READY: 'mv-page-ready',  // page tile when ready
-  ROW: 'mv-row',  // tile row
   RTL: 'rtl',  // Right-to-left language text.
   THUMBNAIL: 'mv-thumb',
   THUMBNAIL_MASK: 'mv-mask',
@@ -169,14 +169,23 @@ var numColumnsShown = 0;
 
 
 /**
- * True if the user initiated the current most visited change and false
- * otherwise.
+ * A flag to indicate Most Visited changed caused by user action. If true, then
+ * in onMostVisitedChange() tiles remain visible, to avoid flickering.

[James Hawkins, 2014/07/28 17:43:35]

Optional nit: I believe the comma on this line is not necessary (and makes the
sentence confusing).

[huangs, 2014/07/29 19:43:45]

Done, with slight rephrasing.

  * @type {boolean}
  */
 var userInitiatedMostVisitedChange = false;
 
 
 /**
+ * A barrier to make tiles visible the moment all tiles are loaded.
+ * @type {Barrier}
+ */
+var tileVisibilityBarrier = new Barrier(function() {
+  tilesContainer.style.visibility = 'visible';
+});
+
+
+/**
  * The browser embeddedSearch.newTabPage object.
  * @type {Object}
  */
@@ -213,11 +222,11 @@ var TILE_WIDTH = 140;
 
 
 /**
- * Margin between tiles. Should be equal to mv-tile's -webkit-margin-start.
+ * Margin between tiles. Should be equal to mv-tile's total horizontal margin.
  * @private {number}
  * @const
  */
-var TILE_MARGIN_START = 20;
+var TILE_MARGIN = 20;
 
 
 /** @type {number} @const */
@@ -329,6 +338,7 @@ function onThemeChange() {
   document.body.classList.toggle(CLASSES.ALTERNATE_LOGO, info.alternateLogo);
   updateThemeAttribution(info.attributionUrl);
   setCustomThemeStyle(info);
+
   renderTiles();
 }
 
@@ -432,67 +442,73 @@ function convertToRGBAColor(color) {
  * Handles a new set of Most Visited page data.
  */
 function onMostVisitedChange() {
-  var pages = ntpApiHandle.mostVisited;
-
   if (isBlacklisting) {
-    // Trigger the blacklist animation and re-render the tiles when it
-    // completes.
+    // Trigger the blacklist animation, which then triggers reloadAllTiles().
     var lastBlacklistedTileElement = lastBlacklistedTile.elem;
     lastBlacklistedTileElement.addEventListener(
         'webkitTransitionEnd', blacklistAnimationDone);
     lastBlacklistedTileElement.classList.add(CLASSES.BLACKLIST);
-
   } else {
-    // Otherwise render the tiles using the new data without animation.
-    tiles = [];
-    for (var i = 0; i < MAX_NUM_TILES_TO_SHOW; ++i) {
-      tiles.push(createTile(pages[i], i));
-    }
-    if (!userInitiatedMostVisitedChange) {
-      tilesContainer.hidden = true;
-      window.setTimeout(function() {
-        if (tilesContainer) {
-          tilesContainer.hidden = false;
-        }
-      }, MOST_VISITED_PAINT_TIMEOUT_MSEC);
-    }
-    renderTiles();
+    reloadAllTiles();
   }
 }
 
 
 /**
- * Renders the current set of tiles.
+ * Handles the end of the blacklist animation by showing the notification and
+ * re-rendering the new set of tiles.
  */
-function renderTiles() {
-  var rows = tilesContainer.children;
-  for (var i = 0; i < rows.length; ++i) {
-    removeChildren(rows[i]);
+function blacklistAnimationDone() {
+  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.
+  reloadAllTiles();
+}
+
+
+/**
+ * Fetches new data, creates, and renders tiles.
+ */
+function reloadAllTiles() {
+  var pages = ntpApiHandle.mostVisited;
+
+  if (!userInitiatedMostVisitedChange) {
+    // Temporarily hide titleContainer, then show it (1) on timeout, or (2) when
+    // all tiles finish loading (using tileVisibilityBarrier).
+    tilesContainer.style.visibility = 'hidden';

[James Hawkins, 2014/07/28 17:43:35]

Why are you not using tilesContainer.hidden?

[huangs, 2014/07/29 19:43:45]

visibility='hidden' makes #mv-tiles disappear but still taking space. This
prevents any content after the element (e.g., blacklist undo) from jumping up
when #mv-tile is hidden.

Updated comment.

+    window.setTimeout(function() {
+      if (tilesContainer) {
+        tilesContainer.style.visibility = 'visible';
+      }
+    }, MOST_VISITED_PAINT_TIMEOUT_MSEC);
   }
+  userInitiatedMostVisitedChange = false;
 
-  for (var i = 0, length = tiles.length;
-       i < Math.min(length, numColumnsShown * NUM_ROWS); ++i) {
-    rows[Math.floor(i / numColumnsShown)].appendChild(tiles[i].elem);
+  tiles = [];
+  // Increment barrier to guard against race conditions.

[Jered, 2014/07/28 18:32:50]

What is the race here? I think this add() and its matching remove() share the
same stack in a single thread of control.

[huangs, 2014/07/29 19:43:45]

Indeed this is not needed.  I'm being pedantic w.r.t. how Barrier should be
implemented if we have a multi-threaded environment, where multiple 1-0
transitions can occur while we loop over stuff.

An alternative is to do all the add() beforehand. Problem is that in
createTile() a tile may be skipped. Alternatives to address this are: (1) doing
a better count outside: but this duplicate logic and break encapsulation; (2)
remove() if tile is not generated, but this would makes code fragile. Therefore
I'd like to keep add()-remove() pairs close to each other.

Meanwhile, removed the useless initial increment and added comments.

+  tileVisibilityBarrier.add();
+  for (var i = 0; i < MAX_NUM_TILES_TO_SHOW; ++i) {
+    tiles.push(createTile(pages[i], i));
   }
+  renderTiles();
+  tileVisibilityBarrier.remove();
 }
 
 
 /**
- * Shows most visited tiles if all child iframes are loaded, and hides them
- * otherwise.
+ * Adds the current list of tiles to DOM.
  */
-function updateMostVisitedVisibility() {
-  var iframes = tilesContainer.querySelectorAll('iframe');
-  var ready = true;
-  for (var i = 0, numIframes = iframes.length; i < numIframes; i++) {
-    if (iframes[i].hidden) {
-      ready = false;
-      break;
-    }
-  }
-  if (ready) {
-    tilesContainer.hidden = false;
-    userInitiatedMostVisitedChange = false;
+function renderTiles() {
+  removeChildren(tilesContainer);

[Jered, 2014/07/28 18:32:50]

nit: remove this function and just inline tilesContainer.innerHTML = '';

[huangs, 2014/07/29 19:43:45]

Done.

+  var renderedList = tilesContainer.querySelectorAll(CLASSES.TILE);
+  var size = Math.min(tiles.length, numColumnsShown * NUM_ROWS);

[Jered, 2014/07/28 18:32:51]

Define a function to compute this expression and reuse it where it's needed.

[huangs, 2014/07/29 19:43:45]

The other usage was in onResize().  My previous code had the bug where if you
start from few tiles and resize to get more, no new tiles appear.  Therefore I'm
making renderTiles() do more -- not just add tile to DOM, but also controlling
their visibility.  This then makes it option to delete children, so added
parameters.

+  for (var i = 0; i < size; ++i) {

[Jered, 2014/07/28 18:32:50]

nit: omit braces

[huangs, 2014/07/29 19:43:45]

Done.

+    tilesContainer.appendChild(tiles[i].elem);
   }
 }
 
@@ -563,36 +579,36 @@ function createTile(page, position) {
     //
     // TODO(jered): Find and fix the root (probably Blink) bug.
 
-    titleElement.src = getMostVisitedIframeUrl(
-        MOST_VISITED_TITLE_IFRAME, rid, MOST_VISITED_COLOR,
-        MOST_VISITED_FONT_FAMILY, MOST_VISITED_FONT_SIZE, position);
-
     // Keep this id here. See comment above.

[James Hawkins, 2014/07/28 17:43:34]

s/id/ID/

[huangs, 2014/07/29 19:43:45]

Done.

     titleElement.id = 'title-' + rid;
+    titleElement.className = CLASSES.TITLE;
     titleElement.hidden = true;
+    tileVisibilityBarrier.add();
     titleElement.onload = function() {
       titleElement.hidden = false;
-      updateMostVisitedVisibility();
+      tileVisibilityBarrier.remove();
     };
-    titleElement.className = CLASSES.TITLE;
+    titleElement.src = getMostVisitedIframeUrl(
+        MOST_VISITED_TITLE_IFRAME, rid, MOST_VISITED_COLOR,
+        MOST_VISITED_FONT_FAMILY, MOST_VISITED_FONT_SIZE, position);
     tileElement.appendChild(titleElement);
 
     // The iframe which renders either a thumbnail or domain element.
     var thumbnailElement = document.createElement('iframe');
     thumbnailElement.tabIndex = '-1';
-    thumbnailElement.src = getMostVisitedIframeUrl(
-        MOST_VISITED_THUMBNAIL_IFRAME, rid, MOST_VISITED_COLOR,
-        MOST_VISITED_FONT_FAMILY, MOST_VISITED_FONT_SIZE, position);
-
     // Keep this id here. See comment above.

[James Hawkins, 2014/07/28 17:43:34]

s/id/ID/

[huangs, 2014/07/29 19:43:45]

Done.

     thumbnailElement.id = 'thumb-' + rid;
+    thumbnailElement.className = CLASSES.THUMBNAIL;
     thumbnailElement.hidden = true;
+    tileVisibilityBarrier.add();
     thumbnailElement.onload = function() {
       thumbnailElement.hidden = false;
       tileElement.classList.add(CLASSES.PAGE_READY);
-      updateMostVisitedVisibility();
+      tileVisibilityBarrier.remove();
     };
-    thumbnailElement.className = CLASSES.THUMBNAIL;
+    thumbnailElement.src = getMostVisitedIframeUrl(
+        MOST_VISITED_THUMBNAIL_IFRAME, rid, MOST_VISITED_COLOR,
+        MOST_VISITED_FONT_FAMILY, MOST_VISITED_FONT_SIZE, position);
     tileElement.appendChild(thumbnailElement);
 
     // A mask to darken the thumbnail on focus.
@@ -612,8 +628,7 @@ function createTile(page, position) {
     // The page favicon, if any.
     var faviconUrl = page.faviconUrl;
     if (faviconUrl) {
-      var favicon = createAndAppendElement(
-          tileElement, 'div', CLASSES.FAVICON);
+      var favicon = createAndAppendElement(tileElement, 'div', CLASSES.FAVICON);
       favicon.style.backgroundImage = 'url(' + faviconUrl + ')';
     }
     return new Tile(tileElement, rid);
@@ -664,23 +679,6 @@ function hideNotification() {
 
 
 /**
- * Handles the end of the blacklist animation by showing the notification and
- * re-rendering the new set of tiles.
- */
-function blacklistAnimationDone() {
-  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();
-}
-
-
-/**
  * Handles a click on the notification undo link by hiding the notification and
  * informing Chrome.
  */
@@ -705,39 +703,32 @@ function onRestoreAll() {
 
 
 /**
- * Re-renders the tiles if the number of columns has changed.  As a temporary
- * fix for crbug/240510, updates the width of the fakebox and most visited tiles
- * container.
+ * Resizes elements because the number of tile columns may need to change in
+ * response to resizing. Also shows or hides extra tiles tiles according to the
+ * new width of the page.
  */
 function onResize() {
   // If innerWidth is zero, then use the maximum snap size.
   var innerWidth = window.innerWidth || 820;
-
-  // These values should remain in sync with local_ntp.css.
-  // TODO(jeremycho): Delete once the root cause of crbug/240510 is resolved.
-  var setWidths = function(tilesContainerWidth) {
+  // Each tile has left and right margins that sum to TILE_MARGIN.
+  var tileRequiredWidth = TILE_WIDTH + TILE_MARGIN;
+  var availableWidth = innerWidth + TILE_MARGIN - MIN_TOTAL_HORIZONTAL_PADDING;

[Jered, 2014/07/28 18:32:51]

Why do we have an extra TILE_MARGIN px available? I think we just have the
window width minus the min padding.

[huangs, 2014/07/29 19:43:45]

We want to add a bit more width to account for extra 10px + 10px margin of
#mv-tiles

This also preserves old behavior: we have
  MIN_TOTAL_HORIZONTAL_PADDING = 200
  TILE_MARGIN = 20
  tileRequiredWidth = 140 + 20 + 160
Now suppose innerWidth = 820, then
  820 + 20 - 200 = 640
  640 / 160 = 4
We also get 660 --> 3.

+  var newNumColumns = Math.floor(availableWidth / tileRequiredWidth);
+  newNumColumns =

[Jered, 2014/07/28 18:32:50]

nit: I think this expression would be clearer if written
if (newNumColumns < MIN_NUM_COLUMNS)
  newNumColumns = MIN_NUM_COLUMNS;
if (newNumColumns > MAX_NUM_COLUMNS)
  newNumColumns = MAX_NUM_COLUMNS.

[huangs, 2014/07/29 19:43:45]

Done, using "else if" for the ">" check.

+      Math.max(MIN_NUM_COLUMNS, Math.min(newNumColumns, MAX_NUM_COLUMNS));
+  if (numColumnsShown != newNumColumns) {
+    numColumnsShown = newNumColumns;
+    var tilesContainerWidth = numColumnsShown * tileRequiredWidth;
     tilesContainer.style.width = tilesContainerWidth + 'px';
-    if (fakebox)
-      fakebox.style.width = (tilesContainerWidth - 2) + 'px';
-  };
-  if (innerWidth >= 820)
-    setWidths(620);
-  else if (innerWidth >= 660)
-    setWidths(460);
-  else
-    setWidths(300);
-
-  var tileRequiredWidth = TILE_WIDTH + TILE_MARGIN_START;
-  // Adds margin-start to the available width to compensate the extra margin
-  // counted above for the first tile (which does not have a margin-start).
-  var availableWidth = innerWidth + TILE_MARGIN_START -
-      MIN_TOTAL_HORIZONTAL_PADDING;
-  var numColumnsToShow = Math.floor(availableWidth / tileRequiredWidth);
-  numColumnsToShow = Math.max(MIN_NUM_COLUMNS,
-                              Math.min(MAX_NUM_COLUMNS, numColumnsToShow));
-  if (numColumnsToShow != numColumnsShown) {
-    numColumnsShown = numColumnsToShow;
-    renderTiles();
+    if (fakebox)  // -2 to account for border.
+      fakebox.style.width = (tilesContainerWidth - TILE_MARGIN - 2) + 'px';
+    // Shows only rendered tiles in the first NUM_ROWS rows.
+    var renderedList = tilesContainer.querySelectorAll(CLASSES.TILE);
+    var numVisible = Math.min(tiles.length, numColumnsShown * NUM_ROWS);
+    // Not using .hidden because it does not work for inline-block elements.
+    for (var i = 0; i < renderedList.length; ++i) {

[James Hawkins, 2014/07/28 17:43:35]

nit: No braces for single line blocks.

[huangs, 2014/07/29 19:43:45]

Done, though functionality moved to renderTiles().

+      renderedList[i].style.display = i < numVisible ? 'inline-block' : 'none';
+    }
   }
 }
 
@@ -924,12 +915,6 @@ 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 (configData.isGooglePage) {
     var logo = document.createElement('div');
     logo.id = IDS.LOGO;
@@ -939,7 +924,7 @@ function init() {
     fakebox.innerHTML =
         '<input id="' + IDS.FAKEBOX_INPUT +
             '" autocomplete="off" tabindex="-1" aria-hidden="true">' +
-        '<div id=cursor></div>';
+        '<div id="cursor"></div>';
 
     ntpContents.insertBefore(fakebox, ntpContents.firstChild);
     ntpContents.insertBefore(logo, ntpContents.firstChild);
@@ -965,7 +950,6 @@ function init() {
   var notificationCloseButton = $(IDS.NOTIFICATION_CLOSE_BUTTON);
   notificationCloseButton.addEventListener('click', hideNotification);
 
-  userInitiatedMostVisitedChange = false;
   window.addEventListener('resize', onResize);
   onResize();