Remove x11::Event::window

x11::Event::window() was added during the transition from Xlib to
XProto because we had some code that relied on Xlib's XEvent->window.
Now, there are only 2 users left, so this CL refactors the remaining
users and removes x11::Event::window.  Having a "window" associated
with each event was never a good idea since events can have multiple
associated windows, or no windows, or a "drawable" that is a window,
etc.

R=sky
are disabled

Bug: None
Change-Id: Ib03a7998cd918b566529520e70aa90ca7ecceca6
Low-Coverage-Reason: TESTS_ARE_DISABLED: x_server_clipboard.cc tests
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4938031
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1209722}

remoting/host/linux/x_server_clipboard.cc

@@ -109,25 +109,32 @@ void XServerClipboard::SetClipboard(const std::string& mime_type,
 }
 
 void XServerClipboard::ProcessXEvent(const x11::Event& event) {
-  if (clipboard_window_ == x11::Window::None ||
-      event.window() != clipboard_window_) {
+  if (clipboard_window_ == x11::Window::None) {
     return;
   }
 
   if (auto* property_notify = event.As<x11::PropertyNotifyEvent>()) {
-    OnPropertyNotify(*property_notify);
+    if (property_notify->window == clipboard_window_) {
+      OnPropertyNotify(*property_notify);
+    }
   } else if (auto* selection_notify = event.As<x11::SelectionNotifyEvent>()) {
-    OnSelectionNotify(*selection_notify);
+    if (selection_notify->requestor == clipboard_window_) {
+      OnSelectionNotify(*selection_notify);
+    }
   } else if (auto* selection_request = event.As<x11::SelectionRequestEvent>()) {
-    OnSelectionRequest(*selection_request);
+    if (selection_request->owner == clipboard_window_) {
+      OnSelectionRequest(*selection_request);
+    }
   } else if (auto* selection_clear = event.As<x11::SelectionClearEvent>()) {
-    OnSelectionClear(*selection_clear);
-  }
-
-  if (auto* xfixes_selection_notify =
-          event.As<x11::XFixes::SelectionNotifyEvent>()) {
-    OnSetSelectionOwnerNotify(xfixes_selection_notify->selection,
-                              xfixes_selection_notify->selection_timestamp);
+    if (selection_clear->owner == clipboard_window_) {
+      OnSelectionClear(*selection_clear);
+    }
+  } else if (auto* xfixes_selection_notify =
+                 event.As<x11::XFixes::SelectionNotifyEvent>()) {
+    if (xfixes_selection_notify->window == clipboard_window_) {
+      OnSetSelectionOwnerNotify(xfixes_selection_notify->selection,
+                                xfixes_selection_notify->selection_timestamp);
+    }
   }
 }
 

ui/events/x/events_x_utils.cc

@@ -162,35 +162,6 @@ int GetEventFlagsFromXState(uint32_t state) {
   return GetEventFlagsFromXState(static_cast<x11::KeyButMask>(state));
 }
 
-int GetEventFlagsFromXKeyEvent(const x11::Event& xev) {
-  auto* key = xev.As<x11::KeyEvent>();
-  DCHECK(key);
-  const auto state = static_cast<int>(key->state);
-
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  const int ime_fabricated_flag = 0;
-#else
-  // XIM fabricates key events for the character compositions by XK_Multi_key.
-  // For example, when a user hits XK_Multi_key, XK_apostrophe, and XK_e in
-  // order to input "é", then XIM generates a key event with keycode=0 and
-  // state=0 for the composition, and the sequence of X11 key events will be
-  // XK_Multi_key, XK_apostrophe, **NoSymbol**, and XK_e.  If the user used
-  // shift key and/or caps lock key, state can be ShiftMask, LockMask or both.
-  //
-  // We have to send these fabricated key events to XIM so it can correctly
-  // handle the character compositions.
-  const auto detail = static_cast<uint8_t>(key->detail);
-  const auto shift_lock_mask =
-      static_cast<int>(x11::KeyButMask::Shift | x11::KeyButMask::Lock);
-  const bool fabricated_by_xim = detail == 0 && (state & ~shift_lock_mask) == 0;
-  const int ime_fabricated_flag =
-      fabricated_by_xim ? ui::EF_IME_FABRICATED_KEY : 0;
-#endif
-
-  return GetEventFlagsFromXState(state) |
-         (xev.send_event() ? ui::EF_FINAL : 0) | ime_fabricated_flag;
-}
-
 int GetEventFlagsFromXGenericEvent(const x11::Event& x11_event) {
   auto* xievent = x11_event.As<x11::Input::DeviceEvent>();
   DCHECK(xievent);
@@ -491,10 +462,37 @@ EventType EventTypeFromXEvent(const x11::Event& xev) {
   return ET_UNKNOWN;
 }
 
+int GetEventFlagsFromXKeyEvent(const x11::KeyEvent& key, bool send_event) {
+  const auto state = static_cast<int>(key.state);
+
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  const int ime_fabricated_flag = 0;
+#else
+  // XIM fabricates key events for the character compositions by XK_Multi_key.
+  // For example, when a user hits XK_Multi_key, XK_apostrophe, and XK_e in
+  // order to input "é", then XIM generates a key event with keycode=0 and
+  // state=0 for the composition, and the sequence of X11 key events will be
+  // XK_Multi_key, XK_apostrophe, **NoSymbol**, and XK_e.  If the user used
+  // shift key and/or caps lock key, state can be ShiftMask, LockMask or both.
+  //
+  // We have to send these fabricated key events to XIM so it can correctly
+  // handle the character compositions.
+  const auto detail = static_cast<uint8_t>(key.detail);
+  const auto shift_lock_mask =
+      static_cast<int>(x11::KeyButMask::Shift | x11::KeyButMask::Lock);
+  const bool fabricated_by_xim = detail == 0 && (state & ~shift_lock_mask) == 0;
+  const int ime_fabricated_flag =
+      fabricated_by_xim ? ui::EF_IME_FABRICATED_KEY : 0;
+#endif
+
+  return GetEventFlagsFromXState(state) | (send_event ? ui::EF_FINAL : 0) |
+         ime_fabricated_flag;
+}
+
 int EventFlagsFromXEvent(const x11::Event& xev) {
-  if (xev.As<x11::KeyEvent>()) {
+  if (auto* key = xev.As<x11::KeyEvent>()) {
     XModifierStateWatcher::GetInstance()->UpdateStateFromXEvent(xev);
-    return GetEventFlagsFromXKeyEvent(xev);
+    return GetEventFlagsFromXKeyEvent(*key, xev.send_event());
   }
   if (auto* button = xev.As<x11::ButtonEvent>()) {
     int flags = GetEventFlagsFromXState(button->state);

ui/events/x/events_x_utils.h

@@ -23,6 +23,11 @@ namespace ui {
 // Gets the EventType from a x11::Event.
 EVENTS_X_EXPORT EventType EventTypeFromXEvent(const x11::Event& xev);
 
+// Gets the EventFlags from a x11::KeyEvent.  `send_event` indicates if the
+// event was sent by an X11 client instead of the server.
+EVENTS_X_EXPORT int GetEventFlagsFromXKeyEvent(const x11::KeyEvent& key,
+                                               bool send_event);
+
 // Gets the EventFlags from a x11::Event.
 EVENTS_X_EXPORT int EventFlagsFromXEvent(const x11::Event& xev);
 

ui/gfx/x/event.cc

@@ -46,8 +46,6 @@ Event::Event(Event&& event) {
 }
 
 Event& Event::operator=(Event&& event) {
-  // `window_` borrowed from `event_`, so it must be reset first.
-  window_ = std::move(event.window_);
   event_ = std::move(event.event_);
   type_id_ = event.type_id_;
   sequence_ = event.sequence_;

ui/gfx/x/event.h

@@ -37,7 +37,6 @@ class COMPONENT_EXPORT(X11) Event {
                   delete reinterpret_cast<DecayT*>(e);
                 }
               }};
-    window_ = event->GetWindow();
   }
 
   Event();
@@ -71,12 +70,6 @@ class COMPONENT_EXPORT(X11) Event {
 
   uint32_t sequence() const { return sequence_; }
 
-  Window window() const { return window_ ? *window_ : Window::None; }
-  void set_window(Window window) {
-    if (window_)
-      *window_ = window;
-  }
-
   bool Initialized() const { return !!event_; }
 
  private:
@@ -92,10 +85,6 @@ class COMPONENT_EXPORT(X11) Event {
   // XProto event state.
   int type_id_ = 0;
   std::unique_ptr<void, void (*)(void*)> event_ = {nullptr, nullptr};
-
-  // This member points to a field in |event_|, or may be nullptr if there's no
-  // associated window for the event.  It's owned by |event_|, not us.
-  raw_ptr<Window> window_ = nullptr;
 };
 
 }  // namespace x11

ui/gfx/x/gen_xproto.py

@@ -727,40 +727,6 @@ def declare_fields(self, fields):
             for field_type_name in self.declare_field(field):
                 self.write('%s %s{};' % field_type_name)
 
-    # This tries to match XEvent.xany.window, except the window will be
-    # Window::None for events that don't have a window, unlike the XEvent
-    # union which will get whatever data happened to be at the offset of
-    # xany.window.
-    def get_window_field(self, event):
-        # The window field is not stored at any particular offset in the event,
-        # so get a list of all the window fields.
-        WINDOW_TYPES = set([
-            ('xcb', 'WINDOW'),
-            ('xcb', 'DRAWABLE'),
-            ('xcb', 'Glx', 'DRAWABLE'),
-        ])
-        # The window we want may not be the first in the list if there are
-        # multiple windows. This is a list of all possible window names,
-        # ordered from highest to lowest priority.
-        WINDOW_NAMES = [
-            'window',
-            'event',
-            'request_window',
-            'owner',
-        ]
-        windows = set([
-            field.field_name for field in event.fields
-            if field.field_type in WINDOW_TYPES
-        ])
-        if len(windows) == 0:
-            return ''
-        if len(windows) == 1:
-            return list(windows)[0]
-        for name in WINDOW_NAMES:
-            if name in windows:
-                return name
-        assert False
-
     def declare_event(self, event, name):
         event_name = name[-1] + 'Event'
         with Indent(self, 'struct %s {' % adjust_type_name(event_name), '};'):
@@ -775,11 +741,6 @@ def declare_event(self, event, name):
                     for opcode, opname in sorted(items):
                         self.write('%s = %s,' % (opname, opcode))
             self.declare_fields(event.fields)
-            self.write()
-            window_field = self.get_window_field(event)
-            ret = ('reinterpret_cast<x11::Window*>(&%s)' %
-                   window_field if window_field else 'nullptr')
-            self.write('x11::Window* GetWindow() { return %s; }' % ret)
         self.write()
 
     def declare_error(self, error, name):
@@ -1517,7 +1478,6 @@ def gen_event(self, name, event, proto):
                 self.write('{0} = static_cast<decltype({0})>({1});'.format(
                     'event_->opcode', opcode))
             self.write('event->event_ = {event_, deleter_};')
-            self.write('event->window_ = event_->GetWindow();')
             self.write('return;')
         self.write()
 

ui/gfx/x/generated_protos/glx.h

@@ -284,10 +284,6 @@ class COMPONENT_EXPORT(X11) Glx {
     uint16_t width{};
     uint16_t height{};
     uint16_t count{};
-
-    x11::Window* GetWindow() {
-      return reinterpret_cast<x11::Window*>(&drawable);
-    }
   };
 
   struct BufferSwapCompleteEvent {
@@ -301,10 +297,6 @@ class COMPONENT_EXPORT(X11) Glx {
     uint32_t msc_hi{};
     uint32_t msc_lo{};
     uint32_t sbc{};
-
-    x11::Window* GetWindow() {
-      return reinterpret_cast<x11::Window*>(&drawable);
-    }
   };
 
   struct RenderRequest {

ui/gfx/x/generated_protos/randr.h

@@ -244,10 +244,6 @@ class COMPONENT_EXPORT(X11) RandR {
     uint16_t height{};
     uint16_t mwidth{};
     uint16_t mheight{};
-
-    x11::Window* GetWindow() {
-      return reinterpret_cast<x11::Window*>(&request_window);
-    }
   };
 
   struct MonitorInfo {
@@ -334,8 +330,6 @@ class COMPONENT_EXPORT(X11) RandR {
     absl::optional<Pp> pp{};
     absl::optional<Rc> rc{};
     absl::optional<Lc> lc{};
-
-    x11::Window* GetWindow() { return nullptr; }
   };
 
   struct QueryVersionRequest {