Adds a new NTP endpoint for the single frame of the fast NTP

chrome/browser/resources/local_ntp/most_visited_single.js

Index: chrome/browser/resources/local_ntp/most_visited_single.js
diff --git a/chrome/browser/resources/local_ntp/most_visited_single.js b/chrome/browser/resources/local_ntp/most_visited_single.js
new file mode 100644
index 0000000000000000000000000000000000000000..2d1dda1044e91b7b946111ca8aaebcc86f307324
--- /dev/null
+++ b/chrome/browser/resources/local_ntp/most_visited_single.js
@@ -0,0 +1,293 @@
+/* Copyright 2015 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. */
+
+ // Single iframe for NTP tiles.
+(function() {
+'use strict';
+
+<include src="../../../../ui/webui/resources/js/util.js">
+
+/**
+ * The different types of events that are logged from the NTP.  This enum is
+ * used to transfer information from the NTP javascript to the renderer and is

[Mathieu, 2015/03/12 15:53:26]

nit: JavaScript

[fserb, 2015/03/12 16:05:34]

OMG

+ * not used as a UMA enum histogram's logged value.
+ * Note: Keep in sync with common/ntp_logging_events.h
+ * @enum {number}
+ * @const
+ */
+var LOGTYPE = {

[huangs, 2015/03/12 06:16:56]

NIT: LOG_TYPE?

[fserb, 2015/03/12 16:05:33]

Done.

+  // The suggestion is coming from the server.
+  NTP_SERVER_SIDE_SUGGESTION: 0,

[huangs, 2015/03/12 06:16:56]

The following are unused:
  NTP_SERVER_SIDE_SUGGESTION
  NTP_GRAY_TILE
  NTP_EXTERNAL_TILE
  NTP_GRAY_TILE_FALLBACK
  NTP_EXTERNAL_TILE_FALLBACK

Please verify if they're somehow missed?  Add  "// Unused here." if intended.

[fserb, 2015/03/12 16:05:33]

Done.

+  // The suggestion is coming from the client.
+  NTP_CLIENT_SIDE_SUGGESTION: 1,
+  // Indicates a tile was rendered, no matter if it's a thumbnail, a gray tile
+  // or an external tile.
+  NTP_TILE: 2,
+  // The tile uses a local thumbnail image.
+  NTP_THUMBNAIL_TILE: 3,
+  // Used when no thumbnail is specified and a gray tile with the domain is used
+  // as the main tile.
+  NTP_GRAY_TILE: 4,
+  // The visuals of that tile are handled externally by the page itself.
+  NTP_EXTERNAL_TILE: 5,
+  // There was an error in loading both the thumbnail image and the fallback
+  // (if it was provided), resulting in a grey tile.

[huangs, 2015/03/12 06:16:56]

NIT: grey

[fserb, 2015/03/12 16:05:34]

Done.

+  NTP_THUMBNAIL_ERROR: 6,
+  // Used a gray tile with the domain as the fallback for a failed thumbnail.
+  NTP_GRAY_TILE_FALLBACK: 7,
+  // The visuals of that tile's fallback are handled externally.
+  NTP_EXTERNAL_TILE_FALLBACK: 8,
+  // The user moused over an NTP tile or title.
+  NTP_MOUSEOVER: 9
+};
+
+/**
+ * The origin of this request.
+ * @const {string}
+ */
+var DOMAIN_ORIGIN = '{{ORIGIN}}';
+
+/**
+ * Counter for DOM elements that we are waiting to finish loading.
+ * @type {number}
+ */
+var loadedCounter = 1;
+
+/**
+ * DOM element containing the tiles we are going to present next.
+ * Works as a double-buffer that is shown when we receive a "show" postMessage.
+ * @type {DOM}

[huangs, 2015/03/12 06:16:56]

Note that {DOMNode} appears below, which is inconsistent.

[fserb, 2015/03/12 16:05:34]

Done.

+ */
+var tiles = null;
+
+/**
+ * Log an event on the NTP

[huangs, 2015/03/12 06:16:56]

NIT: "." at end.

[fserb, 2015/03/12 16:05:33]

Done.

+ * @param {number} eventName Event from NTP_LOGGING_EVENT_TYPE.

[huangs, 2015/03/12 06:16:56]

NIT: |eventType| instead of |eventName|?

Also NTP_LOGGING_EVENT_TYPE got renamed?

[fserb, 2015/03/12 16:05:33]

Done.

+ */
+var logEvent = function(eventName) {
+  chrome.embeddedSearch.newTabPage.logEvent(eventName);
+};
+

[Mathieu, 2015/03/12 15:53:26]

nit: either choose 2 lines between functions or 1 line, but be consistent in the
file. I would pick 1 line.

[fserb, 2015/03/12 16:05:34]

I


decided


on


two


lines.

+
+/**
+ * Down count the DOM elements that we are waiting for the page to load.
+ * When we get to 0, we send a message to the parent window.
+ * This is usually used as an EventListener of onload/onerror.
+ */
+var countLoad = function() {
+  loadedCounter--;
+  if (loadedCounter <= 0) {
+    window.parent.postMessage({cmd: 'loaded' }, DOMAIN_ORIGIN);

[huangs, 2015/03/12 06:16:56]

NIT: extra space before "}"?

[fserb, 2015/03/12 16:05:33]

Done.

+    loadedCounter = 1;
+  }
+};
+
+
+/**
+ * Handle postMessages coming from the host page to the iframe.
+ * We try to keep the logic here to a minimum and just dispatch to the relevant
+ * functions.
+ **/
+var handlePostMessage = function(event) {
+  var cmd = event.data.cmd;
+  var args = event.data;

[huangs, 2015/03/12 06:16:56]

|args| is used only once, and in the "else" case you used |event.data| directly.

[fserb, 2015/03/12 16:05:33]

Done.

+
+  if (cmd == 'tile') {
+    addTile(args);
+  } else if (cmd == 'show') {
+    showTiles();
+    countLoad();
+  } else {
+    console.error('Unknown command: ' + event.data);
+  }
+};
+
+/**
+ * Called when the host page has finished sending us tile information and
+ * we are ready to show the new tiles and drop the old ones.
+ */
+var showTiles = function() {
+  // store the tiles on the current closure.

[huangs, 2015/03/12 06:16:56]

Nit: Capitalize "Store".

[fserb, 2015/03/12 16:05:34]

Done.

+  var cur = tiles;
+
+  // Create empty tiles until we have 8.
+  while (cur.childNodes.length < 8) {

[huangs, 2015/03/12 06:16:56]

Magic number 8.

[fserb, 2015/03/12 16:05:34]

Done.

+    addTile({});
+  }
+
+  var parent = $('most-visited');

[huangs, 2015/03/12 06:16:56]

Why not
document.querySelector('#most-visited')?

Do you use anything other than '$' from utils.js?

[fserb, 2015/03/12 16:05:34]

Done.

+
+  // Mark old tile DIV for removal after the transition animation is done.
+  var old = parent.querySelector('#mv-tiles');
+  if (old) {
+    old.id = 'mv-tiles-old';
+    cur.addEventListener('webkitTransitionEnd', function(ev) {
+      if (ev.target === cur) {
+        parent.removeChild(old);
+      }
+    });
+  }
+
+  // Add new tileset.
+  cur.id = 'mv-tiles';
+  parent.appendChild(cur);
+  // We want the CSS transition to trigger, so need to add to the DOM before
+  // setting the style.
+  setTimeout(function() {
+    cur.style.opacity = 1.0;
+  }, 0);
+
+  // Make sure the tiles variable contain the next tileset we may use.
+  tiles = document.createElement('div');
+};
+
+
+/**
+ * Called when the host page wants to add a suggestion tile.
+ * For Most Visited, it grabs the data from Chrome and pass on.
+ * For host page generated it just passes the data.
+ * @param {object} args Data for the tile to be rendered.
+ */
+var addTile = function(args) {
+  if (args.rid) {
+    var data = chrome.embeddedSearch.searchBox.getMostVisitedItemData(args.rid);
+    tiles.appendChild(renderTile(data));
+    logEvent(LOGTYPE.NTP_CLIENT_SIDE_SUGGESTION);
+  } else {
+    tiles.appendChild(renderTile(null));
+  }
+};
+
+
+/**
+ * Called when the user decided to add a tile to the blacklist.
+ * It sets of the animation for the blacklist and sends the blacklisted id
+ * to the host page.
+ * @param {DOMNode} tile DOM node of the tile we want to remove.
+ */
+var blacklistTile = function(tile) {
+  tile.classList.add('blacklisted');
+  var sent = false;
+  tile.addEventListener('webkitTransitionEnd', function() {
+    if (sent) return;
+    sent = true;
+    window.parent.postMessage({cmd: 'tileBlacklisted',
+                               rid: Number(tile.getAttribute('rid'))},

[huangs, 2015/03/12 06:16:56]

HTML custom attributes should have '-', so 'data-rid'?

[fserb, 2015/03/12 16:05:34]

Done.

+                              DOMAIN_ORIGIN);
+  });
+};
+
+
+/**
+ * Renders a MostVisited tile to the DOM.
+ * @param {object} data Object containing rid, url, title, favicon, thumbnail.
+ *                      data is null if you want to construct an empty tile.
+ */
+var renderTile = function(data) {
+  var tile = document.createElement('a');
+  tile.className = 'mv-tile';
+
+  if (data == null) {
+    tile.className = 'mv-empty-tile';
+    return tile;
+  }
+
+  logEvent(LOGTYPE.NTP_TILE);
+
+  tile.setAttribute('rid', data.rid);

[huangs, 2015/03/12 06:16:56]

HTML custom attributes should have '-', so 'data-rid'?

[fserb, 2015/03/12 16:05:34]

Done.

+  tile.innerHTML = '<div class="mv-favicon"></div>' +
+    '<div class="mv-title"></div><div class="mv-thumb"></div>' +
+    '<div title="Don\'t show on this page" class="mv-x"></div>';
+
+  tile.href = data.url;
+  tile.title = data.title;
+  tile.onkeypress = function(ev) {

[huangs, 2015/03/12 06:16:56]

Use
tile.addEventListener('keypress', function() {...} );
instead?

[fserb, 2015/03/12 16:05:33]

Done.

+    if (ev.keyCode == 127) { // DELETE
+      blacklistTile(tile);
+      ev.stopPropagation();
+      return false;
+    }
+  };
+  tile.addEventListener('mouseover', function() {
+    logEvent(LOGTYPE.NTP_MOUSEOVER);

[huangs, 2015/03/12 06:16:56]

I'm a bit concerned with using mouseover; could you check to see if this fires
when mouse enter/leaves child elements?  If so then mouseenter would be more
appropriate.

[fserb, 2015/03/12 16:05:33]

Acknowledged.

+  });
+
+  var title = tile.querySelectorAll('.mv-title')[0];

[huangs, 2015/03/12 06:16:55]

title.querySelector('.mv-title') is cleaner (takes the first).  Not sure if JS
would be so smart enough to NOT build the list of everything, then only extract
the first element -- but we can avoid this.

3 more instances below.

[fserb, 2015/03/12 16:05:34]

Done.

+  title.innerHTML = data.title;

[huangs, 2015/03/12 06:16:55]

Is data.title already escape entities?  Otherwise prefer

title.innerText = data.title;

I think the code to generate data is searchbox_extension.cc:
GenerateMostVisitedItem()

[fserb, 2015/03/12 16:05:34]

Done.

+  title.style.direction = data.direction || 'ltr';
+
+  var thumb = tile.querySelectorAll('.mv-thumb')[0];
+
+  var img = document.createElement('img');

[huangs, 2015/03/12 06:16:55]

Move these 2 lines inside "if" since it's unused outside?  Also improves
symmetry compared to favicon code.

[fserb, 2015/03/12 16:05:33]

Done.

+  img.title = data.title;
+  if (data.thumbnailUrl) {
+    img.src = data.thumbnailUrl;
+    loadedCounter += 1;

[huangs, 2015/03/12 06:16:55]

Nit: Inconsistent with "loadedCounter--" you used earlier.  Another instance
below.

[fserb, 2015/03/12 16:05:34]

Done.

+    img.addEventListener('load', countLoad);
+    img.addEventListener('error', countLoad);
+    img.addEventListener('error', function(ev) {
+      thumb.classList.add('failed-img');
+      thumb.removeChild(img);
+      logEvent(LOGTYPE.NTP_THUMBNAIL_ERROR);
+    });
+    thumb.appendChild(img);
+    logEvent(LOGTYPE.NTP_THUMBNAIL_TILE);
+  } else {
+    thumb.classList.add('failed-img');
+  }
+
+  var favicon = tile.querySelectorAll('.mv-favicon')[0];
+  if (data.faviconUrl) {
+    var fi = document.createElement('img');
+    fi.src = '../' + data.faviconUrl;
+    fi.title = '';

[huangs, 2015/03/12 06:16:56]

Wouldn't |ti.title| be empty already?  Please comment if there's some functional
thing.

[fserb, 2015/03/12 16:05:33]

Done.

[Mathieu, 2015/03/12 17:39:18]

I don't see the results of this comment.

+    loadedCounter += 1;
+    fi.addEventListener('load', countLoad);
+    fi.addEventListener('error', countLoad);
+    fi.addEventListener('error', function(ev) {
+      favicon.classList.add('failed-favicon');
+    });
+    favicon.appendChild(fi);
+  } else {
+    favicon.classList.add('failed-favicon');
+  }
+
+  var mvx = tile.querySelectorAll('.mv-x')[0];
+  mvx.onclick = function(ev) {

[huangs, 2015/03/12 06:16:55]

mvx.addEventListener('click', ...) ?

[fserb, 2015/03/12 16:05:34]

Done.

+    blacklistTile(tile);
+    ev.stopPropagation();
+    return false;
+  };
+
+  return tile;
+};
+
+
+/**
+ * Do some initialization and parses the query arguments passed to the iframe.
+ */
+var init = function() {
+  // Creates a new DOM element to hold the tiles.
+  tiles = document.createElement('div');
+
+  // Parse query arguments.

[Mathieu, 2015/03/12 15:53:26]

are we only doing this for RTL? consider early break, or perhaps moving the
parseQueryParameters logic into a private function.

[fserb, 2015/03/12 16:05:34]

We are only using it for RTL for now, but I expect to use it more for ML.

[Mathieu, 2015/03/12 17:39:18]

Then consider moving to a private function, it cleans up this function.

+  var query = window.location.search.substring(1).split('&');
+  var args = {};
+  for (var i = 0; i < query.length; ++i) {
+    var val = query[i].split('=');
+    if (val[0] == '') continue;
+    args[decodeURIComponent(val[0])] = decodeURIComponent(val[1]);
+  }
+
+  // Enable RTL.
+  if (args['rtl'] == '1') {
+    var html = document.querySelector('html');
+    html.dir = 'rtl';
+  }
+
+  window.addEventListener('message', handlePostMessage);
+};
+
+window.addEventListener('DOMContentLoaded', init);
+})();