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

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...

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 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.

(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.

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

(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.

(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.

(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 on attachment 569669 [details] [diff] [review] Patch v1.1 Waiting to hear back about string ownership issue.

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

(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 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 on attachment 569811 [details] [diff] [review] Patch v1.2 >+ enum States { >+ eState_Unkwonw = 0, eState_Unknown

Updated patch using the new hal API.

This patch requires the patch in bug 698003.

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)

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

(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?

(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.

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.

(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).

(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.

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.

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

(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.

(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.

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.

(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/.

(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

Nice catch btw ;)

Better with the correct file...

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.

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 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.

(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!

(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.

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