Chrome OS: Add bluetooth dbus clients for device discovery

Chrome OS: Add bluetooth dbus clients for device discovery

BUG=chromium-os:21319
TEST=Tested with demo code at http://codereview.chromium.org/8233042/

Change-Id: I53fe54c5bd7d6eea970ea25a5a26fa1ded9bbfad


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

[Vince Laviano, 2011-10-25 00:27:44]

Splitting the dbus layer code out from the bluetooth discovery demo CL at
http://codereview.chromium.org/8233042/ for separate review.

[satorux1, 2011-10-25 02:17:10]

Looks good overall. Minor comments only.

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

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:26: VLOG(1) <<
"BluetoothAdapterClientImpl ctor";
"ctor" -> "is created" or something, to make log messages more readable.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:30: proxies_.clear();
There is no harm, but it's unnecessary.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:36: CHECK(observer !=
NULL);
DCHECK(observer);  We generally use DCHECK.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:137: void
RemoveObjectProxyForPath(const std::string& path) {
path -> object_path to make it extra clearer (path may sound like a file path).

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:139: ProxyMap::iterator
it = proxies_.find(path);
You can just do

proxies_.erase(path);

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:149: std::string name;
name of what? would be nice to make it more descriptive.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:163: bool discovering;
please initialize this with false to avoid possible non-determinism.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:262: static bool
PopArrayOfDictEntries(dbus::MessageReader* reader,
This function doesn't have to be inside the class. Would be nice to move this to
the beginning of the file and put it into anonymous namespace.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:278: if
(!dict_entry_reader.PopString(&key)) {
So the key needs to be string, right? Would be nice to document about it in the
function comment.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:281:
dbus::MessageReader variant_reader(message);
So the value needs to be variant, right? Would be nice to document about it.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:406: ProxyMap proxies_;
proxy_map_ may make it clearer that this variable is a map.

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

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.h:32: bool discovering) =
0;
Instead of = 0, you might want to go with {}, so that clients don't have to
implement functions for events that they are not interested.

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

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_manager_client.h:45: typedef
base::Callback<void(std::string, bool)> DefaultAdapterCallback;
Please document parameters.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_manager_client.h:47: // Retrieves the
dbus object path for the default adapter.
What's is the default adapter? I'd like to see some explanation about it here.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/db...
File chrome/browser/chromeos/dbus/dbus_thread_manager.cc (right):

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/db...
chrome/browser/chromeos/dbus/dbus_thread_manager.cc:53:
CHECK(bluetooth_manager_client_.get() != NULL);
I think we don't need this. Create() is essentially "new", and "new" never
returns NULL.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/db...
chrome/browser/chromeos/dbus/dbus_thread_manager.cc:57:
CHECK(bluetooth_adapter_client_.get() != NULL);
ditto.

[Vince Laviano, 2011-10-25 19:18:03]

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

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:26: VLOG(1) <<
"BluetoothAdapterClientImpl ctor";
On 2011/10/25 02:17:10, satorux1 wrote:
> "ctor" -> "is created" or something, to make log messages more readable.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:30: proxies_.clear();
On 2011/10/25 02:17:10, satorux1 wrote:
> There is no harm, but it's unnecessary.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:36: CHECK(observer !=
NULL);
On 2011/10/25 02:17:10, satorux1 wrote:
> DCHECK(observer);  We generally use DCHECK.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:137: void
RemoveObjectProxyForPath(const std::string& path) {
On 2011/10/25 02:17:10, satorux1 wrote:
> path -> object_path to make it extra clearer (path may sound like a file
path).

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:139: ProxyMap::iterator
it = proxies_.find(path);
On 2011/10/25 02:17:10, satorux1 wrote:
> You can just do
> 
> proxies_.erase(path);

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:149: std::string name;
On 2011/10/25 02:17:10, satorux1 wrote:
> name of what? would be nice to make it more descriptive.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:163: bool discovering;
On 2011/10/25 02:17:10, satorux1 wrote:
> please initialize this with false to avoid possible non-determinism. 
> 

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:262: static bool
PopArrayOfDictEntries(dbus::MessageReader* reader,
On 2011/10/25 02:17:10, satorux1 wrote:
> This function doesn't have to be inside the class. Would be nice to move this
to
> the beginning of the file and put it into anonymous namespace.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:278: if
(!dict_entry_reader.PopString(&key)) {
On 2011/10/25 02:17:10, satorux1 wrote:
> So the key needs to be string, right? Would be nice to document about it in
the
> function comment.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:281:
dbus::MessageReader variant_reader(message);
On 2011/10/25 02:17:10, satorux1 wrote:
> So the value needs to be variant, right? Would be nice to document about it.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:406: ProxyMap proxies_;
On 2011/10/25 02:17:10, satorux1 wrote:
> proxy_map_ may make it clearer that this variable is a map.

Done.

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

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.h:32: bool discovering) =
0;
On 2011/10/25 02:17:10, satorux1 wrote:
> Instead of = 0, you might want to go with {}, so that clients don't have to
> implement functions for events that they are not interested.

Done.

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

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_manager_client.h:45: typedef
base::Callback<void(std::string, bool)> DefaultAdapterCallback;
On 2011/10/25 02:17:10, satorux1 wrote:
> Please document parameters.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/bl...
chrome/browser/chromeos/dbus/bluetooth_manager_client.h:47: // Retrieves the
dbus object path for the default adapter.
On 2011/10/25 02:17:10, satorux1 wrote:
> What's is the default adapter? I'd like to see some explanation about it here.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/db...
File chrome/browser/chromeos/dbus/dbus_thread_manager.cc (right):

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/db...
chrome/browser/chromeos/dbus/dbus_thread_manager.cc:53:
CHECK(bluetooth_manager_client_.get() != NULL);
On 2011/10/25 02:17:10, satorux1 wrote:
> I think we don't need this. Create() is essentially "new", and "new" never
> returns NULL.

Done.

http://codereview.chromium.org/8375042/diff/1/chrome/browser/chromeos/dbus/db...
chrome/browser/chromeos/dbus/dbus_thread_manager.cc:57:
CHECK(bluetooth_adapter_client_.get() != NULL);
On 2011/10/25 02:17:10, satorux1 wrote:
> ditto.

Done.

[satorux1, 2011-10-25 19:55:17]

almost there!

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc (right):

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:193: DCHECK(observer !=
NULL);
I think DCHECK(observer) is more common in Chrome code.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:221:
static_cast<dbus::ObjectProxy*>(it->second);
I think the static cast is unnecessary here.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:222:
DCHECK(adapter_proxy != NULL);
This may sound like an overuse of DCHECK(), but it's up to you.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:245:
static_cast<dbus::ObjectProxy*>(it->second);
ditto.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:264:
DCHECK(adapter_proxy != NULL);
we don't need this. GetObjectProxy guarantees not to return NULL.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:407: DCHECK(response !=
NULL);
LOG_IF(WARNING, response) << ... 

would be nicer. |response| is NULL if the method call is failed.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:414: DCHECK(response !=
NULL);
ditto.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/bluetooth_manager_client.cc (right):

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_manager_client.cc:30:
DCHECK(bluetooth_manager_proxy_ != NULL);
I'd remove this.

[Vince Laviano, 2011-10-25 20:31:42]

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc (right):

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:193: DCHECK(observer !=
NULL);
On 2011/10/25 19:55:17, satorux1 wrote:
> I think DCHECK(observer) is more common in Chrome code.

Done.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:221:
static_cast<dbus::ObjectProxy*>(it->second);
On 2011/10/25 19:55:17, satorux1 wrote:
> I think the static cast is unnecessary here.

Done.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:222:
DCHECK(adapter_proxy != NULL);
On 2011/10/25 19:55:17, satorux1 wrote:
> This may sound like an overuse of DCHECK(), but it's up to you.

Done.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:245:
static_cast<dbus::ObjectProxy*>(it->second);
On 2011/10/25 19:55:17, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:264:
DCHECK(adapter_proxy != NULL);
On 2011/10/25 19:55:17, satorux1 wrote:
> we don't need this. GetObjectProxy guarantees not to return NULL.

Done.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:407: DCHECK(response !=
NULL);
On 2011/10/25 19:55:17, satorux1 wrote:
> LOG_IF(WARNING, response) << ... 
> 
> would be nicer. |response| is NULL if the method call is failed.

Done.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_adapter_client.cc:414: DCHECK(response !=
NULL);
On 2011/10/25 19:55:17, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/bluetooth_manager_client.cc (right):

http://codereview.chromium.org/8375042/diff/5001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/bluetooth_manager_client.cc:30:
DCHECK(bluetooth_manager_proxy_ != NULL);
On 2011/10/25 19:55:17, satorux1 wrote:
> I'd remove this.

Done.

[satorux1, 2011-10-25 21:48:38]

LGTM

[commit-bot: I haz the power, 2011-10-25 21:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vlaviano@chromium.org/8375042/8003

[commit-bot: I haz the power, 2011-10-25 23:15:30]

Change committed as 107234