[AiS] for desktop, two lines and font sytles

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 e6f67e2adc2cfc82516f86d75f0553b173b42485..46f57a042f675a47f90dd9a01840c28b5a503b3b 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -23,10 +23,13 @@
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/omnibox/omnibox_field_trial.h"
 #include "components/omnibox/suggestion_answer.h"
 #include "grit/components_scaled_resources.h"
 #include "grit/theme_resources.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_utils.h"
@@ -37,7 +40,13 @@
 #include "ui/native_theme/native_theme.h"
 
 using ui::NativeTheme;
-
+struct TextStyle {
+  ui::ResourceBundle::FontStyle font;
+  NativeTheme::ColorId color;

[Peter Kasting, 2015/03/17 22:01:44]

Here's an idea:

Instead of three member ColorIds, declare an array:

  NativeTheme::ColorId colors[OmniboxResultView::NUM_STATES];

Now you don't need a GetStateColor() method, because AppendAnswerText() can just
do:

  render_text->ApplyColor(text_style.colors[GetState()], range);

Then, since GetStateColor() was the only method taking a TextStyle, the header
no longer needs to forward-declare TextStyle, and the definition of TextStyle
here can move back into the anonymous namespace as you had it previously.

[dschuyler, 2015/03/17 22:57:55]

I've added a TODO aobut this.  

+  NativeTheme::ColorId color_hovered;
+  NativeTheme::ColorId color_selected;
+  gfx::BaselineStyle baseline;
+};
 namespace {
 
 // Calls back to the OmniboxResultView when the requested image is downloaded.
@@ -68,11 +77,6 @@ void AnswerImageObserver::OnImageChanged(
     view_->SetAnswerImage(gfx::ImageSkia::CreateFrom1xBitmap(image));
 }
 
-// The minimum distance between the top and bottom of the {icon|text} and the
-// top or bottom of the row.
-const int kMinimumIconVerticalPadding = 2;
-const int kMinimumTextVerticalPadding = 3;
-
 // A mapping from OmniboxResultView's ResultViewState/ColorKind types to
 // NativeTheme colors.
 struct TranslationTable {
@@ -112,6 +116,94 @@ struct TranslationTable {
     OmniboxResultView::SELECTED, OmniboxResultView::DIVIDER },
 };
 
+const TextStyle kTextStyles[] = {
+    // 1  ANSWER_TEXT
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNormalText,
+     NativeTheme::kColorId_ResultsTableHoveredText,
+     NativeTheme::kColorId_ResultsTableSelectedText,
+     gfx::NORMAL_BASELINE},
+    // 2  HEADLINE_TEXT
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableHoveredDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::NORMAL_BASELINE},
+    // 3  TOP_ALIGNED_TEXT
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableHoveredDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::SUPERIOR},
+    // 4  DESCRIPTION_TEXT
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableHoveredDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::NORMAL_BASELINE},
+    // 5  DESCRIPTION_TEXT_NEGATIVE
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNegativeText,
+     NativeTheme::kColorId_ResultsTableNegativeHoveredText,
+     NativeTheme::kColorId_ResultsTableNegativeSelectedText,
+     gfx::INFERIOR},
+    // 6  DESCRIPTION_TEXT_POSITIVE
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTablePositiveText,
+     NativeTheme::kColorId_ResultsTablePositiveHoveredText,
+     NativeTheme::kColorId_ResultsTablePositiveSelectedText,
+     gfx::INFERIOR},
+    // 7  MORE_INFO_TEXT
+    {ui::ResourceBundle::SmallFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableHoveredDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::NORMAL_BASELINE},
+    // 8  SUGGESTION_TEXT
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNormalText,
+     NativeTheme::kColorId_ResultsTableHoveredText,
+     NativeTheme::kColorId_ResultsTableSelectedText,
+     gfx::NORMAL_BASELINE},
+    // 9  SUGGESTION_TEXT_POSITIVE
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTablePositiveText,
+     NativeTheme::kColorId_ResultsTablePositiveHoveredText,
+     NativeTheme::kColorId_ResultsTablePositiveSelectedText,
+     gfx::NORMAL_BASELINE},
+    // 10 SUGGESTION_TEXT_NEGATIVE
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNegativeText,
+     NativeTheme::kColorId_ResultsTableNegativeHoveredText,
+     NativeTheme::kColorId_ResultsTableNegativeSelectedText,
+     gfx::NORMAL_BASELINE},
+    // 11 SUGGESTION_LINK_COLOR
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNormalUrl,
+     NativeTheme::kColorId_ResultsTableHoveredUrl,
+     NativeTheme::kColorId_ResultsTableSelectedUrl,
+     gfx::NORMAL_BASELINE},
+    // 12 STATUS_TEXT
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableHoveredDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::INFERIOR},
+    // 13 PERSONALIZED_SUGGESTION_TEXT
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNormalText,
+     NativeTheme::kColorId_ResultsTableHoveredText,
+     NativeTheme::kColorId_ResultsTableSelectedText,
+     gfx::NORMAL_BASELINE},
+};
+
+const TextStyle& GetTextStyle(size_t type) {
+  if (type > arraysize(kTextStyles))
+    type = 1;
+  // Subtract one because the types are one based (not zero based).
+  return kTextStyles[type - 1];
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -159,7 +251,6 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
                                      const gfx::FontList& font_list)
     : edge_item_padding_(LocationBarView::kItemPadding),
       item_padding_(LocationBarView::kItemPadding),
-      minimum_text_vertical_padding_(kMinimumTextVerticalPadding),
       model_(model),
       model_index_(model_index),
       location_bar_view_(location_bar_view),
@@ -250,9 +341,10 @@ void OmniboxResultView::Invalidate() {
 }
 
 gfx::Size OmniboxResultView::GetPreferredSize() const {
-  return gfx::Size(0, std::max(
-      default_icon_size_ + (kMinimumIconVerticalPadding * 2),
-      GetTextHeight() + (minimum_text_vertical_padding_ * 2)));
+  if (!match_.answer)
+    return gfx::Size(0, GetContentLineHeight());
+  // An answer implies a match and a description in a large font.
+  return gfx::Size(0, GetContentLineHeight() + GetAnswerLineHeight());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -293,21 +385,25 @@ void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
       &contents_max_width,
       &description_max_width);
 
-  x = DrawRenderText(match, contents, true, canvas, x, y, contents_max_width);
+  int after_contents_x =
+      DrawRenderText(match, contents, true, canvas, x, y, contents_max_width);
 
   if (description_max_width != 0) {
-    x = DrawRenderText(match, separator_rendertext_.get(), false, canvas, x, y,
-                       separator_width_);
-
-    if (!answer_image_.isNull()) {
-      canvas->DrawImageInt(answer_image_,
-                           // Source x, y, w, h.
-                           0, 0, answer_image_.width(), answer_image_.height(),
-                           // Destination x, y, w, h.
-                           GetMirroredXInView(x),
-                           y + kMinimumIconVerticalPadding, default_icon_size_,
-                           default_icon_size_, true);
-      x += default_icon_size_ + LocationBarView::kIconInternalPadding;
+    if (match.answer) {
+      y += GetContentLineHeight();
+      if (!answer_image_.isNull()) {
+        int answer_icon_size = GetAnswerLineHeight();
+        canvas->DrawImageInt(
+            answer_image_,
+            // Source x, y, w, h.
+            0, 0, answer_image_.width(), answer_image_.height(),
+            // Destination x, y, w, h.
+            GetMirroredXInView(x), y, answer_icon_size, answer_icon_size, true);
+        x += answer_icon_size + LocationBarView::kIconInternalPadding;
+      }
+    } else {
+      x = DrawRenderText(match, separator_rendertext_.get(), false, canvas,
+                         after_contents_x, y, separator_width_);
     }
 
     DrawRenderText(match, description, false, canvas, x, y,
@@ -378,17 +474,17 @@ int OmniboxResultView::DrawRenderText(
         gfx::DIRECTIONALITY_FORCE_RTL : gfx::DIRECTIONALITY_FORCE_LTR);
     prefix_render_text->SetHorizontalAlignment(
           is_match_contents_rtl ? gfx::ALIGN_RIGHT : gfx::ALIGN_LEFT);
-    prefix_render_text->SetDisplayRect(gfx::Rect(
-          mirroring_context_->mirrored_left_coord(
-              prefix_x, prefix_x + prefix_width), y,
-          prefix_width, height()));
+    prefix_render_text->SetDisplayRect(
+        gfx::Rect(mirroring_context_->mirrored_left_coord(
+                      prefix_x, prefix_x + prefix_width),
+                  y, prefix_width, GetContentLineHeight()));
     prefix_render_text->Draw(canvas);
   }
 
   // Set the display rect to trigger eliding.
-  render_text->SetDisplayRect(gfx::Rect(
-      mirroring_context_->mirrored_left_coord(x, right_x), y,
-      right_x - x, height()));
+  render_text->SetDisplayRect(
+      gfx::Rect(mirroring_context_->mirrored_left_coord(x, right_x), y,
+                right_x - x, GetContentLineHeight()));
   render_text->Draw(canvas);
   return right_x;
 }
@@ -554,10 +650,12 @@ void OmniboxResultView::InitContentsRenderTextIfNecessary() const {
 void OmniboxResultView::Layout() {
   const gfx::ImageSkia icon = GetIcon();
 
-  icon_bounds_.SetRect(edge_item_padding_ +
-      ((icon.width() == default_icon_size_) ?
-          0 : LocationBarView::kIconInternalPadding),
-      (height() - icon.height()) / 2, icon.width(), icon.height());
+  icon_bounds_.SetRect(
+      edge_item_padding_ + ((icon.width() == default_icon_size_)
+                                ? 0
+                                : LocationBarView::kIconInternalPadding),
+      (GetContentLineHeight() - icon.height()) / 2, icon.width(),
+      icon.height());
 
   int text_x = edge_item_padding_ + default_icon_size_ + item_padding_;
   int text_width = width() - text_x - edge_item_padding_;
@@ -602,9 +700,18 @@ void OmniboxResultView::OnPaint(gfx::Canvas* canvas) {
     if (!description_rendertext_) {
       if (match_.answer) {
         base::string16 text;
-        for (const auto& textfield : match_.answer->second_line().text_fields())
-          text += textfield.text();
         description_rendertext_ = CreateRenderText(text);
+        for (const SuggestionAnswer::TextField& textfield :
+             match_.answer->second_line().text_fields())
+          AppendAnswerText(description_rendertext_.get(), textfield);
+        if (match_.answer->second_line().additional_text()) {
+          AppendAnswerText(description_rendertext_.get(),
+                           *match_.answer->second_line().additional_text());
+        }
+        if (match_.answer->second_line().status_text()) {
+          AppendAnswerText(description_rendertext_.get(),
+                           *match_.answer->second_line().status_text());
+        }
       } else if (!match_.description.empty()) {
         description_rendertext_ = CreateClassifiedRenderText(
             match_.description, match_.description_class, true);
@@ -640,3 +747,40 @@ void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   Layout();
   SchedulePaint();
 }
+
+int OmniboxResultView::GetAnswerLineHeight() const {
+  // GetTextStyle(1) is the largest fonts used and so set the boundary that all

[Peter Kasting, 2015/03/17 22:01:44]

Nit: "....largest font used and so defines the..."

[dschuyler, 2015/03/17 22:57:55]

Done.

+  // the other answer styles fit within.
+  return ui::ResourceBundle::GetSharedInstance()
+      .GetFontList(GetTextStyle(1).font)
+      .GetHeight();

[Peter Kasting, 2015/03/17 22:01:44]

Nit: Can the result value here change at runtime, e.g. due to the user changing
their system default font or theme or something?  If not, you could cache this
in a static local to avoid computing it on every call.

[dschuyler, 2015/03/17 22:57:55]

I don't know if that varies by platform.  Can you recommend how I can find out? 
I also don't know how much the performance may vary for this call as is on
different platforms.  Is it better optimize now or get the basics working first?
 (I know it's a common rule of thumb to optimize later, but it can depend on the
circumstances).

[Peter Kasting, 2015/03/17 23:18:06]

The only ways I know of are trying to do code inspection and trying to test
Chrome to see what it does with system theme changes.  The problem with the
latter is that we probably get it wrong today, as we have a variety of bugs on
file about not respecting theme changes.

I did some code inspection.  I think the fonts can potentially change at runtime
on ChromeOS at least, so I would leave the code as-is.
 
The reason I suggested an optimization here is that it would be easy and you
call this function very frequently, and we've heard a lot of complaints about
omnibox rendering doing unnecessary work in the past.

Examining the code that implements this, it's not very expensive, so I'm not
worried.

[dschuyler, 2015/03/18 00:13:36]

Thanks for checking into that.

+}
+
+int OmniboxResultView::GetContentLineHeight() const {
+  return std::max(default_icon_size_ + (kMinimumIconVerticalPadding * 2),
+                  GetTextHeight() + (kMinimumTextVerticalPadding * 2));
+}
+
+SkColor OmniboxResultView::GetStateColor(const TextStyle& text_style) const {
+  switch (GetState()) {
+    case SELECTED:
+      return GetNativeTheme()->GetSystemColor(text_style.color_selected);
+    case HOVERED:
+      GetNativeTheme()->GetSystemColor(text_style.color_hovered);

[Peter Kasting, 2015/03/17 22:01:44]

Missing return here!

[dschuyler, 2015/03/17 22:57:55]

!
Done.

+    default:
+      return GetNativeTheme()->GetSystemColor(text_style.color);
+  }
+}
+
+void OmniboxResultView::AppendAnswerText(
+    gfx::RenderText* render_text,
+    const SuggestionAnswer::TextField& text_field) {
+  int offset = render_text->text().length();
+  gfx::Range range(offset, offset + text_field.text().length());
+  render_text->AppendText(text_field.text());
+  const TextStyle& text_style = GetTextStyle(text_field.type());
+  render_text->SetFontList(

[Peter Kasting, 2015/03/17 22:01:44]

Doesn't this change the font for the whole RenderText object?

Can RenderText even support multiple fonts?  If not maybe we need to use
different RenderText objects for the different pieces here.

[dschuyler, 2015/03/17 22:57:55]

Thanks for pointing that out.  I added a TODO to follow up on this.  It's not
currently showing issues, but it could in the future.  So it will be good to
follow up on.

+      ui::ResourceBundle::GetSharedInstance().GetFontList(text_style.font));
+  render_text->ApplyColor(GetStateColor(text_style), range);
+  render_text->ApplyBaselineStyle(text_style.baseline, range);
+}