Enabled pressing TAB to cycle through the Omnibox results.

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

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // For WinDDK ATL compatibility, these ATL headers must come first.
 #include "build/build_config.h"
 #if defined(OS_WIN)
 #include <atlbase.h>  // NOLINT
 #include <atlwin.h>  // NOLINT
 #endif
 
 #include "chrome/browser/ui/views/autocomplete/autocomplete_result_view.h"
 
 #include <algorithm>  // NOLINT
 
 #include "base/i18n/bidi_line_iterator.h"
+#include "chrome/browser/autocomplete/autocomplete_popup_model.h"
 #include "chrome/browser/ui/views/autocomplete/autocomplete_result_view_model.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/text/text_elider.h"
 #include "ui/gfx/canvas_skia.h"
 #include "ui/gfx/color_utils.h"
 
@@ skipping 77 matching lines @@
 AutocompleteResultView::AutocompleteResultView(
     AutocompleteResultViewModel* model,
     int model_index,
     const gfx::Font& font,
     const gfx::Font& bold_font)
     : model_(model),
       model_index_(model_index),
       normal_font_(font),
       bold_font_(bold_font),
       ellipsis_width_(font.GetStringWidth(string16(kEllipsis))),
-      mirroring_context_(new MirroringContext()) {
+      mirroring_context_(new MirroringContext()),
+      ALLOW_THIS_IN_INITIALIZER_LIST(animation_(new ui::SlideAnimation(this))) {
   CHECK_GE(model_index, 0);
   if (default_icon_size_ == 0) {
     default_icon_size_ = ResourceBundle::GetSharedInstance().GetBitmapNamed(
         AutocompleteMatch::TypeToIcon(AutocompleteMatch::URL_WHAT_YOU_TYPED))->
         width();
   }
 }
 
 AutocompleteResultView::~AutocompleteResultView() {
 }
@@ skipping 41 matching lines @@
                                                      colors[i][BACKGROUND]);
     }
     initialized = true;
   }
 
   return colors[state][kind];
 }
 
 void AutocompleteResultView::SetMatch(const AutocompleteMatch& match) {
   match_ = match;
+  animation_->Reset();
+
+  if (match.associated_keyword.get()) {

[Peter Kasting, 2012/01/12 22:59:59]

There is a memory safety issue here, as well as a simplicity one.

Memory-safety-wise, you need to do "keyword_icon_->set_parent_owned(false);"
after creating it, because you own the object (via the scoped_ptr) rather than
the view hierarchy owning it.

Simplicity-wise, instead of creating this object in here, let's do it in the
constructor.  Then the code here can just say something like:

  if (match.associated_keyword.get()) {
    if (!keyword_icon_->parent())
      AddChildView(keyword_icon_.get());
  } else if keyword_icon_->parent()) {
    RemoveChildView(keyword_icon_.get());
  }

This also eliminates the need for the Invalidate() function.

[aaron.randolph, 2012/01/13 00:03:48]

I'm not following how this eliminates the Invalidate function.  I believe I
still need that function to determine when to switch the ImageView between the
normal icon and the selected icon?  (I could do it in OnPaint but I don't like
copying the image on every paint event, especially during an animation, when it
only needs to be done once beforehand.)

[Peter Kasting, 2012/01/13 00:36:41]

You're right, I missed the normal vs. selected part.  Somehow the Invalidate()
solution feels slightly wrong, like there must be a better place to hook
precisely when we're selected or not, but I don't see one.

+    keyword_icon_.reset(new views::ImageView());
+    keyword_icon_->EnableCanvasFlippingForRTLUI(true);
+    keyword_icon_->SetImage(GetKeywordIcon());
+    AddChildView(keyword_icon_.get());
+  } else if (keyword_icon_.get()) {
+    RemoveChildView(keyword_icon_.get());
+    keyword_icon_.reset();
+  }
+
   Layout();
 }
 
+void AutocompleteResultView::ShowKeyword(bool show_keyword) {
+  if (show_keyword)
+    animation_->Show();
+  else
+    animation_->Hide();
+}
+
+void AutocompleteResultView::Invalidate() {
+  if (keyword_icon_.get())
+    keyword_icon_->SetImage(GetKeywordIcon());
+
+  SchedulePaint();
+}
+
+gfx::Size AutocompleteResultView::GetPreferredSize() {
+  return gfx::Size(0, std::max(
+      default_icon_size_ + (kMinimumIconVerticalPadding * 2),
+      GetTextHeight() + (kMinimumTextVerticalPadding * 2)));
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // AutocompleteResultView, protected:
 
 void AutocompleteResultView::PaintMatch(gfx::Canvas* canvas,
                                         const AutocompleteMatch& match,
                                         int x) {
   x = DrawString(canvas, match.contents, match.contents_class, false, x,
                  text_bounds_.y());
 
   // Paint the description.
@@ skipping 67 matching lines @@
         icon = IDR_OMNIBOX_STAR_SELECTED;
         break;
       default:
         NOTREACHED();
         break;
     }
   }
   return ResourceBundle::GetSharedInstance().GetBitmapNamed(icon);
 }
 
+const SkBitmap* AutocompleteResultView::GetKeywordIcon() const {
+  return ResourceBundle::GetSharedInstance().GetBitmapNamed(
+      model_->IsSelectedIndex(model_index_) ? IDR_OMNIBOX_TTS_SELECTED :

[Peter Kasting, 2012/01/12 22:59:59]

Nit: Break after '?' rather than ':'

[Peter Kasting, 2012/01/13 00:36:41]

Nit: (GetState() == SELECTED) seems fractionally cleaner (although if that's
true the GetIcon() should probably make the same change)

+          IDR_OMNIBOX_TTS);
+}
+
 int AutocompleteResultView::DrawString(
     gfx::Canvas* canvas,
     const string16& text,
     const ACMatchClassifications& classifications,
     bool force_dim,
     int x,
     int y) {
   if (text.empty())
     return x;
 
@@ skipping 210 matching lines @@
       //     ellipsis we'll append to it, or
       //   * It is also bold, in which case we don't want to fall back
       //     to a normal ellipsis anyway (see comment above).
     }
   }
 
   // We couldn't draw anything.
   runs->clear();
 }
 
-gfx::Size AutocompleteResultView::GetPreferredSize() {
-  return gfx::Size(0, std::max(
-      default_icon_size_ + (kMinimumIconVerticalPadding * 2),
-      GetTextHeight() + (kMinimumTextVerticalPadding * 2)));
-}
-
 void AutocompleteResultView::Layout() {
   const SkBitmap* icon = GetIcon();
+
   icon_bounds_.SetRect(LocationBarView::kEdgeItemPadding +
       ((icon->width() == default_icon_size_) ?
           0 : LocationBarView::kIconInternalPadding),
       (height() - icon->height()) / 2, icon->width(), icon->height());
 
   int text_x = LocationBarView::kEdgeItemPadding + default_icon_size_ +
       LocationBarView::kItemPadding;
   int text_height = GetTextHeight();
+  int text_width;
+
+  if (match_.associated_keyword.get()) {
+    const SkBitmap* kw_icon = GetKeywordIcon();

[Peter Kasting, 2012/01/13 00:36:41]

Nit: Rather than calling GetKeywordIcon() here, it seems like we can just use
|keyword_icon_|'s width and height in this function, because the ImageView
should auto-size to its image's size (IIRC).  That saves a line, and plus, if we
ever make that view more than just an image, this code will all still work
correctly.

[aaron.randolph, 2012/01/13 03:08:07]

Actually, it doesn't appear that ImageView sets the view's width and height to
the image size, which seems a bit odd to me. It will return the image size if
you call GetPreferredSize or GetImageSize or GetImage().width, however.

+    const int kw_collapsed_size = kw_icon->width() +
+        LocationBarView::kEdgeItemPadding;
+    const int max_kw_x = bounds().width() - kw_collapsed_size;
+    const int kw_x = animation_->CurrentValueBetween(max_kw_x,
+        LocationBarView::kEdgeItemPadding);
+    const int kw_text_x = kw_x + kw_icon->width() +
+        LocationBarView::kItemPadding;
+
+    text_width = kw_x - text_x - LocationBarView::kItemPadding;
+    keyword_text_bounds_.SetRect(kw_text_x, 0, std::max(
+        bounds().width() - kw_text_x - LocationBarView::kEdgeItemPadding, 0),
+        text_height);
+    keyword_icon_->SetBounds(kw_x, (height() - kw_icon->height()) / 2,

[Peter Kasting, 2012/01/13 00:36:41]

Nit: This can just be SetPosition().

+        kw_icon->width(), kw_icon->height());
+  } else {
+    text_width = bounds().width() - text_x - LocationBarView::kEdgeItemPadding;
+  }
+
   text_bounds_.SetRect(text_x, std::max(0, (height() - text_height) / 2),
-      std::max(bounds().width() - text_x - LocationBarView::kEdgeItemPadding,
-      0), text_height);
+      std::max(text_width, 0), text_height);
+}
+
+void AutocompleteResultView::OnBoundsChanged(
+    const gfx::Rect& previous_bounds) {
+  animation_->SetSlideDuration(width() / 4);
 }
 
 void AutocompleteResultView::OnPaint(gfx::Canvas* canvas) {
   const ResultViewState state = GetState();
   if (state != NORMAL)
     canvas->GetSkCanvas()->drawColor(GetColor(state, BACKGROUND));
 
-  // Paint the icon.
-  canvas->DrawBitmapInt(*GetIcon(), GetMirroredXForRect(icon_bounds_),
-                        icon_bounds_.y());
+  if (!match_.associated_keyword.get() ||
+      keyword_icon_->x() > icon_bounds_.right()) {
+    // Paint the icon.
+    canvas->DrawBitmapInt(*GetIcon(), GetMirroredXForRect(icon_bounds_),
+        icon_bounds_.y());
 
-  // Paint the text.
-  int x = GetMirroredXForRect(text_bounds_);
-  mirroring_context_->Initialize(x, text_bounds_.width());
-  PaintMatch(canvas, match_, x);
+    // Paint the text.
+    int x = GetMirroredXForRect(text_bounds_);
+    mirroring_context_->Initialize(x, text_bounds_.width());
+    PaintMatch(canvas, match_, x);
+  }
+
+  if (match_.associated_keyword.get()) {
+    // Paint the keyword text.
+    int x = GetMirroredXForRect(keyword_text_bounds_);
+    mirroring_context_->Initialize(x, keyword_text_bounds_.width());
+    PaintMatch(canvas, *match_.associated_keyword.get(), x);
+  }
 }
+
+void AutocompleteResultView::AnimationProgressed(
+    const ui::Animation* animation) {
+  Layout();
+  SchedulePaint();
+}
+