Implement a mostly working InfoBubble with a shim BookmarkBubble.

Implement a mostly working InfoBubble with a shim BookmarkBubble.

This ended up being implemented as a toplevel instead of a popup, to handle
things like virtual desktop switching.  I imagine there are some other problems
we might hit, like the window getting decorated, etc.  Although I think the
shape mask might prevent decorations from being visible.  This is not pixel
perfect with Windows, since we're not anti-aliasing the frame border.

[Dean McNamee, 2009-04-30 13:10:56]

Oh man, so involved.  I actually implemented the anti-aliasing so we were pixel
perfect with Windows, but it made 3 subtly different copies of the frame polygon
code, and was a bit of a mess.

[Evan Martin, 2009-04-30 15:22:54]

I have some concerns about whether this works with all window managers (in
particular that code where you try to reposition yourself when moved might cause
trouble by fighting).  I also wonder whether you should pass in the parent
window so you can set your popup as a transient.

But this LGTM as a start -- we'll need to experiment on a few people's boxes
with this code to see where we need to go.

[ecattell, 2009-04-30 19:55:37]

http://codereview.chromium.org/100203/diff/17/18
File chrome/browser/gtk/bookmark_bubble_gtk.cc (right):

http://codereview.chromium.org/100203/diff/17/18#newcode25
Line 25: DCHECK(bubble);
Please provide some logging output for this check...

DCHECK(bubble) << "This will happen if ...";

Anything that will help someone troubleshoot what is going on...such as the
input values that caused the check.

http://codereview.chromium.org/100203/diff/17/19
File chrome/browser/gtk/bookmark_bubble_gtk.h (right):

http://codereview.chromium.org/100203/diff/17/19#newcode9
Line 9: #include "googleurl/src/gurl.h"
you should use forward declares rather than including this:

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Header_File_Dependenc...

http://codereview.chromium.org/100203/diff/17/19#newcode18
Line 18: class BookmarkBubbleGtk {
Since you only have one static function, you should probably add
DISALLOW_IMPLICIT_CONSTRUCTORS() to forbid people from creating this class.

http://codereview.chromium.org/100203/diff/17/19#newcode20
Line 20: static void Show(const gfx::Rect& rect,
Please add comments for this function.

http://codereview.chromium.org/100203/diff/17/22
File chrome/browser/gtk/info_bubble_gtk.h (right):

http://codereview.chromium.org/100203/diff/17/22#newcode8
Line 8: #include "base/basictypes.h"
Out of order includes:

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In...

http://codereview.chromium.org/100203/diff/17/22#newcode20
Line 20: // of |rect|.  The bubble will host |widget| as the content.
widget -> content?

http://codereview.chromium.org/100203/diff/17/22#newcode26
Line 26: void Close();
What does Close() do?  And if it is just a wrapper around
Close(closed_by_escape), then you should keep it inline.

http://codereview.chromium.org/100203/diff/17/22#newcode32
Line 32: // Closes the window notifying the delegate. |closed_by_escape| is true
if
notifying -> and notifies

http://codereview.chromium.org/100203/diff/17/23
File chrome/browser/gtk/toolbar_star_toggle_gtk.cc (right):

http://codereview.chromium.org/100203/diff/17/23#newcode35
Line 35: gint x, y;
Please initialize basic types (like ints)

[Dean McNamee, 2009-05-01 15:01:46]

Thanks for the great review, changes addressed in:

http://codereview.chromium.org/99276

http://codereview.chromium.org/100203/diff/17/18
File chrome/browser/gtk/bookmark_bubble_gtk.cc (right):

http://codereview.chromium.org/100203/diff/17/18#newcode25
Line 25: DCHECK(bubble);
On 2009/04/30 19:55:37, ecattell wrote:
> Please provide some logging output for this check...
> 
> DCHECK(bubble) << "This will happen if ...";
> 
> Anything that will help someone troubleshoot what is going on...such as the
> input values that caused the check.

I generally try to avoid DCHECK strings unless they're super useful.  If you
work on Chrome, you know how to check down a DCHECK firing.  Also, the strings
end up in our Release binaries (long story).

I don't have anything particularly interesting to say about this, it should
never fail.

http://codereview.chromium.org/100203/diff/17/22
File chrome/browser/gtk/info_bubble_gtk.h (right):

http://codereview.chromium.org/100203/diff/17/22#newcode8
Line 8: #include "base/basictypes.h"
Fixed, sorry.

On 2009/04/30 19:55:37, ecattell wrote:
> Out of order includes:
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In...

http://codereview.chromium.org/100203/diff/17/22#newcode20
Line 20: // of |rect|.  The bubble will host |widget| as the content.
On 2009/04/30 19:55:37, ecattell wrote:
> widget -> content?

Fixed.

http://codereview.chromium.org/100203/diff/17/22#newcode26
Line 26: void Close();
On 2009/04/30 19:55:37, ecattell wrote:
> What does Close() do?  And if it is just a wrapper around
> Close(closed_by_escape), then you should keep it inline.

Heh, that's a good question.  I am not even sure if I need it yet, part of this
was based on the Windows interface, but they use a different UI system, so some
of this stuff was required for virtual overriding the behavior.  I am working on
implementing more of the bookmark bubble and I'll have an idea if I should drop
it, or what it should do.

http://codereview.chromium.org/100203/diff/17/22#newcode32
Line 32: // Closes the window notifying the delegate. |closed_by_escape| is true
if
On 2009/04/30 19:55:37, ecattell wrote:
> notifying -> and notifies

Yeah, and I don't actually even have a delegate right now, so this comment is
super wrong.

http://codereview.chromium.org/100203/diff/17/23
File chrome/browser/gtk/toolbar_star_toggle_gtk.cc (right):

http://codereview.chromium.org/100203/diff/17/23#newcode35
Line 35: gint x, y;
On 2009/04/30 19:55:37, ecattell wrote:
> Please initialize basic types (like ints)

It is initialized on the next line, do you still think it needs to be set it to
0?

[Evan Martin, 2009-05-01 15:33:50]

On 2009/05/01 15:01:46, Dean McNamee wrote:
> http://codereview.chromium.org/100203/diff/17/23
> File chrome/browser/gtk/toolbar_star_toggle_gtk.cc (right):
> 
> http://codereview.chromium.org/100203/diff/17/23#newcode35
> Line 35: gint x, y;
> On 2009/04/30 19:55:37, ecattell wrote:
> > Please initialize basic types (like ints)
> 
> It is initialized on the next line, do you still think it needs to be set it
to
> 0?

Some compilers (and maybe valgrind?) will complain about this, since they don't
know x/y are out params.  In fact, I would suspect GTK might have the
annotations in place to tell GCC that they're out params just for that reason...