[GTK] Report isMinimized and correctly restore app windows.

chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc

Index: chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc
diff --git a/chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc b/chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc
index e96e322bcb66b627003f07e7302ff2aa96be3260..1b725c7c804907ba8b3bf532df47498302b60441 100644
--- a/chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc
+++ b/chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/ui/gtk/extensions/native_app_window_gtk.h"
 
+#include <gdk/gdkx.h>
+#include <vector>
+
+#include "base/message_loop/message_pump_gtk.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.h"
@@ -28,6 +32,12 @@ namespace {
 // gtk_window_get_position() after the last GTK configure-event signal.
 const int kDebounceTimeoutMilliseconds = 100;
 
+const char* kAtomsToCache[] = {
+  "_NET_WM_STATE",
+  "_NET_WM_STATE_HIDDEN",
+  NULL
+};
+
 } // namespace
 
 NativeAppWindowGtk::NativeAppWindowGtk(ShellWindow* shell_window,
@@ -38,7 +48,8 @@ NativeAppWindowGtk::NativeAppWindowGtk(ShellWindow* shell_window,
       is_active_(false),
       content_thinks_its_fullscreen_(false),
       frameless_(params.frame == ShellWindow::FRAME_NONE),
-      frame_cursor_(NULL) {
+      frame_cursor_(NULL),
+      atom_cache_(base::MessagePumpGtk::GetDefaultXDisplay(), kAtomsToCache) {
   window_ = GTK_WINDOW(gtk_window_new(GTK_WINDOW_TOPLEVEL));
 
   gfx::NativeView native_view =
@@ -127,6 +138,10 @@ NativeAppWindowGtk::NativeAppWindowGtk(ShellWindow* shell_window,
     g_signal_connect(window_, "motion-notify-event",
                      G_CALLBACK(OnMouseMoveEventThunk), this);
   }
+  GdkWindow* window = gtk_widget_get_window(GTK_WIDGET(window_));
+  gdk_window_add_filter(window,
+                        &NativeAppWindowGtk::OnXEventThunk,
+                        this);
 
   // Add the keybinding registry.
   extension_keybinding_registry_.reset(new ExtensionKeybindingRegistryGtk(
@@ -140,6 +155,9 @@ NativeAppWindowGtk::NativeAppWindowGtk(ShellWindow* shell_window,
 
 NativeAppWindowGtk::~NativeAppWindowGtk() {
   ui::ActiveWindowWatcherX::RemoveObserver(this);
+  gdk_window_remove_filter(NULL,
+                           &NativeAppWindowGtk::OnXEventThunk,
+                           this);
 }
 
 bool NativeAppWindowGtk::IsActive() const {
@@ -224,6 +242,7 @@ void NativeAppWindowGtk::Deactivate() {
 }
 
 void NativeAppWindowGtk::Maximize() {
+  gtk_window_present(window_);

[scheib, 2013/07/16 22:11:21]

I'm not sure why this is needed here?

[zhchbin, 2013/07/17 02:39:32]

After minimize the app window, calling maximize should show the window. Without
gtk_window_present, the window will still hide. I add these code to make sure
the behavior is the same on Windows platform. If I'm wrong, please let me know
and I will remove these.

[scheib, 2013/07/17 04:47:39]

Ah! I agree that we should move the different systems to be coherent with each
other, and see that Windows does this, and so agree with this change. Add a
comment to explain it.

[zhchbin, 2013/07/17 09:05:36]

Done.

   gtk_window_maximize(window_);
 }
 
@@ -236,6 +255,8 @@ void NativeAppWindowGtk::Restore() {
     gtk_window_unmaximize(window_);
   else if (IsMinimized())
     gtk_window_deiconify(window_);
+
+  gtk_window_present(window_);

[scheib, 2013/07/16 22:11:21]

We should only present if the window isn't hidden by us, and only if it is
minimized and we are relying on present to do the job deiconify is failing to
do.

[zhchbin, 2013/07/17 02:39:32]

However, I find these behavior are different on the windows and linux platform.
So I add it to keep consistency. For example, on the windows platform, calling
restore after hide will show the window, but on the linux, the window will still
hide. Is this by design?

[scheib, 2013/07/17 04:47:39]

Hmm, on win7 with window state sample (from github and on web store) if I hide
and then restore the window comes back all black -- another bug.
(http://crbug.com/261013). If you are able to have windows hide and then restore
shows the window then keep the code this way on linux as well but add a comment
explaining it (consistent with windows).

[zhchbin, 2013/07/17 09:05:36]

Done.

 }
 
 void NativeAppWindowGtk::SetBounds(const gfx::Rect& bounds) {
@@ -257,6 +278,33 @@ void NativeAppWindowGtk::SetBounds(const gfx::Rect& bounds) {
   }
 }
 
+GdkFilterReturn NativeAppWindowGtk::OnXEvent(GdkXEvent* xevent,

[scheib, 2013/07/16 22:11:21]

Please add a comment explaining why this method is here and what it is doing.
e.g. ... a work around for gdk not reporting minimize state changes, listening
for property notify events for state and checking to see if the state includes
hidden, and from that setting or clearing the iconified bit. Probably reference
https://bugs.launchpad.net/unity/+bug/998073

[zhchbin, 2013/07/17 02:39:32]

Done.

[scheib, 2013/07/17 04:47:39]

Thank you, though the comment needs a bit more clean up:

// Work around gtk+ not reporting minimize state changes, by listening for
// property notify events for state and checking to see if the state includes
// hidden. Then from that setting or clearing the iconified bit.
// http://crbug.com/162794.

[zhchbin, 2013/07/17 09:05:36]

Done.

+                                              GdkEvent* event) {
+  XEvent* xev = static_cast<XEvent*>(xevent);

[scheib, 2013/07/16 22:11:21]

name it xevent, and name the method parameter gdkxevent

[zhchbin, 2013/07/17 02:39:32]

Done.

+  ::Atom state = atom_cache_.GetAtom("_NET_WM_STATE");

[scheib, 2013/07/16 22:11:21]

variable only used once, right? Consider calling GetAtom in the location it's
needed to reduce lines of code and indirection required to read it. Same for
state_hidden.

[zhchbin, 2013/07/17 02:39:32]

Done.

+  std::vector< ::Atom> atom_list;
+
+  if (xev->type == PropertyNotify &&
+      xev->xproperty.atom == state &&
+      ui::GetAtomArrayProperty(GDK_WINDOW_XWINDOW(GTK_WIDGET(window_)->window),
+                               "_NET_WM_STATE",
+                               &atom_list)) {
+    ::Atom state_hidden = atom_cache_.GetAtom("_NET_WM_STATE_HIDDEN");
+    std::vector< ::Atom>::iterator it = std::find(atom_list.begin(),
+                                                  atom_list.end(),
+                                                  state_hidden);

[scheib, 2013/07/16 22:11:21]

One thing I don't understand is why the hidden atom is not set when a window is
hidden with gtk_widget_hide. I do not want to report isMinimized when we have
hidden a window.

[zhchbin, 2013/07/17 02:39:32]

Glance through the source code of "gtk_widget_hide", I didn't find anything that
it sets _NET_WM_STATE_HIDDEN. And on the implementation of
DesktopRootWindowHostX11::IsMinimized, it also depend on "_NET_WM_STATE_HIDDEN"
to check. Code:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...

+    if (it != atom_list.end())
+      state_ = GDK_WINDOW_STATE_ICONIFIED;
+    else
+      state_ =
+          static_cast<GdkWindowState>(state_ & ~GDK_WINDOW_STATE_ICONIFIED);
+
+    shell_window_->OnNativeWindowChanged();
+  }
+
+  return GDK_FILTER_CONTINUE;
+}
+
 void NativeAppWindowGtk::FlashFrame(bool flash) {
   gtk_window_set_urgency_hint(window_, flash);
 }