Add abstract BluetoothProfile class

Add abstract BluetoothProfile class

This class will form the base of the 4.0 BR+LE compatible API,
allowing for both incoming connections for profiles we're clients
for and implementing services within Chrome.

BUG=229636
TEST=none

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

[keybuk, 2013-04-09 22:04:50]

youngki: as discussed in meeting, here's the proposed BluetoothProfile API.

BluetoothProfileCompat would be implemented for the existing classes and use
GetServiceRecords() and ConnectToService()

BluetoothProfileExperimentalChromeOS would be implemented for the new Chrome OS
stack that perform the operations natively

BluetoothProfile::Register() will look like
BluetoothAdapterFactory::GetAdapter() and select the appropriate implementation

[youngki, 2013-04-10 01:28:49]

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.h (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:52: // guaranteed to hold a reference so
this may be outlive all of them.
s/may be outlive/may outlive/

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:54: virtual void
SetConnectionCallback(const SocketCallback& callback) = 0;
Since bluetooth.onConnection is an event and we are adding listeners to it, we
cannot actually have the callbacks here; instead we should communicate through
BluetoothAdapter::Observer, and let it dispatch the bluetooth.onConnection event
to the listeners. So we should have

BluetoothAdapter::Observer::OnConnected(scoped_refptr<BluetoothSocket> socket);

And somehow when the socket is made we should call OnConnected() of the
bluetooth adapter observers with the socket.

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:56: // Human readable name of the Profile,
e.g. "Health Device".
So all the below methods are for the profiles exported from the adapter itself?
Does it mean that if I want to register a profile from a remote device, I do not
need to call the below methods?

[youngki, 2013-04-10 16:38:19]

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.cc (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.cc:24: // subclass just like
BluetoothAdapterFactory
Are we going to follow the similar structure as BluetoothAdapterFactory? If so
then we will have a lazy instance of a list of profiles in this file? shouldn't
we also make Unregister static? I can't clearly see how we could delete the
profile if Unregister() is a member function of BluetoothProfile.

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.h (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:54: virtual void
SetConnectionCallback(const SocketCallback& callback) = 0;
On 2013/04/10 01:28:49, youngki wrote:
> Since bluetooth.onConnection is an event and we are adding listeners to it, we
> cannot actually have the callbacks here; instead we should communicate through
> BluetoothAdapter::Observer, and let it dispatch the bluetooth.onConnection
event
> to the listeners. So we should have
> 
> BluetoothAdapter::Observer::OnConnected(scoped_refptr<BluetoothSocket>
socket);
> 
> And somehow when the socket is made we should call OnConnected() of the
> bluetooth adapter observers with the socket.

Or, if we want to keep this interface, we could actually pass
BluetoothAdapter::CallOnConnectionObserverMethod(scoped_refptr<BluetoothSocket>)
as a callback and have that function call the observer method. Maybe that's
easier to implement.

[youngki, 2013-04-10 19:32:39]

lgtm

[youngki, 2013-04-10 19:32:54]

On 2013/04/10 19:32:39, youngki wrote:
> lgtm

We discussed this offline. LGTM

[youngki, 2013-04-10 19:39:04]

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.h (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:52: // guaranteed to hold a reference so
this may be outlive all of them.
On 2013/04/10 01:28:49, youngki wrote:
> s/may be outlive/may outlive/

Make sure to fix this before committing.

[youngki, 2013-04-11 18:33:41]

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.cc (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.cc:16: 
How about calling Unregister() in the destructor?

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.h (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:93: virtual ~BluetoothProfile();
Shouldn't the destructor be protected? Otherwise the object won't get
destroyed...

[keybuk, 2013-04-17 22:38:35]

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.cc (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.cc:16: 
On 2013/04/11 18:33:41, youngki wrote:
> How about calling Unregister() in the destructor?

The intent is that Unregister() *calls* the destructor - will clarify in the
comments

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.cc:24: // subclass just like
BluetoothAdapterFactory
On 2013/04/10 16:38:19, youngki wrote:
> Are we going to follow the similar structure as BluetoothAdapterFactory? If so
> then we will have a lazy instance of a list of profiles in this file?
shouldn't
> we also make Unregister static? I can't clearly see how we could delete the
> profile if Unregister() is a member function of BluetoothProfile.

As discussed, No; it should be entirely okay to deal with that in other ways

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.h (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:52: // guaranteed to hold a reference so
this may be outlive all of them.
On 2013/04/10 19:39:05, youngki wrote:
> On 2013/04/10 01:28:49, youngki wrote:
> > s/may be outlive/may outlive/
> 
> Make sure to fix this before committing.

Done.

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:93: virtual ~BluetoothProfile();
On 2013/04/11 18:33:41, youngki wrote:
> Shouldn't the destructor be protected? Otherwise the object won't get
> destroyed...

Unregister() should be doing the deletion - so it seems appropriate for it to be
Private

[keybuk, 2013-04-17 22:39:29]

ptal; this replaces the get/set methods with an Options struct as we discussed

other review comments should be addressed

[youngki, 2013-04-19 16:26:36]

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.h (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:93: virtual ~BluetoothProfile();
On 2013/04/17 22:38:35, keybuk wrote:
> On 2013/04/11 18:33:41, youngki wrote:
> > Shouldn't the destructor be protected? Otherwise the object won't get
> > destroyed...
> 
> Unregister() should be doing the deletion - so it seems appropriate for it to
be
> Private

I think you are trying to follow RefCounted model where the destructor is
private and RefCounted<>::Release deletes itself, right? Could you explain what
is the benefit of using Unregister() instead of destructor to delete the object?
Are we trying to count the references to the object?

https://chromiumcodereview.appspot.com/13862023/diff/18001/device/bluetooth/b...
File device/bluetooth/bluetooth_profile.cc (right):

https://chromiumcodereview.appspot.com/13862023/diff/18001/device/bluetooth/b...
device/bluetooth/bluetooth_profile.cc:10: 
Don't we have to put the BluetoothProfile::Options constructor implementation
here as well? Or make them an inline constructor?

[keybuk, 2013-04-19 17:58:18]

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.h (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:93: virtual ~BluetoothProfile();
On 2013/04/19 16:26:36, youngki wrote:
> On 2013/04/17 22:38:35, keybuk wrote:
> > On 2013/04/11 18:33:41, youngki wrote:
> > > Shouldn't the destructor be protected? Otherwise the object won't get
> > > destroyed...
> > 
> > Unregister() should be doing the deletion - so it seems appropriate for it
to
> be
> > Private
> 
> I think you are trying to follow RefCounted model where the destructor is
> private and RefCounted<>::Release deletes itself, right? Could you explain
what
> is the benefit of using Unregister() instead of destructor to delete the
object?
> Are we trying to count the references to the object?

I don't like patterns where you get an object via a method, but are expected to
"delete" it - e.g

obj = Foo::Register();
delete obj;

delete is fine if you used new:

obj = new Foo;
delete obj;

but for the former, I tend to prefer:

obj = Foo::Register();
obj->Unregister();

I'm totally okay to go with delete if that's Google style, but working on the
D-Bus code (which uses Unregister() or Detach() like this) led me to believe I
was already ;)

https://chromiumcodereview.appspot.com/13862023/diff/18001/device/bluetooth/b...
File device/bluetooth/bluetooth_profile.cc (right):

https://chromiumcodereview.appspot.com/13862023/diff/18001/device/bluetooth/b...
device/bluetooth/bluetooth_profile.cc:10: 
On 2013/04/19 16:26:36, youngki wrote:
> Don't we have to put the BluetoothProfile::Options constructor implementation
> here as well? Or make them an inline constructor?

Yes, this is still "draft to see if youngki approves" :)

[youngki, 2013-04-19 18:20:39]

lgtm

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.h (right):

https://chromiumcodereview.appspot.com/13862023/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.h:93: virtual ~BluetoothProfile();
On 2013/04/19 17:58:18, keybuk wrote:
> On 2013/04/19 16:26:36, youngki wrote:
> > On 2013/04/17 22:38:35, keybuk wrote:
> > > On 2013/04/11 18:33:41, youngki wrote:
> > > > Shouldn't the destructor be protected? Otherwise the object won't get
> > > > destroyed...
> > > 
> > > Unregister() should be doing the deletion - so it seems appropriate for it
> to
> > be
> > > Private
> > 
> > I think you are trying to follow RefCounted model where the destructor is
> > private and RefCounted<>::Release deletes itself, right? Could you explain
> what
> > is the benefit of using Unregister() instead of destructor to delete the
> object?
> > Are we trying to count the references to the object?
> 
> I don't like patterns where you get an object via a method, but are expected
to
> "delete" it - e.g
> 
> obj = Foo::Register();
> delete obj;
> 
> delete is fine if you used new:
> 
> obj = new Foo;
> delete obj;
> 
> but for the former, I tend to prefer:
> 
> obj = Foo::Register();
> obj->Unregister();
> 
> I'm totally okay to go with delete if that's Google style, but working on the
> D-Bus code (which uses Unregister() or Detach() like this) led me to believe I
> was already ;)

sounds good

https://chromiumcodereview.appspot.com/13862023/diff/18001/device/bluetooth/b...
File device/bluetooth/bluetooth_profile.cc (right):

https://chromiumcodereview.appspot.com/13862023/diff/18001/device/bluetooth/b...
device/bluetooth/bluetooth_profile.cc:10: 
On 2013/04/19 17:58:18, keybuk wrote:
> On 2013/04/19 16:26:36, youngki wrote:
> > Don't we have to put the BluetoothProfile::Options constructor
implementation
> > here as well? Or make them an inline constructor?
> 
> Yes, this is still "draft to see if youngki approves" :)

ship it!

[keybuk, 2013-04-19 18:22:32]

Oh, and the other reason:

I figured Unregister might actually be asynchronous on some platforms - so it's
more "transfer ownership back to the implementation", then it can make method
calls, etc. and later on delete it

[youngki, 2013-04-19 18:23:35]

lgtm

[commit-bot: I haz the power, 2013-04-19 18:38:35]

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

[commit-bot: I haz the power, 2013-04-19 21:07:22]

Change committed as 195280