Implement GPowerProfileMonitor for Android thermal and battery saver …#223
Open
maceip wants to merge 4 commits intoIgalia:mainfrom
Open
Implement GPowerProfileMonitor for Android thermal and battery saver …#223maceip wants to merge 4 commits intoIgalia:mainfrom
maceip wants to merge 4 commits intoIgalia:mainfrom
Conversation
…states - **GPowerProfileMonitor GIO extension** aggregating NDK `AThermalManager` (API 30+) thermal status and Java `PowerManager` battery saver broadcasts - **Runtime API detection** via `__builtin_available(android 30, *)` for thermal monitoring with graceful fallback on older devices - **Thread-safe state management** using `std::atomic<bool>` with `g_main_context_invoke()` marshaling to GLib main thread - **Android 13+ compatibility** with `RECEIVER_NOT_EXPORTED` flag for `BroadcastReceiver` registration - **wpe-platform-core code style** with camelCase GObject functions, proper `dispose`/`get_property` handlers, and `g_object_class_override_property()`
aperezdc
requested changes
Jan 27, 2026
Member
aperezdc
left a comment
aperezdc
left a comment
There was a problem hiding this comment.
While I appreciate the intention of providing an implementation for the power monitor, I think this needs quite some work:
- Naming is all over the place. Please see the comments.
- The code does not even build, partly because of the naming inconsistencies.
- The errors from the lint checker have not been taken into account, as shown by the failed CI job.
- After massaging it a little bit to get it to build, the code does not seem to even work as intended: the Java part works and tries to call into C++ to change the battery status, but registration with the GIO extension point does not seem to have any effect and the
WPEAndroidPowerProfileMonitornever gets instantiated, resulting in logged messages of the formWPEAndroidPowerProfileMonitor::set_battery_saver called before init, ignoring.
While I cannot be sure, overall this smells like an LLM hallucination, so @maceip please refrain from submitting patches that do not even build, and make sure that they at the very least can be built, pass the lint checks (which are part of the Git commit hooks, and should have been configured automatically in your working copy), and have been tested by hand. I will be happy to re-review if these basic guidelines are followed.
Thanks!
Comment on lines
+119
to
+122
| static void wpeAndroidPowerProfileMonitorIfaceInit(GPowerProfileMonitorInterface* iface) | ||
| { | ||
| // Interface implemented via "power-saver-enabled" property override | ||
| (void)iface; |
Member
There was a problem hiding this comment.
C++ allows omitting the argument variable name, so we can do this:
Suggested change
| static void wpeAndroidPowerProfileMonitorIfaceInit(GPowerProfileMonitorInterface* iface) | |
| { | |
| // Interface implemented via "power-saver-enabled" property override | |
| (void)iface; | |
| static void wpeAndroidPowerProfileMonitorIfaceInit(GPowerProfileMonitorInterface*) | |
| { | |
| // Interface implemented via "power-saver-enabled" property override |
| * mode is enabled, the monitor reports power-saver-enabled=TRUE to WebKit. | ||
| */ | ||
| struct _WPEAndroidPowerMonitor { | ||
| GObject parentInstance; |
Member
There was a problem hiding this comment.
The convention when defining a subclass using GObject is to use the name parent for the base/parent instance member. This is not super important because GObject does not enforce the naming, but it is good practice to follow the conventions 😉:
Suggested change
| GObject parentInstance; | |
| GObject parent; |
Comment on lines
+45
to
+46
| WPEAndroidPowerProfileMonitor::configureJNIMappings(); | ||
| WPEAndroidPowerProfileMonitor::registerExtension(); |
Member
There was a problem hiding this comment.
For the classes that may be registered using JNI we use the WK prefix, so this should be called WKPowerProfileMonitor.
| * When either thermal throttling is active (SEVERE or higher) or Battery Saver | ||
| * mode is enabled, the monitor reports power-saver-enabled=TRUE to WebKit. | ||
| */ | ||
| struct _WPEAndroidPowerMonitor { |
Member
There was a problem hiding this comment.
This should be _WPEAndroidPowerProfileMonitor, to match the naming in the rest of the file.
Suggested change
| struct _WPEAndroidPowerMonitor { | |
| struct _WPEAndroidPowerProfileMonitor { |
Comment on lines
+47
to
+48
| std::atomic<bool> isBatterySaverActive; | ||
| std::atomic<bool> isThermalThrottling; |
Member
There was a problem hiding this comment.
Using C++ types directly inside a struct allocated by GLib won't run constructors/destructors; the fact that this works in this case is by pure chance.
Either use an inner struct that gets initialized using placement-new, or use plain int values in combination with g_atomic_int_get() and g_atomic_int_set()
Comment on lines
+210
to
+216
| g_main_context_invoke( | ||
| nullptr, | ||
| +[](gpointer userData) -> gboolean { | ||
| updatePowerStateOnMainThread(WPE_ANDROID_POWER_PROFILE_MONITOR(userData)); | ||
| return G_SOURCE_REMOVE; | ||
| }, | ||
| s_singleton); |
Member
There was a problem hiding this comment.
This piece of code is repeated in three different places. I would put the call to g_main_context_invoke() inside the updatePowerStateOnMainThread() helper function to factor it out.
Then probably wpe_android_power_profile_monitor_set_battery_saver() can be removed.
Comment on lines
+70
to
+72
| // AThermalStatus values: | ||
| // ATHERMAL_STATUS_NONE = 0, LIGHT = 1, MODERATE = 2, SEVERE = 3, | ||
| // CRITICAL = 4, EMERGENCY = 5, SHUTDOWN = 6 |
Member
There was a problem hiding this comment.
This comment does not really add any interesting information, IMO it could be removed.
Comment on lines
+25
to
+40
| #define WPE_TYPE_ANDROID_POWER_PROFILE_MONITOR (wpe_android_power_profile_monitor_get_type()) | ||
| G_DECLARE_FINAL_TYPE( | ||
| WPEAndroidPowerMonitor, wpe_android_power_profile_monitor, WPE, ANDROID_POWER_PROFILE_MONITOR, GObject) | ||
|
|
||
| /** | ||
| * Updates the battery saver state from Java layer. | ||
| * Called via JNI when the battery saver mode changes. | ||
| */ | ||
| void wpe_android_power_profile_monitor_set_battery_saver(gboolean isPowerSaveMode); | ||
|
|
||
| /** | ||
| * Updates the thermal throttling state. | ||
| * Can be called from native code, though thermal status is typically | ||
| * monitored internally via AThermalManager. | ||
| */ | ||
| void wpe_android_power_profile_monitor_set_thermal_throttling(gboolean isThermalThrottling); |
Member
There was a problem hiding this comment.
This is all implementation details of the power profile monitor and do not need to be in the header.
| * sees "power-saver-enabled" as TRUE, causing reduced timer precision, stopped | ||
| * smooth animations, and throttled background tabs. | ||
| */ | ||
| class WPEAndroidPowerProfileMonitor { |
Member
There was a problem hiding this comment.
Suggested change
| class WPEAndroidPowerProfileMonitor { | |
| class WKPowerProfileMonitor { |
| Runtime/WKWebsiteDataManager.cpp | ||
| Runtime/WKWebView.cpp) | ||
| Runtime/WKWebView.cpp | ||
| Runtime/WPEAndroidPowerProfileMonitor.cpp) |
Member
There was a problem hiding this comment.
Suggested change
| Runtime/WPEAndroidPowerProfileMonitor.cpp) | |
| Runtime/WKPowerProfileMonitor.cpp) |
Author
|
Let me close this and clean it up and reopen
…On Tue, Jan 27, 2026, 11:29 PM Adrian Perez ***@***.***> wrote:
***@***.**** requested changes on this pull request.
While I appreciate the intention of providing an implementation for the
power monitor, I think this needs quite some work:
- Naming is all over the place. Please see the comments.
- The code does not even build, partly because of the naming
inconsistencies.
- The errors from the lint checker have not been taken into account,
as shown by the failed CI job.
- After massaging it a little bit to get it to build, the code does
not seem to even work as intended: the Java part works and tries to call
into C++ to change the battery status, but registration with the GIO
extension point does not seem to have any effect and the
WPEAndroidPowerProfileMonitor never gets instantiated, resulting in
logged messages of the form WPEAndroidPowerProfileMonitor::set_battery_saver
called before init, ignoring.
While I cannot be sure, overall this smells like an LLM hallucination, so
@maceip <https://github.com/maceip> please refrain from submitting
patches that do not even build, and make sure that they *at the very
least* can be built, pass the lint checks (which are part of the Git
commit hooks, and should have been configured automatically in your working
copy), and have been tested by hand. I will be happy to re-review if these
basic guidelines are followed.
Thanks!
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.cpp
<#223 (comment)>:
> +static void wpeAndroidPowerProfileMonitorIfaceInit(GPowerProfileMonitorInterface* iface)
+{
+ // Interface implemented via "power-saver-enabled" property override
+ (void)iface;
C++ allows omitting the argument variable name, so we can do this:
⬇️ Suggested change
-static void wpeAndroidPowerProfileMonitorIfaceInit(GPowerProfileMonitorInterface* iface)
-{
- // Interface implemented via "power-saver-enabled" property override
- (void)iface;
+static void wpeAndroidPowerProfileMonitorIfaceInit(GPowerProfileMonitorInterface*)
+{
+ // Interface implemented via "power-saver-enabled" property override
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.cpp
<#223 (comment)>:
> +
+// NDK Thermal API (API level 30+) - always include, use runtime detection
+#include <android/thermal.h>
+
+/**
+ * WPEAndroidPowerProfileMonitor implements GPowerProfileMonitor for Android.
+ *
+ * Aggregates two signals to determine "Low Power Mode":
+ * 1. Thermal Status (via NDK AThermalManager on API 30+)
+ * 2. Battery Saver Mode (via Java PowerManager broadcast)
+ *
+ * When either thermal throttling is active (SEVERE or higher) or Battery Saver
+ * mode is enabled, the monitor reports power-saver-enabled=TRUE to WebKit.
+ */
+struct _WPEAndroidPowerMonitor {
+ GObject parentInstance;
The convention when defining a subclass using GObject is to use the name
parent for the base/parent instance member. This is not super important
because GObject does not enforce the naming, but it is good practice to
follow the conventions 😉:
⬇️ Suggested change
- GObject parentInstance;
+ GObject parent;
------------------------------
In wpeview/src/main/cpp/Runtime/EntryPoint.cpp
<#223 (comment)>:
> + WPEAndroidPowerProfileMonitor::configureJNIMappings();
+ WPEAndroidPowerProfileMonitor::registerExtension();
For the classes that may be registered using JNI we use the WK prefix, so
this should be called WKPowerProfileMonitor.
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.cpp
<#223 (comment)>:
> +#include <gio/gio.h>
+
+// NDK Thermal API (API level 30+) - always include, use runtime detection
+#include <android/thermal.h>
+
+/**
+ * WPEAndroidPowerProfileMonitor implements GPowerProfileMonitor for Android.
+ *
+ * Aggregates two signals to determine "Low Power Mode":
+ * 1. Thermal Status (via NDK AThermalManager on API 30+)
+ * 2. Battery Saver Mode (via Java PowerManager broadcast)
+ *
+ * When either thermal throttling is active (SEVERE or higher) or Battery Saver
+ * mode is enabled, the monitor reports power-saver-enabled=TRUE to WebKit.
+ */
+struct _WPEAndroidPowerMonitor {
This should be _WPEAndroidPowerProfileMonitor, to match the naming in the
rest of the file.
⬇️ Suggested change
-struct _WPEAndroidPowerMonitor {
+struct _WPEAndroidPowerProfileMonitor {
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.cpp
<#223 (comment)>:
> + std::atomic<bool> isBatterySaverActive;
+ std::atomic<bool> isThermalThrottling;
Using C++ types directly inside a struct allocated by GLib won't run
constructors/destructors; the fact that this works in this case is by pure
chance.
Either use an inner struct that gets initialized using placement-new, or
use plain int values in combination with g_atomic_int_get() and
g_atomic_int_set()
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.cpp
<#223 (comment)>:
> +
+ AThermalManager* thermalManager;
+
+ // Thread-safe flags: NDK thermal callbacks occur on Binder threads,
+ // JNI calls may be on UI thread
+ std::atomic<bool> isBatterySaverActive;
+ std::atomic<bool> isThermalThrottling;
+};
+
+enum { PROP_0, PROP_POWER_SAVER_ENABLED, N_PROPERTIES };
+
+static WPEAndroidPowerMonitor* s_singleton = nullptr;
+
+static void wpeAndroidPowerProfileMonitorIfaceInit(GPowerProfileMonitorInterface* iface);
+
+G_DEFINE_FINAL_TYPE_WITH_CODE(WPEAndroidPowerMonitor, wpe_android_power_profile_monitor, G_TYPE_OBJECT,
As mentioned above, GObject has strong conventions for the naming, and
this does not build as-is, the type has to be named
WPEAndroidPowerProfileMonitor:
⬇️ Suggested change
-G_DEFINE_FINAL_TYPE_WITH_CODE(WPEAndroidPowerMonitor, wpe_android_power_profile_monitor, G_TYPE_OBJECT,
+G_DEFINE_FINAL_TYPE_WITH_CODE(WPEAndroidPowerProfileMonitor, wpe_android_power_profile_monitor, G_TYPE_OBJECT,
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.cpp
<#223 (comment)>:
> +void wpe_android_power_profile_monitor_set_thermal_throttling(gboolean isThermalThrottling)
+{
+ if (s_singleton == nullptr) {
+ Logging::logDebug("WPEAndroidPowerProfileMonitor::set_thermal_throttling called before init, ignoring");
+ return;
+ }
+
+ bool newValue = (isThermalThrottling != FALSE);
+ if (s_singleton->isThermalThrottling.load() != newValue) {
+ Logging::logDebug(
+ "WPEAndroidPowerProfileMonitor: Thermal throttling changed to %s", newValue ? "true" : "false");
+ s_singleton->isThermalThrottling.store(newValue);
+ g_main_context_invoke(
+ nullptr,
+ +[](gpointer userData) -> gboolean {
+ updatePowerStateOnMainThread(WPE_ANDROID_POWER_PROFILE_MONITOR(userData));
+ return G_SOURCE_REMOVE;
+ },
+ s_singleton);
+ }
+}
This function is not used anywhere and should be removed.
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.cpp
<#223 (comment)>:
> + g_main_context_invoke(
+ nullptr,
+ +[](gpointer userData) -> gboolean {
+ updatePowerStateOnMainThread(WPE_ANDROID_POWER_PROFILE_MONITOR(userData));
+ return G_SOURCE_REMOVE;
+ },
+ s_singleton);
This piece of code is repeated in three different places. I would put the
call to g_main_context_invoke() inside the updatePowerStateOnMainThread()
helper function to factor it out.
Then probably wpe_android_power_profile_monitor_set_battery_saver() can
be removed.
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.cpp
<#223 (comment)>:
> +// AThermalStatus values:
+// ATHERMAL_STATUS_NONE = 0, LIGHT = 1, MODERATE = 2, SEVERE = 3,
+// CRITICAL = 4, EMERGENCY = 5, SHUTDOWN = 6
This comment does not really add any interesting information, IMO it could
be removed.
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.h
<#223 (comment)>:
> +#define WPE_TYPE_ANDROID_POWER_PROFILE_MONITOR (wpe_android_power_profile_monitor_get_type())
+G_DECLARE_FINAL_TYPE(
+ WPEAndroidPowerMonitor, wpe_android_power_profile_monitor, WPE, ANDROID_POWER_PROFILE_MONITOR, GObject)
+
+/**
+ * Updates the battery saver state from Java layer.
+ * Called via JNI when the battery saver mode changes.
+ */
+void wpe_android_power_profile_monitor_set_battery_saver(gboolean isPowerSaveMode);
+
+/**
+ * Updates the thermal throttling state.
+ * Can be called from native code, though thermal status is typically
+ * monitored internally via AThermalManager.
+ */
+void wpe_android_power_profile_monitor_set_thermal_throttling(gboolean isThermalThrottling);
This is all implementation details of the power profile monitor and do not
need to be in the header.
------------------------------
In wpeview/src/main/cpp/Runtime/WPEAndroidPowerProfileMonitor.h
<#223 (comment)>:
> +void wpe_android_power_profile_monitor_set_thermal_throttling(gboolean isThermalThrottling);
+
+G_END_DECLS
+
+/**
+ * WPEAndroidPowerProfileMonitor implements GPowerProfileMonitor for Android.
+ *
+ * Aggregates two signals to determine "Low Power Mode":
+ * 1. Thermal Status (via NDK AThermalManager, API 30+)
+ * 2. Battery Saver Mode (via Java PowerManager broadcast)
+ *
+ * When either condition indicates power constraints, WebKit's LowPowerModeNotifierGLib
+ * sees "power-saver-enabled" as TRUE, causing reduced timer precision, stopped
+ * smooth animations, and throttled background tabs.
+ */
+class WPEAndroidPowerProfileMonitor {
⬇️ Suggested change
-class WPEAndroidPowerProfileMonitor {
+class WKPowerProfileMonitor {
------------------------------
In wpeview/src/main/cpp/CMakeLists.txt
<#223 (comment)>:
> @@ -132,7 +132,8 @@ add_library(
Runtime/WKWebContext.cpp
Runtime/WKSettings.cpp
Runtime/WKWebsiteDataManager.cpp
- Runtime/WKWebView.cpp)
+ Runtime/WKWebView.cpp
+ Runtime/WPEAndroidPowerProfileMonitor.cpp)
⬇️ Suggested change
- Runtime/WPEAndroidPowerProfileMonitor.cpp)
+ Runtime/WKPowerProfileMonitor.cpp)
—
Reply to this email directly, view it on GitHub
<#223 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGEMEGUON4FPL5XAXQ7EK34I7RGJAVCNFSM6AAAAACS2PNAO2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTOMJSGYYTMOBUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…states GPowerProfileMonitor GIO extension aggregating NDK AThermalManager (API 30+) thermal status and Java PowerManager battery saver broadcasts. - Runtime API detection via __builtin_available(android 30, *) for thermal monitoring with graceful fallback on older devices - Thread-safe state management using g_atomic_int_* for GLib compatibility - Memory-safe callbacks with proper g_object_ref/unref lifecycle - WKPowerProfileMonitor C++ wrapper with JNI mappings for Java integration When either thermal throttling is active (SEVERE or higher) or Battery Saver mode is enabled, the monitor reports power-saver-enabled=TRUE to WebKit, causing reduced timer precision, stopped smooth animations, and throttled background tabs. https://claude.ai/code/session_01QfbnP3t2TsjoSLJA37TtME
Implement GPowerProfileMonitor for Android thermal and battery saver …
Member
|
@maceip Right now we are busy trying to get the support for WPEPlatform merged in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AThermalManager(API 30+) thermal status and JavaPowerManagerbattery saver broadcasts__builtin_available(android 30, *)for thermal monitoring with graceful fallback on older devicesthis should be a draft -- my build env isnt up rn so its not passing local ci