InstantExtended(gtk): Hide top match if told to so.

chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc

Index: chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc
diff --git a/chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc b/chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc
index 23ab592f21028d2cebd792d253dace6104bddc8c..b4db25c34185bc1d82a752f896083fd63aa5f161 100644
--- a/chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc
+++ b/chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc
@@ -98,15 +98,6 @@ gfx::Rect GetWindowRect(GdkWindow* window) {
   return gfx::Rect(width, height);
 }
 
-// Return a Rect for the space for a result line.  This excludes the border,
-// but includes the padding.  This is the area that is colored for a selection.
-gfx::Rect GetRectForLine(size_t line, int width) {
-  return gfx::Rect(kBorderThickness,
-                   (line * kHeightPerResult) + kBorderThickness,
-                   width - (kBorderThickness * 2),
-                   kHeightPerResult);
-}
-
 // TODO(deanm): Find some better home for this, and make it more efficient.
 size_t GetUTF8Offset(const string16& text, size_t text_offset) {
   return UTF16ToUTF8(text.substr(0, text_offset)).length();
@@ -170,113 +161,43 @@ GdkColor SelectedURLColor(GdkColor foreground, GdkColor background) {
 }
 }  // namespace
 
-void OmniboxPopupViewGtk::SetupLayoutForMatch(
-    PangoLayout* layout,
-    const string16& text,
-    const AutocompleteMatch::ACMatchClassifications& classifications,
-    const GdkColor* base_color,
-    const GdkColor* dim_color,
-    const GdkColor* url_color,
-    const std::string& prefix_text) {
-  // In RTL, mark text with left-to-right embedding mark if there is no strong
-  // RTL characters inside it, so the ending punctuation displays correctly
-  // and the eliding ellipsis displays correctly. We only mark the text with
-  // LRE. Wrapping it with LRE and PDF by calling AdjustStringForLocaleDirection
-  // or WrapStringWithLTRFormatting will render the elllipsis at the left of the
-  // elided pure LTR text.
-  bool marked_with_lre = false;
-  string16 localized_text = text;
-  // Pango is really easy to overflow and send into a computational death
-  // spiral that can corrupt the screen. Assume that we'll never have more than
-  // 2000 characters, which should be a safe assumption until we all get robot
-  // eyes. http://crbug.com/66576
-  if (localized_text.length() > 2000)
-    localized_text = localized_text.substr(0, 2000);
-  bool is_rtl = base::i18n::IsRTL();
-  if (is_rtl && !base::i18n::StringContainsStrongRTLChars(localized_text)) {
-    localized_text.insert(0, 1, base::i18n::kLeftToRightEmbeddingMark);
-    marked_with_lre = true;
-  }
-
-  // We can have a prefix, or insert additional characters while processing the
-  // classifications.  We need to take this in to account when we translate the
-  // UTF-16 offsets in the classification into text_utf8 byte offsets.
-  size_t additional_offset = prefix_text.length();  // Length in utf-8 bytes.
-  std::string text_utf8 = prefix_text + UTF16ToUTF8(localized_text);
-
-  PangoAttrList* attrs = pango_attr_list_new();
-
-  // TODO(deanm): This is a hack, just to handle coloring prefix_text.
-  // Hopefully I can clean up the match situation a bit and this will
-  // come out cleaner.  For now, apply the base color to the whole text
-  // so that our prefix will have the base color applied.
-  PangoAttribute* base_fg_attr = pango_attr_foreground_new(
-      base_color->red, base_color->green, base_color->blue);
-  pango_attr_list_insert(attrs, base_fg_attr);  // Ownership taken.
-
-  // Walk through the classifications, they are linear, in order, and should
-  // cover the entire text.  We create a bunch of overlapping attributes,
-  // extending from the offset to the end of the string.  The ones created
-  // later will override the previous ones, meaning we will still setup each
-  // portion correctly, we just don't need to compute the end offset.
-  for (ACMatchClassifications::const_iterator i = classifications.begin();
-       i != classifications.end(); ++i) {
-    size_t offset = GetUTF8Offset(localized_text, i->offset) +
-                    additional_offset;
-
-    // TODO(deanm): All the colors should probably blend based on whether this
-    // result is selected or not.  This would include the green URLs.  Right
-    // now the caller is left to blend only the base color.  Do we need to
-    // handle things like DIM urls?  Turns out DIM means something different
-    // than you'd think, all of the description text is not DIM, it is a
-    // special case that is not very common, but we should figure out and
-    // support it.
-    const GdkColor* color = base_color;
-    if (i->style & ACMatchClassification::URL) {
-      color = url_color;
-      // Insert a left to right embedding to make sure that URLs are shown LTR.
-      if (is_rtl && !marked_with_lre) {
-        std::string lre(kLRE);
-        text_utf8.insert(offset, lre);
-        additional_offset += lre.length();
-      }
-    }
-
-    if (i->style & ACMatchClassification::DIM)
-      color = dim_color;
-
-    PangoAttribute* fg_attr = pango_attr_foreground_new(
-        color->red, color->green, color->blue);
-    fg_attr->start_index = offset;
-    pango_attr_list_insert(attrs, fg_attr);  // Ownership taken.
-
-    // Matched portions are bold, otherwise use the normal weight.
-    PangoWeight weight = (i->style & ACMatchClassification::MATCH) ?
-        PANGO_WEIGHT_BOLD : PANGO_WEIGHT_NORMAL;
-    PangoAttribute* weight_attr = pango_attr_weight_new(weight);
-    weight_attr->start_index = offset;
-    pango_attr_list_insert(attrs, weight_attr);  // Ownership taken.
-  }
-
-  pango_layout_set_text(layout, text_utf8.data(), text_utf8.length());
-  pango_layout_set_attributes(layout, attrs);  // Ref taken.
-  pango_attr_list_unref(attrs);
-}
-
 OmniboxPopupViewGtk::OmniboxPopupViewGtk(const gfx::Font& font,
                                          OmniboxView* omnibox_view,
                                          OmniboxEditModel* edit_model,
                                          GtkWidget* location_bar)
     : signal_registrar_(new ui::GtkSignalRegistrar),
-      model_(new OmniboxPopupModel(this, edit_model)),
       omnibox_view_(omnibox_view),
       location_bar_(location_bar),
       window_(gtk_window_new(GTK_WINDOW_POPUP)),
       layout_(NULL),
-      theme_service_(GtkThemeService::GetFrom(edit_model->profile())),
+      theme_service_(NULL),
       font_(font.DeriveFont(kEditFontAdjust)),
       ignore_mouse_drag_(false),
       opened_(false) {
+  // edit_model may be NULL in unit tests.
+  if (edit_model) {
+    model_.reset(new OmniboxPopupModel(this, edit_model));
+    theme_service_ = GtkThemeService::GetFrom(edit_model->profile());
+  }
+}
+
+OmniboxPopupViewGtk::~OmniboxPopupViewGtk() {
+  // Stop listening to our signals before we destroy the model. I suspect that
+  // we can race window destruction, otherwise.
+  signal_registrar_.reset();

[Evan Stade, 2013/08/15 18:57:29]

This should not be necessary. I realize you're just moving this code, but it
still doesn't make sense. I've just created a patch to fix this:

https://codereview.chromium.org/23257002/

+
+  // Explicitly destroy our model here, before we destroy our GTK widgets.
+  // This is because the model destructor can call back into us, and we need
+  // to make sure everything is still valid when it does.
+  model_.reset();
+  // layout_ may be NULL in unit tests.
+  if (layout_) {
+    g_object_unref(layout_);
+    gtk_widget_destroy(window_);
+  }
+}
+
+void OmniboxPopupViewGtk::Init() {
   gtk_widget_set_can_focus(window_, FALSE);
   // Don't allow the window to be resized.  This also forces the window to
   // shrink down to the size of its child contents.
@@ -325,19 +246,6 @@ OmniboxPopupViewGtk::OmniboxPopupViewGtk(const gfx::Font& font,
   // on. http://crbug.com/22015.
 }
 
-OmniboxPopupViewGtk::~OmniboxPopupViewGtk() {
-  // Stop listening to our signals before we destroy the model. I suspect that
-  // we can race window destruction, otherwise.
-  signal_registrar_.reset();
-
-  // Explicitly destroy our model here, before we destroy our GTK widgets.
-  // This is because the model destructor can call back into us, and we need
-  // to make sure everything is still valid when it does.
-  model_.reset();
-  g_object_unref(layout_);
-  gtk_widget_destroy(window_);
-}
-
 bool OmniboxPopupViewGtk::IsOpen() const {
   return opened_;
 }
@@ -352,13 +260,14 @@ void OmniboxPopupViewGtk::InvalidateLine(size_t line) {
 }
 
 void OmniboxPopupViewGtk::UpdatePopupAppearance() {
-  const AutocompleteResult& result = model_->result();
-  if (result.empty()) {
+  const AutocompleteResult& result = GetResult();
+  const size_t hidden_matches = GetHiddenMatchCount();
+  if (result.size() <= hidden_matches) {
     Hide();
     return;
   }
 
-  Show(result.size());
+  Show(result.size() - hidden_matches);
   gtk_widget_queue_draw(window_);
 }
 
@@ -433,6 +342,121 @@ void OmniboxPopupViewGtk::Observe(int type,
   gtk_widget_modify_bg(window_, GTK_STATE_NORMAL, &background_color_);
 }
 
+size_t OmniboxPopupViewGtk::LineFromY(int y) const {
+  size_t line = std::max(y - kBorderThickness, 0) / kHeightPerResult;
+  return std::min(line + GetHiddenMatchCount(), GetResult().size() - 1);
+}
+
+gfx::Rect OmniboxPopupViewGtk::GetRectForLine(size_t line, int width) const {
+  size_t visible_line = line - GetHiddenMatchCount();
+  return gfx::Rect(kBorderThickness,
+                   (visible_line * kHeightPerResult) + kBorderThickness,
+                   width - (kBorderThickness * 2),
+                   kHeightPerResult);
+}
+
+size_t OmniboxPopupViewGtk::GetHiddenMatchCount() const {
+  return GetResult().ShouldHideTopMatch() ? 1 : 0;
+}
+
+const AutocompleteResult& OmniboxPopupViewGtk::GetResult() const {
+  return model_->result();
+}
+
+// static
+void OmniboxPopupViewGtk::SetupLayoutForMatch(
+    PangoLayout* layout,
+    const string16& text,
+    const AutocompleteMatch::ACMatchClassifications& classifications,
+    const GdkColor* base_color,
+    const GdkColor* dim_color,
+    const GdkColor* url_color,
+    const std::string& prefix_text) {
+  // In RTL, mark text with left-to-right embedding mark if there is no strong
+  // RTL characters inside it, so the ending punctuation displays correctly
+  // and the eliding ellipsis displays correctly. We only mark the text with
+  // LRE. Wrapping it with LRE and PDF by calling AdjustStringForLocaleDirection
+  // or WrapStringWithLTRFormatting will render the elllipsis at the left of the
+  // elided pure LTR text.
+  bool marked_with_lre = false;
+  string16 localized_text = text;
+  // Pango is really easy to overflow and send into a computational death
+  // spiral that can corrupt the screen. Assume that we'll never have more than
+  // 2000 characters, which should be a safe assumption until we all get robot
+  // eyes. http://crbug.com/66576
+  if (localized_text.length() > 2000)
+    localized_text = localized_text.substr(0, 2000);
+  bool is_rtl = base::i18n::IsRTL();
+  if (is_rtl && !base::i18n::StringContainsStrongRTLChars(localized_text)) {
+    localized_text.insert(0, 1, base::i18n::kLeftToRightEmbeddingMark);
+    marked_with_lre = true;
+  }
+
+  // We can have a prefix, or insert additional characters while processing the
+  // classifications.  We need to take this in to account when we translate the
+  // UTF-16 offsets in the classification into text_utf8 byte offsets.
+  size_t additional_offset = prefix_text.length();  // Length in utf-8 bytes.
+  std::string text_utf8 = prefix_text + UTF16ToUTF8(localized_text);
+
+  PangoAttrList* attrs = pango_attr_list_new();
+
+  // TODO(deanm): This is a hack, just to handle coloring prefix_text.
+  // Hopefully I can clean up the match situation a bit and this will
+  // come out cleaner.  For now, apply the base color to the whole text
+  // so that our prefix will have the base color applied.
+  PangoAttribute* base_fg_attr = pango_attr_foreground_new(
+      base_color->red, base_color->green, base_color->blue);
+  pango_attr_list_insert(attrs, base_fg_attr);  // Ownership taken.
+
+  // Walk through the classifications, they are linear, in order, and should
+  // cover the entire text.  We create a bunch of overlapping attributes,
+  // extending from the offset to the end of the string.  The ones created
+  // later will override the previous ones, meaning we will still setup each
+  // portion correctly, we just don't need to compute the end offset.
+  for (ACMatchClassifications::const_iterator i = classifications.begin();
+       i != classifications.end(); ++i) {
+    size_t offset = GetUTF8Offset(localized_text, i->offset) +
+                    additional_offset;
+
+    // TODO(deanm): All the colors should probably blend based on whether this
+    // result is selected or not.  This would include the green URLs.  Right
+    // now the caller is left to blend only the base color.  Do we need to
+    // handle things like DIM urls?  Turns out DIM means something different
+    // than you'd think, all of the description text is not DIM, it is a
+    // special case that is not very common, but we should figure out and
+    // support it.
+    const GdkColor* color = base_color;
+    if (i->style & ACMatchClassification::URL) {
+      color = url_color;
+      // Insert a left to right embedding to make sure that URLs are shown LTR.
+      if (is_rtl && !marked_with_lre) {
+        std::string lre(kLRE);
+        text_utf8.insert(offset, lre);
+        additional_offset += lre.length();
+      }
+    }
+
+    if (i->style & ACMatchClassification::DIM)
+      color = dim_color;
+
+    PangoAttribute* fg_attr = pango_attr_foreground_new(
+        color->red, color->green, color->blue);
+    fg_attr->start_index = offset;
+    pango_attr_list_insert(attrs, fg_attr);  // Ownership taken.
+
+    // Matched portions are bold, otherwise use the normal weight.
+    PangoWeight weight = (i->style & ACMatchClassification::MATCH) ?
+        PANGO_WEIGHT_BOLD : PANGO_WEIGHT_NORMAL;
+    PangoAttribute* weight_attr = pango_attr_weight_new(weight);
+    weight_attr->start_index = offset;
+    pango_attr_list_insert(attrs, weight_attr);  // Ownership taken.
+  }
+
+  pango_layout_set_text(layout, text_utf8.data(), text_utf8.length());
+  pango_layout_set_attributes(layout, attrs);  // Ref taken.
+  pango_attr_list_unref(attrs);
+}
+
 void OmniboxPopupViewGtk::Show(size_t num_results) {
   gint origin_x, origin_y;
   GdkWindow* gdk_window = gtk_widget_get_window(location_bar_);
@@ -466,17 +490,12 @@ void OmniboxPopupViewGtk::StackWindow() {
   ui::StackPopupWindow(window_, toplevel);
 }
 
-size_t OmniboxPopupViewGtk::LineFromY(int y) {
-  size_t line = std::max(y - kBorderThickness, 0) / kHeightPerResult;
-  return std::min(line, model_->result().size() - 1);
-}
-
 void OmniboxPopupViewGtk::AcceptLine(size_t line,
                                      WindowOpenDisposition disposition) {
   // OpenMatch() may close the popup, which will clear the result set and, by
   // 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(line);
+  AutocompleteMatch match = GetResult().match_at(line);
   omnibox_view_->OpenMatch(match, disposition, GURL(), line);
 }
 
@@ -526,7 +545,7 @@ void OmniboxPopupViewGtk::GetVisibleMatchForInput(
     size_t index,
     const AutocompleteMatch** match,
     bool* is_selected_keyword) {
-  const AutocompleteResult& result = model_->result();
+  const AutocompleteResult& result = GetResult();
 
   if (result.match_at(index).associated_keyword.get() &&
       model_->selected_line() == index &&
@@ -590,7 +609,7 @@ gboolean OmniboxPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
 gboolean OmniboxPopupViewGtk::HandleExpose(GtkWidget* widget,
                                            GdkEventExpose* event) {
   bool ltr = !base::i18n::IsRTL();
-  const AutocompleteResult& result = model_->result();
+  const AutocompleteResult& result = GetResult();
 
   gfx::Rect window_rect = GetWindowRect(event->window);
   gfx::Rect damage_rect = gfx::Rect(event->area);
@@ -617,7 +636,7 @@ gboolean OmniboxPopupViewGtk::HandleExpose(GtkWidget* widget,
 
   pango_layout_set_height(layout_, kHeightPerResult * PANGO_SCALE);
 
-  for (size_t i = 0; i < result.size(); ++i) {
+  for (size_t i = GetHiddenMatchCount(); i < result.size(); ++i) {
     gfx::Rect line_rect = GetRectForLine(i, window_rect.width());
     // Only repaint and layout damaged lines.
     if (!line_rect.Intersects(damage_rect))