Enabled pressing TAB to cycle through the Omnibox results.

chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc

Index: chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
===================================================================
--- chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc	(revision 93757)
+++ chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc	(working copy)
@@ -24,6 +24,7 @@
 #include "ui/gfx/canvas_skia.h"
 #include "ui/gfx/insets.h"
 #include "ui/gfx/path.h"
+#include "ui/gfx/rect.h"
 #include "unicode/ubidi.h"
 #include "views/controls/button/text_button.h"
 #include "views/controls/label.h"
@@ -142,7 +143,7 @@
   virtual ~AutocompletePopupWidget() {}
 
  private:
-   DISALLOW_COPY_AND_ASSIGN(AutocompletePopupWidget);
+  DISALLOW_COPY_AND_ASSIGN(AutocompletePopupWidget);
 };
 
 class AutocompletePopupContentsView::InstantOptInView
@@ -273,14 +274,38 @@
 void AutocompletePopupContentsView::LayoutChildren() {
   gfx::Rect contents_rect = GetContentsBounds();
   int top = contents_rect.y();
-  for (int i = 0; i < child_count(); ++i) {
-    View* v = child_at(i);
-    if (v->IsVisible()) {
-      v->SetBounds(contents_rect.x(), top, contents_rect.width(),
-                   v->GetPreferredSize().height());
-      top = v->bounds().bottom();
+  size_t child_rv_count = child_count();
+
+  for (size_t i = 0; i < child_rv_count; i += 2) {
+    View* result = child_at(i);
+    if (!result->IsVisible())
+      continue;
+
+    AutocompleteResultView* keyword =
+        static_cast<AutocompleteResultView*>(child_at(i + 1));
+    if (keyword->IsVisible()) {
+      const float collapsed_width = static_cast<float>(
+          keyword->GetCollapsedSize().width() + LocationBarView::kItemPadding);
+      const float kw_collapsed_x = static_cast<float>(contents_rect.width() -
+          collapsed_width);
+      const int kw_x = static_cast<int>(kw_collapsed_x - ((kw_collapsed_x -
+          collapsed_width) * keyword_animations_[i / 2]->GetCurrentValue()));
+
+      result->SetBounds(contents_rect.x(), top, kw_x,
+          result->GetPreferredSize().height());
+      keyword->SetBounds(kw_x + LocationBarView::kItemPadding, top,
+          contents_rect.width() - (kw_x - LocationBarView::kItemPadding),
+          keyword->GetPreferredSize().height());
+    } else {
+      result->SetBounds(contents_rect.x(), top, contents_rect.width(),
+          result->GetPreferredSize().height());
     }
+    top = result->bounds().bottom();
   }
+
+  if (opt_in_view_ && opt_in_view_->IsVisible())

[Peter Kasting, 2011/07/27 20:18:25]

How come we didn't use to have this?  Was it an error?

[aaron.randolph, 2011/07/28 16:01:55]

The old loop adjusted all children, implicitly including the opt_in_view.  With
the new loop, I increment by two, resulting in it skipping the final
(opt_in_view) view.  So I made it explicit.  Looking at it again, it appears
child_rv_count is unnecessary so I'll remove that.

+    opt_in_view_->SetBounds(contents_rect.x(), top, contents_rect.width(),
+        opt_in_view_->GetPreferredSize().height());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -291,7 +316,14 @@
 }
 
 void AutocompletePopupContentsView::InvalidateLine(size_t line) {
-  child_at(static_cast<int>(line))->SchedulePaint();
+    child_at(static_cast<int>(line * 2))->SchedulePaint();
+    child_at(static_cast<int>(line * 2 + 1))->SchedulePaint();
+
+    if (line == model_->selected_line() && model_->keyword_selected()) {

[Peter Kasting, 2011/07/27 20:18:25]

Nit: No need for {}

+      keyword_animations_[line]->Show();
+    } else {
+      keyword_animations_[line]->Hide();
+    }
 }
 
 void AutocompletePopupContentsView::UpdatePopupAppearance() {
@@ -299,6 +331,11 @@
     // No matches, close any existing popup.
     if (popup_ != NULL) {
       size_animation_.Stop();
+
+      for (SlideAnimations::iterator i(keyword_animations_.begin());
+          i != keyword_animations_.end(); ++i)
+        (*i)->Stop();
+
       // NOTE: Do NOT use CloseNow() here, as we may be deep in a callstack
       // triggered by the popup receiving a message (e.g. LBUTTONUP), and
       // destroying the popup would cause us to read garbage when we unwind back
@@ -316,19 +353,33 @@
     DCHECK(child_rv_count > 0);
     child_rv_count--;
   }
-  for (size_t i = 0; i < model_->result().size(); ++i) {
-    AutocompleteResultView* result_view;
-    if (i >= child_rv_count) {
-      result_view =
-          CreateResultView(this, i, result_font_, result_bold_font_);
-      AddChildViewAt(result_view, static_cast<int>(i));
+  const size_t result_size = model_->result().size();
+  CreateCachedViews(child_rv_count, result_size * 2);

[Peter Kasting, 2011/07/27 20:18:25]

Nit: It's always bugged me that we create child views on demand but don't
destroy them when they're no longer needed.  It seems like it would be simpler
to just create all the children up front in the constructor, and make them all
hidden.  We could create all the keyword animations at that point too.

[aaron.randolph, 2011/07/28 16:01:55]

I agree.  I'll make that change for the next update.

+  for (size_t i = 0; i < result_size; ++i) {
+    const AutocompleteMatch& match = GetMatchAtIndex(i);
+    AutocompleteResultView* view = static_cast<AutocompleteResultView*>(
+        child_at(i * 2));
+    view->SetMatch(match);
+    view->SetVisible(true);
+
+    // Create a view and animation for an associated keyword, even if
+    // the match doesn't have a keyword.
+    view = static_cast<AutocompleteResultView*>(child_at(i * 2 + 1));
+    if (match.associated_keyword.get()) {

[Peter Kasting, 2011/07/27 20:18:25]

Nit: Shorter:

    view->SetVisible(match.associated_keyword.get());
    if (view->IsVisible())
      view->SetMatch(*match.associated_keyword);

+      view->SetMatch(*match.associated_keyword);
+      view->SetVisible(true);
     } else {
-      result_view = static_cast<AutocompleteResultView*>(child_at(i));
-      result_view->SetVisible(true);
+      view->SetVisible(false);
     }
-    result_view->SetMatch(GetMatchAtIndex(i));
+
+    if (keyword_animations_.size() <= i) {
+      ui::SlideAnimation* anim = new ui::SlideAnimation(this);

[Peter Kasting, 2011/07/27 20:18:25]

Nit: Instead of using a ScopedVector and allocating these on the heap, could we
just use a ui::SlideAnimation[]? This way they'd all be stack-allocated and it'd
hopefully be a fraction simpler.

+      anim->SetSlideDuration(500);
+      keyword_animations_.push_back(anim);
+    }
+    keyword_animations_[i]->Reset();
   }
-  for (size_t i = model_->result().size(); i < child_rv_count; ++i)
+  for (size_t i = result_size * 2; i < child_rv_count; ++i)
     child_at(i)->SetVisible(false);
 
   PromoCounter* counter = model_->profile()->GetInstantPromoCounter();
@@ -375,9 +426,20 @@
     popup_->SetBounds(GetPopupBounds());
   }
 
+  LayoutChildren();

[Peter Kasting, 2011/07/27 20:18:25]

Is this call necessary?  It seems like it ought not to be, because if it were,
then we'd presumably also need to UpdateBlurRegion() like Layout() does.

[aaron.randolph, 2011/07/28 16:01:55]

It's necessary so the result views will resize to include keyword hints. 
Without it, the keyword hints don't display.   I'll change both these lines to
just a call to Layout().

   SchedulePaint();
 }
 
+void AutocompletePopupContentsView::CreateCachedViews(
+    size_t child_count,
+    size_t required_count) {
+  for (size_t i = child_count; i < required_count; ++i) {
+    AutocompleteResultView* result_view =
+        CreateResultView(this, i, result_font_, result_bold_font_);
+    AddChildViewAt(result_view, static_cast<int>(i));
+  }
+}
+
 gfx::Rect AutocompletePopupContentsView::GetTargetBounds() {
   return target_bounds_;
 }
@@ -394,11 +456,16 @@
 // AutocompletePopupContentsView, AutocompleteResultViewModel implementation:
 
 bool AutocompletePopupContentsView::IsSelectedIndex(size_t index) const {
-  return HasMatchAt(index) ? index == model_->selected_line() : false;
+  size_t selected_line = model_->selected_line() * 2;
+
+  if (model_->keyword_selected())
+    selected_line++;
+
+  return index == selected_line;
 }
 
 bool AutocompletePopupContentsView::IsHoveredIndex(size_t index) const {
-  return HasMatchAt(index) ? index == model_->hovered_line() : false;
+  return index == model_->hovered_line() * 2;
 }
 
 const SkBitmap* AutocompletePopupContentsView::GetIconIfExtensionMatch(
@@ -416,6 +483,8 @@
   // We should only be running the animation when the popup is already visible.
   DCHECK(popup_ != NULL);
   popup_->SetBounds(GetPopupBounds());
+
+  Layout();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -509,6 +578,29 @@
   canvas->drawColor(AutocompleteResultView::GetColor(
       AutocompleteResultView::NORMAL, AutocompleteResultView::BACKGROUND));
   View::PaintChildren(canvas);
+
+  // Draw dividing lines between results and keyword views

[Peter Kasting, 2011/07/27 20:18:25]

Nit: Trailing period

+  SkPaint paint;
+  paint.setColor(AutocompleteResultView::GetColor(
+    AutocompleteResultView::NORMAL, AutocompleteResultView::DIMMED_TEXT));
+  paint.setStrokeWidth(SkIntToScalar(1));
+  paint.setAntiAlias(true);
+
+  for (size_t i = 0; i < model_->result().size(); ++i) {
+    views::View* kw_child = child_at(i * 2 + 1);
+    if (kw_child->IsVisible()) {
+      gfx::Rect bounds = kw_child->bounds();
+
+      canvas->drawLine(

[Peter Kasting, 2011/07/27 20:18:25]

If I'm reading this right, it draws the line in the first column of pixels on
the keyword side.

I think it'd be better to actually have 1 px between the normal and keyword
portions of the row and draw the line inside this pixel.  That way the line
would be outside the selected area no matter which side is selected.

+        SkIntToScalar(bounds.x()),
+        SkIntToScalar(bounds.y() +
+            LocationBarView::kNormalHorizontalEdgeThickness),
+        SkIntToScalar(bounds.x()),
+        SkIntToScalar(bounds.y() + bounds.height() -
+            LocationBarView::kNormalHorizontalEdgeThickness * 2),
+        paint);
+    }
+  }
 }
 
 int AutocompletePopupContentsView::CalculatePopupHeight() {
@@ -645,10 +737,8 @@
   // extension, |match| and its contents.  So copy the relevant match out to
   // make sure it stays alive until the call completes.
   AutocompleteMatch match = model_->result().match_at(index);
-  string16 keyword;
-  const bool is_keyword_hint = model_->GetKeywordForMatch(match, &keyword);
   omnibox_view_->OpenMatch(match, disposition, GURL(), index,
-                         is_keyword_hint ? string16() : keyword);
+      match.keyword);
 }
 
 size_t AutocompletePopupContentsView::GetIndexForPoint(
@@ -659,7 +749,7 @@
   int nb_match = model_->result().size();
   DCHECK(nb_match <= child_count());
   for (int i = 0; i < nb_match; ++i) {
-    views::View* child = child_at(i);
+    views::View* child = child_at(i * 2);
     gfx::Point point_in_child_coords(point);
     View::ConvertPointToView(this, child, &point_in_child_coords);
     if (child->HitTest(point_in_child_coords))