Try and improve result coalescing/display in the omnibox popup.

This eliminates the delay timer in the popup view, and also eliminates the coalesce timer in the controller.  Instead, we simply coalesce until we're done, we have at least as many results as we're already showing, or the "maximum delay timeout" (300 ms) fires, indicating we've gone too long without updating.

Additionally, in order to be more responsive when typing rapidly, the controller updates observers immediately with the available results from a previous query if one is still running when a new query is started.  While in theory this seems like it might produce flicker, in practice _not_ having it also results in flicker (just less-predictable flicker) since the 300 ms timeout starts kicking in at random times relative to when new keys are pressed.

I also fixed a few small problems with leaving 1-pixel high white rows at the bottom of the popup during rapid typing (which weren't visible before this change since the popup would never shrink during rapid typing).

After eliminating the timeout in the popup view, I was able to refactor the code to be shorter since a few members and helper functions could all be inlined.  Then I added some long comments and made things not much shorter after all :/.  I also changed two other (self-contained) unrelated spots in the popup to be shorter.

Please patch this in locally and try how it feels.  Things to test with this change vs. the old code vs. the old, old (original omnibox) code:
* Type one letter at a time with long pauses in between; see how flickery the popup is
* Type one letter (e.g. "a") and then type rapidly for a while; see how responsive the popup is
* Type words like "amazon", "compusa" and "comcast" at various different speeds and observe the flicker vs. responsiveness tradeoff
* Type or paste some long series of letters (that default to searching), then rapidly press and release the ctrl key

My hope is that this hits a good balance (it's very difficult to be both flicker-free and responsive, I view the previous two sets of code as being off first one side of the scale and then the other).  Possible tweaks include the animation tweening mechanism and timing (I experimented with various different speeds and linear tweening, nothing felt significantly better to me but my machine sucks w.r.t. animation quality) and tweaking the controller "max timeout" value and notification behavior upon starting a new query (I tried notifying only if two keys had been typed since the last notification, it didn't feel better).

BUG=none
TEST=see above
Review URL: http://codereview.chromium.org/149659

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@21322 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 66 insertions, 144 deletions

diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc
index 7b7b1c2..0288677 100644
--- a/chrome/browser/autocomplete/autocomplete.cc
+++ b/chrome/browser/autocomplete/autocomplete.cc
@@ -617,18 +617,13 @@ void AutocompleteResult::Validate() const {
 const int AutocompleteController::kNoItemSelected = -1;
 
 namespace {
-// The amount of time we'll wait after a provider returns before updating,
-// in order to coalesce results.
-const int kResultCoalesceMs = 100;
-
 // The maximum time we'll allow the results to go without updating to the
 // latest set.
 const int kResultUpdateMaxDelayMs = 300;
 };
 
 AutocompleteController::AutocompleteController(Profile* profile)
-    : update_pending_(false),
-      done_(true) {
+    : done_(true) {
   providers_.push_back(new SearchProvider(this, profile));
   providers_.push_back(new HistoryURLProvider(this, profile));
   providers_.push_back(new KeywordProvider(this, profile));
@@ -672,11 +667,10 @@ void AutocompleteController::Start(const std::wstring& text,
   const bool minimal_changes = (input_.text() == old_input_text) &&
       (input_.synchronous_only() == old_synchronous_only);
 
-  // If we're starting a brand new query, stop caring about any old query.
-  if (!minimal_changes && !done_) {
-    update_pending_ = false;
-    coalesce_timer_.Stop();
-  }
+  // If we're starting a brand new query, send the previous results to the
+  // observers.
+  if (!minimal_changes && !done_)
+    CommitResult();
 
   // Start the new query.
   for (ACProviders::iterator i(providers_.begin()); i != providers_.end();
@@ -696,11 +690,9 @@ void AutocompleteController::Stop(bool clear_result) {
   }
 
   done_ = true;
-  update_pending_ = false;
   if (clear_result)
     result_.Reset();
   latest_result_.CopyFrom(result_);
-  coalesce_timer_.Stop();
   max_delay_timer_.Stop();
 }
 
@@ -711,8 +703,7 @@ void AutocompleteController::DeleteMatch(const AutocompleteMatch& match) {
 
   // Notify observers of this change immediately, so the UI feels responsive to
   // the user's action.
-  if (update_pending_)
-    CommitResult();
+  CommitResult();
 }
 
 void AutocompleteController::OnProviderUpdate(bool updated_matches) {
@@ -772,23 +763,13 @@ void AutocompleteController::UpdateLatestResult(bool is_synchronous_pass) {
         Details<const AutocompleteResult>(&latest_result_));
   }
 
-  if (done_) {
+  if (done_ || (latest_result_.size() >= result_.size()))
     CommitResult();
-  } else if (!update_pending_) {
-    // Coalesce the results for the next kPopupCoalesceMs milliseconds.
-    update_pending_ = true;
-    coalesce_timer_.Stop();
-    coalesce_timer_.Start(TimeDelta::FromMilliseconds(kResultCoalesceMs), this,
-                          &AutocompleteController::CommitResult);
-  }
 }
 
 void AutocompleteController::CommitResult() {
   // The max update interval timer either needs to be reset (if more updates
-  // are to come) or stopped (when we're done with the query).  The coalesce
-  // timer should always just be stopped.
-  update_pending_ = false;
-  coalesce_timer_.Stop();
+  // are to come) or stopped (when we're done with the query).
   if (done_)
     max_delay_timer_.Stop();
   else
diff --git a/chrome/browser/autocomplete/autocomplete.h b/chrome/browser/autocomplete/autocomplete.h
index 9d566fc..09ecf8f 100644
--- a/chrome/browser/autocomplete/autocomplete.h
+++ b/chrome/browser/autocomplete/autocomplete.h
@@ -707,7 +707,6 @@ class AutocompleteController : public ACProviderListener {
   explicit AutocompleteController(const ACProviders& providers)
       : providers_(providers),
         history_contents_provider_(NULL),
-        update_pending_(false),
         done_(true) {
   }
 #endif
@@ -804,23 +803,14 @@ class AutocompleteController : public ACProviderListener {
 
   // The latest result available from the autocomplete providers.  This may be
   // different than result_ if we've gotten results from our providers that we
-  // haven't yet shown the user.  If more matches may be coming, we'll wait to
-  // display these in hopes of minimizing flicker in GUI observers; see
-  // |coalesce_timer_|.
+  // haven't yet shown the user.  If there aren't yet as many matches as in
+  // |result|, we'll wait to display these in hopes of minimizing flicker in GUI
+  // observers.
   AutocompleteResult latest_result_;
 
-  // True when there are newer results in |latest_result_| than in |result_| and
-  // observers have not been notified about them.
-  bool update_pending_;
-
   // True if a query is not currently running.
   bool done_;
 
-  // Timer that tracks how long it's been since the last provider update we
-  // received.  Instead of notifying about each update immediately, we batch
-  // updates into groups.
-  base::OneShotTimer<AutocompleteController> coalesce_timer_;
-
   // Timer that tracks how long it's been since the last time we updated the
   // onscreen results.  This is used to ensure that observers update somewhat
   // responsively even when the user types continuously.
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
index 422c1a9..8f30703 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
@@ -402,18 +402,16 @@ SkColor AutocompleteResultView::GetTextColor() const {
 
 SkBitmap* AutocompleteResultView::GetIcon() const {
   bool selected = model_->IsSelectedIndex(model_index_);
+  if (match_.starred)
+    return selected ? icon_star_selected_ : icon_star_;
   switch (match_.type) {
     case AutocompleteMatch::URL_WHAT_YOU_TYPED:
     case AutocompleteMatch::HISTORY_URL:
     case AutocompleteMatch::NAVSUGGEST:
-      if (match_.starred)
-        return selected ? icon_star_selected_ : icon_star_;
       return selected ? icon_url_selected_ : icon_url_;
     case AutocompleteMatch::HISTORY_TITLE:
     case AutocompleteMatch::HISTORY_BODY:
     case AutocompleteMatch::HISTORY_KEYWORD:
-      if (match_.starred)
-        return selected ? icon_star_selected_ : icon_star_;
       return selected ? icon_history_selected_ : icon_history_;
     case AutocompleteMatch::SEARCH_WHAT_YOU_TYPED:
     case AutocompleteMatch::SEARCH_HISTORY:
@@ -424,9 +422,8 @@ SkBitmap* AutocompleteResultView::GetIcon() const {
       return selected ? icon_more_selected_ : icon_more_;
     default:
       NOTREACHED();
-      break;
+      return NULL;
   }
-  return NULL;
 }
 
 int AutocompleteResultView::DrawString(
@@ -521,9 +518,8 @@ int AutocompleteResultView::DrawStringFragment(
 }
 
 gfx::Font AutocompleteResultView::GetFragmentFont(int style) const {
-  if (style & ACMatchClassification::MATCH)
-    return font_.DeriveFont(0, gfx::Font::BOLD);
-  return font_;
+  return (style & ACMatchClassification::MATCH) ?
+      font_.DeriveFont(0, gfx::Font::BOLD) : font_;
 }
 
 SkColor AutocompleteResultView::GetFragmentTextColor(int style) const {
@@ -670,23 +666,24 @@ AutocompletePopupContentsView::AutocompletePopupContentsView(
       edit_view_(edit_view),
       popup_positioner_(popup_positioner),
       result_font_(font.DeriveFont(kEditFontAdjust)),
-      ALLOW_THIS_IN_INITIALIZER_LIST(size_initiator_factory_(this)),
-      ALLOW_THIS_IN_INITIALIZER_LIST(size_animation_(this)),
-      result_rows_(0) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(size_animation_(this)) {
   set_border(new PopupBorder);
 }
 
 gfx::Rect AutocompletePopupContentsView::GetPopupBounds() const {
-  if (size_animation_.IsAnimating()) {
-    gfx::Rect current_frame_bounds = start_bounds_;
-    int total_height_delta = target_bounds_.height() - start_bounds_.height();
-    int current_height_delta = static_cast<int>(
-        size_animation_.GetCurrentValue() * total_height_delta);
-    current_frame_bounds.set_height(
-        start_bounds_.height() + current_height_delta);
-    return current_frame_bounds;
-  }
-  return GetTargetBounds();
+  if (!size_animation_.IsAnimating())
+    return target_bounds_;
+
+  gfx::Rect current_frame_bounds = start_bounds_;
+  int total_height_delta = target_bounds_.height() - start_bounds_.height();
+  // Round |current_height_delta| instead of truncating so we won't leave single
+  // white pixels at the bottom of the popup as long when animating very small
+  // height differences.
+  int current_height_delta = static_cast<int>(
+      size_animation_.GetCurrentValue() * total_height_delta - 0.5);
+  current_frame_bounds.set_height(
+      current_frame_bounds.height() + current_height_delta);
+  return current_frame_bounds;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -701,11 +698,9 @@ void AutocompletePopupContentsView::InvalidateLine(size_t line) {
 }
 
 void AutocompletePopupContentsView::UpdatePopupAppearance() {
-  result_rows_ = model_->result().size();
   if (model_->result().empty()) {
     // No matches, close any existing popup.
     if (popup_->IsWindow()) {
-      size_initiator_factory_.RevokeAll();
       size_animation_.Stop();
       popup_->Hide();
     }
@@ -714,40 +709,52 @@ void AutocompletePopupContentsView::UpdatePopupAppearance() {
 
   // Update the match cached by each row, in the process of doing so make sure
   // we have enough row views.
-  int child_view_count = GetChildViewCount();
-  for (size_t i = 0; i < result_rows_; ++i) {
-    AutocompleteResultView* result_view = NULL;
-    if (i >= static_cast<size_t>(child_view_count)) {
+  int total_child_height = 0;
+  size_t child_view_count = GetChildViewCount();
+  for (size_t i = 0; i < model_->result().size(); ++i) {
+    AutocompleteResultView* result_view;
+    if (i >= child_view_count) {
       result_view = new AutocompleteResultView(this, i, result_font_);
       AddChildView(result_view);
     } else {
       result_view = static_cast<AutocompleteResultView*>(GetChildViewAt(i));
     }
     result_view->set_match(GetMatchAtIndex(i));
+    total_child_height += result_view->GetPreferredSize().height();
   }
 
-  if (popup_->IsVisible() && !IsGrowingLarger()) {
-    // When we're going to shrink, defer any size change activity for a short
-    // time. This is because as the user types characters, synchronous results
-    // for the new string come back immediately which causes the popup to shrink
-    // down for a few hundred ms until async results hit which causes the popup
-    // to flicker vertically.
-    size_initiator_factory_.RevokeAll();
-    MessageLoop::current()->PostDelayedTask(FROM_HERE,
-        size_initiator_factory_.NewRunnableMethod(
-        &AutocompletePopupContentsView::StartSizing), 300);
-  } else if (popup_->IsWindow()) {
-    // If we're growing larger or being shown for the first time, we don't use
-    // an animation since this makes the results popping in feel less
-    // instantaneous.
-    size_initiator_factory_.RevokeAll();
-    target_bounds_ = GetTargetBounds();
-    start_bounds_ = target_bounds_;
-    popup_->Show();
-  } else {
-    // If we've never been shown, we need to actually create the window, too.
+  // Calculate desired bounds.
+  gfx::Rect new_target_bounds = popup_positioner_->GetPopupBounds();
+  new_target_bounds.set_height(total_child_height);
+  gfx::Insets insets;
+  border()->GetInsets(&insets);
+  new_target_bounds.Inset(-insets.left(), -insets.top(), -insets.right(),
+                          -insets.bottom());
+
+  // If we're animating and our target height changes, reset the animation.
+  // NOTE: If we just reset blindly on _every_ update, then when the user types
+  // rapidly we could get "stuck" trying repeatedly to animate shrinking by the
+  // last few pixels to get to one visible result.
+  if (new_target_bounds.height() != target_bounds_.height())
+    size_animation_.Reset();
+  target_bounds_ = new_target_bounds;
+
+  if (!popup_->IsWindow()) {
+    // If we've never been shown, we need to create the window.
     popup_->Init(edit_view_, this);
+  } else {
+    // Animate the popup shrinking, but don't animate growing larger (or
+    // appearing for the first time) since that would make the popup feel less
+    // responsive.
+    GetWidget()->GetBounds(&start_bounds_, true);
+    if (popup_->IsVisible() &&
+        (target_bounds_.height() < start_bounds_.height()))
+      size_animation_.Show();
+    else
+      start_bounds_ = target_bounds_;
+    popup_->Show();
   }
+
   SchedulePaint();
 }
 
@@ -927,38 +934,3 @@ void AutocompletePopupContentsView::MakeCanvasTransparent(
   canvas->FillRectInt(0, 0, canvas->getDevice()->width(),
                       canvas->getDevice()->height(), paint);
 }
-
-void AutocompletePopupContentsView::StartSizing() {
-  CalculateAnimationFrameBounds();
-  if (start_bounds_ != target_bounds_) {
-    size_animation_.Reset(0.0);
-    size_animation_.Show();
-  } else {
-    popup_->Show();
-  }
-}
-
-void AutocompletePopupContentsView::CalculateAnimationFrameBounds() {
-  GetWidget()->GetBounds(&start_bounds_, true);
-  target_bounds_ = GetTargetBounds();
-}
-
-gfx::Rect AutocompletePopupContentsView::GetTargetBounds() const {
-  gfx::Insets insets;
-  border()->GetInsets(&insets);
-  gfx::Rect target_bounds = popup_positioner_->GetPopupBounds();
-  int height = 0;
-  for (size_t i = 0; i < result_rows_; ++i)
-    height += GetChildViewAt(i)->GetPreferredSize().height();
-  target_bounds.set_height(height);
-  target_bounds.Inset(-insets.left(), -insets.top(), -insets.right(),
-                      -insets.bottom());
-  return target_bounds;
-}
-
-bool AutocompletePopupContentsView::IsGrowingLarger() const {
-  gfx::Rect current_bounds;
-  GetWidget()->GetBounds(&current_bounds, true);
-  gfx::Rect target_bounds = GetTargetBounds();
-  return current_bounds.height() < target_bounds.height();
-}
diff --git a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
index 534e020..a86914d 100644
--- a/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
+++ b/chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
@@ -94,18 +94,6 @@ class AutocompletePopupContentsView : public views::View,
   // Makes the contents of the canvas slightly transparent.
   void MakeCanvasTransparent(gfx::Canvas* canvas);
 
-  // Starts sizing the popup to its new target size.
-  void StartSizing();
-
-  // Calculate the start and target bounds of the popup for an animation.
-  void CalculateAnimationFrameBounds();
-
-  // Gets the ideal bounds to display the number of result rows.
-  gfx::Rect GetTargetBounds() const;
-
-  // Returns true if the popup needs to grow larger to show |result_rows_|.
-  bool IsGrowingLarger() const;
-
 #if defined(OS_WIN)
   // The popup that contains this view.
   scoped_ptr<AutocompletePopupWin> popup_;
@@ -124,21 +112,12 @@ class AutocompletePopupContentsView : public views::View,
   // by the edit that created us.
   gfx::Font result_font_;
 
-  // See discussion in UpdatePopupAppearance.
-  ScopedRunnableMethodFactory<AutocompletePopupContentsView>
-      size_initiator_factory_;
-
   // The popup sizes vertically using an animation when the popup is getting
   // shorter (not larger, that makes it look "slow").
   SlideAnimation size_animation_;
   gfx::Rect start_bounds_;
   gfx::Rect target_bounds_;
 
-  // The number of rows required by the result set. This can differ from the
-  // number of child views, as we retain rows after they are removed from the
-  // model so we can animate the popup and still have the removed rows painted.
-  size_t result_rows_;
-
   DISALLOW_COPY_AND_ASSIGN(AutocompletePopupContentsView);
 };