Draggable region support for frameless app window on GTK.

Draggable region support for frameless app window on GTK.

BUG=134169
TEST=manually


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

[Junmin Zhu, 2012-08-07 14:48:14]

It depends on jianli's CL(http://codereview.chromium.org/10822004), 
Please kindly help review the patch, thanks.

[Evan Stade, 2012-08-07 23:20:58]

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:72:
g_signal_connect(window_, "button-press-event",
you only need to this if frameless.

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:220: GdkEventButton *
event) {
no space before *

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:225: event->x_root,
event->y_root, event->time);
I think you actually want to return TRUE to indicate you've handled the press

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.h (right):

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
chrome/browser/ui/gtk/extensions/shell_window_gtk.h:94: gfx::ScopedSkRegion
draggable_region_;
I don't even know why this type exists. Why not just used scoped_ptr<SkRegion>?
Better yet use SkRegion.

[jeremya, 2012-08-08 04:38:37]

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:227: }
Is this sufficient? Do we get behaviour like maximising when double-clicking on
the draggable region, and a menu on right-click?

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.h (right):

http://codereview.chromium.org/10834205/diff/1/chrome/browser/ui/gtk/extensio...
chrome/browser/ui/gtk/extensions/shell_window_gtk.h:65: const
std::vector<content::DraggableRegion>& regions) OVERRIDE;
Technically this isn't content::WebContentsDelegate implementation :)

[Junmin Zhu, 2012-08-08 14:28:02]

Hi estade and jeremya, thanks for your review. 

jeremya, as I known, gtk and Xwindow don't provide API to trigger system menu
for right-click on title bar. So I prefer to ignore this feature. If I am wrong,
please kindly correct me.

Please kindly review it, Thanks.

[Evan Stade, 2012-08-08 21:01:42]

just nits

http://codereview.chromium.org/10834205/diff/6001/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/10834205/diff/6001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:72: if (frameless_)
curlies

http://codereview.chromium.org/10834205/diff/6001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:218: if (frameless_ &&
draggable_region_.get() &&
you don't need the frameless_ check now.

http://codereview.chromium.org/10834205/diff/6001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:225: bool is_maximized_ = 
gdk_window_get_state(GTK_WIDGET(widget)->window) &
there are too many spaces after the =

http://codereview.chromium.org/10834205/diff/6001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:227: !is_maximized_ ?
gtk_window_maximize(GTK_WINDOW(widget)) :
nit: I would get rid of the ! here because it just makes it slightly harder to
read.

http://codereview.chromium.org/10834205/diff/6001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:261: // Exclude those
desinated as non-draggable.
designated

http://codereview.chromium.org/10834205/diff/6001/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.h (right):

http://codereview.chromium.org/10834205/diff/6001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.h:94: scoped_ptr<SkRegion>
draggable_region_;
why isn't this just an SkRegion? (empty by default)

[jeremya, 2012-08-09 00:22:44]

lgtm (though i'm not an OWNER). You should consider maximizing the window on
double-click in the draggable area, though :)

[Evan Stade, 2012-08-09 02:36:19]

On 2012/08/09 00:22:44, jeremya wrote:
> lgtm (though i'm not an OWNER). You should consider maximizing the window on
> double-click in the draggable area, though :)

yea, you might look at the code in BrowserWindowGtk: 

http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/gtk/brow...

it handles middle click as well (for lower window). And you can see that the
left click handling for drag is a bit more complicated.

[Junmin Zhu, 2012-08-09 09:21:05]

On 2012/08/09 02:36:19, Evan Stade wrote:
> On 2012/08/09 00:22:44, jeremya wrote:
> > lgtm (though i'm not an OWNER). You should consider maximizing the window on
> > double-click in the draggable area, though :)
> 
> yea, you might look at the code in BrowserWindowGtk: 
> 
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/gtk/brow...
> 
> it handles middle click as well (for lower window). And you can see that the
> left click handling for drag is a bit more complicated.

Thanks estade, that's great example.

I add middle click support. For left click handling, I don't find the issue
described (start a gtk_window_begin_move_drag on the second GDK_BUTTON_PRESS
would cause the call to gtk_window_maximize fails) on my Ubuntu12.04, but not
sure on other window manager, so I add this part code too. 

Please kindly review it, thanks.

[Evan Stade, 2012-08-10 02:50:26]

that code is pretty intricate, can we find a way to share it?

[Junmin Zhu, 2012-08-15 02:58:34]

On 2012/08/10 02:50:26, Evan Stade wrote:
> that code is pretty intricate, can we find a way to share it?

On 2012/08/10 02:50:26, Evan Stade wrote:
> that code is pretty intricate, can we find a way to share it?

Have re-factored the patch, would you help to review it again?
Thanks.

[Evan Stade, 2012-08-15 21:20:35]

http://codereview.chromium.org/10834205/diff/13001/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/10834205/diff/13001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:234: last_click_position_ =
gfx::Point(static_cast<int>(event->x),
you could share more code if you passed &last_click_time_ and
&last_click_position_ into the function, and let the function do the updating.

http://codereview.chromium.org/10834205/diff/13001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:242: } else if (event->type
== GDK_2BUTTON_PRESS) {
nit: it's weird that the if and the else if cases have the order of the operands
switched around.

http://codereview.chromium.org/10834205/diff/13001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:243: bool is_maximized_ =
gdk_window_get_state(GTK_WIDGET(widget)->window) &
first, local variables should not have final underscores

second, this doesn't really seem like a great use of the ternary operator
because the code inside the two conditions does not overlap at all.

[Junmin Zhu, 2012-08-16 05:32:37]

1. refactor HandleTitleBarLeftMousePress function in browser_window_gtk, which
would need to adjust panel_browser_window_gtk.cc too.

2. move UnMaximize to gtk_window_util.h too.

Add jennb for panel review(one of panel's owner.)

[Junmin Zhu, 2012-08-16 05:33:08]

Please kindly review it, thanks.

[jennb, 2012-08-16 18:05:11]

lgtm for Panels changes.

[Evan Stade, 2012-08-16 18:48:07]

http://codereview.chromium.org/10834205/diff/10009/chrome/browser/ui/gtk/gtk_...
File chrome/browser/ui/gtk/gtk_window_util.cc (right):

http://codereview.chromium.org/10834205/diff/10009/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_window_util.cc:18: static GdkEventButton
last_click_event;
non non-POD statics.

http://codereview.chromium.org/10834205/diff/10009/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_window_util.cc:114: gfx::Rect bounds,
const ref

http://codereview.chromium.org/10834205/diff/10009/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_window_util.cc:159: gfx::Rect bounds,
const ref x2

[Evan Stade, 2012-08-16 19:04:21]

http://codereview.chromium.org/10834205/diff/10009/chrome/browser/ui/gtk/gtk_...
File chrome/browser/ui/gtk/gtk_window_util.cc (right):

http://codereview.chromium.org/10834205/diff/10009/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_window_util.cc:18: static GdkEventButton
last_click_event;
On 2012/08/16 18:48:07, Evan Stade wrote:
> non non-POD statics.

this was supposed to say "no non-POD statics".

[Junmin Zhu, 2012-08-17 02:45:59]

Update the patch.
Please kindly review it, thanks.

[Junmin Zhu, 2012-08-18 10:05:37]

On 2012/08/17 02:45:59, Junmin Zhu wrote:
> Update the patch.
> Please kindly review it, thanks.

Rebase patch to trunk.
Please kindly review it, thanks.

[Evan Stade, 2012-08-20 19:44:00]

lgtm

[Evan Stade, 2012-08-20 20:18:20]

http://codereview.chromium.org/10834205/diff/13014/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.h (right):

http://codereview.chromium.org/10834205/diff/13014/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/shell_window_gtk.h:16: #include
"third_party/skia/include/core/SkRegion.h"
alphabetize

[commit-bot: I haz the power, 2012-08-21 04:45:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junmin.zhu@intel.com/10834205/14001

[commit-bot: I haz the power, 2012-08-21 07:04:46]

Change committed as 152524