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

chrome/browser/resources/local_ntp/local_ntp.js

 // Copyright 2013 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.
 
 
 /**
  * @fileoverview The local InstantExtended NTP.
  */
 
 
 /**
  * Controls rendering the new tab page for InstantExtended.
  * @return {Object} A limited interface for testing the local NTP.
  */
 function LocalNTP() {
 <include src="../../../../ui/webui/resources/js/assert.js">
+<include src="local_ntp_util.js">
 <include src="window_disposition_util.js">
 
 
 /**
  * Enum for classnames.
  * @enum {string}
  * @const
  */
 var CLASSES = {
   ALTERNATE_LOGO: 'alternate-logo', // Shows white logo if required by theme
   BLACKLIST: 'mv-blacklist', // triggers tile blacklist animation
   BLACKLIST_BUTTON: 'mv-x',
   DELAYED_HIDE_NOTIFICATION: 'mv-notice-delayed-hide',
   FAKEBOX_DISABLE: 'fakebox-disable', // Makes fakebox non-interactive
   FAKEBOX_FOCUS: 'fakebox-focused', // Applies focus styles to the fakebox
   // Applies drag focus style to the fakebox
   FAKEBOX_DRAG_FOCUS: 'fakebox-drag-focused',
   FAVICON: 'mv-favicon',
   HIDE_BLACKLIST_BUTTON: 'mv-x-hide', // hides blacklist button during animation
   HIDE_FAKEBOX_AND_LOGO: 'hide-fakebox-logo',
   HIDE_NOTIFICATION: 'mv-notice-hide',
   // Vertically centers the most visited section for a non-Google provided page.
   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',
   TILE: 'mv-tile',
   TITLE: 'mv-title'
 };
 
 
 /**
  * Enum for HTML element ids.
@@ skipping 109 matching lines @@
 
 /**
  * Current number of tiles columns shown based on the window width, including
  * those that just contain filler.
  * @type {number}
  */
 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 so no flickering occurs.
  * @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}
  */
 var ntpApiHandle;
 
 
 /**
  * The browser embeddedSearch.searchBox object.
  * @type {Object}
  */
@@ skipping 16 matching lines @@
 
 /**
  * Total tile width. Should be equal to mv-tile's width + 2 * border-width.
  * @private {number}
  * @const
  */
 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 */
 var MAX_NUM_TILES_TO_SHOW = 8;
 
 
 /** @type {number} @const */
 var MIN_NUM_COLUMNS = 2;
 
 
@@ skipping 91 matching lines @@
 
   var background = [convertToRGBAColor(info.backgroundColorRgba),
                     info.imageUrl,
                     info.imageTiling,
                     info.imageHorizontalAlignment,
                     info.imageVerticalAlignment].join(' ').trim();
   document.body.style.background = background;
   document.body.classList.toggle(CLASSES.ALTERNATE_LOGO, info.alternateLogo);
   updateThemeAttribution(info.attributionUrl);
   setCustomThemeStyle(info);
-  renderTiles();
+
+  renderTiles(true);
 }
 
 
 /**
  * Updates the NTP style according to theme.
  * @param {Object=} opt_themeInfo The information about the theme. If it is
  * omitted the style will be reverted to the default.
  * @private
  */
 function setCustomThemeStyle(opt_themeInfo) {
@@ skipping 82 matching lines @@
 function convertToRGBAColor(color) {
   return 'rgba(' + color[0] + ',' + color[1] + ',' + color[2] + ',' +
                     color[3] / 255 + ')';
 }
 
 
 /**
  * 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]);
-  }
-
-  for (var i = 0, length = tiles.length;
-       i < Math.min(length, numColumnsShown * NUM_ROWS); ++i) {
-    rows[Math.floor(i / numColumnsShown)].appendChild(tiles[i].elem);
-  }
+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();
 }
 
 
 /**
- * Shows most visited tiles if all child iframes are loaded, and hides them
- * otherwise.
+ * Fetches new data, creates, and renders tiles.
  */
-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;
-    }
+function reloadAllTiles() {
+  var pages = ntpApiHandle.mostVisited;
+
+  if (!userInitiatedMostVisitedChange) {
+    // Temporarily make titleContainer invisible, but it still takes up space.
+    // We make it visible again (1) on timeout, or (2) when all tiles finish
+    // loading (using tileVisibilityBarrier).
+    tilesContainer.style.visibility = 'hidden';
+    window.setTimeout(function() {
+      if (tilesContainer) {
+        tilesContainer.style.visibility = 'visible';

[Jered, 2014/07/30 17:52:14]

You should also reset the barrier here. Otherwise, if you time out reload 1,
then reload again (tearing down the onload listeners), the barrier will be stuck
above 0 and we'll time out every subsequent reload.

Alternatively, create a new barrier for each reload.

[huangs, 2014/07/30 21:14:47]

Doing both: (1) making Barrier cancellable and doing so on tomeout, (2)
instantiating new Barrier, and injecting it into createTile(). The variable is
retained via closure in the handler functions.

Moving the code to renderAndShowTiles().

+      }
+    }, MOST_VISITED_PAINT_TIMEOUT_MSEC);
   }
-  if (ready) {
-    tilesContainer.hidden = false;
-    userInitiatedMostVisitedChange = false;
-  }
+  userInitiatedMostVisitedChange = false;
+
+  tiles = [];
+  // Javascript is single-threaded, so don't need to wrap the loop with

[Jered, 2014/07/30 17:52:14]

nit: omit this comment. Readers should already know this fact.

[huangs, 2014/07/30 21:14:47]

Done.

+  // tileVisibilityBarrier.add() and tileVisibilityBarrier.remove().
+  for (var i = 0; i < MAX_NUM_TILES_TO_SHOW; ++i)
+    tiles.push(createTile(pages[i], i));
+  renderTiles(true);
 }
 
 
+/**
+ * Renders the current list of visible tiles to DOM, and hides tiles that are
+ * already in the DOM but should not be seen.
+ * @param {boolean} clearAll Whether to clear existing rendered tiles.
+ */
+function renderTiles(clearAll) {

[Jered, 2014/07/30 17:52:14]

Instead of adding this boolean parameter, either clear or don't clear the tiles
container before calling this function. I think the logic is the same.

[huangs, 2014/07/30 21:14:47]

Done.

+  var numExisting = 0;
+  if (clearAll)
+    tilesContainer.innerHTML = '';
+  else
+    numExisting = tilesContainer.querySelectorAll('.' + CLASSES.TILE).length;
+  // Only add visible tiles to the DOM, to avoid creating invisible tiles that
+  // produce meaningless impression metrics. However, if a tile becomes
+  // invisible then we leave it in DOM to prevent reload if they're shown again.
+  var numDesired = Math.min(tiles.length, numColumnsShown * NUM_ROWS);
+  var i;

[Jered, 2014/07/30 17:52:14]

nit: you can say var i in the loop initializers below. javascript has function
scope, not block scope.

[huangs, 2014/07/30 21:14:47]

Done.

+  for (i = numExisting; i < numDesired; ++i)
+    tilesContainer.appendChild(tiles[i].elem);
+
+  // Show only the desired tiles. Not using .hidden because it does not work
+  // for inline-block elements.
+  for (i = 0; i < numDesired; ++i)
+    tiles[i].elem.style.display = 'inline-block';
+  // If |numDesired| < |numExisting| then hide extra tiles (e.g., this occurs
+  // when window is downsized).
+  for (; i < numExisting; ++i)
+    tiles[i].elem.style.display = 'none';
+}
+
+
 /**
  * Builds a URL to display a most visited tile component in an iframe.
  * @param {string} filename The desired most visited component filename.
  * @param {number} rid The restricted ID.
  * @param {string} color The text color for text in the iframe.
  * @param {string} fontFamily The font family for text in the iframe.
  * @param {number} fontSize The font size for text in the iframe.
  * @param {number} position The position of the iframe in the UI.
  * @return {string} An URL to display the most visited component in an iframe.
  */
@@ skipping 46 matching lines @@
     // begins loading RIDs n, n+1, ..., n+k-1; after the second event, these get
     // destroyed and a new set begins loading RIDs n+k, n+k+1, ..., n+2k-1.
     // Now due to crbug.com/68841, Chrome incorrectly loads the content for the
     // first set of iframes into the most recent set of iframes.
     //
     // Giving iframes distinct ids seems to cause some invalidation and prevent
     // associating the incorrect data.
     //
     // TODO(jered): Find and fix the root (probably Blink) bug.
 
+    // Keep this ID here. See comment above.
+    titleElement.id = 'title-' + rid;
+    titleElement.className = CLASSES.TITLE;
+    titleElement.hidden = true;
+    tileVisibilityBarrier.add();
+    titleElement.onload = function() {
+      titleElement.hidden = false;
+      tileVisibilityBarrier.remove();
+    };
     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.hidden = true;
-    titleElement.onload = function() {
-      titleElement.hidden = false;
-      updateMostVisitedVisibility();
-    };
-    titleElement.className = CLASSES.TITLE;
     tileElement.appendChild(titleElement);
 
     // The iframe which renders either a thumbnail or domain element.
     var thumbnailElement = document.createElement('iframe');
     thumbnailElement.tabIndex = '-1';
+    // Keep this ID here. See comment above.
+    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);
+      tileVisibilityBarrier.remove();
+    };
     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.hidden = true;
-    thumbnailElement.onload = function() {
-      thumbnailElement.hidden = false;
-      tileElement.classList.add(CLASSES.PAGE_READY);
-      updateMostVisitedVisibility();
-    };
-    thumbnailElement.className = CLASSES.THUMBNAIL;
     tileElement.appendChild(thumbnailElement);
 
     // A mask to darken the thumbnail on focus.
     var maskElement = createAndAppendElement(
         tileElement, 'div', CLASSES.THUMBNAIL_MASK);
 
     // The button used to blacklist this page.
     var blacklistButton = createAndAppendElement(
         tileElement, 'div', CLASSES.BLACKLIST_BUTTON);
     var blacklistFunction = generateBlacklistFunction(rid);
     blacklistButton.addEventListener('click', blacklistFunction);
     blacklistButton.title = configData.translatedStrings.removeThumbnailTooltip;
 
     // When a tile is focused, have delete also blacklist the page.
     registerKeyHandler(tileElement, KEYCODE.DELETE, blacklistFunction);
 
     // 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);
   } else {
     return new Tile(tileElement);
   }
 }
 
 
 /**
@@ skipping 30 matching lines @@
 
 /**
  * Hides the blacklist notification.
  */
 function hideNotification() {
   notification.classList.add(CLASSES.HIDE_NOTIFICATION);
 }
 
 
 /**
- * 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.
  */
 function onUndo() {
   userInitiatedMostVisitedChange = true;
   hideNotification();
   var lastBlacklistedRID = lastBlacklistedTile.rid;
   if (typeof lastBlacklistedRID != 'undefined')
     ntpApiHandle.undoMostVisitedDeletion(lastBlacklistedRID);
 }
 
 
 /**
  * Handles a click on the restore all notification link by hiding the
  * notification and informing Chrome.
  */
 function onRestoreAll() {
   userInitiatedMostVisitedChange = true;
   hideNotification();
   ntpApiHandle.undoAllMostVisitedDeletions();
 }
 
 
 /**
- * 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;
+  // 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/30 17:52:14]

I still don't understand this math. Imagine MIN_TOTAL_HORIZONTAL_PADDING were 0.
Would it then be ok to position tiles such that the window has a horizontal
scrollbar to show the extra margin?

[huangs, 2014/07/30 21:14:47]

Yeah it would be bizarre if the extra margin creates unnecessary scroll bar. But
then MIN_TOTAL_HORIZONTAL_PADDING gives us ample space to grow into.  And on
desktop (well, Windows) the window cannot go sufficiently small for this to
happen.

If we ever need make padding small (mobile?), we can put a slightly shifted
#mv-tiles inside a wrapper tile with "overflow:hidden" to solve this problem.

+  var newNumColumns = Math.floor(availableWidth / tileRequiredWidth);
+  if (newNumColumns < MIN_NUM_COLUMNS)
+    newNumColumns = MIN_NUM_COLUMNS;
+  else if (newNumColumns > MAX_NUM_COLUMNS)
+    newNumColumns = MAX_NUM_COLUMNS;
 
-  // 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) {
+  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';
+    renderTiles(false);
   }
 }
 
 
 /**
  * Returns the tile corresponding to the specified page RID.
  * @param {number} rid The page RID being looked up.
  * @return {Tile} The corresponding tile.
  */
 function getTileByRid(rid) {
@@ skipping 122 matching lines @@
 /**
  * Removes a node from its parent.
  * @param {Node} node The node to remove.
  */
 function removeNode(node) {
   node.parentNode.removeChild(node);
 }
 
 
 /**
- * Removes all the child nodes on a DOM node.
- * @param {Node} node Node to remove children from.
- */
-function removeChildren(node) {
-  node.innerHTML = '';
-}
-
-
-/**
  * @param {!Element} element The element to register the handler for.
  * @param {number} keycode The keycode of the key to register.
  * @param {!Function} handler The key handler to register.
  */
 function registerKeyHandler(element, keycode, handler) {
   element.addEventListener('keydown', function(event) {
     if (event.keyCode == keycode)
       handler(event);
   });
 }
@@ skipping 15 matching lines @@
  * Prepares the New Tab Page by adding listeners, rendering the current
  * theme, the most visited pages section, and Google-specific elements for a
  * Google-provided page.
  */
 function init() {
   tilesContainer = $(IDS.TILES);
   notification = $(IDS.NOTIFICATION);
   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;
 
     fakebox = document.createElement('div');
     fakebox.id = IDS.FAKEBOX;
     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);
   } else {
     document.body.classList.add(CLASSES.NON_GOOGLE_PAGE);
   }
 
   var notificationMessage = $(IDS.NOTIFICATION_MESSAGE);
   notificationMessage.textContent =
       configData.translatedStrings.thumbnailRemovedNotification;
   var undoLink = $(IDS.UNDO_LINK);
   undoLink.addEventListener('click', onUndo);
   registerKeyHandler(undoLink, KEYCODE.ENTER, onUndo);
   undoLink.textContent = configData.translatedStrings.undoThumbnailRemove;
   var restoreAllLink = $(IDS.RESTORE_ALL_LINK);
   restoreAllLink.addEventListener('click', onRestoreAll);
   registerKeyHandler(restoreAllLink, KEYCODE.ENTER, onUndo);
   restoreAllLink.textContent =
       configData.translatedStrings.restoreThumbnailsShort;
   $(IDS.ATTRIBUTION_TEXT).textContent =
       configData.translatedStrings.attributionIntro;
 
   var notificationCloseButton = $(IDS.NOTIFICATION_CLOSE_BUTTON);
   notificationCloseButton.addEventListener('click', hideNotification);
 
-  userInitiatedMostVisitedChange = false;
   window.addEventListener('resize', onResize);
   onResize();
 
   var topLevelHandle = getEmbeddedSearchApiHandle();
 
   ntpApiHandle = topLevelHandle.newTabPage;
   ntpApiHandle.onthemechange = onThemeChange;
   ntpApiHandle.onmostvisitedchange = onMostVisitedChange;
 
   ntpApiHandle.oninputstart = onInputStart;
@@ skipping 61 matching lines @@
 
 return {
   init: init,
   listen: listen
 };
 }
 
 if (!window.localNTPUnitTest) {
   LocalNTP().listen();
 }