[XProto] Remove usage of XRRUpdateConfiguration and XRandR

Since we never initialize the libxrandr (by calling XRRQueryVersion), Xlib cannot decode the incoming randr notify events. This causes a crash in XRRUpdateConfiguration. This CL solves the issue by removing all calls to XRRUpdateConfiguration, which updates the XDisplay screen dimensions. This data is only used in the following APIs: XDisplayWidth XDisplayHeight XDisplayCells XDisplayWidthMM XDisplayHeightMM XWidthOfScreen XHeightOfScreen XWidthMMOfScreen XHeightMMOfScreen DisplayWidth DisplayHeight DisplayCells DisplayWidthMM DisplayHeightMM WidthOfScreen HeightOfScreen WidthMMOfScreen HeightMMOfScreen Therefore, this CL also adds a PRESUBMIT check to ensure they aren't being used. Since this is the last usage of libxrandr, the build config for it is removed. R=nickdiego,sky Bug: 1102059 Change-Id: Ib60811259ae23ffa8af9c50e0bb3d4ac2158e5af Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2285068 Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> Reviewed-by: Nick Yamane <nickdiego@igalia.com> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org> Cr-Commit-Position: refs/heads/master@{#786148}
Denger-Network · Jul 8, 2020 · 74d064b · 74d064b
1 parent def0fa3
commit 74d064b
Show file tree

Hide file tree

Showing 9 changed files with 74 additions and 43 deletions.
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
@@ -452,6 +452,14 @@
         r"^ui[\\/]base[\\/]x[\\/]xwmstartupcheck[\\/]xwmstartupcheck\.cc$",
         ),
     ),
+    (
+      r'/\WX?(((Width|Height)(MM)?OfScreen)|(Display(Width|Height)))\(',
+      (
+       'Use the corresponding fields in x11::Screen instead.',
+      ),
+      True,
+      (),
+    ),
     (
       r'/XInternAtom|xcb_intern_atom',
       (

diff --git a/build/config/linux/BUILD.gn b/build/config/linux/BUILD.gn
@@ -60,10 +60,6 @@ config("xext") {
   libs = [ "Xext" ]
 }
 
-config("xrandr") {
-  libs = [ "Xrandr" ]
-}
-
 config("xfixes") {
   libs = [ "Xfixes" ]
 }

diff --git a/remoting/host/desktop_resizer_x11.cc b/remoting/host/desktop_resizer_x11.cc
@@ -111,7 +111,8 @@ class ScreenResources {
   std::unique_ptr<x11::RandR::GetScreenResourcesCurrentReply> resources_;
 };
 
-class DesktopResizerX11 : public DesktopResizer {
+class DesktopResizerX11 : public DesktopResizer,
+                          public x11::Connection::Delegate {
  public:
   DesktopResizerX11();
   ~DesktopResizerX11() override;
@@ -124,6 +125,10 @@ class DesktopResizerX11 : public DesktopResizer {
   void RestoreResolution(const ScreenResolution& original) override;
 
  private:
+  // x11::Connection::Delegate:
+  bool ShouldContinueStream() const override;
+  void DispatchXEvent(x11::Event* event) override;
+
   // Add a mode matching the specified resolution and switch to it.
   void SetResolutionNewMode(const ScreenResolution& resolution);
 
@@ -174,28 +179,14 @@ DesktopResizerX11::DesktopResizerX11()
 DesktopResizerX11::~DesktopResizerX11() = default;
 
 ScreenResolution DesktopResizerX11::GetCurrentResolution() {
-  // Xrandr requires that we process RRScreenChangeNotify events, otherwise
-  // DisplayWidth and DisplayHeight do not return the current values. Normally,
-  // this would be done via a central X event loop, but we don't have one, hence
-  // this horrible hack.
-  //
-  // Note that the WatchFileDescriptor approach taken in XServerClipboard
-  // doesn't work here because resize events have already been read from the
-  // X server socket by the time the resize function returns, hence the
-  // file descriptor is never seen as readable.
-  if (has_randr_) {
-    while (XEventsQueued(connection_.display(), QueuedAlready)) {
-      XEvent event;
-      XNextEvent(connection_.display(), &event);
-      XRRUpdateConfiguration(&event);
-    }
-  }
+  // Process pending events so that the connection setup data is updated
+  // with the correct display metrics.
+  if (has_randr_)
+    connection_.Dispatch(this);
 
   ScreenResolution result(
-      webrtc::DesktopSize(DisplayWidth(connection_.display(),
-                                       DefaultScreen(connection_.display())),
-                          DisplayHeight(connection_.display(),
-                                        DefaultScreen(connection_.display()))),
+      webrtc::DesktopSize(connection_.default_screen().width_in_pixels,
+                          connection_.default_screen().height_in_pixels),
       webrtc::DesktopVector(kDefaultDPI, kDefaultDPI));
   return result;
 }
@@ -257,6 +248,12 @@ void DesktopResizerX11::RestoreResolution(const ScreenResolution& original) {
   SetResolution(original);
 }
 
+bool DesktopResizerX11::ShouldContinueStream() const {
+  return true;
+}
+
+void DesktopResizerX11::DispatchXEvent(x11::Event* event) {}
+
 void DesktopResizerX11::SetResolutionNewMode(
     const ScreenResolution& resolution) {
   // The name of the mode representing the current client view resolution and

diff --git a/ui/base/x/BUILD.gn b/ui/base/x/BUILD.gn
@@ -65,8 +65,6 @@ jumbo_component("x") {
     ]
   }
 
-  public_configs = [ "//build/config/linux:xrandr" ]
-
   defines = [ "IS_UI_BASE_X_IMPL" ]
 
   deps = [

diff --git a/ui/base/x/x11_display_manager.cc b/ui/base/x/x11_display_manager.cc
@@ -64,12 +64,6 @@ void XDisplayManager::RemoveObserver(display::DisplayObserver* observer) {
 
 bool XDisplayManager::ProcessEvent(x11::Event* xev) {
   DCHECK(xev);
-  if (xev->As<x11::RandR::ScreenChangeNotifyEvent>()) {
-    // TODO(https://crbug.com/1102059): Remove this since the Xlib even is
-    // nullptr since we don't initialize the extension, which causes a crash.
-    XRRUpdateConfiguration(&xev->xlib_event());
-    return true;
-  }
   auto* prop = xev->As<x11::PropertyNotifyEvent>();
   if (xev->As<x11::RandR::NotifyEvent>() ||
       (prop && prop->atom == gfx::GetAtom("_NET_WORKAREA"))) {

diff --git a/ui/base/x/x11_util.cc b/ui/base/x/x11_util.cc
@@ -1266,15 +1266,12 @@ bool IsX11WindowFullScreen(x11::Window window) {
   if (!ui::GetOuterWindowBounds(window, &window_rect))
     return false;
 
-  // We can't use display::Screen here because we don't have an aura::Window. So
-  // instead just look at the size of the default display.
-  //
-  // TODO(erg): Actually doing this correctly would require pulling out xrandr,
-  // which we don't even do in the desktop screen yet.
-  ::XDisplay* display = gfx::GetXDisplay();
-  ::Screen* screen = DefaultScreenOfDisplay(display);
-  int width = WidthOfScreen(screen);
-  int height = HeightOfScreen(screen);
+  // TODO(thomasanderson): We should use
+  // display::Screen::GetDisplayNearestWindow() instead of using the
+  // connection screen size, which encompasses all displays.
+  auto* connection = x11::Connection::Get();
+  int width = connection->default_screen().width_in_pixels;
+  int height = connection->default_screen().height_in_pixels;
   return window_rect.size() == gfx::Size(width, height);
 }
 

diff --git a/ui/gfx/x/connection.cc b/ui/gfx/x/connection.cc
@@ -14,6 +14,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "ui/gfx/x/bigreq.h"
 #include "ui/gfx/x/event.h"
+#include "ui/gfx/x/randr.h"
 #include "ui/gfx/x/x11_switches.h"
 #include "ui/gfx/x/xproto_internal.h"
 #include "ui/gfx/x/xproto_types.h"
@@ -171,6 +172,7 @@ void Connection::Dispatch(Delegate* delegate) {
 
     Event event = std::move(events_.front());
     events_.pop_front();
+    PreDispatchEvent(event);
     delegate->DispatchXEvent(&event);
   };
 
@@ -213,4 +215,40 @@ void Connection::AddRequest(unsigned int sequence,
   requests_.emplace(sequence, std::move(callback));
 }
 
+void Connection::PreDispatchEvent(const Event& event) {
+  // This is adapted from XRRUpdateConfiguration.
+  if (auto* configure = event.As<x11::ConfigureNotifyEvent>()) {
+    int index = ScreenIndexFromRootWindow(configure->window);
+    if (index != -1) {
+      setup_.roots[index].width_in_pixels = configure->width;
+      setup_.roots[index].height_in_pixels = configure->height;
+    }
+  } else if (auto* screen = event.As<x11::RandR::ScreenChangeNotifyEvent>()) {
+    int index = ScreenIndexFromRootWindow(configure->window);
+    DCHECK_GE(index, 0);
+    bool portrait = static_cast<bool>(
+        screen->rotation &
+        (x11::RandR::Rotation::Rotate_90 | x11::RandR::Rotation::Rotate_270));
+    if (portrait) {
+      setup_.roots[index].width_in_pixels = screen->height;
+      setup_.roots[index].height_in_pixels = screen->width;
+      setup_.roots[index].width_in_millimeters = screen->mheight;
+      setup_.roots[index].height_in_millimeters = screen->mwidth;
+    } else {
+      setup_.roots[index].width_in_pixels = screen->width;
+      setup_.roots[index].height_in_pixels = screen->height;
+      setup_.roots[index].width_in_millimeters = screen->mwidth;
+      setup_.roots[index].height_in_millimeters = screen->mheight;
+    }
+  }
+}
+
+int Connection::ScreenIndexFromRootWindow(x11::Window root) const {
+  for (size_t i = 0; i < setup_.roots.size(); i++) {
+    if (setup_.roots[i].root == root)
+      return i;
+  }
+  return -1;
+}
+
 }  // namespace x11
diff --git a/ui/gfx/x/connection.h b/ui/gfx/x/connection.h
@@ -96,6 +96,10 @@ class COMPONENT_EXPORT(X11) Connection : public XProto,
 
   bool HasNextResponse() const;
 
+  void PreDispatchEvent(const Event& event);
+
+  int ScreenIndexFromRootWindow(x11::Window root) const;
+
   XDisplay* const display_;
 
   uint32_t extended_max_request_length_ = 0;

diff --git a/ui/gfx/x/x11.h b/ui/gfx/x/x11.h
@@ -31,7 +31,6 @@ extern "C" {
 #include <X11/extensions/XShm.h>
 #include <X11/extensions/XTest.h>
 #include <X11/extensions/Xfixes.h>
-#include <X11/extensions/Xrandr.h>
 #include <X11/extensions/Xrender.h>
 #include <X11/extensions/record.h>
 #include <X11/extensions/sync.h>