[GTK] Rewrite LowPowerModeNotifier to use GPowerProfileMonitor
Created attachment 441733 [details] Patch
Created attachment 441734 [details] Patch
This fixes bug #179978, right? Why not reuse that bug?
(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. :/
*** Bug 179978 has been marked as a duplicate of this bug. ***
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.
(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.
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.
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.
Created attachment 441893 [details] Patch
Created attachment 441898 [details] Patch
(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.
Created attachment 441904 [details] Patch
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.
Created attachment 442018 [details] Patch
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].