Omnibox UI Experiments: Add Vertical Layout experiment (title-on-top).

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 aff68fc9c801574b0e5d4e3d531f17d431fe392f..a81053079f92c85f6036ffc8a50ac6fed3d7c77b 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -17,6 +17,7 @@
 
 #include <algorithm>  // NOLINT
 
+#include "base/feature_list.h"
 #include "base/i18n/bidi_line_iterator.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/strings/string_number_conversions.h"
@@ -303,6 +304,8 @@ gfx::Size OmniboxResultView::GetPreferredSize() const {
   int height = GetTextHeight() + (2 * GetVerticalMargin());
   if (match_.answer)
     height += GetAnswerHeight() + kVerticalPadding;
+  else if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout))
+    height += GetTextHeight() + kVerticalPadding;
   return gfx::Size(0, height);
 }
 
@@ -361,32 +364,48 @@ void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
       &contents_max_width,
       &description_max_width);
 
-  int after_contents_x = DrawRenderText(match, contents, CONTENTS, canvas,
-                                        x, y, contents_max_width);
+  // Answers in Suggest results.
+  if (match.answer && description_max_width != 0) {
+    DrawRenderText(match, contents, CONTENTS, canvas, x, y, contents_max_width);
+    y += GetTextHeight() + kVerticalPadding;
+    if (!answer_image_.isNull()) {
+      int answer_icon_size = GetAnswerHeight();
+      canvas->DrawImageInt(answer_image_, 0, 0, answer_image_.width(),
+                           answer_image_.height(), GetMirroredXInView(x), y,
+                           answer_icon_size, answer_icon_size, true);
+      // TODO(dschuyler): Perhaps this should be based on the font size
+      // instead of hardcoded to 2 dp (e.g. by adding a space in an
+      // appropriate font to the beginning of the description, then reducing
+      // the additional padding here to zero).
+      const int kAnswerIconToTextPadding = 2;
+      x += answer_icon_size + kAnswerIconToTextPadding;
+    }
+    return;
+  }
 
-  if (description_max_width != 0) {
-    if (match.answer) {
-      y += GetTextHeight() + kVerticalPadding;
-      if (!answer_image_.isNull()) {
-        int answer_icon_size = GetAnswerHeight();
-        canvas->DrawImageInt(
-            answer_image_,
-            0, 0, answer_image_.width(), answer_image_.height(),
-            GetMirroredXInView(x), y, answer_icon_size, answer_icon_size, true);
-        // TODO(dschuyler): Perhaps this should be based on the font size
-        // instead of hardcoded to 2 dp (e.g. by adding a space in an
-        // appropriate font to the beginning of the description, then reducing
-        // the additional padding here to zero).
-        const int kAnswerIconToTextPadding = 2;
-        x += answer_icon_size + kAnswerIconToTextPadding;
-      }
+  // Regular results.
+  if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {
+    if (description_max_width == 0) {
+      // For no description, shift down halfways to draw contents in middle.
+      y += (GetTextHeight() + kVerticalPadding) / 2;
+      DrawRenderText(match, contents, CONTENTS, canvas, x, y,
+                     contents_max_width);

[Peter Kasting, 2017/05/25 20:24:39]

Nit: I'd probably rework this to avoid the duplicated call:

  if (description_max_width == 0)
    y += ...;
  DrawRenderText(..., CONTENTS, ...);
  if (description_max_width != 0) {
    ...

This in turn makes this much more parallel to the non-vertical-layout path, so
that you could probably combine with that.

However, see also comments on bug regarding this behavior.

[tommycli, 2017/05/25 21:59:10]

Done.

I was not able to combine it with the old behavior as the logic is fairly
different.

     } else {
+      DrawRenderText(match, contents, CONTENTS, canvas, x, y,
+                     contents_max_width);
+      y += GetTextHeight() + kVerticalPadding;
+      DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
+                     description_max_width);
+    }
+  } else {
+    x = DrawRenderText(match, contents, CONTENTS, canvas, x, y,
+                       contents_max_width);
+    if (description_max_width != 0) {
       x = DrawRenderText(match, separator_rendertext_.get(), SEPARATOR, canvas,
-                         after_contents_x, y, separator_width_);
+                         x, y, separator_width_);
+      DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
+                     description_max_width);
     }
-
-    DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
-                   description_max_width);
   }
 }
 
@@ -599,8 +618,15 @@ void OmniboxResultView::InitContentsRenderTextIfNecessary() const {
       contents_rendertext_ =
           CreateAnswerText(match_.answer->first_line(), font_list_);
     } else {
+      bool swap_match_text =

[Peter Kasting, 2017/05/25 20:24:39]

Don't we have an experiment somewhere that sometimes displays titles before URLs
for certain things (like zerosuggestions)?

I would look for that code and try to combine this with it.  I think that maybe
happens in AutocompleteResult?  It seems like that's more where you want to do
something like this, and the OmniboxResultView should be a "dumb client".  Even
if you left stuff here, I'm not real keen on sticking the contents in
|description_rendertext_| and the description in |contents_rendertext_| -- those
are just misleading.

Maybe the members here should be |first_rendertext_| and |second_rendertext_| or
something, so we isolate "physical" descriptions like first/second from semantic
ones like content/description. 

(We might want to consider renaming the autocomplete match fields to not be
"contents" and "description" but rather things that actually reflect what they
are, like URLs and titles and search queries and such?  Not only would this feel
less random, it would be more robust -- the current code assumes that non-search
matches always put URLs in the contents and titles in the description, but maybe
that wouldn't always be true.)

[tommycli, 2017/05/25 21:59:10]

Hey Peter -- Looking at what ZeroSuggest does, they display the title in place
of the URL:
https://cs.chromium.org/chromium/src/components/omnibox/browser/zero_suggest_...

It wouldn't be quite appropriate here...

In fact this swapping logic is cribbed from the iOS implementation:
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_po...

In the future, if we are going to always have vertical matches, we will just
swap the title and description in the actual provider, so we don't have to do it
here.

But for now, so long as we support both vertical and side-by-side, I think we're
going to have to do the swapping at the UI level.

Thanks!

Tommy

+          base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout) &&
+          !AutocompleteMatch::IsSearchType(match_.type) &&
+          !match_.description.empty();
+
       contents_rendertext_ = CreateClassifiedRenderText(
-          match_.contents, match_.contents_class, false);
+          swap_match_text ? match_.description : match_.contents,
+          swap_match_text ? match_.description_class : match_.contents_class,
+          false);
     }
   }
 }
@@ -617,8 +643,12 @@ void OmniboxResultView::Layout() {
   const int end_x = width() - start_x;
 
   const gfx::ImageSkia icon = GetIcon();
-  const int icon_y =
-      GetVerticalMargin() + (GetTextHeight() - icon.height()) / 2;
+  int icon_y = GetVerticalMargin();
+  if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {

[Peter Kasting, 2017/05/25 20:24:39]

Nit: {} not necessary

Another way to write all this might be:

  int row_height = GetTextHeight();
  if (vertical layout)
    row_height += kVerticalPadding + GetTextHeight();
  const int icon_y = GetVerticalMargin() + (row_height - icon.height()) / 2;

This is shorter and IMO a little less error-prone.

[tommycli, 2017/05/25 21:59:09]

Done. Thanks for the concrete suggestions!

+    icon_y += (2 * GetTextHeight() + kVerticalPadding - icon.height()) / 2;
+  } else {
+    icon_y += (GetTextHeight() - icon.height()) / 2;
+  }
   icon_bounds_.SetRect(start_x, icon_y, icon.width(), icon.height());
 
   const int text_x = start_x + LocationBarView::kIconWidth + horizontal_padding;
@@ -660,8 +690,16 @@ void OmniboxResultView::OnPaint(gfx::Canvas* canvas) {
         description_rendertext_ =
             CreateAnswerText(match_.answer->second_line(), GetAnswerFont());
       } else if (!match_.description.empty()) {
+        // If the description is empty, we wouldn't swap with the contents --
+        // nor would we create the description RenderText object anyways.
+        bool swap_match_text = base::FeatureList::IsEnabled(
+                                   omnibox::kUIExperimentVerticalLayout) &&
+                               !AutocompleteMatch::IsSearchType(match_.type);
+
         description_rendertext_ = CreateClassifiedRenderText(
-            match_.description, match_.description_class, true);
+            swap_match_text ? match_.contents : match_.description,
+            swap_match_text ? match_.contents_class : match_.description_class,
+            false);
       }
     }
     PaintMatch(match_, contents_rendertext_.get(),