Closed Bug 1282433 Opened 4 years ago Closed 3 years ago

Battery API broken on linux

Categories

(Core :: DOM: Core & HTML, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file, 5 obsolete files)

On linux no updates are received when the battery state changes.
Attached patch battery.patch (obsolete) — Splinter Review
overholt: Do you know anyone about to review dbus code?
Flags: needinfo?(overholt)
karlt? glandium?
Flags: needinfo?(overholt)
Attachment #8765516 - Flags: review?(mh+mozilla)
Attachment #8765516 - Flags: review?(mh+mozilla) → review?(karlt)
Comment on attachment 8765516 [details] [diff] [review]
battery.patch

Please provide a commit message.
Attachment #8765516 - Flags: review?(karlt)
Attached patch battery.patch (obsolete) — Splinter Review
Attachment #8765516 - Attachment is obsolete: true
Attachment #8765760 - Flags: review?(karlt)
Comment on attachment 8765760 [details] [diff] [review]
battery.patch

Review of attachment 8765760 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/linux/UPowerClient.cpp
@@ +235,5 @@
> +
> +  mUPowerProxyProp = dbus_g_proxy_new_for_name(mDBusConnection,
> +                                           "org.freedesktop.UPower",
> +                                           "/org/freedesktop/UPower",
> +                                           DBUS_INTERFACE_PROPERTIES);

I already fixed the indentation here.
Comment on attachment 8765760 [details] [diff] [review]
battery.patch

> Bug 1282433 - UPowerClient should use dbus PropertiesChanged signal for retrieving battery status changes, r?=karlt

There are some things I don't understand here.
Can you explain for me, please?

Why listen for "PropertiesChanged" on /org/freedesktop/UPower
but get properties on the device path?

In dbus-monitor --system, I see messages like these

> signal time=1467196381.664556 sender=:1.11 -> destination=(null destination) serial=13464 path=/org/freedesktop/UPower/devices/battery_BAT0; interface=org.freedesktop.UPower.Device; member=Changed
> signal time=1467196381.664655 sender=:1.11 -> destination=(null destination) serial=13465 path=/org/freedesktop/UPower; interface=org.freedesktop.UPower; member=DeviceChanged
   string "/org/freedesktop/UPower/devices/battery_BAT0"

What does DeviceChanged indicate, if not changes in device properties?
Flags: needinfo?(amarchesini)
Attachment #8765760 - Flags: review?(karlt)
> What does DeviceChanged indicate, if not changes in device properties?

Can you tell me which version of UPower do you have?
This is not what happens on my laptop having ubuntu xenial and upower 0.99 and in other linux system tested by others.
Also the official documentation of upower says that we should listen for PropertiesChanged notification.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #7)
> > What does DeviceChanged indicate, if not changes in device properties?
> 
> Can you tell me which version of UPower do you have?

I'm using 0.9.23.

> This is not what happens on my laptop having ubuntu xenial and upower 0.99
> and in other linux system tested by others.
> Also the official documentation of upower says that we should listen for
> PropertiesChanged notification.

It seems that 0.99 is using a different interface with the same name, as indicated in https://cgit.freedesktop.org/upower/commit/NEWS?id=384bae5af83fe05a91fc77c25e4d18eba9911cb1

So, listening for both DeviceChanged and PropertiesChanged does seem appropriate for now.  Please add a comment to explain why two different mechanisms are used to listen for notification of the same changes.
Should I take this as a r+?
Flags: needinfo?(karlt)
No, I haven't reviewed yet.  Still waiting for the answer to the first question in comment 6.
Flags: needinfo?(karlt)
Comment on attachment 8765760 [details] [diff] [review]
battery.patch

Review of attachment 8765760 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/linux/UPowerClient.cpp
@@ +232,5 @@
>    dbus_g_proxy_connect_signal(mUPowerProxy, "DeviceChanged",
>                                G_CALLBACK (DeviceChanged), this, nullptr);
> +
> +
> +  mUPowerProxyProp = dbus_g_proxy_new_for_name(mDBusConnection,

This is DBUS proxy for PropertyChanged. I cannot use the other proxy because it points to a different interface/service/name.

@@ +239,5 @@
> +                                           DBUS_INTERFACE_PROPERTIES);
> +
> +  dbus_g_proxy_add_signal(mUPowerProxyProp, "PropertiesChanged", G_TYPE_STRING,
> +                          dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),
> +                          G_TYPE_STRV, G_TYPE_INVALID);

DBus works in such a way that, in order to connect a signal to a callback you must describe the 'prototype' of that signal. This signal receives a string + an hashtable + strv.
For hashtable you also need to describe what it contains. Here the reason why we have these 3 lines.
Similar to what we have for the DeviceChanged signal.

@@ +241,5 @@
> +  dbus_g_proxy_add_signal(mUPowerProxyProp, "PropertiesChanged", G_TYPE_STRING,
> +                          dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),
> +                          G_TYPE_STRV, G_TYPE_INVALID);
> +  dbus_g_proxy_connect_signal(mUPowerProxyProp, "PropertiesChanged",
> +                              G_CALLBACK (PropertiesChanged), this, nullptr);

Finally the connect :)

@@ +260,5 @@
>    dbus_g_proxy_disconnect_signal(mUPowerProxy, "DeviceChanged",
>                                   G_CALLBACK (DeviceChanged), this);
>  
> +  dbus_g_proxy_disconnect_signal(mUPowerProxyProp, "PropertiesChanged",
> +                                 G_CALLBACK (PropertiesChanged), this);

Disconnect is easy.

@@ +274,5 @@
>    g_object_unref(mUPowerProxy);
>    mUPowerProxy = nullptr;
>  
> +  g_object_unref(mUPowerProxyProp);
> +  mUPowerProxyProp = nullptr;

nullify stuff...

@@ +364,5 @@
> +                                UPowerClient* aListener)
> +{
> +  if (!aListener->mTrackedDevice) {
> +    return;
> +  }

If we don't have a TrackedDevice, the initialization is unfinished, or we are in shuttingdown.
Similar setup for DeviceChanged.

@@ +371,5 @@
> +  if (g_strcmp0(aObjectPath, "org.freedesktop.UPower")) {
> +#else
> +  if (g_ascii_strcasecmp(aObjectPath, "org.freedesktop.UPower")) {
> +#endif
> +    return;

The main difference between PropertiesChanged and DeviceChanged, is that we don't receive the name of the device in aObjectPath. Instead we receive a generic org.freedesktop.UPower. In theory we can remove this block, or have it in #ifdef DEBUG/#endif.

@@ +374,5 @@
> +#endif
> +    return;
> +  }
> +
> +  aListener->GetDevicePropertiesAsync(aListener->mTrackedDeviceProxy);

Here we continue as DeviceChanged does.
Attachment #8765760 - Flags: review?(karlt)
Comment on attachment 8765760 [details] [diff] [review]
battery.patch

Hmm.  I guess I was referring to the second question in comment 6.

(In reply to Andrea Marchesini (:baku) from comment #11)
> > +  mUPowerProxyProp = dbus_g_proxy_new_for_name(mDBusConnection,
> 
> This is DBUS proxy for PropertyChanged. I cannot use the other proxy because
> it points to a different interface/service/name.

The object path here is "/org/freedesktop/UPower".  Presumably that means
the signal is sent when a property on this UPower object changes, and not
necessarily when a property changes on one of the device objects.

https://dbus.freedesktop.org/doc/dbus-specification.html

"If one or more properties change on an object, the
 org.freedesktop.DBus.Properties.PropertiesChanged signal may be emitted"

> > +  aListener->GetDevicePropertiesAsync(aListener->mTrackedDeviceProxy);
> 
> Here we continue as DeviceChanged does.

.. to get properties using a path describing a *device* object.

> If we don't have a TrackedDevice, the initialization is unfinished, or we
> are in shuttingdown.
> Similar setup for DeviceChanged.
> 
> @@ +371,5 @@
> > +  if (g_strcmp0(aObjectPath, "org.freedesktop.UPower")) {
> > +#else
> > +  if (g_ascii_strcasecmp(aObjectPath, "org.freedesktop.UPower")) {
> > +#endif
> > +    return;
> 
> The main difference between PropertiesChanged and DeviceChanged, is that we
> don't receive the name of the device in aObjectPath. Instead we receive a
> generic org.freedesktop.UPower. In theory we can remove this block, or have
> it in #ifdef DEBUG/#endif.

This check is cheap and so good, I think, to filter out changes on any other interfaces.
Note that the |interface_name| parameter is not an object path, so please name it appropriately.  Similarly, please name the other parameters consistently with the interface, or leave out the parameter names.

GLib 2.22 is required to build, so just use g_strcmp0.

A more significant difference is that DeviceChanged and PropertiesChanged are emitted on different objects.

DeviceChanged was emitted on the UPower object when the device changed.

The properties fetched are on the device object.
I assume the PropertiesChanged signal on the device object is the one of
interest?

Please also add a comment to explain in the code why two different mechanisms
are used to listen for notification of the same changes.

> Bug 1282433 - UPowerClient should use dbus PropertiesChanged signal for retrieving battery status changes, r?=karlt

Please use imperative mood in the first line of commit messages to describe
what the patch changes, rather than subjunctive describing what should be
done.

"use dbus PropertiesChanged signal for notification of battery status changes

From UPower 0.99, DeviceChanged has been removed from the UPower interface and
PropertiesChanged support added."
Attachment #8765760 - Flags: review?(karlt) → review-
Attached patch battery.patch (obsolete) — Splinter Review
Attachment #8765760 - Attachment is obsolete: true
Attachment #8774228 - Flags: review?(karlt)
Comment on attachment 8774228 [details] [diff] [review]
battery.patch

>+  mUPowerProxyProp = dbus_g_proxy_new_for_name(mDBusConnection,
>+                                               "org.freedesktop.UPower",
>+                                               "/org/freedesktop/UPower",
>+                                               DBUS_INTERFACE_PROPERTIES);
>+
>+  dbus_g_proxy_add_signal(mUPowerProxyProp, "PropertiesChanged", G_TYPE_STRING,

This is the "UPower" object.  Its properties are described at
https://upower.freedesktop.org/docs/UPower.html#UPower.properties
and not useful AFAICS.

Listen to PropertiesChanged on the device object using mTrackedDeviceProxy, for changes in the properties used in UpdateSavedInfo():
https://upower.freedesktop.org/docs/Device.html#Device.properties
Attachment #8774228 - Flags: review?(karlt) → review-
Attached patch battery.patch (obsolete) — Splinter Review
Both approaches have the same result. Here a patch with your comments applied.
Attachment #8774228 - Attachment is obsolete: true
Attachment #8775935 - Flags: review?(karlt)
Karlt, news?
Flags: needinfo?(karlt)
BatteryAPI is not exposed to content anymore.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
Comment on attachment 8775935 [details] [diff] [review]
battery.patch

This makes sense, thanks.

>+  static void PropertiesChanged(DBusGProxy* aProxy, const gchar* aInterfaceName,
>+                                GHashTable* aHashTable, char *word[],
>+                                UPowerClient* aListener);

Please either name the GHashTable* and char** parameters consistently
with the interface, or leave out the parameter names.

Please also comments in the code to explain why two different mechanisms
are used to listen for notification of the same changes.

>+    dbus_g_proxy_add_signal(mTrackedDeviceProxy, "PropertiesChanged", G_TYPE_STRING,
>+			    dbus_g_type_get_map("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),

Please wrap to stay within 80 columns.

>+UPowerClient::PropertiesChanged(DBusGProxy* aProxy, const gchar* aInterfaceName,
>+                                GHashTable* aHashTable, char *aWord[],
>+                                UPowerClient* aListener)
>+{
>+  if (!aListener->mTrackedDevice) {
>+    return;
>+  }

mTrackedDevice is always set here, so remove the dead return statement.
An assert is fine if you like, but the member is not used anyway, so there's
no need.
Flags: needinfo?(karlt)
Attachment #8775935 - Flags: review?(karlt) → review-
We are not removing this API.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attached patch battery.patch (obsolete) — Splinter Review
Attachment #8775935 - Attachment is obsolete: true
Attachment #8805930 - Flags: review?(karlt)
Comment on attachment 8805930 [details] [diff] [review]
battery.patch

>+                                GHashTable*, char *aWord[],

Please remove the name of the |aWord| parameter and place the '*' beside the type.  The name is misleading because it is not a word, but a pointer to the first element of an array of names of invalidated_properties.  Either "gchar*[]" or "gchar**" is fine as they are equivalent [1].

[1] http://stackoverflow.com/questions/4810664/how-do-i-use-arrays-in-c/4810672#4810672
Attachment #8805930 - Flags: review?(karlt) → review-
Attached patch battery.patchSplinter Review
Attachment #8805930 - Attachment is obsolete: true
Attachment #8806236 - Flags: review?(karlt)
Attachment #8806236 - Flags: review?(karlt) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03ccd2a9b30c
Use dbus PropertiesChanged signal for notification of battery status changes, r=karlt
https://hg.mozilla.org/mozilla-central/rev/03ccd2a9b30c
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.