Bug 231958 – [GTK] Rewrite LowPowerModeNotifier to use GPowerProfileMonitor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Patrick Griffis]

[GTK] Rewrite LowPowerModeNotifier to use GPowerProfileMonitor

[Patrick Griffis]

Created attachment 441733 [details]
Patch

[Patrick Griffis]

Created attachment 441734 [details]
Patch

[Michael Catanzaro]

This fixes bug #179978, right? Why not reuse that bug?

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #3)
> This fixes bug #179978, right? Why not reuse that bug?

I forgot it was a thing until I already did it. :/

[Michael Catanzaro]

*** Bug 179978 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Comment on attachment 441734 [details]
Patch

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

This is nice.

> Source/WebCore/platform/LowPowerModeNotifier.h:65
>  #elif USE(GLIB)
> -    void updateWarningLevel();
> -    void warningLevelChanged();
> -    static void gPropertiesChangedCallback(LowPowerModeNotifier*, GVariant* changedProperties);
> +    static void powerSaverEnabledNotifyCallback(LowPowerModeNotifier*, GParamSpec*, GPowerProfileMonitor*);
>  
> -    GRefPtr<GDBusProxy> m_displayDeviceProxy;
> -    GRefPtr<GCancellable> m_cancellable;
> +#if GLIB_CHECK_VERSION(2, 69, 1)
> +    GRefPtr<GPowerProfileMonitor> m_powerProfileMonitor;
> +#endif
>      LowPowerModeChangeCallback m_callback;
>      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.

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 441734 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441734&action=review

> 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.

Trying it out I think its just more messy. Like `m_lowPowerModeEnabled` defaulting to `false` and the impl just returning that results in less ifdefs. I also am not sure what changes you want in the .cpp file as its already as compact as it can be.

[Michael Catanzaro]

Comment on attachment 441734 [details]
Patch

OK, LGTM. I'll save cq+ for Carlos Garcia, just in case he objects to removing the compat path for older GLib.

[Carlos Garcia Campos]

Comment on attachment 441734 [details]
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.

[Patrick Griffis]

Created attachment 441893 [details]
Patch

[Patrick Griffis]

Created attachment 441898 [details]
Patch

[Patrick Griffis]

(In reply to Carlos Garcia Campos from comment #9)
> Comment on attachment 441734 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441734&action=review

Thanks for the detailed review I've applied these changes.

> > Source/WebCore/platform/LowPowerModeNotifier.h:64
> >      LowPowerModeChangeCallback m_callback;
> 
> I think we could move this out of the ifdefs.

Moving this out caused other EWS failures regarding constructor ordering. I just found it cleaner to leave it as-is.

> > 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.

Windows EWS fails with this, the preprocessor doesn't like it, so I've kept it as two #ifdefs.

> > 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.

Good catch.

[Patrick Griffis]

Created attachment 441904 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 441904 [details]
Patch

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

> Source/WebCore/platform/LowPowerModeNotifier.h:36
> +#include <gio/gio.h>

Move the include to the cpp file and here simply forward declare GPowerProfileMonitor

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:36
> +    m_callback(isLowPowerModeEnabled());

Why do we call the callback here? It's expected to be called when the mode changes, no? or is it because the object can be created when already in low power mode? That's not needed because the caller is expected to check the mode right after creating the object.

[Patrick Griffis]

Created attachment 442018 [details]
Patch

[EWS]

Committed r284670 (243390@main): <https://commits.webkit.org/243390@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442018 [details].