Implement GtkFloatingContainer

GTK: Implement GtkFloatingContainer and implement StatusBubble on top of it.

This introduces a hybrid GtkBin/GtkFixed container which exposes a signal to absolutely position widgets. This also fixes the current flickering issues with the status bubble.

http://crbug.com/11635
TEST=Goto a site with a long list of links (I used reddit.com) and move the mouse cursor up and down the list quickly. There shouldn't be flickering in the top left corner.

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

[tony, 2009-05-28 17:35:48]

Some small nits below. Otherwise, LGTM.  Might want to get a second vote.

http://codereview.chromium.org/115835/diff/18/1010
File chrome/browser/gtk/gtk_floating_container.cc (right):

http://codereview.chromium.org/115835/diff/18/1010#newcode171
Line 171: }
Do we want to gtk assert if the widget is not in the container?

http://codereview.chromium.org/115835/diff/18/1010#newcode259
Line 259: GetChild(GTK_FLOATING_CONTAINER(container), child);
Do you have to NULL check floating_child before using it below?

http://codereview.chromium.org/115835/diff/18/1010#newcode284
Line 284: 
Same question about NULL checking floating_child.

http://codereview.chromium.org/115835/diff/18/1011
File chrome/browser/gtk/gtk_floating_container.h (right):

http://codereview.chromium.org/115835/diff/18/1011#newcode7
Line 7: 
Please add a paragraph describing this gtk container and how it differs from
gtk_fixed.  Bonus points for an example usage.

[James Hawkins, 2009-05-28 17:45:20]

http://codereview.chromium.org/115835/diff/18/1010
File chrome/browser/gtk/gtk_floating_container.cc (right):

http://codereview.chromium.org/115835/diff/18/1010#newcode148
Line 148: GList* children = floating->floating_children;
nit: Maybe rename children to floating_child or child_ptr, etc.  Same usage
below.

[Evan Martin, 2009-05-28 18:04:46]

in general, lookin' sweet

http://codereview.chromium.org/115835/diff/18/1010
File chrome/browser/gtk/gtk_floating_container.cc (right):

http://codereview.chromium.org/115835/diff/18/1010#newcode21
Line 21: CHILD_PROP_0,
For what it's worth, all this property stuff is not really necessary and only
used for language bindings.  Your call on whether to leave it or not.

http://codereview.chromium.org/115835/diff/18/1010#newcode26
Line 26: GtkFloatingContainerChild* GetChild(GtkFloatingContainer* container,
Docs on all new functions.

http://codereview.chromium.org/115835/diff/18/1010#newcode29
Line 29: while (children) {
for (children = ...; children; children = children->next)

might be easier to read.

http://library.gnome.org/devel/glib/unstable/glib-Doubly-Linked-Lists.html#g-...
g_list_next(), your call as to whether to use it

http://codereview.chromium.org/115835/diff/18/1010#newcode142
Line 142: // since that's what is needed by toplevels, which derive from
GtkBin."
It seems here you're doing the opposite of what this comment wants.  Could you
describe why?

http://codereview.chromium.org/115835/diff/18/1010#newcode149
Line 149: while (children) {
Could use GetChild() here, no?

http://codereview.chromium.org/115835/diff/18/1010#newcode210
Line 210: if (!GTK_WIDGET_NO_WINDOW(widget) && GTK_WIDGET_REALIZED(widget)) {
Aren't you guaranteed windowless by your constructor?

http://codereview.chromium.org/115835/diff/18/1010#newcode221
Line 221: gtk_widget_size_allocate(bin->child, allocation);
Can you just call the parent class's size_allocate?

http://codereview.chromium.org/115835/diff/18/1010#newcode313
Line 313: gtk_widget_set_parent(widget, GTK_WIDGET(container));
http://library.gnome.org/devel/gtk/unstable/GtkWidget.html#gtk-widget-set-parent
indicates this may set the "location" of the child.  not sure if you need to do
anythign special here (perhaps add_floating needs to take an (x,y)?) or if
you're safe because nothing has been shown yet.

http://codereview.chromium.org/115835/diff/18/1010#newcode316
Line 316: g_list_append(container->floating_children, child_info);
I guess prepend is faster, not that it matters.

http://codereview.chromium.org/115835/diff/18/1011
File chrome/browser/gtk/gtk_floating_container.h (right):

http://codereview.chromium.org/115835/diff/18/1011#newcode34
Line 34: GtkBin bin;
Docs on all members.  For "bin" it's sufficient to just say something like
"parent class".

http://codereview.chromium.org/115835/diff/18/1011#newcode36
Line 36: GList* floating_children;
E.g., what is this a list of?  Do we own the pointers?

http://codereview.chromium.org/115835/diff/18/1011#newcode53
Line 53: // Use gtk_container_remove to remove all widgets; both the bin and the
shape.
What does "shape" mean here?

http://codereview.chromium.org/115835/diff/18/1011#newcode57
Line 57: // signature:
Should describe what the signal handler should *do*, too.  :)

[Elliot Glaysher, 2009-05-28 19:06:45]

Take another look, Evan?

http://codereview.chromium.org/115835/diff/18/1010
File chrome/browser/gtk/gtk_floating_container.cc (right):

http://codereview.chromium.org/115835/diff/18/1010#newcode21
Line 21: CHILD_PROP_0,
On 2009/05/28 18:04:46, Evan Martin wrote:
> For what it's worth, all this property stuff is not really necessary and only
> used for language bindings.  Your call on whether to leave it or not.

Are you sure? The prototype for *_set_child_property only takes the
corresponding gint value and the gtk_container_class_install_child_property()
seems to bind a numeric id to its string name used in the external interface...

http://codereview.chromium.org/115835/diff/18/1010#newcode149
Line 149: while (children) {
On 2009/05/28 18:04:46, Evan Martin wrote:
> Could use GetChild() here, no?

Can't, because of g_list_remove_link. It'd be cleaner if I could...

http://codereview.chromium.org/115835/diff/18/1010#newcode210
Line 210: if (!GTK_WIDGET_NO_WINDOW(widget) && GTK_WIDGET_REALIZED(widget)) {
On 2009/05/28 18:04:46, Evan Martin wrote:
> Aren't you guaranteed windowless by your constructor?

Anyone with a pointer to |widget| can modify that, though.
gtk_fixed_size_allocate() does this same check.

http://codereview.chromium.org/115835/diff/18/1010#newcode221
Line 221: gtk_widget_size_allocate(bin->child, allocation);
On 2009/05/28 18:04:46, Evan Martin wrote:
> Can you just call the parent class's size_allocate?

I tried that, but it doesn't work. I assume it's because the implementation is
actually GtkWidget and it's doing that weird forall trick...

http://codereview.chromium.org/115835/diff/18/1010#newcode313
Line 313: gtk_widget_set_parent(widget, GTK_WIDGET(container));
On 2009/05/28 18:04:46, Evan Martin wrote:
>
http://library.gnome.org/devel/gtk/unstable/GtkWidget.html#gtk-widget-set-parent
> indicates this may set the "location" of the child.  not sure if you need to
do
> anythign special here (perhaps add_floating needs to take an (x,y)?) or if
> you're safe because nothing has been shown yet.

I'd prefer to not take an (x,y) coordinate because the whole idea is that the
callback sets the location. I don't think this is a problem since won't this
trigger a size-allocate in container?

[Evan Martin, 2009-05-28 19:08:48]

LGTM  (does it fix the flickering status bubble problem?

[Elliot Glaysher, 2009-05-28 19:12:30]

On 2009/05/28 19:08:48, Evan Martin wrote:
> LGTM  (does it fix the flickering status bubble problem?

Yes.