Restyle omnibox popup

This change the native omnibox to look and behave a bit more like the HTML Instant Extended one.

The popup positioning logic now has to be done at a bit of a higher level since it needs to know the window width as well as just the omnibox width. I added a new function in LocationBar::Delegate to get this that's implemented by the toolbar view (the lowest-level UI component that knows about the window width as well as the omnibox width).

More information must now be passed to the popup, so I added a new OmniboxViewDelegate that's implemented by the LocationBar to give the popup its context. This cleaned up some dependencies where the popup had to know about the location bar and did things silly things like this:
location_bar_->GetWidget()->GetNativeView()

R=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204715

[brettw, 2013-06-04 20:53:26]

I have a few things todo but I'd like to get this started since we're in a bit
of a rush. I'm going to start testing Views Linux now, currently have not tested
on any non-Windows platform.

The max search suggestions from 3 -> 5 is up in the air. Currently talking with
mpearson and doing some testing to see if this is right. I'd ignore this for
now.

I want to remove the logo from the popup (requires a bunch of complexity if your
provider isn't Google and it looks stupid if you only have one suggestion) but
am awaiting a bit more guidance on this before removing the code. So I'd ignore
the provider logo in the popup as well for now.

[brettw, 2013-06-04 23:43:12]

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:639: int num_matches =
kMaxSuggestMatches * 4;
This change needs to be reverted.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1109: const size_t
max_total_matches = kMaxSuggestMatches + verbatim_matches_size;
Ditto

[Peter Kasting, 2013-06-06 18:52:15]

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:639: int num_matches =
kMaxSuggestMatches * 4;
On 2013/06/04 23:43:12, brettw wrote:
> This change needs to be reverted.

I assume you wrote this because you've reverted them locally but didn't want to
upload a new patch.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:100:
provider_logo_->set_owned_by_client();
Why use set_owned_by_client() and a scoped_ptr instead of using a raw pointer
and having normal views ownership?  You'd probably want to move this block of
code to Init() but that doesn't seem like a problem.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:144:
provider_logo_->SetPosition(gfx::Point(5, 5));
These numbers are pretty magic

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:148: int width =
contents_rect.width() - left_margin_ - right_margin_;
Or just:

  contents_rect.Inset(left_margin_, outside_vertical_padding_, right_margin_,
                      0);

And then use the rect coords below.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:441:
//OnPaintBorder(canvas);
Remove this

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:463: void
OmniboxPopupContentsView::MakeContentsPath(
This function can just be inlined into its lone caller now.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:466: SkRect rect;
Can avoid this temp by using the 4-arg version of SkPath::addRect().

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:503: gfx::Rect
OmniboxPopupContentsView::CalculateTargetBounds(int h) {
This can be inlined into its caller too

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:250: int
OmniboxResultView::GetTextHeight() const {
Nit: Move this definition up to continue to match the header declaration order

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_delegate.h (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_delegate.h:20: class
OmniboxViewDelegate {
Since the only implementer of this is the LocationBarView, and all the classes
in question are views code that know about LocationBarView (it's in the same
source directory), it seems like we could just add these functions directly to
that class and then have the classes that access them take a LocationBarView*. 
Sure, that makes it less explicit which pieces of the LBV those classes will
need to access, but I think having fewer delegates/interfaces is worth that
tradeoff.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_delegate.h:32: // Returns the
native widget to be the parent of the popup.
Nit: to be -> that is?

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:185: views::View* webview_;
You don't seem to make use of this new member

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.cc:383: void
ToolbarView::GetOmniboxPopupPositioningInfo(
It seems like LocationBarView could implement this function directly; every
place we use |this| it could simply use its parent view.  It doesn't even need
to know that that's a ToolbarView.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.cc:390: if (border) {
When is this true?

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.cc:399:
location_bar_bounds.Inset(LocationBarView::kNormalHorizontalEdgeThickness,
Note: You'll need to update this for recent changes in the LocationBarView.  (I
think you just need to use kNormalEdgeThickness.)

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/toolbar_view.h (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.h:50: // the omnibox popup.
This sentence is a run on sentence this is.

Also I don't know what it's supposed to mean, maybe this is part of an aborted
change?

[brettw, 2013-06-06 20:46:49]

I addressed everything except the comment below.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.cc:390: if (border) {
On 2013/06/06 18:52:16, Peter Kasting wrote:
> When is this true?

I have no idea. This code is just moved from
OmniboxPopupContentsView::CalculateTargetBounds

[Peter Kasting, 2013-06-06 21:01:11]

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.cc:383: void
ToolbarView::GetOmniboxPopupPositioningInfo(
On 2013/06/06 18:52:16, Peter Kasting wrote:
> It seems like LocationBarView could implement this function directly; every
> place we use |this| it could simply use its parent view.  It doesn't even need
> to know that that's a ToolbarView.

You didn't do this.

https://codereview.chromium.org/15745031/diff/10001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.cc:390: if (border) {
On 2013/06/06 20:46:49, brettw wrote:
> On 2013/06/06 18:52:16, Peter Kasting wrote:
> > When is this true?
> 
> I have no idea. This code is just moved from
> OmniboxPopupContentsView::CalculateTargetBounds

I tracked this down.  This was added for the app launcher code that no longer
exists.  Just nuke this.

[brettw, 2013-06-06 22:45:51]

PTAL. I got the two delegate things conflated which is why I missed the second
one. This one looks even better.

I removed the extra border thing also.

[Peter Kasting, 2013-06-06 22:52:03]

LGTM

https://codereview.chromium.org/15745031/diff/116001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/15745031/diff/116001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:439: // appear at the
same height as the Star bubble.
Nit: I'd just nuke this comment (and the blank line above it); the first half
should already be well-known to someone reading this code, and the second half
is no longer relevant.

Also, it seems like maybe you'd want to inset (both sides?) by GetItemPadding()
as well?  I suspect this currently happens in the code that draws the rows
inside the popup, at least for the left side, but maybe it'd be simpler or
clearer to do it here.  Insetting both ends by GetItemPadding() would mean you'd
have the exact offset that the icon should start at as well as the exact width
until we elide the omnibox text.

[brettw, 2013-06-06 23:28:56]

On 2013/06/06 22:52:03, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/15745031/diff/116001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/15745031/diff/116001/chrome/browser/ui/views/...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:439: // appear at
the
> same height as the Star bubble.
> Nit: I'd just nuke this comment (and the blank line above it); the first half
> should already be well-known to someone reading this code, and the second half
> is no longer relevant.

Okay, I removed this.


> Also, it seems like maybe you'd want to inset (both sides?) by
GetItemPadding()
> as well?  I suspect this currently happens in the code that draws the rows
> inside the popup, at least for the left side, but maybe it'd be simpler or
> clearer to do it here.  Insetting both ends by GetItemPadding() would mean
you'd
> have the exact offset that the icon should start at as well as the exact width
> until we elide the omnibox text.

I'd very much prefer not to turn this into a bigger refactor than necessary
right now.

[Peter Kasting, 2013-06-06 23:54:23]

On 2013/06/06 23:28:56, brettw wrote:
> 
> > Also, it seems like maybe you'd want to inset (both sides?) by
> GetItemPadding()
> > as well?  I suspect this currently happens in the code that draws the rows
> > inside the popup, at least for the left side, but maybe it'd be simpler or
> > clearer to do it here.  Insetting both ends by GetItemPadding() would mean
> you'd
> > have the exact offset that the icon should start at as well as the exact
width
> > until we elide the omnibox text.
> 
> I'd very much prefer not to turn this into a bigger refactor than necessary
> right now.

OK, I didn't intend for it to be more than changing one line here and maybe one
line over there, depending on what the other code does.  I mostly just don't
know if the other code is correct or not, and it seemed like doing all the
necessary insetting here instead of just half would be easier to understand and
keep accurate.

[brettw, 2013-06-07 05:02:17]

Committed patchset #13 manually as r204715 (presubmit successful).

[Lyricconch, 2013-07-10 14:04:46]

On 2013/06/07 05:02:17, brettw wrote:
> Committed patchset #13 manually as r204715 (presubmit successful).

is there any good reason for this design?
with 1024*768 screen and 3-4 extension icons, 
it eats almost 1/6 width just showing "side blank".

"blank-urls-blank" style of address-bar drop-down is very annoying.
most time drop-down list disappears soon after appears, 
so that blankS are flashing on page content.

[Peter Kasting, 2013-07-10 16:38:10]

CLs are not the place to debate design decisions after the fact.

The original omnibox dropdown covered most of this same area.  Eventually the
new one will fill the left blank with an appropriate logo.