Linux: Refactor X11DesktopHandler

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc

Index: ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
diff --git a/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc b/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
index aeadc78f2fe40a9863c39daa451852a86f78cf9f..11f8fe57ec53ad1cc3ff8ece36767da798cdb445 100644
--- a/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
+++ b/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
@@ -91,6 +91,7 @@ const char* kAtomsToCache[] = {
   "UTF8_STRING",
   "WM_DELETE_WINDOW",
   "WM_PROTOCOLS",
+  "_NET_ACTIVE_WINDOW",
   "_NET_FRAME_EXTENTS",
   "_NET_WM_CM_S0",
   "_NET_WM_DESKTOP",
@@ -182,6 +183,11 @@ DesktopWindowTreeHostX11::DesktopWindowTreeHostX11(
       custom_window_shape_(false),
       urgency_hint_set_(false),
       activatable_(true),
+      has_pointer_(false),
+      has_pointer_grab_(false),
+      has_pointer_focus_(false),
+      has_window_focus_(false),
+      has_focus_(false),
       close_widget_factory_(this) {
 }
 
@@ -230,20 +236,75 @@ gfx::Rect DesktopWindowTreeHostX11::GetX11RootWindowOuterBounds() 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();

[danakj, 2016/08/19 21:43:45]

Why do we not do this if we had pointer focus but lose it? Think
focus-follows-mouse WM behaviour.

[Tom (Use chromium acct), 2016/08/22 20:27:07]

You're right, I was unclear about the definition of capture.  It looks like the
cross-platform code uses the Windows definition of mouse capture, which is
"either when the mouse is over the capturing window, or when the mouse button
was pressed while the mouse was over the capturing window and the button is
still down."  I've updated the code to be exactly following this definition.

It's a little confusing having OnHostLostWindowCapture.  I think it should
instead be OnHostLostPointerCapture, especially since we now have variables
representing window focus and pointer focus in X11 code O_O

[danakj, 2016/08/26 19:58:43]

Agree, thanks for figuring out what it means

+
+  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();
   }
+  if (was_active_ != IsActive()) {
+    desktop_native_widget_aura_->HandleActivationChanged(IsActive());
+    native_widget_delegate_->AsWidget()->GetRootView()->SchedulePaint();
+  }
+}
 
-  desktop_native_widget_aura_->HandleActivationChanged(active);
+void DesktopWindowTreeHostX11::OnCrossingEvent(bool enter,
+                                               bool focus,

[danakj, 2016/08/19 21:43:45]

focus_in_window_or_ancestor?

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.

+                                               int detail) {
+  if (detail == NotifyInferior)

[danakj, 2016/08/19 21:43:45]

When the mouse moves from the root into |xwindow_| this would be an enter
NotifyInferior we care about, no? And when it moves from |xwindow_| to root it
would be a leave NotifyInferior?

[danakj, 2016/08/19 23:17:32]

Oh I see.

EnterNotify with Inferior means we're coming from a child to |xwindow_|
LeaveNotify with Inferior means we're going to a child from |xwindow_|.

Can you comment that?

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.

+    return;
+
+  BeforeActivationStateChanged();
+
+  has_pointer_ = enter;
+  if (focus && !has_window_focus_)

[danakj, 2016/08/19 21:43:45]

// If focus is not in an ancestor then this crossing event can't move pointer
focus relative to our window. Pointer focus can only be on an inferior of the
focused window, which we are not.  <- sound right?

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.  I wrote something different though.

+    has_pointer_focus_ = has_pointer_;
 
-  native_widget_delegate_->AsWidget()->GetRootView()->SchedulePaint();
+  AfterActivationStateChanged();
+}
+
+void DesktopWindowTreeHostX11::OnFocusEvent(bool focus_in,
+                                            int mode,
+                                            int detail) {
+  if (detail == NotifyInferior)

[danakj, 2016/08/19 23:29:58]

Can you say the same thing here moreorless?

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.

+    return;
+  BeforeActivationStateChanged();
+  if (mode != NotifyWhileGrabbed && detail != NotifyPointer)

[danakj, 2016/08/19 23:29:58]

Can you throw some comments here kinda like the header file? Like If not
NotifyWhileGrabbed then either we're gaining/losing focus without a grab or
because of a grab starting/ending?

[danakj, 2016/08/19 23:59:30]

Also can you explain by ignore Pointer here in a comment, esp as I don't see
this explained well in the focus_tracking.txt. My understanding is:

NotifyPointer happens in cases where we are acquiring or losing pointer focus
(ie focus is moving between windows, and we gain/lose focus only due to the
pointer) which is tracked by has_pointer_focus_ so in all cases we don't want to
consider has_window_focus_ or has_focus_ (which both exclude has_pointer_focus_)
to change.

I think it'd help me structure this in my head a bit if you did

if (detail != Pointer) {
  if (mode != WhileGrabbed)
    has_focus_ = focus_in;
  if (mode != Grab && mode != Ungrab)
    has_window_focus_ = focus_in;
}

Now all the "window focus stuff" is together.

if (mode != Grab && mode != Ungrab && has_pointer_) {
  switch (detail) ...
}

And the "pointer focus stuff" is on its own.

[danakj, 2016/08/19 23:59:30]

Maybe better would be to say that while grabbed we don't care about focus
changes? Events will continue going to the grab anyway, which we heard about via
NotifyGrab/Ungrab? I'm not actually sure how to generate a NotifyWhileGrabbed in
the wild.

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.

+    has_focus_ = focus_in;
+  if (mode != NotifyGrab && mode != NotifyUngrab &&
+      mode != XINotifyPassiveGrab && mode != XINotifyPassiveUngrab) {
+    if (detail != NotifyPointer)
+      has_window_focus_ = focus_in;
+    if (has_pointer_) {
+      switch (detail) {
+        case NotifyAncestor:
+        case NotifyVirtual:

[danakj, 2016/08/19 23:59:30]

Can you leave a comment why NonLinear(Virtual) are ignored here? I think this is
something like:

In the case of NonLinear, focus was not in an ancestor or descendant but the
pointer is known to be in our window. So we must not have had pointer focus (as
that only goes to ancestors/dendendants) and we are not gaining pointer focus.
So we can ignore those cases.

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.

+          has_pointer_focus_ = !focus_in;
+          break;
+        case NotifyPointer:
+          has_pointer_focus_ = focus_in;
+          break;
+        default:
+          break;
+      }
+    }
+  }
+  ignore_input_ = false;
+  AfterActivationStateChanged();
 }
 
 void DesktopWindowTreeHostX11::AddObserver(
@@ -308,8 +369,8 @@ void DesktopWindowTreeHostX11::OnNativeWidgetCreated(
   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.
@@ -406,7 +467,7 @@ void DesktopWindowTreeHostX11::ShowWindowWithState(
     ui::WindowShowState show_state) {
   if (compositor())
     compositor()->SetVisible(true);
-  if (!window_mapped_)
+  if (!IsVisible())
     MapWindow(show_state);
 
   switch (show_state) {
@@ -426,8 +487,7 @@ void DesktopWindowTreeHostX11::ShowWindowWithState(
   // 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();
   }
 
@@ -443,7 +503,7 @@ void DesktopWindowTreeHostX11::ShowMaximizedWithBounds(
 }
 
 bool DesktopWindowTreeHostX11::IsVisible() const {
-  return window_mapped_;
+  return window_mapped_ && !wait_for_unmap_;
 }
 
 void DesktopWindowTreeHostX11::SetSize(const gfx::Size& requested_size) {
@@ -614,22 +674,77 @@ void DesktopWindowTreeHostX11::SetShape(
 }
 
 void DesktopWindowTreeHostX11::Activate() {
-  if (!window_mapped_)
+  if (!IsVisible() || !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"));
+
+  Time timestamp = ui::X11EventSource::GetInstance()->GetTimestamp();
+
+  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] = timestamp;
+    // TODO(thomasanderson): if another chrome window is active, specify that in
+    // data.l[2].  The EWMH spec claims this may make the WM more likely to
+    // service our _NET_ACTIVE_WINDOW request.
+    xclient.xclient.data.l[2] = None;
+    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, timestamp);
+    // 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.
+  bool is_active = (has_focus_ || has_pointer_focus_) && !ignore_input_;

[danakj, 2016/08/19 23:29:58]

FWIW normally an X window will show itself as visible active even tho it doesn't
have focus during a grab. This looks like it would not do that. I think you want
actually has_window_focus_ here if this is for display the focus state to the
user (title bar color etc), but has_focus_ if this is for deciding if we expect
keyboard input.

Try alt-tabbing with most WMs and I think you should see a LeaveFocus with
NotifyGrab while holding alt, and EnterFocus NotifyUngrab when letting it go
(can see this in Openbox at least with this demo:
https://github.com/danakj/openbox/blob/master/tests/focusout.c

[Tom (Use chromium acct), 2016/08/22 20:27:07]

The cross-platform code seems to want a window to be active when it will receive
keyboard input, so that's the definition I went with here.

Like you said, we will receive keyboard input when has_focus_ == true.
However, we will also receive keyboard input when has_focus_ == false, but
has_pointer_focus_ == true.  An ancestor window has the focus, but the pointer
is over our window, so we will receive keystrokes.

A stripped-down demo of this is http://www.sbin.org/doc/Xlib/chapt_09.html,
under "Monitoring whether keyboard input will be available".
Cool demo, I didn't know you had your own WM.  I'll try it out :)

[danakj, 2016/08/22 20:32:42]

OK just to be clear then, when alt-tabbing does the active chrome window change
color to be unfocused now?

+
+  // is_active => window_mapped_
+  // !window_mapped_ => !is_active
+  DCHECK(!is_active || window_mapped_);
+
+  return is_active;
 }
 
 void DesktopWindowTreeHostX11::Maximize() {
@@ -650,7 +765,7 @@ void DesktopWindowTreeHostX11::Maximize() {
 
   // Some WMs do not respect maximization hints on unmapped windows, so we
   // save this one for later too.
-  should_maximize_after_map_ = !window_mapped_;
+  should_maximize_after_map_ = !IsVisible();
 
   // When we are in the process of requesting to maximize a window, we can
   // accurately keep track of our restored bounds instead of relying on the
@@ -971,7 +1086,7 @@ void DesktopWindowTreeHostX11::SizeConstraintsChanged() {
 
 gfx::Transform DesktopWindowTreeHostX11::GetRootTransform() const {
   display::Display display = display::Screen::GetScreen()->GetPrimaryDisplay();
-  if (window_mapped_) {
+  if (IsVisible()) {
     aura::Window* win = const_cast<aura::Window*>(window());
     display = display::Screen::GetScreen()->GetDisplayNearestWindow(win);
   }
@@ -996,9 +1111,8 @@ void DesktopWindowTreeHostX11::ShowImpl() {
 }
 
 void DesktopWindowTreeHostX11::HideImpl() {
-  if (window_mapped_) {
+  if (IsVisible()) {
     XWithdrawWindow(xdisplay_, xwindow_, 0);
-    window_mapped_ = false;
     wait_for_unmap_ = true;
   }
   native_widget_delegate_->OnNativeWidgetVisibilityChanged(false);
@@ -1424,7 +1538,7 @@ void DesktopWindowTreeHostX11::OnFrameExtentsUpdated() {
 }
 
 void DesktopWindowTreeHostX11::UpdateMinAndMaxSize() {
-  if (!window_mapped_)
+  if (!IsVisible())
     return;
 
   gfx::Size minimum_in_pixels =
@@ -1480,7 +1594,6 @@ void DesktopWindowTreeHostX11::UpdateWMUserTime(
                     PropModeReplace,
                     reinterpret_cast<const unsigned char *>(&wm_user_time_ms),
                     1);
-    X11DesktopHandler::get()->set_wm_user_time_ms(wm_user_time_ms);
   }
 }
 
@@ -1681,17 +1794,14 @@ void DesktopWindowTreeHostX11::MapWindow(ui::WindowShowState show_state) {
   // 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();
+  ignore_input_ = show_state == ui::SHOW_STATE_INACTIVE;
+  unsigned long wm_user_time_ms =
+      ignore_input_ ? 0 : ui::X11EventSource::GetInstance()->GetTimestamp();
   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();
@@ -1753,15 +1863,15 @@ uint32_t DesktopWindowTreeHostX11::DispatchEvent(
   switch (xev->type) {
     case EnterNotify:
     case LeaveNotify: {
+      OnCrossingEvent(xev->type == EnterNotify, xev->xcrossing.focus,
+                      xev->xcrossing.detail);
+
       // 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;
-
-      ui::MouseEvent mouse_event(xev);
-      DispatchMouseEvent(&mouse_event);
+      if (xev->xcrossing.detail != NotifyInferior) {
+        ui::MouseEvent mouse_event(xev);
+        DispatchMouseEvent(&mouse_event);
+      }
       break;
     }
     case Expose: {
@@ -1777,8 +1887,7 @@ uint32_t DesktopWindowTreeHostX11::DispatchEvent(
     }
     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;
 
@@ -1809,18 +1918,12 @@ uint32_t DesktopWindowTreeHostX11::DispatchEvent(
       }
       break;
     }
-    case FocusOut:
-      if (xev->xfocus.mode != NotifyGrab) {
-        ReleaseCapture();
-        OnHostLostWindowCapture();
-        X11DesktopHandler::get()->ProcessXEvent(xev);
-      } else {
-        dispatcher()->OnHostLostMouseGrab();
-      }
-      break;
     case FocusIn:
-      X11DesktopHandler::get()->ProcessXEvent(xev);
+    case FocusOut: {

[danakj, 2016/08/19 21:43:45]

nit: {} are not needed since there's no vars declared in the case

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.

+      OnFocusEvent(xev->type == FocusIn, event->xfocus.mode,
+                   event->xfocus.detail);
       break;
+    }
     case ConfigureNotify: {
       DCHECK_EQ(xwindow_, xev->xconfigure.window);
       DCHECK_EQ(xwindow_, xev->xconfigure.event);
@@ -1860,6 +1963,22 @@ uint32_t DesktopWindowTreeHostX11::DispatchEvent(
       if (!factory->ShouldProcessXI2Event(xev))
         break;
 
+      XIEnterEvent* enter_event = static_cast<XIEnterEvent*>(xev->xcookie.data);
+      switch (static_cast<XIEvent*>(xev->xcookie.data)->evtype) {
+        case XI_Enter:
+        case XI_Leave:
+          OnCrossingEvent(enter_event->evtype == XI_Enter, enter_event->focus,
+                          enter_event->detail);
+          break;
+        case XI_FocusIn:
+        case XI_FocusOut:
+          OnFocusEvent(enter_event->evtype == XI_FocusIn, enter_event->mode,

[danakj, 2016/08/19 23:59:30]

Would you consider rewriting the mode to non-XI constants here so the
OnFocusEvent code doesn't have to deal with them?

[Tom (Use chromium acct), 2016/08/22 20:27:07]

Done.

+                       enter_event->detail);
+          break;
+        default:
+          break;
+      }
+
       ui::EventType type = ui::EventTypeFromNative(xev);
       XEvent last_event;
       int num_coalesced = 0;
@@ -1941,7 +2060,13 @@ uint32_t DesktopWindowTreeHostX11::DispatchEvent(
       break;
     }
     case UnmapNotify: {

[danakj, 2016/08/19 21:43:45]

jw do we care about DestroyNotify? Presumably no.. but checking

[Tom (Use chromium acct), 2016/08/22 20:27:07]

no

+      window_mapped_ = false;
       wait_for_unmap_ = false;
+      has_pointer_ = false;
+      has_pointer_grab_ = false;
+      has_pointer_focus_ = false;
+      has_window_focus_ = false;
+      has_focus_ = false;
       FOR_EACH_OBSERVER(DesktopWindowTreeHostObserverX11,
                         observer_list_,
                         OnWindowUnmapped(xwindow_));