Panels refactor: Support browserless panels on Linux.

Panels refactor: Support browserless panels on Linux.

BUG=127323
TEST=Enabled tests for refactored panels on Linux


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

[jennb, 2012-08-09 00:08:40]

jian for panel refactor
estade - could you review the browser_window_gtk.cc code that I shifted into
gtk_util so that it can be shared?

Thanks,
Jenn

[jianli, 2012-08-09 00:52:50]

Still reviewing. Some initial comments.

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/gtk/gtk_util.h
File chrome/browser/ui/gtk/gtk_util.h (right):

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/gtk/gtk_util...
chrome/browser/ui/gtk/gtk_util.h:325: GdkCursorType
GdkWindowEdgeToGdkCursorType(GdkWindowEdge edge);
nit: add comment. I have no problem to understand it from the method name, but
other people might not know this is used for resizing purpose.

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

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.h:47: friend class
GtkNativePanelTesting;
Might be better to rename NativePanelTestingGtk to OldNativePanelTestingGtk and
use that name for the new panel testing so that the naming is consistent across
platforms.

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

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.h:51: // Overridden from NativePanel
nit: end with ":".

[jianli, 2012-08-10 01:07:57]

lgtm

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

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.cc:204: render_area_event_box_(NULL) {
contents_expanded_ and accel_group_ not initialized

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.cc:1032: contents_container_allocation.width,
nit: more indenting for better readability.

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

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.h:31: class AcceleratorGtk;
Not used.

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.h:115: bool GetWindowEdge(int x, int y,
GdkWindowEdge* edge);
nit: const modifier at the end.

[Evan Stade, 2012-08-10 01:52:30]

gtk lgtm

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/gtk/gtk_util.cc
File chrome/browser/ui/gtk/gtk_util.cc (right):

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/gtk/gtk_util...
chrome/browser/ui/gtk/gtk_util.cc:1038: // Ubuntu patches their verrsion of GTK+
so that there is always a
sp: verrsion

[Evan Stade, 2012-08-10 02:54:25]

actually, could you create a new util file which is just for gtk browser window
utilities? both you and Junmin Zhu (cc'd) are currently factoring out browser
window code to share among other types of windows.

[jennb, 2012-08-10 03:43:58]

On 2012/08/10 02:54:25, Evan Stade wrote:
> actually, could you create a new util file which is just for gtk browser
window
> utilities? both you and Junmin Zhu (cc'd) are currently factoring out browser
> window code to share among other types of windows.

Will do.

[jennb, 2012-08-10 18:45:42]

Created gtk_window_util file. Was conservative in only moving utils needed by
Panels. Junmin can add to it as part of his refactor.

Also made FocusLostOnMinimize more robust by setting up signal before action
that would cause the expected change.

PTAL.

Thanks.

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/gtk/gtk_util.cc
File chrome/browser/ui/gtk/gtk_util.cc (right):

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/gtk/gtk_util...
chrome/browser/ui/gtk/gtk_util.cc:1038: // Ubuntu patches their verrsion of GTK+
so that there is always a
On 2012/08/10 01:52:30, Evan Stade wrote:
> sp: verrsion

Done.

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/gtk/gtk_util.h
File chrome/browser/ui/gtk/gtk_util.h (right):

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/gtk/gtk_util...
chrome/browser/ui/gtk/gtk_util.h:325: GdkCursorType
GdkWindowEdgeToGdkCursorType(GdkWindowEdge edge);
On 2012/08/09 00:52:51, jianli wrote:
> nit: add comment. I have no problem to understand it from the method name, but
> other people might not know this is used for resizing purpose.

Done.

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

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_gtk.h:47: friend class
GtkNativePanelTesting;
On 2012/08/09 00:52:51, jianli wrote:
> Might be better to rename NativePanelTestingGtk to OldNativePanelTestingGtk
and
> use that name for the new panel testing so that the naming is consistent
across
> platforms.

Changing the name of the new class means not having to change all the old test
files. This is consistent with what I did for Cocoa.

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

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.cc:204: render_area_event_box_(NULL) {
On 2012/08/10 01:07:57, jianli wrote:
> contents_expanded_ and accel_group_ not initialized

Done.

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.cc:1032: contents_container_allocation.width,
On 2012/08/10 01:07:57, jianli wrote:
> nit: more indenting for better readability.

Done.

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

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.h:31: class AcceleratorGtk;
On 2012/08/10 01:07:57, jianli wrote:
> Not used.

Done.

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.h:51: // Overridden from NativePanel
On 2012/08/09 00:52:51, jianli wrote:
> nit: end with ":".

Added period to be consistent with rest of file.

http://codereview.chromium.org/10831226/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_gtk.h:115: bool GetWindowEdge(int x, int y,
GdkWindowEdge* edge);
On 2012/08/10 01:07:57, jianli wrote:
> nit: const modifier at the end.

Done.

[Evan Stade, 2012-08-10 21:16:59]

lgtm

http://codereview.chromium.org/10831226/diff/8002/chrome/browser/ui/gtk/gtk_w...
File chrome/browser/ui/gtk/gtk_window_util.h (right):

http://codereview.chromium.org/10831226/diff/8002/chrome/browser/ui/gtk/gtk_w...
chrome/browser/ui/gtk/gtk_window_util.h:14: namespace gtk_util {
consider giving this namespace a unique name.

[jennb, 2012-08-11 05:07:20]

http://codereview.chromium.org/10831226/diff/8002/chrome/browser/ui/gtk/gtk_w...
File chrome/browser/ui/gtk/gtk_window_util.h (right):

http://codereview.chromium.org/10831226/diff/8002/chrome/browser/ui/gtk/gtk_w...
chrome/browser/ui/gtk/gtk_window_util.h:14: namespace gtk_util {
On 2012/08/10 21:16:59, Evan Stade wrote:
> consider giving this namespace a unique name.

Done.

[commit-bot: I haz the power, 2012-08-12 03:07:52]

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

[commit-bot: I haz the power, 2012-08-12 03:07:59]

Presubmit check for 10831226-5004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[jam, 2012-08-13 16:52:14]

gyp file lgtm

[commit-bot: I haz the power, 2012-08-13 16:53:03]

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

[commit-bot: I haz the power, 2012-08-13 20:06:59]

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

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

Change committed as 151353

[commit-bot: I haz the power, 2012-08-13 23:00:49]

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

[commit-bot: I haz the power, 2012-08-14 05:47:15]

Try job failure for 10831226-29 (retry) (retry) on linux_chromeos for steps
"browser_tests, content_browsertests".
It's a second try, previously, steps "browser_tests, content_browsertests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-14 16:41:09]

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

[commit-bot: I haz the power, 2012-08-14 18:54:51]

Change committed as 151526