Linux: Refactor X11DesktopHandler

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.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 "ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h"
 
 #include <X11/extensions/shape.h>
 #include <X11/extensions/XInput2.h>
 #include <X11/Xatom.h>
 #include <X11/Xregion.h>
@@ skipping 73 matching lines @@
 const int k_NET_WM_STATE_REMOVE = 0;
 
 // Special value of the _NET_WM_DESKTOP property which indicates that the window
 // should appear on all desktops.
 const int kAllDesktops = 0xFFFFFFFF;
 
 const char* kAtomsToCache[] = {
   "UTF8_STRING",
   "WM_DELETE_WINDOW",
   "WM_PROTOCOLS",
+  "_NET_ACTIVE_WINDOW",
   "_NET_FRAME_EXTENTS",
   "_NET_WM_CM_S0",
   "_NET_WM_DESKTOP",
   "_NET_WM_ICON",
   "_NET_WM_NAME",
   "_NET_WM_PID",
   "_NET_WM_PING",
   "_NET_WM_STATE",
   "_NET_WM_STATE_ABOVE",
   "_NET_WM_STATE_FULLSCREEN",
@@ skipping 119 matching lines @@
 gfx::Rect DesktopWindowTreeHostX11::GetX11RootWindowOuterBounds() const {
   gfx::Rect outer_bounds(bounds_in_pixels_);
   outer_bounds.Inset(-native_window_frame_borders_in_pixels_);
   return outer_bounds;
 }
 
 ::Region DesktopWindowTreeHostX11::GetWindowShape() const {
   return window_shape_.get();
 }
 
-void DesktopWindowTreeHostX11::HandleNativeWidgetActivationChanged(
-    bool active) {
-  if (active) {
+void DesktopWindowTreeHostX11::BeforeActivationStateChanged() {
+  was_active_ = IsActive();
+  had_pointer_grab_ = has_pointer_grab_;
+  had_window_focus_ = has_window_focus_;
+}
+
+void DesktopWindowTreeHostX11::AfterActivationStateChanged() {
+  if (had_pointer_grab_ && !has_pointer_grab_)
+    dispatcher()->OnHostLostMouseGrab();
+  if (had_window_focus_ && !has_window_focus_)
+    OnHostLostWindowCapture();
+
+  if (!was_active_ && IsActive()) {
     FlashFrame(false);
     OnHostActivated();
+    // TODO(thomasanderson): Remove this window shuffling and use XWindowCache
+    // instead.
     open_windows().remove(xwindow_);
     open_windows().insert(open_windows().begin(), xwindow_);
-  } else {
-    ReleaseCapture();
   }
-
-  desktop_native_widget_aura_->HandleActivationChanged(active);
-
-  native_widget_delegate_->AsWidget()->GetRootView()->SchedulePaint();
+  if (was_active_ != IsActive()) {
+    desktop_native_widget_aura_->HandleActivationChanged(IsActive());
+    native_widget_delegate_->AsWidget()->GetRootView()->SchedulePaint();

[danakj, 2016/08/11 21:48:20]

This really only needs to be done when becoming active? But I wonder if the
order matters relative to the line above?

[Tom (Use chromium acct), 2016/08/15 18:42:46]

Not only when it becomes active, when the activation changes.
Do you mean we should also repaint when the mouse grab or window focus changes?

+  }
 }
 
 void DesktopWindowTreeHostX11::AddObserver(
     views::DesktopWindowTreeHostObserverX11* observer) {
   observer_list_.AddObserver(observer);
 }
 
 void DesktopWindowTreeHostX11::RemoveObserver(
     views::DesktopWindowTreeHostObserverX11* observer) {
   observer_list_.RemoveObserver(observer);
@@ skipping 44 matching lines @@
     sanitized_params.bounds.set_height(100);
 
   InitX11Window(sanitized_params);
 }
 
 void DesktopWindowTreeHostX11::OnNativeWidgetCreated(
     const Widget::InitParams& params) {
   window()->SetProperty(kViewsWindowForRootWindow, content_window_);
   window()->SetProperty(kHostForRootWindow, this);
 
-  // Ensure that the X11DesktopHandler exists so that it dispatches activation
-  // messages to us.
+  // Ensure that the X11DesktopHandler exists so that it tracks create/destroy
+  // notify events.
   X11DesktopHandler::get();
 
   // TODO(erg): Unify this code once the other consumer goes away.
   SwapNonClientEventHandler(
       std::unique_ptr<ui::EventHandler>(new X11WindowEventFilter(this)));
   SetUseNativeFrame(params.type == Widget::InitParams::TYPE_WINDOW &&
                     !params.remove_standard_frame);
 
   x11_window_move_client_.reset(new X11DesktopWindowMoveClient);
   aura::client::SetWindowMoveClient(window(), x11_window_move_client_.get());
@@ skipping 96 matching lines @@
     case ui::SHOW_STATE_FULLSCREEN:
       SetFullscreen(true);
       break;
     default:
       break;
   }
 
   // Makes the window activated by default if the state is not INACTIVE or
   // MINIMIZED.
   if (show_state != ui::SHOW_STATE_INACTIVE &&
-      show_state != ui::SHOW_STATE_MINIMIZED &&
-      activatable_) {
+      show_state != ui::SHOW_STATE_MINIMIZED) {
     Activate();
   }
 
   native_widget_delegate_->AsWidget()->SetInitialFocus(show_state);
 }
 
 void DesktopWindowTreeHostX11::ShowMaximizedWithBounds(
     const gfx::Rect& restored_bounds) {
   ShowWindowWithState(ui::SHOW_STATE_MAXIMIZED);
   // Enforce |restored_bounds_in_pixels_| since calling Maximize() could have
@@ skipping 166 matching lines @@
     } else {
       window_shape_.reset(gfx::CreateRegionFromSkRegion(*native_region));
     }
 
     custom_window_shape_ = true;
   }
   ResetWindowRegion();
 }
 
 void DesktopWindowTreeHostX11::Activate() {
-  if (!window_mapped_)
+  if (!window_mapped_ || !activatable_)
     return;
 
-  X11DesktopHandler::get()->ActivateWindow(xwindow_);
+  BeforeActivationStateChanged();
+
+  ignore_input_ = false;
+
+  // wmii says that it supports _NET_ACTIVE_WINDOW but does not.
+  // https://code.google.com/p/wmii/issues/detail?id=266
+  static bool wm_supports_active_window =
+      ui::GuessWindowManager() != ui::WM_WMII &&
+      ui::WmSupportsHint(atom_cache_.GetAtom("_NET_ACTIVE_WINDOW"));
+
+  if (wm_supports_active_window) {
+    XEvent xclient;
+    memset(&xclient, 0, sizeof(xclient));
+    xclient.type = ClientMessage;
+    xclient.xclient.window = xwindow_;
+    xclient.xclient.message_type = atom_cache_.GetAtom("_NET_ACTIVE_WINDOW");
+    xclient.xclient.format = 32;
+    xclient.xclient.data.l[0] = 1;  // Specified we are an app.
+    xclient.xclient.data.l[1] = GetTimestamp();
+    xclient.xclient.data.l[2] = None;

[danakj, 2016/08/11 21:48:20]

This is actually a lie if another chrome window is active. Maybe leave a TODO?

[Tom (Use chromium acct), 2016/08/15 18:42:46]

Done.

+    xclient.xclient.data.l[3] = 0;
+    xclient.xclient.data.l[4] = 0;
+
+    XSendEvent(xdisplay_, x_root_window_, False,
+               SubstructureRedirectMask | SubstructureNotifyMask, &xclient);
+  } else {
+    XRaiseWindow(xdisplay_, xwindow_);
+    // Directly ask the X server to give focus to the window. Note
+    // that the call will raise an X error if the window is not
+    // mapped.
+    XSetInputFocus(xdisplay_, xwindow_, RevertToParent, CurrentTime);

[danakj, 2016/08/11 21:48:20]

Can you explain/document why CurrentTime and not GetTimestamp()?

[Tom (Use chromium acct), 2016/08/15 18:42:46]

Copy/paste bug.  Fixed.

+    // At this point, we know we will receive focus, and some tests depend on a
+    // window being IsActive() immediately after an Activate(), so just set this
+    // state now.
+    has_pointer_focus_ = false;
+    has_window_focus_ = true;
+  }
+  AfterActivationStateChanged();
 }
 
 void DesktopWindowTreeHostX11::Deactivate() {
-  if (!IsActive())
-    return;
+  BeforeActivationStateChanged();
+
+  // Ignore future input events.
+  ignore_input_ = true;
 
   ReleaseCapture();
-  X11DesktopHandler::get()->DeactivateWindow(xwindow_);
+  XLowerWindow(xdisplay_, xwindow_);
+
+  AfterActivationStateChanged();
 }
 
 bool DesktopWindowTreeHostX11::IsActive() const {
-  return X11DesktopHandler::get()->IsActiveWindow(xwindow_);
+  // Focus and stacking order are independent in X11.  Since we cannot guarantee
+  // a window is topmost iff it has focus, just use the focus state to determine
+  // if a window is active.  Note that Activate() and Deactivate() change the
+  // stacking order in addition to changing the focus state.
+  return window_mapped_ &&

[danakj, 2016/08/11 21:48:20]

the has focus things can't be true while window_mapped_ is false right? so
checking window_mapped_ looks redundant or could be a DCHECK instead?

[Tom (Use chromium acct), 2016/08/15 18:42:46]

Done.

+         (has_window_focus_ || has_pointer_grab_ || has_pointer_focus_) &&
+         !ignore_input_;
 }
 
 void DesktopWindowTreeHostX11::Maximize() {
   if (HasWMSpecProperty("_NET_WM_STATE_FULLSCREEN")) {
     // Unfullscreen the window if it is fullscreen.
     SetWMSpecState(false,
                    atom_cache_.GetAtom("_NET_WM_STATE_FULLSCREEN"),
                    None);
 
     // Resize the window so that it does not have the same size as a monitor.
@@ skipping 830 matching lines @@
     unsigned long wm_user_time_ms = static_cast<unsigned long>(
         (ui::EventTimeFromNative(event) - base::TimeTicks()).InMilliseconds());
     XChangeProperty(xdisplay_,
                     xwindow_,
                     atom_cache_.GetAtom("_NET_WM_USER_TIME"),
                     XA_CARDINAL,
                     32,
                     PropModeReplace,
                     reinterpret_cast<const unsigned char *>(&wm_user_time_ms),
                     1);
-    X11DesktopHandler::get()->set_wm_user_time_ms(wm_user_time_ms);
   }
 }
 
+Time DesktopWindowTreeHostX11::GetTimestamp() {
+  ui::X11EventSource* event_source = ui::X11EventSource::GetInstance();
+  Time time = event_source->last_seen_server_time();
+  if (time == CurrentTime) {

[danakj, 2016/08/11 21:48:20]

This feels like an approximation for what we want. But if we saw an event with a
time in it, and then leave the event handler, and later call this function,
we'll get an out-of-date timestamp.

I think this would do the right thing if last_seen_server_time became invalid
(CurrentTime) when we end any X event handler. Or maybe we do that?

[Tom (Use chromium acct), 2016/08/15 18:42:45]

We don't currently do that :(
But I think this is the right thing to do.  Maybe this would be better off in a
separate CL?

+    time = event_source->UpdateLastSeenServerTime();
+  }
+  return time;
+}
+
 void DesktopWindowTreeHostX11::SetWMSpecState(bool enabled,
                                               ::Atom state1,
                                               ::Atom state2) {
   XEvent xclient;
   memset(&xclient, 0, sizeof(xclient));
   xclient.type = ClientMessage;
   xclient.xclient.window = xwindow_;
   xclient.xclient.message_type = atom_cache_.GetAtom("_NET_WM_STATE");
   xclient.xclient.format = 32;
   xclient.xclient.data.l[0] =
@@ skipping 177 matching lines @@
   // will ignore toplevel XMoveWindow commands.
   XSizeHints size_hints;
   size_hints.flags = PPosition;
   size_hints.x = bounds_in_pixels_.x();
   size_hints.y = bounds_in_pixels_.y();
   XSetWMNormalHints(xdisplay_, xwindow_, &size_hints);
 
   // If SHOW_STATE_INACTIVE, tell the window manager not to focus the window
   // when mapping. This is done by setting the _NET_WM_USER_TIME to 0. See e.g.
   // http://standards.freedesktop.org/wm-spec/latest/ar01s05.html
-  unsigned long wm_user_time_ms = (show_state == ui::SHOW_STATE_INACTIVE) ?
-      0 : X11DesktopHandler::get()->wm_user_time_ms();
+  has_pointer_ = false;

[danakj, 2016/08/11 21:48:20]

I'd expect these to all be initialized to false, and set to false in UnmapNotify
perhaps, but they should already all be false when we go to Map the window if
the window isn't already mapped. Does that happen?

[Tom (Use chromium acct), 2016/08/15 18:42:45]

shouldn't happen, done

+  has_pointer_grab_ = false;
+  has_pointer_focus_ = false;
+  has_window_focus_ = false;
+  ignore_input_ = show_state == ui::SHOW_STATE_INACTIVE;
+  unsigned long wm_user_time_ms = ignore_input_ ? 0 : GetTimestamp();

[danakj, 2016/08/11 21:48:20]

So here we want to use the "window launch time" in preference to the most recent
event we've seen, right? It may be older and then the WM could know that the
user is using some other window in the meantime and avoid focusing the window
we're mapping here.

Or maybe only if this is happening from the startup_browser_creator. Are there
other cases where we'd map a window ourselves? In those cases we would want to
use the last event timestamp. Do you think there's a way to tell what we want
here?

[Tom (Use chromium acct), 2016/08/15 18:42:46]

We need the window launch time for _NET_ACTIVE_WINDOW.  New browser windows
always seem to get focus.  The problem is when we try to open a new tab in an
existing browser window.
I like your idea of rewriting GetTimestamp() to use the current event timestamp,
and I think we should always use that.

   if (show_state == ui::SHOW_STATE_INACTIVE || wm_user_time_ms != 0) {
-    XChangeProperty(xdisplay_,
-                    xwindow_,
-                    atom_cache_.GetAtom("_NET_WM_USER_TIME"),
-                    XA_CARDINAL,
-                    32,
-                    PropModeReplace,
-                    reinterpret_cast<const unsigned char *>(&wm_user_time_ms),
-                    1);
+    XChangeProperty(
+        xdisplay_, xwindow_, atom_cache_.GetAtom("_NET_WM_USER_TIME"),
+        XA_CARDINAL, 32, PropModeReplace,
+        reinterpret_cast<const unsigned char*>(&wm_user_time_ms), 1);
   }
 
   ui::X11EventSource* event_source = ui::X11EventSource::GetInstance();
   DCHECK(event_source);
 
   if (wait_for_unmap_) {
     // Block until our window is unmapped. This avoids a race condition when
     // remapping an unmapped window.
     event_source->BlockUntilWindowUnmapped(xwindow_);
     DCHECK(!wait_for_unmap_);
@@ skipping 43 matching lines @@
 
   UpdateWMUserTime(event);
 
   // May want to factor CheckXEventForConsistency(xev); into a common location
   // since it is called here.
   switch (xev->type) {
     case EnterNotify:
     case LeaveNotify: {
       // Ignore EventNotify and LeaveNotify events from children of |xwindow_|.
       // NativeViewGLSurfaceGLX adds a child to |xwindow_|.
-      // TODO(pkotwicz|tdanderson): Figure out whether the suppression is
-      // necessary. crbug.com/385716
       if (xev->xcrossing.detail == NotifyInferior)
         break;
 
+      BeforeActivationStateChanged();
+
+      if (xev->xcrossing.mode == NotifyGrab)
+        has_pointer_grab_ = xev->type == EnterNotify;
+      else if (xev->xcrossing.mode == NotifyUngrab)
+        has_pointer_grab_ = false;
+
+      has_pointer_ = xev->type == EnterNotify;
+      if(!xev->xcrossing.focus && has_window_focus_) {

[danakj, 2016/08/11 21:48:20]

git cl format

[Tom (Use chromium acct), 2016/08/15 18:42:45]

Done.

+        has_pointer_focus_ = has_pointer_;

[danakj, 2016/08/11 21:48:20]

In the header we claimed that has_window_focus_ and has_pointer_focus_ are
mutually exclusive but this code here appears to make it possible to disagree if
has_pointer_ is true.

I'm also a bit unclear what the if condition is going for.

!xcrossing.focus means X server does not thing our window or ancestor has focus.

has_window_focus_ means we had a FocusIn on our window but no FocusOut since.

How do we get into that position? XServer must have sent a FocusOut to us in
order to not think our window is focused which we should have seen before this
event?

[Tom (Use chromium acct), 2016/08/15 18:42:46]

Thanks for catching this terrible logic :)
The ! should have been on has_window_focus_.  Please double check again 

+        ignore_input_ = false;

[danakj, 2016/08/11 21:48:20]

I'm hesitant about the whole ignore_input_ thing. Can you tell me about the
Deactivate() thing and what the goals are there? That's not when we middle-click
the window titlebar area is it? Currently that sends to the bottom of the
stacking order but does not try to ignore input.

The implementation of ignore_input_ seems to be that if we Deactivate() then
move the mouse out and back into the window, we will begin responding to input
events, which seems inconsistent or hard to understand. What is the goal in
ignoring input events?

[Tom (Use chromium acct), 2016/08/15 18:42:46]

I don't know when we call Deactivate() in production code, but I know that we're
supposed to stop receiving input (or, at least that's what someone thought at
some point, based on some test cases).  We can't give up our focus, so we need
to just ignore events instead.
Even if we deactivate, users can still manually reactivate the window to make it
receive input again.  I wasn't sure whether we should do this for focus and
crossing events, or just focus events.  But now you have me convinced that it
should be just for focus events, so removing ignore_input_ = false here.

+      }
+      AfterActivationStateChanged();
+
       ui::MouseEvent mouse_event(xev);
       DispatchMouseEvent(&mouse_event);
       break;
     }
     case Expose: {
       gfx::Rect damage_rect_in_pixels(xev->xexpose.x, xev->xexpose.y,
                                       xev->xexpose.width, xev->xexpose.height);
       compositor()->ScheduleRedrawRect(damage_rect_in_pixels);
       break;
     }
     case KeyPress: {
       ui::KeyEvent keydown_event(xev);
       DispatchKeyEvent(&keydown_event);
       break;
     }
     case KeyRelease: {
       // There is no way to deactivate a window in X11 so ignore input if
-      // window is supposed to be 'inactive'. See comments in
-      // X11DesktopHandler::DeactivateWindow() for more details.
+      // window is supposed to be 'inactive'.
       if (!IsActive() && !HasCapture())
         break;
 
       ui::KeyEvent key_event(xev);
       DispatchKeyEvent(&key_event);
       break;
     }
     case ButtonPress:
     case ButtonRelease: {
       ui::EventType event_type = ui::EventTypeFromNative(xev);
@@ skipping 10 matching lines @@
           break;
         }
         case ui::ET_UNKNOWN:
           // No event is created for X11-release events for mouse-wheel buttons.
           break;
         default:
           NOTREACHED() << event_type;
       }
       break;
     }
-    case FocusOut:
-      if (xev->xfocus.mode != NotifyGrab) {
-        ReleaseCapture();
-        OnHostLostWindowCapture();
-        X11DesktopHandler::get()->ProcessXEvent(xev);
-      } else {
-        dispatcher()->OnHostLostMouseGrab();
+    case FocusIn:
+    case FocusOut: {
+      BeforeActivationStateChanged();
+      bool focus_in = xev->type == FocusIn;
+      auto mode = event->xfocus.mode;
+      bool grab_notify = mode == NotifyGrab || mode == XINotifyPassiveGrab ||
+                         mode == NotifyUngrab || mode == XINotifyPassiveUngrab;
+      switch (xev->xfocus.detail) {
+        case NotifyAncestor:
+        case NotifyVirtual:
+          if (has_pointer_ && !grab_notify)
+            has_pointer_focus_ = !focus_in;

[danakj, 2016/08/11 21:48:20]

I think you only want to do this for FocusOut. Otherwise if focus moved to an
inferior window that we're tracking, say, and the pointer was in it, you'd set
has_pointer_focus_ = true, but also has_window_focus_ = true?

I'd suggest breaking up the FocusIn vs FocusOut cases to make this easier. The
documentation for this case says:
It's explicitly for FocusOut only. Unless you're combining some other FocusIn
case, but then comparing the code to the docs is getting very hard at that
point.

[Tom (Use chromium acct), 2016/08/15 18:42:46]

Doesn't it say to do this for FocusIn as well?

+        // fallthrough
+        case NotifyNonlinear:
+        case NotifyNonlinearVirtual:
+          if (!grab_notify)
+            has_window_focus_ = focus_in;

[danakj, 2016/08/11 21:48:20]

The focus_tracking.txt doc says has_window_focus should be changed when detail
!= NotifyInferior. However you're not changing it on NotifyPointer, how come?

There are 3 cases where NotifyPointer can happen:
https://tronche.com/gui/x/xlib/events/input-focus/normal-and-grabbed.html

They all depend on the pointer being in an inferior window of a chrome window,
so it's possible they can't happen, but I'm curious why they're being treated as
they are.

I think it'd be easier to read this if we split the code for setting
has_pointer_focus_ and has_window_focus_ apart. We can have 2 switches if
needed.

[Tom (Use chromium acct), 2016/08/15 18:42:46]

Probably a mistake in the doc.  NotifyPointer shouldn't change the window focus,
right?
Done (split code for has_pointer_focus_ and has_window_focus_)

+          break;
+        case NotifyPointer:
+          if (!grab_notify)
+            has_pointer_focus_ = focus_in;
+        default:
+          break;
       }
+      ignore_input_ = false;
+      AfterActivationStateChanged();
       break;
-    case FocusIn:
-      X11DesktopHandler::get()->ProcessXEvent(xev);
-      break;
+    }
     case ConfigureNotify: {
       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 root window size is maintained properly.
       int translated_x_in_pixels = xev->xconfigure.x;
       int translated_y_in_pixels = xev->xconfigure.y;
       if (!xev->xconfigure.send_event && !xev->xconfigure.override_redirect) {
         Window unused;
@@ skipping 260 matching lines @@
   if (linux_ui) {
     ui::NativeTheme* native_theme = linux_ui->GetNativeTheme(window);
     if (native_theme)
       return native_theme;
   }
 
   return ui::NativeThemeAura::instance();
 }
 
 }  // namespace views