Subclassing the InfoBubble to handle content anchored bubbles

Subclassing the InfoBubble to handle anchoring bubbles basedon their content, not what they point to, like the one we wantto show for the extension apps.

BUG=41270
TEST=Run Chrome with --enable-extension-apps --app-launcher-new-tab and press the NewTab button. A panel should open that has a mini omnibox which should be situated right on top of the underlying omnibox.

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

[Peter Kasting, 2010-04-19 20:55:12]

Rather than adding set_arrow_offset() and defining a subclass, I wonder if a
better way might not be to add a new BubbleBorder::GetBounds() function
alongside the existing one which takes a contents rect instead of a contents
size?  Doing this right seems like it could obviate the need for you to write a
subclass at all.

http://codereview.chromium.org/1572045/diff/14020/51002
File chrome/browser/views/app_launcher.cc (right):

http://codereview.chromium.org/1572045/diff/14020/51002#newcode292
chrome/browser/views/app_launcher.cc:292: kNavigationBarBottomPadding;
Nit: Spacing change is weird, the old spacing was better.

http://codereview.chromium.org/1572045/diff/14020/51002#newcode313
chrome/browser/views/app_launcher.cc:313: AppLauncher* AppLauncher::Show(
Nit:

Show(A,
     B,
     C);
...when things fit that way (which they do here)

http://codereview.chromium.org/1572045/diff/14020/51004
File chrome/browser/views/app_launcher.h (right):

http://codereview.chromium.org/1572045/diff/14020/51004#newcode55
chrome/browser/views/app_launcher.h:55: // be in screen coordinates).
Nit: Update the comments here to say what |bubble_anchor| does.

http://codereview.chromium.org/1572045/diff/14020/51004#newcode58
chrome/browser/views/app_launcher.h:58: static AppLauncher* Show(Browser*
browser, const gfx::Rect& bounds,
Nit: One arg per line is slightly preferred

http://codereview.chromium.org/1572045/diff/14020/51007
File chrome/browser/views/bubble_border.cc (right):

http://codereview.chromium.org/1572045/diff/14020/51007#newcode109
chrome/browser/views/bubble_border.cc:109: int arrow_offset =
override_arrow_x_offset_ ? override_arrow_x_offset_ :
Nit: Calculate this where you need it, not at function top.

http://codereview.chromium.org/1572045/diff/14020/51005
File chrome/browser/views/info_bubble.h (right):

http://codereview.chromium.org/1572045/diff/14020/51005#newcode72
chrome/browser/views/info_bubble.h:72: 
Nit: Unneeded blank line

http://codereview.chromium.org/1572045/diff/14020/51005#newcode93
chrome/browser/views/info_bubble.h:93: // contents should use. 
|extra_x_arrow_offset| specifies how far into the
Nit: This comment change doesn't apply to this version of the patch.

http://codereview.chromium.org/1572045/diff/14020/51009
File chrome/browser/views/pinned_contents_info_bubble.cc (right):

http://codereview.chromium.org/1572045/diff/14020/51009#newcode24
chrome/browser/views/pinned_contents_info_bubble.cc:24: gfx::Insets insets2;
Nit: "insets2"?

http://codereview.chromium.org/1572045/diff/14020/51010
File chrome/browser/views/pinned_contents_info_bubble.h (right):

http://codereview.chromium.org/1572045/diff/14020/51010#newcode73
chrome/browser/views/pinned_contents_info_bubble.h:73: explicit
PinnedContentsInfoBubble(const gfx::Point& bubble_anchor)
Why are these three functions protected?  Shouldn't they all be private?

[Finnur, 2010-04-19 22:35:58]

Good points. All addressed, except for one:

On 2010/04/19 20:55:12, Peter Kasting wrote:
> Rather than adding set_arrow_offset() and defining a
> subclass, I wonder if a better way might not be to
> add a new BubbleBorder::GetBounds() function
> alongside the existing one which takes a contents
> rect instead of a contents size?  Doing this right
> seems like it could obviate the need for you to write a
> subclass at all.

Yeah, I initially went down that route and found that it added complexities to
the InfoBubble API with a (contents_rect) parameter that needed to be plumbed
all the way down to BubbleBorder (from InfoBubble to BorderWidget to
BorderContents to BubbleBorder) and most callers would not care about and hence
leave blank.

Furthermore, just adding a new GetBounds function to BubbleBorder is not enough
to get this working since BubbleBorder::Paint() also relies on the calculated
offset of the arrow, so we'd have to have some way to influence the arrow offset
anyway. GetBounds shouldn't have side-effects, so the logical addition would be
something like a set_arrow_offset() function.

And lastly, I'm not sure it is worth it spending a lot of effort in eliminating
the subclass with this CL because I think it would just make a comeback when we
start doing the rest of the work specified in the mock (ie. drawing the arrow
differently).

[Peter Kasting, 2010-04-19 23:03:54]

Rubber-stamp (didn't re-review).