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

ui/aura/desktop_host_linux.cc

Index: ui/aura/desktop_host_linux.cc
diff --git a/ui/aura/desktop_host_linux.cc b/ui/aura/desktop_host_linux.cc
index ed47ca21dad976628412990014f2c581be1e0b07..0cf0db06f86ce4e2fd8393ddd19d735a1102cd7b 100644
--- a/ui/aura/desktop_host_linux.cc
+++ b/ui/aura/desktop_host_linux.cc
@@ -198,7 +198,12 @@ class DesktopHostLinux : public DesktopHost {
   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_;
@@ -211,7 +216,7 @@ class DesktopHostLinux : public DesktopHost {
   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);
 };
@@ -221,19 +226,22 @@ DesktopHostLinux::DesktopHostLinux(const gfx::Rect& bounds)
       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 |
@@ -308,10 +316,8 @@ base::MessagePumpDispatcher::DispatchStatus DesktopHostLinux::Dispatch(
       // 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;
     }
@@ -377,25 +383,38 @@ gfx::AcceleratedWidget DesktopHostLinux::GetAcceleratedWidget() {
 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) {
@@ -421,8 +440,14 @@ gfx::Point DesktopHostLinux::QueryMouseLocation() {
                 &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() {