Implement DraggedTabGtk, the object that handles rendering either a dragged t...

Implement DraggedTabGtk, the object that handles rendering either a dragged tab or tab contents during a tab drag.

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

[Evan Martin, 2009-05-18 17:22:16]

I admit I don't understand all of this code too well, so here are some
superficial comments.

http://codereview.chromium.org/113532/diff/19/21
File chrome/browser/gtk/tabs/dragged_tab_controller_gtk.cc (right):

http://codereview.chromium.org/113532/diff/19/21#newcode111
Line 111: // It would be nice to respond to this message by changing the
should this be marked TODO(jhawkins)?

http://codereview.chromium.org/113532/diff/19/24
File chrome/browser/gtk/tabs/dragged_tab_controller_gtk.h (right):

http://codereview.chromium.org/113532/diff/19/24#newcode142
Line 142: // Gets the position of a gtk widget.
Screen coordinates?  Window coordinates?

http://codereview.chromium.org/113532/diff/19/27
File chrome/browser/gtk/tabs/dragged_tab_gtk.cc (right):

http://codereview.chromium.org/113532/diff/19/27#newcode17
Line 17: const int kDragFrameBorderSize = 2;
docs on all of these?

http://codereview.chromium.org/113532/diff/19/27#newcode145
Line 145: colormap = gdk_screen_get_rgb_colormap(screen);
2spc

http://codereview.chromium.org/113532/diff/19/27#newcode162
Line 162: cairo_set_source_rgba(cairo_context, 1.0f, 1.0f, 1.0f, 0.0f);
If it's fully transparent, does it not show at all?

http://codereview.chromium.org/113532/diff/19/23
File chrome/browser/gtk/tabs/tab_gtk.h (right):

http://codereview.chromium.org/113532/diff/19/23#newcode156
Line 156: bool dragging_;
doc?

[James Hawkins, 2009-05-18 17:34:07]

http://codereview.chromium.org/113532/diff/19/21
File chrome/browser/gtk/tabs/dragged_tab_controller_gtk.cc (right):

http://codereview.chromium.org/113532/diff/19/21#newcode111
Line 111: // It would be nice to respond to this message by changing the
On 2009/05/18 17:22:16, Evan Martin wrote:
> should this be marked TODO(jhawkins)?

Done.

http://codereview.chromium.org/113532/diff/19/24
File chrome/browser/gtk/tabs/dragged_tab_controller_gtk.h (right):

http://codereview.chromium.org/113532/diff/19/24#newcode142
Line 142: // Gets the position of a gtk widget.
On 2009/05/18 17:22:16, Evan Martin wrote:
> Screen coordinates?  Window coordinates?

Done.

http://codereview.chromium.org/113532/diff/19/27
File chrome/browser/gtk/tabs/dragged_tab_gtk.cc (right):

http://codereview.chromium.org/113532/diff/19/27#newcode17
Line 17: const int kDragFrameBorderSize = 2;
On 2009/05/18 17:22:16, Evan Martin wrote:
> docs on all of these?

Done.

http://codereview.chromium.org/113532/diff/19/27#newcode145
Line 145: colormap = gdk_screen_get_rgb_colormap(screen);
On 2009/05/18 17:22:16, Evan Martin wrote:
> 2spc

Done.

http://codereview.chromium.org/113532/diff/19/27#newcode162
Line 162: cairo_set_source_rgba(cairo_context, 1.0f, 1.0f, 1.0f, 0.0f);
On 2009/05/18 17:22:16, Evan Martin wrote:
> If it's fully transparent, does it not show at all?

The background of the window itself doesn't show, but the contents (child
widgets) do show.  I'll add another comment.

http://codereview.chromium.org/113532/diff/19/23
File chrome/browser/gtk/tabs/tab_gtk.h (right):

http://codereview.chromium.org/113532/diff/19/23#newcode156
Line 156: bool dragging_;
On 2009/05/18 17:22:16, Evan Martin wrote:
> doc?

Done.

[tony, 2009-05-18 17:59:35]

http://codereview.chromium.org/113532/diff/2001/3002
File chrome/browser/gtk/tabs/dragged_tab_controller_gtk.cc (right):

http://codereview.chromium.org/113532/diff/2001/3002#newcode51
Line 51: if (!source_tab_) return;
Nit: return on the next line.

http://codereview.chromium.org/113532/diff/2001/3002#newcode255
Line 255: double unselected_width, selected_width = 0;
Maybe initialize unselected_width as well?

http://codereview.chromium.org/113532/diff/2001/3005
File chrome/browser/gtk/tabs/dragged_tab_controller_gtk.h (right):

http://codereview.chromium.org/113532/diff/2001/3005#newcode143
Line 143: static void GetWidgetPosition(GtkWidget* widget, int* x, int* y);
This looks like a general function.  Maybe move it to chrome/common/gtk_util.h? 
Also, perhaps it should return a gfx::Point?

http://codereview.chromium.org/113532/diff/2001/3005#newcode146
Line 146: static gfx::Rect GetWidgetScreenBounds(GtkWidget* widget);
Same as above about moving to chrome/common/gtk_util.h.

http://codereview.chromium.org/113532/diff/2001/3009
File chrome/browser/gtk/tabs/dragged_tab_gtk.h (right):

http://codereview.chromium.org/113532/diff/2001/3009#newcode41
Line 41: void AnimateToBounds(const gfx::Rect& bounds, Callback0::Type*
callback);
Nit: Maybe make a typedef for Callback0::Type (e.g., AnimateToBoundsCallback).

http://codereview.chromium.org/113532/diff/2001/3001
File chrome/browser/gtk/tabs/tab_gtk.cc (right):

http://codereview.chromium.org/113532/diff/2001/3001#newcode201
Line 201: gdk_event_handler_set(TabGtk::GdkEventHandler, tab, NULL);
Do we ever need to unset this (e.g., OnDragEnd)?

http://codereview.chromium.org/113532/diff/2001/3004
File chrome/browser/gtk/tabs/tab_gtk.h (right):

http://codereview.chromium.org/113532/diff/2001/3004#newcode90
Line 90: static void GdkEventHandler(GdkEvent* event, void* tab);
Can you replace the void* with TabGtk* like we do for other event handler
callbacks?

[James Hawkins, 2009-05-18 18:46:08]

http://codereview.chromium.org/113532/diff/2001/3002
File chrome/browser/gtk/tabs/dragged_tab_controller_gtk.cc (right):

http://codereview.chromium.org/113532/diff/2001/3002#newcode51
Line 51: if (!source_tab_) return;
On 2009/05/18 17:59:35, tony wrote:
> Nit: return on the next line.

Done.

http://codereview.chromium.org/113532/diff/2001/3002#newcode255
Line 255: double unselected_width, selected_width = 0;
On 2009/05/18 17:59:35, tony wrote:
> Maybe initialize unselected_width as well?

Done.

http://codereview.chromium.org/113532/diff/2001/3005
File chrome/browser/gtk/tabs/dragged_tab_controller_gtk.h (right):

http://codereview.chromium.org/113532/diff/2001/3005#newcode143
Line 143: static void GetWidgetPosition(GtkWidget* widget, int* x, int* y);
On 2009/05/18 17:59:35, tony wrote:
> This looks like a general function.  Maybe move it to
chrome/common/gtk_util.h? 
> Also, perhaps it should return a gfx::Point?

Done.

http://codereview.chromium.org/113532/diff/2001/3005#newcode146
Line 146: static gfx::Rect GetWidgetScreenBounds(GtkWidget* widget);
On 2009/05/18 17:59:35, tony wrote:
> Same as above about moving to chrome/common/gtk_util.h.

Done.

http://codereview.chromium.org/113532/diff/2001/3009
File chrome/browser/gtk/tabs/dragged_tab_gtk.h (right):

http://codereview.chromium.org/113532/diff/2001/3009#newcode41
Line 41: void AnimateToBounds(const gfx::Rect& bounds, Callback0::Type*
callback);
On 2009/05/18 17:59:35, tony wrote:
> Nit: Maybe make a typedef for Callback0::Type (e.g., AnimateToBoundsCallback).

Done.

http://codereview.chromium.org/113532/diff/2001/3001
File chrome/browser/gtk/tabs/tab_gtk.cc (right):

http://codereview.chromium.org/113532/diff/2001/3001#newcode201
Line 201: gdk_event_handler_set(TabGtk::GdkEventHandler, tab, NULL);
On 2009/05/18 17:59:35, tony wrote:
> Do we ever need to unset this (e.g., OnDragEnd)?

No.  As long as it exists, we pass events through to gdk.  There's no way to
unset it either.

http://codereview.chromium.org/113532/diff/2001/3004
File chrome/browser/gtk/tabs/tab_gtk.h (right):

http://codereview.chromium.org/113532/diff/2001/3004#newcode90
Line 90: static void GdkEventHandler(GdkEvent* event, void* tab);
On 2009/05/18 17:59:35, tony wrote:
> Can you replace the void* with TabGtk* like we do for other event handler
> callbacks?

Not with this callback, because we don't have a utility macro like G_CALLBACK
for this type of callback.

[tony, 2009-05-18 18:54:06]

Ok, LGTM.