696041 - Battery API: Linux (upower) backend

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Standalone uPower POC

A standalone C++ program using upower to get battery status.

Comment 2

[Mounir Lamouri (:mounir)]

I completely forgot to check what was the license of upower (assuming that it would likely be a library compatible license). Unfortunately, it's licensed in GPL so we can't link against it. We can still use upower but that would require us to use the DBUS interface...

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

In addition of the comments in the code (the TODO for which I will open follow-ups if the code stay as is), I have a few comments:
- The way I'm managing the singleton makes our leak detector thinks we are leaking because the memory is freed too late. Maybe we could statically create and destroy the singleton but I wonder where we should do that.
- I'm using dbus-glib which requires us to have libxul linked against dbus-glib instead of dbus. I don't think it's a big deal and dbus without glib bindings is quite a pain... (for the small of what I've seen).
- We have something in toolkit/system/dubs/ that I could have used but needs to be in pair with a service. Given how hal works, I thought we might not want to do that.

Chris, I piked you for the review because you know the high level code involved (at least you will have to review some part of it) but I don't know how confident you are to review DBUS related code. If you don't feel like reviewing this patch, Roc was the other guy in my list ;)

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 569101 [details] [diff] [review]
Patch v1

In general, when assigning |ptr = [null]|, use |ptr = nsnull|.  Soon
we'll have nullptr and this will be a non-issue, but for now better to
stick with older convention.

>diff --git a/hal/Makefile.in b/hal/Makefile.in

> ifeq (Android,$(OS_TARGET))
> CPPSRCS += AndroidHal.cpp
>+else ifeq (Linux,$(OS_TARGET))
>+  CPPSRCS += LinuxHal.cpp
>+  ifdef MOZ_ENABLE_DBUS
>+    CPPSRCS += UPowerClient.cpp
>+  endif

Nit: Whitespace tends to make some |make|s unhappy, and it's
incongruous with the surrounding lines.  Please choose one style or
the other.

>diff --git a/hal/linux/LinuxHal.cpp b/hal/linux/LinuxHal.cpp

>+void
>+RegisterBatteryObserver(dom::battery::BatteryInformation* aBatteryInfo)
>+{
>+#ifdef MOZ_ENABLE_DBUS

This implementation would be considerably simpler if done like this.

  #ifndef MOZ_ENABLE_DBUS
  void
  RegisterBatteryObserver(dom::battery::BatteryInformation* aBatteryInfo)
  { ... }
  void
  UnregisterBatteryObserver()
  { ... }
  #endif

Then you can implement the HAL APIs in UPowerClient.cpp without having
to expose a "public" interface to it; that is, all the implementation
can live in UPowerClient.cpp as a kind of "module", no header needed.

Originally the plan was to use separable files for these pluggable
implementations, but I won't ask you to do that until we have a need
to use the sysfs battery client outside of gonk.  ifdef OK for now.

>diff --git a/hal/linux/UPowerClient.cpp b/hal/linux/UPowerClient.cpp

For consistency, I would put all this code in the hal_impl namespace.

>+void
>+UPowerClient::BeginListening()
>+{
>+  /**
>+   * TODO: we should probably listen to DeviceAdded and DeviceRemoved signals.
>+   * If we do that, we would have to disconnect from those in StopListening.
>+   * It's not yet implemented because it requires testing hot plugging and
>+   * removal of a battery.
>+   */

Nit: // comment plz.

>+void
>+UPowerClient::UpdateTrackedDevice()
>+{
>+  /**
>+   * We are looking for the first device that is a battery.
>+   * TODO: we could try to combine more than one battery.
>+   */

Nit: // plz.

>+  for (guint i=0; i<devices->len; ++i) {
>+    char* devicePath = static_cast<char*>(g_ptr_array_index(devices, i));
>+    GHashTable* hashTable = GetDeviceProperties(devicePath);
>+
>+    if (g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(hashTable, "Type"))) == sDeviceTypeBattery) {
>+      UpdateSavedInfo(hashTable);
>+      mTrackedDevice = devicePath;

Don't you need to strdup() |devicePath| here?  If not, then I think it
leaks for non-battery devices.

>+/* static */ void
>+UPowerClient::DeviceChanged(DBusGProxy* aProxy, const gchar* aObjectPath, UPowerClient* aListener)
>+{
>+  // Notify observers.
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();

Should be |using namespace mozilla::services|, but this impl detail
may change.

>+GHashTable*
>+UPowerClient::GetDeviceProperties(const char* aDevice)
>+{
>+  DBusGProxy* proxy = dbus_g_proxy_new_for_name(mDBusConnection,
>+                                                "org.freedesktop.UPower",
>+                                                aDevice,
>+                                                "org.freedesktop.DBus.Properties");
>+
>+  GError* error = 0;
>+  GHashTable* hashTable = 0;
>+  GType typeGHashTable = dbus_g_type_get_map("GHashTable", G_TYPE_STRING,
>+                                            G_TYPE_VALUE);
>+  if (!dbus_g_proxy_call(proxy, "GetAll", &error, G_TYPE_STRING,
>+                         "org.freedesktop.UPower.Device", G_TYPE_INVALID,
>+                         typeGHashTable, &hashTable, G_TYPE_INVALID)) {
>+    g_printerr("Error: %s\n", error->message);
>+    g_error_free(error);
>+    g_object_unref(proxy);
>+    return 0;
>+  }
>+
>+  g_object_unref(proxy);
>+

I think it would be worth investing in an RAII helper that does g_object_unref() for you.  Something like,

  struct NS_STACK_CLASS AutoGObjectPtr {
    //...
    ~AutoGObjectPtr() { g_object_unref(ptr); }
  };

>+void
>+UPowerClient::UpdateSavedInfo(GHashTable* aHashTable)
>+{
>+  mLevel = g_value_get_double(static_cast<const GValue*>(g_hash_table_lookup(aHashTable, "Percentage")));
>+
>+  switch (g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(aHashTable, "State")))) {
>+    case eState_Unkwonw:

eState_Unknown?  And this should fall back on kDefaultCharging, right?

>diff --git a/hal/linux/UPowerClient.h b/hal/linux/UPowerClient.h

Per above, this file isn't needed, you can move all this into
UPowerClient.cpp.

This implementation looks good, nice work!.  Most of the comments are
nits or cleanups.  Would like to see the next version.

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> >+  for (guint i=0; i<devices->len; ++i) {
> >+    char* devicePath = static_cast<char*>(g_ptr_array_index(devices, i));
> >+    GHashTable* hashTable = GetDeviceProperties(devicePath);
> >+
> >+    if (g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(hashTable, "Type"))) == sDeviceTypeBattery) {
> >+      UpdateSavedInfo(hashTable);
> >+      mTrackedDevice = devicePath;
> 
> Don't you need to strdup() |devicePath| here?  If not, then I think it
> leaks for non-battery devices.

Well, I think we were leaking...

> >+void
> >+UPowerClient::UpdateSavedInfo(GHashTable* aHashTable)
> >+{
> >+  mLevel = g_value_get_double(static_cast<const GValue*>(g_hash_table_lookup(aHashTable, "Percentage")));
> >+
> >+  switch (g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(aHashTable, "State")))) {
> >+    case eState_Unkwonw:
> 
> eState_Unknown?  And this should fall back on kDefaultCharging, right?

Nice catch!

I'm going to attach a patch with all the requested changes.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Comment 8

[Mounir Lamouri (:mounir)]

Chris, are you okay with the points I mentioned in comment 3?

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #6)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> > >+  for (guint i=0; i<devices->len; ++i) {
> > >+    char* devicePath = static_cast<char*>(g_ptr_array_index(devices, i));
> > >+    GHashTable* hashTable = GetDeviceProperties(devicePath);
> > >+
> > >+    if (g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(hashTable, "Type"))) == sDeviceTypeBattery) {
> > >+      UpdateSavedInfo(hashTable);
> > >+      mTrackedDevice = devicePath;
> > 
> > Don't you need to strdup() |devicePath| here?  If not, then I think it
> > leaks for non-battery devices.
> 
> Well, I think we were leaking...
> 

Need to figure this out :).  Something smells fishy here.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3)
> Created attachment 569100 [details] [diff] [review] [diff] [details] [review]
> Patch v1
> 
> In addition of the comments in the code (the TODO for which I will open
> follow-ups if the code stay as is), I have a few comments:
> - The way I'm managing the singleton makes our leak detector thinks we are
> leaking because the memory is freed too late. Maybe we could statically
> create and destroy the singleton but I wonder where we should do that.

One option would be to only create the singleton when there are listeners and then destroy it when they all go away.  With the changes we discussed to the hal:: API, I think that would be easier to do.

> - I'm using dbus-glib which requires us to have libxul linked against
> dbus-glib instead of dbus. I don't think it's a big deal and dbus without
> glib bindings is quite a pain... (for the small of what I've seen).

Fine by me.

I should have noted that I'm not a dbus expert but I'm comfortable with reviewing this level of usage.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> One option would be to only create the singleton when there are listeners
> and then destroy it when they all go away.  With the changes we discussed to
> the hal:: API, I think that would be easier to do.
> 

(And then recreate it again when there are later listeners, etc.)

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 569669 [details] [diff] [review]
Patch v1.1

Waiting to hear back about string ownership issue.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

Asking a super-review just to have another person looking at the dbus code and approving the dbus-glib requirement for libxul.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3)
> > Created attachment 569100 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Patch v1
> > 
> > In addition of the comments in the code (the TODO for which I will open
> > follow-ups if the code stay as is), I have a few comments:
> > - The way I'm managing the singleton makes our leak detector thinks we are
> > leaking because the memory is freed too late. Maybe we could statically
> > create and destroy the singleton but I wonder where we should do that.
> 
> One option would be to only create the singleton when there are listeners
> and then destroy it when they all go away.  With the changes we discussed to
> the hal:: API, I think that would be easier to do.

I will change that with the new api then.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 569811 [details] [diff] [review]
Patch v1.2

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

You should probably use nsAutoRef instead of AutoGPtr.

Karl is your man for the dbus stuff.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 569811 [details] [diff] [review]
Patch v1.2

>+  enum States {
>+    eState_Unkwonw = 0,

eState_Unknown

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.3

Updated patch using the new hal API.

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Convert AutoGPtr to nsAutoRef

This patch requires the patch in bug 698003.

Comment 19

[Mounir Lamouri (:mounir)]

Roc and Karl, do you think you could have those reviews done in a week time frame? I would like to push Battery API for Firefox 10 and I really want the Linux support :) (except those patches, I only need one small review to have everything ready)

Comment 20

[Karl Tomlinson (:karlt)]

In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> I completely forgot to check what was the license of upower (assuming that
> it would likely be a library compatible license). Unfortunately, it's
> licensed in GPL so we can't link against it. We can still use upower but
> that would require us to use the DBUS interface...

I don't really follow why dlopening a GPL library would make the program a
derived work, while communicating in another manner would not, but that does
seem the intent of the GPL, so I guess playing that game is the right
approach.
http://www.gnu.org/licenses/gpl-faq.html#MereAggregation

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3)
> - I'm using dbus-glib which requires us to have libxul linked against
> dbus-glib instead of dbus. I don't think it's a big deal and dbus without
> glib bindings is quite a pain... (for the small of what I've seen).
> - We have something in toolkit/system/dubs/ that I could have used but needs
> to be in pair with a service. Given how hal works, I thought we might not
> want to do that.

This would be unfixing bug 643695 (and duplicate bug 448375).

Can you explain "in pair with a service" and "how hal works" for me, please?

I actually don't know much about dbus so don't know the reasons for using
DBus in three different ways throughout our code.

Are "DBus Handler Apps" (Bug 441636, which introduced the dbus dependency)
actually useful on desktop distributions or just Maemo?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Karl Tomlinson (:karlt) from comment #21)
> I actually don't know much about dbus so don't know the reasons for using
> DBus in three different ways throughout our code.

toolkit/system/dbus/nsDBusService allows for persistent connections and receiving/handling dbus messages during the lifetime of our app (NetworkManager messages, in particular). It uses DBUS_BUS_SYSTEM and needs to keep the connection open.

nsDBusHandlerApp::LaunchWithURI uses DBUS_BUS_SESSION and does not need to keep the connection open. We might be able to combine it with nsDBusService in some way but that seems more trouble than it's worth.

It seems to me that if it's OK to use XPCOM from hal, we should use nsDBusService from there. If it's not OK to use XPCOM from hal, I recommend deCOMtaminating nsDBusService and moving it somewhere it can be used from hal, and using it for the battery API as well.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although, nsDBusService has the nice property that it's a dynamic component so if dbus libraries aren't around, it just doesn't load. So moving that under hal/ may not make sense.

Comment 24

[Mounir Lamouri (:mounir)]

(In reply to Karl Tomlinson (:karlt) from comment #20)
> I don't really follow why dlopening a GPL library would make the program a
> derived work, while communicating in another manner would not, but that does
> seem the intent of the GPL, so I guess playing that game is the right
> approach.
> http://www.gnu.org/licenses/gpl-faq.html#MereAggregation

This is indeed part of how GPL works. Anyhow, like glandium pointed, on IRC, that would prevent fairly old systems to be able to run firefox without troubles: upower is new and is unlikely widely installed yet.

(In reply to Karl Tomlinson (:karlt) from comment #21)
> Can you explain "in pair with a service" and "how hal works" for me, please?

What I meant is that I think we want to keep XPCOM out of hal/. Chris could confirm that.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> It seems to me that if it's OK to use XPCOM from hal, we should use
> nsDBusService from there. If it's not OK to use XPCOM from hal, I recommend
> deCOMtaminating nsDBusService and moving it somewhere it can be used from
> hal, and using it for the battery API as well.

I don't know in which conditions we could use nsDbusService for communicating with upower but as far as the behavior is concerned, it seems to fit the requirements.

What should we do to move on? Is depending on dbus-glib that bad when we are depending on dbus and glib? If it's not a blocker, we could just go ahead by pushing that patch and find a better solution later. Otherwise, we could try to find a solution or I could rewrite everything without the glib bindings (which is going to be quite a pain).

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to Karl Tomlinson (:karlt) from comment #21)
> I actually don't know much about dbus so don't know the reasons for using
> DBus in three different ways throughout our code.

Does it really make sense to have a third review if you don't know much about DBus? I think roc asked you to review this patch to double-check the DBus code.

Comment 26

[Mounir Lamouri (:mounir)]

All Gnome based distributions must have dbus-glib installed. Most GTK based distributions too (a lot of GTK apps use dbus-glib) so glandium asked me to look at the status of dbus-glib in Kubuntu and Lubuntu and they both have it installed by default. Which means it's likely safe to link against dbus-glib. For non-mainstream distribution, given that we already require dbus and glib, adding dbus-glib is going to require to the user to install a simple package which takes a few hundred kB (less than 1MB for sure).

What I would suggest is to push this patch as is and open a follow-up to make it us nsDbusService if possible. In that case, we would be able to remove dbus-glib from libxul.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mike Hommey, what do you think about taking a dbus-glib dependency here?

Comment 28

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #21)
> Are "DBus Handler Apps" (Bug 441636, which introduced the dbus dependency)
> actually useful on desktop distributions or just Maemo?

These don't seem to be used on mozilla-central so this code (and the dbus
dependency) should all be Maemo-only, or at least controlled through a
configure option if other xul apps want it.

Comment 29

[Karl Tomlinson (:karlt)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #25)
> (In reply to Karl Tomlinson (:karlt) from comment #21)
> > I actually don't know much about dbus so don't know the reasons for using
> > DBus in three different ways throughout our code.
> 
> Does it really make sense to have a third review if you don't know much
> about DBus? I think roc asked you to review this patch to double-check the
> DBus code.

Yes, I don't know whether this was really necessary.  But since I was asked, I
started looking and have some comments/questions.

I trust you'll fold the two patches before pushing.
(i.e. that the second patch was separate to simplify review.)

nsDBusService handles the "Disconnected" signal.
Is there are reason why that is not necessary here?

nsDBusHandlerApp and nsDBusService call
dbus_connection_set_exit_on_disconnect(FALSE).
Is there are reason why that is not necessary here?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #26)
> glandium asked me to
> look at the status of dbus-glib in Kubuntu and Lubuntu and they both have it
> installed by default. Which means it's likely safe to link against
> dbus-glib.

If Mike is happy depending on dbus-glib, then I'm happy.

However, then there's not much point having libdbusservice.so as a separate
module, so we should at least have a follow-up to demodulate dbusservice.

Comment 30

[Mike Hommey [:glandium]]

I'm happy depending how old are systems we do care about. Re-reading bug 448375, which is the original bug where I removed the need for libdbus-glib on libxul.so, it looks like old Ubuntu systems didn't have it installed by default. But these are likely 3+ years old systems.

One way to figure out what systems we need to care about is to look at the update ping stats, and cross reference the gtk and linux kernel versions. Bug 666735 contains some information about systems and the versions they provided, but at quick glance, doesn't seem to say what our user base is using. On the other hand, we now have a dependency on freetype >= 2.3.0, maybe all systems that come with freetype >= 2.3.0 come with dbus-glib installed by default.

Comment 31

[Mounir Lamouri (:mounir)]

(In reply to Karl Tomlinson (:karlt) from comment #29)
> I trust you'll fold the two patches before pushing.
> (i.e. that the second patch was separate to simplify review.)

Yes.

> nsDBusService handles the "Disconnected" signal.
> Is there are reason why that is not necessary here?
> 
> nsDBusHandlerApp and nsDBusService call
> dbus_connection_set_exit_on_disconnect(FALSE).
> Is there are reason why that is not necessary here?

To be honest, I don't know off hand. It's the first time I'm seriously hacking with dbus. I think roc wrote nsDBusService, maybe he knows? I will try to see what that does too and check if it seems required.

> However, then there's not much point having libdbusservice.so as a separate
> module, so we should at least have a follow-up to demodulate dbusservice.

I don't have any opinion here. We could also open a follow up to decommify libdbusservice and use it from hal/.

Comment 32

[Mounir Lamouri (:mounir)]

(In reply to Karl Tomlinson (:karlt) from comment #29)
> nsDBusService handles the "Disconnected" signal.
> Is there are reason why that is not necessary here?

The only way I saw to get the "Disconnected" signal is when dbus is shutdown. Withouth handling this disgnal, mDBusConnection is kept alive and mUPowerProxy too which creates some warnings. Handling this will improve a dbus re-starting more nicely. Though, it wouldn't be perfect because BeginListening is called by hal/ and we can't tell hal/ we should be re-initialized (yet).

> nsDBusHandlerApp and nsDBusService call
> dbus_connection_set_exit_on_disconnect(FALSE).
> Is there are reason why that is not necessary here?

This is actually necessary, we do not want to have _exit() called when were are disconnected which seems to happen by default when dbus_bus_get() is called [1].

[1] http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga19091beb74f1504b0e862a7ad10e71cd

Comment 33

[Mounir Lamouri (:mounir)]

Attachment: Better story with disconnect signal

Nice catch btw ;)

Comment 34

[Mounir Lamouri (:mounir)]

Attachment: Better story with disconnect signal

Better with the correct file...

Comment 35

[Karl Tomlinson (:karlt)]

Comment on attachment 571023 [details] [diff] [review]
Better story with disconnect signal

The filter should be explicitly removed.  DBusGConnection is "shared with
other callers of" dbus_g_bus_get(), so an unref will not necessarily remove
the filter.

>+    static_cast<UPowerClient*>(aData)->StopListening();
>+    return DBUS_HANDLER_RESULT_HANDLED;

This prevents other filters from running and nsDBusService at least will want
to know about this.  Let the other filters run.

>+  dbus_connection_set_exit_on_disconnect(
>+    dbus_g_connection_get_connection(mDBusConnection),
>+    false);
>+
>+  // Listening to signals the DBus connection is going to get so we will know
>+  // when it is lost and we will be able to disconnect cleanly.
>+  dbus_connection_add_filter(dbus_g_connection_get_connection(mDBusConnection),
>+                             ConnectionSignalFilter, this, nsnull);

Using a local variable for DBusConnection would save having to call
dbus_g_connection_get_connection() twice.

Comment 36

[Mounir Lamouri (:mounir)]

Attachment: Better story with disconnect signal

Sorry, I didn't know dbus_g_bus_get was returning a global value. I should have look more carefully at the doc. Nice catch again!

Comment 37

[Karl Tomlinson (:karlt)]

Comment on attachment 570229 [details] [diff] [review]
Patch v1.3

Chris has already reviewed this so you don't need my review.

If this is enabled by default, you'll need to replace g_strcmp0 as it needs GLib 2.16 and we build against 2.12.  Test on tryserver.

Blocking on DBus calls could well cause some issues, but maybe that can be addressed separately.

Comment 38

[Mounir Lamouri (:mounir)]

(In reply to Karl Tomlinson (:karlt) from comment #37)
> If this is enabled by default, you'll need to replace g_strcmp0 as it needs
> GLib 2.16 and we build against 2.12.  Test on tryserver.

In my queue, I already changed the patch because two methods were not available in the buildbots. I put a #if.

> Blocking on DBus calls could well cause some issues, but maybe that can be
> addressed separately.

You mean those calls: dbus_g_proxy_call? I believe we want that to be sync otherwise we would run into race conditions. What kind of issues could that cause?

Thanks for the review!

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/72357a8f0d89

Comment 40

[Karl Tomlinson (:karlt)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #38)
> > Blocking on DBus calls could well cause some issues, but maybe that can be
> > addressed separately.
> 
> You mean those calls: dbus_g_proxy_call? I believe we want that to be sync
> otherwise we would run into race conditions. What kind of issues could that
> cause?

I was just thinking of responsiveness, if the dbus server is busy executing some other method, but I don't know how dbus works.  It may be OK.  I guess the dbus server can process our requests while waiting for responses from messages on other interfaces.

We sometimes do similar things waiting for responses from the X server, but, if the X server is unresponsive, there is little we can do.

Comment 41

[Mounir Lamouri (:mounir)]

Someone knows what's the process to get the requirement page updated?