[AiS] for desktop, two lines and font sytles

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

 // Copyright (c) 2012 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/omnibox/omnibox_result_view.h"
 
 #include <algorithm>  // NOLINT
 
 #include "base/i18n/bidi_line_iterator.h"
 #include "base/memory/scoped_vector.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/bitmap_fetcher/bitmap_fetcher_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/omnibox/omnibox_popup_model.h"
 #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"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/range/range.h"
 #include "ui/gfx/render_text.h"
 #include "ui/gfx/text_utils.h"
 #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.
 // This is a separate class instead of being implemented on OmniboxResultView
 // because BitmapFetcherService currently takes ownership of this object.
 // TODO(dschuyler): Make BitmapFetcherService use the more typical non-owning
 // ObserverList pattern and have OmniboxResultView implement the Observer call
 // directly.
 class AnswerImageObserver : public BitmapFetcherService::Observer {
  public:
@@ skipping 10 matching lines @@
 };
 
 void AnswerImageObserver::OnImageChanged(
     BitmapFetcherService::RequestId request_id,
     const SkBitmap& image) {
   DCHECK(!image.empty());
   if (view_)
     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 {
   ui::NativeTheme::ColorId id;
   OmniboxResultView::ResultViewState state;
   OmniboxResultView::ColorKind kind;
 } static const kTranslationTable[] = {
   { NativeTheme::kColorId_ResultsTableNormalBackground,
     OmniboxResultView::NORMAL, OmniboxResultView::BACKGROUND },
   { NativeTheme::kColorId_ResultsTableHoveredBackground,
@@ skipping 19 matching lines @@
   { NativeTheme::kColorId_ResultsTableSelectedUrl,
     OmniboxResultView::SELECTED, OmniboxResultView::URL },
   { NativeTheme::kColorId_ResultsTableNormalDivider,
     OmniboxResultView::NORMAL, OmniboxResultView::DIVIDER },
   { NativeTheme::kColorId_ResultsTableHoveredDivider,
     OmniboxResultView::HOVERED, OmniboxResultView::DIVIDER },
   { NativeTheme::kColorId_ResultsTableSelectedDivider,
     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
 
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxResultView, public:
 
 // This class is a utility class for calculations affected by whether the result
 // view is horizontally mirrored.  The drawing functions can be written as if
 // all drawing occurs left-to-right, and then use this class to get the actual
 // coordinates to begin drawing onscreen.
 class OmniboxResultView::MirroringContext {
@@ skipping 27 matching lines @@
 
   DISALLOW_COPY_AND_ASSIGN(MirroringContext);
 };
 
 OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
                                      int model_index,
                                      LocationBarView* location_bar_view,
                                      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),
       image_service_(BitmapFetcherServiceFactory::GetForBrowserContext(
           location_bar_view_->profile())),
       font_list_(font_list),
       font_height_(
           std::max(font_list.GetHeight(),
                    font_list.DeriveWithStyle(gfx::Font::BOLD).GetHeight())),
       mirroring_context_(new MirroringContext()),
@@ skipping 70 matching lines @@
 void OmniboxResultView::Invalidate() {
   keyword_icon_->SetImage(GetKeywordIcon());
   // While the text in the RenderTexts may not have changed, the styling
   // (color/bold) may need to change. So we reset them to cause them to be
   // recomputed in OnPaint().
   ResetRenderTexts();
   SchedulePaint();
 }
 
 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());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxResultView, protected:
 
 OmniboxResultView::ResultViewState OmniboxResultView::GetState() const {
   if (model_->IsSelectedIndex(model_index_))
     return SELECTED;
   return model_->IsHoveredIndex(model_index_) ? HOVERED : NORMAL;
 }
@@ skipping 20 matching lines @@
   int contents_max_width, description_max_width;
   OmniboxPopupModel::ComputeMatchMaxWidths(
       contents->GetContentWidth(),
       separator_width_,
       description ? description->GetContentWidth() : 0,
       mirroring_context_->remaining_width(x),
       !AutocompleteMatch::IsSearchType(match.type),
       &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,
                    description_max_width);
   }
 }
 
 int OmniboxResultView::DrawRenderText(
     const AutocompleteMatch& match,
     gfx::RenderText* render_text,
@@ skipping 50 matching lines @@
       const int start_offset = std::max(prefix_width,
           std::min(remaining_width - max_match_contents_width, offset));
       right_x = x + std::min(remaining_width, start_offset + max_width);
       x += start_offset;
       prefix_x = x - prefix_width;
     }
     prefix_render_text->SetDirectionalityMode(is_match_contents_rtl ?
         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;
 }
 
 scoped_ptr<gfx::RenderText> OmniboxResultView::CreateRenderText(
     const base::string16& text) const {
   scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateInstance());
   render_text->SetDisplayRect(gfx::Rect(gfx::Size(INT_MAX, 0)));
   render_text->SetCursorEnabled(false);
   render_text->SetElideBehavior(gfx::ELIDE_TAIL);
@@ skipping 145 matching lines @@
   if (!contents_rendertext_) {
     contents_rendertext_.reset(
         CreateClassifiedRenderText(
             match_.contents, match_.contents_class, false).release());
   }
 }
 
 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_;
 
   if (match_.associated_keyword.get()) {
     const int kw_collapsed_size =
         keyword_icon_->width() + edge_item_padding_;
     const int max_kw_x = width() - kw_collapsed_size;
     const int kw_x =
         animation_->CurrentValueBetween(max_kw_x, edge_item_padding_);
@@ skipping 24 matching lines @@
   if (!ShowOnlyKeywordMatch()) {
     canvas->DrawImageInt(GetIcon(), GetMirroredXForRect(icon_bounds_),
                          icon_bounds_.y());
     int x = GetMirroredXForRect(text_bounds_);
     mirroring_context_->Initialize(x, text_bounds_.width());
     InitContentsRenderTextIfNecessary();
 
     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);
       }
     }
     PaintMatch(match_, contents_rendertext_.get(),
                description_rendertext_.get(), canvas, x);
   }
 
   AutocompleteMatch* keyword_match = match_.associated_keyword.get();
@@ skipping 15 matching lines @@
     }
     PaintMatch(*keyword_match, keyword_contents_rendertext_.get(),
                keyword_description_rendertext_.get(), canvas, x);
   }
 }
 
 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);
+}