Quick reimplementation of StatusBubbleGtk to not suck as much.

Quick reimplementation of StatusBubbleGtk to not suck as much.

The TabContentsContainerGtk now uses a GtkFixed to store its
children, and the status bubble is now a child of that GtkFixed
so that it can be absolutely positioned on top of the rendered
data. Since it is no longer a GTK_WINDOW_POPUP, all the weird
stuff related to different window managers goes away.

http://crbug.com/11635

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

[Evan Martin, 2009-05-19 19:27:09]

I am concerned this will regress the tab contents.  It seems it should not be so
invasive into its parent.

http://codereview.chromium.org/113590/diff/1008/18
File chrome/browser/gtk/status_bubble_gtk.cc (right):

http://codereview.chromium.org/113590/diff/1008/18#newcode20
Line 20: const int kInternalTopBottomPadding = 1;
comments on these?

http://codereview.chromium.org/113590/diff/1008/18#newcode83
Line 83: // Friday.
In test_shell we have a GtkFixed subclass that lets us control positioning
manually.  We'll need it for plugins anyway (which behave very similarly --
webkit wants total control over their positioning).  This comment is good; you
may just want to mention that there's a class to borrow from test_shell when we
get around to fixing it.

http://codereview.chromium.org/113590/diff/1008/18#newcode100
Line 100: // way) so totally ignore this message.
This sentence is missing word.

Maybe leave open the option for sliding, at least to the extent of hiding.

http://codereview.chromium.org/113590/diff/1008/18#newcode104
Line 104: label_ = gtk_label_new("Label");
I think you can pass in "" or even NULL to have it empty.

http://codereview.chromium.org/113590/diff/1008/20
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/113590/diff/1008/20#newcode15
Line 15: void ChildrenSizeAllocate(GtkWidget* widget, void* param) {
Needs a comment.

You can either make this function just take a GtkAllocation* (which is GTK
style) or do the reinterpret_cast as the first line of hte function (which is
Dean style)

http://codereview.chromium.org/113590/diff/1008/20#newcode155
Line 155: // Set all tab contents to the size of the allocation.
It seems this comment isn't accurate?

http://codereview.chromium.org/113590/diff/1008/21
File chrome/browser/gtk/tab_contents_container_gtk.h (right):

http://codereview.chromium.org/113590/diff/1008/21#newcode54
Line 54: // informed of its parrent and its parent's allocation (it makes a
decision
typo: parrent

http://codereview.chromium.org/113590/diff/1008/21#newcode71
Line 71: // displaying the current TabContents and bottom half displaying the
loading
Yes, I was thinking this might be the fix for the splitting.

I wonder if rather than "this used to be a" it would be better worded as "this
should not be a" (and with the rest of hte sentence changed to reflect that).

[Elliot Glaysher, 2009-05-19 19:59:39]

Updated.

http://codereview.chromium.org/113590/diff/1008/20
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/113590/diff/1008/20#newcode155
Line 155: // Set all tab contents to the size of the allocation.
On 2009/05/19 19:27:09, Evan Martin wrote:
> It seems this comment isn't accurate?

I do not understand this comment.

[Evan Martin, 2009-05-19 20:26:20]

I apologize for my dumb questions.

http://codereview.chromium.org/113590/diff/2001/3002
File chrome/browser/gtk/status_bubble_gtk.cc (right):

http://codereview.chromium.org/113590/diff/2001/3002#newcode55
Line 55: parent_ = parent;
Why can't this just be the size-allocate handler?

http://codereview.chromium.org/113590/diff/2001/3004
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/113590/diff/2001/3004#newcode20
Line 20: if (strcmp(gtk_widget_get_name(widget), "status-bubble") != 0) {
Aha!  I see why I was confused.  This test is checking that it *isn't* a status
bubble.  Stupid strcmp is so confusing.

http://codereview.chromium.org/113590/diff/2001/3004#newcode163
Line 163: container->status_bubble_->SetParentAllocation(fixed, allocation);
Should we be returning true/false here to prevent the normal handling?

[Elliot Glaysher, 2009-05-19 20:52:27]

http://codereview.chromium.org/113590/diff/2001/3002
File chrome/browser/gtk/status_bubble_gtk.cc (right):

http://codereview.chromium.org/113590/diff/2001/3002#newcode55
Line 55: parent_ = parent;
On 2009/05/19 20:26:21, Evan Martin wrote:
> Why can't this just be the size-allocate handler?

We need to change the allocation whenever the label gets a new value since we're
manually handling the requisition/allocation steps ourselves. If we set the
label's allocation now, it won't change when the text changes. The label starts
off empty, and the GtkFixed's size-allocate fires once the window is brought up
and the space requisitioned is the size of an empty label (plus borders, etc).
This size won't automatically change when the label is set since we're handling
all this manually, right?

http://codereview.chromium.org/113590/diff/2001/3004
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/113590/diff/2001/3004#newcode163
Line 163: container->status_bubble_->SetParentAllocation(fixed, allocation);
On 2009/05/19 20:26:21, Evan Martin wrote:
> Should we be returning true/false here to prevent the normal handling?

size-allocate doesn't return a value.
http://library.gnome.org/devel/gtk/unstable/GtkWidget.html#GtkWidget-size-all...

[Evan Martin, 2009-05-19 21:07:37]

On 2009/05/19 20:52:27, Elliot Glaysher wrote:
> This size won't automatically change when the label is set since we're
handling
> all this manually, right?

I'm not sure -- it seems GtkFixed respects the child's requested size (note that
it has no width/height control, only x/y).

[Elliot Glaysher, 2009-05-19 22:25:25]

On 2009/05/19 21:07:37, Evan Martin wrote:
> On 2009/05/19 20:52:27, Elliot Glaysher wrote:
> > This size won't automatically change when the label is set since we're
> handling
> > all this manually, right?
> 
> I'm not sure -- it seems GtkFixed respects the child's requested size (note
that
> it has no width/height control, only x/y).

I've tried it where all the SetParentAllocation was ripped out and container_
had a custom size-allocate handler. It doesn't work and always aligns the status
bubble along the top.

- I can't set its position in the GtkFixed in the size-allocate without still
having SetParentAllocation pass the parent GtkFixed over.
- The allocation passed into container_'s size-allocate handler is only large
enough for |container_| and doesn't really help me with positioning.

[Evan Martin, 2009-05-19 22:27:23]

OK, LGTM for now.

Please run with valgrind and try opening and closing a lot of tabs as well as
visiting an SSL interstitial (try https://gutschke.com) just because I'm scared
of changes to the tab container.

http://codereview.chromium.org/113590/diff/2001/3003
File chrome/browser/gtk/status_bubble_gtk.h (right):

http://codereview.chromium.org/113590/diff/2001/3003#newcode69
Line 69: // bottom of this and doesn't portrude.
typo: protrude

[Evan Stade, 2009-05-19 23:41:25]

http://codereview.chromium.org/113590/diff/1013/1015
File chrome/browser/gtk/status_bubble_gtk.cc (right):

http://codereview.chromium.org/113590/diff/1013/1015#newcode128
Line 128: gtk_widget_set_app_paintable(container_.get(), TRUE);
why app paintable?

http://codereview.chromium.org/113590/diff/1013/1017
File chrome/browser/gtk/tab_contents_container_gtk.cc (right):

http://codereview.chromium.org/113590/diff/1013/1017#newcode87
Line 87: // It is possible to detach an unrealized, unparrented TabContents if
you
typo: unparented