Fix layout of omnibox rows for material design

Fix layout of omnibox rows for material design

Guarantee the correct height of omnibox result
rows in material design by separating the
constants for the top/bottom dropdown padding and
the minimum text padding within a single row.

Furthermore, ensure the correct layout of answers
that appear on separate rows by using the
correct row height for the text bounds.

BUG=522585
TEST=manual

Committed: https://crrev.com/8b533c3be70ed43cbee09a912eabdec1d3de38ea
Cr-Commit-Position: refs/heads/master@{#355059}

[tdanderson, 2015-10-13 19:06:51]

tdanderson@chromium.org changed reviewers:
+ pkasting@chromium.org

[tdanderson, 2015-10-13 19:06:52]

Peter, can you please take a look?

[Peter Kasting, 2015-10-13 19:21:17]

https://codereview.chromium.org/1399253005/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/1399253005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:734: return
std::max(font_height, GetContentLineHeight());
Where in the specs is this requirement?  I don't understand why this is
desirable, it seems like a loss to me.

[tdanderson, 2015-10-13 19:26:43]

On 2015/10/13 19:21:17, Peter Kasting wrote:
>
https://codereview.chromium.org/1399253005/diff/1/chrome/browser/ui/views/omn...
> File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):
> 
>
https://codereview.chromium.org/1399253005/diff/1/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_result_view.cc:734: return
> std::max(font_height, GetContentLineHeight());
> Where in the specs is this requirement?  I don't understand why this is
> desirable, it seems like a loss to me.

The spec says that content rows and answer rows should have the
same height. Without this CL, GetAnswerLineHeight() always returns
24, which is smaller than the required 32pt row height needed
for hybrid.

[Peter Kasting, 2015-10-13 19:31:49]

On 2015/10/13 19:26:43, tdanderson wrote:
> On 2015/10/13 19:21:17, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/1399253005/diff/1/chrome/browser/ui/views/omn...
> > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/1399253005/diff/1/chrome/browser/ui/views/omn...
> > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:734: return
> > std::max(font_height, GetContentLineHeight());
> > Where in the specs is this requirement?  I don't understand why this is
> > desirable, it seems like a loss to me.
> 
> The spec says that content rows and answer rows should have the
> same height.

Right, I was saying, I couldn't find that in the spec.

Besides that, I don't understand why that would be a good thing.  An answer row
is already an extension of the row above, so there's no need to enlarge it for
touch reasons of the like.  When its font is smaller than the row above,
expanding the padding to make the rows the same will just make the answer row
look way too spaced-out.

I really think we should not make this change.

[tdanderson, 2015-10-19 13:50:59]

Description was changed from

==========
Modify omnibox answer row height for material design

For the material design omnibox dropdown, make answer
rows be at least as tall as normal content rows.

BUG=522585
TEST=manual
==========

to

==========
Fix layout of omnibox rows for material design

Guarantee the correct height of omnibox result
rows in material design by separating the 
constants for the top/bottom dropdown padding and
the minimum text padding within a single row.

Furthermore, ensure the correct layout of answers
that appear on separate rows by using the
correct row height for the text bounds.

BUG=522585
TEST=manual
==========

[tdanderson, 2015-10-19 13:53:25]

Peter, can you please take a look at my new approach in Patch Set 2? Sebastien
has signed off on the appearance from iterating over email, but I can share the
screenshots on the bug if desired.

[Peter Kasting, 2015-10-19 20:47:11]

LGTM

https://codereview.chromium.org/1399253005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/layout_constants.cc (right):

https://codereview.chromium.org/1399253005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_constants.cc:99: const int
kOmniboxDropdownTextPadding[] = {3, 3, 3};
Should this one become not-a-layout-constant?

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

https://codereview.chromium.org/1399253005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:458: int height =
(render_text_type == DESCRIPTION && match.answer) ?
Nit: const

I'd probably also place this line just below the comment below since it's really
"part of" the SetDisplayRect() call.

https://codereview.chromium.org/1399253005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:464: right_x - x,
height));
Wow, wouldn't this have made the non-MD answer content mispositioned before? 
Unless GetContentsLineHeight() and GetAnswerLineHeight() had been returning the
same value...

[tdanderson, 2015-10-20 14:20:15]

https://codereview.chromium.org/1399253005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/layout_constants.cc (right):

https://codereview.chromium.org/1399253005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_constants.cc:99: const int
kOmniboxDropdownTextPadding[] = {3, 3, 3};
On 2015/10/19 20:47:11, Peter Kasting wrote:
> Should this one become not-a-layout-constant?

I'd rather keep it here for now; it's nice to have
all related constants in one place, and there is
still a chance that these could need adjusting.

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

https://codereview.chromium.org/1399253005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:458: int height =
(render_text_type == DESCRIPTION && match.answer) ?
On 2015/10/19 20:47:11, Peter Kasting wrote:
> Nit: const
> 
> I'd probably also place this line just below the comment below since it's
really
> "part of" the SetDisplayRect() call.

Done.

https://codereview.chromium.org/1399253005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:464: right_x - x,
height));
On 2015/10/19 20:47:11, Peter Kasting wrote:
> Wow, wouldn't this have made the non-MD answer content mispositioned before? 
> Unless GetContentsLineHeight() and GetAnswerLineHeight() had been returning
the
> same value...

Yes, without MD GetContentsLineHeight() was returning 25
and GetAnswerLineHeight() was returning 24, making this
go unnoticed. It showed up when I switched to hybrid where
the contents line height is 32.

[tdanderson, 2015-10-20 14:20:19]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2015-10-20 14:20:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1399253005/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-10-20 14:20:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399253005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399253005/40001

[commit-bot: I haz the power, 2015-10-20 14:57:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-20 14:57:52]

Patchset 3 (id:??) landed as
https://crrev.com/8b533c3be70ed43cbee09a912eabdec1d3de38ea
Cr-Commit-Position: refs/heads/master@{#355059}