[webkit-reviews] review denied: [Bug 231958] [GTK] Rewrite LowPowerModeNotifier to use GPowerProfileMonitor : [Attachment 441734] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied  review:
Bug 231958: [GTK] Rewrite LowPowerModeNotifier to use GPowerProfileMonitor
https://bugs.webkit.org/show_bug.cgi?id=231958

Attachment 441734: Patch

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




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

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

> Source/WebCore/platform/LowPowerModeNotifier.h:59
> +    static void powerSaverEnabledNotifyCallback(LowPowerModeNotifier*,
GParamSpec*, GPowerProfileMonitor*);

This is only used when glib is 2.69.1, we can use lambda instead for the
callback though.

> Source/WebCore/platform/LowPowerModeNotifier.h:64
>      LowPowerModeChangeCallback m_callback;

I think we could move this out of the ifdefs.

>> Source/WebCore/platform/LowPowerModeNotifier.h:65
>>	bool m_lowPowerModeEnabled { false };
> 
> Can you simplify things by doing #elif USE(GLIB) && GLIB_CHECK_VERSION(2, 69,
1) all in one condition? I bet it will turn out a little nicer. Ditto for the
.cpp file.

Do we still need m_lowPowerModeEnabled? Can't we just use
g_power_profile_monitor_get_power_saver_enabled()?

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:32
> -    : m_cancellable(adoptGRef(g_cancellable_new()))
> -    , m_callback(WTFMove(callback))
> +    : m_callback(WTFMove(callback))
>  {

Move the m_powerProfileMonitor initializartion here

#if GLIB_CHECK_VERSION(2, 69, 1)
    , m_powerProfileMonitor(adoptGRef(g_power_profile_monitor_dup_default()))
#endif

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:39
> +    g_signal_connect_swapped(m_powerProfileMonitor.get(),
"notify::power-saver-enabled", G_CALLBACK(powerSaverEnabledNotifyCallback),
this);

You can use G_CALLBACK(+[](LowPowerModeNotifier* self, GParamSpec*,
GPowerProfileMonitor* monitor) {

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:47
> +    self->m_lowPowerModeEnabled =
g_power_profile_monitor_get_power_saver_enabled(monitor);
> +    self->m_callback(self->m_lowPowerModeEnabled);

So, this could be
self->m_callback(g_power_profile_monitor_get_power_saver_enabled(monitor));

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:-100
> -    g_cancellable_cancel(m_cancellable.get());

Since we are getting a reference of a global object, unref won't destroy the
monitor, so you must disconnect the notify signal or we will get a crash the
nest time it changes.