[GTK] Report isMinimized and correctly restore app windows.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "chrome/browser/ui/gtk/gtk_util.h"
 #include "chrome/browser/ui/gtk/gtk_window_util.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
 #include "ui/base/x/active_window_watcher_x.h"
 #include "ui/gfx/gtk_util.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/rect.h"
 
 using apps::ShellWindow;
 
 namespace {
 
 // The timeout in milliseconds before we'll get the true window position with
 // 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,
                                        const ShellWindow::CreateParams& params)
     : shell_window_(shell_window),
       window_(NULL),
       state_(GDK_WINDOW_STATE_WITHDRAWN),
       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 =
       web_contents()->GetView()->GetNativeView();
   gtk_container_add(GTK_CONTAINER(window_), native_view);
 
   if (params.bounds.x() != INT_MIN && params.bounds.y() != INT_MIN)
     gtk_window_move(window_, params.bounds.x(), params.bounds.y());
 
   // This is done to avoid a WM "feature" where setting the window size to
@@ skipping 68 matching lines @@
   g_signal_connect(window_, "configure-event",
                    G_CALLBACK(OnConfigureThunk), this);
   g_signal_connect(window_, "window-state-event",
                    G_CALLBACK(OnWindowStateThunk), this);
   if (frameless_) {
     g_signal_connect(window_, "button-press-event",
                      G_CALLBACK(OnButtonPressThunk), this);
     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(
       shell_window_->profile(),
       window_,
       extensions::ExtensionKeybindingRegistry::PLATFORM_APPS_ONLY,
       shell_window_));
 
   ui::ActiveWindowWatcherX::AddObserver(this);
 }
 
 NativeAppWindowGtk::~NativeAppWindowGtk() {
   ui::ActiveWindowWatcherX::RemoveObserver(this);
+  gdk_window_remove_filter(NULL,
+                           &NativeAppWindowGtk::OnXEventThunk,
+                           this);
 }
 
 bool NativeAppWindowGtk::IsActive() const {
   if (ui::ActiveWindowWatcherX::WMSupportsActivation())
     return is_active_;
 
   // This still works even though we don't get the activation notification.
   return gtk_window_is_active(window_);
 }
 
@@ skipping 64 matching lines @@
 
 void NativeAppWindowGtk::Activate() {
   gtk_window_present(window_);
 }
 
 void NativeAppWindowGtk::Deactivate() {
   gdk_window_lower(gtk_widget_get_window(GTK_WIDGET(window_)));
 }
 
 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_);
 }
 
 void NativeAppWindowGtk::Minimize() {
   gtk_window_iconify(window_);
 }
 
 void NativeAppWindowGtk::Restore() {
   if (IsMaximized())
     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) {
   gfx::Rect content_bounds = bounds;
   content_bounds.Inset(GetFrameInsets());
   gtk_window_move(window_, content_bounds.x(), content_bounds.y());
   if (!resizable_) {
     if (frameless_ &&
         gtk_window_util::BoundsMatchMonitorSize(window_, content_bounds)) {
       content_bounds.set_height(content_bounds.height() - 1);
     }
     // TODO(jeremya): set_size_request doesn't honor min/max size, so the
     // bounds should be constrained manually.
     gtk_widget_set_size_request(GTK_WIDGET(window_),
         content_bounds.width(), content_bounds.height());
   } else {
     gtk_window_util::SetWindowSize(window_,
         gfx::Size(bounds.width(), bounds.height()));
   }
 }
 
+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);
 }
 
 bool NativeAppWindowGtk::IsAlwaysOnTop() const {
   return false;
 }
 
 void NativeAppWindowGtk::RenderViewHostChanged() {
   web_contents()->GetView()->Focus();
@@ skipping 277 matching lines @@
 
   draggable_region_.reset(ShellWindow::RawDraggableRegionsToSkRegion(regions));
 }
 
 // static
 NativeAppWindow* NativeAppWindow::Create(
     ShellWindow* shell_window,
     const ShellWindow::CreateParams& params) {
   return new NativeAppWindowGtk(shell_window, params);
 }