Provide vertical padding to the RenderTexts used in suggestions.

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

Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
index 6d29e0a88eb076eed56a869c07f408f5eeb59282..599d209974a24357dd44770b2f4786d5308a0cee 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -50,9 +50,13 @@ using ui::NativeTheme;
 
 namespace {
 
-// The padding that should be placed between content and description in a
-// vertical layout.
-static const int kVerticalPadding = 3;
+// The vertical margin that should be used above and below each suggestion.
+static const int kVerticalMargin = 1;

[Peter Kasting, 2017/06/13 23:42:50]

So why a margin of 1 and padding of 4 instead of a margin of 0 and padding of 6?

[Justin Donnelly, 2017/06/15 18:56:53]

Because that won't work in the multi-part case (answers and/or vertical layout).
It would introduce more padding between the two parts than we want. (That's what
caused this bug in the first place, was me changing the layout so that we could
have a different amount of space between the two parts than we have between
suggestions.)

[Peter Kasting, 2017/06/15 22:43:34]

OK, but using 1 and 4 is sufficient for your needs?

[Justin Donnelly, 2017/06/16 14:14:30]

Yes. I've tested this extensively on Linux and Windows. (And I've discovered a
couple other Windows issues as a result. I guess I really need to be testing on
Windows more.)

+
+// 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.
+static const int kVerticalPadding = 4;

[Peter Kasting, 2017/06/13 23:42:50]

Nit: kAdditionalTextLineHeight?

Should this just be set as part of the line height for the RenderText used to
draw this, so it doesn't have to be added in manually?

[Justin Donnelly, 2017/06/15 18:56:53]

That would add additional, undesired space between lines in multi-line answers.

 
 // A mapping from OmniboxResultView's ResultViewState/ColorKind types to
 // NativeTheme colors.
@@ -303,9 +307,9 @@ void OmniboxResultView::OnSelected() {
 gfx::Size OmniboxResultView::CalculatePreferredSize() const {
   int height = GetTextHeight() + (2 * GetVerticalMargin());
   if (match_.answer)
-    height += GetAnswerHeight() + kVerticalPadding;
+    height += GetAnswerHeight();
   else if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout))
-    height += GetTextHeight() + kVerticalPadding;
+    height += GetTextHeight();
   return gfx::Size(0, height);
 }
 
@@ -332,7 +336,7 @@ OmniboxResultView::ResultViewState OmniboxResultView::GetState() const {
 }
 
 int OmniboxResultView::GetTextHeight() const {
-  return font_height_;
+  return font_height_ + kVerticalPadding;
 }
 
 void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
@@ -367,7 +371,7 @@ void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
   // Answers in Suggest results.
   if (match.answer && description_max_width != 0) {
     DrawRenderText(match, contents, CONTENTS, canvas, x, y, contents_max_width);
-    y += GetTextHeight() + kVerticalPadding;
+    y += GetTextHeight();
     if (!answer_image_.isNull()) {
       int answer_icon_size = GetAnswerHeight();
       canvas->DrawImageInt(answer_image_, 0, 0, answer_image_.width(),
@@ -389,12 +393,12 @@ void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
   if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {
     // For no description, shift down halfways to draw contents in middle.
     if (description_max_width == 0)
-      y += (GetTextHeight() + kVerticalPadding) / 2;
+      y += GetTextHeight() / 2;
 
     DrawRenderText(match, contents, CONTENTS, canvas, x, y, contents_max_width);
 
     if (description_max_width != 0) {
-      y += GetTextHeight() + kVerticalPadding;
+      y += GetTextHeight();
       DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
                      description_max_width);
     }
@@ -647,7 +651,7 @@ void OmniboxResultView::Layout() {
 
   int row_height = GetTextHeight();
   if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout))
-    row_height += kVerticalPadding + GetTextHeight();
+    row_height += match_.answer ? GetTextHeight() : GetAnswerHeight();
 
   const int icon_y = GetVerticalMargin() + (row_height - icon.height()) / 2;
   icon_bounds_.SetRect(start_x, icon_y, icon.width(), icon.height());
@@ -747,7 +751,7 @@ int OmniboxResultView::GetAnswerHeight() const {
   // If the answer specifies a maximum of 1 line we can simply return the answer
   // font height.
   if (match_.answer->second_line().num_text_lines() == 1)
-    return GetAnswerFont().GetHeight();
+    return GetAnswerFont().GetHeight() + kVerticalPadding;
 
   // Multi-line answers require layout in order to determine the number of lines
   // the RenderText will use.
@@ -757,7 +761,9 @@ int OmniboxResultView::GetAnswerHeight() const {
   }
   description_rendertext_->SetDisplayRect(gfx::Rect(text_bounds_.width(), 0));
   description_rendertext_->GetStringSize();
-  return GetAnswerFont().GetHeight() * description_rendertext_->GetNumLines();
+  return (GetAnswerFont().GetHeight() *
+          description_rendertext_->GetNumLines()) +
+         kVerticalPadding;
 }
 
 int OmniboxResultView::GetVerticalMargin() const {
@@ -771,7 +777,7 @@ int OmniboxResultView::GetVerticalMargin() const {
       Md::GetMode() == Md::MATERIAL_HYBRID ? 8 : 4);
   const int min_height = LocationBarView::kIconWidth + 2 * kIconVerticalPad;
 
-  return std::max(kVerticalPadding, (min_height - GetTextHeight()) / 2);
+  return std::max(kVerticalMargin, (min_height - GetTextHeight()) / 2);
 }
 
 std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateAnswerText(