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, 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}
  */
 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();
 }
 
 
 /**
  * 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
  */
@@ skipping 83 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 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);
   }
-  if (ready) {
-    tilesContainer.hidden = false;
-    userInitiatedMostVisitedChange = false;
+  userInitiatedMostVisitedChange = false;
+
+  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();
+}
+
+
+/**
+ * Adds the current list of tiles to DOM.
+ */
+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);
   }
 }
 
 
 /**
  * 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.
@@ skipping 50 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.

[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;
+      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.

[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);
+      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;
-
-  // 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';
+    }
   }
 }
 
 
 /**
  * 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 166 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();
 }