Convert InfoBubble to using BubbleBorder. This also replaces the border grap...

Convert InfoBubble to using BubbleBorder.  This also replaces the border graphics with a slightly darker set from Nicholas.

The downside here is that the Linux views code is definitely going to be broken by this.  I'm not sure of the best design for Linux views, so I haven't implemented one.

BUG=21028
TEST=Open info bubbles (click star button, hover lock icon) and observe they are rendered with an identical border/shadow to the Omnibox dropdown, and at the same vertical position.

[sky, 2009-09-16 16:46:32]

http://codereview.chromium.org/195099/diff/1/23
File chrome/browser/views/bubble_border.cc (right):

http://codereview.chromium.org/195099/diff/1/23#newcode193
Line 193: SkBitmap* edge, * arrow;
nit: having the floating * feels odd (it looks like multiplication). If you
declare arrow on it's own line you avoid that.

http://codereview.chromium.org/195099/diff/1/18
File chrome/browser/views/bubble_border.h (right):

http://codereview.chromium.org/195099/diff/1/18#newcode45
Line 45: // set the arrow location to something other than NONE.
Rather than 'you should cal this iff', do you really mean this only matters if
you set an arrow location other than NONE?

http://codereview.chromium.org/195099/diff/1/22
File chrome/browser/views/info_bubble.cc (right):

http://codereview.chromium.org/195099/diff/1/22#newcode103
Line 103: // coordinates.  We will take up the entire parent window.
Is this really necessary? As BorderContents is set as the contents view
shouldn't it get resized to fill the bounds of the border widget?

http://codereview.chromium.org/195099/diff/1/22#newcode169
Line 169: SetWindowPos(owner, 0, 0, 0, 0,
Does this actually do anything?

http://codereview.chromium.org/195099/diff/1/22#newcode193
Line 193: };
nit: no ;

http://codereview.chromium.org/195099/diff/1/22#newcode196
Line 196: // ContentsView
---------------------------------------------------------------
This is the same as:

View* view = new View();
view->SetBackground(Background::CreateSolidBackground(kBackgroundColor));
view->AddChildView(contents);

Although your code doesn't layout the child nor ask for it's preferred size. You
sure you don't also want:

view->SetLayout(new FillLayout());

http://codereview.chromium.org/195099/diff/1/22#newcode278
Line 278: contents->GetPreferredSize(),
contents_view->GetPreferredSize(), but only once you've add the layout manager
as I suggest above.

http://codereview.chromium.org/195099/diff/1/22#newcode288
Line 288: contents->SetBounds(contents_bounds);
SetBounds should resize the contents_view, which should in turn size the
contents for you (once you set a layout manager as I suggest above). Do you
really need to set the bounds of contents or contents_view?

http://codereview.chromium.org/195099/diff/1/20
File chrome/browser/views/info_bubble.h (right):

http://codereview.chromium.org/195099/diff/1/20#newcode58
Line 58: 
nit: nuke the newline here.

http://codereview.chromium.org/195099/diff/1/21
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/195099/diff/1/21#newcode1065
Line 1065: : info_bubble_(NULL),
The : should be indented 4 spaces.

http://codereview.chromium.org/195099/diff/1/21#newcode1141
Line 1141: : LocationBarImageView(bubble_positioner),
nit: indent : 4 spaces

http://codereview.chromium.org/195099/diff/1/19
File chrome/browser/views/location_bar_view.h (right):

http://codereview.chromium.org/195099/diff/1/19#newcode288
Line 288: // A positioner used to give the info bubble the correct target
bounds.
Can you mention who owns this.

[Peter Kasting, 2009-09-16 19:55:09]

http://codereview.chromium.org/195099/diff/1/23
File chrome/browser/views/bubble_border.cc (right):

http://codereview.chromium.org/195099/diff/1/23#newcode193
Line 193: SkBitmap* edge, * arrow;
On 2009/09/16 16:46:32, sky wrote:
> nit: having the floating * feels odd (it looks like multiplication). If you
> declare arrow on it's own line you avoid that.

Done (though I'm surprised this is weird to you, it's standard C)

http://codereview.chromium.org/195099/diff/1/18
File chrome/browser/views/bubble_border.h (right):

http://codereview.chromium.org/195099/diff/1/18#newcode45
Line 45: // set the arrow location to something other than NONE.
On 2009/09/16 16:46:32, sky wrote:
> Rather than 'you should cal this iff', do you really mean this only matters if
> you set an arrow location other than NONE?

Yes, though I had wanted to emphasize that people really should call this if
they call that; but I've changed the comment.

http://codereview.chromium.org/195099/diff/1/22
File chrome/browser/views/info_bubble.cc (right):

http://codereview.chromium.org/195099/diff/1/22#newcode103
Line 103: // coordinates.  We will take up the entire parent window.
On 2009/09/16 16:46:32, sky wrote:
> Is this really necessary? As BorderContents is set as the contents view
> shouldn't it get resized to fill the bounds of the border widget?

Apparently not necessary.  Removed.

http://codereview.chromium.org/195099/diff/1/22#newcode169
Line 169: SetWindowPos(owner, 0, 0, 0, 0,
On 2009/09/16 16:46:32, sky wrote:
> Does this actually do anything?

It sets the Z-order to be just above the owner.

http://codereview.chromium.org/195099/diff/1/22#newcode193
Line 193: };
On 2009/09/16 16:46:32, sky wrote:
> nit: no ;

Done.

http://codereview.chromium.org/195099/diff/1/22#newcode196
Line 196: // ContentsView
---------------------------------------------------------------
On 2009/09/16 16:46:32, sky wrote:
> This is the same as:
> 
> View* view = new View();
> view->SetBackground(Background::CreateSolidBackground(kBackgroundColor));
> view->AddChildView(contents);
> 
> Although your code doesn't layout the child nor ask for it's preferred size.
You
> sure you don't also want:
> 
> view->SetLayout(new FillLayout());

I added all this stuff and ripped out ContentsView.

http://codereview.chromium.org/195099/diff/1/22#newcode278
Line 278: contents->GetPreferredSize(),
On 2009/09/16 16:46:32, sky wrote:
> contents_view->GetPreferredSize(), but only once you've add the layout manager
> as I suggest above.

Done, but does it matter?

http://codereview.chromium.org/195099/diff/1/22#newcode288
Line 288: contents->SetBounds(contents_bounds);
On 2009/09/16 16:46:32, sky wrote:
> SetBounds should resize the contents_view, which should in turn size the
> contents for you (once you set a layout manager as I suggest above). Do you
> really need to set the bounds of contents or contents_view?

Apparently not.  Removed.

http://codereview.chromium.org/195099/diff/1/20
File chrome/browser/views/info_bubble.h (right):

http://codereview.chromium.org/195099/diff/1/20#newcode58
Line 58: 
On 2009/09/16 16:46:32, sky wrote:
> nit: nuke the newline here.

Done.

http://codereview.chromium.org/195099/diff/1/21
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/195099/diff/1/21#newcode1065
Line 1065: : info_bubble_(NULL),
On 2009/09/16 16:46:32, sky wrote:
> The : should be indented 4 spaces.

Done.

http://codereview.chromium.org/195099/diff/1/21#newcode1141
Line 1141: : LocationBarImageView(bubble_positioner),
On 2009/09/16 16:46:32, sky wrote:
> nit: indent : 4 spaces

Done.

http://codereview.chromium.org/195099/diff/1/19
File chrome/browser/views/location_bar_view.h (right):

http://codereview.chromium.org/195099/diff/1/19#newcode288
Line 288: // A positioner used to give the info bubble the correct target
bounds.
On 2009/09/16 16:46:32, sky wrote:
> Can you mention who owns this.

Done.

[sky, 2009-09-16 20:06:16]

LGTM, but make sure it doesn't break views on linux.

  -Scott