Implement chrome.bluetoothSocket.listenUsing*()

Implement chrome.bluetoothSocket.listenUsing*()

Includes the C++ BluetoothAdapter and BluetoothSocket API changes,
but does not include platform-specific implementations. Tests are
included since there is sufficient mock code to drive a test.

BUG=349475, 364581
TEST=browser_tests --gtest_filter=BluetoothSocketApiTest.Listen

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

[keybuk, 2014-05-08 21:50:24]

armansito: for /bluetooth/
rpaquay: for API review and _win
isherman: for _mac
iserhman + miket: for IDL change, making listen() consistent with other methods
and using chrome.runtime.lastError

[armansito, 2014-05-08 22:00:15]

lgtm

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_api.cc
(right):

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_api.cc:48: if
(socket->IsConnected())
Does IsConnected imply listening?

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc
(right):

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc:148:
} else if (socket->IsConnected()) {
nit: No need for else since the previous block returns.

[keybuk, 2014-05-08 22:02:46]

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_api.cc
(right):

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_api.cc:48: if
(socket->IsConnected())
On 2014/05/08 22:00:16, armansito wrote:
> Does IsConnected imply listening?

A BluetoothApiSocket can be:
 - created
 - connected
 - listening
 - disconnected

IsConnected() only matches the second one - that's also the only state for which
the address member would be valid.

[armansito, 2014-05-08 22:25:14]

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_api.cc
(right):

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_api.cc:48: if
(socket->IsConnected())
On 2014/05/08 22:02:46, keybuk wrote:
> On 2014/05/08 22:00:16, armansito wrote:
> > Does IsConnected imply listening?
> 
> A BluetoothApiSocket can be:
>  - created
>  - connected
>  - listening
>  - disconnected
> 
> IsConnected() only matches the second one - that's also the only state for
which
> the address member would be valid.

OK. I commented on this because your previous comment said "isn't connected or
listening". So the removed comment was incorrect or do we want to check for
listening as well? Does listening mean that it's listening for incoming
connections and hence there is no address associated with it?

[keybuk, 2014-05-08 22:32:36]

On 2014/05/08 22:25:14, armansito wrote:

> OK. I commented on this because your previous comment said "isn't connected or
> listening". So the removed comment was incorrect or do we want to check for
> listening as well? Does listening mean that it's listening for incoming
> connections and hence there is no address associated with it?

The removed TODO didn't quite reflect the implementation I ended up doing.

No need to check for "listening" state here for the address.

[armansito, 2014-05-08 22:33:51]

On 2014/05/08 22:32:36, keybuk wrote:
> On 2014/05/08 22:25:14, armansito wrote:
> 
> > OK. I commented on this because your previous comment said "isn't connected
or
> > listening". So the removed comment was incorrect or do we want to check for
> > listening as well? Does listening mean that it's listening for incoming
> > connections and hence there is no address associated with it?
> 
> The removed TODO didn't quite reflect the implementation I ended up doing.
> 
> No need to check for "listening" state here for the address.

sgtm

[rpaquay, 2014-05-09 16:09:52]

lgtm

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc
(right):

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc:311:
// This happens when resuming a socket which already had an active "acce0t"
accept?

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc:317:
// Dispatch "onAcceptError" event but don't start another read to avoid
read => accept

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc:318:
// potential infinite reads if we have a persistent network error.
reads => accepts

[keybuk, 2014-05-09 18:28:23]

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc
(right):

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc:311:
// This happens when resuming a socket which already had an active "acce0t"
On 2014/05/09 16:09:53, rpaquay wrote:
> accept?

Done.

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc:317:
// Dispatch "onAcceptError" event but don't start another read to avoid
On 2014/05/09 16:09:53, rpaquay wrote:
> read => accept

Done.

https://codereview.chromium.org/278663002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc:318:
// potential infinite reads if we have a persistent network error.
On 2014/05/09 16:09:53, rpaquay wrote:
> reads => accepts

Done.

[Ilya Sherman, 2014-05-09 23:44:14]

Mac changes lgtm.

https://codereview.chromium.org/278663002/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_mac.h (right):

https://codereview.chromium.org/278663002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_mac.h:65: const ErrorCompletionCallback&
error_callback) OVERRIDE;
This method name is a bit confusing in this class, since there's already an
AcceptConnection method, but I guess that's ok.

[keybuk, 2014-05-09 23:44:50]

The CQ bit was checked by keybuk@chromium.org

[commit-bot: I haz the power, 2014-05-09 23:48:25]

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

[commit-bot: I haz the power, 2014-05-10 01:55:34]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-05-10 02:02:23]

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

[commit-bot: I haz the power, 2014-05-10 02:02:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[keybuk, 2014-05-10 02:08:37]

ah, missed LGTM from miket or kalman for IDL change

[not at google - send to devlin, 2014-05-12 17:09:25]

lgtm

[keybuk, 2014-05-12 17:10:01]

The CQ bit was checked by keybuk@chromium.org

[commit-bot: I haz the power, 2014-05-12 17:11:10]

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

[commit-bot: I haz the power, 2014-05-12 18:09:55]

Change committed as 269814