aura: Fix launcher tooltips:

aura: Fix launcher tooltips:
1. Refactor out app_list_bubble_border so that it can be used for launcher
tooltip border.
2. UI polish: increase padding, make tooltip single line, reduce maximum
tooltip width.

BUG=133292
TEST=manual

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

[msw, 2012-08-03 06:32:21]

Driveby, mostly minor nits. This looks pretty decent; thanks!

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/bub...
File ui/views/bubble/bubble_frame_view.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/bub...
ui/views/bubble/bubble_frame_view.cc:95: // BubbleBackground also has a
reference to border. So we must also update it.
nit: Consider "Update the background, which relies on the border." or similar.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
File ui/views/bubble/imageless_bubble_border.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:131: gfx::ShadowValue(gfx::Point(0,
5), 30, SkColorSetARGB(0x72, 0, 0, 0)),
It seems odd to store a vector of ShadowValues for the single value it contains.
You don't need to change that now (I suppose it's useful for drawing);
but please use SetShadow here and nix the pointless array.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:200: bool
ImagelessBubbleBorder::ArrowAtTopOrBottom() const {
This should be removed (or just return !is_arrow_on_horizontal() temporarily).

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:207: bool
ImagelessBubbleBorder::ArrowOnLeftOrRight() const {
This should be removed (or just return is_arrow_on_horizontal() temporarily).

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:298: arrow_height_),
nit: leave this indented 4 more spaces, as it was.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:313: arrow_height_,
nit: leave this indented 4 more spaces, as it was.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:394: // Pads with kBoprderSize pixels
to leave space for border lines.
nit: "kBopderSize" misspelled and technically incorrect now.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
File ui/views/bubble/imageless_bubble_border.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:15: // Renders a border, similar to
the BubbleBorder completely using skia (as
Consider: "A BubbleBorder rendered with Skia drawing commands instead of
images." or similar.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:17: class VIEWS_EXPORT
ImagelessBubbleBorder : public BubbleBorder {
I dislike the name, but lack a good suggestion, maybe "BubbleBorderGeometric"?

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:19: 
nit: remove blank line.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:75: // Overridden from Border:
nit: match override comment style from above.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:86: // Offset in pixels relative the
default middle position.
nit: offset of what? Please clarify comment and/or rename.

[varunjain, 2012-08-03 15:17:43]

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/bub...
File ui/views/bubble/bubble_frame_view.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/bub...
ui/views/bubble/bubble_frame_view.cc:95: // BubbleBackground also has a
reference to border. So we must also update it.
On 2012/08/03 06:32:21, msw wrote:
> nit: Consider "Update the background, which relies on the border." or similar.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
File ui/views/bubble/imageless_bubble_border.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:131: gfx::ShadowValue(gfx::Point(0,
5), 30, SkColorSetARGB(0x72, 0, 0, 0)),
On 2012/08/03 06:32:21, msw wrote:
> It seems odd to store a vector of ShadowValues for the single value it
contains.
> You don't need to change that now (I suppose it's useful for drawing);
> but please use SetShadow here and nix the pointless array.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:200: bool
ImagelessBubbleBorder::ArrowAtTopOrBottom() const {
On 2012/08/03 06:32:21, msw wrote:
> This should be removed (or just return !is_arrow_on_horizontal() temporarily).

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:207: bool
ImagelessBubbleBorder::ArrowOnLeftOrRight() const {
On 2012/08/03 06:32:21, msw wrote:
> This should be removed (or just return is_arrow_on_horizontal() temporarily).

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:298: arrow_height_),
On 2012/08/03 06:32:21, msw wrote:
> nit: leave this indented 4 more spaces, as it was.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:313: arrow_height_,
On 2012/08/03 06:32:21, msw wrote:
> nit: leave this indented 4 more spaces, as it was.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.cc:394: // Pads with kBoprderSize pixels
to leave space for border lines.
On 2012/08/03 06:32:21, msw wrote:
> nit: "kBopderSize" misspelled and technically incorrect now.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
File ui/views/bubble/imageless_bubble_border.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:15: // Renders a border, similar to
the BubbleBorder completely using skia (as
On 2012/08/03 06:32:21, msw wrote:
> Consider: "A BubbleBorder rendered with Skia drawing commands instead of
> images." or similar.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:17: class VIEWS_EXPORT
ImagelessBubbleBorder : public BubbleBorder {
On 2012/08/03 06:32:21, msw wrote:
> I dislike the name, but lack a good suggestion, maybe "BubbleBorderGeometric"?

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:19: 
On 2012/08/03 06:32:21, msw wrote:
> nit: remove blank line.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:75: // Overridden from Border:
On 2012/08/03 06:32:21, msw wrote:
> nit: match override comment style from above.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3008/ui/views/bubble/ima...
ui/views/bubble/imageless_bubble_border.h:86: // Offset in pixels relative the
default middle position.
On 2012/08/03 06:32:21, msw wrote:
> nit: offset of what? Please clarify comment and/or rename.

Done.

[msw, 2012-08-03 17:37:36]

LGTM with a couple nits; thank you!

https://chromiumcodereview.appspot.com/10834140/diff/3010/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/3010/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.cc:122: views::BubbleBorder::NO_SHADOW),
nit: this fits on the line above.

https://chromiumcodereview.appspot.com/10834140/diff/3010/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.cc:130: gfx::Point(0, 5), 30,
SkColorSetARGB(0x72, 0, 0, 0)));
nit: please make the color a const, possibly the blur and offset values too.

[varunjain, 2012-08-03 17:47:38]

https://chromiumcodereview.appspot.com/10834140/diff/3010/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/3010/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.cc:122: views::BubbleBorder::NO_SHADOW),
On 2012/08/03 17:37:37, msw wrote:
> nit: this fits on the line above.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/3010/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.cc:130: gfx::Point(0, 5), 30,
SkColorSetARGB(0x72, 0, 0, 0)));
On 2012/08/03 17:37:37, msw wrote:
> nit: please make the color a const, possibly the blur and offset values too.

Done.

[sky, 2012-08-03 17:59:25]

LGTM - but change the name of BubbleBorderGeometric

https://chromiumcodereview.appspot.com/10834140/diff/5012/ash/launcher/launch...
File ash/launcher/launcher_tooltip_manager.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ash/launcher/launch...
ash/launcher/launcher_tooltip_manager.cc:85: };
DISALLOW...

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:16: class VIEWS_EXPORT
BubbleBorderGeometric : public BubbleBorder {
Surely there be a better name than this. I would take BubbleBorder2 over this
name.

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:40: int corner_radius() { return
corner_radius_; }
const (43, 46 and 49)

[msw, 2012-08-03 18:08:26]

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.cc:29: const gfx::Point kShadowOffset(0,
5);
Don't static init objects, use ints kShadowOffsetX/Y.

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:16: class VIEWS_EXPORT
BubbleBorderGeometric : public BubbleBorder {
On 2012/08/03 17:59:25, sky wrote:
> Surely there be a better name than this. I would take BubbleBorder2 over this
> name.

Hahaha, sorry Scott, I didn't think it was that bad. - Mike

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:83: int anchor_edge_offset_;
From this new value and its uses, it looks like you're trying to get the
bubble's arrow point to lay within the anchor view's bounds by some amount (or
perhaps moving the bubble further away from the anchor view's bounds).

I've handled this in the past by having BubbleDelegateView implementations
override GetAnchorRect() and inset or outset the rect by the desired amount. Can
you try that approach?

[varunjain, 2012-08-03 18:22:42]

https://chromiumcodereview.appspot.com/10834140/diff/5012/ash/launcher/launch...
File ash/launcher/launcher_tooltip_manager.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ash/launcher/launch...
ash/launcher/launcher_tooltip_manager.cc:85: };
On 2012/08/03 17:59:25, sky wrote:
> DISALLOW...

Done.

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.cc:29: const gfx::Point kShadowOffset(0,
5);
On 2012/08/03 18:08:26, msw wrote:
> Don't static init objects, use ints kShadowOffsetX/Y.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:16: class VIEWS_EXPORT
BubbleBorderGeometric : public BubbleBorder {
On 2012/08/03 17:59:25, sky wrote:
> Surely there be a better name than this. I would take BubbleBorder2 over this
> name.

BubbleBorder2 it is!

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:40: int corner_radius() { return
corner_radius_; }
On 2012/08/03 17:59:25, sky wrote:
> const (43, 46 and 49)

Done.

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:83: int anchor_edge_offset_;
On 2012/08/03 18:08:26, msw wrote:
> From this new value and its uses, it looks like you're trying to get the
> bubble's arrow point to lay within the anchor view's bounds by some amount (or
> perhaps moving the bubble further away from the anchor view's bounds).
> 
> I've handled this in the past by having BubbleDelegateView implementations
> override GetAnchorRect() and inset or outset the rect by the desired amount.
Can
> you try that approach?

I think it is better to provide the BubbleDelegateView the functionality to
explicitly set the offset rather than having to override GetAnchorRect() every
time. The current BubbleBorder also has this functionality
(override_arrow_offset_).

[msw, 2012-08-03 19:55:45]

Thanks for taking my thoughts into consideration, I think gfx::Insets is the
right way to go. I'll be quick to review when you upload a new patch set.

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:83: int anchor_edge_offset_;
On 2012/08/03 18:22:42, varunjain wrote:
> On 2012/08/03 18:08:26, msw wrote:
> > From this new value and its uses, it looks like you're trying to get the
> > bubble's arrow point to lay within the anchor view's bounds by some amount
(or
> > perhaps moving the bubble further away from the anchor view's bounds).
> > 
> > I've handled this in the past by having BubbleDelegateView implementations
> > override GetAnchorRect() and inset or outset the rect by the desired amount.
> Can
> > you try that approach?
> 
> I think it is better to provide the BubbleDelegateView the functionality to
> explicitly set the offset rather than having to override GetAnchorRect() every
> time. The current BubbleBorder also has this functionality
> (override_arrow_offset_).

To recap offline discussion (really for posterity):

1) I believe override_arrow_offset_ / SetArrowOffset moves the arrow point
alongside the edge of the anchor rect, whereas you wish to move the arrow point
further from (or closer within) the anchor rect bounds. Besides, SetArrowOffset
apparently has no callers, so perhaps that should be deprecated.

2) I propose that you add a "gfx::Insets anchor_inset_;" value and setter/getter
to BubbleDelegateView that is applied to the anchor rect in
BubbleDelegateView::GetAnchorRect().

Here's why: Using a gfx::Insets with gfx::Rect makes more sense than applying a
single "int x" like a gfx::Insets(x, x, x, x) value to the gfx::Rect in arrow
positioning calculations (which increases complexity, cognitive load, and
maintenance costs). Insets have very clearly defined behavior, whereas the old
bubble code used a single int value and applied it incorrectly when the bubble
was flipped (from the top of the anchor rect to the bottom). This will also
allow the several existing GetAnchorRect() overrides that just apply a
gfx::Insets to the anchor view's bounds to use this simpler mechanism (better
code elsewhere, yay!). Finally, this also allows users to apply different values
on each edge (if needed).

[varunjain, 2012-08-03 20:29:25]

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_geometric.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/5012/ui/views/bubble/bub...
ui/views/bubble/bubble_border_geometric.h:83: int anchor_edge_offset_;
On 2012/08/03 19:55:46, msw wrote:
> On 2012/08/03 18:22:42, varunjain wrote:
> > On 2012/08/03 18:08:26, msw wrote:
> > > From this new value and its uses, it looks like you're trying to get the
> > > bubble's arrow point to lay within the anchor view's bounds by some amount
> (or
> > > perhaps moving the bubble further away from the anchor view's bounds).
> > > 
> > > I've handled this in the past by having BubbleDelegateView implementations
> > > override GetAnchorRect() and inset or outset the rect by the desired
amount.
> > Can
> > > you try that approach?
> > 
> > I think it is better to provide the BubbleDelegateView the functionality to
> > explicitly set the offset rather than having to override GetAnchorRect()
every
> > time. The current BubbleBorder also has this functionality
> > (override_arrow_offset_).
> 
> To recap offline discussion (really for posterity):
> 
> 1) I believe override_arrow_offset_ / SetArrowOffset moves the arrow point
> alongside the edge of the anchor rect, whereas you wish to move the arrow
point
> further from (or closer within) the anchor rect bounds. Besides,
SetArrowOffset
> apparently has no callers, so perhaps that should be deprecated.
> 
> 2) I propose that you add a "gfx::Insets anchor_inset_;" value and
setter/getter
> to BubbleDelegateView that is applied to the anchor rect in
> BubbleDelegateView::GetAnchorRect().
> 
> Here's why: Using a gfx::Insets with gfx::Rect makes more sense than applying
a
> single "int x" like a gfx::Insets(x, x, x, x) value to the gfx::Rect in arrow
> positioning calculations (which increases complexity, cognitive load, and
> maintenance costs). Insets have very clearly defined behavior, whereas the old
> bubble code used a single int value and applied it incorrectly when the bubble
> was flipped (from the top of the anchor rect to the bottom). This will also
> allow the several existing GetAnchorRect() overrides that just apply a
> gfx::Insets to the anchor view's bounds to use this simpler mechanism (better
> code elsewhere, yay!). Finally, this also allows users to apply different
values
> on each edge (if needed).

Done.

[msw, 2012-08-03 20:42:00]

Fantastic! LGTM with nits, thank you!

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
File ui/views/bubble/bubble_border.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border.h:121: virtual int border_thickness() const;
nit: this function shouldn't be hacker_style, it should be CamelCase. Should be
an easy fix, it's only called in BubbleBorder and your new call sites.

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border.h:130: ArrowLocation arrow_loc) const;
nit: indent

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_2.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.cc:227: insets->Set(shadow_padding.top() +
arrow_height_,
nit: if the use of this arrow_height_ was incorrect as you claimed offline,
perhaps add a TODO to fix it later with some notion of why it's wrong.

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_2.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.h:36: int corner_radius() { return
corner_radius_; }
nit: make the function const:
int corner_radius() const { return corner_radius_; }

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.h:39: const int border_size() { return
border_size_; }
sky's comment was to make the function const, not the return value:
int border_size() const { return border_size_; }

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.h:42: const int arrow_height() { return
arrow_height_; }
ditto

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.h:45: const int arrow_width() { return
arrow_width_; }
ditto

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
File ui/views/bubble/bubble_delegate.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_delegate.h:75: void set_anchor_insets(const gfx::Insets&
anchor_insets) {
nit: if you name the parameter |insets| it's a one-liner:
void set_anchor_insets(const gfx::Insets& insets) { anchor_insets_ = insets; }

[varunjain, 2012-08-03 20:54:23]

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
File ui/views/bubble/bubble_border.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border.h:121: virtual int border_thickness() const;
On 2012/08/03 20:42:00, msw wrote:
> nit: this function shouldn't be hacker_style, it should be CamelCase. Should
be
> an easy fix, it's only called in BubbleBorder and your new call sites.

Done.

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border.h:130: ArrowLocation arrow_loc) const;
On 2012/08/03 20:42:00, msw wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_2.cc (right):

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.cc:227: insets->Set(shadow_padding.top() +
arrow_height_,
On 2012/08/03 20:42:00, msw wrote:
> nit: if the use of this arrow_height_ was incorrect as you claimed offline,
> perhaps add a TODO to fix it later with some notion of why it's wrong.

The use of arrow_height_ is correct over here since its calculating the insets
which should include the arrow size. It was incorrect in GetBounds().

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
File ui/views/bubble/bubble_border_2.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.h:36: int corner_radius() { return
corner_radius_; }
On 2012/08/03 20:42:00, msw wrote:
> nit: make the function const:
> int corner_radius() const { return corner_radius_; }

Done.

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.h:39: const int border_size() { return
border_size_; }
On 2012/08/03 20:42:00, msw wrote:
> sky's comment was to make the function const, not the return value:
> int border_size() const { return border_size_; }

Ha!.. how did I miss this! Thanks for catching. Done.

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.h:42: const int arrow_height() { return
arrow_height_; }
On 2012/08/03 20:42:00, msw wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_border_2.h:45: const int arrow_width() { return
arrow_width_; }
On 2012/08/03 20:42:00, msw wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
File ui/views/bubble/bubble_delegate.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/9004/ui/views/bubble/bub...
ui/views/bubble/bubble_delegate.h:75: void set_anchor_insets(const gfx::Insets&
anchor_insets) {
On 2012/08/03 20:42:00, msw wrote:
> nit: if you name the parameter |insets| it's a one-liner:
> void set_anchor_insets(const gfx::Insets& insets) { anchor_insets_ = insets; }

Done.

[commit-bot: I haz the power, 2012-08-03 20:56:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10834140/13003

[msw, 2012-08-03 20:57:53]

Still LGTM w/1 optional nit; fine to commit as-is though.

https://chromiumcodereview.appspot.com/10834140/diff/13003/ui/views/bubble/bu...
File ui/views/bubble/bubble_border.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/13003/ui/views/bubble/bu...
ui/views/bubble/bubble_border.h:121: virtual int BorderThickness() const;
minor nit: Should be GetBorderThickness().

[varunjain, 2012-08-03 21:10:25]

https://chromiumcodereview.appspot.com/10834140/diff/13003/ui/views/bubble/bu...
File ui/views/bubble/bubble_border.h (right):

https://chromiumcodereview.appspot.com/10834140/diff/13003/ui/views/bubble/bu...
ui/views/bubble/bubble_border.h:121: virtual int BorderThickness() const;
On 2012/08/03 20:57:54, msw wrote:
> minor nit: Should be GetBorderThickness().

Done.

[commit-bot: I haz the power, 2012-08-03 21:11:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10834140/4015

[commit-bot: I haz the power, 2012-08-03 23:01:38]

Try job failure for 10834140-4015 (retry) (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...