Summary: | [GLib] Let WebCore know of low power situations | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gustavo Noronha (kov) <gustavo> | ||||||||
Component: | WebKitGTK | Assignee: | Gustavo Noronha (kov) <gustavo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bugs-noreply, bugzilla, cgarcia, clopez, mcatanzaro | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 181825 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Gustavo Noronha (kov)
2017-10-03 06:14:16 PDT
Created attachment 322513 [details]
Patch
Need libupower-glib-dev to be installed on the bots 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 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. Created attachment 322520 [details]
Patch
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. Created attachment 322532 [details]
Patch
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? (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 =( Committed r222834: <http://trac.webkit.org/changeset/222834> (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 (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 Reopening. Will you handle this, Gustavo? There's a patch for better documentation here: https://bugs.freedesktop.org/show_bug.cgi?id=103361 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? (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. 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/ (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 Committed r223954: <https://trac.webkit.org/changeset/223954> (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 =) (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. (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 =) (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 :) |