Fixing iframe jank in the local omnibox popup.

chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js

Index: chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
diff --git a/chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js b/chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
index c86515e86f200762e5c27d05268ac0d9d1b4b99c..012be1fedb015d5043433c561f7d3645d24da5c6 100644
--- a/chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
+++ b/chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
@@ -7,10 +7,6 @@
  * features are not supported on the server.
  */
 
-// ==========================================================
-//  Enums.
-// ==========================================================
-
 /**
  * Possible behaviors for navigateContentWindow.
  * @enum {number}
@@ -27,10 +23,6 @@ var WindowOpenDisposition = {
  */
 var MIDDLE_MOUSE_BUTTON = 1;
 
-// =============================================================================
-//                               Util functions
-// =============================================================================
-
 /**
  * The maximum number of suggestions to show.
  * @type {number}
@@ -47,112 +39,85 @@ var MAX_SUGGESTIONS_TO_SHOW = 5;
 var INLINE_SUGGESTION_THRESHOLD = 1200;
 
 /**
- * Suggestion provider type corresponding to a verbatim URL suggestion.
+ * The color code for a query.
  * @type {string}
  * @const
  */
-var VERBATIM_URL_TYPE = 'url-what-you-typed';
+var QUERY_COLOR = '#000';

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

NIT: const instead of var? The internet tells me Chrome supports that.

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

no, don't do that - const is deprected, banned, non-standard, and slow.

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

Not done.

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

Going with dbeam's advice.

 
 /**
- * Suggestion provider type corresponding to a verbatim search suggestion.
+ * The color code for a display URL.
  * @type {string}
  * @const
  */
-var VERBATIM_SEARCH_TYPE = 'search-what-you-typed';
+var URL_COLOR = '#093';
 
 /**
- * The omnibox input value during the last onnativesuggestions event.
+ * The color code for a suggestion title.
  * @type {string}
+ * @const
  */
-var lastInputValue = '';
+var TITLE_COLOR = '#666';
 
 /**
- * The ordered restricted ids of the currently displayed suggestions.  Since the
- * suggestions contain the user's personal data (browser history) the searchBox
- * API embeds the content of the suggestion in a shadow dom, and assigns a
- * random restricted id to each suggestion which is accessible to the JS.
- * @type {Array.<number>}
+ * A top position which is off-screen.
+ * @type {string}
+ * @const
  */
-
-var restrictedIds = [];
+var OFF_SCREEN = '-1000px';
 
 /**
- * The index of the currently selected suggestion or -1 if none are selected.
- * @type {number}
+ * The expected origin of a suggestion iframe.
+ * @type {string}
+ * @const
  */
-var selectedIndex = -1;
+var SUGGESTION_ORIGIN = 'chrome-search://suggestion';
 
 /**
- * Shortcut for document.getElementById.
- * @param {string} id of the element.
- * @return {HTMLElement} with the id.
+ * Suggestion provider type corresponding to a verbatim URL suggestion.
+ * @type {string}
+ * @const
  */
-function $(id) {
-  return document.getElementById(id);
-}
+var VERBATIM_URL_TYPE = 'url-what-you-typed';
 
 /**
- * Displays a suggestion.
- * @param {Object} suggestion The suggestion to render.
- * @param {HTMLElement} box The html element to add the suggestion to.
- * @param {boolean} select True to select the selection.
+ * Suggestion provider type corresponding to a verbatim search suggestion.
+ * @type {string}
+ * @const
  */
-function addSuggestionToBox(suggestion, box, select) {
-  var suggestionDiv = document.createElement('div');
-  suggestionDiv.classList.add('suggestion');
-  suggestionDiv.classList.toggle('selected', select);
-  suggestionDiv.classList.toggle('search', suggestion.is_search);
-
-  if (suggestion.destination_url) {  // iframes.
-    var suggestionIframe = document.createElement('iframe');
-    suggestionIframe.className = 'contents';
-    suggestionIframe.src = suggestion.destination_url;
-    suggestionIframe.id = suggestion.rid;
-    suggestionDiv.appendChild(suggestionIframe);
-  } else {
-    var contentsContainer = document.createElement('div');
-    var contents = suggestion.combinedNode;
-    contents.classList.add('contents');
-    contentsContainer.appendChild(contents);
-    suggestionDiv.appendChild(contentsContainer);
-    suggestionDiv.onclick = function(event) {
-      handleSuggestionClick(suggestion.rid, event.button);
-    };
-  }
+var VERBATIM_SEARCH_TYPE = 'search-what-you-typed';
 
-  restrictedIds.push(suggestion.rid);
-  box.appendChild(suggestionDiv);
-}
+/**
+ * The displayed suggestions.
+ * @type {SuggestionsBox}
+ */
+var active;
 
 /**
- * Renders the input suggestions.
- * @param {Array} nativeSuggestions An array of native suggestions to render.
+ * The suggestions being rendered.
+ * @type {SuggestionsBox}
  */
-function renderSuggestions(nativeSuggestions) {
-  var box = document.createElement('div');
-  box.id = 'suggestionsBox';
-  $('suggestions-box-container').appendChild(box);
+var pending;
 
-  for (var i = 0, length = nativeSuggestions.length;
-       i < Math.min(MAX_SUGGESTIONS_TO_SHOW, length); ++i) {
-    addSuggestionToBox(nativeSuggestions[i], box, i == selectedIndex);
-  }
-}
+/**
+ * A pool of iframes to display suggestions.
+ * @type {IframePool}
+ */
+var iframePool;
 
 /**
- * Clears the suggestions being displayed.
+ * A serial number for the next suggestions rendered.
+ * @type {number}
  */
-function clearSuggestions() {
-  $('suggestions-box-container').innerHTML = '';
-  restrictedIds = [];
-  selectedIndex = -1;
-}
+var nextRequestId = 0;
 
 /**
- * @return {number} The height of the dropdown.
+ * Shortcut for document.querySelector.
+ * @param {string} selector A selector to query the desired element.
+ * @return {HTMLElement} matching selector.
  */
-function getDropdownHeight() {
-  return $('suggestions-box-container').offsetHeight;
+function $(selector) {

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

please change this to $$(), IMO

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

Done.

[Dan Beam, 2013/04/05 21:58:49]

oh, I'm sorry, I meant $qs(), $$() usually means more than one will be returned.
 sorry, I thought I updated this.

[Jered, 2013/04/05 22:05:03]

Done.

+  return document.querySelector(selector);
 }
 
 /**
@@ -162,53 +127,480 @@ function getDropdownHeight() {
  */
 function shouldSelectSuggestion(suggestion, inVerbatimMode) {
   var isVerbatimUrl = suggestion.type == VERBATIM_URL_TYPE;
-  var inlinableSuggestion = suggestion.type != VERBATIM_SEARCH_TYPE &&
+  var inlineableSuggestion = suggestion.type != VERBATIM_SEARCH_TYPE &&
       suggestion.rankingData.relevance > INLINE_SUGGESTION_THRESHOLD;
   // Verbatim URLs should always be selected. Otherwise, select suggestions
   // with a high enough score unless we are in verbatim mode (e.g. backspacing
   // away).
-  return isVerbatimUrl || (!inVerbatimMode && inlinableSuggestion);
+  return isVerbatimUrl || (!inVerbatimMode && inlineableSuggestion);
 }
 
 /**
- * Updates selectedIndex, bounding it between -1 and the total number of
- * of suggestions - 1 (looping as necessary), and selects the corresponding
- * suggestion.
- * @param {boolean} increment True to increment the selected suggestion, false
- *     to decrement.
+ * Extract the desired navigation behavior from a click button.
+ * @param {number} button The Event#button property of a click event.
+ * @return {WindowOpenDisposition} The desired behavior for
+ *     navigateContentWindow.
  */
-function updateSelectedSuggestion(increment) {
-  var numSuggestions = restrictedIds.length;
-  if (!numSuggestions)
-    return;
+function getDispositionFromClickButton(button) {
+  if (button == MIDDLE_MOUSE_BUTTON)
+    return WindowOpenDisposition.NEW_BACKGROUND_TAB;
+  return WindowOpenDisposition.CURRENT_TAB;
+}
+
+
+/**
+ * Manages a pool of chrome-search suggestion result iframes.
+ * @constructor
+ */
+function IframePool() {
+  /**
+   * HTML iframe elements.
+   * @type {Array.<Element>}
+   * @private
+   */
+  this.iframes_ = [];
+}
+
+/**
+ * Initializes the pool with blank result template iframes, positioned off
+ * screen.
+ */
+IframePool.prototype.init = function() {
+  for (var i = 0; i < 2 * MAX_SUGGESTIONS_TO_SHOW; i++) {
+    var iframe = document.createElement('iframe');
+    iframe.className = 'contents';
+    iframe.id = 'suggestion-text-' + i;

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

^ what's the point of this?

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

ids are passed via postMessage() to the loader iframe to tell it which frames
from parent.frames to load.

+    iframe.src = 'chrome-search://suggestion/result.html';
+    iframe.style.top = OFF_SCREEN;
+    iframe.addEventListener('mouseover', (function(id) {

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

iframe.addEventListener('mouseover', function(e) {
  if (active)
    active.hover(e.currentTarget.id);
});

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

why do you need to do hover/mouseout in JS?

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

Done.

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

The element hovered is an absolutely positioned* iframe but we want to draw the
hover with a div positioned behind it**. The iframe has no fixed dom
relationship to the hover div, so is not really amenable to CSS styling.

* The iframes must be absolutely positioned because removing an iframe from the
dom and adding it back reloads it, which makes the load spinner flash with every
keystroke.
** An alternative would be to make iframes large enough to encompass the entire
hover region and add a body:hover in the iframes, but then they'd also have to
know about selection so they don't show a hover when selected with the keyboard
(which is not quite the same as being focused, in the usual sense), which would
involve more postMessages, so we'd also need Javascript for that approach.

+      active && active.hover(id);

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

s/active &&/if (active)/

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

Done.

+    }).bind(undefined, iframe.id), false);
+    iframe.addEventListener('mouseout', (function(id) {
+      active && active.unhover(id);
+    }).bind(undefined, iframe.id), false);
+    document.body.appendChild(iframe);
+    this.iframes_[i] = iframe;

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

this.iframes_.push(iframe);

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

Done.

+  }
+};
+
+/**
+ * Retrieves a free suggestion iframe from the pool.
+ * @return {Element} An iframe suitable for holding a suggestion.
+ */
+IframePool.prototype.get = function() {

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

this seems like a bad name (seems like it's a const getter or something), can
you change to .reserve() or .acquire() or something?

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

Done.

+  return this.iframes_.pop();
+};
+
+/**
+ * Releases a suggestion iframe back into the pool.
+ * @param {Element} iframe The iframe to return to the pool.
+ */
+IframePool.prototype.release = function(iframe) {
+  this.iframes_.push(iframe);
+  iframe.style.top = OFF_SCREEN;

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

why does this need to be offscreen rather than [hidden]?

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

No particular reason. I think visibility: hidden would probably work ok. Do you
have a strong preference, given the element must be repositioned to show up in
the proper place, anyway?

[Dan Beam, 2013/04/05 22:17:26]

if it's display:none; the page renders more quickly (minutely?) as it's not in
the render tree.

+};
+
+
+/**
+ * An individual suggestion.
+ * @param {Object} data Autocomplete fields for this suggestion.
+ * @constructor
+ */
+function Suggestion(data) {
+  /**
+   * Autocomplete fields for this suggestion.
+   * @type {Object}

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

should this be !Object? (and/or should you be doing assert(data))?

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

Done. Where is assert() defined?

[Dan Beam, 2013/04/05 22:17:26]

ui/webui/resources/js/util.js

[Jered, 2013/04/05 22:28:26]

Sorry, I meant how do I get at that code from here? I tried adding an assert()
but the method is undefined. The Javascript in this CL is maybe a bit unusual
because it runs in its own special origin.

+   * @private
+   */
+  this.data_ = data;
+
+  /**
+   * A placeholder element where the iframe for this suggestion will be
+   * positioned.
+   * @type {Element}
+   * @private
+   */
+  this.div_;

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

unless you guys are compiling this, this is pretty useless...

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

Force of habit. Removed.

+
+  /**
+   * An iframe showing the contents of this suggestion.
+   * @type {Element}
+   * @private
+   */
+  this.iframe_;
+}
+
+/**
+ * Releases the iframe reserved for this suggestion.
+ */
+Suggestion.prototype.destroy = function() {
+  if (this.iframe_)
+    iframePool.release(this.iframe_);
+};
+
+/**
+ * Creates and appends the placeholder div for this suggestion to box.
+ * @param {Element} box A suggestions box.
+ * @param {boolean} selected True if the suggestion should be drawn as selected
+ *     and false otherwise.
+ */
+Suggestion.prototype.appendToBox = function(box, selected) {
+  var div = document.createElement('div');
+  div.classList.add('suggestion');
+  div.classList.toggle('selected', selected);
+  div.classList.toggle('search', this.data_.is_search);
+  box.appendChild(div);
+  this.div_ = div;
+};
+
+/**
+ * Sets the iframe element for this suggestion.
+ * @param {Element} iframe The iframe.
+ */
+Suggestion.prototype.setIframe = function(iframe) {

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

any reason these aren't public members or ES5 getter/setters?

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

Just habit. What would be most consistent with webui style?

[Dan Beam, 2013/04/05 22:17:26]

well, normal webui style would be:

Suggestion.prototype = {
  set iframe(iframe) { this.iframe_ = iframe; },
};

but you're using more Closurey style, so I guess it'd be

Suggestion.prototype.__defineSetter__('iframe', function(iframe) {
  this.iframe_ = iframe;
});

[Jered, 2013/04/05 22:28:26]

What style do you prefer? I'm happy to write webui style rather than closure
style as I think the latter is awfully verbose, but have just gotten used to it.

+  this.iframe_ = iframe;
+};
+
+/**
+ * @return {number} The restricted id associated with this suggestion.
+ */
+Suggestion.prototype.getRestrictedId = function() {
+  return this.data_.rid;
+};
+
+/**
+ * Repositions the suggestion iframe to align with its expected dropdown
+ * position.
+ * @param {boolean} isRtl True if rendering right-to-left and false if not.
+ * @param {number} startMargin Leading space before suggestion.
+ * @param {number} totalMargin Total non-content space on suggestion line.

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

Enforce that |startMargin| and |totalMargin| are numbers, since they get
interpolated into CSS?

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

It looks bizarre to me, but I've done this.

+ */
+Suggestion.prototype.reposition = function(isRtl, startMargin, totalMargin) {
+  // Add in the expected parent offset and the top margin.
+  this.iframe_.style.top = (this.div_.offsetTop + 4) + 'px';

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

nit: unnecessary ()

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

Done.

+  this.iframe_.style[isRtl ? 'right' : 'left'] = startMargin + 'px';

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

it's arguable that you should just style both things, i.e.

  this.iframe_.style.left = startMargin + 'px';  // will work in LTR.
  this.iframe_.style.right = startMargin + 'px';  // overrides in RTL.

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

Huh? How would that work and take into account apiHandle.isRtl?

[Dan Beam, 2013/04/05 22:17:26]

this works because you're setting the [dir]

[Jered, 2013/04/05 22:28:26]

I'm setting the dir but not on the iframe or on a parent element.

[Dan Beam, 2013/04/05 22:32:47]

ah, ok, stick with your original plan then

[Jered, 2013/04/05 23:43:41]

ack

+  this.iframe_.style.width = '-webkit-calc(100% - ' +
+      (totalMargin + 26) + 'px)';
+};
+
+/**
+ * Updates the suggestion selection state.
+ * @param {boolean} selected True if drawn selected or false if not.
+ */
+Suggestion.prototype.select = function(selected) {
+  this.div_.classList.toggle('selected', selected);
+};
+
+/**
+ * Updates the suggestion hover state.
+ * @param {boolean} hovered True if drawn hovered or false if not.
+ */
+Suggestion.prototype.hover = function(hovered) {
+  this.div_.classList.toggle('hovered', hovered);
+};
+
+/**
+ * @param {Window} iframeWindow The content window of an iframe.
+ * @return {boolean} True if this suggestion's iframe has the specified window
+ *     and false if not.
+ */
+Suggestion.prototype.hasIframeWindow = function(iframeWindow) {
+  return this.iframe_.contentWindow == iframeWindow;
+};
+
+/**
+ * @param {string} id An element id.
+ * @return {boolean} True if this suggestion's iframe has the specified id and
+ *     false if not.
+ */
+Suggestion.prototype.hasIframeId = function(id) {
+  return this.iframe_.id == id;
+};
+
+
+/**
+ * Displays a suggestions box.
+ * @param {string} inputValue The user text that prompted these suggestions.
+ * @param {Array.<Object>} suggestionData Suggestion data to display.

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

can this be null or have null objects?

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

Done.

+ * @param {number} selectedIndex The index of the suggestion selected.
+ * @constructor
+ */
+function SuggestionsBox(inputValue, suggestionData, selectedIndex) {
+  /**
+   * The user text that prompted these suggestions.
+   * @type {string}
+   * @private
+   */
+  this.inputValue_ = inputValue;
+
+  /**
+   * The index of the suggestion currently selected, whether by default or
+   * because the user arrowed down to it.
+   * @type {number}
+   * @private
+   */
+  this.selectedIndex_ = selectedIndex;
+
+  /**
+   * The index of the suggestion currently under the mouse pointer.
+   * @type {number}
+   * @private
+   */
+  this.hoveredIndex_ = -1;
+
+  /**
+   * A stamp to distinguish this suggestions box from others.
+   * @type {number}
+   * @private
+   */
+  this.requestId_ = nextRequestId++;
+
+  /**
+   * The ordered suggestions this box is displaying.
+   * @type {Array.<Suggestion>}
+   * @private
+   */
+  this.suggestions_ = [];
+  for (var i = 0; i < suggestionData.length; i++)
+    this.suggestions_[i] = new Suggestion(suggestionData[i]);

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

nit: curlies, .push()

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

Done.

+
+  /**
+   * An embedded search API handle.
+   * @type {Object}
+   * @private
+   */
+  this.apiHandle_ = getApiObjectHandle();
+
+  /**
+   * The container for this suggestions box. div.pending-container if inactive
+   * and div.active-container if active.
+   * @type {Element}
+   * @private
+   */
+  this.container_ = $('.pending-container');

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

assert(this.container_); maybe?

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

Please see above.

+}
 
-  var oldSelection = $('suggestionsBox').querySelector('.selected');
+/**
+ * Releases suggestion iframes and ignores any load done message for the
+ * current suggestions.
+ */
+SuggestionsBox.prototype.destroy = function() {
+  for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++)

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

can suggestions be null? is so this could skip some suggestions from being
destroy()ed

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

No.

+    suggestion.destroy();

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

nit: curlies

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

Done.

+  this.responseId = -1;
+};
+
+/**
+ * Starts rendering new suggestions.
+ */
+SuggestionsBox.prototype.loadSuggestions = function() {
+  // Create a placeholder DOM in the invisible container.
+  this.container_.innerHTML = '';
+  var box = document.createElement('div');
+  box.className = 'suggestionsBox';

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

suggestions-box

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

Done.

+  this.container_.appendChild(box);
+  var iframesToLoad = {};
+  for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) {
+    suggestion.appendToBox(box, i == this.selectedIndex_);
+    var iframe = iframePool.get();
+    suggestion.setIframe(iframe);
+    iframesToLoad[iframe.id] = suggestion.getRestrictedId();
+  }
+  // Ask the loader iframe to populate the iframes just reserved.
+  var loadRequest = {
+    'load': iframesToLoad,
+    'requestId': this.requestId_,
+    'style': {
+      'queryColor': QUERY_COLOR,
+      'urlColor': URL_COLOR,
+      'titleColor': TITLE_COLOR
+    }
+  };
+  $('#suggestion-loader').contentWindow.postMessage(loadRequest,
+      SUGGESTION_ORIGIN);
+};
+
+/**
+ * Returns whether the response is for the suggestions in this box.

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

^ doesn't seem necessary

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

Done.

+ * @param {number} responseId The id of a request that just finished rendering.
+ * @return {boolean} True if the request is for the suggestions in this box and

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

Whether the request is for the suggestions in this box

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

Done.

+ *     false if not.
+ */
+SuggestionsBox.prototype.isResponseCurrent = function(responseId) {
+  return responseId == this.requestId_;
+};
+
+/**
+ * Moves suggestion iframes into position.
+ */
+SuggestionsBox.prototype.repositionSuggestions = function() {
+  // Note: This may be called before margins are ready. In that case,
+  // suggestion iframes will initially be too large and then size down
+  // onresize.
+  var startMargin = this.apiHandle_.startMargin;
+  var totalMargin = window.innerWidth - this.apiHandle_.width;
+  var isRtl = this.apiHandle_.isRtl;
+  for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++)

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

why are you doing this everywhere vs.

  suggestions.forEach(function(suggestion, i) {

?

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

Is forEach more appropriate for webui Javascript? I don't see this a lot
elsewhere in google Javascript. I'm in the habit of writing loops this way for
collections that don't contain nulls due to [go/js-style "Iterating over Node
Lists"].

[Dan Beam, 2013/04/05 22:17:26]

this is an array, not a node list.

[Jered, 2013/04/05 22:28:26]

What style do you prefer?

[Dan Beam, 2013/04/05 22:32:47]

forEach() if I require a closure to a reference (i.e. trying to us something in
an event handler and want to make sure it stays the same), otherwise:

  for (var i = 0; i < this.suggestions_.length; ++i) {
    this.suggestions_[i].doSomething();
  }

[Jered, 2013/04/05 23:43:41]

Ok, I've opted for the latter style for loop in this file.

+    suggestion.reposition(isRtl, startMargin, totalMargin);

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

same nit re: curlies

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

Done.

+};
+
+/**
+ * Selects the suggestion before the current selection.
+ */
+SuggestionsBox.prototype.selectPrevious = function() {
+  var numSuggestions = this.suggestions_.length;
+  this.selectedIndex_--;
+  if (this.selectedIndex_ < -1)
+    this.selectedIndex_ = -1;

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

nit: \n

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

Done.

+  this.redrawSelection_();
+  this.redrawHover_();
+};
+
+/**
+ * Selects the suggestion after the current selection.
+ */
+SuggestionsBox.prototype.selectNext = function() {
+  var numSuggestions = this.suggestions_.length;
+  this.selectedIndex_++;
+  if (this.selectedIndex_ > numSuggestions - 1)
+    this.selectedIndex_ = numSuggestions - 1;
+  this.redrawSelection_();
+  this.redrawHover_();

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

nit: seems this could share code with selectPrevious

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

Done.

+};
+
+/**
+ * Redraws the selected suggestion.
+ * @private
+ */
+SuggestionsBox.prototype.redrawSelection_ = function() {
+  var oldSelection = this.container_.querySelector('.selected');
   if (oldSelection)
     oldSelection.classList.remove('selected');
+  if (this.selectedIndex_ == -1) {
+    this.apiHandle_.setValue(this.inputValue_);
+  } else {
+    this.suggestions_[this.selectedIndex_].select(true);
+    this.apiHandle_.setRestrictedValue(
+        this.suggestions_[this.selectedIndex_].getRestrictedId());
+  }
+};
 
-  if (increment)
-    selectedIndex = ++selectedIndex > numSuggestions - 1 ? -1 : selectedIndex;
-  else
-    selectedIndex = --selectedIndex < -1 ? numSuggestions - 1 : selectedIndex;
-  var apiHandle = getApiObjectHandle();
-  if (selectedIndex == -1) {
-    apiHandle.setValue(lastInputValue);
+/**
+ * Returns the restricted id of the iframe clicked.
+ * @param {Window} iframeWindow The window of the iframe that was clicked.

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

can Window be null?

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

Done.

+ * @return {number?} The restricted id clicked or null if none.

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

?number I think is more common

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

Done.

+ */
+SuggestionsBox.prototype.getClickTarget = function(iframeWindow) {
+  for (var i = 0, suggestion; suggestion = this.suggestions_[i]; ++i)

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

curlies

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

Done.

+    if (suggestion.hasIframeWindow(iframeWindow))
+      return suggestion.getRestrictedId();
+  return null;
+};
+
+/**
+ * Called when the user hovers on the specified iframe.
+ * @param {string} iframeId The id of the iframe hovered.
+ */
+SuggestionsBox.prototype.hover = function(iframeId) {
+  this.hoveredIndex_ = -1;
+  for (var i = 0, suggestion; suggestion = this.suggestions_[i]; ++i) {
+    if (suggestion.hasIframeId(iframeId)) {
+      this.hoveredIndex_ = i;
+      break;
+    }
+  }
+  this.redrawHover_();
+};
+
+/**
+ * Called when the user unhovers the specified iframe.
+ * @param {string} iframeId The id of the iframe hovered.
+ */
+SuggestionsBox.prototype.unhover = function(iframeId) {
+  if (this.suggestions_[this.hoveredIndex_] &&
+      this.suggestions_[this.hoveredIndex_].hasIframeId(iframeId)) {
+    this.hoveredIndex_ = -1;
+    this.redrawHover_();

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

this.clearHover();

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

Done.

+  }
+};
+
+/**
+ * Clears the current hover.
+ */
+SuggestionsBox.prototype.clearHover = function() {
+  this.hoveredIndex_ = -1;
+  this.redrawHover_();
+};
+
+/**
+ * Redraws the mouse hover background.
+ * @private
+ */
+SuggestionsBox.prototype.redrawHover_ = function() {
+  var oldHover = this.container_.querySelector('.hovered');
+  if (oldHover)
+    oldHover.classList.remove('hovered');
+  if (this.hoveredIndex_ != -1 && this.hoveredIndex_ != this.selectedIndex_)
+    this.suggestions_[this.hoveredIndex_].hover(true);
+};
+
+/**
+ * Marks the suggestions container as active.
+ */
+SuggestionsBox.prototype.activate = function() {
+  this.container_.className = 'active-container';
+};
+
+/**
+ * Marks the suggestions container as inactive.
+ */
+SuggestionsBox.prototype.deactivate = function() {
+  this.container_.className = 'pending-container';
+  this.container_.innerHTML = '';
+};
+
+/**
+ * @return {number} The height of the suggestions container.
+ */
+SuggestionsBox.prototype.getHeight = function() {
+  return this.container_.offsetHeight;
+};
+
+/**
+ * Clears the currently active suggestions and shows pending suggestions.
+ */
+function makePendingSuggestionsActive() {
+  if (active) {
+    active.deactivate();
+    active.destroy();
   } else {
-    var newSelection = $('suggestionsBox').querySelector(
-        '.suggestion:nth-of-type(' + (selectedIndex + 1) + ')');
-    newSelection.classList.add('selected');
-    apiHandle.setRestrictedValue(restrictedIds[selectedIndex]);
+    // Initially there will be no active suggestions, but we still want to use
+    // div.active-container to load the next suggestions.
+    $('.active-container').className = 'pending-container';
   }
+  pending.activate();
+  active = pending;
+  pending = null;
+  active.repositionSuggestions();
+  getApiObjectHandle().showOverlay(active.getHeight());
 }
 
-// =============================================================================
-//                             Handlers / API stuff
-// =============================================================================
+/**
+ * Hides the active suggestions box.
+ */
+function hideActiveSuggestions() {
+  getApiObjectHandle().showOverlay(0);
+  if (active) {
+    $('.active-container').innerHTML = '';
+    active.destroy();
+  }
+  active = null;
+}
 
 /**
  * @return {Object} the handle to the searchBox API.
  */
- function getApiObjectHandle() {
+function getApiObjectHandle() {
   if (window.cideb)
     return window.cideb;
   if (window.navigator && window.navigator.embeddedSearch &&
@@ -224,22 +616,28 @@ function updateSelectedSuggestion(increment) {
  * Updates suggestions in response to a onchange or onnativesuggestions call.
  */
 function updateSuggestions() {
+  if (pending) {
+    pending.destroy();
+    pending = null;
+  }
   var apiHandle = getApiObjectHandle();
-  lastInputValue = apiHandle.value;
-
-  clearSuggestions();
-  var nativeSuggestions = apiHandle.nativeSuggestions;
-  if (nativeSuggestions.length) {
-    nativeSuggestions.sort(function(a, b) {
+  var inputValue = apiHandle.value;
+  var suggestions = apiHandle.nativeSuggestions;
+  if (!inputValue || !suggestions.length) {
+    hideActiveSuggestions();
+    return;
+  }
+  if (suggestions.length) {
+    suggestions.sort(function(a, b) {
       return b.rankingData.relevance - a.rankingData.relevance;
     });
-    if (shouldSelectSuggestion(nativeSuggestions[0], apiHandle.verbatim))
+    var selectedIndex = -1;
+    if (shouldSelectSuggestion(suggestions[0], apiHandle.verbatim))
       selectedIndex = 0;
-    renderSuggestions(nativeSuggestions);
+    pending = new SuggestionsBox(inputValue,
+        suggestions.slice(0, MAX_SUGGESTIONS_TO_SHOW), selectedIndex);
+    pending.loadSuggestions();
   }
-
-  var height = getDropdownHeight();
-  apiHandle.showOverlay(height);
 }
 
 /**
@@ -262,91 +660,76 @@ function appendSuggestionStyles() {
           (window.innerWidth - apiHandle.width - startMargin) + 'px;' +
       '  font: ' + apiHandle.fontSize + 'px "' + apiHandle.font + '";' +
       '}';
-  document.querySelector('head').appendChild(style);
+  $('head').appendChild(style);

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

$qs() would be useful here as if you're looking at other webui code this would
mean #head, not <head>

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

Is $$ ok?

+  if (active)
+    active.repositionSuggestions();
   window.removeEventListener('resize', appendSuggestionStyles);
 }
 
 /**
- * Extract the desired navigation behavior from a click button.
- * @param {number} button The Event#button property of a click event.
- * @return {WindowOpenDisposition} The desired behavior for
- *     navigateContentWindow.
- */
-function getDispositionFromClickButton(button) {
-  if (button == MIDDLE_MOUSE_BUTTON)
-    return WindowOpenDisposition.NEW_BACKGROUND_TAB;
-  return WindowOpenDisposition.CURRENT_TAB;
-}
-
-/**
- * Handles suggestion clicks.
- * @param {number} restrictedId The restricted id of the suggestion being
- *     clicked.
- * @param {number} button The Event#button property of a click event.
- *
- */
-function handleSuggestionClick(restrictedId, button) {
-  clearSuggestions();
-  getApiObjectHandle().navigateContentWindow(
-      restrictedId, getDispositionFromClickButton(button));
-}
-
-/**
- * chrome.searchBox.onkeypress implementation.
+ * Makes keys navigate through suggestions.
  * @param {Object} e The key being pressed.
  */
 function handleKeyPress(e) {
-  switch (e.keyCode) {
-    case 38:  // Up arrow
-      updateSelectedSuggestion(false);
-      break;
-    case 40:  // Down arrow
-      updateSelectedSuggestion(true);
-      break;
+  if (active) {

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

nit:

  if (!active)
    return;

  switch (e.keyIdentifier) {
    case 'Up':
      ...
    case 'Down':
  ...

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

Done.

[Jered, 2013/04/05 15:51:39]

Oh, bummer, this does not work. e has only a keyCode member. I've revised this
to use keyCode with named constants.

[Dan Beam, 2013/04/05 22:17:26]

strange, that works for me right now.

[Jered, 2013/04/05 22:28:26]

This keypress is actually some kind of doctored up mouse event I think, it comes
from Instant and not from wherever those normally come from. It's got just
keyCode.

+    switch (e.keyCode) {
+      case 38:  // Up arrow
+        active.selectPrevious();
+        break;
+      case 40:  // Down arrow
+        active.selectNext();
+        break;
+    }
   }
 }
 
 /**
- * Handles the postMessage calls from the result iframes.
- * @param {Object} message The message containing details of clicks the iframes.
+ * Handles postMessage calls from suggestion iframes.
+ * @param {Object} message A notification that all iframes are done loading or
+ *     that an iframe was clicked.
  */
 function handleMessage(message) {
-  if (message.origin != 'null' || !message.data ||
-      message.data.eventType != 'click') {
+  if (message.origin != SUGGESTION_ORIGIN)
     return;
-  }
 
-  var iframes = document.getElementsByClassName('contents');
-  for (var i = 0; i < iframes.length; ++i) {
-    if (iframes[i].contentWindow == message.source) {
-      handleSuggestionClick(parseInt(iframes[i].id, 10),
-                            message.data.button);
-      break;
+  if ('loaded' in message.data) {
+    if (pending && pending.isResponseCurrent(message.data.loaded)) {
+      makePendingSuggestionsActive();
+    }
+  } else if ('click' in message.data) {

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

} else if ('click' in message.data && active) {

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

My intent was to avoid ambiguity for future messages, so that a message with
.click is always interpreted as this kind of message regardless of whether we're
able to handle it.

+    if (active) {
+      var restrictedId = active.getClickTarget(message.source);
+      if (restrictedId != null) {

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

if (restrictedId) (if possible)

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

I think that 0 is a valid restrictedId.

+        hideActiveSuggestions();
+        getApiObjectHandle().navigateContentWindow(restrictedId,
+            getDispositionFromClickButton(message.data.click));
+      }
     }
   }
 }
 
 /**
- * chrome.searchBox.embeddedSearch.onsubmit implementation.
- */
-function onSubmit() {
-}
-
-/**
- * Sets up the searchBox API.
+ * Sets up the embedded search API and creates suggestion iframes.
  */
-function setUpApi() {
+function init() {

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

^ can you take these out of the global namespace?

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

By these, do you mean everything in this file? What namespace should I use? I'm
not familiar with webui conventions.

[Dan Beam, 2013/04/05 22:17:26]

if it's not necessary to access them from anywhere else, just an anonymous
namespace, i.e.

(function() {
  ...
}());

or if they do need to be exported,

cr.define('namespace_unix_hacker', function() {
  ...
  return {PublishedGlobal: PublishedGlobal};
});

[Jered, 2013/04/05 23:43:41]

Done.

+  iframePool = new IframePool();
+  iframePool.init();
   var apiHandle = getApiObjectHandle();
   apiHandle.onnativesuggestions = updateSuggestions;
   apiHandle.onchange = updateSuggestions;
   apiHandle.onkeypress = handleKeyPress;
-  apiHandle.onsubmit = onSubmit;
-  $('suggestions-box-container').dir = apiHandle.rtl ? 'rtl' : 'ltr';
+  // Instant checks for this handler to be bound.
+  apiHandle.onsubmit = function() {};
+  $('.active-container').dir = apiHandle.rtl ? 'rtl' : 'ltr';
+  $('.pending-container').dir = apiHandle.rtl ? 'rtl' : 'ltr';
   // Delay adding these styles until the window width is available.
   window.addEventListener('resize', appendSuggestionStyles);
   if (apiHandle.nativeSuggestions.length)
-    handleNativeSuggestions();
+    updateSuggestions();
 }
 
-document.addEventListener('DOMContentLoaded', setUpApi);
+document.addEventListener('DOMContentLoaded', init);
 window.addEventListener('message', handleMessage, false);
+window.addEventListener('blur', function() {
+  active && active.clearHover();

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

if (active)

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

Done.

+}, false);