Omnibox UI: Fix Answers in Suggest display in Views

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
@@ skipping 347 matching lines @@
       match.answer != nullptr ||
       base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout);
   OmniboxPopupModel::ComputeMatchMaxWidths(
       contents->GetContentWidth(), separator_width_,
       description ? description->GetContentWidth() : 0,
       mirroring_context_->remaining_width(x), description_on_separate_line,
       !AutocompleteMatch::IsSearchType(match.type), &contents_max_width,
       &description_max_width);
 
   // Answers in Suggest results.
   if (match.answer && description_max_width != 0) {

[Peter Kasting, 2017/06/09 02:33:48]

Looking more closely at this code I'm concerned with this conditional.  I don't
think we want to check description_max_width for the answer case.  That will be
true when the omnibox is too narrow to display both contents and description on
one line.  But since answers are always vertical layout, we don't care about
checking that.

This is one reason why it may have made sense to collapse "answer" with
"vertical layout" as I proposed in your refactor, though I didn't realize it at
the time.

Also, in both the answer and vertical layout cases, we want to draw both
contents and description with the full width.  The xxx_max_width variables will
be set incorrectly today since ComputeMaxMatchWidths() assumes horizontal
layout.

[tommycli, 2017/06/09 16:09:21]

Hey Peter... I think we are okay since we have description_on_separate_line set
to true for the vertical layout case. Both the max width variables should be set
correctly.

I can definitely buy that we don't need to check description_max_width != 0 on
this line though. WDYT?

[Peter Kasting, 2017/06/09 19:30:53]

Wow, I totally didn't see that functionality.  I = blind.  Sorry.
Yeah, it seems like it should be removed.

     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;
     }
+    DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
+                   description_max_width);

[Peter Kasting, 2017/06/09 02:33:48]

Depending on how we address the above, we may be able to refactor this so this
DrawRenderText() call happens after these conditional blocks that draw the
CONTENTS and update x/y.

[tommycli, 2017/06/09 16:09:21]

Yes I definitely thought about this. Ultimately I thought it would be clearer if
all the operations for the answer case were together and all the operations for
the non-answer horizontal, and all the operations for the non-answer vertical
were together.

We can change in a followup if you hove strong opinions here.

[Peter Kasting, 2017/06/09 19:30:53]

I think we should change it.  Here's my mock attempt to do so.  To me this is
clearer and simpler.  You'd want to address the TODOs I added here.

  // Draw contents.
  // For vertical layouts with no description, vertically center.
  // TODO(pkasting): I think we should remove this behavior
  const bool vertical =
      base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout);
  const bool show_description = description_max_width > 0;
  if (vertical && !show_description)
    y += (GetTextHeight() + kVerticalPadding) / 2;
  const int contents_end = DrawRenderText(match, contents, CONTENTS, canvas, x,
                                          y, contents_max_width);

  // Move to position to draw answer/description.
  if (vertical || match.answer) {
    y += GetTextHeight() + kVerticalPadding;
  } else if (show_description) {
    x = DrawRenderText(match, separator_rendertext_.get(), SEPARATOR, canvas,
                       contents_end, y, separator_width_);
  }

  // Draw answer image, if any.
  // TODO(pkasting): Not clear whether we need first half of conditional
  if (match.answer && !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;
  }

  // Draw description.
  // TODO(pkasting): Conditional should maybe be hoisted to DrawRenderText()
  if (show_description) {
    DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
                   description_max_width);
  }

     return;
   }
 
   // Regular results.
   if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {
     // For no description, shift down halfways to draw contents in middle.
     if (description_max_width == 0)
       y += (GetTextHeight() + kVerticalPadding) / 2;
 
     DrawRenderText(match, contents, CONTENTS, canvas, x, y, contents_max_width);
@@ skipping 451 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);
 }