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

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 "base/macros.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_result_view.h"
 
 #include <limits.h>
 
 #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"
 #include "base/strings/string_util.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/location_bar/background_with_1_px_border.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/grit/components_scaled_resources.h"
@@ skipping 266 matching lines @@
   // updating the text in the omnibox but this alert and GetAccessibleNodeData
   // below make the answer contents accessible.
   if (match_.answer)
     NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
 }
 
 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);
 }
 
 void OmniboxResultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->SetName(match_.answer
                          ? l10n_util::GetStringFUTF16(
                                IDS_OMNIBOX_ACCESSIBLE_ANSWER, match_.contents,
                                match_.answer->second_line().AccessibleText())
                          : match_.contents);
 }
@@ skipping 38 matching lines @@
   OmniboxPopupModel::ComputeMatchMaxWidths(
       contents->GetContentWidth(),
       separator_width_,
       description ? description->GetContentWidth() : 0,
       mirroring_context_->remaining_width(x),
       match.answer != nullptr,
       !AutocompleteMatch::IsSearchType(match.type),
       &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) {
+  // 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;
-      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;
-      }
-    } else {
+      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);
   }
 }
 
 int OmniboxResultView::DrawRenderText(
     const AutocompleteMatch& match,
     gfx::RenderText* render_text,
     RenderTextType render_text_type,
     gfx::Canvas* canvas,
     int x,
     int y,
@@ skipping 192 matching lines @@
   return match_.associated_keyword &&
       (keyword_icon_->x() <= icon_bounds_.right());
 }
 
 void OmniboxResultView::InitContentsRenderTextIfNecessary() const {
   if (!contents_rendertext_) {
     if (match_.answer) {
       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);
     }
   }
 }
 
 void OmniboxResultView::Layout() {
   const int horizontal_padding =
       GetLayoutConstant(LOCATION_BAR_ELEMENT_PADDING) +
       LocationBarView::kIconInteriorPadding;
   // The horizontal bounds we're given are the outside bounds, so we can match
   // the omnibox border outline shape exactly in OnPaint().  We have to inset
   // here to keep the icons lined up.
   const int start_x = BackgroundWith1PxBorder::kLocationBarBorderThicknessDip +
                       horizontal_padding;
   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;
   int text_width = end_x - text_x;
 
   if (match_.associated_keyword.get()) {
     const int max_kw_x = end_x - keyword_icon_->width();
     const int kw_x = animation_->CurrentValueBetween(max_kw_x, start_x);
     const int kw_text_x = kw_x + keyword_icon_->width() + horizontal_padding;
 
@@ skipping 21 matching lines @@
                          icon_bounds_.y());
     int x = GetMirroredXForRect(text_bounds_);
     mirroring_context_->Initialize(x, text_bounds_.width());
     InitContentsRenderTextIfNecessary();
 
     if (!description_rendertext_) {
       if (match_.answer) {
         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(),
                description_rendertext_.get(), canvas, x);
   }
 
   AutocompleteMatch* keyword_match = match_.associated_keyword.get();
   if (keyword_match) {
     int x = GetMirroredXForRect(keyword_text_bounds_);
     mirroring_context_->Initialize(x, keyword_text_bounds_.width());
@@ skipping 132 matching lines @@
   destination->AppendText(text);
   const TextStyle& text_style = GetTextStyle(text_type);
   // TODO(dschuyler): follow up on the problem of different font sizes within
   // one RenderText.  Maybe with destination->SetFontList(...).
   destination->ApplyWeight(
       is_bold ? gfx::Font::Weight::BOLD : gfx::Font::Weight::NORMAL, range);
   destination->ApplyColor(
       GetNativeTheme()->GetSystemColor(text_style.colors[GetState()]), range);
   destination->ApplyBaselineStyle(text_style.baseline, range);
 }