[webkit-reviews] review granted: [Bug 178485] [GTK] Implement PAL::SleepDisabler : [Attachment 327980] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted 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 327980: Patch

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




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

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

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:38
> +SleepDisablerGtk::SleepDisablerGtk(const char* reason, Type type)

I'm sorry I didn't notice this before, but this is not specific to GTK at all,
I would call this GLib instead or even Fdo since this is the Freedesktop.org
implementation.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:58
> +	       if (error && g_error_matches(error.get(), G_IO_ERROR,
G_IO_ERROR_CANCELLED))

g_error_matches() handles null errors

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:62
> +	       auto* self = static_cast<SleepDisablerGtk*>(userData);
> +	       if (!error) {

You could early return here in case of error, just setting m_cancellable to
nullptr, and saving an indentation level for the rest of the code. Actually,
since it's expected that proxy is nullptr in case of error, I would remove the
assret adn check proxy instead of error to return early.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:65
> +		   GUniquePtr<char>
nameOwner(g_dbus_proxy_get_name_owner(proxy.get()));
> +		   if (nameOwner) {

if (GUniquePtr<char> nameOwner(g_dbus_proxy_get_name_owner(proxy.get())))

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:66
> +		       self->m_screenSaverProxy = proxy;

WTFMove(proxy)

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:92
> +	       if (error && g_error_matches(error.get(), G_IO_ERROR,
G_IO_ERROR_CANCELLED))

g_error_matches() handles null errors

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:116
> +	       GUniqueOutPtr<GError> error;
> +	       GRefPtr<GVariant> returnValue =
adoptGRef(g_dbus_proxy_call_finish(G_DBUS_PROXY(proxy), result,
&error.outPtr()));
> +	       if (error)
> +		   g_warning("Calling %s.UnInhibit failed: %s",
g_dbus_proxy_get_interface_name(G_DBUS_PROXY(proxy)), error->message);

Is this really useful? I would call g_dbus_proxy_call without a callback.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.h:30
> +#include <gio/gio.h>

I don't think this is needed, but if you need it, try to forward declare
GDBusProxy and GCancellable and move the header inclusion to the cpp

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:-503
> -   
g_cancellable_cancel(webView->priv->screenSaverInhibitCancellable.get());

sleepDisabler = nullptr to cancel it earlier.

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

Does the reason really need to be translated? where is the user going to see
this message?