[Omnibox] remove/reduce text_height and font_list parameters

This Cl removes the text_height parameter from the match cell ctor as
suggested in https://chromium-review.googlesource.com/c/chromium/src/+/1048865
It further removes other unnecessary passing of text_height or font_list
parameters.

Bug: None
Change-Id: Ifd6eaf9b113d09ff2e18b7e780ad50887e78e850
Reviewed-on: https://chromium-review.googlesource.com/1050869Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557616}

chrome/browser/ui/views/omnibox/omnibox_match_cell_view.cc

@@ -151,12 +151,10 @@ class OmniboxImageView : public views::ImageView {
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxMatchCellView:
 
-OmniboxMatchCellView::OmniboxMatchCellView(OmniboxResultView* result_view,
-                                           int text_height)
+OmniboxMatchCellView::OmniboxMatchCellView(OmniboxResultView* result_view)
     : is_old_style_answer_(false),
       is_rich_suggestion_(false),
-      is_search_type_(false),
-      text_height_(text_height) {
+      is_search_type_(false) {
   AddChildView(icon_view_ = new OmniboxImageView());
   AddChildView(image_view_ = new OmniboxImageView());
   AddChildView(content_view_ = new OmniboxTextView(result_view));
@@ -171,8 +169,9 @@ OmniboxMatchCellView::OmniboxMatchCellView(OmniboxResultView* result_view,
 OmniboxMatchCellView::~OmniboxMatchCellView() = default;
 
 gfx::Size OmniboxMatchCellView::CalculatePreferredSize() const {
-  int height = text_height_ +
-               GetVerticalInsets(text_height_, is_old_style_answer_).height();
+  const int text_height = content_view_->GetLineHeight();
+  int height = text_height +
+               GetVerticalInsets(text_height, is_old_style_answer_).height();
   if (is_rich_suggestion_ || is_old_style_answer_) {
     height += GetDescriptionHeight();
   }
@@ -242,14 +241,15 @@ void OmniboxMatchCellView::Layout() {
 }
 
 void OmniboxMatchCellView::LayoutOldStyleAnswer() {
+  const int text_height = content_view_->GetLineHeight();
   int x = GetIconAlignmentOffset() + HorizontalPadding();
-  int y = GetVerticalInsets(text_height_, /*is_old_style_answer=*/true).top();
+  int y = GetVerticalInsets(text_height, /*is_old_style_answer=*/true).top();
   icon_view_->SetSize(icon_view_->CalculatePreferredSize());
   icon_view_->SetPosition(
-      gfx::Point(x, y + (text_height_ - icon_view_->height()) / 2));
+      gfx::Point(x, y + (text_height - icon_view_->height()) / 2));
   x += icon_view_->width() + HorizontalPadding();
-  content_view_->SetBounds(x, y, width() - x, text_height_);
-  y += text_height_;
+  content_view_->SetBounds(x, y, width() - x, text_height);
+  y += text_height;
   if (image_view_->visible()) {
     // The description may be multi-line. Using the view height results in
     // an image that's too large, so we use the line height here instead.
@@ -267,14 +267,15 @@ void OmniboxMatchCellView::LayoutOldStyleAnswer() {
 }
 
 void OmniboxMatchCellView::LayoutRichSuggestion() {
+  const int text_height = content_view_->GetLineHeight();
   int x = GetIconAlignmentOffset() + HorizontalPadding();
-  int y = GetVerticalInsets(text_height_, /*is_old_style_answer=*/false).top();
+  int y = GetVerticalInsets(text_height, /*is_old_style_answer=*/false).top();
   image_view_->SetImageSize(gfx::Size(kRichImageSize, kRichImageSize));
-  image_view_->SetBounds(x, y + (text_height_ * 2 - kRichImageSize) / 2,
+  image_view_->SetBounds(x, y + (text_height * 2 - kRichImageSize) / 2,
                          kRichImageSize, kRichImageSize);
   x += kRichImageSize + HorizontalPadding();
-  content_view_->SetBounds(x, y, width() - x, text_height_);
-  y += text_height_;
+  content_view_->SetBounds(x, y, width() - x, text_height);
+  y += text_height;
   int description_width = width() - x;
   description_view_->SetBounds(
       x, y, description_width,
@@ -283,11 +284,12 @@ void OmniboxMatchCellView::LayoutRichSuggestion() {
 }
 
 void OmniboxMatchCellView::LayoutSplit() {
+  const int text_height = content_view_->GetLineHeight();
   int x = GetIconAlignmentOffset() + HorizontalPadding();
   icon_view_->SetSize(icon_view_->CalculatePreferredSize());
-  int y = GetVerticalInsets(text_height_, /*is_old_style_answer=*/false).top();
+  int y = GetVerticalInsets(text_height, /*is_old_style_answer=*/false).top();
   icon_view_->SetPosition(
-      gfx::Point(x, y + (text_height_ - icon_view_->height()) / 2));
+      gfx::Point(x, y + (text_height - icon_view_->height()) / 2));
   x += icon_view_->width() + HorizontalPadding();
   int content_width = content_view_->CalculatePreferredSize().width();
   int description_width = description_view_->CalculatePreferredSize().width();
@@ -296,13 +298,13 @@ void OmniboxMatchCellView::LayoutSplit() {
       content_width, separator_size.width(), description_width, width() - x,
       /*description_on_separate_line=*/false, !is_search_type_, &content_width,
       &description_width);
-  content_view_->SetBounds(x, y, content_width, text_height_);
+  content_view_->SetBounds(x, y, content_width, text_height);
   if (description_width != 0) {
     x += content_view_->width();
     separator_view_->SetSize(separator_size);
-    separator_view_->SetBounds(x, y, separator_view_->width(), text_height_);
+    separator_view_->SetBounds(x, y, separator_view_->width(), text_height);
     x += separator_view_->width();
-    description_view_->SetBounds(x, y, description_width, text_height_);
+    description_view_->SetBounds(x, y, description_width, text_height);
   } else {
     description_view_->SetSize(gfx::Size());
     separator_view_->SetSize(gfx::Size());

chrome/browser/ui/views/omnibox/omnibox_match_cell_view.h

@@ -17,8 +17,7 @@ class OmniboxTextView;
 
 class OmniboxMatchCellView : public views::View {
  public:
-  explicit OmniboxMatchCellView(OmniboxResultView* result_view,
-                                int text_height);
+  explicit OmniboxMatchCellView(OmniboxResultView* result_view);
   ~OmniboxMatchCellView() override;
 
   views::ImageView* icon() { return icon_view_; }
@@ -49,7 +48,6 @@ class OmniboxMatchCellView : public views::View {
   bool is_old_style_answer_;
   bool is_rich_suggestion_;
   bool is_search_type_;
-  int text_height_;
 
   // Weak pointers for easy reference.
   views::ImageView* icon_view_;   // An icon representing the type or content.

chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc

@@ -210,14 +210,12 @@ class OmniboxPopupContentsView::AutocompletePopupWidget
 // OmniboxPopupContentsView, public:
 
 OmniboxPopupContentsView::OmniboxPopupContentsView(
-    const gfx::FontList& font_list,
     OmniboxView* omnibox_view,
     OmniboxEditModel* edit_model,
     LocationBarView* location_bar_view)
     : model_(new OmniboxPopupModel(this, edit_model)),
       omnibox_view_(omnibox_view),
       location_bar_view_(location_bar_view),
-      font_list_(font_list),
       start_margin_(0),
       end_margin_(0) {
   // The contents is owned by the LocationBarView.
@@ -227,7 +225,7 @@ OmniboxPopupContentsView::OmniboxPopupContentsView(
     InitializeWideShadows();
 
   for (size_t i = 0; i < AutocompleteResult::GetMaxMatches(); ++i) {
-    OmniboxResultView* result_view = new OmniboxResultView(this, i, font_list_);
+    OmniboxResultView* result_view = new OmniboxResultView(this, i);
     result_view->SetVisible(false);
     AddChildViewAt(result_view, static_cast<int>(i));
   }

chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h

@@ -26,8 +26,7 @@ class OmniboxView;
 // A view representing the contents of the autocomplete popup.
 class OmniboxPopupContentsView : public views::View, public OmniboxPopupView {
  public:
-  OmniboxPopupContentsView(const gfx::FontList& font_list,
-                           OmniboxView* omnibox_view,
+  OmniboxPopupContentsView(OmniboxView* omnibox_view,
                            OmniboxEditModel* edit_model,
                            LocationBarView* location_bar_view);
   ~OmniboxPopupContentsView() override;
@@ -123,9 +122,6 @@ class OmniboxPopupContentsView : public views::View, public OmniboxPopupView {
 
   LocationBarView* location_bar_view_;
 
-  // The font list used for result rows, based on the omnibox font list.
-  gfx::FontList font_list_;
-
   int start_margin_;
   int end_margin_;
 

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

@@ -44,7 +44,6 @@
 #include "ui/gfx/paint_vector_icon.h"
 
 namespace {
-
 // The vertical padding to provide each RenderText in addition to the height of
 // the font. Where possible, RenderText uses this additional space to vertically
 // center the cap height of the font instead of centering the entire font.
@@ -85,20 +84,15 @@ int HorizontalPadding() {
 // OmniboxResultView, public:
 
 OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
-                                     int model_index,
-                                     const gfx::FontList& font_list)
+                                     int model_index)
     : model_(model),
       model_index_(model_index),
       is_hovered_(false),
-      font_height_(std::max(
-          font_list.GetHeight(),
-          font_list.DeriveWithWeight(gfx::Font::Weight::BOLD).GetHeight())),
       animation_(new gfx::SlideAnimation(this)) {
   CHECK_GE(model_index, 0);
 
-  AddChildView(suggestion_view_ =
-                   new OmniboxMatchCellView(this, GetTextHeight()));
-  AddChildView(keyword_view_ = new OmniboxMatchCellView(this, GetTextHeight()));
+  AddChildView(suggestion_view_ = new OmniboxMatchCellView(this));
+  AddChildView(keyword_view_ = new OmniboxMatchCellView(this));
 
   keyword_view_->icon()->EnableCanvasFlippingForRTLUI(true);
   keyword_view_->icon()->SetImage(gfx::CreateVectorIcon(
@@ -122,8 +116,11 @@ void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
 
   // Set up 'switch to tab' button.
   if (match.has_tab_match && !match_.associated_keyword.get()) {
-    suggestion_tab_switch_button_ =
-        std::make_unique<OmniboxTabSwitchButton>(model_, this, GetTextHeight());
+    // TODO(dschuyler): the kVerticalPadding is added here, and again within
+    // OmniboxTabSwitchButton. Should that just be done once?
+    suggestion_tab_switch_button_ = std::make_unique<OmniboxTabSwitchButton>(
+        model_, this,
+        suggestion_view_->content()->GetLineHeight() + kVerticalPadding);
     suggestion_tab_switch_button_->set_owned_by_client();
     AddChildView(suggestion_tab_switch_button_.get());
   } else {
@@ -323,10 +320,6 @@ void OmniboxResultView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxResultView, private:
 
-int OmniboxResultView::GetTextHeight() const {
-  return font_height_ + kVerticalPadding;
-}
-
 gfx::Image OmniboxResultView::GetIcon() const {
   return model_->GetMatchIcon(match_, GetColor(OmniboxPart::RESULTS_ICON));
 }

chrome/browser/ui/views/omnibox/omnibox_result_view.h

@@ -38,9 +38,7 @@ class OmniboxResultView : public views::View,
                           private gfx::AnimationDelegate,
                           public views::ButtonListener {
  public:
-  OmniboxResultView(OmniboxPopupContentsView* model,
-                    int model_index,
-                    const gfx::FontList& font_list);
+  OmniboxResultView(OmniboxPopupContentsView* model, int model_index);
   ~OmniboxResultView() override;
 
   // Helper to get the color for |part| using the current state and tint.
@@ -112,9 +110,6 @@ class OmniboxResultView : public views::View,
   // Whether this view is in the hovered state.
   bool is_hovered_;
 
-  // Cache the font height as a minor optimization.
-  int font_height_;
-
   // The data this class is built to display (the "Omnibox Result").
   AutocompleteMatch match_;
 

chrome/browser/ui/views/omnibox/omnibox_result_view_unittest.cc

@@ -29,7 +29,10 @@ static constexpr int kTestResultViewIndex = 4;
 class TestOmniboxPopupContentsView : public OmniboxPopupContentsView {
  public:
   explicit TestOmniboxPopupContentsView(OmniboxEditModel* edit_model)
-      : OmniboxPopupContentsView(gfx::FontList(), nullptr, edit_model, nullptr),
+      : OmniboxPopupContentsView(
+            /*omnibox_view=*/nullptr,
+            edit_model,
+            /*location_bar_view=*/nullptr),
         selected_index_(0) {}
 
   void SetSelectedLine(size_t index) override { selected_index_ = index; }
@@ -55,8 +58,8 @@ class OmniboxResultViewTest : public views::ViewsTestBase {
         nullptr, nullptr, std::make_unique<TestOmniboxClient>());
     popup_view_ =
         std::make_unique<TestOmniboxPopupContentsView>(edit_model_.get());
-    result_view_ = new OmniboxResultView(popup_view_.get(),
-                                         kTestResultViewIndex, gfx::FontList());
+    result_view_ =
+        new OmniboxResultView(popup_view_.get(), kTestResultViewIndex);
 
     // Create a widget and assign bounds to support calls to HitTestPoint.
     widget_.reset(new views::Widget);

chrome/browser/ui/views/omnibox/omnibox_text_view.cc

@@ -233,21 +233,20 @@ void OmniboxTextView::SetText(const base::string16& text) {
 
 void OmniboxTextView::SetText(const SuggestionAnswer::ImageLine& line) {
   wrap_text_lines_ = line.num_text_lines() > 1;
-  // This assumes that the first text type in the line can be used to specify
-  // the font for all the text fields in the line.  For now this works but
-  // eventually it may be necessary to get RenderText to support multiple font
-  // sizes or use multiple RenderTexts.
   render_text_.reset();
-  render_text_ = CreateText(line, GetFontForType(line.text_fields()[0].type()));
+  render_text_ = CreateText(line);
   UpdateLineHeight();
 }
 
 std::unique_ptr<gfx::RenderText> OmniboxTextView::CreateText(
-    const SuggestionAnswer::ImageLine& line,
-    const gfx::FontList& font_list) const {
+    const SuggestionAnswer::ImageLine& line) const {
   std::unique_ptr<gfx::RenderText> destination =
       CreateRenderText(base::string16());
-  destination->SetFontList(font_list);
+  // This assumes that the first text type in the line can be used to specify
+  // the font for all the text fields in the line.  For now this works but
+  // eventually it may be necessary to get RenderText to support multiple font
+  // sizes or use multiple RenderTexts.
+  destination->SetFontList(GetFontForType(line.text_fields()[0].type()));
 
   for (const SuggestionAnswer::TextField& text_field : line.text_fields())
     AppendText(destination.get(), text_field.text(), text_field.type());

chrome/browser/ui/views/omnibox/omnibox_text_view.h

@@ -63,8 +63,7 @@ class OmniboxTextView : public views::View {
 
   // Creates a RenderText with text and styling from the image line.
   std::unique_ptr<gfx::RenderText> CreateText(
-      const SuggestionAnswer::ImageLine& line,
-      const gfx::FontList& font_list) const;
+      const SuggestionAnswer::ImageLine& line) const;
 
   // Adds |text| to |destination|.  |text_type| is an index into the
   // kTextStyles constant defined in the .cc file and is used to style the text,

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -159,8 +159,8 @@ void OmniboxViewViews::Init() {
 
   if (location_bar_view_) {
     // Initialize the popup view using the same font.
-    popup_view_.reset(new OmniboxPopupContentsView(GetFontList(), this, model(),
-                                                   location_bar_view_));
+    popup_view_.reset(
+        new OmniboxPopupContentsView(this, model(), location_bar_view_));
   }
 
   // Override the default FocusableBorder from Textfield, since the