Bug 115719

Summary: [GTK] Add a UPower-based BatteryProvider
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, bugzilla, commit-queue, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 115718    
Bug Blocks: 95582, 115720, 125453    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Rebased patch.
none
Rebased patch. mrobinson: review+

Description Zan Dobersek 2013-05-07 02:55:44 PDT
[GTK] Add a UPower-based BatteryProvider
Comment 1 Zan Dobersek 2013-05-07 03:27:59 PDT
Created attachment 200885 [details]
Patch
Comment 2 WebKit Commit Bot 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.
Comment 3 Zan Dobersek 2013-05-07 08:50:46 PDT
Here's the patch for the BatteryProviderUPower class.
Comment 4 Martin Robinson 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.
Comment 5 Zan Dobersek 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.
Comment 6 Zan Dobersek 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.
Comment 7 Zan Dobersek 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.
Comment 8 Zan Dobersek 2013-05-08 11:20:30 PDT
Created attachment 201084 [details]
Patch
Comment 9 WebKit Commit Bot 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.
Comment 10 Benjamin Poulain 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.
Comment 11 Zan Dobersek 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.
Comment 12 Zan Dobersek 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
Comment 13 Zan Dobersek 2013-05-15 01:01:09 PDT
Created attachment 201800 [details]
Patch
Comment 14 WebKit Commit Bot 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.
Comment 15 Zan Dobersek 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.
Comment 16 Anders Carlsson 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?
Comment 17 Martin Robinson 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.
Comment 18 Zan Dobersek 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/.
Comment 19 Zan Dobersek 2013-12-09 07:31:13 PST
Created attachment 218758 [details]
Rebased patch.
Comment 20 Zan Dobersek 2013-12-09 07:32:34 PST
Created attachment 218759 [details]
Rebased patch.
Comment 21 Martin Robinson 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.
Comment 22 Zan Dobersek 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.
Comment 23 Zan Dobersek 2013-12-11 10:13:47 PST
Committed r160444: <http://trac.webkit.org/changeset/160444>
Comment 24 Bastien Nocera 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