MonitorSignal low-level wrapper introduced

chromeos/dbus/dbus.h

Index: chromeos/dbus/dbus.h
diff --git a/chromeos/dbus/dbus.h b/chromeos/dbus/dbus.h
index 025802160d4f4155ef60a9ea3522171b7fbd7a7b..ac1ae06e28de6d502e91a4fd9d4d1652c8c2501f 100644
--- a/chromeos/dbus/dbus.h
+++ b/chromeos/dbus/dbus.h
@@ -5,6 +5,7 @@
 #ifndef CHROMEOS_DBUS_H_
 #define CHROMEOS_DBUS_H_
 
+#include <dbus/dbus-glib-lowlevel.h>

[Daniel Erat, 2011/02/10 23:07:24]

do you need this in the header, or can you forward-declare DBusMessage and
DBusConnection?  (i'm not sure whether they're regular structs or typedef-ed
anonymous structs)

[glotov, 2011/02/11 16:36:52]

Done.

 #include <dbus/dbus-glib.h>
 #include <glib-object.h>
 
@@ -416,6 +417,29 @@ void SendSignalWithNoArgumentsToSystemBus(const char* path,
                                           const char* interface_name,
                                           const char* signal_name);
 
+// \brief Low-level signal monitor base class.
+//
+// Used when there is no definite named signal sender (that Proxy
+// could be used for).
+
+class MonitorSignal {

[Daniel Erat, 2011/02/10 23:13:36]

ah, just looked at the change where you're using it.  please rename the class to
something like "SignalWatcher" that makes it clear what it does.

[glotov, 2011/02/11 16:36:52]

Done.

+ public:
+  MonitorSignal() : interface_(NULL), signal_(NULL) {}
+  ~MonitorSignal();
+  void StartMonitoring(const char* interface, const char* signal);

[Daniel Erat, 2011/02/10 23:07:24]

it might be a bit cleaner to pass |interface| and |signal| in through the c'tor
instead of including them here, since they're really tied to the lifetime of the
object, or maybe you should just start the monitoring from the c'tor as well.

[glotov, 2011/02/11 16:36:52]

Start monitoring in ctor is not good because derived class might not have
completed its initialization by the time our constructor is done and monitoring
started.

About putting |interface| and |signal| in ctor. The only disadvantage is that
StartMonitoring() would not be able to check if it had already been called (or
additional flag would be needed). So I preferred to pass them here.

+ private:
+  // Callback invoked on the given signal arrival.
+  virtual void OnSignal(DBusMessage* message) = 0;

[Daniel Erat, 2011/02/10 23:07:24]

nit: add a blank line between each method signature and the next method's
comment so this is easier to read

[glotov, 2011/02/11 16:36:52]

Done.

+  // Returns a string matching the D-Bus messages that we want to listen for.
+  std::string GetDBusMatchString() const;
+  // A D-Bus message filter to receive signals.
+  static DBusHandlerResult FilterDBusMessage(DBusConnection* dbus_conn,
+                                             DBusMessage* message,
+                                             void* data);
+  const char* interface_;
+  const char* signal_;
+};
+
 }  // namespace dbus
 }  // namespace chromeos