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..3916221bf7040b28179e4501632c020b48b2ef69 100644
--- a/chrome/browser/resources/local_ntp/local_ntp.js
+++ b/chrome/browser/resources/local_ntp/local_ntp.js
@@ -39,7 +39,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',
@@ -205,6 +204,13 @@ var fakeboxInputBehavior = NTP_DISPOSE_STATE.HIDE_FAKEBOX_AND_LOGO;
 
 
 /**
+ * A counter to control the visibility of tile elements.
+ * @type {number}
+ */
+var tileVisibilityCounter = 0;

[beaudoin, 2014/07/24 15:24:36]

I'd prefer numHiddenTiles with a matching comment.

[huangs, 2014/07/24 19:33:11]

Per discussion, numHiddenTiles will be off by one because we need to increment
before creating tiles to guard against race condition.

Using Barrier to do this.

+
+
+/**
  * Total tile width. Should be equal to mv-tile's width + 2 * border-width.
  * @private {number}
  * @const
@@ -329,6 +335,7 @@ function onThemeChange() {
   document.body.classList.toggle(CLASSES.ALTERNATE_LOGO, info.alternateLogo);
   updateThemeAttribution(info.attributionUrl);
   setCustomThemeStyle(info);
+
   renderTiles();
 }
 
@@ -432,22 +439,15 @@ 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. On completion, the handler will call
+    // reloadAllTiles().
     var lastBlacklistedTileElement = lastBlacklistedTile.elem;
     lastBlacklistedTileElement.addEventListener(
         'webkitTransitionEnd', blacklistAnimationDone);
     lastBlacklistedTileElement.classList.add(CLASSES.BLACKLIST);
 

[Mathieu, 2014/07/24 15:11:25]

remove extra new line

[huangs, 2014/07/24 19:33:11]

Done.

   } 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() {
@@ -456,41 +456,61 @@ function onMostVisitedChange() {
         }
       }, MOST_VISITED_PAINT_TIMEOUT_MSEC);
     }
-    renderTiles();
+    reloadAllTiles();
   }
 }
 
 
 /**
- * Renders the current set of tiles.
+ * Fetches new data, creates, and renders tiles.
+ */
+function reloadAllTiles() {
+  var pages = ntpApiHandle.mostVisited;
+
+  tiles = [];
+  deltaMostVisitedVisibility(1);

[beaudoin, 2014/07/24 15:24:36]

I'd use just a ++ here and call the other 
  decrementNumberOfVisibleTiles();

[huangs, 2014/07/24 19:33:12]

Changed to Barrier.

+  for (var i = 0; i < MAX_NUM_TILES_TO_SHOW; ++i) {
+    tiles.push(createTile(pages[i], i));
+  }
+  renderTiles();
+  deltaMostVisitedVisibility(-1);
+}
+
+
+/**
+ * Adds the current list of tiles to DOM.
  */
 function renderTiles() {
-  var rows = tilesContainer.children;
-  for (var i = 0; i < rows.length; ++i) {
-    removeChildren(rows[i]);
+  removeChildren(tilesContainer);
+  var renderedList = tilesContainer.querySelectorAll('.mv-tile');
+  var size = Math.min(tiles.length, numColumnsShown * NUM_ROWS);
+  for (var i = 0; i < size; ++i) {
+    tilesContainer.appendChild(tiles[i].elem);
   }
+}
+
 
-  for (var i = 0, length = tiles.length;
-       i < Math.min(length, numColumnsShown * NUM_ROWS); ++i) {
-    rows[Math.floor(i / numColumnsShown)].appendChild(tiles[i].elem);
+/**
+ * Shows or hides rendered tiles, depending on the number of columns shown.
+ */
+function showTiles() {
+  var renderedList = tilesContainer.querySelectorAll('.mv-tile');
+  var numVisible = Math.min(tiles.length, numColumnsShown * NUM_ROWS);
+  for (var i = 0; i < renderedList.length; ++i) {
+    renderedList[i].style.display = i < numVisible ? 'inline-block' : 'none';
   }
 }
 
 
 /**
- * Shows most visited tiles if all child iframes are loaded, and hides them
+ * Changes the visibility counter for most visited tiles by |delta|, and

[Mathieu, 2014/07/24 15:11:25]

*Most Visited

[huangs, 2014/07/24 19:33:11]

Done.

+ * makes the tiles visible once the count decreases to 0.
  * otherwise.

[Mathieu, 2014/07/24 15:11:25]

fix comment

[huangs, 2014/07/24 19:33:11]

Done.

+ * @param {number} delta The amount to change counter by, usually 1 or -1.
  */
-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) {
+function deltaMostVisitedVisibility(delta) {

[Mathieu, 2014/07/24 15:11:25]

As discussed offline, reconsider if this function is needed.

[huangs, 2014/07/24 19:33:11]

This function is needed: in onMostVisitedChange(), the container gets hidden for
0.5s before being shown.  This flow preempts the wait when all the tiles have
been loaded.  Updating comment.

+  tileVisibilityCounter += delta;
+  if (tileVisibilityCounter === 0) {
     tilesContainer.hidden = false;
     userInitiatedMostVisitedChange = false;
   }
@@ -563,36 +583,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.
     titleElement.id = 'title-' + rid;
+    titleElement.className = CLASSES.TITLE;
     titleElement.hidden = true;
+    deltaMostVisitedVisibility(1);

[beaudoin, 2014/07/24 15:24:36]

++

[huangs, 2014/07/24 19:33:11]

Acknowledged.

     titleElement.onload = function() {
       titleElement.hidden = false;
-      updateMostVisitedVisibility();
+      deltaMostVisitedVisibility(-1);

[beaudoin, 2014/07/24 15:24:36]

decrementNumberOfVisibleTiles();

[huangs, 2014/07/24 19:33:11]

Acknowledged.

     };
-    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.
     thumbnailElement.id = 'thumb-' + rid;
+    thumbnailElement.className = CLASSES.THUMBNAIL;
     thumbnailElement.hidden = true;
+    deltaMostVisitedVisibility(1);

[beaudoin, 2014/07/24 15:24:37]

Same.

[huangs, 2014/07/24 19:33:12]

Acknowledged.

     thumbnailElement.onload = function() {
       thumbnailElement.hidden = false;
       tileElement.classList.add(CLASSES.PAGE_READY);
-      updateMostVisitedVisibility();
+      deltaMostVisitedVisibility(-1);
     };
-    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.
@@ -676,7 +696,7 @@ function blacklistAnimationDone() {
   // Need to call explicitly to re-render the tiles, since the initial
   // onmostvisitedchange issued by the blacklist function only triggered
   // the animation.
-  onMostVisitedChange();
+  reloadAllTiles();
 }
 
 
@@ -718,14 +738,14 @@ function onResize() {
   var setWidths = function(tilesContainerWidth) {
     tilesContainer.style.width = tilesContainerWidth + 'px';
     if (fakebox)
-      fakebox.style.width = (tilesContainerWidth - 2) + 'px';
+      fakebox.style.width = (tilesContainerWidth - 22) + 'px';
   };
   if (innerWidth >= 820)
-    setWidths(620);
+    setWidths(640);
   else if (innerWidth >= 660)
-    setWidths(460);
+    setWidths(480);
   else
-    setWidths(300);
+    setWidths(320);
 
   var tileRequiredWidth = TILE_WIDTH + TILE_MARGIN_START;
   // Adds margin-start to the available width to compensate the extra margin
@@ -737,7 +757,7 @@ function onResize() {
                               Math.min(MAX_NUM_COLUMNS, numColumnsToShow));
   if (numColumnsToShow != numColumnsShown) {
     numColumnsShown = numColumnsToShow;
-    renderTiles();
+    showTiles();
   }
 }
 
@@ -924,12 +944,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 +953,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);