Fixing iframe jank in the local omnibox popup.

chrome/browser/resources/omnibox_result_loader.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 Orchestrates loading of suggestion content in several
+ * chrome-search://suggestion iframes.
+ */
+
+/**
+ * The origin of the embedding page.
+ * This string literal must be in double quotes for proper escaping.
+ * @type {string}
+ * @const
+ */
+var EMBEDDER_ORIGIN = "%s";
+
+/**
+ * Converts an RGB color number to a hex color string if valid.
+ * @param {number} color A 6-digit hex RGB color code as a number.
+ * @return {?string} A CSS representation of the color or null if invalid.
+ */
+function convertColor(color) {
+  if (typeof(color) == 'number' && !isNaN(color) &&
+      (color % 1) == 0 && color >= 0 && color <= 0xffffff) {

[Dan Beam, 2013/04/06 00:18:12]

if (!isFinite(color) || color < 0 || color > 0xffffff)
  return null;

also, what is the % 1 for?

[Jered, 2013/04/06 02:45:16]

I'll keep the typeof check for extra sanity if you don't mind.
Colors cannot have fractional parts.

[Dan Beam, 2013/04/06 02:51:47]

Math.floor() or Math.round() or assert(color % 1 == 0) or
assert(Math.floor(color) == color);

[Jered, 2013/04/06 16:00:30]

Is this clearer?

+    var hexColor = color.toString(16);
+    return '#000000'.substr(0, 7 - hexColor.length) + hexColor;

[Dan Beam, 2013/04/06 00:18:12]

this is pretty gross, but at least add a comment saying "Pads with initial zeros
(e.g. 0x0000ff.toString(16) yields 'ff')."

[Jered, 2013/04/06 02:45:16]

Done.

+  }
+  return null;
+}
+
+/**
+ * Checks and returns suggestion style.
+ * @param {!Object} pageStyle Instant page-specified overrides for suggestion
+ *     styles.
+ * @return {!Object} Checked styles or defaults.
+ */
+function getStyle(pageStyle) {
+  var apiHandle = chrome.embeddedSearch.searchBox;
+  var style = {
+    queryColor: '#000000',
+    urlColor: '#009933',
+    titleColor: '#666666',

[Dan Beam, 2013/04/06 00:18:12]

shouldn't these be 0x666666, etc.?

[Jered, 2013/04/06 02:45:16]

No, these are the safe fallback values which will be interpolated as written.

[Dan Beam, 2013/04/06 02:51:47]

ah, I see, sorry

+    font: apiHandle.font,
+    fontSize: apiHandle.fontSize
+  };
+  if ('queryColor' in pageStyle)
+    style.queryColor = convertColor(pageStyle.queryColor) || style.queryColor;
+  if ('urlColor' in pageStyle)
+    style.urlColor = convertColor(pageStyle.urlColor) || style.urlColor;
+  if ('titleColor' in pageStyle)
+    style.titleColor = convertColor(pageStyle.titleColor) || style.titleColor;
+  return style;
+}
+
+/**
+ * Renders a native history suggestion.
+ * @param {!Document} resultDoc The suggestion template document.
+ * @param {!Object} suggestion The NativeSuggestion to render.
+ * @param {!Object} pageStyle Page-specificed styles.

[Dan Beam, 2013/04/06 00:18:12]

shouldn't you be doing something along the lines of

  @param {?Object=} opt_pageStyle Optional page-specified styles.

(? mean it could be null, = means it could be undefined)

if you need a pageStyle || {}?

[Jered, 2013/04/06 02:45:16]

I don't need that because the only caller of updateResult ensures pageStyle is
not null. Removed.

+ */
+function updateResult(resultDoc, suggestion, pageStyle) {
+  var style = getStyle(pageStyle || {});
+  resultDoc.body.style.fontSize = style.fontSize + 'px';
+  resultDoc.body.style.fontFamily = style.font;
+  var contentsNode = resultDoc.querySelector('#contents');
+  contentsNode.textContent = suggestion.contents;
+  contentsNode.style.color = suggestion.is_search ?
+      style.queryColor : style.urlColor;
+  var optionalNode = resultDoc.querySelector('#optional');
+  if (suggestion.description) {
+    var titleNode = resultDoc.querySelector('#title');
+    titleNode.textContent = suggestion.description;
+    optionalNode.style.color = style.titleColor;
+    optionalNode.classList.remove('hide');
+  } else {
+    optionalNode.classList.add('hide');
+  }
+}
+
+/**
+ * Handles a postMessage from the embedding page requesting to populate history
+ * suggestion iframes.
+ * @param {!Object} message The message.
+ */
+function handleMessage(message) {
+  // Only allow messages from the embedding page, which should be an Instant
+  // search provider or the local omnibox dropdown (and not e.g. a site which
+  // it has iframed.)
+  if (message.origin != EMBEDDER_ORIGIN)
+    return;
+
+  var apiHandle = chrome.embeddedSearch.searchBox;
+  if ('load' in message.data) {
+    var loaded = [];
+    for (var id in message.data.load) {
+      var restrictedId = message.data.load[id];
+      var suggestion = apiHandle.getSuggestionData(restrictedId);
+      var iframe = window.parent.frames[id];
+      if (iframe) {
+        updateResult(iframe.document, suggestion, message.data.style || {});
+        loaded.push(id);
+      }
+    }
+    message.source.postMessage(
+        {'loaded': message.data.requestId}, message.origin);
+  }
+}
+
+window.addEventListener('message', handleMessage, false);