Resize support for detached GTK Panels.

Resize support for detached GTK Panels.

Will consolidate GTK panel dragging to use the new
logic in a separate patch.

BUG=None
TEST=Detach panels and drag mouse on the edge. Verify panel resizes.


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

[jennb, 2012-04-11 21:59:33]

Evan - could you review my gtk/gdk usage, such as the invisible event box and
pointer/keyboard grabs?

Dimich - please review panel stuff.

Essentially, I am creating my own drag widget to process mouse+keyboard events
while the mouse is pressed instead of doing a system resize-drag due to panel
bounds being controlled by the panel manager. The move-drag logic will be
changed to use PanelDragGtk in a separate patch.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_gtk.cc (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:158: DCHECK(gdk_window != NULL);
Evan - any idea why this DCHECK fails if I create my invisible event box and add
it to window_vbox_ in PanelBrowserWindowGtk::Init()? It only works if I create
the event box at the time of the first mouse press on the titlebar/window-edge.
I don't understand why.

[dcheng, 2012-04-12 00:12:45]

Drive-by.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_gtk.cc (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:16: panel::ResizingSides
GdkWindowEdgeToResizingSide(GdkWindowEdge& edge) {
GdkWindowEdge instead of GdkWindowEdge& ?

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:53: Panel* panel_;
Nit: DISABLE_COPY_AND_ASSIGN for these 3 classes.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:62: void DragStarted(gfx::Point
point) {
Do you need to use OVERRIDE here?

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:76: ResizeDragDelegate(Panel* panel,
GdkWindowEdge& edge)
Ditto for mutable reference.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:80: void DragStarted(gfx::Point
point) {
Ditto for OVERRIDE.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:125: bool
PanelDragGtk::AssertCleanState() {
I think this can just return void since you always return true.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_gtk.h (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.h:23: class PanelDragGtk {
Given that this is named drag_helper_ in PanelBrowserWindowGtk, I think it's
appropriate to call this PanelDragHelper. But I don't have a strong opinion
about this.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.h:94: PanelDragDelegate* drag_delegate_;
Nit: DISABLE_COPY_AND_ASSIGN

[Dmitry Titov, 2012-04-12 00:24:44]

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_gtk.cc (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:57: class MoveDragDelegate : public
PanelDragDelegate {
The same name "Drag" is used in 2 different contexts here. I've chatted about
this with Andrei, it seems one idea could be to call the mouse action "Drag",
which can cause PanelMove or PanelResize - essentially, renaming "PanelDrag"
into "PanelResize" and reserving the "Drag" for the mouse operation of moving
the mouse with a button pressed.
This will also cause the current panel_drag_controller to become
panel_move_controller, and this new class then can be
panel_mouse_drag_controller, which would better reflect what it is.
It might be a bigger change then you want to do in this patch, but it can be a
nice cleanup afterwards.

[Evan Stade, 2012-04-12 03:25:28]

without actually looking at BrowserWindowGtk, it seems like we could be sharing
some code with BrowserWindowGtk

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:148:
gdk_window_get_cursor(gtk_widget_get_window(GTK_WIDGET(window_)));
2 more indent

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_gtk.cc (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:45: virtual ~PanelDragDelegate() {}
add some vertical space

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:48: virtual void
DragStarted(gfx::Point point) = 0;
docs for the Drag* functions (e.g. what coordinate system is point using?)

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:158: DCHECK(gdk_window != NULL);
On 2012/04/11 21:59:33, jennb wrote:
> Evan - any idea why this DCHECK fails if I create my invisible event box and
add
> it to window_vbox_ in PanelBrowserWindowGtk::Init()? It only works if I create
> the event box at the time of the first mouse press on the
titlebar/window-edge.
> I don't understand why.

I may not be understanding the question exactly but it sounds like you're asking
for the window before the widget is realized (see docs for gtk_widget_realize) 

p.s. I think DCHECK(pointer) is typical

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:160: gdk_pointer_grab(gdk_window,
2 more spaces indent

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:169: gdk_keyboard_grab(gdk_window,
TRUE, event->time);
2 more spaces indent

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:176: << keyboard_grab_status<< ")";
space before <<

[jennb, 2012-04-12 18:00:24]

BrowserWindowGtk uses gtk_window_begin_move_drag() and
gtk_window_begin_resize_drag() to do all this. Panels cannot use those system
drag methods because panel size and bounds are controlled by the panel manager,
thus this isn't code that needs to be shared with BrowserWindowGtk.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:148:
gdk_window_get_cursor(gtk_widget_get_window(GTK_WIDGET(window_)));
On 2012/04/12 03:25:29, Evan Stade wrote:
> 2 more indent

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_gtk.cc (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:16: panel::ResizingSides
GdkWindowEdgeToResizingSide(GdkWindowEdge& edge) {
On 2012/04/12 00:12:45, dcheng wrote:
> GdkWindowEdge instead of GdkWindowEdge& ?

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:45: virtual ~PanelDragDelegate() {}
On 2012/04/12 03:25:29, Evan Stade wrote:
> add some vertical space

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:48: virtual void
DragStarted(gfx::Point point) = 0;
On 2012/04/12 03:25:29, Evan Stade wrote:
> docs for the Drag* functions (e.g. what coordinate system is point using?)

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:53: Panel* panel_;
On 2012/04/12 00:12:45, dcheng wrote:
> Nit: DISABLE_COPY_AND_ASSIGN for these 3 classes.

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:57: class MoveDragDelegate : public
PanelDragDelegate {
On 2012/04/12 00:24:44, Dmitry Titov wrote:
> The same name "Drag" is used in 2 different contexts here. I've chatted about
> this with Andrei, it seems one idea could be to call the mouse action "Drag",
> which can cause PanelMove or PanelResize - essentially, renaming "PanelDrag"
> into "PanelResize" and reserving the "Drag" for the mouse operation of moving
> the mouse with a button pressed.
> This will also cause the current panel_drag_controller to become
> panel_move_controller, and this new class then can be
> panel_mouse_drag_controller, which would better reflect what it is.
> It might be a bigger change then you want to do in this patch, but it can be a
> nice cleanup afterwards.

In GTK, move-drag and resize-drag is used to distinguish the two actions. "Drag"
just means moving the mouse with button pressed. I followed that pattern in this
patch.

In a separate patch, I will rename panel_drag_controller to
panel_move_drag_controller and panel_resize_controller to
panel_resize_drag_controller and replace "ResizeByMouse" with "ResizeDrag".

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:62: void DragStarted(gfx::Point
point) {
On 2012/04/12 00:12:45, dcheng wrote:
> Do you need to use OVERRIDE here?

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:76: ResizeDragDelegate(Panel* panel,
GdkWindowEdge& edge)
On 2012/04/12 00:12:45, dcheng wrote:
> Ditto for mutable reference.

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:80: void DragStarted(gfx::Point
point) {
On 2012/04/12 00:12:45, dcheng wrote:
> Ditto for OVERRIDE.

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:125: bool
PanelDragGtk::AssertCleanState() {
On 2012/04/12 00:12:45, dcheng wrote:
> I think this can just return void since you always return true.

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:160: gdk_pointer_grab(gdk_window,
On 2012/04/12 03:25:29, Evan Stade wrote:
> 2 more spaces indent

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:169: gdk_keyboard_grab(gdk_window,
TRUE, event->time);
On 2012/04/12 03:25:29, Evan Stade wrote:
> 2 more spaces indent

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.cc:176: << keyboard_grab_status<< ")";
On 2012/04/12 03:25:29, Evan Stade wrote:
> space before <<

Done.

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_gtk.h (right):

http://codereview.chromium.org/10020057/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.h:94: PanelDragDelegate* drag_delegate_;
On 2012/04/12 00:12:45, dcheng wrote:
> Nit: DISABLE_COPY_AND_ASSIGN

Done.

[Dmitry Titov, 2012-04-12 18:28:13]

LGTM for generic panels.

[Evan Stade, 2012-04-12 23:44:50]

lgtm

[commit-bot: I haz the power, 2012-04-12 23:52:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10020057/6001

[commit-bot: I haz the power, 2012-04-13 02:15:35]

Change committed as 132126