Improvements to Linux InfoBubble and BookmarkBubble.

Improvements to Linux InfoBubble and BookmarkBubble.
  - Introduce a delegate to notify when the bubble is closed.
  - Destroy the objects when the widgets are destroyed.
  - Cleanup some style issues, add a bunch more comments.

[Evan Martin, 2009-05-01 15:32:13]

I leave it to ecattell to review.  I appreciate all the comments!  I should be
more demanding in the future.

http://codereview.chromium.org/99276/diff/1/3
File chrome/browser/gtk/bookmark_bubble_gtk.h (right):

http://codereview.chromium.org/99276/diff/1/3#newcode45
Line 45: virtual ~BookmarkBubbleGtk();
Chrome style is to not do virtual destructors when they're unnecessary, even to
the point of disabling the compiler warnings about it.  I lost half a day to
this once. :\

http://codereview.chromium.org/99276/diff/1/4
File chrome/browser/gtk/info_bubble_gtk.cc (right):

http://codereview.chromium.org/99276/diff/1/4#newcode185
Line 185: gtk_window_present(GTK_WINDOW(window_));
One idea to experiment with for making the window stay on top when you drag the
parent is gtk_window_set_transient_for().

[Dean McNamee, 2009-05-05 09:07:59]

poke

On 2009/05/01 15:32:13, Evan Martin wrote:
> I leave it to ecattell to review.  I appreciate all the comments!  I should be
> more demanding in the future.
> 
> http://codereview.chromium.org/99276/diff/1/3
> File chrome/browser/gtk/bookmark_bubble_gtk.h (right):
> 
> http://codereview.chromium.org/99276/diff/1/3#newcode45
> Line 45: virtual ~BookmarkBubbleGtk();
> Chrome style is to not do virtual destructors when they're unnecessary, even
to
> the point of disabling the compiler warnings about it.  I lost half a day to
> this once. :\
> 
> http://codereview.chromium.org/99276/diff/1/4
> File chrome/browser/gtk/info_bubble_gtk.cc (right):
> 
> http://codereview.chromium.org/99276/diff/1/4#newcode185
> Line 185: gtk_window_present(GTK_WINDOW(window_));
> One idea to experiment with for making the window stay on top when you drag
the
> parent is gtk_window_set_transient_for().

[ecattell, 2009-05-05 16:48:53]

Almost there ;-)

http://codereview.chromium.org/99276/diff/12/13
File chrome/browser/gtk/bookmark_bubble_gtk.cc (left):

http://codereview.chromium.org/99276/diff/12/13#oldcode21
Line 21: gtk_box_pack_start(GTK_BOX(content), gtk_label_new("Hej!"), TRUE, TRUE,
0);
What is Hej! ?

http://codereview.chromium.org/99276/diff/12/13
File chrome/browser/gtk/bookmark_bubble_gtk.cc (right):

http://codereview.chromium.org/99276/diff/12/13#newcode22
Line 22: gint g_last_active = 0;
we prefer static class members rather than global variables. But, what you have
is also acceptable.

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

http://codereview.chromium.org/99276/diff/12/14
File chrome/browser/gtk/bookmark_bubble_gtk.h (right):

http://codereview.chromium.org/99276/diff/12/14#newcode28
Line 28: // Shows the bookmark bubble, pointing at |rect|.
Is profile required?  what is newly_bookedmarked for?

Also, pointers should go after input-only variables:

http://www.corp.google.com/eng/doc/cppguide.xml#Function_Parameter_Ordering

http://codereview.chromium.org/99276/diff/12/14#newcode34
Line 34: // Implements the InfoBubbleGtkDelegate.
What does this function actually do? Save values, validate the close?  Clean up
memory?  etc...

http://codereview.chromium.org/99276/diff/12/14#newcode44
Line 44: // and interface, but this isn't performance critical so might as well.
and->an

http://codereview.chromium.org/99276/diff/12/14#newcode47
Line 47: static gboolean HandleDestroyThunk(GtkWidget* widget,
Thunk?

please explain why you have a wrapper around HandleDestroy

http://codereview.chromium.org/99276/diff/12/14#newcode52
Line 52: gboolean HandleDestroy();
What does Handledestroy do?

http://codereview.chromium.org/99276/diff/12/14#newcode61
Line 61: // We let the InfoBubble own our content, and then we delete ourselves
we delete ourself (there is only one of "us")

http://codereview.chromium.org/99276/diff/12/14#newcode63
Line 63: GtkWidget* content_;
Shouldn't these be scoped_ptrs if we delete them?

http://codereview.chromium.org/99276/diff/12/14#newcode66
Line 66: GtkWidget* combo_;
Who owns the memory for this? Were is it initialized? Aren't we responsible for
destroying it?

[Dean McNamee, 2009-05-05 17:22:27]

New snapshot.

http://codereview.chromium.org/99276/diff/12/13
File chrome/browser/gtk/bookmark_bubble_gtk.cc (left):

http://codereview.chromium.org/99276/diff/12/13#oldcode21
Line 21: gtk_box_pack_start(GTK_BOX(content), gtk_label_new("Hej!"), TRUE, TRUE,
0);
On 2009/05/05 16:48:53, ecattell wrote:
> What is Hej! ?

It's Danish for "Hi", it's just a placeholder, someone will have to write the
real bubble.

http://codereview.chromium.org/99276/diff/12/13
File chrome/browser/gtk/bookmark_bubble_gtk.cc (right):

http://codereview.chromium.org/99276/diff/12/13#newcode22
Line 22: gint g_last_active = 0;
On 2009/05/05 16:48:53, ecattell wrote:
> we prefer static class members rather than global variables. But, what you
have
> is also acceptable.
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Static_and_Global_Var...

I usually try to do the opposite, since it keeps things unrelated to the
interface out of the interface.  Once you see how long Chrome takes to build,
you'll appreciate not having to modify a header :)

http://codereview.chromium.org/99276/diff/12/14
File chrome/browser/gtk/bookmark_bubble_gtk.h (right):

http://codereview.chromium.org/99276/diff/12/14#newcode28
Line 28: // Shows the bookmark bubble, pointing at |rect|.
On 2009/05/05 16:48:53, ecattell wrote:
> Is profile required?  what is newly_bookedmarked for?
> 
> Also, pointers should go after input-only variables:
> 
> http://www.corp.google.com/eng/doc/cppguide.xml#Function_Parameter_Ordering

All I see about this is:

. When ordering function parameters, put all input-only parameters before any
output parameters. 

Profile is not an output parameter.

Both profile and newly_bookmarked are required, it's just they aren't use yet
(but they will be).  Profile is needed to get at the bookmark system. 
newly_bookmarked tells us if clicking the bubble just created this bookmark, I
commented it on the member down below.

http://codereview.chromium.org/99276/diff/12/14#newcode34
Line 34: // Implements the InfoBubbleGtkDelegate.
On 2009/05/05 16:48:53, ecattell wrote:
> What does this function actually do? Save values, validate the close?  Clean
up
> memory?  etc...

Well, it's just a notification.  I suppose I should say what I do when I get
notified.

http://codereview.chromium.org/99276/diff/12/14#newcode44
Line 44: // and interface, but this isn't performance critical so might as well.
On 2009/05/05 16:48:53, ecattell wrote:
> and->an

Evan says I should drop this anyway :\

http://codereview.chromium.org/99276/diff/12/14#newcode47
Line 47: static gboolean HandleDestroyThunk(GtkWidget* widget,
On 2009/05/05 16:48:53, ecattell wrote:
> Thunk?
> 
> please explain why you have a wrapper around HandleDestroy

I have to do this for most of our GTK code, because you only have C style
callbacks where you can supply a pointer.  Therefor if you want a method call,
you have to thunk it yourself.  We do it a ton, so I'm not sure there should be
a long explanation of why at every place we do it.

http://codereview.chromium.org/99276/diff/12/14#newcode63
Line 63: GtkWidget* content_;
On 2009/05/05 16:48:53, ecattell wrote:
> Shouldn't these be scoped_ptrs if we delete them?

No, it's kinda more of a handle than a pointer.  They are reference counted by
GTK.  In this case, content_ is destroyed when the uses closes the window (which
is what HandleDestroy handles).  We effectively just have a weak pointer.

http://codereview.chromium.org/99276/diff/12/14#newcode66
Line 66: GtkWidget* combo_;
On 2009/05/05 16:48:53, ecattell wrote:
> Who owns the memory for this? Were is it initialized? Aren't we responsible
for
> destroying it?

No, the content_ widget ends up owning it.  I would have to explain GTK
reference counting, it's kinda complicated to understand at first.

[ecattell, 2009-05-05 18:56:21]

LGTM

(Not sure how to approve CLs in chromium, hopefully it is the same as mondrian)