Add support for creating bluetooth RFCOMM sockets.

Add support for creating bluetooth RFCOMM sockets.

BUG=119473
TEST=added some unittests

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

[bryeung, 2012-04-11 21:33:10]

Scott: have you had a chance to look at this?

[keybuk, 2012-04-16 22:25:36]

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:417: delete socket;
Could this be encapsulated as simply dropping the reference to the socket from
the map?

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:423: const std::string
&service_uuid) {
nit: "& " not " &"

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:442: delete socket;
As above, could this be a drop of a reference rather than an explicit delete?

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:245:
base::WeakPtr<BluetoothSocket> OpenSocket(const std::string &service_uuid);
nit "& " not " &"

Should this be a weak ptr or a refcounted pointer?

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:248: void CloseSocket(int
socket_id);
If BluetoothSocket were refcounted, we could encapsulate close as "the last
owner dropped the socket"

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:494: SocketMap socket_map_;
What's the int here? the fd?

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.cc (right):

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_socket.cc:12: BluetoothDevice
*device, const std::string &service_uuid, int id)
nit: "* " and "& " not " *" and " &"

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_socket.cc:16: // TODO(keybuk): dbus
stuff
no dbus here, all socket ;)

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:22: int id);
nit: "* " and "& " not " *" and " &"

Public constructor suggests it's ok to instantiate these directly, rather than
going through BluetoothDevice.

Maybe better to have a private constructor and make BluetoothDevice a friend?

What is "id" ?

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:26: int id() const { return
id_; }
What is id? I tend to dislike arbitrary lookup types.

Is this the file descriptor? It should just be named as such

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:29: void Close();
RAII: Close should be the destructor.

https://chromiumcodereview.appspot.com/10007008/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:34: const int id_;
Is this going to be derivable? How will we deal with the differences between
RFCOMM and L2CAP?

[bryeung, 2012-04-18 20:30:31]

ptal

This isn't done yet, but I'd like to get some more early feedback.

The biggest known issues are:
1) It still needs testing.  (I think it's right, but need to do more to verify).
2) The extensions part is not done (no one holds onto the BluetoothSocket right
now), and needs to be split off into it's own CL anyway.
3) It depends on libbluetooth (for strtoba).  Any suggestions how to get around
importing bluez into third_party?

[keybuk, 2012-04-19 01:05:39]

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:470: // TODO(bryeung): add
support for L2CAP sockets as well.
This seems to be in the wrong place, I was expecting this code to be in
BluetoothSocket's constructor, I think it would be better there

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:482: // If EINPROGRESS,
optimisticly assume that it will succeed eventually
I believe EINPROGRESS is the only likely return value in the case of a
non-blocking connect, so no need for the comment

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:485: if (status == 0 ||
status == EINPROGRESS) {
connect doesn't return EINPROGRESS, it sets errno and returns -1 ?

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:505: if
(!service_record.Init(i->second))
Didn't we drop this Init stuff for a constructor? This looks like old code

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:507: 
You don't check if this is an rfcomm service?

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:509: service_uuid,
service_record.rfcomm_channel());
Would be cleaner if this was a BluetoothSocket constructor

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:255:
base::Callback<void(scoped_refptr<BluetoothSocket>)> callback);
Comment should explain ownership, that the socket can outlive the adapter, etc.

Callback should be a typedef (SocketCallback maybe)

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:337: uint8_t channel);
this seems RFCOMM specific (channel), L2CAP uses PSMs

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:345:
base::Callback<void(scoped_refptr<BluetoothSocket>)> callback,
typedef

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:40:
supports_rfcomm_ = false;
Isn't this in the constructor now

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:57: }
may as well add L2CAP + L2CAP PSM while you're in here

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_service_record.h (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.h:21:
BluetoothServiceRecord() : supports_rfcomm_(false) {}
This should move to the .cc file

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.h:28: uint8_t
rfcomm_channel() const { return rfcomm_channel_; }
add L2CAP and L2CAP PSM as well?

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.cc (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_socket.cc:13: : device_(device),
service_uuid_(service_uuid), fd_(fd) {}
place initializers on separate lines for clarity

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:18: // a BluetoothDevice.
Comment should explain about ref counting.

[keybuk, 2012-04-19 01:06:07]

if strtoba is the only thing from libbluetooth we're using, that can be
hand-reimplemented easily enough

[bryeung, 2012-04-19 01:09:17]

Yes: sorry.  I updated this CL before we finished iterating on the
previous one.  I'll rebase, integrate your comments, fix up the
strtoba stuff and update this tomorrow morning.  Hopefully I can get
it done by lunch :-)

Thanks for all the review work today!

Bryan

On Wed, Apr 18, 2012 at 9:05 PM,  <keybuk@chromium.org> wrote:
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.cc:470: //
> TODO(bryeung): add support for L2CAP sockets as well.
> This seems to be in the wrong place, I was expecting this code to be in
> BluetoothSocket's constructor, I think it would be better there
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.cc:482: // If
> EINPROGRESS, optimisticly assume that it will succeed eventually
> I believe EINPROGRESS is the only likely return value in the case of a
> non-blocking connect, so no need for the comment
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.cc:485: if (status ==
> 0 || status == EINPROGRESS) {
> connect doesn't return EINPROGRESS, it sets errno and returns -1 ?
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.cc:505: if
> (!service_record.Init(i->second))
> Didn't we drop this Init stuff for a constructor? This looks like old
> code
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.cc:507:
> You don't check if this is an rfcomm service?
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.cc:509: service_uuid,
> service_record.rfcomm_channel());
> Would be cleaner if this was a BluetoothSocket constructor
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.h:255:
> base::Callback<void(scoped_refptr<BluetoothSocket>)> callback);
> Comment should explain ownership, that the socket can outlive the
> adapter, etc.
>
> Callback should be a typedef (SocketCallback maybe)
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.h:337: uint8_t
> channel);
> this seems RFCOMM specific (channel), L2CAP uses PSMs
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.h:345:
> base::Callback<void(scoped_refptr<BluetoothSocket>)> callback,
> typedef
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> File chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc
> (right):
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:40:
> supports_rfcomm_ = false;
> Isn't this in the constructor now
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:57: }
> may as well add L2CAP + L2CAP PSM while you're in here
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> File chrome/browser/chromeos/bluetooth/bluetooth_service_record.h
> (right):
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_service_record.h:21:
> BluetoothServiceRecord() : supports_rfcomm_(false) {}
> This should move to the .cc file
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_service_record.h:28: uint8_t
> rfcomm_channel() const { return rfcomm_channel_; }
> add L2CAP and L2CAP PSM as well?
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> File chrome/browser/chromeos/bluetooth/bluetooth_socket.cc (right):
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_socket.cc:13: :
> device_(device), service_uuid_(service_uuid), fd_(fd) {}
> place initializers on separate lines for clarity
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):
>
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_socket.h:18: // a
> BluetoothDevice.
> Comment should explain about ref counting.
>
> http://codereview.chromium.org/10007008/

[keybuk, 2012-04-19 01:10:40]

NP :) thanks for your work on this, it's great to be getting this in

On Wed, Apr 18, 2012 at 6:09 PM, Bryan Yeung <bryeung@chromium.org> wrote:

> Yes: sorry.  I updated this CL before we finished iterating on the
> previous one.  I'll rebase, integrate your comments, fix up the
> strtoba stuff and update this tomorrow morning.  Hopefully I can get
> it done by lunch :-)
>
> Thanks for all the review work today!
>
> Bryan
>
> On Wed, Apr 18, 2012 at 9:05 PM,  <keybuk@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.cc:470: //
> > TODO(bryeung): add support for L2CAP sockets as well.
> > This seems to be in the wrong place, I was expecting this code to be in
> > BluetoothSocket's constructor, I think it would be better there
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.cc:482: // If
> > EINPROGRESS, optimisticly assume that it will succeed eventually
> > I believe EINPROGRESS is the only likely return value in the case of a
> > non-blocking connect, so no need for the comment
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.cc:485: if (status ==
> > 0 || status == EINPROGRESS) {
> > connect doesn't return EINPROGRESS, it sets errno and returns -1 ?
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.cc:505: if
> > (!service_record.Init(i->second))
> > Didn't we drop this Init stuff for a constructor? This looks like old
> > code
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.cc:507:
> > You don't check if this is an rfcomm service?
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.cc:509: service_uuid,
> > service_record.rfcomm_channel());
> > Would be cleaner if this was a BluetoothSocket constructor
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.h:255:
> > base::Callback<void(scoped_refptr<BluetoothSocket>)> callback);
> > Comment should explain ownership, that the socket can outlive the
> > adapter, etc.
> >
> > Callback should be a typedef (SocketCallback maybe)
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.h:337: uint8_t
> > channel);
> > this seems RFCOMM specific (channel), L2CAP uses PSMs
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_device.h:345:
> > base::Callback<void(scoped_refptr<BluetoothSocket>)> callback,
> > typedef
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > File chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc
> > (right):
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:40:
> > supports_rfcomm_ = false;
> > Isn't this in the constructor now
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:57: }
> > may as well add L2CAP + L2CAP PSM while you're in here
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > File chrome/browser/chromeos/bluetooth/bluetooth_service_record.h
> > (right):
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_service_record.h:21:
> > BluetoothServiceRecord() : supports_rfcomm_(false) {}
> > This should move to the .cc file
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_service_record.h:28: uint8_t
> > rfcomm_channel() const { return rfcomm_channel_; }
> > add L2CAP and L2CAP PSM as well?
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > File chrome/browser/chromeos/bluetooth/bluetooth_socket.cc (right):
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_socket.cc:13: :
> > device_(device), service_uuid_(service_uuid), fd_(fd) {}
> > place initializers on separate lines for clarity
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):
> >
> >
>
http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
> > chrome/browser/chromeos/bluetooth/bluetooth_socket.h:18: // a
> > BluetoothDevice.
> > Comment should explain about ref counting.
> >
> > http://codereview.chromium.org/10007008/
>

[bryeung, 2012-04-19 19:42:43]

Got a bit distracted cleaning this up and adding some unit tests.  I think it's
ready for another look.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:470: // TODO(bryeung): add
support for L2CAP sockets as well.
On 2012/04/19 01:05:40, keybuk wrote:
> This seems to be in the wrong place, I was expecting this code to be in
> BluetoothSocket's constructor, I think it would be better there

I put the code here because I wasn't sure how to deal with failures if this was
in BluetoothSocket's constructor (because this code can return NULL).

But now that you mention this, I do agree it makes more sense in
BluetoothSocket.  I've made the constructor private and added a static function
(which can return null) to create BluetoothSockets.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:482: // If EINPROGRESS,
optimisticly assume that it will succeed eventually
On 2012/04/19 01:05:40, keybuk wrote:
> I believe EINPROGRESS is the only likely return value in the case of a
> non-blocking connect, so no need for the comment

Done.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:485: if (status == 0 ||
status == EINPROGRESS) {
On 2012/04/19 01:05:40, keybuk wrote:
> connect doesn't return EINPROGRESS, it sets errno and returns -1 ?

Oops.  Good catch.  Thanks.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:505: if
(!service_record.Init(i->second))
On 2012/04/19 01:05:40, keybuk wrote:
> Didn't we drop this Init stuff for a constructor? This looks like old code

Yep: we hadn't finished iterating on the other CL when I updated this one. 
Should look better now.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:507: 
On 2012/04/19 01:05:40, keybuk wrote:
> You don't check if this is an rfcomm service?

Done.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:509: service_uuid,
service_record.rfcomm_channel());
On 2012/04/19 01:05:40, keybuk wrote:
> Would be cleaner if this was a BluetoothSocket constructor

Done.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:255:
base::Callback<void(scoped_refptr<BluetoothSocket>)> callback);
On 2012/04/19 01:05:40, keybuk wrote:
> Comment should explain ownership, that the socket can outlive the adapter,
etc.
> 
> Callback should be a typedef (SocketCallback maybe)

Done.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:337: uint8_t channel);
On 2012/04/19 01:05:40, keybuk wrote:
> this seems RFCOMM specific (channel), L2CAP uses PSMs

Yep: I was going to make it more generic when I add L2CAP support later.  My
priority right now is to get RFCOMM completed because I need it for the current
2-week apps team sprint.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:345:
base::Callback<void(scoped_refptr<BluetoothSocket>)> callback,
On 2012/04/19 01:05:40, keybuk wrote:
> typedef

Done.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:40:
supports_rfcomm_ = false;
On 2012/04/19 01:05:40, keybuk wrote:
> Isn't this in the constructor now

Yep :-)

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:57: }
On 2012/04/19 01:05:40, keybuk wrote:
> may as well add L2CAP + L2CAP PSM while you're in here

future CL?

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_service_record.h (right):

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.h:21:
BluetoothServiceRecord() : supports_rfcomm_(false) {}
On 2012/04/19 01:05:40, keybuk wrote:
> This should move to the .cc file

Done.

http://codereview.chromium.org/10007008/diff/10001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.h:28: uint8_t
rfcomm_channel() const { return rfcomm_channel_; }
On 2012/04/19 01:05:40, keybuk wrote:
> add L2CAP and L2CAP PSM as well?

Can that wait for a future CL?  I'm focused on RFCOMM for the moment because
that's what I need for the current apps sprint.

[keybuk, 2012-04-19 23:30:11]

almost there! the use of a BluetoothServiceRecord as the arg to
BluetoothSocket::CreateSocket is perfect

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:348: uint8_t channel);
OpenSocket is gone now, right?

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc (right):

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:48: const
std::string& xml_data)
nit: style guide requires that the first arg be on a new line, I believe

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.cc (right):

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_socket.cc:20: // static
usually these go at the end of the file rather than the top

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:33: virtual
~BluetoothSocket();
destructor should be public?

[bryeung, 2012-04-20 00:17:56]

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:348: uint8_t channel);
On 2012/04/19 23:30:11, keybuk wrote:
> OpenSocket is gone now, right?

Oops!  Good catch.

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc (right):

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_service_record.cc:48: const
std::string& xml_data)
On 2012/04/19 23:30:11, keybuk wrote:
> nit: style guide requires that the first arg be on a new line, I believe

Done.

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.cc (right):

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_socket.cc:20: // static
On 2012/04/19 23:30:11, keybuk wrote:
> usually these go at the end of the file rather than the top

Done.

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):

http://codereview.chromium.org/10007008/diff/23001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:33: virtual
~BluetoothSocket();
On 2012/04/19 23:30:11, keybuk wrote:
> destructor should be public?

I don't think so...

From the comments above RefCounted in base/memory/ref_counted.h:

"You should always make your destructor private, to avoid any code deleting the
object accidently while there are references to it."

[keybuk, 2012-04-20 00:25:31]

lgtm

[commit-bot: I haz the power, 2012-04-20 00:29:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10007008/28002

[commit-bot: I haz the power, 2012-04-20 00:59:44]

Try job failure for 10007008-28002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-20 23:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10007008/28002

[commit-bot: I haz the power, 2012-04-21 00:06:20]

Try job failure for 10007008-28002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-21 10:35:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10007008/28002

[bryeung, 2012-04-21 10:46:45]

Ugh: sorry for all the spam guys.

I'm still working with the troopers to get the try servers appropriately updated
to be able to build this change.

[commit-bot: I haz the power, 2012-04-21 13:37:43]

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

[commit-bot: I haz the power, 2012-04-21 17:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10007008/28002

[commit-bot: I haz the power, 2012-04-21 20:06:39]

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

[commit-bot: I haz the power, 2012-04-23 17:15:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10007008/28002

[commit-bot: I haz the power, 2012-04-23 18:42:56]

Change committed as 133485