win: Implement Bluetooth server.

win: Implement Bluetooth server.

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

[xiyuan, 2014-04-15 22:46:39]

Suggested split:
keybuk@ for overall review.
    Hope it is not making profile CL harder.
rpaquay@ for _win specific files
wtc@ for net/*

Thanks.

[keybuk, 2014-04-16 08:06:16]

https://codereview.chromium.org/236203018/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_win.h (right):

https://codereview.chromium.org/236203018/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_win.h:50: const net::NetLog::Source& source);
It seems that the BluetoothProfileWin implementation ends up doing a lot of
stuff that would logically belong in BluetoothSocketWin instead

Would a better implementation to be to move the socket code, including accept,
etc. to BluetoothSocketWin and just have BluetoothProfileWin contain a reference
to the listening one?

[wtc, 2014-04-23 16:52:32]

On 2014/04/15 22:46:39, xiyuan wrote:
>
> wtc@ for net/*

Sorry for the late reply. I was on vacation and just came to work yesterday. I
will try to review this CL today or tomorrow.

[xiyuan, 2014-04-23 16:54:48]

No worries. I am still moving the code from BtProfileWin into BtSocketWin per
keybuk@'s suggestion. The net/ files are not going to change but you can still
wait until I upload the new PS (probably sometime later today).

[xiyuan, 2014-04-23 17:42:55]

CL updated to move service publishing code from BluetoothProfileWin to
BluetoothSocketWin.
PTAL. Thanks.

https://codereview.chromium.org/236203018/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_win.h (right):

https://codereview.chromium.org/236203018/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_win.h:50: const net::NetLog::Source& source);
On 2014/04/16 08:06:17, keybuk wrote:
> It seems that the BluetoothProfileWin implementation ends up doing a lot of
> stuff that would logically belong in BluetoothSocketWin instead
> 
> Would a better implementation to be to move the socket code, including accept,
> etc. to BluetoothSocketWin and just have BluetoothProfileWin contain a
reference
> to the listening one?

Done.

[rpaquay, 2014-04-23 22:52:05]

https://codereview.chromium.org/236203018/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_win.cc (right):

https://codereview.chromium.org/236203018/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.cc:63: ZeroMemory(this,
sizeof(ServiceRegData));
Is it safe to ZeroMemory a struct that contains a std::string16?

https://codereview.chromium.org/236203018/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_win.h (right):

https://codereview.chromium.org/236203018/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.h:49: // and the service is published
successfully. Otheriwse, |error_callback| is
Otherwise

[xiyuan, 2014-04-23 23:19:42]

https://codereview.chromium.org/236203018/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_win.cc (right):

https://codereview.chromium.org/236203018/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.cc:63: ZeroMemory(this,
sizeof(ServiceRegData));
On 2014/04/23 22:52:06, rpaquay wrote:
> Is it safe to ZeroMemory a struct that contains a std::string16?

Nope. Replaced with ZeroMemory on individual members.

https://codereview.chromium.org/236203018/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_win.h (right):

https://codereview.chromium.org/236203018/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.h:49: // and the service is published
successfully. Otheriwse, |error_callback| is
On 2014/04/23 22:52:06, rpaquay wrote:
> Otherwise

Done.

[keybuk, 2014-04-24 11:49:28]

lgtm

I'm not too worried about the BluetoothProfile removal, since I can remove BP
without removing BPWin and leave you the job of cleaning that up ;-)

[rpaquay, 2014-04-24 20:08:32]

lgtm with nit

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

https://codereview.chromium.org/236203018/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_win.h:76:
scoped_refptr<BluetoothSocketThreadWin> socket_thread();
Sorry I didn't notice this earlier, but I think you could change these accessor
1) to be const
2) to be inlines

e.g.
const scoped_refptr<base::SequencedTaskRunner>& ui_task_runner() const { return
...}
const scoped_refptr<BluetoothSocketThreadWin>& socket_thread() const { return
...}

[xiyuan, 2014-04-24 20:57:04]

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

https://codereview.chromium.org/236203018/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_win.h:76:
scoped_refptr<BluetoothSocketThreadWin> socket_thread();
On 2014/04/24 20:08:32, rpaquay wrote:
> Sorry I didn't notice this earlier, but I think you could change these
accessor
> 1) to be const
> 2) to be inlines
> 
> e.g.
> const scoped_refptr<base::SequencedTaskRunner>& ui_task_runner() const {
return
> ...}
> const scoped_refptr<BluetoothSocketThreadWin>& socket_thread() const { return
> ...}

Done.

[wtc, 2014-04-24 23:09:31]

Review comments on patch set 4:

Overall the code seems good. I reviewed the whole CL but focused on src/net. The
most important comment is that AdoptBoundSocket should be documented better, and
ideally a test should be added to src/net to use that method.

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_profile_win.cc (right):

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_win.cc:78: end_point.address()[0]);

It would be nice to add a comment to explain why the address appears to be
printed in reverse order.

The format specifier 'X' matches an unsigned int, but end_point.address()[5] is
a uint8. So there appears to be a type mismatch. I think we are relying on C's
integral promotion here. From K & R, 2nd Ed., page 45:

  In the absence of a function prototype, char and short become int,
  and float becomes double.

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_win.cc:217: return
static_cast<BluetoothAdapterWin*>(adapter_.get());

Just wanted to check that static_cast is the right cast to use here.

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_win.cc (right):

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.cc:488:
scoped_socket->AdoptBoundSocket(socket_fd);

At this point, we haven't called bind() yet, so the method name
"AdoptBoundSocket" is confusing.

It seems that the key point is that socket_fd is a stream-oriented socket of a
non-TCP address family. So TCPSocket can adopt socket_fd, but we can't call any
TCPSocket methods that involve the address, such as Bind(). So perhaps we should
name this method "AdoptNonTCPSocket" and explain the limitations.

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1170: bool GetIPAddressFromSockAddr(const struct sockaddr*
sock_addr,

I don't see any call to GetIPAddressFromSockAddr in this CL. Can you explain why
GetIPAddressFromSockAddr needs to be modified?

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1192: *address = reinterpret_cast<const unsigned
char*>(&addr->sin6_addr);

Nit: could you change "unsigned char" to "uint8" here for consistency?

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1200: // Bluetooth address size. Windows bluetooth is
supported via winsock.

Nit: capitalize "bluetooth".

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1201: const size_t kBtAddressSize = 6;

1. Nit: it seems better to use "Bth" instead of "Bt" because the address family
macro is "AF_BTH".

2. It seems that this constant should be defined in the net/base/net_util.h
header instead (inside #if defined(OS_WIN)). You also need this constant in
device/bluetooth/bluetooth_profile_win.cc.

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1210: *port = addr->port;

This doesn't extract the serviceClassId member of SOCKADDR_BTH. Is that a
problem?

https://codereview.chromium.org/236203018/diff/80001/net/socket/tcp_socket_wi...
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/236203018/diff/80001/net/socket/tcp_socket_wi...
net/socket/tcp_socket_win.cc:342: int TCPSocketWin::AdoptBoundSocket(SOCKET
socket) {

I compared this method with AdoptConnectedSocket. The difference about
|peer_address| is expected, but I don't understand why this method doesn't need
to create |core_|. Could you add a comment to explain that?

https://codereview.chromium.org/236203018/diff/80001/net/socket/tcp_socket_win.h
File net/socket/tcp_socket_win.h (right):

https://codereview.chromium.org/236203018/diff/80001/net/socket/tcp_socket_wi...
net/socket/tcp_socket_win.h:39: int AdoptBoundSocket(SOCKET socket);

IMPORTANT: It would be better to list this method immediately after the similar
AdoptConnectedSocket method. The comment should explain the difference between
the two methods, and what each of the methods is intended for.

Since no code in src/net (not even a unit test) uses the new method, the comment
is crucial. It would be good to add a unit test.

[xiyuan, 2014-04-25 20:29:28]

PTAL. Thanks.

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_profile_win.cc (right):

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_win.cc:78: end_point.address()[0]);
On 2014/04/24 23:09:31, wtc wrote:
> 
> It would be nice to add a comment to explain why the address appears to be
> printed in reverse order.
> 
> The format specifier 'X' matches an unsigned int, but end_point.address()[5]
is
> a uint8. So there appears to be a type mismatch. I think we are relying on C's
> integral promotion here. From K & R, 2nd Ed., page 45:
> 
>   In the absence of a function prototype, char and short become int,
>   and float becomes double.

Comments added that the address is copyied from a BTH_ADDR that stores Bth
address in little endian. That's why we print in reverse order.

You are right about %X. According to wiki, this works now because "arguments of
types narrower than an int will be promoted to int or unsigned int" in the
variable-length part of a variable-length argument list.

http://en.wikipedia.org/wiki/Stdarg.h

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_win.cc:217: return
static_cast<BluetoothAdapterWin*>(adapter_.get());
On 2014/04/24 23:09:31, wtc wrote:
> 
> Just wanted to check that static_cast is the right cast to use here.

I think it is safe here. |adapter_| holds BluetoothAdapter* which is the base
class of BluetoothAdapterWin.

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_socket_win.cc (right):

https://codereview.chromium.org/236203018/diff/80001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.cc:488:
scoped_socket->AdoptBoundSocket(socket_fd);
On 2014/04/24 23:09:31, wtc wrote:
> 
> At this point, we haven't called bind() yet, so the method name
> "AdoptBoundSocket" is confusing.
> 
> It seems that the key point is that socket_fd is a stream-oriented socket of a
> non-TCP address family. So TCPSocket can adopt socket_fd, but we can't call
any
> TCPSocket methods that involve the address, such as Bind(). So perhaps we
should
> name this method "AdoptNonTCPSocket" and explain the limitations.

Renamed the function to AdoptListenSocket per discussion to better describe what
it does. And added comments to document the limitations.

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1170: bool GetIPAddressFromSockAddr(const struct sockaddr*
sock_addr,
On 2014/04/24 23:09:31, wtc wrote:
> 
> I don't see any call to GetIPAddressFromSockAddr in this CL. Can you explain
why
> GetIPAddressFromSockAddr needs to be modified?

It is indirectly called in ip_end_point.FromSockAddr(...) in
TCPSocketWin::AcceptInternal to obtain the peer address.

https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_soc...

The peer address is later used in BluetoothProfileWin::OnNewConnection to find
the corresponding Bth device.

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1192: *address = reinterpret_cast<const unsigned
char*>(&addr->sin6_addr);
On 2014/04/24 23:09:31, wtc wrote:
> 
> Nit: could you change "unsigned char" to "uint8" here for consistency?

Done.

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1200: // Bluetooth address size. Windows bluetooth is
supported via winsock.
On 2014/04/24 23:09:31, wtc wrote:
> 
> Nit: capitalize "bluetooth".

Done.

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1201: const size_t kBtAddressSize = 6;
On 2014/04/24 23:09:31, wtc wrote:
> 
> 1. Nit: it seems better to use "Bth" instead of "Bt" because the address
family
> macro is "AF_BTH".
> 

Done.

> 2. It seems that this constant should be defined in the net/base/net_util.h
> header instead (inside #if defined(OS_WIN)). You also need this constant in
> device/bluetooth/bluetooth_profile_win.cc.

Done.

https://codereview.chromium.org/236203018/diff/80001/net/base/net_util.cc#new...
net/base/net_util.cc:1210: *port = addr->port;
On 2014/04/24 23:09:31, wtc wrote:
> 
> This doesn't extract the serviceClassId member of SOCKADDR_BTH. Is that a
> problem?

serviceClassId is a way to map to a port(aka Bt channel). We use port directly
so don't need this.

https://codereview.chromium.org/236203018/diff/80001/net/socket/tcp_socket_wi...
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/236203018/diff/80001/net/socket/tcp_socket_wi...
net/socket/tcp_socket_win.cc:342: int TCPSocketWin::AdoptBoundSocket(SOCKET
socket) {
On 2014/04/24 23:09:31, wtc wrote:
> 
> I compared this method with AdoptConnectedSocket. The difference about
> |peer_address| is expected, but I don't understand why this method doesn't
need
> to create |core_|. Could you add a comment to explain that?

|core_| is only used for read/write operations and not used when the socket only
does accept. Comments to make it less surprising.

https://codereview.chromium.org/236203018/diff/80001/net/socket/tcp_socket_win.h
File net/socket/tcp_socket_win.h (right):

https://codereview.chromium.org/236203018/diff/80001/net/socket/tcp_socket_wi...
net/socket/tcp_socket_win.h:39: int AdoptBoundSocket(SOCKET socket);
On 2014/04/24 23:09:31, wtc wrote:
> 
> IMPORTANT: It would be better to list this method immediately after the
similar
> AdoptConnectedSocket method. The comment should explain the difference between
> the two methods, and what each of the methods is intended for.
> 
> Since no code in src/net (not even a unit test) uses the new method, the
comment
> is crucial. It would be good to add a unit test.
> 

Renamed to AdoptListenSocket since the adopted |socket| can only be used for
accepting connections. And moved to just below AdoptConnectedSocket.

Also added a AcceptForAdoptedListenSocket test.

[wtc, 2014-04-28 23:54:43]

Patch set 5 LGTM.

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

https://codereview.chromium.org/236203018/diff/100001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket_win.cc:487: // The ownership is transferred in
AdoptListenSocket(). Note that since

Nit: "Creates a |scoped_socket| to own ... in AdoptListenSocket()." can be
deleted because both points are obvious. Add "So bind() is called on |socket_fd|
directly."

https://codereview.chromium.org/236203018/diff/100001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/236203018/diff/100001/net/base/net_util.h#new...
net/base/net_util.h:60: static const size_t kBthAddressSize = 6;

Nit: I suggest using "Bluetooh" instead of "Bth" in the constant's name.

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_u...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_u...
net/socket/tcp_socket_unittest.cc:267: TEST_F(TCPSocketTest,
AcceptForAdoptedListenSocket) {

Nit: it seems better to list this new test right after
TCPSocketTest.AcceptAsync.

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_w...
File net/socket/tcp_socket_win.h (right):

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.h:35: // Both AdoptConnectedSocket and
AdoptListenSocket takes ownership of an

Nit: takes => take

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.h:38: // connection. In some sense, AdoptListenSocket
is more close to Open.

Nit: more close => "closer" or "more similar".

[xiyuan, 2014-04-29 00:50:25]

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

https://codereview.chromium.org/236203018/diff/100001/device/bluetooth/blueto...
device/bluetooth/bluetooth_socket_win.cc:487: // The ownership is transferred in
AdoptListenSocket(). Note that since
On 2014/04/28 23:54:43, wtc wrote:
> 
> Nit: "Creates a |scoped_socket| to own ... in AdoptListenSocket()." can be
> deleted because both points are obvious. Add "So bind() is called on
|socket_fd|
> directly."

Done.

https://codereview.chromium.org/236203018/diff/100001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/236203018/diff/100001/net/base/net_util.h#new...
net/base/net_util.h:60: static const size_t kBthAddressSize = 6;
On 2014/04/28 23:54:43, wtc wrote:
> 
> Nit: I suggest using "Bluetooh" instead of "Bth" in the constant's name.

Done.

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_u...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_u...
net/socket/tcp_socket_unittest.cc:267: TEST_F(TCPSocketTest,
AcceptForAdoptedListenSocket) {
On 2014/04/28 23:54:43, wtc wrote:
> 
> Nit: it seems better to list this new test right after
> TCPSocketTest.AcceptAsync.

Done.

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_w...
File net/socket/tcp_socket_win.h (right):

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.h:35: // Both AdoptConnectedSocket and
AdoptListenSocket takes ownership of an
On 2014/04/28 23:54:43, wtc wrote:
> 
> Nit: takes => take

Done.

https://codereview.chromium.org/236203018/diff/100001/net/socket/tcp_socket_w...
net/socket/tcp_socket_win.h:38: // connection. In some sense, AdoptListenSocket
is more close to Open.
On 2014/04/28 23:54:43, wtc wrote:
> 
> Nit: more close => "closer" or "more similar".

Changed to "more similar".

[xiyuan, 2014-04-29 01:05:10]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-04-29 01:06:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/236203018/140001

[commit-bot: I haz the power, 2014-04-29 01:43:57]

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

[commit-bot: I haz the power, 2014-04-29 01:43:57]

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

[xiyuan, 2014-04-29 02:39:13]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-04-29 02:39:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/236203018/140001

[commit-bot: I haz the power, 2014-04-29 03:14:41]

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

[commit-bot: I haz the power, 2014-04-29 03:14:42]

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

[xiyuan, 2014-04-29 03:26:50]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-04-29 03:28:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/236203018/140001

[commit-bot: I haz the power, 2014-04-29 10:24:36]

Change committed as 266831