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 94623)
+++ 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
@@ -246,6 +247,19 @@
   set_border(bubble_border);
   // The contents is owned by the LocationBarView.
   set_parent_owned(false);
+
+  for (size_t i = 0; i < AutocompleteResult::kMaxMatches * 2; ++i) {

[sky, 2011/08/01 16:02:15]

I don't like always creating the max result views and slideanimations.

+    AutocompleteResultView* result_view =
+      CreateResultView(this, i, result_font_, result_bold_font_);
+    AddChildViewAt(result_view, static_cast<int>(i));
+    result_view->SetVisible(false);
+
+    if (i < AutocompleteResult::kMaxMatches) {
+      ui::SlideAnimation* anim = new ui::SlideAnimation(this);
+      anim->SetSlideDuration(500);

[sky, 2011/08/01 16:02:15]

500 is too long. Can you use the default?

+      keyword_animations_.push_back(anim);
+    }
+  }
 }
 
 AutocompletePopupContentsView::~AutocompletePopupContentsView() {
@@ -273,14 +287,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();
+
+  for (int i = 0; i < child_count(); i += 2) {
+    View* result = child_at(i);
+    if (!result->IsVisible())
+      continue;
+
+    AutocompleteResultView* keyword =

[sky, 2011/08/01 16:02:15]

Casts like this are easy to get wrong. Use an id or classname to find the views
you care about.

[aaron.randolph, 2011/08/02 21:54:58]

Just to be clear, are you suggesting I keep a list of AutocompleteResultView* as
a member and iterate through that instead of using child_at?  Or did you have an
alternative idea in mind?

[Peter Kasting, 2011/08/04 20:07:53]

Nit: TBH, I don't think this suggestion of Scott's is a net win, especially if
we're just DCHECKing it (GetClassName() will never get called in release mode,
etc., meaning we add a lot of boilerplate for little gain).

Maybe if instead of using "child_count()" above you use
"AutocompleteResult::kMaxMatches * 2", this will be safer against the addition
of more children later, and then you can go back to just casting directly.  That
will also make this code a bit less confusing w.r.t. apparently sizing all
children in this loop but not actually doing so for the |opt_in_view_|.

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

[sky, 2011/08/01 16:02:15]

To make lifetime easier can be push the animation into the resultsview? And use
CurrentValueBetween.

+
+      result->SetBounds(contents_rect.x(), top,
+          kw_x - LocationBarView::kItemPadding,

[Peter Kasting, 2011/07/29 21:08:44]

Don't we only want to subtract 1?

+          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())
+    opt_in_view_->SetBounds(contents_rect.x(), top, contents_rect.width(),
+        opt_in_view_->GetPreferredSize().height());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -291,7 +329,13 @@
 }
 
 void AutocompletePopupContentsView::InvalidateLine(size_t line) {
-  child_at(static_cast<int>(line))->SchedulePaint();
+    child_at(static_cast<int>(line * 2))->SchedulePaint();

[sky, 2011/08/01 16:02:15]

spacing if off.

+    child_at(static_cast<int>(line * 2 + 1))->SchedulePaint();
+
+    if (line == model_->selected_line() && model_->keyword_selected())
+      keyword_animations_[line]->Show();
+    else
+      keyword_animations_[line]->Hide();
 }
 
 void AutocompletePopupContentsView::UpdatePopupAppearance() {
@@ -299,6 +343,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 +365,22 @@
     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));
-    } else {
-      result_view = static_cast<AutocompleteResultView*>(child_at(i));
-      result_view->SetVisible(true);
-    }
-    result_view->SetMatch(GetMatchAtIndex(i));
+  const size_t result_size = model_->result().size();
+  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);
+
+    view = static_cast<AutocompleteResultView*>(child_at(i * 2 + 1));
+    view->SetVisible(match.associated_keyword.get() != NULL);
+    if (view->IsVisible())
+      view->SetMatch(*match.associated_keyword);
+
+    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,7 +427,7 @@
     popup_->SetBounds(GetPopupBounds());
   }
 
-  SchedulePaint();
+  Layout();
 }
 
 gfx::Rect AutocompletePopupContentsView::GetTargetBounds() {
@@ -394,11 +446,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 +473,8 @@
   // We should only be running the animation when the popup is already visible.
   DCHECK(popup_ != NULL);
   popup_->SetBounds(GetPopupBounds());
+
+  Layout();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -509,6 +568,31 @@
   canvas->drawColor(AutocompleteResultView::GetColor(
       AutocompleteResultView::NORMAL, AutocompleteResultView::BACKGROUND));
   View::PaintChildren(canvas);
+
+  // Draw dividing lines between results and keyword views.
+  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(
+        SkIntToScalar(bounds.x() -
+            LocationBarView::kNormalHorizontalEdgeThickness),

[Peter Kasting, 2011/07/29 21:08:44]

Nit: Simpler: Use the even children and draw at (bounds.right()) instead of
using the odd children and subtracting a constant.

[aaron.randolph, 2011/07/29 21:57:10]

I would still need to add the constant to the right edge in order to get the
line centered between the two views.  Otherwise, the line draws on the edge of
the selection, and I was under the belief you wanted a gap from the selection
border?

[Peter Kasting, 2011/07/30 00:00:29]

I was imagining the left and right sides having exactly one pixel -- the border
pixel -- between them.  So e.g. left side is from [0, 10), border pixel at 10,
right side at [11, 21) or similar.  That would mean that the border pixel could
be drawn at the left side's ".right()" since the left side would draw "up to but
not including" that pixel.

If you implement this and it looks dumb we can add padding, but I figured this
would be more symmetrical with how we draw the selection right up to (but not
including) the outside edge of the popup.

[aaron.randolph, 2011/07/30 00:34:17]

I'm with you now.  I was thinking right() was inclusive, not exclusive.  I've
updated it for the next CL.

+        SkIntToScalar(bounds.y() +
+            LocationBarView::kNormalHorizontalEdgeThickness),
+        SkIntToScalar(bounds.x() -
+            LocationBarView::kNormalHorizontalEdgeThickness),
+        SkIntToScalar(bounds.y() + bounds.height() -
+            LocationBarView::kNormalHorizontalEdgeThickness * 2),
+        paint);
+    }
+  }
 }
 
 int AutocompletePopupContentsView::CalculatePopupHeight() {
@@ -645,10 +729,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 +741,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))