Implement device::BatteryStatus support for UPower daemon 0.99.x

Implement device::BatteryStatus support for UPower daemon 0.99.x

The org.freedesktop.UPower API was changed when upgrading the UPower daemon
from version 0.9.23 to 0.99.x. The BatteryStatusManagerLinux used the
"Changed" signal which was replaced by the "PropertyChanged" signal.

Change the BatteryStatusManagerLinux to use the new UPower API
(and keep compatibility with the 0.9.23 version, which is still used in
Ubuntu 14.04 LTS).

1. use dbus::PropertySet: the dbus::PropertySet provides simple access to
the properties and connection to the property-changed notifications.

2. Use UPower method GetDisplayDevice:
The 'DisplayDevice' is a composite battery device. That was added in UPower
version 0.99.0. If we don't get that device or if it is no battery, then we
continue to enumerate all devices.

3. Listen to 'DeviceAdded' and 'DeviceRemoved' signals:
Re-enumerate battery devices if a device is added/removed.

4. Compatibility with UPower version < 0.99
Only old UPower versions need to connect to the 'Changed' signal.

5. Rewrite the existing unittests to use a BatteryStatusManagerLinux instance
with a dbus::MockBus and mock the dbus-methods/properties for the test.
Add more unittests:
- for changing device properties
- for the DisplayDevice
- for enumerating devices
- for the DeviceAdded and DeviceRemoved signals

Committed: https://crrev.com/472a020d5783be9433678edac28953e24e1f5922
Cr-Commit-Position: refs/heads/master@{#413745}

[markuso, 2016-06-13 12:37:34]

Description was changed from

==========
Implement device::BatteryStatus support for UPower daemon 0.99.x

The org.freedesktop.UPower API was changed when upgrading the UPower daemon
from version 0.9.23 to 0.99.x. The BatteryStatusManagerLinux used the
"Changed" signal which was replaced by the "PropertyChanged" signal.

Change the BatteryStatusManagerLinux to use the new UPower API
(and keep compatibility with the 0.9.23 version, which is still used in
Ubuntu 14.04 LTS).

1. use dbus::PropertySet: the dbus::PropertySet provides simple access to
the properties and connection to the property-changed notifications.

2. Use UPower method GetDisplayDevice:
The 'DisplayDevice' is a composite battery device. That was added in UPower
version 0.99.0. If we don't get that device or if it is no battery, then we
continue to enumerate all devices.

3. Listen to 'DeviceAdded' and 'DeviceRemoved' signals:
Re-enumerate battery devices if a device is added/removed.

4. Compatibility with UPower version < 0.99
Only old UPower versions need to connect to the 'Changed' signal.

5. Rewrite the existing unittests to use a BatteryStatusManagerLinux instance
with a dbus::MockBus and mock the dbus-methods/properties for the test.
Add more unittests:
- for changing device properties
- for the DisplayDevice
- for enumerating devices
- for the DeviceAdded and DeviceRemoved signals

BUG=
==========

to

==========
Implement device::BatteryStatus support for UPower daemon 0.99.x

The org.freedesktop.UPower API was changed when upgrading the UPower daemon
from version 0.9.23 to 0.99.x. The BatteryStatusManagerLinux used the
"Changed" signal which was replaced by the "PropertyChanged" signal.

Change the BatteryStatusManagerLinux to use the new UPower API
(and keep compatibility with the 0.9.23 version, which is still used in
Ubuntu 14.04 LTS).

1. use dbus::PropertySet: the dbus::PropertySet provides simple access to
the properties and connection to the property-changed notifications.

2. Use UPower method GetDisplayDevice:
The 'DisplayDevice' is a composite battery device. That was added in UPower
version 0.99.0. If we don't get that device or if it is no battery, then we
continue to enumerate all devices.

3. Listen to 'DeviceAdded' and 'DeviceRemoved' signals:
Re-enumerate battery devices if a device is added/removed.

4. Compatibility with UPower version < 0.99
Only old UPower versions need to connect to the 'Changed' signal.

5. Rewrite the existing unittests to use a BatteryStatusManagerLinux instance
with a dbus::MockBus and mock the dbus-methods/properties for the test.
Add more unittests:
- for changing device properties
- for the DisplayDevice
- for enumerating devices
- for the DeviceAdded and DeviceRemoved signals
==========

[markuso, 2016-06-13 12:37:34]

markuso@opera.com changed reviewers:
+ timvolodine@chromium.org

[markuso, 2016-06-13 12:40:32]

@reviewer: does this patch look OK to you?

I tested this both on Ubuntu 16.04, which uses UPower daemon version 0.99.xx and
on 14.04 LTS which uses the old UPower daemon version 0.9.23.

[timvolodine, 2016-06-16 16:12:24]

@markuso:
From my first scan looks reasonable to me see comments below.
It's quite a rewrite, thanks for keeping this code up-to-date!

Also, could you kick-off the try-bots please to see that nothing fails..

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
File device/battery/battery_status_manager_linux.cc (right):

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:55: base::Version
daemon_version();
const?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:401: dbus::ObjectPath
display_device_path = upower_->GetDisplayDevice();
is this some kind of virtual 'battery' instance?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:440: 
de we need hook-up 'DeviceChanged'/'PropertiesChanged' signals here?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:475: bool success) {}
is this empty intentionally? in the previous implementation the battery info is
updated once connected, is this the case here as well?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:489: FindBatteryDevice();
but it will always pick the first one it encounters? also, if I am looking
correctly seems like it won't execute the for loop since battery_!=null ?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:500: FindBatteryDevice();
same question here

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
File device/battery/battery_status_manager_linux_unittest.cc (right):

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:31: const char
kUPowerInterfaceName[] = "org.freedesktop.UPower";
A number of constants here duplicate similar constants in the
battery_status_manager_linux.cc file. Any way we could avoid these duplicates?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:153: LOG(ERROR) <<
"Unexpected method call: " << method_call->ToString();
should this be a FAIL() <<.. ?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:273: LOG(ERROR) <<
"Unexpected method call: " << method_call->ToString();
same question here

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:1233: 
nit: maybe an idea for extra test (which doesn't seem to be really covered):
when there is no battery but other devices present.

[markuso, 2016-06-17 08:44:28]

> Also, could you kick-off the try-bots please to see that nothing fails..

It seems I don't have privileges to start a try job:

$ git cl try
Loaded default bots from CQ config (chromium/src/infra/config/cq.cfg)
ERROR: Access denied: User user:markuso@opera.com cannot add builds to bucket
master.tryserver.chromium.android

[markuso, 2016-06-17 08:45:20]

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
File device/battery/battery_status_manager_linux.cc (right):

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:55: base::Version
daemon_version();
On 2016/06/16 16:12:24, timvolodine wrote:
> const?

It is not const, because it calls dbus::Property::GetAndBlock(). That is not a
const method.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:401: dbus::ObjectPath
display_device_path = upower_->GetDisplayDevice();
On 2016/06/16 16:12:24, timvolodine wrote:
> is this some kind of virtual 'battery' instance?

Yes it is a composite battery (so it should resolve your TODO about adding
support for multiple batteries - if only we know that it is always available).
It was added in UPower version 0.99.0.

It doesn't need to be a battery. In my desktop PC it is basically an empty
device.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:440: 
On 2016/06/16 16:12:24, timvolodine wrote:
> de we need hook-up 'DeviceChanged'/'PropertiesChanged' signals here?

We are already receiving 'PropertiesChanged' signal through the
BatteryProperties (dbus::PropertySet) - that is one of the reasons I started to
use the dbus::PropertySet.

The 'DeviceChanged' signal was removed in 0.99.0, we could hook up to the signal
in the block below (together with the 'Changed' signal, which was also removed
in 0.99.0).
I didn't do that, because the original code didn't.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:475: bool success) {}
On 2016/06/16 16:12:24, timvolodine wrote:
> is this empty intentionally? in the previous implementation the battery info
is
> updated once connected, is this the case here as well?

In this case we update the battery info once we are finished with the iteration,
so there is no need to do anything asynchronously.

I'm not sure if you had any special reason to wait for the notification until we
are connected. I suspect it was done to allow dbus time to establish the
connection and that there was no point in trying to get the status-information
earlier.

Since we use the dbus::Property and dbus::Property::GetAndBlock(),
dbus::Property takes care about that and we block until we have all properties.
So I don't think there is need to add another asynchronous layer.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:489: FindBatteryDevice();
On 2016/06/16 16:12:24, timvolodine wrote:
> but it will always pick the first one it encounters? also, if I am looking
> correctly seems like it won't execute the for loop since battery_!=null ?

Yes, it will always pick the first battery, either the DisplayDevice or the
first from the enumeration. That is the same logic as before, with the exception
of the DisplayDevice and now we can update the chosen battery if the user
adds/removes one.

FindBatteryDevice() first moves battery_ to a local variable (current, line
392). So battery_ will be empty and we enter the loop.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:500: FindBatteryDevice();
On 2016/06/16 16:12:24, timvolodine wrote:
> same question here

Same answer ;)

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
File device/battery/battery_status_manager_linux_unittest.cc (right):

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:31: const char
kUPowerInterfaceName[] = "org.freedesktop.UPower";
On 2016/06/16 16:12:24, timvolodine wrote:
> A number of constants here duplicate similar constants in the
> battery_status_manager_linux.cc file. Any way we could avoid these duplicates?

I could move them to the battery_status_manager_linux.h file.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:153: LOG(ERROR) <<
"Unexpected method call: " << method_call->ToString();
On 2016/06/16 16:12:24, timvolodine wrote:
> should this be a FAIL() <<.. ?

I followed the guide of the Google Mock CookBook
(https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md)
which suggest to use NiceMock and not fail if you encounter an unknown method
call. So here I only print a warning that something unexpected happens.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:273: LOG(ERROR) <<
"Unexpected method call: " << method_call->ToString();
On 2016/06/16 16:12:24, timvolodine wrote:
> same question here

Same answer

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:1233: 
On 2016/06/16 16:12:24, timvolodine wrote:
> nit: maybe an idea for extra test (which doesn't seem to be really covered):
> when there is no battery but other devices present.

True, there is no test for that.

There is "NoBattery" when no device is present, there is "SkipNonBatteryDevice"
to skip a device and find a battery afterwards. There is
"DisplayDeviceNoBattery" to skip the DisplayDevice if it is no battery.
So at least we already cover all steps that would be executed if there were
devices, but none of them a battery.

[markuso, 2016-06-17 10:01:17]

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
File device/battery/battery_status_manager_linux_unittest.cc (right):

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:31: const char
kUPowerInterfaceName[] = "org.freedesktop.UPower";
On 2016/06/17 08:45:19, markuso wrote:
> On 2016/06/16 16:12:24, timvolodine wrote:
> > A number of constants here duplicate similar constants in the
> > battery_status_manager_linux.cc file. Any way we could avoid these
duplicates?
> 
> I could move them to the battery_status_manager_linux.h file.

Done.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:1233: 
With the added test "OldDaemonSignalDeviceChanged" there is now such a part
included: first there is one device that is no battery. In a second step the
device is changed to type battery.

[markuso, 2016-07-01 08:42:08]

I think I have either answered all questions or provided a fix for the issues
you mentioned.
The trybot results look OK.

Are there any other issues I should address?

[timvolodine, 2016-07-05 16:21:07]

yes thanks markuso@, appreciate your work! sorry for the delay, got pulled into
other things..

overall looks ok to me, just a few questions below.
did you get the chance to test it on a linux laptop (with a battery)?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
File device/battery/battery_status_manager_linux.cc (right):

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:475: bool success) {}
On 2016/06/17 08:45:19, markuso wrote:
> On 2016/06/16 16:12:24, timvolodine wrote:
> > is this empty intentionally? in the previous implementation the battery info
> is
> > updated once connected, is this the case here as well?
> 
> In this case we update the battery info once we are finished with the
iteration,
> so there is no need to do anything asynchronously.
> 
> I'm not sure if you had any special reason to wait for the notification until
we
> are connected. I suspect it was done to allow dbus time to establish the
> connection and that there was no point in trying to get the status-information
> earlier.
> 
> Since we use the dbus::Property and dbus::Property::GetAndBlock(),
> dbus::Property takes care about that and we block until we have all
properties.
> So I don't think there is need to add another asynchronous layer.

I see, don't remember exactly why it was originally onSignalConnected, but your
suspicion sounds right ;)

synchronous is ok as long as it is on a dedicated thread, which I think it is
right?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:489: FindBatteryDevice();
On 2016/06/17 08:45:19, markuso wrote:
> On 2016/06/16 16:12:24, timvolodine wrote:
> > but it will always pick the first one it encounters? also, if I am looking
> > correctly seems like it won't execute the for loop since battery_!=null ?
> 
> Yes, it will always pick the first battery, either the DisplayDevice or the
> first from the enumeration. That is the same logic as before, with the
exception
> of the DisplayDevice and now we can update the chosen battery if the user
> adds/removes one.
> 
> FindBatteryDevice() first moves battery_ to a local variable (current, line
> 392). So battery_ will be empty and we enter the loop.

ok, could you please add a comment in FindBatteryDevice() with this explanation?

in line 392: doesn't the std::move(battery_) leave battery_ in an unspecified
state?

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
File device/battery/battery_status_manager_linux_unittest.cc (right):

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux_unittest.cc:1233: 
On 2016/06/17 10:01:17, markuso wrote:
> With the added test "OldDaemonSignalDeviceChanged" there is now such a part
> included: first there is one device that is no battery. In a second step the
> device is changed to type battery.

yes, thanks, just thought that the 'no battery' case would be quite common, e.g.
on desktops.

[timvolodine, 2016-07-05 16:26:39]

timvolodine@chromium.org changed reviewers:
+ rockot@chromium.org

[timvolodine, 2016-07-05 16:26:39]

also given the size of this patch maybe good if a second reviewer took a brief
look: rockot@?

[Ken Rockot(use gerrit already), 2016-07-10 14:57:15]

lgtm just some nits

I didn't thoroughly review all the logic in the tests

https://codereview.chromium.org/2066503002/diff/60001/device/battery/battery_...
File device/battery/battery_status_manager_linux-inl.h (right):

https://codereview.chromium.org/2066503002/diff/60001/device/battery/battery_...
device/battery/battery_status_manager_linux-inl.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
I don't see a good reason to add this file. Please just declare the constants in
battery_status_manager_linux.h and define them in the cc.

https://codereview.chromium.org/2066503002/diff/60001/device/battery/battery_...
File device/battery/battery_status_manager_linux.h (right):

https://codereview.chromium.org/2066503002/diff/60001/device/battery/battery_...
device/battery/battery_status_manager_linux.h:13: }  // namespace dbus
nit: no need for the closing comment here

[markuso, 2016-07-21 12:38:12]

On 2016/07/05 16:21:07, timvolodine wrote:
> did you get the chance to test it on a linux laptop (with a battery)?

Yes, I did - on one with Ubuntu 16.04 which has the new UPower version (0.99.xx)
and on another laptop with some older linux version which uses UPower 0.9.xx
with the old API.

[markuso, 2016-07-21 12:39:31]

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
File device/battery/battery_status_manager_linux.cc (right):

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:475: bool success) {}
On 2016/07/05 16:21:07, timvolodine wrote:
> On 2016/06/17 08:45:19, markuso wrote:
> > On 2016/06/16 16:12:24, timvolodine wrote:
> > > is this empty intentionally? in the previous implementation the battery
info
> > is
> > > updated once connected, is this the case here as well?
> > 
> > In this case we update the battery info once we are finished with the
> iteration,
> > so there is no need to do anything asynchronously.
> > 
> > I'm not sure if you had any special reason to wait for the notification
until
> we
> > are connected. I suspect it was done to allow dbus time to establish the
> > connection and that there was no point in trying to get the
status-information
> > earlier.
> > 
> > Since we use the dbus::Property and dbus::Property::GetAndBlock(),
> > dbus::Property takes care about that and we block until we have all
> properties.
> > So I don't think there is need to add another asynchronous layer.
> 
> I see, don't remember exactly why it was originally onSignalConnected, but
your
> suspicion sounds right ;)
> 
> synchronous is ok as long as it is on a dedicated thread, which I think it is
> right?

Yes, there is the class BatteryStatusNotificationThread which does all the
synchronous work.

https://codereview.chromium.org/2066503002/diff/20001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:489: FindBatteryDevice();
On 2016/07/05 16:21:07, timvolodine wrote:
> On 2016/06/17 08:45:19, markuso wrote:
> > On 2016/06/16 16:12:24, timvolodine wrote:
> > > but it will always pick the first one it encounters? also, if I am looking
> > > correctly seems like it won't execute the for loop since battery_!=null ?
> > 
> > Yes, it will always pick the first battery, either the DisplayDevice or the
> > first from the enumeration. That is the same logic as before, with the
> exception
> > of the DisplayDevice and now we can update the chosen battery if the user
> > adds/removes one.
> > 
> > FindBatteryDevice() first moves battery_ to a local variable (current, line
> > 392). So battery_ will be empty and we enter the loop.
> 
> ok, could you please add a comment in FindBatteryDevice() with this
explanation?

I'll add such a comment.
 
> in line 392: doesn't the std::move(battery_) leave battery_ in an unspecified
> state?

line 392: std::unique_ptr<BatteryObject> current = std::move(battery_);

this uses the unique_ptr& operator=( unique_ptr&& r ); and is implemented as if
calling

current.reset(battery_.release());

So battery_ is in a well defined state, it is set to nullptr.

See http://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D

https://codereview.chromium.org/2066503002/diff/60001/device/battery/battery_...
File device/battery/battery_status_manager_linux-inl.h (right):

https://codereview.chromium.org/2066503002/diff/60001/device/battery/battery_...
device/battery/battery_status_manager_linux-inl.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/07/10 14:57:15, Ken Rockot wrote:
> I don't see a good reason to add this file. Please just declare the constants
in
> battery_status_manager_linux.h and define them in the cc.

This file was added to share the names between unittests and implementation in
battery_status_manager_linux.cc.
If I declare them in battery_status_manager_linux.h, then I need to declare them
as DEVICE_BATTERY_EXPORT - otherwise there are linker problems (if I recall
correctly on linking the unittests). Adding DEVICE_BATTERY_EXPORT for these
constants pollutes the symbol table unnecessarily.

[markuso, 2016-08-19 12:02:51]

Finally I had the time to test all the changes on a laptop that has two
batteries and where I can remove the batteries. That laptop has UPower version
0.9.23.

Testing on that laptop I noticed a few bugs. I added notes to the corresponding
places and I'll upload a new fix for these issues.

With the fixes everything seems to work smoothly both on the laptop with the
removable batteries and UPower version 0.9.23 and on a laptop with UPower
version 0.99.

https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
File device/battery/battery_status_manager_linux.cc (right):

https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:48: :
dbus::PropertySet(object_proxy, kUPowerDeviceInterfaceName, callback) {
I noticed this is the wrong interface-name. We need to use kUPowerInterfaceName
- these are after all the properties of the "org.freedesktop.UPower" object and
not of some of its devices. Using the wrong interface-name causes us to not get
the UPower properties, so we won't see the UPower daemon version and we won't
enable the compatibility mode.

https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:353: kUPowerServiceName,
kUPowerSignalDeviceChanged,
There is no need to connect to the "DeviceChanged" signal (for a daemon below
version 0.99).
That signal is sent if any UPower device has changed properties. For us it is
enough to get a signal if the one device we monitor has changed properties. We
get that from connecting the battery_ instance to the signal "Changed".

https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:507: if
(reader.PopObjectPath(&removed_device_path) &&
UPower specifies that the signal 'DeviceRemoved' has an object-path as argument,
however IRL I observed that signal with a string argument, so we need to cover
both cases and call FindBatteryDevice() if either we couldn't get the argument
or the removed device-path is the battery_ device we monitor.

https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:518:
battery_->properties()->Invalidate();
If the system uses a UPower daemon older than version 0.99, then we connect the
battery_ object's "Changed" signal to this method. Thus we are notified about
changed battery_ properties. Here we invalidate all property-values (since the
signal does not provide information about which properties were changed). Thus
the dbus::Property<>::GetAndBlock() will fetch the new value. If a new value was
fetched GetAndBlock() triggers the property-changed callback
BatteryPropertyChanged(). Both BatteryPropertyChanged() and BatteryChanged()
call NotifyBatteryStatus(). We need to avoid a recursion like

BatteryChanged() -> NotifyBatteryStatus() -> property1->GetAndBlock() ->
BatteryPropertyChanged() -> NotifyBatteryStatus() -> property2->GetAndBlock() ->
...

[timvolodine, 2016-08-19 16:18:32]

On 2016/08/19 12:02:51, markuso wrote:
> Finally I had the time to test all the changes on a laptop that has two
> batteries and where I can remove the batteries. That laptop has UPower version
> 0.9.23.
> 
> Testing on that laptop I noticed a few bugs. I added notes to the
corresponding
> places and I'll upload a new fix for these issues.
> 
> With the fixes everything seems to work smoothly both on the laptop with the
> removable batteries and UPower version 0.9.23 and on a laptop with UPower
> version 0.99.
> 
>
https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
> File device/battery/battery_status_manager_linux.cc (right):
> 
>
https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
> device/battery/battery_status_manager_linux.cc:48: :
> dbus::PropertySet(object_proxy, kUPowerDeviceInterfaceName, callback) {
> I noticed this is the wrong interface-name. We need to use
kUPowerInterfaceName
> - these are after all the properties of the "org.freedesktop.UPower" object
and
> not of some of its devices. Using the wrong interface-name causes us to not
get
> the UPower properties, so we won't see the UPower daemon version and we won't
> enable the compatibility mode.
> 
>
https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
> device/battery/battery_status_manager_linux.cc:353: kUPowerServiceName,
> kUPowerSignalDeviceChanged,
> There is no need to connect to the "DeviceChanged" signal (for a daemon below
> version 0.99).
> That signal is sent if any UPower device has changed properties. For us it is
> enough to get a signal if the one device we monitor has changed properties. We
> get that from connecting the battery_ instance to the signal "Changed".
> 
>
https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
> device/battery/battery_status_manager_linux.cc:507: if
> (reader.PopObjectPath(&removed_device_path) &&
> UPower specifies that the signal 'DeviceRemoved' has an object-path as
argument,
> however IRL I observed that signal with a string argument, so we need to cover
> both cases and call FindBatteryDevice() if either we couldn't get the argument
> or the removed device-path is the battery_ device we monitor.
> 
>
https://codereview.chromium.org/2066503002/diff/80001/device/battery/battery_...
> device/battery/battery_status_manager_linux.cc:518:
> battery_->properties()->Invalidate();
> If the system uses a UPower daemon older than version 0.99, then we connect
the
> battery_ object's "Changed" signal to this method. Thus we are notified about
> changed battery_ properties. Here we invalidate all property-values (since the
> signal does not provide information about which properties were changed). Thus
> the dbus::Property<>::GetAndBlock() will fetch the new value. If a new value
was
> fetched GetAndBlock() triggers the property-changed callback
> BatteryPropertyChanged(). Both BatteryPropertyChanged() and BatteryChanged()
> call NotifyBatteryStatus(). We need to avoid a recursion like
> 
> BatteryChanged() -> NotifyBatteryStatus() -> property1->GetAndBlock() ->
> BatteryPropertyChanged() -> NotifyBatteryStatus() -> property2->GetAndBlock()
->
> ...

great to hear, thanks for rigorously testing this!

[markuso, 2016-08-23 13:12:22]

The CQ bit was checked by markuso@opera.com

[markuso, 2016-08-23 13:12:23]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2066503002/#ps140001
(title: "Implement device::BatteryStatus support for UPower daemon 0.99.x
(rebased)")

[commit-bot: I haz the power, 2016-08-23 13:12:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 15:49:27]

Description was changed from

==========
Implement device::BatteryStatus support for UPower daemon 0.99.x

The org.freedesktop.UPower API was changed when upgrading the UPower daemon
from version 0.9.23 to 0.99.x. The BatteryStatusManagerLinux used the
"Changed" signal which was replaced by the "PropertyChanged" signal.

Change the BatteryStatusManagerLinux to use the new UPower API
(and keep compatibility with the 0.9.23 version, which is still used in
Ubuntu 14.04 LTS).

1. use dbus::PropertySet: the dbus::PropertySet provides simple access to
the properties and connection to the property-changed notifications.

2. Use UPower method GetDisplayDevice:
The 'DisplayDevice' is a composite battery device. That was added in UPower
version 0.99.0. If we don't get that device or if it is no battery, then we
continue to enumerate all devices.

3. Listen to 'DeviceAdded' and 'DeviceRemoved' signals:
Re-enumerate battery devices if a device is added/removed.

4. Compatibility with UPower version < 0.99
Only old UPower versions need to connect to the 'Changed' signal.

5. Rewrite the existing unittests to use a BatteryStatusManagerLinux instance
with a dbus::MockBus and mock the dbus-methods/properties for the test.
Add more unittests:
- for changing device properties
- for the DisplayDevice
- for enumerating devices
- for the DeviceAdded and DeviceRemoved signals
==========

to

==========
Implement device::BatteryStatus support for UPower daemon 0.99.x

The org.freedesktop.UPower API was changed when upgrading the UPower daemon
from version 0.9.23 to 0.99.x. The BatteryStatusManagerLinux used the
"Changed" signal which was replaced by the "PropertyChanged" signal.

Change the BatteryStatusManagerLinux to use the new UPower API
(and keep compatibility with the 0.9.23 version, which is still used in
Ubuntu 14.04 LTS).

1. use dbus::PropertySet: the dbus::PropertySet provides simple access to
the properties and connection to the property-changed notifications.

2. Use UPower method GetDisplayDevice:
The 'DisplayDevice' is a composite battery device. That was added in UPower
version 0.99.0. If we don't get that device or if it is no battery, then we
continue to enumerate all devices.

3. Listen to 'DeviceAdded' and 'DeviceRemoved' signals:
Re-enumerate battery devices if a device is added/removed.

4. Compatibility with UPower version < 0.99
Only old UPower versions need to connect to the 'Changed' signal.

5. Rewrite the existing unittests to use a BatteryStatusManagerLinux instance
with a dbus::MockBus and mock the dbus-methods/properties for the test.
Add more unittests:
- for changing device properties
- for the DisplayDevice
- for enumerating devices
- for the DeviceAdded and DeviceRemoved signals
==========

[commit-bot: I haz the power, 2016-08-23 15:49:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-23 15:51:24]

Description was changed from

==========
Implement device::BatteryStatus support for UPower daemon 0.99.x

The org.freedesktop.UPower API was changed when upgrading the UPower daemon
from version 0.9.23 to 0.99.x. The BatteryStatusManagerLinux used the
"Changed" signal which was replaced by the "PropertyChanged" signal.

Change the BatteryStatusManagerLinux to use the new UPower API
(and keep compatibility with the 0.9.23 version, which is still used in
Ubuntu 14.04 LTS).

1. use dbus::PropertySet: the dbus::PropertySet provides simple access to
the properties and connection to the property-changed notifications.

2. Use UPower method GetDisplayDevice:
The 'DisplayDevice' is a composite battery device. That was added in UPower
version 0.99.0. If we don't get that device or if it is no battery, then we
continue to enumerate all devices.

3. Listen to 'DeviceAdded' and 'DeviceRemoved' signals:
Re-enumerate battery devices if a device is added/removed.

4. Compatibility with UPower version < 0.99
Only old UPower versions need to connect to the 'Changed' signal.

5. Rewrite the existing unittests to use a BatteryStatusManagerLinux instance
with a dbus::MockBus and mock the dbus-methods/properties for the test.
Add more unittests:
- for changing device properties
- for the DisplayDevice
- for enumerating devices
- for the DeviceAdded and DeviceRemoved signals
==========

to

==========
Implement device::BatteryStatus support for UPower daemon 0.99.x

The org.freedesktop.UPower API was changed when upgrading the UPower daemon
from version 0.9.23 to 0.99.x. The BatteryStatusManagerLinux used the
"Changed" signal which was replaced by the "PropertyChanged" signal.

Change the BatteryStatusManagerLinux to use the new UPower API
(and keep compatibility with the 0.9.23 version, which is still used in
Ubuntu 14.04 LTS).

1. use dbus::PropertySet: the dbus::PropertySet provides simple access to
the properties and connection to the property-changed notifications.

2. Use UPower method GetDisplayDevice:
The 'DisplayDevice' is a composite battery device. That was added in UPower
version 0.99.0. If we don't get that device or if it is no battery, then we
continue to enumerate all devices.

3. Listen to 'DeviceAdded' and 'DeviceRemoved' signals:
Re-enumerate battery devices if a device is added/removed.

4. Compatibility with UPower version < 0.99
Only old UPower versions need to connect to the 'Changed' signal.

5. Rewrite the existing unittests to use a BatteryStatusManagerLinux instance
with a dbus::MockBus and mock the dbus-methods/properties for the test.
Add more unittests:
- for changing device properties
- for the DisplayDevice
- for enumerating devices
- for the DeviceAdded and DeviceRemoved signals

Committed: https://crrev.com/472a020d5783be9433678edac28953e24e1f5922
Cr-Commit-Position: refs/heads/master@{#413745}
==========

[commit-bot: I haz the power, 2016-08-23 15:51:25]

Patchset 8 (id:??) landed as
https://crrev.com/472a020d5783be9433678edac28953e24e1f5922
Cr-Commit-Position: refs/heads/master@{#413745}

[timvolodine, 2016-09-19 18:38:12]

retrospective update: this was lgtm.