Chrome OS: Add bluetooth API for device discovery

Chrome OS: Add bluetooth API for device discovery

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

Change-Id: I46ca12e310d4ed890fa9f1a3f91faf40637847a1


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

[Vince Laviano, 2011-10-26 02:01:30]

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

[satorux1, 2011-10-26 19:26:55]

Looks good overall.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:14: }  // namespace base
In .cc file, we usually don't forward like this, as it won't matter much for the
compilation time. Please just incldue base/values.h.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:16: using
base::DictionaryValue;
Let's not do this. saving 6 bytes don't help much.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:86: << address << " (with
" << device_properties.size() << " properties)";
<< for logging should be vertically aligned like:

VLOG(1) << ...
        << ...

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:90: BluetoothDevice
*device = BluetoothDevice::Create(device_properties);
Move * to left.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:98: delete device;
You might want to use scoped_ptr<> instead

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:133: return new
BluetoothAdapterImpl(id);
It's totally fine to hide Impl class in .cc file, but if you don't plan to
provide a stub implementation, you can just get rid of the Impl class, and add
code directly to BluetoothAdapter class, that'll save some lines of code.

However, as it's already written this way (pImpl pattern), it'd make more sense
to keep the structure.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:17: class BluetoothAdapter
{
Please add a class comment.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:20: const DictionaryValue&
properties)
base::DictionaryValue

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:24: name_(name),
device_name may be a bit clearer

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:26: virtual const
std::string& Address() const = 0;
GetAddress()? As it's essentially an accessor, address() may also be ok.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:29: virtual uint32
BluetoothClass() const = 0;
uint32 doesn't seem to be a good type. Can you return an enum instead like:

enum BluetoothClass {
  SomeClass,
  SomeOtherClass,
};

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:32: virtual const
std::string& Icon() const = 0;
What will string contain? An URL to an image? Please add a bit more comment, and
rename the function to something more descriptive.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:35: virtual const
std::string& Name() const = 0;
Maybe a bit redundant but DeviceName() may be a bit clearer.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:41: virtual const
DictionaryValue& DictionaryRep() const = 0;
AsDictionary() may be a bit better name?

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_manager.cc (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_manager.cc:82: default_adapter_ =
BluetoothAdapter::Create(adapter);
You might want to use scoped_ptr for default_adapter_, and do
default_adapter_.reset(BluetoothAdapter::Create(adapter)) here.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_manager.h (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_manager.h:43: static
BluetoothManager* GetInstance();
Please add function comments to them.

[Vince Laviano, 2011-10-26 21:41:55]

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:14: }  // namespace base
On 2011/10/26 19:26:55, satorux1 wrote:
> In .cc file, we usually don't forward like this, as it won't matter much for
the
> compilation time. Please just incldue base/values.h.

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:16: using
base::DictionaryValue;
On 2011/10/26 19:26:55, satorux1 wrote:
> Let's not do this. saving 6 bytes don't help much.

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:86: << address << " (with
" << device_properties.size() << " properties)";
On 2011/10/26 19:26:55, satorux1 wrote:
> << for logging should be vertically aligned like:
> 
> VLOG(1) << ...
>         << ...

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:90: BluetoothDevice
*device = BluetoothDevice::Create(device_properties);
On 2011/10/26 19:26:55, satorux1 wrote:
> Move * to left.

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:98: delete device;
On 2011/10/26 19:26:55, satorux1 wrote:
> You might want to use scoped_ptr<> instead

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:133: return new
BluetoothAdapterImpl(id);
On 2011/10/26 19:26:55, satorux1 wrote:
> It's totally fine to hide Impl class in .cc file, but if you don't plan to
> provide a stub implementation, you can just get rid of the Impl class, and add
> code directly to BluetoothAdapter class, that'll save some lines of code.
> 
> However, as it's already written this way (pImpl pattern), it'd make more
sense
> to keep the structure.

Yes, I'd prefer to keep the separation.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:17: class BluetoothAdapter
{
On 2011/10/26 19:26:55, satorux1 wrote:
> Please add a class comment.

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:20: const DictionaryValue&
properties)
On 2011/10/26 19:26:55, satorux1 wrote:
> base::DictionaryValue

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:24: name_(name),
On 2011/10/26 19:26:55, satorux1 wrote:
> device_name may be a bit clearer

These are named identically to the underlying properties defined in the BlueZ
D-Bus API, so there's a danger of creating confusion by making them different.
"bluetooth_class" is an exception, because "class" is a reserved word in C++.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:26: virtual const
std::string& Address() const = 0;
On 2011/10/26 19:26:55, satorux1 wrote:
> GetAddress()? As it's essentially an accessor, address() may also be ok.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

I had originally named them that way in my CL at
http://codereview.chromium.org/8233042/ and received some code review comments
from tfarina@ pointing to a document that said that accessors that are virtual
functions ought not to be named in unix_hacker_style because it suggests that
they are inexpensive when they may not be.  So, I changed the accessor names to
be CamelCase. I could either add a Get prefix to these CamelCase names, or
revert back to my original unix_hacker_style naming.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:29: virtual uint32
BluetoothClass() const = 0;
On 2011/10/26 19:26:55, satorux1 wrote:
> uint32 doesn't seem to be a good type. Can you return an enum instead like:
> 
> enum BluetoothClass {
>   SomeClass,
>   SomeOtherClass,
> };

This is a bitfield that contains 3 subfields representing service class, major
device class, and minor device class. We could mask out the subfields and
provide access to them as 3 separate method calls, each returning an enum value,
but I think that would be better done in a future CL once we  know what clients
care about. If we implement this in full generality, we'll have a pretty lengthy
set of enums, since each major device class applies its own meaning to the bits
in the minor device class field. See
http://bluetooth-pentest.narod.ru/software/bluetooth_class_of_device-service_...
to get a sense of how this works.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:32: virtual const
std::string& Icon() const = 0;
On 2011/10/26 19:26:55, satorux1 wrote:
> What will string contain? An URL to an image? Please add a bit more comment,
and
> rename the function to something more descriptive.

"icon" is a property defined in the BlueZ D-Bus API, and I'd like to maintain a
name that's consistent with BlueZ.

It contains one of the strings defined in table 6 at
http://standards.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.htm...
that describes which sort of icon ought to be displayed for the device (but
doesn't specify a particular image). I'll add a comment explaining this.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:35: virtual const
std::string& Name() const = 0;
On 2011/10/26 19:26:55, satorux1 wrote:
> Maybe a bit redundant but DeviceName() may be a bit clearer.

Ditto. Would like to retain the BlueZ property name.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:41: virtual const
DictionaryValue& DictionaryRep() const = 0;
On 2011/10/26 19:26:55, satorux1 wrote:
> AsDictionary() may be a bit better name?

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_manager.cc (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_manager.cc:82: default_adapter_ =
BluetoothAdapter::Create(adapter);
On 2011/10/26 19:26:55, satorux1 wrote:
> You might want to use scoped_ptr for default_adapter_, and do
> default_adapter_.reset(BluetoothAdapter::Create(adapter)) here.

Done.

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_manager.h (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_manager.h:43: static
BluetoothManager* GetInstance();
On 2011/10/26 19:26:55, satorux1 wrote:
> Please add function comments to them.

Done.

[satorux1, 2011-10-26 22:28:10]

LGTM

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/8394030/diff/3001/chrome/browser/chromeos/blue...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:29: virtual uint32
BluetoothClass() const = 0;
On 2011/10/26 21:41:55, Vince Laviano wrote:
> On 2011/10/26 19:26:55, satorux1 wrote:
> > uint32 doesn't seem to be a good type. Can you return an enum instead like:
> > 
> > enum BluetoothClass {
> >   SomeClass,
> >   SomeOtherClass,
> > };
> 
> This is a bitfield that contains 3 subfields representing service class, major
> device class, and minor device class. We could mask out the subfields and
> provide access to them as 3 separate method calls, each returning an enum
value,
> but I think that would be better done in a future CL once we  know what
clients
> care about. If we implement this in full generality, we'll have a pretty
lengthy
> set of enums, since each major device class applies its own meaning to the
bits
> in the minor device class field. See
>
http://bluetooth-pentest.narod.ru/software/bluetooth_class_of_device-service_...
> to get a sense of how this works.

Please add that information as a comment here.

http://codereview.chromium.org/8394030/diff/12001/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc (right):

http://codereview.chromium.org/8394030/diff/12001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:89:
device.reset(BluetoothDevice::Create(device_properties));
scoped_ptr<BluetoothDevice> device(
    BluetoothDevice::Create(device_properties));

http://codereview.chromium.org/8394030/diff/12001/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_manager.h (right):

http://codereview.chromium.org/8394030/diff/12001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_manager.h:47: // Returns a pointer
to the global BluetoothManager instance.
Initialize() needs to be called beforehand, right? Would be nice to document
about it.

[Vince Laviano, 2011-10-26 22:38:12]

http://codereview.chromium.org/8394030/diff/12001/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc (right):

http://codereview.chromium.org/8394030/diff/12001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:89:
device.reset(BluetoothDevice::Create(device_properties));
On 2011/10/26 22:28:10, satorux1 wrote:
> scoped_ptr<BluetoothDevice> device(
>     BluetoothDevice::Create(device_properties));

Done.

http://codereview.chromium.org/8394030/diff/12001/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_manager.h (right):

http://codereview.chromium.org/8394030/diff/12001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_manager.h:47: // Returns a pointer
to the global BluetoothManager instance.
On 2011/10/26 22:28:10, satorux1 wrote:
> Initialize() needs to be called beforehand, right? Would be nice to document
> about it.

Done.

[commit-bot: I haz the power, 2011-10-26 22:41:45]

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

[satorux1, 2011-10-26 22:47:12]

LGTM

[commit-bot: I haz the power, 2011-10-26 23:46:25]

Change committed as 107470