Shift omnibox dropdown in and up on Windows, and square off the top, so it co...

Shift omnibox dropdown in and up on Windows, and square off the top, so it connects to the location bar.  Also fix info bubble positioning against location bar icons to put the arrow "up against the icon edge" (fixes the arrow overlapping some page action icons).

Remove BubblePositioner, which is now no longer needed.

BUG=27570, 40730
TEST=Omnibox dropdown should line up with editable area edges, icons and text should line up with icon and text in the omnibox.  Info bubbles should still be positioned correctly 

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44268

[Peter Kasting, 2010-04-08 01:17:56]

shess: Mac
estade: Linux
beng: Everything else

MAC AND LINUX NOTE: You should patch this in locally and test the omnibox
dropdown positioning (once the change passes the trybots).  I think I got it
right on Mac.  I'm almost certain I got it wrong on Linux.

[Scott Hess - ex-Googler, 2010-04-08 01:30:36]

LGTM, but I won't be able to patch it in locally for Mac until tomorrow.

http://codereview.chromium.org/1578021/diff/1/8
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/1578021/diff/1/8#newcode329
chrome/browser/autocomplete/autocomplete_popup_view_mac.mm:329: const int
kLocationStackEdgeWidth = 1;
Since "location stack" refers to the group of things this is no longer based on,
maybe kBorderWidth instead?

[Or just leave it alone - I have a small resolution-independence bug to fix in
this very code.]

[Scott Hess - ex-Googler, 2010-04-08 17:29:27]

On 2010/04/08 01:30:36, shess wrote:
> LGTM, but I won't be able to patch it in locally for Mac until tomorrow.

Patched it in on Mac and it runs fine.

I notice that the trybots aren't giving you a thumbs-up.  I have another change
I'd like to land in autocomplete_popup_view_mac.mm which is going to conflict
with your change, so I'd prefer to have yours in first.  Would you mind horribly
if I hived off the Mac-specific part of this change and got that checked in, or
would that cause too much merge conflict?

[Ben Goodger (Google), 2010-04-08 18:12:39]

Windows/Views bits LGTM.

[Evan Stade, 2010-04-08 18:58:08]

http://codereview.chromium.org/1578021/diff/25002/12030
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/1578021/diff/25002/12030#newcode201
chrome/browser/gtk/location_bar_view_gtk.cc:201: hbox_.get()));
I don't think you need to pass hbox_ at all, you can just use the location
entry's widget, no?

[Peter Kasting, 2010-04-08 19:18:40]

http://codereview.chromium.org/1578021/diff/25002/12030
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/1578021/diff/25002/12030#newcode201
chrome/browser/gtk/location_bar_view_gtk.cc:201: hbox_.get()));
On 2010/04/08 18:58:08, Evan Stade wrote:
> I don't think you need to pass hbox_ at all, you can just use the location
> entry's widget, no?

Doesn't the location entry widget not include the location icon, content blocked
icons, page action icons, star, etc.?

[Peter Kasting, 2010-04-08 19:36:12]

Sigh... after trying to fight with the "Gtk Views versus non-Views" compile
issues, I finally gave in and just #ifdefed side-by-side codepaths.  Ugly, but I
think it works.  If there's a better way, let me know.

[Peter Kasting, 2010-04-08 19:39:48]

Added three more reviewers so they can apply this patch locally and see if it
breaks the omnibox dropdown positioning for three other cases:
oshima: compact location bar
jcivelli: app launcher
sky: chromeos omnibox

If you guys can give me thumbs up or down, that'll help ensure I don't do
anything bad.  I'd like to land this today.

[Evan Stade, 2010-04-08 20:05:46]

nevermind what I said before. Anyway, LGTM with the positioning code as follows:


void AutocompletePopupViewGtk::Show(size_t num_results) {
  gint origin_x, origin_y;
  gdk_window_get_origin(location_bar_->window, &origin_x, &origin_y);
  const GtkAllocation& allocation = location_bar_->allocation;
  gtk_window_move(GTK_WINDOW(window_),
      origin_x + allocation.x - kBorderThickness,
      origin_y + allocation.y + allocation.height - kBorderThickness - 1);
  gtk_widget_set_size_request(window_,
      allocation.width + kBorderThickness * 2,
      num_results * kHeightPerResult + kBorderThickness * 2);
  gtk_widget_show(window_);
  StackWindow();
  opened_ = true;
}


On 2010/04/08 19:39:48, Peter Kasting wrote:
> Added three more reviewers so they can apply this patch locally and see if it
> breaks the omnibox dropdown positioning for three other cases:
> oshima: compact location bar
> jcivelli: app launcher
> sky: chromeos omnibox
> 
> If you guys can give me thumbs up or down, that'll help ensure I don't do
> anything bad.  I'd like to land this today.

[jcivelli, 2010-04-08 20:20:51]

You can actually test it on Windows by using --app-launcher-new-tab
Opening a new tab will open the app launcher (contents might be broken
right now, but the omnibox should be working).

Jay

On Thu, Apr 8, 2010 at 1:05 PM,  <estade@chromium.org> wrote:
> nevermind what I said before. Anyway, LGTM with the positioning code as
> follows:
>
>
> void AutocompletePopupViewGtk::Show(size_t num_results) {
>  gint origin_x, origin_y;
>  gdk_window_get_origin(location_bar_->window, &origin_x, &origin_y);
>  const GtkAllocation& allocation = location_bar_->allocation;
>  gtk_window_move(GTK_WINDOW(window_),
>      origin_x + allocation.x - kBorderThickness,
>      origin_y + allocation.y + allocation.height - kBorderThickness - 1);
>  gtk_widget_set_size_request(window_,
>      allocation.width + kBorderThickness * 2,
>      num_results * kHeightPerResult + kBorderThickness * 2);
>  gtk_widget_show(window_);
>  StackWindow();
>  opened_ = true;
> }
>
>
> On 2010/04/08 19:39:48, Peter Kasting wrote:
>>
>> Added three more reviewers so they can apply this patch locally and see if
>> it
>> breaks the omnibox dropdown positioning for three other cases:
>> oshima: compact location bar
>> jcivelli: app launcher
>> sky: chromeos omnibox
>
>> If you guys can give me thumbs up or down, that'll help ensure I don't do
>> anything bad.  I'd like to land this today.
>
>
>
> http://codereview.chromium.org/1578021/show
>

[oshima, 2010-04-08 21:15:19]

I got linkage error with patch4. (same as chromeos try bot had)

I just sync to TOT and and patch failed at location_bar_view.cc.
Can you sync & upload new patch?

On 2010/04/08 20:20:51, jcivelli_google.com wrote:
> You can actually test it on Windows by using --app-launcher-new-tab
> Opening a new tab will open the app launcher (contents might be broken
> right now, but the omnibox should be working).
> 
> Jay
> 
> On Thu, Apr 8, 2010 at 1:05 PM,  <mailto:estade@chromium.org> wrote:
> > nevermind what I said before. Anyway, LGTM with the positioning code as
> > follows:
> >
> >
> > void AutocompletePopupViewGtk::Show(size_t num_results) {
> > &nbsp;gint origin_x, origin_y;
> > &nbsp;gdk_window_get_origin(location_bar_->window, &origin_x, &origin_y);
> > &nbsp;const GtkAllocation& allocation = location_bar_->allocation;
> > &nbsp;gtk_window_move(GTK_WINDOW(window_),
> > &nbsp; &nbsp; &nbsp;origin_x + allocation.x - kBorderThickness,
> > &nbsp; &nbsp; &nbsp;origin_y + allocation.y + allocation.height -
kBorderThickness - 1);
> > &nbsp;gtk_widget_set_size_request(window_,
> > &nbsp; &nbsp; &nbsp;allocation.width + kBorderThickness * 2,
> > &nbsp; &nbsp; &nbsp;num_results * kHeightPerResult + kBorderThickness * 2);
> > &nbsp;gtk_widget_show(window_);
> > &nbsp;StackWindow();
> > &nbsp;opened_ = true;
> > }
> >
> >
> > On 2010/04/08 19:39:48, Peter Kasting wrote:
> >>
> >> Added three more reviewers so they can apply this patch locally and see if
> >> it
> >> breaks the omnibox dropdown positioning for three other cases:
> >> oshima: compact location bar
> >> jcivelli: app launcher
> >> sky: chromeos omnibox
> >
> >> If you guys can give me thumbs up or down, that'll help ensure I don't do
> >> anything bad. &nbsp;I'd like to land this today.
> >
> >
> >
> > http://codereview.chromium.org/1578021/show
> >
>

[Peter Kasting, 2010-04-08 21:30:18]

New snap up.  Makes app launcher look right (although the app launcher location
bar has no location icon in it, which looks weird...), and fixes ChromeOS build.

[Evan Stade, 2010-04-08 22:01:21]

http://codereview.chromium.org/1578021/diff/55004/60009
File chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc (right):

http://codereview.chromium.org/1578021/diff/55004/60009#newcode288
chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc:288: gint origin_x,
origin_y;
nit: indentation

[oshima, 2010-04-08 23:07:23]

I patched it manually and it worked as expected. I think this is ok for now on
chromeos side. I'll talk to Cole and will update it if necessary.

Just one minor request below.

http://codereview.chromium.org/1578021/diff/55004/60027
File chrome/browser/chromeos/compact_location_bar_view.cc (right):

http://codereview.chromium.org/1578021/diff/55004/60027#newcode37
chrome/browser/chromeos/compact_location_bar_view.cc:37: const int
kAutocompletePopupWidth = 700;
can you remove this?