aura: Try to make Linux host resize code more reliable.

ui/aura/desktop_host_linux.cc

 // Copyright (c) 2011 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 "ui/aura/desktop_host.h"
 
 #include <X11/cursorfont.h>
 #include <X11/Xlib.h>
 
 // Get rid of a macro from Xlib.h that conflicts with Aura's RootWindow class.
@@ skipping 180 matching lines @@
 
   // DesktopHost Overrides.
   virtual void SetDesktop(Desktop* desktop) OVERRIDE;
   virtual gfx::AcceleratedWidget GetAcceleratedWidget() OVERRIDE;
   virtual void Show() OVERRIDE;
   virtual gfx::Size GetSize() const OVERRIDE;
   virtual void SetSize(const gfx::Size& size) OVERRIDE;
   virtual void SetCursor(gfx::NativeCursor cursor_type) OVERRIDE;
   virtual gfx::Point QueryMouseLocation() OVERRIDE;
 
-  // Returns true if there's an X window manager present.
+  // Handle notification from the X server that |xwindow_| has been resized.
+  void HandleConfigureNotify(const gfx::Size& new_size);
+
+  // Returns true if there's an X window manager present... in most cases.  Some
+  // window managers (notably, ion3) don't implement enough of ICCCM for us to
+  // detect that they're there.
   bool IsWindowManagerPresent();
 
   Desktop* desktop_;
 
   // The display and the native X window hosting the desktop.
   Display* xdisplay_;
   ::Window xwindow_;
 
   // Current Aura cursor.
   gfx::NativeCursor current_cursor_;
 
   // The size of |xwindow_|.
-  gfx::Rect bounds_;
+  gfx::Size size_;

[Daniel Erat, 2011/10/21 23:00:37]

using a Rect here didn't make sense; we never need the position, and worse, we
weren't updating it in response to ConfigureNotify events

[sadrul, 2011/10/22 18:53:04]

Nice

 
   DISALLOW_COPY_AND_ASSIGN(DesktopHostLinux);
 };
 
 DesktopHostLinux::DesktopHostLinux(const gfx::Rect& bounds)
     : desktop_(NULL),
       xdisplay_(NULL),
       xwindow_(0),
       current_cursor_(aura::kCursorNull),
-      bounds_(bounds) {
+      size_(bounds.size()) {
   // This assumes that the message-pump creates and owns the display.
   xdisplay_ = base::MessagePumpX::GetDefaultXDisplay();
 
   // Ingore the requested bounds and just cover the whole screen if there's no
   // X window manager present.
-  if (!IsWindowManagerPresent())
-    bounds_.SetRect(
+  gfx::Rect actual_bounds = bounds;
+  if (!IsWindowManagerPresent()) {
+    actual_bounds.SetRect(
         0, 0, DisplayWidth(xdisplay_, 0), DisplayHeight(xdisplay_, 0));

[sadrul, 2011/10/22 18:53:04]

I was thinking about this change ... perhaps we are doing this in the wrong
place. Perhaps we should let the client/shell decide the desktop size for the
environment. That way, chrome can make sure it's fullscreen when it's running in
proper chromeos, and not on a chromeos build on a linux desktop.

[Daniel Erat, 2011/10/23 04:02:55]

Yeah, I wasn't entirely convinced about this part.  Moving the logic to the
shell means that the shell needs to know some platform-dependent things, though:
- whether there's a window manager running or not (this could be controlled by a
flag that we set in the Chrome OS scripts, I guess)
- the actual resolution of the host display (not sure how this should get there)

[sadrul, 2011/10/23 20:45:02]

I was thinking perhaps we can always to this for chromeos (by using
system::runtime_environment::IsRunningOnChromeOS from within chrome to confirm
chrome is running in proper chromeos, and not a chromeos build on the desktop).

+    size_ = actual_bounds.size();
+  }
 
   xwindow_ = XCreateSimpleWindow(xdisplay_, DefaultRootWindow(xdisplay_),
-                                 bounds_.x(), bounds_.y(),
-                                 bounds_.width(), bounds_.height(),
+                                 actual_bounds.x(), actual_bounds.y(),
+                                 actual_bounds.width(), actual_bounds.height(),
                                  0, 0, 0);
 
   long event_mask = ButtonPressMask | ButtonReleaseMask |
                     KeyPressMask | KeyReleaseMask |
                     ExposureMask | VisibilityChangeMask |
                     StructureNotifyMask | PropertyChangeMask |
                     PointerMotionMask;
   XSelectInput(xdisplay_, xwindow_, event_mask);
   XFlush(xdisplay_);
 
@@ skipping 54 matching lines @@
       break;
     }
     case ConfigureNotify: {
       DCHECK_EQ(xdisplay_, xev->xconfigure.display);
       DCHECK_EQ(xwindow_, xev->xconfigure.window);
       DCHECK_EQ(xwindow_, xev->xconfigure.event);
 
       // It's possible that the X window may be resized by some other means than
       // from within aura (e.g. the X window manager can change the size). Make
       // sure the desktop size is maintained properly.
-      gfx::Size size(xev->xconfigure.width, xev->xconfigure.height);
-      if (bounds_.size() != size)
-        bounds_.set_size(size);
-      desktop_->OnHostResized(size);
+      HandleConfigureNotify(
+          gfx::Size(xev->xconfigure.width, xev->xconfigure.height));
       handled = true;
       break;
     }
 
     case GenericEvent: {
       ui::TouchFactory* factory = ui::TouchFactory::GetInstance();
       if (!factory->ShouldProcessXI2Event(xev))
         break;
 
       // If this is a motion event we want to coalesce all pending motion
@@ skipping 45 matching lines @@
   desktop_ = desktop;
 }
 
 gfx::AcceleratedWidget DesktopHostLinux::GetAcceleratedWidget() {
   return xwindow_;
 }
 
 void DesktopHostLinux::Show() {
   XMapWindow(xdisplay_, xwindow_);
 
+  // Wait for notification that the window is mapped, which happens
+  // asynchronously if there's a window manager running.
+  gfx::Size new_size = size_;
+  while (true) {
+    XEvent event;
+    XWindowEvent(xdisplay_, xwindow_, StructureNotifyMask, &event);
+    if (event.type == ConfigureNotify)
+      new_size.SetSize(event.xconfigure.width, event.xconfigure.height);
+    else if (event.type == MapNotify)
+      break;
+  }
+
   // If there's no window manager running, we need to assign the X input focus
-  // to our host window.  (If there's no window manager running, it should also
-  // be safe to assume that the host window will have been mapped by the time
-  // that our SetInputFocus request is received.)
-  if (!IsWindowManagerPresent())
+  // to our host window.
+  if (!IsWindowManagerPresent()) {
     XSetInputFocus(xdisplay_, xwindow_, RevertToNone, CurrentTime);

[sadrul, 2011/10/22 18:53:04]

How about we just XMapWindow+XFlush here and return, and call XSetInputFocus
from MapNotify event in Dispatch

[Daniel Erat, 2011/10/23 04:02:55]

Thanks, I like this more as well.  Sounds fine, as I don't think there's any
requirement that the host window actually has / is on the way to receiving the
focus by the time that this method returns.

+    XFlush(xdisplay_);
+  }
 
-  XFlush(xdisplay_);
+  // If we removed any ConfigureNotify events from the queue, make sure that we
+  // apply size changes mentioned in them.
+  if (new_size != size_)
+    HandleConfigureNotify(new_size);
 }
 
 gfx::Size DesktopHostLinux::GetSize() const {
-  return bounds_.size();
+  return size_;
 }
 
 void DesktopHostLinux::SetSize(const gfx::Size& size) {
-  if (bounds_.size() == size)
-    return;
-  bounds_.set_size(size);
-  XResizeWindow(xdisplay_, xwindow_, size.width(), size.height());
+  if (size_ != size)
+    XResizeWindow(xdisplay_, xwindow_, size.width(), size.height());
 }
 
 void DesktopHostLinux::SetCursor(gfx::NativeCursor cursor) {
   if (current_cursor_ == cursor)
     return;
   current_cursor_ = cursor;
   // Custom web cursors are handled directly.
   if (cursor == kCursorCustom)
     return;
   int cursor_shape = CursorShapeFromNative(cursor);
   ::Cursor xcursor = ui::GetXCursor(cursor_shape);
   XDefineCursor(xdisplay_, xwindow_, xcursor);
 }
 
 gfx::Point DesktopHostLinux::QueryMouseLocation() {
   ::Window root_return, child_return;
   int root_x_return, root_y_return, win_x_return, win_y_return;
   unsigned int mask_return;
   XQueryPointer(xdisplay_,
                 xwindow_,
                 &root_return,
                 &child_return,
                 &root_x_return, &root_y_return,
                 &win_x_return, &win_y_return,
                 &mask_return);
-  return gfx::Point(max(0, min(bounds_.width(), win_x_return)),
-                    max(0, min(bounds_.height(), win_y_return)));
+  return gfx::Point(max(0, min(size_.width(), win_x_return)),
+                    max(0, min(size_.height(), win_y_return)));
+}
+
+void DesktopHostLinux::HandleConfigureNotify(const gfx::Size& size) {
+  if (size_ != size)
+    size_ = size;
+  desktop_->OnHostResized(size);
 }
 
 bool DesktopHostLinux::IsWindowManagerPresent() {
   // Per ICCCM 2.8, "Manager Selections", window managers should take ownership
   // of WM_Sn selections (where n is a screen number).
   ::Atom wm_s0_atom = XInternAtom(xdisplay_, "WM_S0", False);
   return XGetSelectionOwner(xdisplay_, wm_s0_atom) != None;
 }
 
 }  // namespace
 
 // static
 DesktopHost* DesktopHost::Create(const gfx::Rect& bounds) {
   return new DesktopHostLinux(bounds);
 }
 
 }  // namespace aura