1 From 8c1ebea5f601b0b5247535dcdfd01755f3e6e1a6 Mon Sep 17 00:00:00 2001
2 From: Andrew Wolfers <aswolfers@chromium.org>
3 Date: Tue, 19 Jul 2022 15:01:25 +0000
4 Subject: [PATCH] [x11][ozone] Fix X11 screensaver suspension.
6 X11 screensaver suspension was broken by https://crrev.com/c/3507472,
7 in which usage patterns were migrated to a non-stacking paradigm.
9 "Non-stacking" screensaver suspension describes an overriding behavior,
10 such that the last suspending or un-suspending call defines the current
11 state. Conversely, a "stacking" screensaver suspension paradigm allows
12 for parallel suspension, such that suspending calls are expected to be
13 matched by an equal number of un-suspending calls.
15 Documentation for `PlatformScreen::SetScreenSaverSuspended` (inherited
16 by `X11ScreenOzone`) explains that it should be used in a non-stacking
17 manner [1], which contradicts the child class's underlying
20 > If XScreenSaverSuspend is called multiple times with suspend set to
21 > 'True', it must be called an equal number of times with suspend set
22 > to 'False' in order for the screensaver timer to be restarted.
24 This change updates the documentation/API of the `PlatformScreen`
25 parent class to correctly describe the stacking behavior of child class
26 `X11ScreenOzone`. This change also updates the implementation of
27 `WaylandScreen` to a stacking version. Lastly, this change updates the
28 call sites of `PlatformScreen` according to the API change.
30 [1] https://crsrc.org/c/ui/ozone/public/platform_screen.h;l=96
31 [2] https://linux.die.net/man/3/xscreensaverunsetattributes
37 Change-Id: I60975c8da9f86a0f26f3c32cf49c4a7eeeea6a12
38 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759067
39 Commit-Queue: Andrew Wolfers <aswolfers@chromium.org>
40 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
41 Reviewed-by: Scott Violet <sky@chromium.org>
42 Cr-Commit-Position: refs/heads/main@{#1025717}
44 (cherry picked from commit e61f08f8dbf1ec7cead427f3c497934e9d0db35f)
46 ui/aura/screen_ozone.cc | 14 ++++++--
47 ui/aura/screen_ozone.h | 29 ++++++++++++----
48 ui/base/x/x11_util.h | 4 ++-
49 ui/display/screen.cc | 21 ++----------
50 ui/display/screen.h | 34 ++++++-------------
51 .../platform/wayland/host/wayland_screen.cc | 31 +++++++++++++++++
52 .../platform/wayland/host/wayland_screen.h | 30 +++++++++++++++-
53 ui/ozone/platform/x11/x11_screen_ozone.cc | 27 +++++++++++++--
54 ui/ozone/platform/x11/x11_screen_ozone.h | 19 ++++++++++-
55 ui/ozone/public/platform_screen.cc | 8 +++--
56 ui/ozone/public/platform_screen.h | 26 +++++++++++---
57 11 files changed, 182 insertions(+), 61 deletions(-)
59 diff --git a/ui/aura/screen_ozone.cc b/ui/aura/screen_ozone.cc
60 index a78a6a48f1..09f62dc982 100644
61 --- a/ui/aura/screen_ozone.cc
62 +++ b/ui/aura/screen_ozone.cc
65 #include "ui/aura/screen_ozone.h"
69 #include "ui/aura/client/screen_position_client.h"
70 #include "ui/aura/window.h"
71 #include "ui/aura/window_tree_host.h"
72 @@ -108,8 +110,16 @@ display::Display ScreenOzone::GetPrimaryDisplay() const {
75 #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
76 -bool ScreenOzone::SetScreenSaverSuspended(bool suspend) {
77 - return platform_screen_->SetScreenSaverSuspended(suspend);
78 +ScreenOzone::ScreenSaverSuspenderOzone::ScreenSaverSuspenderOzone(
79 + std::unique_ptr<ui::PlatformScreen::PlatformScreenSaverSuspender> suspender)
80 + : suspender_(std::move(suspender)) {}
82 +ScreenOzone::ScreenSaverSuspenderOzone::~ScreenSaverSuspenderOzone() = default;
84 +std::unique_ptr<display::Screen::ScreenSaverSuspender>
85 +ScreenOzone::SuspendScreenSaver() {
86 + return std::make_unique<ScreenSaverSuspenderOzone>(
87 + platform_screen_->SuspendScreenSaver());
89 #endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
91 diff --git a/ui/aura/screen_ozone.h b/ui/aura/screen_ozone.h
92 index 2970a0e0e7..d033abf366 100644
93 --- a/ui/aura/screen_ozone.h
94 +++ b/ui/aura/screen_ozone.h
96 #include "build/chromeos_buildflags.h"
97 #include "ui/aura/aura_export.h"
98 #include "ui/display/screen.h"
101 -class PlatformScreen;
103 +#include "ui/ozone/public/platform_screen.h"
107 @@ -48,6 +45,10 @@ class AURA_EXPORT ScreenOzone : public display::Screen {
108 display::Display GetDisplayMatching(
109 const gfx::Rect& match_rect) const override;
110 display::Display GetPrimaryDisplay() const override;
111 +#if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
112 + std::unique_ptr<display::Screen::ScreenSaverSuspender> SuspendScreenSaver()
114 +#endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
115 bool IsScreenSaverActive() const override;
116 base::TimeDelta CalculateIdleTime() const override;
117 void AddObserver(display::DisplayObserver* observer) override;
118 @@ -65,11 +66,27 @@ class AURA_EXPORT ScreenOzone : public display::Screen {
120 ui::PlatformScreen* platform_screen() { return platform_screen_.get(); }
123 #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
124 - bool SetScreenSaverSuspended(bool suspend) override;
125 + class ScreenSaverSuspenderOzone
126 + : public display::Screen::ScreenSaverSuspender {
128 + explicit ScreenSaverSuspenderOzone(
129 + std::unique_ptr<ui::PlatformScreen::PlatformScreenSaverSuspender>
132 + ScreenSaverSuspenderOzone(const ScreenSaverSuspenderOzone&) = delete;
133 + ScreenSaverSuspenderOzone& operator=(const ScreenSaverSuspenderOzone&) =
136 + ~ScreenSaverSuspenderOzone() override;
139 + std::unique_ptr<ui::PlatformScreen::PlatformScreenSaverSuspender>
142 #endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
145 gfx::AcceleratedWidget GetAcceleratedWidgetForWindow(
146 aura::Window* window) const;
148 diff --git a/ui/base/x/x11_util.h b/ui/base/x/x11_util.h
149 index bf36efe170..0692571582 100644
150 --- a/ui/base/x/x11_util.h
151 +++ b/ui/base/x/x11_util.h
152 @@ -337,7 +337,9 @@ COMPONENT_EXPORT(UI_BASE_X) bool IsCompositingManagerPresent();
153 COMPONENT_EXPORT(UI_BASE_X) bool IsX11WindowFullScreen(x11::Window window);
155 // Suspends or resumes the X screen saver, and returns whether the operation was
156 -// successful. Must be called on the UI thread.
157 +// successful. Must be called on the UI thread. If called multiple times with
158 +// |suspend| set to true, the screen saver is not un-suspended until this method
159 +// is called an equal number of times with |suspend| set to false.
160 COMPONENT_EXPORT(UI_BASE_X) bool SuspendX11ScreenSaver(bool suspend);
162 // Returns true if the window manager supports the given hint.
163 diff --git a/ui/display/screen.cc b/ui/display/screen.cc
164 index b9723889ce..70dc0a9f5c 100644
165 --- a/ui/display/screen.cc
166 +++ b/ui/display/screen.cc
167 @@ -85,26 +85,11 @@ void Screen::SetDisplayForNewWindows(int64_t display_id) {
170 #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
171 -std::unique_ptr<Screen::ScreenSaverSuspender> Screen::SuspendScreenSaver() {
172 - SetScreenSaverSuspended(true);
173 - screen_saver_suspension_count_++;
174 - return base::WrapUnique(new Screen::ScreenSaverSuspender(this));
177 -Screen::ScreenSaverSuspender::~ScreenSaverSuspender() {
178 - // Check that this suspender still refers to the active screen. Particularly
179 - // in tests, the screen might be destructed before the suspender.
180 - if (screen_ == GetScreen()) {
181 - screen_->screen_saver_suspension_count_--;
182 - if (screen_->screen_saver_suspension_count_ == 0) {
183 - screen_->SetScreenSaverSuspended(false);
187 +Screen::ScreenSaverSuspender::~ScreenSaverSuspender() = default;
189 -bool Screen::SetScreenSaverSuspended(bool suspend) {
190 +std::unique_ptr<Screen::ScreenSaverSuspender> Screen::SuspendScreenSaver() {
191 NOTIMPLEMENTED_LOG_ONCE();
195 #endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
197 diff --git a/ui/display/screen.h b/ui/display/screen.h
198 index a86c5b63cc..d04534006f 100644
199 --- a/ui/display/screen.h
200 +++ b/ui/display/screen.h
201 @@ -133,28 +133,22 @@ class DISPLAY_EXPORT Screen {
203 class ScreenSaverSuspender {
205 - ScreenSaverSuspender(const Screen&) = delete;
206 - ScreenSaverSuspender& operator=(const Screen&) = delete;
207 + ScreenSaverSuspender() = default;
209 - // Notifies |screen_| that this instance is being destructed, and causes its
210 - // platform-specific screensaver to be un-suspended if this is the last such
211 - // remaining instance.
212 - ~ScreenSaverSuspender();
213 + ScreenSaverSuspender(const ScreenSaverSuspender&) = delete;
214 + ScreenSaverSuspender& operator=(const ScreenSaverSuspender&) = delete;
217 - friend class Screen;
219 - explicit ScreenSaverSuspender(Screen* screen) : screen_(screen) {}
222 + // Causes the platform-specific screensaver to be un-suspended iff this is
223 + // the last remaining instance.
224 + virtual ~ScreenSaverSuspender() = 0;
227 // Suspends the platform-specific screensaver until the returned
228 - // |ScreenSaverSuspender| is destructed. This method allows stacking multiple
229 - // overlapping calls, such that the platform-specific screensaver will not be
230 - // un-suspended until all returned |SreenSaverSuspender| instances have been
232 - std::unique_ptr<ScreenSaverSuspender> SuspendScreenSaver();
233 + // |ScreenSaverSuspender| is destructed, or returns nullptr if suspension
234 + // failed. This method allows stacking multiple overlapping calls, such that
235 + // the platform-specific screensaver will not be un-suspended until all
236 + // returned |ScreenSaverSuspender| instances have been destructed.
237 + virtual std::unique_ptr<ScreenSaverSuspender> SuspendScreenSaver();
238 #endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
240 // Returns whether the screensaver is currently running.
241 @@ -200,12 +194,6 @@ class DISPLAY_EXPORT Screen {
242 const gfx::GpuExtraInfo& gpu_extra_info);
245 -#if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
246 - // Suspends or un-suspends the platform-specific screensaver, and returns
247 - // whether the operation was successful.
248 - virtual bool SetScreenSaverSuspended(bool suspend);
249 -#endif // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
251 void set_shutdown(bool shutdown) { shutdown_ = shutdown; }
254 diff --git a/ui/ozone/platform/wayland/host/wayland_screen.cc b/ui/ozone/platform/wayland/host/wayland_screen.cc
255 index 0c7dc5c02b..18cd81b472 100644
256 --- a/ui/ozone/platform/wayland/host/wayland_screen.cc
257 +++ b/ui/ozone/platform/wayland/host/wayland_screen.cc
258 @@ -327,6 +327,37 @@ display::Display WaylandScreen::GetDisplayMatching(
259 return display_matching ? *display_matching : GetPrimaryDisplay();
262 +std::unique_ptr<WaylandScreen::WaylandScreenSaverSuspender>
263 +WaylandScreen::WaylandScreenSaverSuspender::Create(WaylandScreen& screen) {
264 + auto suspender = base::WrapUnique(new WaylandScreenSaverSuspender(screen));
265 + if (suspender->is_suspending_) {
266 + screen.screen_saver_suspension_count_++;
273 +WaylandScreen::WaylandScreenSaverSuspender::WaylandScreenSaverSuspender(
274 + WaylandScreen& screen)
275 + : screen_(screen.GetWeakPtr()) {
276 + is_suspending_ = screen.SetScreenSaverSuspended(true);
279 +WaylandScreen::WaylandScreenSaverSuspender::~WaylandScreenSaverSuspender() {
280 + if (screen_ && is_suspending_) {
281 + screen_->screen_saver_suspension_count_--;
282 + if (screen_->screen_saver_suspension_count_ == 0) {
283 + screen_->SetScreenSaverSuspended(false);
288 +std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
289 +WaylandScreen::SuspendScreenSaver() {
290 + return WaylandScreenSaverSuspender::Create(*this);
293 bool WaylandScreen::SetScreenSaverSuspended(bool suspend) {
294 if (!connection_->zwp_idle_inhibit_manager())
296 diff --git a/ui/ozone/platform/wayland/host/wayland_screen.h b/ui/ozone/platform/wayland/host/wayland_screen.h
297 index 87358f4f06..8e5515104a 100644
298 --- a/ui/ozone/platform/wayland/host/wayland_screen.h
299 +++ b/ui/ozone/platform/wayland/host/wayland_screen.h
300 @@ -68,7 +68,8 @@ class WaylandScreen : public PlatformScreen {
301 const gfx::Point& point) const override;
302 display::Display GetDisplayMatching(
303 const gfx::Rect& match_rect) const override;
304 - bool SetScreenSaverSuspended(bool suspend) override;
305 + std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
306 + SuspendScreenSaver() override;
307 bool IsScreenSaverActive() const override;
308 base::TimeDelta CalculateIdleTime() const override;
309 void AddObserver(display::DisplayObserver* observer) override;
310 @@ -76,7 +77,33 @@ class WaylandScreen : public PlatformScreen {
311 std::vector<base::Value> GetGpuExtraInfo(
312 const gfx::GpuExtraInfo& gpu_extra_info) override;
315 + // Suspends or un-suspends the platform-specific screensaver, and returns
316 + // whether the operation was successful. Can be called more than once with the
317 + // same value for |suspend|, but those states should not stack: the first
318 + // alternating value should toggle the state of the suspend.
319 + bool SetScreenSaverSuspended(bool suspend);
322 + class WaylandScreenSaverSuspender
323 + : public PlatformScreen::PlatformScreenSaverSuspender {
325 + WaylandScreenSaverSuspender(const WaylandScreenSaverSuspender&) = delete;
326 + WaylandScreenSaverSuspender& operator=(const WaylandScreenSaverSuspender&) =
329 + ~WaylandScreenSaverSuspender() override;
331 + static std::unique_ptr<WaylandScreenSaverSuspender> Create(
332 + WaylandScreen& screen);
335 + explicit WaylandScreenSaverSuspender(WaylandScreen& screen);
337 + base::WeakPtr<WaylandScreen> screen_;
338 + bool is_suspending_ = false;
341 // All parameters are in DIP screen coordinates/units except |physical_size|,
342 // which is in physical pixels.
343 void AddOrUpdateDisplay(uint32_t output_id,
344 @@ -103,6 +130,7 @@ class WaylandScreen : public PlatformScreen {
347 wl::Object<zwp_idle_inhibitor_v1> idle_inhibitor_;
348 + uint32_t screen_saver_suspension_count_ = 0;
350 base::WeakPtrFactory<WaylandScreen> weak_factory_;
352 diff --git a/ui/ozone/platform/x11/x11_screen_ozone.cc b/ui/ozone/platform/x11/x11_screen_ozone.cc
353 index 53265ab58a..b450df9c83 100644
354 --- a/ui/ozone/platform/x11/x11_screen_ozone.cc
355 +++ b/ui/ozone/platform/x11/x11_screen_ozone.cc
358 #include "ui/ozone/platform/x11/x11_screen_ozone.h"
362 #include "base/containers/flat_set.h"
363 #include "ui/base/linux/linux_desktop.h"
364 #include "ui/base/x/x11_idle_query.h"
365 @@ -131,8 +133,29 @@ display::Display X11ScreenOzone::GetDisplayMatching(
366 return matching_display ? *matching_display : GetPrimaryDisplay();
369 -bool X11ScreenOzone::SetScreenSaverSuspended(bool suspend) {
370 - return SuspendX11ScreenSaver(suspend);
371 +X11ScreenOzone::X11ScreenSaverSuspender::X11ScreenSaverSuspender() {
372 + is_suspending_ = SuspendX11ScreenSaver(true);
375 +std::unique_ptr<X11ScreenOzone::X11ScreenSaverSuspender>
376 +X11ScreenOzone::X11ScreenSaverSuspender::Create() {
377 + auto suspender = base::WrapUnique(new X11ScreenSaverSuspender());
378 + if (suspender->is_suspending_) {
385 +X11ScreenOzone::X11ScreenSaverSuspender::~X11ScreenSaverSuspender() {
386 + if (is_suspending_) {
387 + SuspendX11ScreenSaver(false);
391 +std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
392 +X11ScreenOzone::SuspendScreenSaver() {
393 + return X11ScreenSaverSuspender::Create();
396 bool X11ScreenOzone::IsScreenSaverActive() const {
397 diff --git a/ui/ozone/platform/x11/x11_screen_ozone.h b/ui/ozone/platform/x11/x11_screen_ozone.h
398 index d86acae9aa..81e0fd13d8 100644
399 --- a/ui/ozone/platform/x11/x11_screen_ozone.h
400 +++ b/ui/ozone/platform/x11/x11_screen_ozone.h
401 @@ -50,7 +50,8 @@ class X11ScreenOzone : public PlatformScreen,
402 const gfx::Point& point) const override;
403 display::Display GetDisplayMatching(
404 const gfx::Rect& match_rect) const override;
405 - bool SetScreenSaverSuspended(bool suspend) override;
406 + std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
407 + SuspendScreenSaver() override;
408 bool IsScreenSaverActive() const override;
409 base::TimeDelta CalculateIdleTime() const override;
410 void AddObserver(display::DisplayObserver* observer) override;
411 @@ -66,6 +67,22 @@ class X11ScreenOzone : public PlatformScreen,
413 friend class X11ScreenOzoneTest;
415 + class X11ScreenSaverSuspender
416 + : public PlatformScreen::PlatformScreenSaverSuspender {
418 + X11ScreenSaverSuspender(const X11ScreenSaverSuspender&) = delete;
419 + X11ScreenSaverSuspender& operator=(const X11ScreenSaverSuspender&) = delete;
421 + ~X11ScreenSaverSuspender() override;
423 + static std::unique_ptr<X11ScreenSaverSuspender> Create();
426 + X11ScreenSaverSuspender();
428 + bool is_suspending_ = false;
431 // Overridden from ui::XDisplayManager::Delegate:
432 void OnXDisplayListUpdated() override;
433 float GetXDisplayScaleFactor() const override;
434 diff --git a/ui/ozone/public/platform_screen.cc b/ui/ozone/public/platform_screen.cc
435 index 98f599aa41..2353208396 100644
436 --- a/ui/ozone/public/platform_screen.cc
437 +++ b/ui/ozone/public/platform_screen.cc
438 @@ -30,9 +30,13 @@ std::string PlatformScreen::GetCurrentWorkspace() {
442 -bool PlatformScreen::SetScreenSaverSuspended(bool suspend) {
443 +PlatformScreen::PlatformScreenSaverSuspender::~PlatformScreenSaverSuspender() =
446 +std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
447 +PlatformScreen::SuspendScreenSaver() {
448 NOTIMPLEMENTED_LOG_ONCE();
453 bool PlatformScreen::IsScreenSaverActive() const {
454 diff --git a/ui/ozone/public/platform_screen.h b/ui/ozone/public/platform_screen.h
455 index 091220a99f..e4adfafce3 100644
456 --- a/ui/ozone/public/platform_screen.h
457 +++ b/ui/ozone/public/platform_screen.h
458 @@ -89,11 +89,27 @@ class COMPONENT_EXPORT(OZONE_BASE) PlatformScreen {
459 virtual display::Display GetDisplayMatching(
460 const gfx::Rect& match_rect) const = 0;
462 - // Suspends or un-suspends the platform-specific screensaver, and returns
463 - // whether the operation was successful. Can be called more than once with the
464 - // same value for |suspend|, but those states should not stack: the first
465 - // alternating value should toggle the state of the suspend.
466 - virtual bool SetScreenSaverSuspended(bool suspend);
467 + // Object which suspends the platform-specific screensaver for the duration of
469 + class PlatformScreenSaverSuspender {
471 + PlatformScreenSaverSuspender() = default;
473 + PlatformScreenSaverSuspender(const PlatformScreenSaverSuspender&) = delete;
474 + PlatformScreenSaverSuspender& operator=(
475 + const PlatformScreenSaverSuspender&) = delete;
477 + // Causes the platform-specific screensaver to be un-suspended iff this is
478 + // the last remaining instance.
479 + virtual ~PlatformScreenSaverSuspender() = 0;
482 + // Suspends the platform-specific screensaver until the returned
483 + // |PlatformScreenSaverSuspender| is destructed, or returns nullptr if
484 + // suspension failed. This method allows stacking multiple overlapping calls,
485 + // such that the platform-specific screensaver will not be un-suspended until
486 + // all returned |PlatformScreenSaverSuspender| instances have been destructed.
487 + virtual std::unique_ptr<PlatformScreenSaverSuspender> SuspendScreenSaver();
489 // Returns whether the screensaver is currently running.
490 virtual bool IsScreenSaverActive() const;