Implement Bluetooth server socket support for RFCOMM on Mac.

Implement Bluetooth server socket support for RFCOMM on Mac.

BUG=364409
TEST=Able to receive incomming connections via the chrome.bluetooth API.
R=keybuk@chromium.org, rpaquay@chromium.org

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

[Ilya Sherman, 2014-04-18 23:37:54]

Renaud, PTAL.

keybuk@, I've included you as an FYI, though I of course also welcome your
comments if you would like to review the CL.

[Ilya Sherman, 2014-04-19 05:11:25]

https://codereview.chromium.org/243963002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_profile_mac.mm (right):

https://codereview.chromium.org/243963002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_mac.mm:155: // service.
Is this appropriate, or is the channel provided in options intended to merely be
a hint?

https://codereview.chromium.org/243963002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_mac.mm:247: // service, i.e. if
|service_record_handle_| is |kInvalidServiceRecordHandle|?
^^^ thoughts?

https://codereview.chromium.org/243963002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_mac.mm:310: // functions essentially as a
minor memory leak.
^^^ Thoughts?

[keybuk, 2014-04-19 07:54:05]

https://codereview.chromium.org/243963002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_profile_mac.mm (right):

https://codereview.chromium.org/243963002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_mac.mm:155: // service.
On 2014/04/19 05:11:25, Ilya Sherman wrote:
> Is this appropriate, or is the channel provided in options intended to merely
be
> a hint?

When specified, it should be equivalent to requesting a TCP Port - so this is
appropriate.

When not specified, it should be equivalent to "bind to any free"

[Ilya Sherman, 2014-04-22 00:27:20]

https://codereview.chromium.org/243963002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_profile_mac.mm (right):

https://codereview.chromium.org/243963002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_mac.mm:155: // service.
On 2014/04/19 07:54:05, keybuk wrote:
> On 2014/04/19 05:11:25, Ilya Sherman wrote:
> > Is this appropriate, or is the channel provided in options intended to
merely
> be
> > a hint?
> 
> When specified, it should be equivalent to requesting a TCP Port - so this is
> appropriate.
> 
> When not specified, it should be equivalent to "bind to any free"

Sounds good.  I've added a TODO for now, as there chrome.bluetooth API doesn't
currently offer a clear way to register an RFCOMM service without specifying a
channel explicitly.

[rpaquay, 2014-04-23 22:15:55]

lgtm

https://codereview.chromium.org/243963002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_profile_mac.mm (right):

https://codereview.chromium.org/243963002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_mac.mm:251: // service, i.e. if
|service_record_handle_| is |kInvalidServiceRecordHandle|?
Ideally, we would have a way to report the error to the Chrome App (via
chrome.runtime.lastError), similar to the way we report error on
"ConnectToProfile". Given the future churn with BluetoothProfile, this can be
done in a later CL, but I think we should have a tracking bug.

https://codereview.chromium.org/243963002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_mac.mm:314: // functions essentially as a
minor memory leak.
These issues are serious imho. Do we have a bug tracking them?

https://codereview.chromium.org/243963002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_mac.mm (right):

https://codereview.chromium.org/243963002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_mac.mm:125: }
There is little common code between a client and a server socket, which makes
reasoning about this code somewhat tricky. Out of curiosity, why did you decide
to not create a separate class?

[Ilya Sherman, 2014-04-24 06:50:10]

Thanks!  I'll file bugs and update the TODOs to reference the bugs prior to
committing.  (wfh/sick right now, which is why I haven't already done so)

https://codereview.chromium.org/243963002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_mac.mm (right):

https://codereview.chromium.org/243963002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_mac.mm:125: }
On 2014/04/23 22:15:55, rpaquay wrote:
> There is little common code between a client and a server socket, which makes
> reasoning about this code somewhat tricky. Out of curiosity, why did you
decide
> to not create a separate class?

I actually originally planned to create a separate class, but to my surprise, I
found that most of the code does make sense to share between the client and
server socket.  Essentially, the only difference between the two is how the
socket is connected, which is the Connect() vs AcceptConnection() methods. 
Beyond that, the sockets are bi-directional, so all of the code for reading and
writing ends up being shared.

I think all of the code that's BluetoothProfileMac right now will need to end up
in a separate server-type class for registering/unregistering a service, as well
as listening to incoming connections.  That's one level up from the socket
class, though.

[Ilya Sherman, 2014-04-25 22:44:44]

https://chromiumcodereview.appspot.com/243963002/diff/40001/device/bluetooth/...
File device/bluetooth/bluetooth_profile_mac.mm (right):

https://chromiumcodereview.appspot.com/243963002/diff/40001/device/bluetooth/...
device/bluetooth/bluetooth_profile_mac.mm:251: // service, i.e. if
|service_record_handle_| is |kInvalidServiceRecordHandle|?
On 2014/04/23 22:15:55, rpaquay wrote:
> Ideally, we would have a way to report the error to the Chrome App (via
> chrome.runtime.lastError), similar to the way we report error on
> "ConnectToProfile". Given the future churn with BluetoothProfile, this can be
> done in a later CL, but I think we should have a tracking bug.

Filed a bug and referenced it here.

https://chromiumcodereview.appspot.com/243963002/diff/40001/device/bluetooth/...
device/bluetooth/bluetooth_profile_mac.mm:314: // functions essentially as a
minor memory leak.
On 2014/04/23 22:15:55, rpaquay wrote:
> These issues are serious imho. Do we have a bug tracking them?

Filed bugs and referenced them here.

[Ilya Sherman, 2014-04-25 22:45:06]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:47:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/243963002/60001

[commit-bot: I haz the power, 2014-04-25 23:54:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:54:20]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Ilya Sherman, 2014-04-25 23:57:50]

keybuk@, it looks like I need your approval for the changes to
bluetooth_profile.cc.  PTAL.

[keybuk, 2014-04-27 08:15:50]

lgtm

[Ilya Sherman, 2014-04-28 17:38:34]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-28 17:39:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/243963002/60001

[commit-bot: I haz the power, 2014-04-28 17:45:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 17:45:57]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Ilya Sherman, 2014-04-28 17:56:56]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:02:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/243963002/80001

[commit-bot: I haz the power, 2014-04-28 18:25:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:25:35]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[Ilya Sherman, 2014-04-28 18:27:25]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:28:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/243963002/80001

[commit-bot: I haz the power, 2014-04-28 19:27:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:27:35]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Ilya Sherman, 2014-04-28 19:35:00]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:36:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/243963002/80001

[commit-bot: I haz the power, 2014-04-28 23:54:21]

Change committed as 266698