[webkit-reviews] review denied: [Bug 178485] [GTK] Implement PAL::SleepDisabler : [Attachment 327908] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Michael Catanzaro
<mcatanzaro at igalia.com>'s request for review:
Bug 178485: [GTK] Implement PAL::SleepDisabler
https://bugs.webkit.org/show_bug.cgi?id=178485

Attachment 327908: Patch

https://bugs.webkit.org/attachment.cgi?id=327908&action=review




--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 327908
  --> https://bugs.webkit.org/attachment.cgi?id=327908
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327908&action=review

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:54
> +    g_dbus_proxy_new_for_bus(G_BUS_TYPE_SESSION,
static_cast<GDBusProxyFlags>(G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES |
G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS),
> +	   nullptr, "org.freedesktop.ScreenSaver", "/ScreenSaver",
"org.freedesktop.ScreenSaver", m_cancellable.get(),

One good thing of the current code is that we connect to the screensaver on
demand and only once. Here it happens on demand, because SleepDisabler is only
created when needed and then destroyed, but we are connecting to the bus every
time it's created. Maybe we can create a private singleton for the proxy.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:57
> +	       auto* self = static_cast<SleepDisablerGtk*>(userData);
> +	       self->m_screenSaverProxy =
adoptGRef(g_dbus_proxy_new_for_bus_finish(result, nullptr));

self might be destroyed here at this point. You need to finish the operation
and properly handle the case of cancellation. In case of cancellation just
return silently, in case of other error, show a warnings and otherwise then get
self and initialize the proxy.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:61
> +		   GUniquePtr<char>
nameOwner(g_dbus_proxy_get_name_owner(self->m_screenSaverProxy.get()));
> +		   if (nameOwner)
> +		       self->acquireInhibitor();

How can happen that you get a valid proxy for ScreenSaver and name_owner is
nullptr?

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:87
> +	       auto* self = static_cast<SleepDisablerGtk*>(userData);
> +	       GUniqueOutPtr<GError> error;
> +	       GRefPtr<GVariant> returnValue =
adoptGRef(g_dbus_proxy_call_finish(self->m_screenSaverProxy.get(), result,
&error.outPtr()));

Same here about self and cancellation.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.h:45
> +    GRefPtr<GDBusProxy> m_screenSaverProxy { nullptr };

GRefPtr already initializes the pointer to nullptr.

> Source/WebCore/html/HTMLMediaElement.cpp:6614
> +    // FIXME: This #if should be removed, because GTK now supports
SleepDisabler, and because this
> +    // code is harmless on ports where SleepDisabler does nothing. However,
after brief testing on
> +    // YouTube, I noticed that sleep is not disabled during normal video
playback, but it *is*
> +    // disabled when a page playing video enters the page cache, until the
associated web view is
> +    // destroyed. Keep it Cocoa-only for now, so that cached pages don't
block sleep indefinitely.

Why don't you open a bug report to fix whatever is wrong instead of adding a
comment in the code?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:194
> +    std::unique_ptr<PAL::SleepDisabler> sleepDisabler { nullptr };

unique_ptr already initializes the pointer to nullptr.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1246
> -    webkitWebViewBaseInhibitScreenSaver(webkitWebViewBase);
> +    priv->sleepDisabler = PAL::SleepDisabler::create(_("Website running in
fullscreen mode"), PAL::SleepDisabler::Type::Display);

So, do we need to inhibit the screensaver from both the UI and Web process?