Omnibox UI: Fix Answers in Suggest display in Views

Omnibox UI: Fix Answers in Suggest display in Views

Fixes a regression introduced by an Omnibox UI experiment patch.

BUG=729974
Review-Url: https://codereview.chromium.org/2929003002
Cr-Commit-Position: refs/heads/master@{#478299}
Committed: https://chromium.googlesource.com/chromium/src/+/f2637fc408350b9cd9fa90cf35bad8c286776b9c

[tommycli, 2017-06-08 17:14:50]

tommycli@chromium.org changed reviewers:
+ jdonnelly@chromium.org, pkasting@chromium.org

[tommycli, 2017-06-08 17:14:50]

pkasting / jdonnelly: PTAL.

[tommycli, 2017-06-08 17:16:05]

On 2017/06/08 17:14:50, tommycli wrote:
> pkasting / jdonnelly: PTAL.

Prev. patch accidentally drops draw call for the AIS text part.

Ideally in the future it would be nice if we abstracted out the draw calls
between Mac and Views and could have a unit test in the common code.

[Justin Donnelly, 2017-06-08 17:21:09]

lgtm

[tommycli, 2017-06-08 17:33:04]

Description was changed from

==========
Omnibox: Fix Answers in Suggest display in Views

Fixes a regression introduced by an Omnibox UI experiment patch.

BUG=729974
==========

to

==========
Omnibox UI: Fix Answers in Suggest display in Views

Fixes a regression introduced by an Omnibox UI experiment patch.

BUG=729974
==========

[tommycli, 2017-06-08 17:34:15]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 17:34:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 18:23:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 18:23:26]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-06-09 00:03:51]

On 2017/06/08 17:21:09, Justin Donnelly wrote:
> lgtm

Great thanks.

pkasting: PTAL (or maybe I can just TBR for these types of changes?)

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

This change made me stare at the code longer, and I think it's wrong the way it
is today.

Your fix fixes a bug, so LGTM to check in, but let's not leave the code this
way.  See more detail below.

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:368: if (match.answer &&
description_max_width != 0) {
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.

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:384:
description_max_width);
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]

pkasting: Thanks!

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:368: if (match.answer &&
description_max_width != 0) {
On 2017/06/09 02:33:48, Peter Kasting (busy Wed-Thu) wrote:
> 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.

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?

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:384:
description_max_width);
On 2017/06/09 02:33:48, Peter Kasting (busy Wed-Thu) wrote:
> 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.

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.

[tommycli, 2017-06-09 16:09:25]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-06-09 16:09:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 16:13:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497024565830240,
"parent_rev": "feecd1919ed0b1e35e8262f540cf884e8719adc7", "commit_rev":
"f2637fc408350b9cd9fa90cf35bad8c286776b9c"}

[commit-bot: I haz the power, 2017-06-09 16:13:27]

Description was changed from

==========
Omnibox UI: Fix Answers in Suggest display in Views

Fixes a regression introduced by an Omnibox UI experiment patch.

BUG=729974
==========

to

==========
Omnibox UI: Fix Answers in Suggest display in Views

Fixes a regression introduced by an Omnibox UI experiment patch.

BUG=729974

Review-Url: https://codereview.chromium.org/2929003002
Cr-Commit-Position: refs/heads/master@{#478299}
Committed:
https://chromium.googlesource.com/chromium/src/+/f2637fc408350b9cd9fa90cf35ba...
==========

[commit-bot: I haz the power, 2017-06-09 16:13:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f2637fc408350b9cd9fa90cf35ba...

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

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:368: if (match.answer &&
description_max_width != 0) {
On 2017/06/09 16:09:21, tommycli wrote:
> On 2017/06/09 02:33:48, Peter Kasting (busy Wed-Thu) wrote:
> > 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.
> 
> 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.

Wow, I totally didn't see that functionality.  I = blind.  Sorry.

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

Yeah, it seems like it should be removed.

https://codereview.chromium.org/2929003002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:384:
description_max_width);
On 2017/06/09 16:09:21, tommycli wrote:
> On 2017/06/09 02:33:48, Peter Kasting (busy Wed-Thu) wrote:
> > 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.
> 
> 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.

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);
  }