WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
115719
[GTK] Add a UPower-based BatteryProvider
https://bugs.webkit.org/show_bug.cgi?id=115719
Summary
[GTK] Add a UPower-based BatteryProvider
Zan Dobersek
Reported
2013-05-07 02:55:44 PDT
[GTK] Add a UPower-based BatteryProvider
Attachments
Patch
(16.65 KB, patch)
2013-05-07 03:27 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(16.59 KB, patch)
2013-05-08 11:20 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(17.06 KB, patch)
2013-05-15 01:01 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(17.06 KB, patch)
2013-11-06 03:54 PST
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Rebased patch.
(17.09 KB, patch)
2013-12-09 07:31 PST
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Rebased patch.
(17.09 KB, patch)
2013-12-09 07:32 PST
,
Zan Dobersek
mrobinson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2013-05-07 03:27:59 PDT
Created
attachment 200885
[details]
Patch
WebKit Commit Bot
Comment 2
2013-05-07 03:29:58 PDT
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.
Zan Dobersek
Comment 3
2013-05-07 08:50:46 PDT
Here's the patch for the BatteryProviderUPower class.
Martin Robinson
Comment 4
2013-05-07 12:01:41 PDT
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.
Zan Dobersek
Comment 5
2013-05-07 12:18:36 PDT
(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
.
Zan Dobersek
Comment 6
2013-05-08 10:46:26 PDT
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.
Zan Dobersek
Comment 7
2013-05-08 10:51:54 PDT
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.
Zan Dobersek
Comment 8
2013-05-08 11:20:30 PDT
Created
attachment 201084
[details]
Patch
WebKit Commit Bot
Comment 9
2013-05-08 11:22:31 PDT
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.
Benjamin Poulain
Comment 10
2013-05-09 02:15:49 PDT
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.
Zan Dobersek
Comment 11
2013-05-09 07:37:24 PDT
(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
.
Zan Dobersek
Comment 12
2013-05-09 08:03:03 PDT
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
Zan Dobersek
Comment 13
2013-05-15 01:01:09 PDT
Created
attachment 201800
[details]
Patch
WebKit Commit Bot
Comment 14
2013-05-15 01:03:31 PDT
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.
Zan Dobersek
Comment 15
2013-05-15 01:12:59 PDT
(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.
Anders Carlsson
Comment 16
2013-10-17 10:09:30 PDT
I don't like that this adds yet another platform subdirectory. Can't this go inside platform/glib or platform/gtk or something?
Martin Robinson
Comment 17
2013-10-17 10:25:18 PDT
GLib probably makes sense since this uses UPower GLib. In the long term, having a single directory for Linuxy/FreeDesktop systems is probably sensible.
Zan Dobersek
Comment 18
2013-11-06 03:54:41 PST
Created
attachment 216160
[details]
Patch Rebased patch, with the UPower battery provider files moved under Source/WebCore/platform/glib/.
Zan Dobersek
Comment 19
2013-12-09 07:31:13 PST
Created
attachment 218758
[details]
Rebased patch.
Zan Dobersek
Comment 20
2013-12-09 07:32:34 PST
Created
attachment 218759
[details]
Rebased patch.
Martin Robinson
Comment 21
2013-12-11 06:27:21 PST
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.
Zan Dobersek
Comment 22
2013-12-11 10:01:31 PST
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.
Zan Dobersek
Comment 23
2013-12-11 10:13:47 PST
Committed
r160444
: <
http://trac.webkit.org/changeset/160444
>
Bastien Nocera
Comment 24
2013-12-13 03:56:15 PST
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug