MonitorSignal low-level wrapper introduced

MonitorSignal low-level wrapper introduced

Change-Id: I12dd3725dcd05ba5fd490d1a94575fd794aa335b

BUG=chromium-os:11121
TEST=none

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=8a93c90

[glotov, 2011-02-10 20:53:37]

Hi! Please have a look.

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

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.cc
File chromeos/dbus/dbus.cc (right):

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.cc#newcode7
chromeos/dbus/dbus.cc:7: #include <base/stringprintf.h>
nit: alphabetize these

(technically, i think that the dbus includes are supposed to come first, then a
blank line, and then the base includes, but with double-quotes instead of angled
brackets -- see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...)

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.cc#newcod...
chromeos/dbus/dbus.cc:265: // Snoop on D-Bus messages so we can get notified
about brightness changes.
remove the part about brightness here

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.cc#newcod...
chromeos/dbus/dbus.cc:297: dbus_bus_remove_match(dbus_conn,
GetDBusMatchString().c_str(), &error);
i think that you'll segfault right now if someone creates a MonitorSignal but
then destroys it without calling StartMonitoring() first

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h
File chromeos/dbus/dbus.h (right):

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h#newcode8
chromeos/dbus/dbus.h:8: #include <dbus/dbus-glib-lowlevel.h>
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)

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h#newcode429
chromeos/dbus/dbus.h:429: void StartMonitoring(const char* interface, const
char* signal);
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.

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h#newcode432
chromeos/dbus/dbus.h:432: virtual void OnSignal(DBusMessage* message) = 0;
nit: add a blank line between each method signature and the next method's
comment so this is easier to read

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

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h
File chromeos/dbus/dbus.h (right):

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h#newcode425
chromeos/dbus/dbus.h:425: class MonitorSignal {
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]

All done. Please have a look.

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.cc
File chromeos/dbus/dbus.cc (right):

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.cc#newcode7
chromeos/dbus/dbus.cc:7: #include <base/stringprintf.h>
On 2011/02/10 23:07:24, Daniel Erat wrote:
> nit: alphabetize these
> 
> (technically, i think that the dbus includes are supposed to come first, then
a
> blank line, and then the base includes, but with double-quotes instead of
angled
> brackets -- see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...)

Done.

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.cc#newcod...
chromeos/dbus/dbus.cc:265: // Snoop on D-Bus messages so we can get notified
about brightness changes.
On 2011/02/10 23:07:24, Daniel Erat wrote:
> remove the part about brightness here

Done.

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.cc#newcod...
chromeos/dbus/dbus.cc:297: dbus_bus_remove_match(dbus_conn,
GetDBusMatchString().c_str(), &error);
On 2011/02/10 23:07:24, Daniel Erat wrote:
> i think that you'll segfault right now if someone creates a MonitorSignal but
> then destroys it without calling StartMonitoring() first

Done.

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h
File chromeos/dbus/dbus.h (right):

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h#newcode8
chromeos/dbus/dbus.h:8: #include <dbus/dbus-glib-lowlevel.h>
On 2011/02/10 23:07:24, Daniel Erat wrote:
> 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)

Done.

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h#newcode425
chromeos/dbus/dbus.h:425: class MonitorSignal {
On 2011/02/10 23:13:36, Daniel Erat wrote:
> 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.

Done.

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h#newcode429
chromeos/dbus/dbus.h:429: void StartMonitoring(const char* interface, const
char* signal);
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.

http://codereview.chromium.org/6485006/diff/2001/chromeos/dbus/dbus.h#newcode432
chromeos/dbus/dbus.h:432: virtual void OnSignal(DBusMessage* message) = 0;
On 2011/02/10 23:07:24, Daniel Erat wrote:
> nit: add a blank line between each method signature and the next method's
> comment so this is easier to read

Done.

[Daniel Erat, 2011-02-11 18:31:44]

Thanks, almost there.

http://codereview.chromium.org/6485006/diff/3003/chromeos/dbus/dbus.h
File chromeos/dbus/dbus.h (right):

http://codereview.chromium.org/6485006/diff/3003/chromeos/dbus/dbus.h#newcode444
chromeos/dbus/dbus.h:444: const char* interface_;
use std::string instead for these so you don't crash if the caller frees the
strings that it passes in to the c'tor.  (this whole file should be written to
use std::string instead, but that's another change.)

[glotov, 2011-02-11 18:55:49]

http://codereview.chromium.org/6485006/diff/3003/chromeos/dbus/dbus.h
File chromeos/dbus/dbus.h (right):

http://codereview.chromium.org/6485006/diff/3003/chromeos/dbus/dbus.h#newcode444
chromeos/dbus/dbus.h:444: const char* interface_;
On 2011/02/11 18:31:45, Daniel Erat wrote:
> use std::string instead for these so you don't crash if the caller frees the
> strings that it passes in to the c'tor.  (this whole file should be written to
> use std::string instead, but that's another change.)

Done.

[Daniel Erat, 2011-02-11 19:30:45]

Thanks, LGTM.