Bug 177810 – [GLib] Let WebCore know of low power situations

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

[GLib] Let WebCore know of low power situations

[Gustavo Noronha (kov)]

Created attachment 322513 [details]
Patch

[Gustavo Noronha (kov)]

Need libupower-glib-dev to be installed on the bots

[Carlos Garcia Campos]

Comment on attachment 322513 [details]
Patch

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

> Source/WebCore/platform/LowPowerModeNotifier.cpp:31
> -#if !PLATFORM(IOS)
> +#if !PLATFORM(IOS) && !PLATFORM(GTK) && !PLATFORM(WPE)

I think we should make libupower optional and add a USE() or HAVE() macro for it.

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:27
> +    , m_lowPowerModeEnabled(false)

Move this to the header.

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:29
> +    m_device = adoptGRef(up_client_get_display_device(m_upClient.get()));

This could be right after the m_upClient initialization, no?

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:43
> +    if (warningLevel > UP_DEVICE_LEVEL_DISCHARGING && warningLevel < UP_DEVICE_LEVEL_NORMAL)
> +        m_lowPowerModeEnabled = true;
> +    else
> +        m_lowPowerModeEnabled = false;

m_lowPowerModeEnabled = warningLevel > UP_DEVICE_LEVEL_DISCHARGING && warningLevel < UP_DEVICE_LEVEL_NORMAL no?

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:54
> +LowPowerModeNotifier::~LowPowerModeNotifier()
> +{
> +}

= default

[Michael Catanzaro]

Comment on attachment 322513 [details]
Patch

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

>> Source/WebCore/platform/LowPowerModeNotifier.cpp:31
>> +#if !PLATFORM(IOS) && !PLATFORM(GTK) && !PLATFORM(WPE)
> 
> I think we should make libupower optional and add a USE() or HAVE() macro for it.

I agree that it needs a USE macro, but I don't think it should be optional. upower is guaranteed to exist everywhere. And this is much more important for embedded than for desktops, so it doesn't make sense to allow disabling it for embedded.

> Source/cmake/OptionsGTK.cmake:54
> +find_package(UPowerGLib)

It should be REQUIRED. Especially since the build will fail with a compiler error without it!

If for some reason we decide to make it optional, then make sure the build works without it.

[Gustavo Noronha (kov)]

Created attachment 322520 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 322520 [details]
Patch

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

> Source/WebCore/platform/LowPowerModeNotifier.h:66
> +    bool m_lowPowerModeEnabled = false;

bool m_lowPowerModeEnabled { false };

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:48
> +LowPowerModeNotifier::~LowPowerModeNotifier() = default;

Now that I think of it, maybe we cant to disconnect the signal here.

[Gustavo Noronha (kov)]

Created attachment 322532 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 322532 [details]
Patch

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

> Source/WebCore/platform/LowPowerModeNotifier.h:38
> +#include <libupower-glib/upower.h>

Sorry, I didn't notice this before, but can we move this include to the cpp file?

[Gustavo Noronha (kov)]

(In reply to Carlos Garcia Campos from comment #8)
> > Source/WebCore/platform/LowPowerModeNotifier.h:38
> > +#include <libupower-glib/upower.h>
> 
> Sorry, I didn't notice this before, but can we move this include to the cpp
> file?

I tried forward declaring the upower types but the compiler didn't like it, complained about mismatch in how they were declared when building the cpp file. They don't seem to be declared in a way that is friendly to forward declarations, so I don't think we can =(

[Gustavo Noronha (kov)]

Committed r222834: <http://trac.webkit.org/changeset/222834>

[Carlos Alberto Lopez Perez]

(In reply to Gustavo Noronha (kov) from comment #10)
> Committed r222834: <http://trac.webkit.org/changeset/222834>

This broke the build.

FAILED: Source/WebCore/CMakeFiles/WebCore.dir/platform/glib/LowPowerModeNotifierGLib.cpp.o 
/usr/lib/ccache/c++   -DBUILDING_GTK__=1 -DBUILDING_WEBKIT -DBUILDING_WITH_CMAKE=1 -DBUILDING_WebCore -DDATA_DIR=\"share\" -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DSTATICALLY_LINKED_WITH_PAL=1 -DUSER_AGENT_MAJOR_VERSION=\"605\" -DUSER_AGENT_MINOR_VERSION=\"1\" -DWEBKITGTK_API_VERSION_STRING=\"4.0\" -I. -I../../Source/WebCore -I../../Source/WebCore/Modules/airplay -I../../Source/WebCore/Modules/beacon -I../../Source/WebCore/Modules/applepay -I../../Source/WebCore/Modules/cache -I../../Source/WebCore/Modules/credentials -I../../Source/WebCore/Modules/encryptedmedia -I../../Source/WebCore/Modules/encryptedmedia/legacy -I../../Source/WebCore/Modules/entriesapi -I../../Source/WebCore/Modules/fetch -I../../Source/WebCore/Modules/geolocation -I../../Source/WebCore/Modules/indexeddb -I../../Source/WebCore/Modules/indexeddb/client -I../../Source/WebCore/Modules/indexeddb/server -I../../Source/WebCore/Modules/indexeddb/shared -I../../Source/WebCore/Modules/mediacontrols -I../../Source/WebCore/Modules/mediasession -I../../Source/WebCore/Modules/mediasource -I../../Source/WebCore/Modules/mediastream -I../../Source/WebCore/Modules/navigatorcontentutils -I../../Source/WebCore/Modules/notifications -I../../Source/WebCore/Modules/paymentrequest -I../../Source/WebCore/Modules/plugins -I../../Source/WebCore/Modules/quota -I../../Source/WebCore/Modules/speech -I../../Source/WebCore/Modules/streams -I../../Source/WebCore/Modules/webaudio -I../../Source/WebCore/Modules/webdatabase -I../../Source/WebCore/Modules/webdriver -I../../Source/WebCore/Modules/websockets -I../../Source/WebCore/Modules/webvr -I../../Source/WebCore/accessibility -I../../Source/WebCore/animation -I../../Source/WebCore/bindings -I../../Source/WebCore/bindings/js -I../../Source/WebCore/bridge -I../../Source/WebCore/bridge/c -I../../Source/WebCore/bridge/jsc -I../../Source/WebCore/contentextensions -I../../Source/WebCore/crypto -I../../Source/WebCore/crypto/algorithms -I../../Source/WebCore/crypto/keys -I../../Source/WebCore/crypto/parameters -I../../Source/WebCore/css -I../../Source/WebCore/css/parser -I../../Source/WebCore/cssjit -I../../Source/WebCore/dom -I../../Source/WebCore/dom/default -I../../Source/WebCore/domjit -I../../Source/WebCore/editing -I../../Source/WebCore/fileapi -I../../Source/WebCore/history -I../../Source/WebCore/html -I../../Source/WebCore/html/canvas -I../../Source/WebCore/html/forms -I../../Source/WebCore/html/parser -I../../Source/WebCore/html/shadow -I../../Source/WebCore/html/track -I../../Source/WebCore/inspector -I../../Source/WebCore/loader -I../../Source/WebCore/loader/appcache -I../../Source/WebCore/loader/archive -I../../Source/WebCore/loader/archive/mhtml -I../../Source/WebCore/loader/cache -I../../Source/WebCore/loader/icon -I../../Source/WebCore/mathml -I../../Source/WebCore/page -I../../Source/WebCore/page/animation -I../../Source/WebCore/page/csp -I../../Source/WebCore/page/scrolling -I../../Source/WebCore/platform -I../../Source/WebCore/platform/animation -I../../Source/WebCore/platform/audio -I../../Source/WebCore/platform/crypto -I../../Source/WebCore/platform/encryptedmedia -I../../Source/WebCore/platform/gamepad -I../../Source/WebCore/platform/graphics -I../../Source/WebCore/platform/graphics/cpu/arm -I../../Source/WebCore/platform/graphics/cpu/arm/filters -I../../Source/WebCore/platform/graphics/displaylists -I../../Source/WebCore/platform/graphics/filters -I../../Source/WebCore/platform/graphics/harfbuzz -I../../Source/WebCore/platform/graphics/harfbuzz/ng -I../../Source/WebCore/platform/graphics/opentype -I../../Source/WebCore/platform/graphics/transforms -I../../Source/WebCore/platform/mediastream -I../../Source/WebCore/platform/mediastream/libwebrtc -I../../Source/WebCore/platform/mock -I../../Source/WebCore/platform/mock/mediasource -I../../Source/WebCore/platform/network -I../../Source/WebCore/platform/sql -I../../Source/WebCore/platform/text -I../../Source/WebCore/platform/text/icu -I../../Source/WebCore/plugins -I../../Source/WebCore/rendering -I../../Source/WebCore/rendering/line -I../../Source/WebCore/rendering/mathml -I../../Source/WebCore/rendering/shapes -I../../Source/WebCore/rendering/style -I../../Source/WebCore/rendering/svg -I../../Source/WebCore/replay -I../../Source/WebCore/storage -I../../Source/WebCore/style -I../../Source/WebCore/svg -I../../Source/WebCore/svg/animation -I../../Source/WebCore/svg/graphics -I../../Source/WebCore/svg/graphics/filters -I../../Source/WebCore/svg/properties -I../../Source/WebCore/websockets -I../../Source/WebCore/workers -I../../Source/WebCore/workers/service -I../../Source/WebCore/workers/service/server -I../../Source/WebCore/xml -I../../Source/WebCore/xml/parser -IDerivedSources/WebCore -I../../Source/WebCore/PAL -I../../Source -IDerivedSources/ForwardingHeaders/ANGLE -I../../Source/WebCore/platform/graphics/gpu -I../../Source/ThirdParty/woff2/src -I../../Source/ThirdParty/xdgmime/src -isystem ../DependenciesGTK/Root/include/cairo -I../../Source/WebCore/platform/graphics/cairo -I../DependenciesGTK/Root/include/freetype2/freetype -I../DependenciesGTK/Root/include/freetype2 -I../DependenciesGTK/Root/include/harfbuzz -I../../Source/WebCore/platform/graphics/freetype -I../../Source/WebCore/platform/mediastream/openwebrtc -I../../Source/WebCore/platform/graphics/gstreamer -I../../Source/WebCore/platform/graphics/gstreamer/mse -I../../Source/WebCore/platform/graphics/gstreamer/eme -I../../Source/WebCore/platform/audio/gstreamer -I../../Source/WebCore/platform/image-decoders -I../../Source/WebCore/platform/image-decoders/bmp -I../../Source/WebCore/platform/image-decoders/gif -I../../Source/WebCore/platform/image-decoders/ico -I../../Source/WebCore/platform/image-decoders/jpeg -I../../Source/WebCore/platform/image-decoders/png -I../../Source/WebCore/platform/image-decoders/webp -I../../Source/WebCore/platform/graphics/texmap -I../../Source/WebCore/page/scrolling/coordinatedgraphics -I../../Source/WebCore/platform/graphics/texmap/coordinated -I../../Source/ThirdParty/ANGLE -I../../Source/ThirdParty/ANGLE/include/KHR -I../../Source/WebCore/accessibility/atk -I../../Source/WebCore/editing/atk -I../../Source/WebCore/page/gtk -I../../Source/WebCore/platform/geoclue -I../../Source/WebCore/platform/gtk -I../../Source/WebCore/platform/graphics/egl -I../../Source/WebCore/platform/graphics/glx -I../../Source/WebCore/platform/graphics/gtk -I../../Source/WebCore/platform/graphics/opengl -I../../Source/WebCore/platform/graphics/wayland -I../../Source/WebCore/platform/graphics/x11 -I../../Source/WebCore/platform/mediastream/gtk -I../../Source/WebCore/platform/network/gtk -I../../Source/WebCore/platform/network/soup -I../../Source/WebCore/platform/text/gtk -I../../Source/WebCore/bindings/gobject -isystem ../DependenciesGTK/Root/include/libxml2 -isystem ../DependenciesGTK/Root/include/gstreamer-1.0 -isystem ../DependenciesGTK/Root/include/glib-2.0 -isystem ../DependenciesGTK/Root/lib/glib-2.0/include -isystem ../DependenciesGTK/Root/lib/gstreamer-1.0/include -isystem /usr/include/libdrm -isystem ../DependenciesGTK/Root/include/atk-1.0 -isystem /usr/include/enchant -isystem ../DependenciesGTK/Root/include/gio-unix-2.0 -isystem /usr/include/libsecret-1 -isystem ../DependenciesGTK/Root/include/libsoup-2.4 -I../../Source/JavaScriptCore -I../../Source/JavaScriptCore/.. -I../../Source/JavaScriptCore/API -I../../Source/JavaScriptCore/ForwardingHeaders -I../../Source/JavaScriptCore/assembler -I../../Source/JavaScriptCore/b3 -I../../Source/JavaScriptCore/b3/air -I../../Source/JavaScriptCore/bindings -I../../Source/JavaScriptCore/builtins -I../../Source/JavaScriptCore/bytecode -I../../Source/JavaScriptCore/bytecompiler -I../../Source/JavaScriptCore/dfg -I../../Source/JavaScriptCore/disassembler -I../../Source/JavaScriptCore/disassembler/udis86 -I../../Source/JavaScriptCore/disassembler/ARM64 -I../../Source/JavaScriptCore/domjit -I../../Source/JavaScriptCore/ftl -I../../Source/JavaScriptCore/heap -I../../Source/JavaScriptCore/debugger -I../../Source/JavaScriptCore/inspector -I../../Source/JavaScriptCore/inspector/agents -I../../Source/JavaScriptCore/inspector/augmentable -I../../Source/JavaScriptCore/inspector/remote -I../../Source/JavaScriptCore/interpreter -I../../Source/JavaScriptCore/jit -I../../Source/JavaScriptCore/llint -I../../Source/JavaScriptCore/parser -I../../Source/JavaScriptCore/profiler -I../../Source/JavaScriptCore/runtime -I../../Source/JavaScriptCore/tools -I../../Source/JavaScriptCore/wasm -I../../Source/JavaScriptCore/wasm/js -I../../Source/JavaScriptCore/yarr -IDerivedSources/ForwardingHeaders -IDerivedSources/JavaScriptCore -IDerivedSources/JavaScriptCore/inspector -IDerivedSources/JavaScriptCore/runtime -IDerivedSources/JavaScriptCore/yarr -I../../Source/JavaScriptCore/inspector/remote/glib -I../../Source/bmalloc -I../../Source/WTF -IDerivedSources -I../../Source/ThirdParty -fdiagnostics-color=always -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wextra -Wall  -fno-strict-aliasing -fno-exceptions -std=c++14 -fno-rtti -O3 -DNDEBUG -fPIC -MD -MT Source/WebCore/CMakeFiles/WebCore.dir/platform/glib/LowPowerModeNotifierGLib.cpp.o -MF Source/WebCore/CMakeFiles/WebCore.dir/platform/glib/LowPowerModeNotifierGLib.cpp.o.d -o Source/WebCore/CMakeFiles/WebCore.dir/platform/glib/LowPowerModeNotifierGLib.cpp.o -c /home/slave/webkitgtk/gtk-linux-64-release/build/Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp
/home/slave/webkitgtk/gtk-linux-64-release/build/Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp: In member function ‘void WebCore::LowPowerModeNotifier::updateState()’:
/home/slave/webkitgtk/gtk-linux-64-release/build/Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:39:90: error: ‘UP_DEVICE_LEVEL_NORMAL’ was not declared in this scope
     m_lowPowerModeEnabled = warningLevel > UP_DEVICE_LEVEL_DISCHARGING && warningLevel < UP_DEVICE_LEVEL_NORMAL;
                                                                                          ^~~~~~~~~~~~~~~~~~~~~~
[3520/4814] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/x11/XUniqueResource.cpp.o


https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/5786/steps/compile-webkit/logs/stdio


Is UP_DEVICE_LEVEL_NORMAL a valid keyword? for which version of libupower-glib?

Debian stable has this:


# apt-cache policy libupower-glib-dev
libupower-glib-dev:
  Installed: 0.99.4-4+b1
  Candidate: 0.99.4-4+b1
  Version table:
 *** 0.99.4-4+b1 500
        500 http://httpredir.debian.org/debian stretch/main amd64 Packages
        100 /var/lib/dpkg/status

# cat /usr/include/libupower-glib/up-types.h|grep UP_DEVICE_LEVEL_
	UP_DEVICE_LEVEL_UNKNOWN,
	UP_DEVICE_LEVEL_NONE,
	UP_DEVICE_LEVEL_DISCHARGING,
	UP_DEVICE_LEVEL_LOW,
	UP_DEVICE_LEVEL_CRITICAL,
	UP_DEVICE_LEVEL_ACTION,
	UP_DEVICE_LEVEL_LAST

[Bastien Nocera]

(In reply to Carlos Alberto Lopez Perez from comment #11)
> Is UP_DEVICE_LEVEL_NORMAL a valid keyword? for which version of
> libupower-glib?

0.99.5 (introduced in commit 4f9230900bdf2ff4fc22373a3a84f10146b9c1a8 and used in the one after that).

But you're completely right, this is incorrect:
 87 /**•
 88  * UpDeviceLevel:•
 89  *•
 90  * The level of a battery. Some values are only relevant to the WarningLevel•
 91  * property, some others to the BatteryLevel property.•

 39    m_lowPowerModeEnabled = warningLevel > UP_DEVICE_LEVEL_DISCHARGING && warningLevel < UP_DEVICE_LEVEL_NORMAL;

DISCHARGING is not used for WarningLevel, just for BatteryLevel. Unfortunately, the API docs on the web are for old versions (there's a bug filed against that). The D-Bus API documentation in UPower is however canonical.

I'll update the UpDeviceLevel API docs now.

Valid values for WarningLevel are:
UP_DEVICE_LEVEL_UNKNOWN
UP_DEVICE_LEVEL_NONE
UP_DEVICE_LEVEL_DISCHARGING (for UPSes)
UP_DEVICE_LEVEL_LOW
UP_DEVICE_LEVEL_CRITICAL
UP_DEVICE_LEVEL_ACTION

You should test the warning level against:
> NONE && <= UP_DEVICE_LEVEL_ACTION

[Michael Catanzaro]

Reopening. Will you handle this, Gustavo?

[Bastien Nocera]

There's a patch for better documentation here:
https://bugs.freedesktop.org/show_bug.cgi?id=103361

[Gustavo Noronha (kov)]

The build failure was fixed with a follow-up commit: http://trac.webkit.org/changeset/222835/webkit

I changed the upper limit check to <= ACTION. I'm not sure I understand the suggested change to the lower limit check, though. Is DISCHARGING to be considered a "very low power" situation, then?

[Bastien Nocera]

(In reply to Gustavo Noronha (kov) from comment #15)
> The build failure was fixed with a follow-up commit:
> http://trac.webkit.org/changeset/222835/webkit
> 
> I changed the upper limit check to <= ACTION. I'm not sure I understand the
> suggested change to the lower limit check, though. Is DISCHARGING to be
> considered a "very low power" situation, then?

Because it's not a valid value for WarningLevel. See the documentation patch.

[Gustavo Noronha (kov)]

Oh, OK. I see what you mean. So although DISCHARGING is an UpDeviceLevel it will never appear as a value for warning level. I don't think it makes a difference in practice, but I can land that change for sure. By the way, the documentation here is very outdated, it doesn't even mention the display device. Do you have the powers to get it refreshed? =) https://upower.freedesktop.org/docs/

[Bastien Nocera]

(In reply to Gustavo Noronha (kov) from comment #17)
> Oh, OK. I see what you mean. So although DISCHARGING is an UpDeviceLevel it
> will never appear as a value for warning level.

Yes.

> I don't think it makes a
> difference in practice, but I can land that change for sure.

The difference it makes is not requiring a newer UPower where UP_DEVICE_LEVEL_DISCHARGING exists :)

> By the way, the
> documentation here is very outdated, it doesn't even mention the display
> device. Do you have the powers to get it refreshed? =)
> https://upower.freedesktop.org/docs/

I don't. It's filed though:
https://bugs.freedesktop.org/show_bug.cgi?id=102363

[Gustavo Noronha (kov)]

Committed r223954: <https://trac.webkit.org/changeset/223954>

[Gustavo Noronha (kov)]

(In reply to Bastien Nocera from comment #18)
> > I don't think it makes a
> > difference in practice, but I can land that change for sure.
> 
> The difference it makes is not requiring a newer UPower where
> UP_DEVICE_LEVEL_DISCHARGING exists :)

I believe that's from around the same time as DisplayDevice, but anyway, it's landed \o/

> > By the way, the
> > documentation here is very outdated, it doesn't even mention the display
> > device. Do you have the powers to get it refreshed? =)
> > https://upower.freedesktop.org/docs/
> 
> I don't. It's filed though:
> https://bugs.freedesktop.org/show_bug.cgi?id=102363

Seems to have been updated today! Thanks =)

[Bastien Nocera]

(In reply to Gustavo Noronha (kov) from comment #20)
> (In reply to Bastien Nocera from comment #18)
> > > I don't think it makes a
> > > difference in practice, but I can land that change for sure.
> > 
> > The difference it makes is not requiring a newer UPower where
> > UP_DEVICE_LEVEL_DISCHARGING exists :)
> 
> I believe that's from around the same time as DisplayDevice, but anyway,
> it's landed \o/

It's not. It was added in 0.99.5 (released July 2017), DisplayDevice was added in 0.99.0 (released in 2013).

(I added both of those to upower)

> > > By the way, the
> > > documentation here is very outdated, it doesn't even mention the display
> > > device. Do you have the powers to get it refreshed? =)
> > > https://upower.freedesktop.org/docs/
> > 
> > I don't. It's filed though:
> > https://bugs.freedesktop.org/show_bug.cgi?id=102363
> 
> Seems to have been updated today! Thanks =)

Yeah, I poked Richard to fix it, and I'm trying to add the docs to developer.gnome.org as well, so we don't rely on non-automated sites for docs.

[Gustavo Noronha (kov)]

(In reply to Bastien Nocera from comment #21)
> It's not. It was added in 0.99.5 (released July 2017), DisplayDevice was
> added in 0.99.0 (released in 2013).

I think you're right about NORMAL, but DISCHARGING is from 2013 as well, no?

https://cgit.freedesktop.org/upower/commit/?id=b446cac8f697fb1417f8d07190f95fa40b690260

> > Seems to have been updated today! Thanks =)
> 
> Yeah, I poked Richard to fix it, and I'm trying to add the docs to
> developer.gnome.org as well, so we don't rely on non-automated sites for
> docs.

Yay, thanks a lot =)

[Bastien Nocera]

(In reply to Gustavo Noronha (kov) from comment #22)
> (In reply to Bastien Nocera from comment #21)
> > It's not. It was added in 0.99.5 (released July 2017), DisplayDevice was
> > added in 0.99.0 (released in 2013).
> 
> I think you're right about NORMAL, but DISCHARGING is from 2013 as well, no?
> 
> https://cgit.freedesktop.org/upower/commit/
> ?id=b446cac8f697fb1417f8d07190f95fa40b690260

Confusion! Anyway, the bug fix makes it possible to build against a 2013 upower, rather than a couple of months old one ;)
Thanks for the correction :)