[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;
@@ skipping 65 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 },
 };
 
+struct TextStyles {
+  ui::ResourceBundle::FontStyle font;
+  NativeTheme::ColorId color;
+  NativeTheme::ColorId color_selected;
+  gfx::BaselineStyle baseline;
+} const kTextStyles[] = {
+    // 1  ANSWER_TEXT
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNormalText,
+     NativeTheme::kColorId_ResultsTableSelectedText,
+     gfx::NORMAL_BASELINE},
+    // 2  HEADLINE_TEXT
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::NORMAL_BASELINE},
+    // 3  TOP_ALIGNED_TEXT
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::SUPERIOR},
+    // 4  DESCRIPTION_TEXT
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::NORMAL_BASELINE},
+    // 5  DESCRIPTION_TEXT_NEGATIVE
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNegativeText,

[Peter Kasting, 2015/03/14 02:12:25]

Your original CL used a dimmer shade of red/green for this pair.  Did you want
to implement something like kColorId_ResultsTableNegative[Normal,Dimmed]Text or
something so you can get those shades in, or are you happy using just bright red
and bright green?  I think since you're having to add custom colors here anyway
either is acceptable.

[dschuyler, 2015/03/16 21:49:09]

That may come up again.  Like the comment about the purple text below, I'd like
to keep it more simple for now.  I imagine that UI may have several alterations
requested after seeing the basic version of AiS for desktop.  Also, I haven't
found cases where the different colors are used in practice.  I'd like to follow
up on this after this CL.

+     NativeTheme::kColorId_ResultsTableNegativeSelectedText,
+     gfx::INFERIOR},
+    // 6  DESCRIPTION_TEXT_POSITIVE
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTablePositiveText,
+     NativeTheme::kColorId_ResultsTablePositiveSelectedText,
+     gfx::INFERIOR},
+    // 7  MORE_INFO_TEXT
+    {ui::ResourceBundle::SmallFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::NORMAL_BASELINE},
+    // 8  SUGGESTION_TEXT
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNormalText,
+     NativeTheme::kColorId_ResultsTableSelectedText,
+     gfx::NORMAL_BASELINE},
+    // 9  SUGGESTION_TEXT_POSITIVE
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTablePositiveText,
+     NativeTheme::kColorId_ResultsTablePositiveSelectedText,
+     gfx::NORMAL_BASELINE},
+    // 10 SUGGESTION_TEXT_NEGATIVE
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNegativeText,
+     NativeTheme::kColorId_ResultsTableNegativeSelectedText,
+     gfx::NORMAL_BASELINE},
+    // 11 SUGGESTION_LINK_COLOR
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNormalUrl,
+     NativeTheme::kColorId_ResultsTableSelectedUrl,
+     gfx::NORMAL_BASELINE},
+    // 12 STATUS_TEXT
+    {ui::ResourceBundle::LargeFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,
+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::INFERIOR},
+    // 13 PERSONALIZED_SUGGESTION_TEXT
+    {ui::ResourceBundle::BaseFont,
+     NativeTheme::kColorId_ResultsTableNormalDimmedText,

[Peter Kasting, 2015/03/14 02:12:25]

I think you want to use the same colors as SUGGESTION_TEXT here.

You may want to implement this by just shortening the style array and mapping 13
to 8 in GetTextStyle.  This will ensure that if we change the suggestion style
later the psuggestion style changes too.

[dschuyler, 2015/03/16 21:49:09]

I changed it to be the same color as suggestion text.

I'd like to make proposal that we keep the array entry because
+ it makes the code simpler
+ to the spec this color entry should be purple.  We've taken the liberty of
making it not purple, but design may come back with another color or re-request
purple.  Let's let UI weigh in before fully shorting out 13 to 8.

+     NativeTheme::kColorId_ResultsTableSelectedDimmedText,
+     gfx::NORMAL_BASELINE},
+};
+
+const TextStyles& GetTextStyle(int type) {
+  if (type > (int)arraysize(kTextStyles))

[Peter Kasting, 2015/03/14 01:54:14]

No C-style casts.

If you're going to take ints here (and in general plumb them through this whole
system), do you need to check for negative values, or has the parsing code
checked for those?

If the latter -- or if it _could_ be the latter -- consider making the
SuggestionAnswer::type filed a size_t instead, which would eliminate any need to
cast here.

[dschuyler, 2015/03/16 21:49:09]

Done.

+    type = 1;
+  // Subtract one because the types are one based (not zero based).
+  return kTextStyles[(type - 1)];

[Peter Kasting, 2015/03/14 01:54:14]

Nit: No {}

[dschuyler, 2015/03/16 21:49:09]

I'm reading that as No ()
Done.

+}
+
 }  // 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 118 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()) {
+        // Add one to draw all the way on to the bottom and left pixels.
+        // i.e. make the destination rect <= rather than strictly less than.
+        int answer_icon_size = 1 + GetAnswerLineHeight();

[Peter Kasting, 2015/03/14 01:54:14]

This seems wrong.  If the answer line height is, for example, 10 pixels, then
asking DrawImageInt to use a size of 10x10 is actually going to draw 10 pixels
high.  After all, we're passing a width and height, not right/bottom
coordinates.

Either I don't understand what you're doing, or you don't, or both :)

[dschuyler, 2015/03/16 21:49:09]

After some internal reflection, I think I simply felt that it looked nice with
the extra pixel line.  It's not necessary, so I've removed it.

Done.

+        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;
+        const base::string16 spacer(base::UTF8ToUTF16("  "));
         for (const auto& textfield : match_.answer->second_line().text_fields())
           text += textfield.text();
+        if (match_.answer->second_line().additional_text()) {
+          text += spacer;
+          text += match_.answer->second_line().additional_text()->text();
+        }
+        if (match_.answer->second_line().status_text())
+          text += match_.answer->second_line().status_text()->text();
         description_rendertext_ = CreateRenderText(text);
+        int text_offset = 0;
+        for (const auto& textfield :
+             match_.answer->second_line().text_fields()) {
+          text_offset = StyleAnswerText(description_rendertext_.get(),
+                                        text_offset, textfield);
+        }
+        if (match_.answer->second_line().additional_text())
+          text_offset = StyleAnswerText(
+              description_rendertext_.get(), text_offset + spacer.length(),
+              *match_.answer->second_line().additional_text());
+        if (match_.answer->second_line().status_text())
+          text_offset =
+              StyleAnswerText(description_rendertext_.get(), text_offset,
+                              *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::StyleAnswerText(
+    gfx::RenderText* render_text,
+    int offset,
+    const SuggestionAnswer::TextField& text_field) const {
+  int length = text_field.text().length();
+  gfx::Range range(offset, offset + length);
+  const TextStyles& text_style = GetTextStyle(text_field.type());
+  render_text->SetFontList(
+      ui::ResourceBundle::GetSharedInstance().GetFontList(text_style.font));
+  render_text->ApplyColor(
+      GetState() == SELECTED

[Peter Kasting, 2015/03/14 01:54:14]

Don't you want to deal with HOVERED here too?  After all, you added color
constants for it.

[dschuyler, 2015/03/16 21:49:09]

I wasn't sure why the existing code sometimes ignored hovered.  I've gone ahead
and added them.
Done.

+          ? GetNativeTheme()->GetSystemColor(text_style.color_selected)
+          : GetNativeTheme()->GetSystemColor(text_style.color),
+      range);
+  render_text->ApplyBaselineStyle(text_style.baseline, range);
+  return offset + length;
+}
+
+int OmniboxResultView::GetAnswerLineHeight() const {
+  // GetTextStyle(1) is (one of) the largest fonts used, so it's a good
+  // reference for line height.

[Peter Kasting, 2015/03/14 01:54:14]

Isn't it more that (1) is used in basically every answer, and that it's intended
to be the size of the whole line, with all other text either using the same font
face or fitted into (1)'s height as subscript/superscript/whatever?

(Also, do you actually render, say, superscript as possibly extending above the
parent font height?  If so, isn't this potentially wrong?)

[dschuyler, 2015/03/16 21:49:09]

That sounds correct.  I think that's saying something very similar.  I'll change
the wording a bit.
When adding baseline styles to RenderText I felt that would be 'proper' to add
the four main baseline styles of superscript, superior, inferior, and subscript.
 In practice though, the answers only really use superior and inferior script
styles.

+  return ui::ResourceBundle::GetSharedInstance()
+      .GetFontList(GetTextStyle(1).font)
+      .GetHeight();
+}
+
+int OmniboxResultView::GetContentLineHeight() const {
+  return std::max(default_icon_size_ + (kMinimumIconVerticalPadding * 2),
+                  GetTextHeight() + (minimum_text_vertical_padding_ * 2));
+}