Compute the correct maximum contents width when eliding away the description in

Compute the correct maximum contents width when eliding away the description in
the omnibox dropdown.

In the case where the description space was so small that the entire description
was eliminated, the contents space was set to whatever the desired contents size
was, which might be larger than the available space.  Instead, clamp this
correctly to the available space, including space we'd previously reserved for
the separator.

Also, in some cases, the previous code would allocate more space to contents
and/or description than they actually wanted.  Because the caller simply used
the returned values to elide, this had no functional effect, but it was arguably
confusing, and could cause bugs if anyone else calls this function in the
future, so this was fixed.  Now both will be given space that's at most as large
as they request.

Another quirk of the old code was that if the available width was less than
kMinimumContentsWidth, the assigned contents width would exceed the available
width.  Again, the caller would ultimately clamp this, but like the above
behavior, this was confusing and seemed error-prone, so this was fixed as well.
This required some other minor adjustments to ensure that this wouldn't result
in returning a negative max width, even if the available width on entry to the
function was less than the separator width.

BUG=none
TEST=none

Committed: https://crrev.com/f01e1802853692c9b1e758e7b2a1029d4ccf655e
Cr-Commit-Position: refs/heads/master@{#326700}

[Peter Kasting, 2015-04-22 02:17:49]

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

[Peter Kasting, 2015-04-22 02:17:49]

No TEST= steps listed because the best way to test this is to modify the code to
run the keyword animation at 1/100th speed and watch carefully how things get
elided.

[Mark P, 2015-04-22 18:29:12]

I have a concern about one line.  Otherwise, the rest of the code looks fine.

--mark

https://codereview.chromium.org/1082833006/diff/1/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_popup_model.cc (right):

https://codereview.chromium.org/1082833006/diff/1/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_popup_model.cc:55: *contents_max_width =
std::min(contents_width, available_width);
This doesn't make sense; you immediately override this value on the next line.

Did you mean the left side of the equals to be contents_width, not
*contents_max_width?

If so, this error indicates you should test this changelist better.

[Peter Kasting, 2015-04-22 23:08:07]

https://codereview.chromium.org/1082833006/diff/1/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_popup_model.cc (right):

https://codereview.chromium.org/1082833006/diff/1/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_popup_model.cc:55: *contents_max_width =
std::min(contents_width, available_width);
On 2015/04/22 18:29:12, Mark P wrote:
> This doesn't make sense; you immediately override this value on the next line.
> 
> Did you mean the left side of the equals to be contents_width, not
> *contents_max_width?

Yes, this was a typo when splitting this change out of my larger change.  I'll
fix this.

> If so, this error indicates you should test this changelist better.

The tests here already failed (likely because of this) :P

[Mark P, 2015-04-23 21:20:57]

Reminder: I will be happy to approve when you fix the bug.

--mark

[Peter Kasting, 2015-04-23 23:13:26]

Bug fixed and tests should pass; PTAL.

[Mark P, 2015-04-23 23:19:48]

lgtm

[Peter Kasting, 2015-04-23 23:20:26]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-04-23 23:21:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082833006/20001

[commit-bot: I haz the power, 2015-04-24 00:24:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-24 00:25:26]

Patchset 2 (id:??) landed as
https://crrev.com/f01e1802853692c9b1e758e7b2a1029d4ccf655e
Cr-Commit-Position: refs/heads/master@{#326700}