Bluetooth: clean up BluetoothDevice

Bluetooth: clean up Bluetooth classes

The abstract BluetoothDevice class has a few problems inherited from
its origin as a ChromeOS-specific class split between implementation
and platform-specific components.

Clean those problems up, specifically:
 - replace bluetooth_class_, name_ and address_ non-abstract members
   with getter functions that the platform should implement.
 - also make IsConnected(), IsConnectable() and IsConnecting()
   abstract functions rather than providing an implementation
 - remove IsVisible() which was a CrOS-specific hack
 - remove IsBonded(), use IsPaired() instead
 - remove service_uuids_ non-abstract member; make GetServices return
   a copy of the list to allow implementations to fetch it on demand

BluetoothDevice retains implementations for GetName(), GetDisplayType()
and ProvidesServiceWithUUID() since those can be implemented entirely
using platform-provided information functions and would be identical
in each platform.

Also rename BluetoothAdapter::address() and BluetoothAdapter::name() to
GetAddress() and GetName() to match since they are also pure virtual.

BUG=none
TEST=device_unittests, browser_tests, unit_tests


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192474

[keybuk, 2013-04-03 21:41:55]

youngki: for device/ OWNERS
jennyz: for ash/system/ OWNERS
stevenjb: for chrome/browser/chromeos/ OWNERS
miket: for chrome/browser/extensions/ OWNERS
jhawkins: for chrome/browser/resources/options/ OWNERS
nkostylev: for chrome/browser/resources/chromeos/ &
chrome/browser/ui/webui/options/chromeos/ OWNERS
mkearney: for chrome/common/extensions/api/ OWNERS

[youngki, 2013-04-03 22:17:17]

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
File device/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
device/bluetooth/bluetooth_device.h:155: virtual uint32 bluetooth_class() const
= 0;
Should we make bluetooth_class() and name() (probably address() too) protected
so that we can enforce the usage of GetName() and GetDeviceType()?

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
device/bluetooth/bluetooth_device.h:193: virtual ServiceList GetServices() const
= 0;
It's just my thinking, but do you think we should just make the caller pass the
mutable vector pointers and let this function fill in that vector? That way we
could avoid copying the entire vector every time this method is called:

virtual void GetServices(ServiceList* services) const = 0;

[keybuk, 2013-04-03 22:18:55]

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
File device/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
device/bluetooth/bluetooth_device.h:155: virtual uint32 bluetooth_class() const
= 0;
On 2013/04/03 22:17:17, youngki wrote:
> Should we make bluetooth_class() and name() (probably address() too) protected
> so that we can enforce the usage of GetName() and GetDeviceType()?

I'm happy with that

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
device/bluetooth/bluetooth_device.h:193: virtual ServiceList GetServices() const
= 0;
On 2013/04/03 22:17:17, youngki wrote:
> It's just my thinking, but do you think we should just make the caller pass
the
> mutable vector pointers and let this function fill in that vector? That way we
> could avoid copying the entire vector every time this method is called:
> 
> virtual void GetServices(ServiceList* services) const = 0;

If you think that would be better - I was kinda assuming we'd get "move
constructors" soon ;-)

[stevenjb, 2013-04-03 23:04:57]

https://codereview.chromium.org/13416005/diff/6001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/13416005/diff/6001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device.h:155: virtual uint32 bluetooth_class() const
= 0;
On 2013/04/03 22:18:56, keybuk wrote:
> On 2013/04/03 22:17:17, youngki wrote:
> > Should we make bluetooth_class() and name() (probably address() too)
protected
> > so that we can enforce the usage of GetName() and GetDeviceType()?
> 
> I'm happy with that
Do we even need these in the public interface, or can they be moved to the
implementation?

[keybuk, 2013-04-04 00:43:41]

On 2013/04/03 23:04:57, stevenjb (chromium) wrote:
>
https://codereview.chromium.org/13416005/diff/6001/device/bluetooth/bluetooth...
> File device/bluetooth/bluetooth_device.h (right):
> 
>
https://codereview.chromium.org/13416005/diff/6001/device/bluetooth/bluetooth...
> device/bluetooth/bluetooth_device.h:155: virtual uint32 bluetooth_class()
const
> = 0;
> On 2013/04/03 22:18:56, keybuk wrote:
> > On 2013/04/03 22:17:17, youngki wrote:
> > > Should we make bluetooth_class() and name() (probably address() too)
> protected
> > > so that we can enforce the usage of GetName() and GetDeviceType()?
> > 
> > I'm happy with that
> Do we even need these in the public interface, or can they be moved to the
> implementation?

BluetoothDevice::GetName and BluetoothDevice::GetDeviceType uses them - so they
have to be in BluetoothDevice as pure virtual functions so BluetoothDevice* can
fill them in

[stevenjb, 2013-04-04 00:47:30]

lgtm

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
File device/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
device/bluetooth/bluetooth_device.h:155: virtual uint32 bluetooth_class() const
= 0;
On 2013/04/03 23:04:57, stevenjb (chromium) wrote:
> On 2013/04/03 22:18:56, keybuk wrote:
> > On 2013/04/03 22:17:17, youngki wrote:
> > > Should we make bluetooth_class() and name() (probably address() too)
> protected
> > > so that we can enforce the usage of GetName() and GetDeviceType()?
> > 
> > I'm happy with that
> Do we even need these in the public interface, or can they be moved to the
> implementation?

If we need these as pure virtuals then we should rename them, e.g.
GetBluetoothClass, GetSystemName, GetAddress().

[stevenjb, 2013-04-04 00:48:36]

On 2013/04/04 00:47:30, stevenjb (chromium) wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
> File device/bluetooth/bluetooth_device.h (right):
> 
>
https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
> device/bluetooth/bluetooth_device.h:155: virtual uint32 bluetooth_class()
const
> = 0;
> On 2013/04/03 23:04:57, stevenjb (chromium) wrote:
> > On 2013/04/03 22:18:56, keybuk wrote:
> > > On 2013/04/03 22:17:17, youngki wrote:
> > > > Should we make bluetooth_class() and name() (probably address() too)
> > protected
> > > > so that we can enforce the usage of GetName() and GetDeviceType()?
> > > 
> > > I'm happy with that
> > Do we even need these in the public interface, or can they be moved to the
> > implementation?
> 
> If we need these as pure virtuals then we should rename them, e.g.
> GetBluetoothClass, GetSystemName, GetAddress().

Oops, and didn't meant to hit 'Quick lg...' However, with renaming those methods
this looks fine to me.

[keybuk, 2013-04-04 01:18:06]

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
File device/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
device/bluetooth/bluetooth_device.h:155: virtual uint32 bluetooth_class() const
= 0;
On 2013/04/04 00:47:31, stevenjb (chromium) wrote:
> On 2013/04/03 23:04:57, stevenjb (chromium) wrote:
> > On 2013/04/03 22:18:56, keybuk wrote:
> > > On 2013/04/03 22:17:17, youngki wrote:
> > > > Should we make bluetooth_class() and name() (probably address() too)
> > protected
> > > > so that we can enforce the usage of GetName() and GetDeviceType()?
> > > 
> > > I'm happy with that
> > Do we even need these in the public interface, or can they be moved to the
> > implementation?
> 
> If we need these as pure virtuals then we should rename them, e.g.
> GetBluetoothClass, GetSystemName, GetAddress().

Done.

[youngki, 2013-04-04 12:59:08]

lgtm

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
File device/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/13416005/diff/6001/device/bluetooth/bl...
device/bluetooth/bluetooth_device.h:193: virtual ServiceList GetServices() const
= 0;
On 2013/04/03 22:18:56, keybuk wrote:
> On 2013/04/03 22:17:17, youngki wrote:
> > It's just my thinking, but do you think we should just make the caller pass
> the
> > mutable vector pointers and let this function fill in that vector? That way
we
> > could avoid copying the entire vector every time this method is called:
> > 
> > virtual void GetServices(ServiceList* services) const = 0;
> 
> If you think that would be better - I was kinda assuming we'd get "move
> constructors" soon ;-)
actually this might not be worth passing a mutable pointer given we are dealing
with std objects;

[stevenjb, 2013-04-04 16:10:47]

LGTM

[youngki, 2013-04-04 19:59:11]

lgtm

[miket_OOO, 2013-04-04 21:36:41]

> miket: for chrome/browser/extensions/ OWNERS

LGTM

[James Hawkins, 2013-04-04 21:37:08]

resources/ LGTM

I suggest breaking up such a large CL in the future.

[Rick Byers, 2013-04-04 22:04:25]

On 2013/04/04 21:37:08, James Hawkins wrote:
> resources/ LGTM
> 
> I suggest breaking up such a large CL in the future.

resources/chromeos LGTM too (though James's LGTM should be sufficient).

[commit-bot: I haz the power, 2013-04-04 22:05:44]

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

[commit-bot: I haz the power, 2013-04-05 03:32:29]

Change committed as 192474

[Nikita (slow), 2013-04-05 10:34:33]

lgtm