Refactor X11ForeignWindowManager (Reland)

ui/base/x/x11_window_event_manager.h

Index: ui/base/x/x11_window_event_manager.h
diff --git a/ui/base/x/x11_window_event_manager.h b/ui/base/x/x11_window_event_manager.h
new file mode 100644
index 0000000000000000000000000000000000000000..f9e1ad96907b06633d1e6a43fe5aacb02c8b983b
--- /dev/null
+++ b/ui/base/x/x11_window_event_manager.h
@@ -0,0 +1,81 @@
+// Copyright 2014 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.
+
+#ifndef UI_BASE_X_X11_WINDOW_EVENT_MANAGER_H_
+#define UI_BASE_X_X11_WINDOW_EVENT_MANAGER_H_
+
+#include <map>
+#include <vector>
+
+#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "ui/base/x/ui_base_x_export.h"
+#include "ui/gfx/x/x11_types.h"
+
+// A process wide singleton for selecting events on X windows which were not
+// created by Chrome.
+namespace base {
+template <typename T>
+struct DefaultSingletonTraits;
+}
+
+namespace ui {
+
+// Manages the events that Chrome has selected on X windows which were not
+// created by Chrome. This class allows several clients to select events

[Daniel Erat, 2016/09/07 00:37:33]

nit: maybe "... allows multiple clients within Chrome to select ..."? i think
that that helps a bit to make it clear that this isn't referring to X clients,
i.e. connections to the X server.

[Tom (Use chromium acct), 2016/09/07 18:02:23]

Done.

+// on the same X window.
+class UI_BASE_X_EXPORT XWindowEventManager {
+ public:
+  static XWindowEventManager* GetInstance();
+
+  class ScopedEventSelector {

[Daniel Erat, 2016/09/07 00:37:33]

nit: add a comment documenting what this does

[Tom (Use chromium acct), 2016/09/07 18:02:23]

Done.

+   public:
+    ScopedEventSelector(XID xid, uint32_t event_mask);
+    ~ScopedEventSelector();
+
+   private:
+    XID xid_;
+    uint32_t event_mask_;
+  };
+
+  // Guarantees that events on |event_mask| will be reported to Chrome.

[Daniel Erat, 2016/09/07 00:37:33]

nit: s/on/in/

[Tom (Use chromium acct), 2016/09/07 18:02:23]

Done.

+  void SelectEvents(XID xid, uint32_t event_mask);

[Daniel Erat, 2016/09/07 00:37:33]

under what circumstances would a caller use this instead of creating a
ScopedEventSelector? if callers should only use ScopedEventSelector, i think you
can make these methods be private.

[Tom (Use chromium acct), 2016/09/07 18:02:23]

There are some classes where an X window is not bound to any object's lifetime. 
For example in X11WholeScreenMoveLoop, we can have

X11WholeScreenMoveLoop()
RunMoveLoop() - creates a window
EndMoveLoop() - destroys the window
RunMoveLoop() - creates a window
EndMoveLoop() - destroys the window
~X11WholeScreenMoveLoop()

[Daniel Erat, 2016/09/07 20:00:07]

okay, but can't X11WholeScreenMoveLoop just have an
std::unique_ptr<XScopedEventSelector> member and init and reset it as needed?
i'm concerned about the case where one class has a bug in its calls to
SelectEvents() and DeselectEvents(), which ends up introducing unpredictable and
hard-to-track-down issues around other classes not receiving events. making
XScopedEventSelector be the only interface for selecting events feels like a
good way to prevent this.

+
+  // Deselects events on |event_mask|.  Chrome will stop receiving events for
+  // any set bit in |event_mask| only if no other client has selected that bit.
+  void DeselectEvents(XID xid, uint32_t event_mask);
+
+ private:
+  friend struct base::DefaultSingletonTraits<XWindowEventManager>;
+
+  struct MultiMask {

[Daniel Erat, 2016/09/07 00:37:33]

this should be a class instead of a struct, since it isn't a simple wrapper
around data. please add a brief comment describing what it does, and consider
just forward-declaring it here and putting the full definition in the .cc file.

[Tom (Use chromium acct), 2016/09/07 18:02:22]

Done.

+    MultiMask();
+    ~MultiMask();
+
+    void AddMask(uint32_t mask);
+    void RemoveMask(uint32_t mask);
+    uint32_t ToMask() const;
+
+    static constexpr auto kMaskSize = 25;
+
+    unsigned int mask_bits[kMaskSize];

[Daniel Erat, 2016/09/07 00:37:33]

rename to mask_bits_; also just make this be an int

[Tom (Use chromium acct), 2016/09/07 18:02:23]

Done.

+  };

[Daniel Erat, 2016/09/07 00:37:33]

DISALLOW_COPY_AND_ASSIGN(MultiMask);

[Tom (Use chromium acct), 2016/09/07 18:02:23]

Done.

+
+  XWindowEventManager();
+  ~XWindowEventManager();
+
+  void BeforeMaskChanged(XID xid);

[Daniel Erat, 2016/09/07 00:37:33]

nit: add comments documenting what these methods do

[Tom (Use chromium acct), 2016/09/07 18:02:23]

Done.

+  void AfterMaskChanged(XID xid);
+
+  // State for [Before|After]MaskChanged
+  uint32_t old_mask_;
+
+  std::map<XID, MultiMask> mask_map_;

[Daniel Erat, 2016/09/07 00:37:33]

std::unique_ptr<MultiMask>

[Tom (Use chromium acct), 2016/09/07 18:02:23]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(XWindowEventManager);
+};
+
+}  // namespace ui
+
+#endif  // UI_BASE_X_X11_FOREIGN_WINDOW_MANAGER_H_