chrome.bluetooth API improvements.

* Replace "read" method with onReceiveXxx events.
* Few minor changes to idl file.
* Socket implementation on Windows fully implemented and with (hopefully) correct threading usage.
* ChromeOS and MacOS socket implementation empty for now (they will come in later CLs).

BUG=336824, 340363, 340413, 343651, 344081
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262175

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

[rpaquay, 2014-03-13 22:47:27]

keybuk@chromium.org: Please review changes in 
chrome/browser/extensions/api/bluetooth/
device/bluetooth/

miket@chromium.org: Please review changes in 
chrome/common/extensions/api/bluetooth.idl
extensions/browser/api/

[keybuk, 2014-03-14 17:58:07]

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:41: // Information about a bluetooth
profile -- or service.
Bluetooth profile/service terminology isn't quite like other systems, we should
avoid using "service" like this to avoid confusion for people who know Bluetooth
well and are intending to implement a Chrome client.

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:260: optional SizeCallback callback);
The chrome.sockets.tcp.send() method takes the socketId and arrayBuffer as
separate arguments, not grouped into the options object -- this should match
that other API

[rpaquay, 2014-03-14 18:27:49]

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:41: // Information about a bluetooth
profile -- or service.
On 2014/03/14 17:58:07, keybuk wrote:
> Bluetooth profile/service terminology isn't quite like other systems, we
should
> avoid using "service" like this to avoid confusion for people who know
Bluetooth
> well and are intending to implement a Chrome client.

Ok, so stick with "profile" term only?

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:260: optional SizeCallback callback);
On 2014/03/14 17:58:07, keybuk wrote:
> The chrome.sockets.tcp.send() method takes the socketId and arrayBuffer as
> separate arguments, not grouped into the options object -- this should match
> that other API

I had a chat with Kalman@ about that. We are between a rock and a hard place:
the bluetooth api uses options everywhere, so we should be consistent with that,
but other api (tcp, etc.) don't, so we should be consistent with that. At the
time, we agreed that consistency within this idl file was (slightly) higher
order than consistency with other idl files.

That said, given the work has grown since then, maybe we should consider getting
rid of all "options" classes and use simple parameters everywhere? (I would do
that in a later CL, though).

[keybuk, 2014-03-14 19:47:06]

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:41: // Information about a bluetooth
profile -- or service.
Yeah, and hope to get rid of that term as well

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:260: optional SizeCallback callback);
I would prefer the Bluetooth API moved towards getting rid of the options
classes, except where they co-incide with the others - since we're already
reworking this API anyway

No need to do that in this CL, we can do it in subsequent CLs

[rpaquay, 2014-03-14 20:30:56]

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:41: // Information about a bluetooth
profile -- or service.
On 2014/03/14 19:47:06, keybuk wrote:
> Yeah, and hope to get rid of that term as well

OK, I'll change this comment and remove "service".

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:260: optional SizeCallback callback);
On 2014/03/14 19:47:06, keybuk wrote:
> I would prefer the Bluetooth API moved towards getting rid of the options
> classes, except where they co-incide with the others - since we're already
> reworking this API anyway
> 
> No need to do that in this CL, we can do it in subsequent CLs

Agreed: crbug/352775

[keybuk, 2014-03-14 21:13:48]

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:365: const net::NetLog::Source& source,
What's the justification for these added parameters?

Are they specific to the resulting BluetoothSocket? In which case why aren't
they part of the BluetoothSocket API?

Or are they specific to the Windows implementation? If not, how would other
platforms, e.g. Chrome OS, use these?

Would different calls to ConnectProfile() expect to pass different parameters
for the task_runners, net_log, source, etc.? Or would they all pass the same?

What about when we move to having ConnectL2cap(), ConnectRfcomm(),
ListenL2cap(), ListenRfcomm() etc. would those need these arguments too?

What about other async functions?

Basically should these be moved towards the original BluetoothAdapter
constructor ?

[rpaquay, 2014-03-14 21:48:09]

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:365: const net::NetLog::Source& source,
On 2014/03/14 21:13:48, keybuk wrote:
> What's the justification for these added parameters?
> 
> Are they specific to the resulting BluetoothSocket? In which case why aren't
> they part of the BluetoothSocket API?
> 
> Or are they specific to the Windows implementation? If not, how would other
> platforms, e.g. Chrome OS, use these?
> 
> Would different calls to ConnectProfile() expect to pass different parameters
> for the task_runners, net_log, source, etc.? Or would they all pass the same?
> 
> What about when we move to having ConnectL2cap(), ConnectRfcomm(),
> ListenL2cap(), ListenRfcomm() etc. would those need these arguments too?
> 
> What about other async functions?
> 
> Basically should these be moved towards the original BluetoothAdapter
> constructor ?

That is a good point, the task runners definitely could move to the constructor
(I assume you mean "BluetoothDevice" not "BluetoothAdapter"). As for the
net_log/source, I am not 100% sure, as I don't quite understand their usage at
this point. Other Chrome APPS apis seem to happily pass "NULL, DefaultSource()"
when using TCPSocket, but I have a feeling this is not quite correct either. In
any ways, I think the decision should probably not be made at the "device"
layer, but at a higher layer, hence the need to pass them as parameters
somewhere (here or constructor).

[keybuk, 2014-03-14 21:50:52]

On 2014/03/14 21:48:09, rpaquay wrote:
> That is a good point, the task runners definitely could move to the
constructor
> (I assume you mean "BluetoothDevice" not "BluetoothAdapter").
> 
Since BluetoothDevice is only constructed from BluetoothAdapter, they'd need to
be there.

This "smells" like the similar kind of approach taken on Chrome OS when dealing
with the underlying D-Bus Thread Manager and "Client" classes used by that
implementation. They're handled during the BluetoothAdapterChromeOS
construction, and passed along to the BluetoothDeviceChromeOS etc. classes when
needed.

> As for the
> net_log/source, I am not 100% sure, as I don't quite understand their usage at
> this point. Other Chrome APPS apis seem to happily pass "NULL,
DefaultSource()"
> when using TCPSocket, but I have a feeling this is not quite correct either.
In
> any ways, I think the decision should probably not be made at the "device"
> layer, but at a higher layer, hence the need to pass them as parameters
> somewhere (here or constructor).

Are these used for logging internally in the classes? Would we want to log e.g.
SDP discovery to them? If that's done at device creation time to populate the
set of UUIDs for the device, they'll need to be around at BluetoothDevice
construction, which again implies being around at BluetoothAdapter construction

[rpaquay, 2014-03-14 22:21:28]

On 2014/03/14 21:50:52, keybuk wrote:
> On 2014/03/14 21:48:09, rpaquay wrote:
> > That is a good point, the task runners definitely could move to the
> constructor
> > (I assume you mean "BluetoothDevice" not "BluetoothAdapter").
> > 
> Since BluetoothDevice is only constructed from BluetoothAdapter, they'd need
to
> be there.
> 
> This "smells" like the similar kind of approach taken on Chrome OS when
dealing
> with the underlying D-Bus Thread Manager and "Client" classes used by that
> implementation. They're handled during the BluetoothAdapterChromeOS
> construction, and passed along to the BluetoothDeviceChromeOS etc. classes
when
> needed.
> 
> > As for the
> > net_log/source, I am not 100% sure, as I don't quite understand their usage
at
> > this point. Other Chrome APPS apis seem to happily pass "NULL,
> DefaultSource()"
> > when using TCPSocket, but I have a feeling this is not quite correct either.
> In
> > any ways, I think the decision should probably not be made at the "device"
> > layer, but at a higher layer, hence the need to pass them as parameters
> > somewhere (here or constructor).
> 
> Are these used for logging internally in the classes? Would we want to log
e.g.
> SDP discovery to them? If that's done at device creation time to populate the
> set of UUIDs for the device, they'll need to be around at BluetoothDevice
> construction, which again implies being around at BluetoothAdapter
construction

net_log afaik is used to log network activity in Chrome in general (I think
that's what showing in chrome://net-internals/#events). We haven't really payed
attention to that for Chrome Apps API until now I believe.

So, anyway, I will moves these parameters to the constructor, I agree this will
make the interface much cleaner.

[xiyuan, 2014-03-17 22:55:10]

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_task_manager_win.cc (right):

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
device/bluetooth/bluetooth_task_manager_win.cc:465: device_address,
L2CAP_PROTOCOL_UUID, service_record_states);
It seems we only need enumerate L2CAP_PROTOCOL_UUID as in previous code.

Previous code has a bug that |sdp_buffer_size| might be changed after returning
from WSALookupServiceNext. Sometime |sdp_buffer_size| becomes too small and
causes service enumeration to fail with WSAEFAULT.

With that fixed (i.e. "sdp_buffer_size = sizeof(sdp_buffer)" at the end of the
WSALookupServiceNext loop and before calling it again), the loop gets all the
services, including the OBEXObjectPushServiceClass_UUID and I don't need to run
the loop for all other protocol UUIDs.

[rpaquay, 2014-03-20 00:48:17]

PTAL:

* Address code review feedback
* Make a few changes needed for the MacOS implementation (CL coming soon after
this hopefully).
* Fix issues arising from (a lot of) recent merge conflicts.

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:41: // Information about a bluetooth
profile -- or service.
On 2014/03/14 20:30:56, rpaquay wrote:
> On 2014/03/14 19:47:06, keybuk wrote:
> > Yeah, and hope to get rid of that term as well
> 
> OK, I'll change this comment and remove "service".

Done.

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:365: const net::NetLog::Source& source,
On 2014/03/14 21:48:09, rpaquay wrote:
> On 2014/03/14 21:13:48, keybuk wrote:
> > What's the justification for these added parameters?
> > 
> > Are they specific to the resulting BluetoothSocket? In which case why aren't
> > they part of the BluetoothSocket API?
> > 
> > Or are they specific to the Windows implementation? If not, how would other
> > platforms, e.g. Chrome OS, use these?
> > 
> > Would different calls to ConnectProfile() expect to pass different
parameters
> > for the task_runners, net_log, source, etc.? Or would they all pass the
same?
> > 
> > What about when we move to having ConnectL2cap(), ConnectRfcomm(),
> > ListenL2cap(), ListenRfcomm() etc. would those need these arguments too?
> > 
> > What about other async functions?
> > 
> > Basically should these be moved towards the original BluetoothAdapter
> > constructor ?
> 
> That is a good point, the task runners definitely could move to the
constructor
> (I assume you mean "BluetoothDevice" not "BluetoothAdapter"). As for the
> net_log/source, I am not 100% sure, as I don't quite understand their usage at
> this point. Other Chrome APPS apis seem to happily pass "NULL,
DefaultSource()"
> when using TCPSocket, but I have a feeling this is not quite correct either.
In
> any ways, I think the decision should probably not be made at the "device"
> layer, but at a higher layer, hence the need to pass them as parameters
> somewhere (here or constructor).

Done.

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_task_manager_win.cc (right):

https://codereview.chromium.org/180163009/diff/220001/device/bluetooth/blueto...
device/bluetooth/bluetooth_task_manager_win.cc:465: device_address,
L2CAP_PROTOCOL_UUID, service_record_states);
On 2014/03/17 22:55:11, xiyuan wrote:
> It seems we only need enumerate L2CAP_PROTOCOL_UUID as in previous code.
> 
> Previous code has a bug that |sdp_buffer_size| might be changed after
returning
> from WSALookupServiceNext. Sometime |sdp_buffer_size| becomes too small and
> causes service enumeration to fail with WSAEFAULT.
> 
> With that fixed (i.e. "sdp_buffer_size = sizeof(sdp_buffer)" at the end of the
> WSALookupServiceNext loop and before calling it again), the loop gets all the
> services, including the OBEXObjectPushServiceClass_UUID and I don't need to
run
> the loop for all other protocol UUIDs.

Done.

[keybuk, 2014-03-20 01:21:55]

mostly a header/idl pass at this point, will do a deeper review of code code
tomorrow

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:47: //
|error_callback| will be called with |kIOPending| error.
count isn't documented

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:57: // sending
data.
buffer_size isn't documented

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:64: const
std::string profile_uuid() const { return profile_uuid_; }
rename to uuid()

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:100: std::string
profile_uuid_;
Rename to uuid_, replace mention of profile in the comment with service?

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:110: int
buffer_size_;
How is this used for L2CAP sockets which are datagram rather than streaming?

https://codereview.chromium.org/180163009/diff/240001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/240001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:16: // Information about the state of
the bluetooth adapater.
nit: Bluetooth
everywhere

https://codereview.chromium.org/180163009/diff/240001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:127: Profile profile;
Replace with DOMString uuid;

https://codereview.chromium.org/180163009/diff/240001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:283: static void send(SendOptions
options,
Since send() is not write() we should make the change to inline arguments now
instead of using a SendOptions

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:69:
adapter_factory.Get().adapter_initializer_ = initializer;
This doesn't run the initializer if the adapter is already initialized by
another part of the code

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:97:
adapter_factory.Get().adapter_initializer_.Run(new_adapter);
the adapter might not be initialized yet?

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:102:
adapter_factory.Get().adapter_initializer_.Run(new_adapter);
adapter might not be initialized yet

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:244: typedef base::Callback<void(const
std::string&)> ErrorCallback;
What are the possible values of this string?
Is this pre-translated?
Feels like it should be an enum.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:50: // message.
need to document parameters

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:58: // reason and a message.
buffer_size not documented

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_device.h:10: #include
"base/sequenced_task_runner.h"
remove

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_socket.h:11: #include
"net/base/io_buffer.h"
seems like you can avoid this include by

namespace net {

class IOBuffer;

}  // namespace net

[keybuk, 2014-03-20 16:34:48]

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:42: const ErrorCompletionCallback&
error_callback) = 0;
Does this need to be part of the public cross-platform API for BluetoothSocket?
Since BluetoothSocket instances are provided in callbacks, they're already
connected.

It might make sense for the platform implementations of BluetoothSocketWin/Mac
to have a Connect() method called by the BluetoothAdapter/DeviceWin/Mac
implementation - but for Chrome OS this would always be NOTIMPLEMENTED()

Which to me implies that it shouldn't be in the public API

[rpaquay, 2014-03-20 18:21:11]

Thanks for the comments so far! I think I addressed current feedback in Patchset
#13.

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:57: // sending
data.
On 2014/03/20 01:21:56, keybuk wrote:
> buffer_size isn't documented

Done.

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:64: const
std::string profile_uuid() const { return profile_uuid_; }
On 2014/03/20 01:21:56, keybuk wrote:
> rename to uuid()

Done.

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:100: std::string
profile_uuid_;
On 2014/03/20 01:21:56, keybuk wrote:
> Rename to uuid_, replace mention of profile in the comment with service?

Done.

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:110: int
buffer_size_;
On 2014/03/20 01:21:56, keybuk wrote:
> How is this used for L2CAP sockets which are datagram rather than streaming?

Unless I am missing something, this is similar to UDP sockets: It it the
responsibility of the app to ensure it specifies a buffer size big enough to
avoid loss of data.

https://codereview.chromium.org/180163009/diff/240001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/240001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:16: // Information about the state of
the bluetooth adapater.
On 2014/03/20 01:21:56, keybuk wrote:
> nit: Bluetooth
> everywhere

Done.

https://codereview.chromium.org/180163009/diff/240001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:127: Profile profile;
On 2014/03/20 01:21:56, keybuk wrote:
> Replace with DOMString uuid;

Done.

https://codereview.chromium.org/180163009/diff/240001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:283: static void send(SendOptions
options,
On 2014/03/20 01:21:56, keybuk wrote:
> Since send() is not write() we should make the change to inline arguments now
> instead of using a SendOptions

Done.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:69:
adapter_factory.Get().adapter_initializer_ = initializer;
On 2014/03/20 01:21:56, keybuk wrote:
> This doesn't run the initializer if the adapter is already initialized by
> another part of the code

The issue is that the "device" layer needs initialization from an upper layer
("extensions::BluetoothAPI"). This is currently specific to Windows only, where
the upper layer needs to pass the FILE thread and net::log instances to the
Windows Adapater implementation.

Other than requiring the upper layer to do guarantee setting the initializer
before the adapter is used, I don't know of any other option.

I am not a fan of this approach, but couldn't think of a better way. Suggestions
are welcome on how to better implement this pattern.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:97:
adapter_factory.Get().adapter_initializer_.Run(new_adapter);
On 2014/03/20 01:21:56, keybuk wrote:
> the adapter might not be initialized yet?
The initialization callback is intended to be part of the Adapter instance class
initialization, not part of the "the physical Bluetooth Adapter" is ready.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:102:
adapter_factory.Get().adapter_initializer_.Run(new_adapter);
On 2014/03/20 01:21:56, keybuk wrote:
> adapter might not be initialized yet

See Above.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:244: typedef base::Callback<void(const
std::string&)> ErrorCallback;
On 2014/03/20 01:21:56, keybuk wrote:
> What are the possible values of this string?
> Is this pre-translated?
> Feels like it should be an enum.

The intent is to provide a descriptive error message to the App, which will
eventually end up in "chrome.lastError.message". There was a precedent was
"BluetoothSocket::getLastErrorMessage()", which I also replaced with a error
callback with a "std::string error" parameter -- getLastErrorMessage was simply
too tricky to deal with with an asynchronous API, imho.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:42: const ErrorCompletionCallback&
error_callback) = 0;
On 2014/03/20 16:34:49, keybuk wrote:
> Does this need to be part of the public cross-platform API for
BluetoothSocket?
> Since BluetoothSocket instances are provided in callbacks, they're already
> connected.
> 
> It might make sense for the platform implementations of BluetoothSocketWin/Mac
> to have a Connect() method called by the BluetoothAdapter/DeviceWin/Mac
> implementation - but for Chrome OS this would always be NOTIMPLEMENTED()
> 
> Which to me implies that it shouldn't be in the public API

Done.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:50: // message.
On 2014/03/20 01:21:56, keybuk wrote:
> need to document parameters

Done.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:58: // reason and a message.
On 2014/03/20 01:21:56, keybuk wrote:
> buffer_size not documented

Done.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_device.h:10: #include
"base/sequenced_task_runner.h"
On 2014/03/20 01:21:56, keybuk wrote:
> remove

Done.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_socket.h:11: #include
"net/base/io_buffer.h"
On 2014/03/20 01:21:56, keybuk wrote:
> seems like you can avoid this include by
> 
> namespace net {
> 
> class IOBuffer;
> 
> }  // namespace net

That didn't work (on Windows at least). I think it is because MOCK_METHOD4 below
is a template class with implementation requiring fully knowing about the
parameter types.

[keybuk, 2014-03-20 18:38:39]

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api_socket.h:110: int
buffer_size_;
On 2014/03/20 18:21:11, rpaquay wrote:
> On 2014/03/20 01:21:56, keybuk wrote:
> > How is this used for L2CAP sockets which are datagram rather than streaming?
> 
> Unless I am missing something, this is similar to UDP sockets: It it the
> responsibility of the app to ensure it specifies a buffer size big enough to
> avoid loss of data.

Ack

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:69:
adapter_factory.Get().adapter_initializer_ = initializer;
Sure, but the extensions::BluetoothAPI may not be the first component of Chrome
to be initialized... in the case where using the Aura shell, wouldn't be the
Aura UI be initialized first?

The Aura UI holds a reference to the BluetoothAdapter

So when the BluetoothAPI class goes to get it, the existing adapter will just be
returned, and the initializer function won't be called.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:97:
adapter_factory.Get().adapter_initializer_.Run(new_adapter);
Ack

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:244: typedef base::Callback<void(const
std::string&)> ErrorCallback;
Okay, is the policy that those are always English? Would there be a standard set
of error messages across platforms, or would Chrome OS return different error
strings to Windows for the same error?

Also I thought Manifests v3 was trying to get rid of lastError?


If always English, and okay for platform-different errors, I'm okay with this
staying - but since ErrorCallback is used for other methods like Disconnect()
and Forget(), this should be a new callback for the ConnectToProfile case only

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_socket.h:11: #include
"net/base/io_buffer.h"
Ack

[rpaquay, 2014-03-25 20:06:09]

PTAL: all current comments should be addressed with this patchset.

* As explained in a comment, this patchset simplifies the threading model.
* Also, fix unit and browser tests.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:69:
adapter_factory.Get().adapter_initializer_ = initializer;
On 2014/03/20 18:38:40, keybuk wrote:
> Sure, but the extensions::BluetoothAPI may not be the first component of
Chrome
> to be initialized... in the case where using the Aura shell, wouldn't be the
> Aura UI be initialized first?
> 
> The Aura UI holds a reference to the BluetoothAdapter
> 
> So when the BluetoothAPI class goes to get it, the existing adapter will just
be
> returned, and the initializer function won't be called.

These are all good points. After thinking about this a little bit more, I
decided that it would be best to avoid any additional platform specific global
initialization. To enable that, I had to rethink the threading model a little
bit more, and I think I came up with a simplified model for Windows, very
similar to the way ChromeOS currently works: Consumers always talks to the API
via the UI thread and the platform specific implementation use a dedicated
thread for socket IO. This makes the latest patchset quite big, but I think it
is for the best.

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/180163009/diff/240001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:244: typedef base::Callback<void(const
std::string&)> ErrorCallback;
On 2014/03/20 18:38:40, keybuk wrote:
> Okay, is the policy that those are always English? Would there be a standard
set
> of error messages across platforms, or would Chrome OS return different error
> strings to Windows for the same error?
> 
> Also I thought Manifests v3 was trying to get rid of lastError?
> 
> 
> If always English, and okay for platform-different errors, I'm okay with this
> staying - but since ErrorCallback is used for other methods like Disconnect()
> and Forget(), this should be a new callback for the ConnectToProfile case only

Yeah, the message are always English, and platform dependent errors are allowed
(On MacOs, for example, having the IOReturn from the IOBluetoothXxx calls
somewhere in the error message could be useful).

As for Manifest v3, my guess is that lastError is going to be replaced with some
sort of exception mechanism. I think we are trying to get rid of the specific
mechanics of using a global variable (lastError), not the ability to have "dev"
English only error messages.

I will change the CL to make the callback specific to ConnecToProfile.

[keybuk, 2014-03-27 20:20:06]

Mostly there, just spotted a few issues

https://codereview.chromium.org/180163009/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://codereview.chromium.org/180163009/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:408: const
std::string& error_message) {
No error_message parameter

https://codereview.chromium.org/180163009/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
(right):

https://codereview.chromium.org/180163009/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h:189: const
std::string& error_message);
Forget doesn't have an error_message parameter

https://codereview.chromium.org/180163009/diff/280001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/280001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:269: optional ResultCallback
callback);
TODO or Bug for changing disconnect to: 

chrome.bluetooth.disconnect(integer socketId, function callback)

To match sockets -- since this CL has been in progress for long enough, I won't
make you do that here ;-)

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/OWNERS
File device/bluetooth/OWNERS (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/OWNERS...
device/bluetooth/OWNERS:2: rpaquay@chromium.org
Put this as a per-file for *_win and *_mac ?

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory.h (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.h:21: typedef
base::Callback<void(BluetoothAdapter* adapter)> AdapterInitializer;
Remove this now?

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:232:
error_callback.Run(kNotImplemented);
Drop this change, this is using ErrorCallback not ConnectToProfileErrorCallback

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:399:
error_callback.Run(kNotImplemented);
as above

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:406:
error_callback.Run(kNotImplemented);
as above

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:569:
error_callback.Run(error_message);
as above

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:578:
error_callback.Run(error_message);
as above

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:42: virtual void Destruct() = 0;
bikeshed: name this Release()

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_socket.h:22: const ErrorCompletionCallback&
error_callback));
There is no Connect, there is only ...

[rpaquay, 2014-03-27 22:00:01]

PTAL.

* Patchset #15 is a "simple" rebase + fix merge conflicts.
* Patchset #16 addresses the last round of code review feedback from keybuk@.

https://codereview.chromium.org/180163009/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://codereview.chromium.org/180163009/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:408: const
std::string& error_message) {
On 2014/03/27 20:20:07, keybuk wrote:
> No error_message parameter

Done.

https://codereview.chromium.org/180163009/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
(right):

https://codereview.chromium.org/180163009/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h:189: const
std::string& error_message);
On 2014/03/27 20:20:07, keybuk wrote:
> Forget doesn't have an error_message parameter

Done.

https://codereview.chromium.org/180163009/diff/280001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/280001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:269: optional ResultCallback
callback);
On 2014/03/27 20:20:07, keybuk wrote:
> TODO or Bug for changing disconnect to: 
> 
> chrome.bluetooth.disconnect(integer socketId, function callback)
> 
> To match sockets -- since this CL has been in progress for long enough, I
won't
> make you do that here ;-)

Done: crbug/357328

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/OWNERS
File device/bluetooth/OWNERS (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/OWNERS...
device/bluetooth/OWNERS:2: rpaquay@chromium.org
On 2014/03/27 20:20:07, keybuk wrote:
> Put this as a per-file for *_win and *_mac ?

Done.

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory.h (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.h:21: typedef
base::Callback<void(BluetoothAdapter* adapter)> AdapterInitializer;
On 2014/03/27 20:20:07, keybuk wrote:
> Remove this now?

Done.

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:232:
error_callback.Run(kNotImplemented);
On 2014/03/27 20:20:07, keybuk wrote:
> Drop this change, this is using ErrorCallback not
ConnectToProfileErrorCallback

Actually, this method has disappeared after rebasing this CL.

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:399:
error_callback.Run(kNotImplemented);
On 2014/03/27 20:20:07, keybuk wrote:
> as above

Done.

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:406:
error_callback.Run(kNotImplemented);
On 2014/03/27 20:20:07, keybuk wrote:
> as above

Done.

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:569:
error_callback.Run(error_message);
On 2014/03/27 20:20:07, keybuk wrote:
> as above

Done.

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_chromeos.cc:578:
error_callback.Run(error_message);
On 2014/03/27 20:20:07, keybuk wrote:
> as above

Done.

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/280001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:42: virtual void Destruct() = 0;
On 2014/03/27 20:20:07, keybuk wrote:
> bikeshed: name this Release()

Done.

[keybuk, 2014-03-27 22:09:16]

lgtm

[rpaquay, 2014-03-27 22:21:14]

miket@chromium.org: I need OWNER lgtm for a few more files, please review
changes in

chrome/common/extensions/api/bluetooth.idl
extensions/browser/api/api_resource_manager.h
extensions/browser/api/async_api_function.h
extensions/browser/extension_function_histogram_value.h

[miket_OOO, 2014-03-28 20:48:42]

lgtm

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:8: // Information about the state of
the bluetooth adapater.
adapter

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:111: // Flag indicating if the socket
is left open when the event page of the
Meggin will change "if" to whether, so might as well do it now.

Distinction: it's not that the flag does its indicating job only in the case
that the socket is persistent. It's that the flag reflects the persistent-or-not
state. A more acute example:

Let's decide if she goes to the park.
Let's decide whether she goes to the park.

In case one, we might or might not go through the process of deciding.
In case two, the outcome depends on our decision.

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:136: // Flag indicating whether the
socket is left open when the application is
good!

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:144: // specified explictly, the
value is not provided.
Nit: I wonder whether "field" is more accurate than "value." In a dictionary, a
value is half of a key-value pair.

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:213: // The data received, with a
maxium size of <code>bufferSize</code>.
maximum (spelling)

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:292: // peer. The default value is
"false". Pausing a socket is typically used by
Convention seems to be to wrap in <code>

[rpaquay, 2014-03-31 15:39:18]

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/220001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:8: // Information about the state of
the bluetooth adapater.
On 2014/03/28 20:48:43, miket wrote:
> adapter

Done.

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/bluetooth.idl (right):

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:111: // Flag indicating if the socket
is left open when the event page of the
On 2014/03/28 20:48:43, miket wrote:
> Meggin will change "if" to whether, so might as well do it now.
> 
> Distinction: it's not that the flag does its indicating job only in the case
> that the socket is persistent. It's that the flag reflects the
persistent-or-not
> state. A more acute example:
> 
> Let's decide if she goes to the park.
> Let's decide whether she goes to the park.
> 
> In case one, we might or might not go through the process of deciding.
> In case two, the outcome depends on our decision.

Done.

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:136: // Flag indicating whether the
socket is left open when the application is
On 2014/03/28 20:48:43, miket wrote:
> good!

Done.

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:144: // specified explictly, the
value is not provided.
On 2014/03/28 20:48:43, miket wrote:
> Nit: I wonder whether "field" is more accurate than "value." In a dictionary,
a
> value is half of a key-value pair.

Done.

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:213: // The data received, with a
maxium size of <code>bufferSize</code>.
On 2014/03/28 20:48:43, miket wrote:
> maximum (spelling)

Done.

https://codereview.chromium.org/180163009/diff/780001/chrome/common/extension...
chrome/common/extensions/api/bluetooth.idl:292: // peer. The default value is
"false". Pausing a socket is typically used by
On 2014/03/28 20:48:43, miket wrote:
> Convention seems to be to wrap in <code>

Done.

[rpaquay, 2014-03-31 15:39:31]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-03-31 15:39:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/180163009/800001

[commit-bot: I haz the power, 2014-03-31 15:39:54]

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

[commit-bot: I haz the power, 2014-03-31 15:39:55]

Failed to apply patch for
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 26 (offset -1 lines).
  Hunk #3 succeeded at 53 (offset -1 lines).
  Hunk #4 succeeded at 64 (offset -1 lines).
  Hunk #5 succeeded at 144 (offset -1 lines).
  Hunk #6 succeeded at 165 (offset -1 lines).
  Hunk #7 succeeded at 453 (offset -1 lines).
  Hunk #8 succeeded at 474 (offset -1 lines).
  Hunk #9 succeeded at 483 (offset -1 lines).
  Hunk #10 FAILED at 508.
  Hunk #11 FAILED at 610.
  3 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc.rej

Patch:       chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
Index: chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
diff --git a/chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
b/chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
index
d556a1bda73131205ce05319fe427bbf6b29fb65..bc1febde9e4d1dede6d27ead4f39e6751559119d
100644
--- a/chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
+++ b/chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
@@ -10,7 +10,9 @@
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/extensions/api/bluetooth/bluetooth_api_utils.h"
 #include "chrome/browser/extensions/api/bluetooth/bluetooth_event_router.h"
+#include
"chrome/browser/extensions/api/bluetooth/bluetooth_socket_event_dispatcher.h"
 #include "chrome/browser/extensions/event_names.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/extensions/api/bluetooth.h"
 #include "chrome/common/extensions/api/bluetooth/bluetooth_manifest_data.h"
 #include "content/public/browser/browser_thread.h"
@@ -27,20 +29,26 @@
 #include "net/base/io_buffer.h"
 
 using content::BrowserContext;
+using content::BrowserThread;
+
 using device::BluetoothAdapter;
 using device::BluetoothDevice;
 using device::BluetoothProfile;
 using device::BluetoothServiceRecord;
 using device::BluetoothSocket;
 
-namespace {
+using extensions::BluetoothApiSocket;
 
-extensions::ExtensionBluetoothEventRouter* GetEventRouter(
-    BrowserContext* context) {
-  return extensions::BluetoothAPI::Get(context)->bluetooth_event_router();
-}
-
-}  // namespace
+namespace AddProfile = extensions::api::bluetooth::AddProfile;
+namespace bluetooth = extensions::api::bluetooth;
+namespace Connect = extensions::api::bluetooth::Connect;
+namespace Disconnect = extensions::api::bluetooth::Disconnect;
+namespace GetDevice = extensions::api::bluetooth::GetDevice;
+namespace GetDevices = extensions::api::bluetooth::GetDevices;
+namespace RemoveProfile = extensions::api::bluetooth::RemoveProfile;
+namespace SetOutOfBandPairingData =
+    extensions::api::bluetooth::SetOutOfBandPairingData;
+namespace Send = extensions::api::bluetooth::Send;
 
 namespace {
 
@@ -48,7 +56,6 @@ const char kCouldNotGetLocalOutOfBandPairingData[] =
     "Could not get local Out Of Band Pairing Data";
 const char kCouldNotSetOutOfBandPairingData[] =
     "Could not set Out Of Band Pairing Data";
-const char kFailedToConnect[] = "Connection failed";
 const char kInvalidDevice[] = "Invalid device";
 const char kInvalidUuid[] = "Invalid UUID";
 const char kPermissionDenied[] = "Permission to add profile denied.";
@@ -60,19 +67,72 @@ const char kSocketNotFoundError[] = "Socket not found:
invalid socket id";
 const char kStartDiscoveryFailed[] = "Starting discovery failed";
 const char kStopDiscoveryFailed[] = "Failed to stop discovery";
 
-}  // namespace
+extensions::ExtensionBluetoothEventRouter* GetEventRouter(
+    BrowserContext* context) {
+  // Note: |context| is valid on UI thread only.
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return extensions::BluetoothAPI::Get(context)->event_router();
+}
+
+linked_ptr<bluetooth::Socket> CreateSocketInfo(int socket_id,
+                                               BluetoothApiSocket* socket) {
+  DCHECK(BrowserThread::CurrentlyOn(BluetoothApiSocket::kThreadId));
+  linked_ptr<bluetooth::Socket> socket_info(new bluetooth::Socket());
+  // This represents what we know about the socket, and does not call through
+  // to the system.
+  socket_info->id = socket_id;
+  if (!socket->name().empty()) {
+    socket_info->name.reset(new std::string(socket->name()));
+  }
+  socket_info->persistent = socket->persistent();
+  if (socket->buffer_size() > 0) {
+    socket_info->buffer_size.reset(new int(socket->buffer_size()));
+  }
+  socket_info->paused = socket->paused();
+  socket_info->device.address = socket->device_address();
+  socket_info->uuid = socket->uuid();
 
-namespace AddProfile = extensions::api::bluetooth::AddProfile;
-namespace bluetooth = extensions::api::bluetooth;
-namespace Connect = extensions::api::bluetooth::Connect;
-namespace Disconnect = extensions::api::bluetooth::Disconnect;
-namespace GetDevice = extensions::api::bluetooth::GetDevice;
-namespace GetDevices = extensions::api::bluetooth::GetDevices;
-namespace Read = extensions::api::bluetooth::Read;
-namespace RemoveProfile = extensions::api::bluetooth::RemoveProfile;
-namespace SetOutOfBandPairingData =
-    extensions::api::bluetooth::SetOutOfBandPairingData;
-namespace Write = extensions::api::bluetooth::Write;
+  return socket_info;
+}
+
+void SetSocketProperties(extensions::BluetoothApiSocket* socket,
+                         bluetooth::SocketProperties* properties) {
+  if (properties->name.get()) {
+    socket->set_name(*properties->name.get());
+  }
+  if (properties->persistent.get()) {
+    socket->set_persistent(*properties->persistent.get());
+  }
+  if (properties->buffer_size.get()) {
+    // buffer size is validated when issuing the actual Recv operation
+    // on the socket.
+    socket->set_buffer_size(*properties->buffer_size.get());
+  }
+}
+
+static void DispatchConnectionEventWorker(
+    void* browser_context_id,
+    const std::string& extension_id,
+    const std::string& profile_uuid,
+    const device::BluetoothDevice* device,
+    scoped_refptr<device::BluetoothSocket> socket) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  content::BrowserContext* context =
+      reinterpret_cast<content::BrowserContext*>(browser_context_id);
+  if (!extensions::ExtensionsBrowserClient::Get()->IsValidContext(context))
+    return;
+
+  extensions::BluetoothAPI* bluetooth_api =
+      extensions::BluetoothAPI::Get(context);
+  if (!bluetooth_api)
+    return;
+
+  bluetooth_api->DispatchConnectionEvent(
+      extension_id, profile_uuid, device, socket);
+}
+
+}  // namespace
 
 namespace extensions {
 
@@ -87,11 +147,17 @@ BluetoothAPI::GetFactoryInstance() {
 
 // static
 BluetoothAPI* BluetoothAPI::Get(BrowserContext* context) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return GetFactoryInstance()->Get(context);
 }
 
-BluetoothAPI::BluetoothAPI(BrowserContext* context)
+BluetoothAPI::ConnectionParams::ConnectionParams() {}
+
+BluetoothAPI::ConnectionParams::~ConnectionParams() {}
+
+BluetoothAPI::BluetoothAPI(content::BrowserContext* context)
     : browser_context_(context) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   ExtensionSystem::Get(browser_context_)->event_router()->RegisterObserver(
       this, bluetooth::OnAdapterStateChanged::kEventName);
   ExtensionSystem::Get(browser_context_)->event_router()->RegisterObserver(
@@ -102,38 +168,241 @@ BluetoothAPI::BluetoothAPI(BrowserContext* context)
       this, bluetooth::OnDeviceRemoved::kEventName);
 }
 
-BluetoothAPI::~BluetoothAPI() {
+BluetoothAPI::~BluetoothAPI() {}
+
+ExtensionBluetoothEventRouter* BluetoothAPI::event_router() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!event_router_) {
+    event_router_.reset(new ExtensionBluetoothEventRouter(browser_context_));
+  }
+  return event_router_.get();
 }
 
-ExtensionBluetoothEventRouter* BluetoothAPI::bluetooth_event_router() {
-  if (!bluetooth_event_router_)
-    bluetooth_event_router_.reset(
-        new ExtensionBluetoothEventRouter(browser_context_));
+scoped_refptr<BluetoothAPI::SocketData> BluetoothAPI::socket_data() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!socket_data_) {
+    ApiResourceManager<BluetoothApiSocket>* socket_manager =
+        ApiResourceManager<BluetoothApiSocket>::Get(browser_context_);
+    DCHECK(socket_manager)
+        << "There is no socket manager. "
+           "If this assertion is failing during a test, then it is likely that
"
+           "TestExtensionSystem is failing to provide an instance of "
+           "ApiResourceManager<BluetoothApiSocket>.";
 
-  return bluetooth_event_router_.get();
+    socket_data_ = socket_manager->data_;
+  }
+  return socket_data_;
+}
+
+scoped_refptr<api::BluetoothSocketEventDispatcher>
+BluetoothAPI::socket_event_dispatcher() {
+  if (!socket_event_dispatcher_) {
+    socket_event_dispatcher_ = new api::BluetoothSocketEventDispatcher(
+        browser_context_, socket_data());
+  }
+  return socket_event_dispatcher_;
 }
 
 void BluetoothAPI::Shutdown() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   ExtensionSystem::Get(browser_context_)->event_router()->UnregisterObserver(
       this);
 }
 
 void BluetoothAPI::OnListenerAdded(const EventListenerInfo& details) {
-  if (bluetooth_event_router()->IsBluetoothSupported())
-    bluetooth_event_router()->OnListenerAdded();
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (event_router()->IsBluetoothSupported())
+    event_router()->OnListenerAdded();
 }
 
 void BluetoothAPI::OnListenerRemoved(const EventListenerInfo& details) {
-  if (bluetooth_event_router()->IsBluetoothSupported())
-    bluetooth_event_router()->OnListenerRemoved();
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (event_router()->IsBluetoothSupported())
+    …
(message too large)

[rpaquay, 2014-03-31 17:54:06]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-03-31 17:56:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/180163009/820001

[commit-bot: I haz the power, 2014-03-31 18:31:45]

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

[commit-bot: I haz the power, 2014-03-31 18:31:46]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[rpaquay, 2014-03-31 21:17:55]

jar@chromium.org, please check the following file:

device/bluetooth/DEPS

It looks like I need LGTM for adding a dependency to "+net". The justification
is that Bluetooth API use sockets for communications, so it makes sense to
re-use classes from "net" as opposed to use OS socket APIs.

[xiyuan, 2014-04-03 23:25:46]

https://codereview.chromium.org/180163009/diff/920001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/920001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:42: virtual void Release() = 0;
Release() confuses scoped_refptr. We might want to use a different name.

[rpaquay, 2014-04-04 19:52:12]

https://codereview.chromium.org/180163009/diff/920001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/920001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:42: virtual void Release() = 0;
On 2014/04/03 23:25:47, xiyuan wrote:
> Release() confuses scoped_refptr. We might want to use a different name. 

What is the confusion about exactly? I changed the name already from Destruct to
Release as code review feedback.

[xiyuan, 2014-04-04 20:05:05]

https://codereview.chromium.org/180163009/diff/920001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/920001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:42: virtual void Release() = 0;
On 2014/04/04 19:52:13, rpaquay wrote:
> On 2014/04/03 23:25:47, xiyuan wrote:
> > Release() confuses scoped_refptr. We might want to use a different name. 
> 
> What is the confusion about exactly? I changed the name already from Destruct
to
> Release as code review feedback.

In scoped_refptr's dtor, "ptr_->Release();" invokes the
BluetoothSocketWin::Release instead of the one in RefCountedThreadSafe.

I am using VS2013. I put a break point in BluetoothSocketWin::Release and it
gets called upon returning from BluetoothSocketWin::CreateBluetoothSocket.

[jar (doing other things), 2014-04-04 21:35:48]

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/DEPS
File device/bluetooth/DEPS (right):

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/DEPS#n...
device/bluetooth/DEPS:5: "+net",
Better would be to have:

+net/base
+net/socket

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket_win.cc (right):

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket_win.cc:21: #include
"net/socket/tcp_socket_win.h"
nit: better might be to include net/socket/tcp_socket.h.

This allows you to write slightly less platform specific code.

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket_win.cc:197: socket_win_.reset(new
net::TCPSocketWin(net_log_, source_));
nit: Better is probably to use generic name TCPSocket() rather than
TCPSocketWin.

[rpaquay, 2014-04-04 21:59:25]

https://codereview.chromium.org/180163009/diff/920001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket.h (right):

https://codereview.chromium.org/180163009/diff/920001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket.h:42: virtual void Release() = 0;
On 2014/04/04 20:05:06, xiyuan wrote:
> On 2014/04/04 19:52:13, rpaquay wrote:
> > On 2014/04/03 23:25:47, xiyuan wrote:
> > > Release() confuses scoped_refptr. We might want to use a different name. 
> > 
> > What is the confusion about exactly? I changed the name already from
Destruct
> to
> > Release as code review feedback.
> 
> In scoped_refptr's dtor, "ptr_->Release();" invokes the
> BluetoothSocketWin::Release instead of the one in RefCountedThreadSafe.
> 
> I am using VS2013. I put a break point in BluetoothSocketWin::Release and it
> gets called upon returning from BluetoothSocketWin::CreateBluetoothSocket. 

Ouch, that is bad. After chatting with keybuk@, we decided to rename to "Close".
Done!

[rpaquay, 2014-04-04 22:27:56]

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/DEPS
File device/bluetooth/DEPS (right):

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/DEPS#n...
device/bluetooth/DEPS:5: "+net",
On 2014/04/04 21:35:49, jar wrote:
> Better would be to have:
> 
> +net/base
> +net/socket

Done.

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_socket_win.cc (right):

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket_win.cc:21: #include
"net/socket/tcp_socket_win.h"
On 2014/04/04 21:35:49, jar wrote:
> nit: better might be to include net/socket/tcp_socket.h.
> 
> This allows you to write slightly less platform specific code.

Done.

https://codereview.chromium.org/180163009/diff/940001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket_win.cc:197: socket_win_.reset(new
net::TCPSocketWin(net_log_, source_));
On 2014/04/04 21:35:49, jar wrote:
> nit: Better is probably to use generic name TCPSocket() rather than
> TCPSocketWin.

Done.

[jar (doing other things), 2014-04-04 22:30:28]

Patch set 26 DEPS for net (and use of net/socket) LGTM

[rpaquay, 2014-04-04 22:33:15]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-04 22:34:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/180163009/940002

[commit-bot: I haz the power, 2014-04-04 23:11:49]

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

[commit-bot: I haz the power, 2014-04-04 23:11:50]

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

[rpaquay, 2014-04-07 15:48:42]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-07 15:48:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/180163009/940003

[commit-bot: I haz the power, 2014-04-07 15:49:19]

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

[commit-bot: I haz the power, 2014-04-07 15:49:20]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

Transient error: <urlopen error _ssl.c:489: The handshake operation timed out>

[rpaquay, 2014-04-07 15:54:57]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-07 15:55:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/180163009/940003

[commit-bot: I haz the power, 2014-04-07 15:55:31]

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

[commit-bot: I haz the power, 2014-04-07 15:55:31]

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #1 FAILED at 769.
  1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej

Patch:       extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
133204a83c859f783ebdde81934c71c48212153e..43c05f0719de2c64b3d567f1ed837361ce4b7474
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -769,6 +769,10 @@ enum HistogramValue {
   BLUETOOTHPRIVATE_DISABLEPAIRING,
   BLUETOOTHPRIVATE_SETPAIRINGRESPONSE,
   NETWORKINGPRIVATE_GETCAPTIVEPORTALSTATUS,
+  BLUETOOTH_UPDATE_SOCKET,
+  BLUETOOTH_SET_SOCKET_PAUSED,
+  BLUETOOTH_GET_SOCKET,
+  BLUETOOTH_GET_SOCKETS,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY

[rpaquay, 2014-04-07 16:19:04]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-07 16:19:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/180163009/1010001

[commit-bot: I haz the power, 2014-04-07 16:49:23]

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

[commit-bot: I haz the power, 2014-04-07 16:49:24]

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

[rpaquay, 2014-04-07 17:42:55]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-07 17:43:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/180163009/1010001

[commit-bot: I haz the power, 2014-04-07 19:21:11]

Change committed as 262175

[rpaquay, 2014-04-07 21:47:56]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-07 21:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/180163009/1030001

[commit-bot: I haz the power, 2014-04-08 01:02:32]

Change committed as 262274