[GTK] Add a UPower-based BatteryProvider
Created attachment 200885 [details] Patch
Attachment 200885 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/GNUmakefile.am', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/upower/BatteryProviderUPower.cpp', u'Source/WebCore/platform/upower/BatteryProviderUPower.h', u'Source/WebCore/platform/upower/BatteryProviderUPowerClient.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am']" exit_code: 1 Source/WebCore/platform/upower/BatteryProviderUPower.cpp:104: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Here's the patch for the BatteryProviderUPower class.
Comment on attachment 200885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200885&action=review The UPower bits look okay to me. I'm a bit confused how this hooks into WebCore since both the provider and client are unused. Is this supposed to hook into the WebKit layer somehow? Couple nitty comments follow. > Source/WebCore/platform/upower/BatteryProviderUPower.cpp:71 > + g_signal_handlers_disconnect_by_func(m_upowerClient.get(), reinterpret_cast<void*>(powerDeviceAlterationCallback), this); > + m_upowerClient.clear(); Destroying the client should make disconnecting the functions unnecessary here, I think. > Source/WebCore/platform/upower/BatteryProviderUPower.cpp:90 > + UpDevice* device = reinterpret_cast<UpDevice*>(g_ptr_array_index(devices, i)); A static cast should be sufficient, I think. > Source/WebCore/platform/upower/BatteryProviderUPower.cpp:111 > + // energyRate should be signed according to the charging/discharging state. > + energyRate = std::abs(energyRate); > + energyRate = deviceState == UP_DEVICE_STATE_DISCHARGING ? -energyRate : energyRate; Why is the sign of the energy rate from UPower unreliable? Could a device be charging, but still losing energy? For instance, if power is being supplied by USB, but the discharge rate is higher? > Source/WebCore/platform/upower/BatteryProviderUPower.cpp:117 > + combinedDeviceState |= (1 << deviceState); combinedDeviceState appears to be unused and I'm not sure I understand what's going on here. > Source/WebCore/platform/upower/BatteryProviderUPower.cpp:130 > + if (combinedEnergyRate >= 0) { How about doing this? double level = combinedEnergyCapacityFull > 0 ? combinedEnergyCapacityCurrent / combinedEnergyCapacityFull : 1; if (combinedEnergyRate >= 0) { m_client->notifyBatteryStatusUpdated(true /* charging */, 3600.0 * (combinedEnergyCapacityFull - combinedEnergyCapacityCurrent) / combinedEnergyRate, std::numeric_limits<souble>::infinity(), level); return; } m_client->notifyBatteryStatusUpdated(false /* charging */, std::numeric_limits<souble>::infinity(), 3600 * combinedEnergyCapacityCurrent / std::abs(combinedEnergyRate), level); > Source/WebCore/platform/upower/BatteryProviderUPower.cpp:132 > + chargingTime = 3600.0 * (combinedEnergyCapacityFull - combinedEnergyCapacityCurrent) / combinedEnergyRate; I don't .0 is necessary here to force floating point math.
(In reply to comment #4) > (From update of attachment 200885 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200885&action=review > > The UPower bits look okay to me. I'm a bit confused how this hooks into WebCore since both the provider and client are unused. Is this supposed to hook into the WebKit layer somehow? Couple nitty comments follow. Yes, BatteryProviderUPower is used in the WebKit layer. In WebKit1, a BatteryClientGtk is required but its implementation is left empty so the feature wouldn't be available there. I think this is in the general spirit of WebKit1 being in maintenance mode, even though the implementation is quite simple to do. BatteryClientGtk is covered in bug #115628. In WebKit2, the WebKitBatteryProvider, located in the UIProcess, covers handling the BatteryProviderUPower instance. WebKitBatteryProvider also serves as the client, relaying any status updates to the WebProcess where the WebBatteryClient class passes along the updates to the WebCore battery module. WebKitBatteryProvider is covered in bug #115720.
Comment on attachment 200885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200885&action=review >> Source/WebCore/platform/upower/BatteryProviderUPower.cpp:71 >> + m_upowerClient.clear(); > > Destroying the client should make disconnecting the functions unnecessary here, I think. Will remove. >> Source/WebCore/platform/upower/BatteryProviderUPower.cpp:90 >> + UpDevice* device = reinterpret_cast<UpDevice*>(g_ptr_array_index(devices, i)); > > A static cast should be sufficient, I think. True. >> Source/WebCore/platform/upower/BatteryProviderUPower.cpp:111 >> + energyRate = deviceState == UP_DEVICE_STATE_DISCHARGING ? -energyRate : energyRate; > > Why is the sign of the energy rate from UPower unreliable? Could a device be charging, but still losing energy? For instance, if power is being supplied by USB, but the discharge rate is higher? The DBus energy rate documentation tells that the energy rate value is positive if the source is discharging and negative if charging. http://upower.freedesktop.org/docs/Device.html#Device:EnergyRate The glib wrapper for the device interface specifies the energy rate property with value between 0 and G_MAXDOUBLE. http://cgit.freedesktop.org/upower/tree/libupower-glib/up-device.c#n1234 Given the latter fact I guess the std::abs call can be removed. OTOH, the issue here is either in the device DBus interface documentation or the glib wrapper. >> Source/WebCore/platform/upower/BatteryProviderUPower.cpp:117 >> + combinedDeviceState |= (1 << deviceState); > > combinedDeviceState appears to be unused and I'm not sure I understand what's going on here. This is a leftover from a previous approach to determining the charging state. >> Source/WebCore/platform/upower/BatteryProviderUPower.cpp:130 >> + if (combinedEnergyRate >= 0) { > > How about doing this? > > double level = combinedEnergyCapacityFull > 0 ? combinedEnergyCapacityCurrent / combinedEnergyCapacityFull : 1; > if (combinedEnergyRate >= 0) { > m_client->notifyBatteryStatusUpdated(true /* charging */, > 3600.0 * (combinedEnergyCapacityFull - combinedEnergyCapacityCurrent) / combinedEnergyRate, > std::numeric_limits<souble>::infinity(), level); > return; > } > > m_client->notifyBatteryStatusUpdated(false /* charging */, > std::numeric_limits<souble>::infinity(), > 3600 * combinedEnergyCapacityCurrent / std::abs(combinedEnergyRate), > level); Works as well. >> Source/WebCore/platform/upower/BatteryProviderUPower.cpp:132 >> + chargingTime = 3600.0 * (combinedEnergyCapacityFull - combinedEnergyCapacityCurrent) / combinedEnergyRate; > > I don't .0 is necessary here to force floating point math. No, it's not.
Comment on attachment 200885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200885&action=review >>> Source/WebCore/platform/upower/BatteryProviderUPower.cpp:111 >>> + energyRate = deviceState == UP_DEVICE_STATE_DISCHARGING ? -energyRate : energyRate; >> >> Why is the sign of the energy rate from UPower unreliable? Could a device be charging, but still losing energy? For instance, if power is being supplied by USB, but the discharge rate is higher? > > The DBus energy rate documentation tells that the energy rate value is positive if the source is discharging and negative if charging. > http://upower.freedesktop.org/docs/Device.html#Device:EnergyRate > > The glib wrapper for the device interface specifies the energy rate property with value between 0 and G_MAXDOUBLE. > http://cgit.freedesktop.org/upower/tree/libupower-glib/up-device.c#n1234 > > Given the latter fact I guess the std::abs call can be removed. OTOH, the issue here is either in the device DBus interface documentation or the glib wrapper. Testing the UPower.Device DBus interface through dbus-send shows that the rate is positive when both charging and discharging, so I believe that the documentation is wrong here.
Created attachment 201084 [details] Patch
Attachment 201084 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/GNUmakefile.am', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/upower/BatteryProviderUPower.cpp', u'Source/WebCore/platform/upower/BatteryProviderUPower.h', u'Source/WebCore/platform/upower/BatteryProviderUPowerClient.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am']" exit_code: 1 Source/WebCore/platform/upower/BatteryProviderUPower.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/upower/BatteryProviderUPower.cpp:105: Use 0 instead of NULL. [readability/null] [5] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 201084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201084&action=review From a quick look, it seems this code does nothing. BatteryProviderUPowerClient has no concrete implementations. I do not see BatteryProviderUPower instantiated anywhere. I am not very excited to see this land in WebCore/platform. > Source/WebCore/platform/upower/BatteryProviderUPower.cpp:135 > + double chargingTime = std::numeric_limits<double>::infinity(); > + if (combinedEnergyRate) > + chargingTime = 3600 * (combinedEnergyCapacityFull - combinedEnergyCapacityCurrent) / combinedEnergyRate; > + m_client->notifyBatteryStatusUpdated(true, chargingTime, 0, level); > + } else { > + double dischargingTime = 3600 * combinedEnergyCapacityCurrent / std::abs(combinedEnergyRate); > + m_client->notifyBatteryStatusUpdated(false, 0, dischargingTime, level); Once side of the branch protects again dividing by zero, the other does not. I would be careful about using infinity (or even NaN) as part of an interface. That generally leads to disasters. > Source/WebCore/platform/upower/BatteryProviderUPowerClient.h:30 > + virtual void notifyBatteryStatusUpdated(bool charging, double chargingTime, double dischargingTime, double level) = 0; This is a weird API. You pass both chargingTime and dischargingTime simultaneously but only one of the two make sense at any point. It would make more sense to have two callback, one for charging, the other for discharge.
(In reply to comment #10) > From a quick look, it seems this code does nothing. BatteryProviderUPowerClient has no concrete implementations. I do not see BatteryProviderUPower instantiated anywhere. The WebKitBatteryProvider class does so, currently up for reviewing in bug #115720.
Comment on attachment 201084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201084&action=review >> Source/WebCore/platform/upower/BatteryProviderUPower.cpp:135 >> + m_client->notifyBatteryStatusUpdated(false, 0, dischargingTime, level); > > Once side of the branch protects again dividing by zero, the other does not. > > I would be careful about using infinity (or even NaN) as part of an interface. That generally leads to disasters. The fist branch covers the positive-or-zero values of the combinedEnergyRate variable, the second the negative ones, so the possible zero value of the combinedEnergyRate needs to be addressed only in the first branch. >> Source/WebCore/platform/upower/BatteryProviderUPowerClient.h:30 >> + virtual void notifyBatteryStatusUpdated(bool charging, double chargingTime, double dischargingTime, double level) = 0; > > This is a weird API. You pass both chargingTime and dischargingTime simultaneously but only one of the two make sense at any point. > > It would make more sense to have two callback, one for charging, the other for discharge. The notifyBatteryStatusUpdated signature follows the BatteryStatus constructor signature[1] and parameter values passed to it reflect the ones the specification[2] suggests. [1] http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/battery/BatteryStatus.h#L30 [2] http://www.w3.org/TR/battery-status/#attributes-1
Created attachment 201800 [details] Patch
Attachment 201800 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/GNUmakefile.am', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/upower/BatteryProviderUPower.cpp', u'Source/WebCore/platform/upower/BatteryProviderUPower.h', u'Source/WebCore/platform/upower/BatteryProviderUPowerClient.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am']" exit_code: 1 Source/WebCore/platform/upower/BatteryProviderUPower.cpp:103: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #13) > Created an attachment (id=201800) [details] > Patch This patch simplifies the BatteryProviderUPowerClient interface, only providing one method that can be used to report both unavailability of the battery status and the valid charging and discharging states. In the first case, only the NotAvailable BatteryProvideUPowerStatus has to be used as the first parameter of the client's updateBatteryStatus call, the other two parameters have no real meaning when the battery status is not available. In the second case, the first parameter notes the battery is either charging or discharging, with the second parameter being the time left until the battery is fully charged or completely depleted, depending on the status. The third parameter represents the current battery level.
I don't like that this adds yet another platform subdirectory. Can't this go inside platform/glib or platform/gtk or something?
GLib probably makes sense since this uses UPower GLib. In the long term, having a single directory for Linuxy/FreeDesktop systems is probably sensible.
Created attachment 216160 [details] Patch Rebased patch, with the UPower battery provider files moved under Source/WebCore/platform/glib/.
Created attachment 218758 [details] Rebased patch.
Created attachment 218759 [details] Rebased patch.
Comment on attachment 218759 [details] Rebased patch. View in context: https://bugs.webkit.org/attachment.cgi?id=218759&action=review > Source/WebCore/platform/glib/BatteryProviderUPower.cpp:92 > + bool isPresent; This should be gboolean. > Source/WebCore/platform/glib/BatteryProviderUPowerClient.h:35 > + virtual void updateBatteryStatus(BatteryProviderUPowerStatus, double timeRemaining = 0, double level = 0) = 0; I'd prefer secondsRemaining if that's the unit here. timeRemaining doesn't make it clear what the unit of the value is. Likewise level might be better termed fillRatio or something similar.
Comment on attachment 218759 [details] Rebased patch. View in context: https://bugs.webkit.org/attachment.cgi?id=218759&action=review >> Source/WebCore/platform/glib/BatteryProviderUPowerClient.h:35 >> + virtual void updateBatteryStatus(BatteryProviderUPowerStatus, double timeRemaining = 0, double level = 0) = 0; > > I'd prefer secondsRemaining if that's the unit here. timeRemaining doesn't make it clear what the unit of the value is. Likewise level might be better termed fillRatio or something similar. 'secondsRemaining' is better. I'll rename 'level' to 'batteryLevel' to explicitly tie it to that concept. To me, 'fillRatio' is a bit unrelated to battery information.
Committed r160444: <http://trac.webkit.org/changeset/160444>
Why doesn't this patch use the new UPower API, in particular the /org/freedesktop/UPower/devices/DisplayDevice object that would avoid doing the sketchy things (combinedEnergyRate...) you do in updateBatteryStatus(). Compare your version with the UPower version: http://cgit.freedesktop.org/upower/tree/src/up-daemon.c#n182