Convert the AutocompletePopupPositioner into a BubblePositioner in preparatio...

Convert the AutocompletePopupPositioner into a BubblePositioner in preparation for using it to position both the Omnibox bubble and InfoBubbles.  It now gets the bounds of the location stack, which can be turned into useful coordinates for both items.

This should not result in any visible change.

BUG=21028
TEST=none

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

[Scott Hess - ex-Googler, 2009-09-15 17:49:57]

LGTM for Mac code.  I patched it in and the omnibox popup seemed to end up at
the right place.

http://codereview.chromium.org/194110/diff/1061/1064
File chrome/browser/autocomplete/autocomplete_popup_view.h (right):

http://codereview.chromium.org/194110/diff/1061/1064#newcode35
Line 35: virtual gfx::Rect GetLocationStackBounds() const = 0;
This seemed kinda weird to me.  I could see having a "BubblePositioner" with a
generic member function like GetPosition().  Then we could pass around a generic
positioner that different code can use for positioning different things.  But
I'm not sure I understand why BubblePositioner would be declared in
autocomplete_popup_view.h and have a GetLocationStackBounds() member function. 
Is this intended to move somewhere else and grow additional functions?

http://codereview.chromium.org/194110/diff/1061/1065
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/194110/diff/1061/1065#newcode348
Line 348: NSRect r =
NSRectFromCGRect(bubble_positioner_->GetLocationStackBounds().ToCGRect());
Line too wide!

http://codereview.chromium.org/194110/diff/1061/1088
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/194110/diff/1061/1088#newcode517
Line 517: - (gfx::Rect)locationStackBounds {
Apropos nothing, I found myself wondering why this method didn't return NSRect,
with the NSRectToCGRect() call in the C++ wrapper.

http://codereview.chromium.org/194110/diff/1061/1088#newcode531
Line 531: NSMaxY(locationFrame));
I'm not sure this is the height that you intend.  You probably want
NSHeight(locationFrame).

That said, I think the height is ignored by the code which sets things up, which
is why it was previously 0.

[Peter Kasting, 2009-09-15 18:26:57]

http://codereview.chromium.org/194110/diff/1061/1064
File chrome/browser/autocomplete/autocomplete_popup_view.h (right):

http://codereview.chromium.org/194110/diff/1061/1064#newcode35
Line 35: virtual gfx::Rect GetLocationStackBounds() const = 0;
On 2009/09/15 17:49:57, shess wrote:
> This seemed kinda weird to me.  I could see having a "BubblePositioner" with a
> generic member function like GetPosition().  Then we could pass around a
generic
> positioner that different code can use for positioning different things.  But
> I'm not sure I understand why BubblePositioner would be declared in
> autocomplete_popup_view.h and have a GetLocationStackBounds() member function.

> Is this intended to move somewhere else and grow additional functions?

It should move elsewhere, I forgot to get to that.  Fixed.

The class doesn't need any other functions at the moment.  I don't understand
your proposal for a generic "GetPosition()" function since the needs here are
cross-platform, not specific to particular classes.

http://codereview.chromium.org/194110/diff/1061/1065
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/194110/diff/1061/1065#newcode348
Line 348: NSRect r =
NSRectFromCGRect(bubble_positioner_->GetLocationStackBounds().ToCGRect());
On 2009/09/15 17:49:57, shess wrote:
> Line too wide!

Fixed.

http://codereview.chromium.org/194110/diff/1061/1088
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/194110/diff/1061/1088#newcode517
Line 517: - (gfx::Rect)locationStackBounds {
On 2009/09/15 17:49:57, shess wrote:
> Apropos nothing, I found myself wondering why this method didn't return
NSRect,
> with the NSRectToCGRect() call in the C++ wrapper.

I don't have any idea.  I'm mucking blindly with this code that somebody else
wrote.  I don't know how to do the Inset() call on an NSRect so I'm going to
leave this to you to clean up as you wish.

http://codereview.chromium.org/194110/diff/1061/1088#newcode531
Line 531: NSMaxY(locationFrame));
On 2009/09/15 17:49:57, shess wrote:
> I'm not sure this is the height that you intend.  You probably want
> NSHeight(locationFrame).

Fixed.

> That said, I think the height is ignored by the code which sets things up,
which
> is why it was previously 0.

It is ignored by the Omnibox positioning code.  If you start using it to place
InfoBubbles like Windows does, it will be needed there.

[Scott Hess - ex-Googler, 2009-09-15 20:54:34]

BTW, I was considering my review drive-by, so LGTM.

[Ben Goodger (Google), 2009-09-15 21:09:06]

OK

http://codereview.chromium.org/194110/diff/8001/2061
File chrome/browser/bubble_positioner.h (right):

http://codereview.chromium.org/194110/diff/8001/2061#newcode19
Line 19: virtual gfx::Rect GetLocationStackBounds() const = 0;
I assume these comments and function name will change when you convert the
infobubbles?

[Peter Kasting, 2009-09-15 22:26:05]

http://codereview.chromium.org/194110/diff/8001/2061
File chrome/browser/bubble_positioner.h (right):

http://codereview.chromium.org/194110/diff/8001/2061#newcode19
Line 19: virtual gfx::Rect GetLocationStackBounds() const = 0;
On 2009/09/15 21:09:06, Ben Goodger wrote:
> I assume these comments and function name will change when you convert the
> infobubbles?

The comments will change, but not the function name.  Why would the function
name change?