Fixing iframe jank in the local omnibox popup.

chrome/browser/resources/omnibox_result_loader.js

+/**
+ * @fileoverview Orchestrates loading of suggestion content in several
+ * chrome-search://suggestion iframes.
+ */
+
+/**
+ * The origin of the embedding page.
+ * Must be in double quotes for proper escaping.
+ * @type {string}
+ * @const
+ */
+var EMBEDDER_ORIGIN = "%s";

[Dan Beam, 2013/04/05 01:07:37]

can you share with with the other file?

[Jered, 2013/04/05 15:30:23]

Because I've got CSP disallowing inline script, I think I need separate js files
for the loader and result iframes (because only the result iframes should bind a
click handler e.g.)

+
+/**
+ * Checks whether a string is a valid color code.
+ * @param {string} color A color code.
+ * @return {boolean} True if color is a valid color code and false otherwise.
+ */
+function isValidColor(color) {
+  // Accept 3 or 6 digit hex colors preceded by '#'.
+  return /^#[0-9A-Fa-f]{3}|#[0-9A-Fa-f]{6}$/.test(color);

[Dan Beam, 2013/04/05 01:07:37]

'\m/d(^o^)b\m/#123456' passes, '#123_OMGWTFBBQ' passes

  return /^#([0-9a-f]{3}|[0-9a-f]{6})$/i.test(color);

tests?

[palmer, 2013/04/05 01:30:32]

This is too permissive.

Express color as an integer, and format it upon interpolation?

Security pattern: Strings are blobs; prefer specific and primitive types to
generic, blobular thingies.

[Dan Beam, 2013/04/05 01:38:45]

^ what can you do with this?

[Jered, 2013/04/05 15:30:23]

Sorry, I got this wrong.

[Jered, 2013/04/05 15:30:23]

That is a good pattern, but it's rather awkward in Javascript. What do you guys
think of this?

+}
+
+/**
+ * Checks and returns suggestion style.
+ * @param {Object} pageStyle Instant page-specified overrides for suggestion
+ *     styles.
+ * @return {Object} Checked styles or defaults.

[Dan Beam, 2013/04/05 01:07:37]

^ can these be null?

[Jered, 2013/04/05 15:30:23]

Done.

[Jered, 2013/04/05 15:30:23]

Done.

+ */
+function getStyle(pageStyle) {
+  var apiHandle = chrome.embeddedSearch.searchBox;
+  var safeStyle = {
+    queryColor: '#000',
+    urlColor: '#093',
+    titleColor: '#666',
+    font: apiHandle.font,
+    fontSize: apiHandle.fontSize
+  };
+  if ('queryColor' in pageStyle && isValidColor(pageStyle.queryColor))
+    safeStyle.queryColor = pageStyle.queryColor;
+  if ('urlColor' in pageStyle && isValidColor(pageStyle.urlColor))
+    safeStyle.urlColor = pageStyle.urlColor;
+  if ('titleColor' in pageStyle && isValidColor(pageStyle.titleColor))
+    safeStyle.titleColor = pageStyle.titleColor;
+  return safeStyle;
+}
+
+/**
+ * Renders a native history suggestion.
+ * @param {Document} resultDoc The suggestion template document.
+ * @param {Object} suggestion The NativeSuggestion to render.
+ * @param {Object} style Checked (not user-set) result style.

[Dan Beam, 2013/04/05 01:07:37]

^ can any of these be null?

[Jered, 2013/04/05 15:30:23]

Done.

+ */
+function updateResult(resultDoc, suggestion, style) {
+  resultDoc.body.style.font = style.fontSize + 'px "' + style.font + '"';

[palmer, 2013/04/05 01:30:32]

Can this function be absolutely sure that |font| and |fontSize| are safe for
interpolation into CSS? (For example, can this function be sure that style is
the return value of |getStyle|?)

Currently, the one caller (|handleMessage|) does send the return value of
|getStyle|. It'd be more robust against future changes to let callers pass
whatever they want, and to have |updateResult| run the style through |getStyle|.

Security pattern: Enforce policy as closely as possible to where it's needed.

[Dan Beam, 2013/04/05 01:38:45]

resultDoc.body.style.fontSize = style.fontSize + 'px';
resultDoc.body.style.fontFamily = style.font;

may help

[Jered, 2013/04/05 15:30:23]

Done.

[Jered, 2013/04/05 15:30:23]

Done.

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

[Dan Beam, 2013/04/05 01:07:37]

\n

[Jered, 2013/04/05 15:30:23]

Done.

+  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);
+      updateResult(window.parent.frames[id].document, suggestion,
+          getStyle(message.data.style || {}));
+      loaded.push(id);
+    }
+    message.source.postMessage(
+        {'loaded': message.data.requestId},

[Dan Beam, 2013/04/05 01:07:37]

nit: this could probably all fit in one line

[Jered, 2013/04/05 15:30:23]

Done.

+        message.origin);
+  }
+}

[Dan Beam, 2013/04/05 01:07:37]

why are these in the global namespace?

[Jered, 2013/04/05 15:30:23]

Let's discuss in other comment, and I'll fix it in the best way everywhere.

+
+window.addEventListener('message', handleMessage, false);