[webkit-reviews] review granted: [Bug 178485] [GTK] Implement PAL::SleepDisabler : [Attachment 327980] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Dec 1 00:41:03 PST 2017
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?
More information about the webkit-reviews
mailing list