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..ed01f0eb5b7e9ca140e32bbde46bce053f107848 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",
@@ -230,20 +231,30 @@ 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();
+
+  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(
@@ -308,8 +319,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.
@@ -426,8 +437,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();
   }
 
@@ -614,22 +624,69 @@ void DesktopWindowTreeHostX11::SetShape(
 }
 
 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() {
@@ -1480,10 +1537,18 @@ 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);
   }
 }
 
+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) {
@@ -1681,17 +1746,17 @@ 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();
+  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();
@@ -1755,11 +1820,23 @@ uint32_t DesktopWindowTreeHostX11::DispatchEvent(
     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;
@@ -1777,8 +1854,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 +1885,34 @@ 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: {
+      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 ConfigureNotify: {
       DCHECK_EQ(xwindow_, xev->xconfigure.window);
       DCHECK_EQ(xwindow_, xev->xconfigure.event);