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.
  */
 
 
@@ skipping 21 matching lines @@
   // 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 Visit changed cuased by user action. If true, then

[Mathieu, 2014/07/25 00:19:17]

nit: extra space, and correct to "Most Visited"

[Mathieu, 2014/07/25 00:19:17]

*caused

[huangs, 2014/07/25 20:12:17]

Done.

[huangs, 2014/07/25 20:12:18]

Done.

+ * in onMostVisitedChange() tiles remain visible, to avoid flickering.
  * @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 74 matching lines @@
 function Tile(elem, opt_rid) {
   /** @type {Element} */
   this.elem = elem;
 
   /** @type {number|undefined} */
   this.rid = opt_rid;
 }
 
 
 /**
+ * A counter with a callback that gets executed on the 1-to-0 transition.
+ *
+ * @param {Function} callback The callback to be executed.
+ * @constructor
+ */
+function Barrier(callback) {

[Mathieu, 2014/07/25 00:19:17]

Could be in a util file included at the top of this file. Keyword: "could"

[huangs, 2014/07/25 20:12:18]

I think the change is short enough to not require including a new file.  Also, I
think we should use Promise instead eventually.  If we really have to do this,
maybe in a refactoring CL?

+  /** @type {Function} */
+  this.callback = callback;
+
+  /** @private {number} */
+  this.count_ = 0;
+}
+
+
+/**
+ * Increments count of the Barrier.
+ */
+Barrier.prototype.lock = function() {
+  ++this.count_;
+};
+
+
+/**
+ * Decrements count of the Barrier, and executes callback on 1-to-0 transition.
+ */
+Barrier.prototype.release = function() {
+  if (this.count_ === 0)  // Guards against underflow.
+    return;
+  --this.count_;
+  if (this.count_ === 0)
+    this.callback();
+};
+
+
+/**
  * Updates the NTP based on the current theme.
  * @private
  */
 function onThemeChange() {
   var info = ntpApiHandle.themeBackgroundInfo;
   if (!info)
     return;
 
   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';
+    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.
+  tileVisibilityBarrier.lock();
+  for (var i = 0; i < MAX_NUM_TILES_TO_SHOW; ++i) {
+    tiles.push(createTile(pages[i], i));
+  }
+  renderTiles();
+  tileVisibilityBarrier.release();
+}
+
+
+/**
+ * Adds the current list of tiles to DOM.
+ */
+function renderTiles() {
+  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);
   }
 }
 
 
 /**
  * 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.
+    titleElement.id = 'title-' + rid;
+    titleElement.className = CLASSES.TITLE;
+    titleElement.hidden = true;
+    tileVisibilityBarrier.lock();

[Mathieu, 2014/07/25 00:19:17]

I don't really like "lock". Wouldn't add, acquire or retain be better?

[huangs, 2014/07/25 20:12:18]

Renamed to add() and remove().

+    titleElement.onload = function() {
+      titleElement.hidden = false;
+      tileVisibilityBarrier.release();
+    };
     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.lock();
+    thumbnailElement.onload = function() {
+      thumbnailElement.hidden = false;
+      tileElement.classList.add(CLASSES.PAGE_READY);
+      tileVisibilityBarrier.release();
+    };
     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);
@@ skipping 51 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 resizing. Also shows or hides extra tiles beyond the bottommost row.

[Mathieu, 2014/07/25 00:19:17]

response *to resizing

[Mathieu, 2014/07/25 00:19:17]

instead of "beyond the bottommost row", how about "Also shows or hides tiles
according to the new width of the page".

[huangs, 2014/07/25 20:12:18]

Done.

[huangs, 2014/07/25 20:12:18]

Done.

  */
 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 is has left and right margins that sum to TILE_MARGIN.

[Mathieu, 2014/07/25 00:19:17]

Each tile *has

[huangs, 2014/07/25 20:12:18]

Done.

+  var tileRequiredWidth = TILE_WIDTH + TILE_MARGIN;
+  var availableWidth = innerWidth + TILE_MARGIN - MIN_TOTAL_HORIZONTAL_PADDING;
+  var newNumColumns = Math.floor(availableWidth / tileRequiredWidth);
+  newNumColumns =
+      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('.mv-tile');
+    var numVisible = Math.min(tiles.length, numColumnsShown * NUM_ROWS);
+    // Not using .hidden becomes it does not work for inline-block elements.

[Mathieu, 2014/07/25 00:19:17]

*because

[huangs, 2014/07/25 20:12:17]

Done.

+    for (var i = 0; i < renderedList.length; ++i) {
+      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();
 }