From 8c1ebea5f601b0b5247535dcdfd01755f3e6e1a6 Mon Sep 17 00:00:00 2001 From: Andrew Wolfers Date: Tue, 19 Jul 2022 15:01:25 +0000 Subject: [PATCH] [x11][ozone] Fix X11 screensaver suspension. X11 screensaver suspension was broken by https://crrev.com/c/3507472, in which usage patterns were migrated to a non-stacking paradigm. "Non-stacking" screensaver suspension describes an overriding behavior, such that the last suspending or un-suspending call defines the current state. Conversely, a "stacking" screensaver suspension paradigm allows for parallel suspension, such that suspending calls are expected to be matched by an equal number of un-suspending calls. Documentation for `PlatformScreen::SetScreenSaverSuspended` (inherited by `X11ScreenOzone`) explains that it should be used in a non-stacking manner [1], which contradicts the child class's underlying implementation [2]. > If XScreenSaverSuspend is called multiple times with suspend set to > 'True', it must be called an equal number of times with suspend set > to 'False' in order for the screensaver timer to be restarted. This change updates the documentation/API of the `PlatformScreen` parent class to correctly describe the stacking behavior of child class `X11ScreenOzone`. This change also updates the implementation of `WaylandScreen` to a stacking version. Lastly, this change updates the call sites of `PlatformScreen` according to the API change. [1] https://crsrc.org/c/ui/ozone/public/platform_screen.h;l=96 [2] https://linux.die.net/man/3/xscreensaverunsetattributes Bug: b:193670013 Bug: b:196213351 Bug: 1329573 Bug: 1339361 Change-Id: I60975c8da9f86a0f26f3c32cf49c4a7eeeea6a12 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759067 Commit-Queue: Andrew Wolfers Reviewed-by: Thomas Anderson Reviewed-by: Scott Violet Cr-Commit-Position: refs/heads/main@{#1025717} (cherry picked from commit e61f08f8dbf1ec7cead427f3c497934e9d0db35f) --- ui/aura/screen_ozone.cc | 14 ++++++-- ui/aura/screen_ozone.h | 29 ++++++++++++---- ui/base/x/x11_util.h | 4 ++- ui/display/screen.cc | 21 ++---------- ui/display/screen.h | 34 ++++++------------- .../platform/wayland/host/wayland_screen.cc | 31 +++++++++++++++++ .../platform/wayland/host/wayland_screen.h | 30 +++++++++++++++- ui/ozone/platform/x11/x11_screen_ozone.cc | 27 +++++++++++++-- ui/ozone/platform/x11/x11_screen_ozone.h | 19 ++++++++++- ui/ozone/public/platform_screen.cc | 8 +++-- ui/ozone/public/platform_screen.h | 26 +++++++++++--- 11 files changed, 182 insertions(+), 61 deletions(-) diff --git a/ui/aura/screen_ozone.cc b/ui/aura/screen_ozone.cc index a78a6a48f1..09f62dc982 100644 --- a/ui/aura/screen_ozone.cc +++ b/ui/aura/screen_ozone.cc @@ -4,6 +4,8 @@ #include "ui/aura/screen_ozone.h" +#include + #include "ui/aura/client/screen_position_client.h" #include "ui/aura/window.h" #include "ui/aura/window_tree_host.h" @@ -108,8 +110,16 @@ display::Display ScreenOzone::GetPrimaryDisplay() const { } #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) -bool ScreenOzone::SetScreenSaverSuspended(bool suspend) { - return platform_screen_->SetScreenSaverSuspended(suspend); +ScreenOzone::ScreenSaverSuspenderOzone::ScreenSaverSuspenderOzone( + std::unique_ptr suspender) + : suspender_(std::move(suspender)) {} + +ScreenOzone::ScreenSaverSuspenderOzone::~ScreenSaverSuspenderOzone() = default; + +std::unique_ptr +ScreenOzone::SuspendScreenSaver() { + return std::make_unique( + platform_screen_->SuspendScreenSaver()); } #endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) diff --git a/ui/aura/screen_ozone.h b/ui/aura/screen_ozone.h index 2970a0e0e7..d033abf366 100644 --- a/ui/aura/screen_ozone.h +++ b/ui/aura/screen_ozone.h @@ -11,10 +11,7 @@ #include "build/chromeos_buildflags.h" #include "ui/aura/aura_export.h" #include "ui/display/screen.h" - -namespace ui { -class PlatformScreen; -} +#include "ui/ozone/public/platform_screen.h" namespace aura { @@ -48,6 +45,10 @@ class AURA_EXPORT ScreenOzone : public display::Screen { display::Display GetDisplayMatching( const gfx::Rect& match_rect) const override; display::Display GetPrimaryDisplay() const override; +#if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) + std::unique_ptr SuspendScreenSaver() + override; +#endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) bool IsScreenSaverActive() const override; base::TimeDelta CalculateIdleTime() const override; void AddObserver(display::DisplayObserver* observer) override; @@ -65,11 +66,27 @@ class AURA_EXPORT ScreenOzone : public display::Screen { protected: ui::PlatformScreen* platform_screen() { return platform_screen_.get(); } + private: #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) - bool SetScreenSaverSuspended(bool suspend) override; + class ScreenSaverSuspenderOzone + : public display::Screen::ScreenSaverSuspender { + public: + explicit ScreenSaverSuspenderOzone( + std::unique_ptr + suspender); + + ScreenSaverSuspenderOzone(const ScreenSaverSuspenderOzone&) = delete; + ScreenSaverSuspenderOzone& operator=(const ScreenSaverSuspenderOzone&) = + delete; + + ~ScreenSaverSuspenderOzone() override; + + private: + std::unique_ptr + suspender_; + }; #endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) - private: gfx::AcceleratedWidget GetAcceleratedWidgetForWindow( aura::Window* window) const; diff --git a/ui/base/x/x11_util.h b/ui/base/x/x11_util.h index bf36efe170..0692571582 100644 --- a/ui/base/x/x11_util.h +++ b/ui/base/x/x11_util.h @@ -337,7 +337,9 @@ COMPONENT_EXPORT(UI_BASE_X) bool IsCompositingManagerPresent(); COMPONENT_EXPORT(UI_BASE_X) bool IsX11WindowFullScreen(x11::Window window); // Suspends or resumes the X screen saver, and returns whether the operation was -// successful. Must be called on the UI thread. +// successful. Must be called on the UI thread. If called multiple times with +// |suspend| set to true, the screen saver is not un-suspended until this method +// is called an equal number of times with |suspend| set to false. COMPONENT_EXPORT(UI_BASE_X) bool SuspendX11ScreenSaver(bool suspend); // Returns true if the window manager supports the given hint. diff --git a/ui/display/screen.cc b/ui/display/screen.cc index b9723889ce..70dc0a9f5c 100644 --- a/ui/display/screen.cc +++ b/ui/display/screen.cc @@ -85,26 +85,11 @@ void Screen::SetDisplayForNewWindows(int64_t display_id) { } #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) -std::unique_ptr Screen::SuspendScreenSaver() { - SetScreenSaverSuspended(true); - screen_saver_suspension_count_++; - return base::WrapUnique(new Screen::ScreenSaverSuspender(this)); -} - -Screen::ScreenSaverSuspender::~ScreenSaverSuspender() { - // Check that this suspender still refers to the active screen. Particularly - // in tests, the screen might be destructed before the suspender. - if (screen_ == GetScreen()) { - screen_->screen_saver_suspension_count_--; - if (screen_->screen_saver_suspension_count_ == 0) { - screen_->SetScreenSaverSuspended(false); - } - } -} +Screen::ScreenSaverSuspender::~ScreenSaverSuspender() = default; -bool Screen::SetScreenSaverSuspended(bool suspend) { +std::unique_ptr Screen::SuspendScreenSaver() { NOTIMPLEMENTED_LOG_ONCE(); - return false; + return nullptr; } #endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) diff --git a/ui/display/screen.h b/ui/display/screen.h index a86c5b63cc..d04534006f 100644 --- a/ui/display/screen.h +++ b/ui/display/screen.h @@ -133,28 +133,22 @@ class DISPLAY_EXPORT Screen { // its existence. class ScreenSaverSuspender { public: - ScreenSaverSuspender(const Screen&) = delete; - ScreenSaverSuspender& operator=(const Screen&) = delete; + ScreenSaverSuspender() = default; - // Notifies |screen_| that this instance is being destructed, and causes its - // platform-specific screensaver to be un-suspended if this is the last such - // remaining instance. - ~ScreenSaverSuspender(); + ScreenSaverSuspender(const ScreenSaverSuspender&) = delete; + ScreenSaverSuspender& operator=(const ScreenSaverSuspender&) = delete; - private: - friend class Screen; - - explicit ScreenSaverSuspender(Screen* screen) : screen_(screen) {} - - Screen* screen_; + // Causes the platform-specific screensaver to be un-suspended iff this is + // the last remaining instance. + virtual ~ScreenSaverSuspender() = 0; }; // Suspends the platform-specific screensaver until the returned - // |ScreenSaverSuspender| is destructed. This method allows stacking multiple - // overlapping calls, such that the platform-specific screensaver will not be - // un-suspended until all returned |SreenSaverSuspender| instances have been - // destructed. - std::unique_ptr SuspendScreenSaver(); + // |ScreenSaverSuspender| is destructed, or returns nullptr if suspension + // failed. This method allows stacking multiple overlapping calls, such that + // the platform-specific screensaver will not be un-suspended until all + // returned |ScreenSaverSuspender| instances have been destructed. + virtual std::unique_ptr SuspendScreenSaver(); #endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) // Returns whether the screensaver is currently running. @@ -200,12 +194,6 @@ class DISPLAY_EXPORT Screen { const gfx::GpuExtraInfo& gpu_extra_info); protected: -#if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) - // Suspends or un-suspends the platform-specific screensaver, and returns - // whether the operation was successful. - virtual bool SetScreenSaverSuspended(bool suspend); -#endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX) - void set_shutdown(bool shutdown) { shutdown_ = shutdown; } private: diff --git a/ui/ozone/platform/wayland/host/wayland_screen.cc b/ui/ozone/platform/wayland/host/wayland_screen.cc index 0c7dc5c02b..18cd81b472 100644 --- a/ui/ozone/platform/wayland/host/wayland_screen.cc +++ b/ui/ozone/platform/wayland/host/wayland_screen.cc @@ -327,6 +327,37 @@ display::Display WaylandScreen::GetDisplayMatching( return display_matching ? *display_matching : GetPrimaryDisplay(); } +std::unique_ptr +WaylandScreen::WaylandScreenSaverSuspender::Create(WaylandScreen& screen) { + auto suspender = base::WrapUnique(new WaylandScreenSaverSuspender(screen)); + if (suspender->is_suspending_) { + screen.screen_saver_suspension_count_++; + return suspender; + } + + return nullptr; +} + +WaylandScreen::WaylandScreenSaverSuspender::WaylandScreenSaverSuspender( + WaylandScreen& screen) + : screen_(screen.GetWeakPtr()) { + is_suspending_ = screen.SetScreenSaverSuspended(true); +} + +WaylandScreen::WaylandScreenSaverSuspender::~WaylandScreenSaverSuspender() { + if (screen_ && is_suspending_) { + screen_->screen_saver_suspension_count_--; + if (screen_->screen_saver_suspension_count_ == 0) { + screen_->SetScreenSaverSuspended(false); + } + } +} + +std::unique_ptr +WaylandScreen::SuspendScreenSaver() { + return WaylandScreenSaverSuspender::Create(*this); +} + bool WaylandScreen::SetScreenSaverSuspended(bool suspend) { if (!connection_->zwp_idle_inhibit_manager()) return false; diff --git a/ui/ozone/platform/wayland/host/wayland_screen.h b/ui/ozone/platform/wayland/host/wayland_screen.h index 87358f4f06..8e5515104a 100644 --- a/ui/ozone/platform/wayland/host/wayland_screen.h +++ b/ui/ozone/platform/wayland/host/wayland_screen.h @@ -68,7 +68,8 @@ class WaylandScreen : public PlatformScreen { const gfx::Point& point) const override; display::Display GetDisplayMatching( const gfx::Rect& match_rect) const override; - bool SetScreenSaverSuspended(bool suspend) override; + std::unique_ptr + SuspendScreenSaver() override; bool IsScreenSaverActive() const override; base::TimeDelta CalculateIdleTime() const override; void AddObserver(display::DisplayObserver* observer) override; @@ -76,7 +77,33 @@ class WaylandScreen : public PlatformScreen { std::vector GetGpuExtraInfo( const gfx::GpuExtraInfo& gpu_extra_info) override; + protected: + // Suspends or un-suspends the platform-specific screensaver, and returns + // whether the operation was successful. Can be called more than once with the + // same value for |suspend|, but those states should not stack: the first + // alternating value should toggle the state of the suspend. + bool SetScreenSaverSuspended(bool suspend); + private: + class WaylandScreenSaverSuspender + : public PlatformScreen::PlatformScreenSaverSuspender { + public: + WaylandScreenSaverSuspender(const WaylandScreenSaverSuspender&) = delete; + WaylandScreenSaverSuspender& operator=(const WaylandScreenSaverSuspender&) = + delete; + + ~WaylandScreenSaverSuspender() override; + + static std::unique_ptr Create( + WaylandScreen& screen); + + private: + explicit WaylandScreenSaverSuspender(WaylandScreen& screen); + + base::WeakPtr screen_; + bool is_suspending_ = false; + }; + // All parameters are in DIP screen coordinates/units except |physical_size|, // which is in physical pixels. void AddOrUpdateDisplay(uint32_t output_id, @@ -103,6 +130,7 @@ class WaylandScreen : public PlatformScreen { #endif wl::Object idle_inhibitor_; + uint32_t screen_saver_suspension_count_ = 0; base::WeakPtrFactory weak_factory_; }; diff --git a/ui/ozone/platform/x11/x11_screen_ozone.cc b/ui/ozone/platform/x11/x11_screen_ozone.cc index 53265ab58a..b450df9c83 100644 --- a/ui/ozone/platform/x11/x11_screen_ozone.cc +++ b/ui/ozone/platform/x11/x11_screen_ozone.cc @@ -4,6 +4,8 @@ #include "ui/ozone/platform/x11/x11_screen_ozone.h" +#include + #include "base/containers/flat_set.h" #include "ui/base/linux/linux_desktop.h" #include "ui/base/x/x11_idle_query.h" @@ -131,8 +133,29 @@ display::Display X11ScreenOzone::GetDisplayMatching( return matching_display ? *matching_display : GetPrimaryDisplay(); } -bool X11ScreenOzone::SetScreenSaverSuspended(bool suspend) { - return SuspendX11ScreenSaver(suspend); +X11ScreenOzone::X11ScreenSaverSuspender::X11ScreenSaverSuspender() { + is_suspending_ = SuspendX11ScreenSaver(true); +} + +std::unique_ptr +X11ScreenOzone::X11ScreenSaverSuspender::Create() { + auto suspender = base::WrapUnique(new X11ScreenSaverSuspender()); + if (suspender->is_suspending_) { + return suspender; + } + + return nullptr; +} + +X11ScreenOzone::X11ScreenSaverSuspender::~X11ScreenSaverSuspender() { + if (is_suspending_) { + SuspendX11ScreenSaver(false); + } +} + +std::unique_ptr +X11ScreenOzone::SuspendScreenSaver() { + return X11ScreenSaverSuspender::Create(); } bool X11ScreenOzone::IsScreenSaverActive() const { diff --git a/ui/ozone/platform/x11/x11_screen_ozone.h b/ui/ozone/platform/x11/x11_screen_ozone.h index d86acae9aa..81e0fd13d8 100644 --- a/ui/ozone/platform/x11/x11_screen_ozone.h +++ b/ui/ozone/platform/x11/x11_screen_ozone.h @@ -50,7 +50,8 @@ class X11ScreenOzone : public PlatformScreen, const gfx::Point& point) const override; display::Display GetDisplayMatching( const gfx::Rect& match_rect) const override; - bool SetScreenSaverSuspended(bool suspend) override; + std::unique_ptr + SuspendScreenSaver() override; bool IsScreenSaverActive() const override; base::TimeDelta CalculateIdleTime() const override; void AddObserver(display::DisplayObserver* observer) override; @@ -66,6 +67,22 @@ class X11ScreenOzone : public PlatformScreen, private: friend class X11ScreenOzoneTest; + class X11ScreenSaverSuspender + : public PlatformScreen::PlatformScreenSaverSuspender { + public: + X11ScreenSaverSuspender(const X11ScreenSaverSuspender&) = delete; + X11ScreenSaverSuspender& operator=(const X11ScreenSaverSuspender&) = delete; + + ~X11ScreenSaverSuspender() override; + + static std::unique_ptr Create(); + + private: + X11ScreenSaverSuspender(); + + bool is_suspending_ = false; + }; + // Overridden from ui::XDisplayManager::Delegate: void OnXDisplayListUpdated() override; float GetXDisplayScaleFactor() const override; diff --git a/ui/ozone/public/platform_screen.cc b/ui/ozone/public/platform_screen.cc index 98f599aa41..2353208396 100644 --- a/ui/ozone/public/platform_screen.cc +++ b/ui/ozone/public/platform_screen.cc @@ -30,9 +30,13 @@ std::string PlatformScreen::GetCurrentWorkspace() { return {}; } -bool PlatformScreen::SetScreenSaverSuspended(bool suspend) { +PlatformScreen::PlatformScreenSaverSuspender::~PlatformScreenSaverSuspender() = + default; + +std::unique_ptr +PlatformScreen::SuspendScreenSaver() { NOTIMPLEMENTED_LOG_ONCE(); - return false; + return nullptr; } bool PlatformScreen::IsScreenSaverActive() const { diff --git a/ui/ozone/public/platform_screen.h b/ui/ozone/public/platform_screen.h index 091220a99f..e4adfafce3 100644 --- a/ui/ozone/public/platform_screen.h +++ b/ui/ozone/public/platform_screen.h @@ -89,11 +89,27 @@ class COMPONENT_EXPORT(OZONE_BASE) PlatformScreen { virtual display::Display GetDisplayMatching( const gfx::Rect& match_rect) const = 0; - // Suspends or un-suspends the platform-specific screensaver, and returns - // whether the operation was successful. Can be called more than once with the - // same value for |suspend|, but those states should not stack: the first - // alternating value should toggle the state of the suspend. - virtual bool SetScreenSaverSuspended(bool suspend); + // Object which suspends the platform-specific screensaver for the duration of + // its existence. + class PlatformScreenSaverSuspender { + public: + PlatformScreenSaverSuspender() = default; + + PlatformScreenSaverSuspender(const PlatformScreenSaverSuspender&) = delete; + PlatformScreenSaverSuspender& operator=( + const PlatformScreenSaverSuspender&) = delete; + + // Causes the platform-specific screensaver to be un-suspended iff this is + // the last remaining instance. + virtual ~PlatformScreenSaverSuspender() = 0; + }; + + // Suspends the platform-specific screensaver until the returned + // |PlatformScreenSaverSuspender| is destructed, or returns nullptr if + // suspension failed. This method allows stacking multiple overlapping calls, + // such that the platform-specific screensaver will not be un-suspended until + // all returned |PlatformScreenSaverSuspender| instances have been destructed. + virtual std::unique_ptr SuspendScreenSaver(); // Returns whether the screensaver is currently running. virtual bool IsScreenSaverActive() const;