chrome: bluetooth: hook up DeviceCreated/Removed signals

chrome: bluetooth: hook up DeviceCreated/Removed signals

The adapter sends signals when known devices are created and removed,
we may need these to obtain properties for the devices so hook them
up so the observer can receive them.

BUG=chromium-os:22086
TEST=verified signals are received

Change-Id: I3931df90f78903308abe244a208901aa7f0a64a4


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120766

[mukesh agrawal, 2012-02-01 00:48:42]

possible parameter error in the Received functions. otherwise, some nits.

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc (right):

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:311: // Called by
dbus:: when a DeviceCreated signal is received.
why "dbus::" (trailing colons)?
(same for other functions)

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:312: void
DeviceCreatedReceived(const std::string& object_path,
you might consider "using std::string" and shortening the parameter types.
(OTOH, there's a bunch of existing code that would need to be change. so i'm
also ok leaving as-is.)

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:326:
DeviceCreated(object_path, device_path));
are the arguments in the correct order? prototype is

  void DeviceCreated(const std::string& adapter_path,
                     const std::string& object_path)

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:331: const std::string&
interface_name,
google style comments out unused parameters. |interface_name| and |signal_name|
should be commented out here.

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:353:
DeviceRemoved(object_path, device_path));
as in DeviceAddedReceived: check callback parameter order

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:358: const std::string&
interface_name,
as above (comment out unused parameters).

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/dbus/bluetooth_adapter_client.h (right):

https://chromiumcodereview.appspot.com/9317012/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.h:32: const std::string&
object_path) {}
i'm somewhat surprised that these have default implementations. (i would have
expected Observer to be an abstract base.)

but it is consistent with the existing code, so i'll leave it up to you whether
or not to change.

[keybuk, 2012-02-01 05:59:24]

Thanks Mukesh, that's exactly the kind of review I was hoping for :) all of
these are "in keeping with surrounding style" (even what looks like, but isn't,
a parameter order issue) but it's things I'll clean up as I go!

[satorux1, 2012-02-01 17:45:18]

http://codereview.chromium.org/9317012/diff/1/chrome/browser/chromeos/dbus/bl...
File chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc (right):

http://codereview.chromium.org/9317012/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:312: void
DeviceCreatedReceived(const std::string& object_path,
On 2012/02/01 00:48:42, mukesh agrawal wrote:
> you might consider "using std::string" and shortening the parameter types.
> (OTOH, there's a bunch of existing code that would need to be change. so i'm
> also ok leaving as-is.)

Please don't do this in Chrome's code for consistency. std::whatever is not
shortened in Chrome's code.

http://codereview.chromium.org/9317012/diff/1/chrome/browser/chromeos/dbus/bl...
File chrome/browser/chromeos/dbus/bluetooth_adapter_client.h (right):

http://codereview.chromium.org/9317012/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.h:32: const std::string&
object_path) {}
On 2012/02/01 00:48:42, mukesh agrawal wrote:
> i'm somewhat surprised that these have default implementations. (i would have
> expected Observer to be an abstract base.)
> 
> but it is consistent with the existing code, so i'll leave it up to you
whether
> or not to change.

I think it's not so uncommon for observers to have default implementations, as
not all observers are interested in all events (i.e. with default
implementations, they can only implement events they are interested in).

[keybuk, 2012-02-04 00:27:12]

can I get an LGTM? :)

[satorux1, 2012-02-04 00:30:40]

Did you address issues mukesh pointed out? he questioned about argument order
etc. Please reply to his comments.

[keybuk, 2012-02-04 00:32:49]

oh, I had it as a draft

[keybuk, 2012-02-04 00:35:47]

http://codereview.chromium.org/9317012/diff/1/chrome/browser/chromeos/dbus/bl...
File chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc (right):

http://codereview.chromium.org/9317012/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:311: // Called by
dbus:: when a DeviceCreated signal is received.
referring to "code in the dbus namespace", ie the generic dbus::ObjectProxy,
dbus::Bus, etc. code

http://codereview.chromium.org/9317012/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:326:
DeviceCreated(object_path, device_path));
yes, the "object_path" here refers to the adapter (this is adapter code), but
from the observer point of view, the object_path is the device being created so
that means we put our object_path into the adapter_path argument and device_path
into the object_path argument

I'm slowly converting the code to use adapter_path, device_path, etc. rather
than the generic object_path, but have to do it one step at a time

http://codereview.chromium.org/9317012/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:331: const std::string&
interface_name,
Chrome style seems to not do that afaict?

[satorux1, 2012-02-04 00:48:20]

LGTM

[commit-bot: I haz the power, 2012-02-04 00:51:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/9317012/5001

[commit-bot: I haz the power, 2012-02-04 03:02:36]

Try job failure for 9317012-5001 (retry) (retry) on mac_rel for step
"net_unittests".
It's a second try, previously, step "net_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-06 14:49:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/9317012/5001

[commit-bot: I haz the power, 2012-02-06 18:12:41]

Try job failure for 9317012-5001 (retry) (retry) (retry) on mac_rel for steps
"test_shell_tests, ui_tests".
It's a second try, previously, steps "test_shell_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-06 20:07:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/9317012/14001

[commit-bot: I haz the power, 2012-02-06 22:10:59]

Try job failure for 9317012-14001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-07 00:39:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/9317012/14001

[commit-bot: I haz the power, 2012-02-07 11:29:55]

Change committed as 120766