[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..d49994964703adae7774a61b5de203cf3041d1d5 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -23,10 +23,12 @@
 #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 "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"
@@ -112,6 +114,109 @@ struct TranslationTable {
     OmniboxResultView::SELECTED, OmniboxResultView::DIVIDER },
 };
 
+// Colors from the Answers in Suggest UI spec.

[Peter Kasting, 2015/03/12 09:46:00]

This comment doesn't seem to tell the reader anything useful; I'd remove it and
the one below.

[dschuyler, 2015/03/13 06:01:22]

Done.

+const SkColor kColorBlack = SkColorSetRGB(0x00, 0x00, 0x00);

[Peter Kasting, 2015/03/12 09:46:00]

Don't define new constants for anything that's already a Skia color constant
(see SkColor.h for all the SK_ColorXXX values).

Also, try to use existing Chrome UI colors (e.g. the text color, the background
color, the link color, etc. all of which are already defined) where the default
definitions match the spec.  Where the spec gives something that doesn't already
exist, we'll either want to dynamically compute it from other colors, change to
a different color that does exist, or add it as (likely) a UI-wide constant,
though in those cases for now it's OK to hardcode things (but this will have to
be fixed before this is used by actual users). 

[dschuyler, 2015/03/13 06:01:21]

Done.

+const SkColor kColorGrey = SkColorSetRGB(0x80, 0x80, 0x80);
+const SkColor kColorRed = SkColorSetRGB(0xff, 0x00, 0x00);
+const SkColor kColorGreen = SkColorSetRGB(0xff, 0x00, 0x00);
+const SkColor kColorBlue = SkColorSetRGB(0x00, 0x00, 0xff);
+const SkColor kColorPurple = SkColorSetRGB(0x80, 0x00, 0x80);
+const SkColor kColorRedish = SkColorSetRGB(0xdd, 0x4b, 0x39);
+const SkColor kColorGreenish = SkColorSetRGB(0x3d, 0x94, 0x00);
+
+// Styles from the Answers in Suggest UI spec.
+struct TextStyles {
+  ui::ResourceBundle::FontStyle font;
+  SkColor color;
+  gfx::BaselineStyle baseline;
+} const kTextStyles[]{

[Peter Kasting, 2015/03/12 09:46:00]

Nit: Missing space

[dschuyler, 2015/03/13 06:01:22]

Done.

+    // 0  Zero is not used as a style type.

[Peter Kasting, 2015/03/12 09:46:00]

Nit: Remove this comment, all it does is imply there ought to be a (placeholder)
zeroth entry here.

Also shouldn't this all be 2-space indented and not 4?

[dschuyler, 2015/03/13 06:01:22]

This is cl format.  Should I report a bug in cl format?
My guess is that cl format is seeing it as a continuation of the struct
TextStyles line.

+    // 1  ANSWER_TEXT
+    {
+     ui::ResourceBundle::LargeFont,

[Peter Kasting, 2015/03/12 09:45:59]

Nit: Indent 2 more, not 1

[dschuyler, 2015/03/13 06:01:21]

I can adjust this by hand.  This should be already fixed in cl format, but the
patch hasn't gotten out to users (us) yet.

+     kColorBlack,
+     gfx::NORMAL_BASELINE,
+    },
+    // 2  HEADLINE_TEXT
+    {
+     ui::ResourceBundle::LargeFont,
+     kColorGrey,
+     gfx::NORMAL_BASELINE,
+    },
+    // 3  TOP_ALIGNED_TEXT
+    {
+     ui::ResourceBundle::LargeFont,
+     kColorGrey,
+     gfx::SUPERIOR,
+    },
+    // 4  DESCRIPTION_TEXT
+    {
+     ui::ResourceBundle::BaseFont,
+     kColorGrey,
+     gfx::NORMAL_BASELINE,
+    },
+    // 5  DESCRIPTION_TEXT_NEGATIVE
+    {
+     ui::ResourceBundle::LargeFont,
+     kColorRedish,
+     gfx::INFERIOR,
+    },
+    // 6  DESCRIPTION_TEXT_POSITIVE
+    {
+     ui::ResourceBundle::LargeFont,
+     kColorGreenish,
+     gfx::INFERIOR,
+    },
+    // 7  MORE_INFO_TEXT
+    {
+     ui::ResourceBundle::SmallFont,
+     kColorGrey,
+     gfx::NORMAL_BASELINE,
+    },
+    // 8  SUGGESTION_TEXT
+    {
+     ui::ResourceBundle::BaseFont,
+     kColorBlack,
+     gfx::NORMAL_BASELINE,
+    },
+    // 9  SUGGESTION_TEXT_POSITIVE
+    {
+     ui::ResourceBundle::BaseFont,
+     kColorGreen,
+     gfx::NORMAL_BASELINE,
+    },
+    // 10 SUGGESTION_TEXT_NEGATIVE
+    {
+     ui::ResourceBundle::BaseFont,
+     kColorRed,
+     gfx::NORMAL_BASELINE,
+    },
+    // 11 SUGGESTION_LINK_COLOR
+    {
+     ui::ResourceBundle::BaseFont,
+     kColorBlue,
+     gfx::NORMAL_BASELINE,
+    },
+    // 12 STATUS_TEXT
+    {
+     ui::ResourceBundle::LargeFont,
+     kColorGrey,
+     gfx::INFERIOR,
+    },
+    // 13 PERSONALIZED_SUGGESTION_TEXT
+    {
+     ui::ResourceBundle::BaseFont,
+     kColorPurple,

[Peter Kasting, 2015/03/12 09:46:00]

While google.com uses purple for personalized suggestions, we've always
intentionally not made them purple in the Chrome omnibox dropdown, and I suggest
continuing to display them as the normal suggestion color instead of as
something else.

[Justin Donnelly, 2015/03/12 14:34:18]

Yes, I used normal suggestion color for these on mobile.

[dschuyler, 2015/03/13 06:01:21]

Done.

[dschuyler, 2015/03/13 06:01:22]

Done.

+     gfx::NORMAL_BASELINE,
+    },
+};
+const int kTextStyleCount = sizeof(kTextStyles) / sizeof(kTextStyles[0]);

[Peter Kasting, 2015/03/12 09:46:00]

Don't do this; use arraysize(kTextStyles) below where you would use this.

[dschuyler, 2015/03/13 06:01:22]

Done.

+
+const TextStyles& GetTextStyle(int type) {
+  // Subtract one because the types are one based (not zero based).
+  return kTextStyles[(type - 1) % kTextStyleCount];

[Peter Kasting, 2015/03/12 09:46:00]

Using % here seems questionable; if the server gives us a style we don't
understand, we probably want to pick a sane default style instead of using what
will appear to be a random selection, or even refuse to render the suggestion
entirely.

[dschuyler, 2015/03/13 06:01:21]

:) too much time focusing on performance.  The modulus avoids the potential
branch prediction miss.

Done.

+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -250,9 +355,14 @@ 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) {

[Peter Kasting, 2015/03/12 09:45:59]

Nit: If you reverse this conditional, you can omit the {} and save a line.

[dschuyler, 2015/03/13 06:01:20]

Done.

+    // An answer implies a match and a description in a large font.
+    return gfx::Size(0, GetContentLineHeight() +
+                            ui::ResourceBundle::GetSharedInstance()
+                                .GetFontList(ui::ResourceBundle::LargeFont)

[Peter Kasting, 2015/03/12 09:45:59]

Hmm, technically rather than hardcoding LargeFont here, shouldn't we be using
the font from the first answer style defined above?

[dschuyler, 2015/03/13 06:01:22]

Done.

+                                .GetHeight());
+  }
+  return gfx::Size(0, GetContentLineHeight());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -293,21 +403,29 @@ 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 = 1 +

[Peter Kasting, 2015/03/12 09:46:00]

Why 1?

[dschuyler, 2015/03/13 06:01:21]

I've added a comment.  While it's hard to tell from size to side on the icon,
the top to bottom clearly shows a missing line of pixels when compared to the
baseline of the adjacent text.  It's fairly common for a scale to rect or paint
rect to draw 0 to n-1.  In this case it looks better to draw 0 to N inclusive.

Done.

+                               ui::ResourceBundle::GetSharedInstance()
+                                   .GetFontList(ui::ResourceBundle::LargeFont)

[Peter Kasting, 2015/03/12 09:46:00]

Same question

[dschuyler, 2015/03/13 06:01:21]

Done.

+                                   .GetBaseline();
+        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,
+            answer_icon_size, answer_icon_size, true);

[Peter Kasting, 2015/03/12 09:45:59]

Do we really want to scale the icon to the text height?  This seems likely to
produce really ugly artifacts, as on different systems the font height is going
to take on every integral value in a range, and it's a small enough number that
surely the icon isn't going to scale well to all of those sizes.

I would think we'd want to simply display the icon at the provided size and
never scale.

[Justin Donnelly, 2015/03/12 14:34:18]

The provided size is not in any way adjusted for the display in use, it's always
128x128. I've talked to the folks who built the backend code and they're open to
changing this based on a client-provided spec but nobody's done this yet.

On Android and iOS the scaling has worked well across a large variety of sizes
and pixel densities. They're clearly not using a cheap scaling algorithm.

Dave, can you try this at varying fonts to see how the scaling performs?  Peter,
if you have reason to believe that the scaling algorithm in Skia or whatever's
responsible for this won't work well, let us know.

[Peter Kasting, 2015/03/12 15:55:44]

I'm just concerned that this can probably take every integral value between
about 16 and about 48.  I would be surprised if the source icons looked good at
all those sizes.

I guess if that's what we have to work with, it's what we have.

[dschuyler, 2015/03/13 06:01:21]

For now at least, I think it's what we have to work with.

Done.

+        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 +496,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 +672,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 +722,30 @@ void OmniboxResultView::OnPaint(gfx::Canvas* canvas) {
     if (!description_rendertext_) {
       if (match_.answer) {
         base::string16 text;
+        const base::string16 spacer(base::UTF8ToUTF16("  "));

[Peter Kasting, 2015/03/12 09:46:00]

You can't concatenate strings this way; this doesn't work for i18n.

You need to define localizable string constants with placeholders for the
different substrings and then use methods like GetStringFUTF16() to get the
complete string along with the offsets at which various substrings start.  (It's
also really weird to use two spaces for something, as depending on the font, the
width of that could be wildly different.  You probably either want one space, or
some kind of number of pixels of space based on the font's n-width, or
something.)

This should also obviate the need for StyleAnswerText() to compute and return
offsets.

[Justin Donnelly, 2015/03/12 14:34:18]

I'm not quite sure what you're proposing here, Peter. Are you suggesting that
the localizable string would be something like "$1 $2 $3"? And we'd substitute
text_fields(), additional_text() and status_text()? This point of this would be
to presumably reorder the three elements for RTL languages? On mobile, I handled
this by simply reordering the three elements for RTL languages manually. I don't
know that there's any more that translators could do that that, the three
answers elements are pretty abstract.

Regarding the two spaces, that's based on a hack I did on Android and iOS. I'm
not proud of it, but it works reasonably well in practice. Pixel spacing is what
I had in mind to add at some point. But in the meantime, you suggest one space
is acceptable. That's already variable based on font, so is there an issue with
two spaces for a bit of extra padding? Is your concern over users who have
chosen a fixed-width font?

[Peter Kasting, 2015/03/12 15:55:44]

Yes.
That, as well as to insert or remove whitespace of various types (e.g. Thai may
not use whitespace at all).
I think you should change what you did :)

I've become very leery of assuming what localizers might do.  It seems like
there are always weird cases where people want to insert dashes or remove spaces
or whatever.

It's not particularly wonderful if the answers response is so free-form that
there's literally no definable meaning to any of the strings and it's impossible
to create a meaningful example string for translators.  Is that what you're
saying the data here looks like?
I'm concerned that between different fonts, the relative width of a space is not
fixed, so depending on the font face chosen, what looks good on one system may
look too close or too far on another system.  I don't know that this is
definitively wrong, it's just (as I said) weird; I've never seen us do this
anywhere else.

[dschuyler, 2015/03/13 06:01:22]

I agree that it's weird.  Justin and I have discussed RTL and i18n related to
answers.  So far there is not a super great direction to go with this, so we're
going with 'reasonable in the interim'.

While I agree with you Peter, I'd like to put this in as a bug or TODO.

Something else that would be helpful is edge-to-edge justification with an
expanding spacer/leading element.  There is Answer UI that is supposed to be
right justified in the UI rectangle the text is drawn in.  In RTL I expect that
may be left justified - i.e. 'away' from the other text.  I'd like to make this
a feature of RenderText.

Overall, it seems like a topic that needs some discussion in a timely fashion,
but I'd like to get this CL landed and revisit this part of it shortly
afterwards. 

[Justin Donnelly, 2015/03/13 21:08:32]

For sure, you're right to be wary of a simplistic approach to localization. But,
yes, there really is very little in the way of semantics attached to these three
values. The spec says things like, "Additional text with a bit of padding on the
left against the main text field." The web UI at google.com puts these three
values in three separate divs and then just re-orders them for RTL, so I took a
similar approach in Chrome.

The fact that I did it by simply concatenating them into a single string is none
too sophisticated, I admit. I don't think that a localized string is the right
answer (given that there's no meaningful instructions we could give the
translators), but having three separate text labels positioned appropriately
would be a better approach.

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

At the least, I think you can make this code cleaner.  I don't think we should
have a block of "traverse all the answer fields again to apply styles" code when
traversing has so many conditionals instead of just being a loop.

One way would be to add some sort of RenderText::AppendStyledText() function
where you could pass in some new text and a style (and maybe color), which would
let you just go through these objects once.  That's kind of special-purpose,
though.

Another would be to keep e.g. a list of pairs of (range end position, text type)
as you add pieces to the string.  Then write a single loop that goes through and
sets the relevant style/baseline/color for each range.  This basically omits the
need for StyleAnswerText().

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

How about AppendText, so as to be a bit more general than AppendStyledText. 
That still allows for setting the styles more efficiently without looping so
much.  That's what is now checked in for your review.
Done.

         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);
@@ -640,3 +781,27 @@ 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
+          ? GetNativeTheme()->GetSystemColor(
+                NativeTheme::kColorId_ResultsTableSelectedText)

[Peter Kasting, 2015/03/12 09:46:00]

This turns all the text the same color when the answer is selected.  That
doesn't seem right.  See e.g. how the existing omnibox dropdown preserves
multiple different colors in the selected rows.

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

Done.

+          : text_style.color,
+      range);
+  render_text->ApplyBaselineStyle(text_style.baseline, range);
+  return offset + length;
+}
+
+int OmniboxResultView::GetContentLineHeight() const {
+  return std::max(default_icon_size_ + (kMinimumIconVerticalPadding * 2),
+                  GetTextHeight() + (minimum_text_vertical_padding_ * 2));
+}