[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..53d1b6f5779248e68b3f26e51e1f812e63a6eafc 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"
@@ -40,6 +43,10 @@ using ui::NativeTheme;
 
 namespace {
 
+// The minimum distance between the top and bottom of the icon and the
+// top or bottom of the row.
+const int kMinimumIconVerticalPadding = 2;
+
 // 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.
@@ -68,11 +75,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 +114,98 @@ struct TranslationTable {
     OmniboxResultView::SELECTED, OmniboxResultView::DIVIDER },
 };
 
+struct TextStyle {
+  ui::ResourceBundle::FontStyle font;
+  ui::NativeTheme::ColorId colors[OmniboxResultView::NUM_STATES];
+  gfx::BaselineStyle baseline;
+} const kTextStyles[] = {
+    // 1  ANSWER_TEXT

[msw, 2015/03/18 18:03:37]

Should we have an enum somewhere with these types?

[dschuyler, 2015/03/18 19:16:48]

There may be one in the server code.  It wouldn't be used in the client though. 
I'm open to adding the enum or leaving it out.  I can see it being a form of
documentation.

[Peter Kasting, 2015/03/18 22:19:33]

I wouldn't do it right now, since the only use of this array is as raw numbers
(not enum values) from the server JSON messages.

+    {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;

[msw, 2015/03/18 18:03:37]

Should an invalid type really default to ANSWER_TEXT with its LargeFont instead
of something like SUGGESTION_TEXT with the base font, colors and baseline?

[dschuyler, 2015/03/18 19:16:48]

It's an excellent question.  I also thought of making it the negative text so it
would look more like an error.  Type 1 seems like it would look less like an
error.  I'm open as to what type an invalid type should switch to.  I'm pretty
sure we should check the type though, since it's input form an outside source at
runtime.

[msw, 2015/03/18 20:20:16]

Probably doesn't matter, but I'd go with something that we'd consider
"baseline", like SUGGESTION_TEXT, over the larger ANSWER_TEXT (which would make
bad server behavior appear more prominent, instead of a no-op). Perhaps
explicitly return an inlined TextStyle here with such values:
{ui::ResourceBundle::BaseFont, {NativeTheme::kColorId_ResultsTableNormalText,
NativeTheme::kColorId_ResultsTableHoveredText,
NativeTheme::kColorId_ResultsTableSelectedText}, gfx::NORMAL_BASELINE}

[Peter Kasting, 2015/03/18 22:19:33]

I disagree, because ANSWER_TEXT is the conceptual "baseline style", despite the
fact that it's large.  Every answer is built around an ANSWER_TEXT, with some
other styles.  So that's the style that should be used on error. 
SUGGESTION_TEXT has a very specific meaning and defaulting to it is not
conceptually correct.

+  // Subtract one because the types are one based (not zero based).
+  return kTextStyles[type - 1];
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -159,7 +253,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 +343,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 +387,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 +476,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 +652,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 +702,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 +749,32 @@ void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   Layout();
   SchedulePaint();
 }
+
+int OmniboxResultView::GetAnswerLineHeight() const {

[msw, 2015/03/18 18:03:37]

This may be called a lot, should the result be cached?

[dschuyler, 2015/03/18 19:16:48]

The concern is whether one or more platforms could change their theme.  Please
let me know what you think of the prior discussion with pkasting in this same CL
about this function.

[msw, 2015/03/18 20:20:16]

I don't think we dynamically update the resource bundle fonts with theme
changes, and I presume changes wouldn't be reflected within the lifetime of am
OmniboxResultView object... I think the performance hit could be severe here,
since these result views are updated as the user types in the omnibox. I'd
suggest caching this height (at least per OmniboxResultView instance, if not
much more widely) without a clear benefit to recalculating the value constantly.

[Peter Kasting, 2015/03/18 22:19:33]

I already looked into this for quite a while after I suggested the same thing,
and I don't think we should cache.

The performance his is not actually severe here.  The resource bundle instance,
font list, and height are all already cached by the functions we're calling.

There is ChromeOS code that does, in fact, dynamically update the font styles
and rebuild the font tables, so given the chance of problems and the minimal
benefit, we shouldn't cache.

+  // GetTextStyle(1) is the largest font used and so defines the boundary that
+  // all the other answer styles fit within.
+  return ui::ResourceBundle::GetSharedInstance()
+      .GetFontList(GetTextStyle(1).font)
+      .GetHeight();
+}
+
+int OmniboxResultView::GetContentLineHeight() const {
+  return std::max(default_icon_size_ + (kMinimumIconVerticalPadding * 2),
+                  GetTextHeight() + (kMinimumTextVerticalPadding * 2));
+}
+
+void OmniboxResultView::AppendAnswerText(
+    gfx::RenderText* render_text,

[msw, 2015/03/18 18:03:37]

Why pass in the RenderText? The callers always use description_rendertext_.get()

[dschuyler, 2015/03/18 19:16:48]

I don't think I follow the question.  Would it be better to do something
different?  I've gotten different suggestions on passing member variables rather
than using them directly - please let me know if this version that uses the
member directly is what you were looking for.

[msw, 2015/03/18 20:20:16]

You got it.

[Justin Donnelly, 2015/03/18 20:42:57]

The more we can make methods depend on their local state rather than a more
global state, the better, I'd say. Did you feel strongly about this, Mike, or
were you just asking out of curiosity?

[msw, 2015/03/18 20:51:39]

I disagree, member functions should access member state without hesitation,
unless they're to be generalized across multiple parts of the member state that
can't be determined in the function context. Otherwise, make this file-local and
pass in State().

[Peter Kasting, 2015/03/18 22:19:33]

I think Mike is correct.  If a function is a member function, it should access
member variables directly, not have them passed to it.  Member variables are not
"global state" for the purpose of member functions.

[Justin Donnelly, 2015/03/19 15:05:05]

We could debate whether "global state" is the correct term, but the problems
with relying on state outside the narrowest possible scope are not specific to
some particular boundary (process-local, thread-local, class-local or whatever).
It's simply the fact that relying on communication via side-effects causes
brittleness.

I take the point that the passed value is both unnecessary here and adds
complexity. Using a combination of a passed value and class state in this case,
as Mike points out, is kind of confusing.

But in the future, I think we'd be better off if we tried harder to avoid
side-effect based communication. Where we can keep our classes small and
focused, member functions accessing member variables makes sense. But inevitably
some of our classes get very large and they start to resemble medium-sized C
programs. At that point, saying it's not "global state" becomes a bit academic.

[Peter Kasting, 2015/03/19 20:11:22]

It sounds like you're advocating a sort of "functional programming" style of
design, where every function takes all the args it needs and only modifies
things directly passed to it (and the return value).

This can be a powerful tool, but much like constness, it only really works if we
go all the way and do it.  The equivalent of that here would be a file-scoped
local function or a static class member function.  A nonstatic class member is
inherently operating on the state of the class.  I don't think it makes sense to
add nonstatic (and, in this case, non-const) member functions that we try to
make "more side-effect-free" by passing things to them, because splitting the
difference just causes confusion rather than more maintainable code.
Sure, but that argument makes sense if, in fact, the classes in question are
large enough to begin to make interaction complex.  Are you saying you believe
that's the case here?  The class certainly seems small enough to be tractable to
me, and the use of member variables here not confusing, but maybe you have a
different perspective.

If your conviction is that in general all classes get too big and unwieldy, then
the right answer would be to not use classes, and (as before) write everything
as side-effect-free standalone functions.  I think that's actually a legitimate
style, but it's exceptional in Chromium code.

The summary of all this is that I agree with some of your concerns in principle,
but in practice I don't think passing member values in to this particular
function is a win.

+    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());
+  // TODO(dschuyler): follow up on the problem of different font sizes within
+  // one RenderText

[msw, 2015/03/18 18:03:37]

nit: trailing period.

[dschuyler, 2015/03/18 19:16:48]

Done.

+  render_text->SetFontList(
+      ui::ResourceBundle::GetSharedInstance().GetFontList(text_style.font));
+  render_text->ApplyColor(
+      GetNativeTheme()->GetSystemColor(text_style.colors[GetState()]), range);
+  render_text->ApplyBaselineStyle(text_style.baseline, range);
+}