Implemented BluetoothSocketWin.

Implemented BluetoothSocketWin.


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

[youngki, 2013-01-24 19:30:51]

Scott, could you review this BluetoothSocketWin CL?

It turns out that Windows also uses the same term "fd" for socket file
descriptor, so I didn't change the interface.

[keybuk, 2013-01-25 01:31:16]

How does this work with https://codereview.chromium.org/12039075/ which seems to
remove the need for an fd() member?

[youngki, 2013-01-25 17:05:50]

On 2013/01/25 01:31:16, keybuk wrote:
> How does this work with https://codereview.chromium.org/12039075/ which seems
to
> remove the need for an fd() member?

Yes, fd() that returns fd_ will be obsolete; still we need to create a socket
and initialize fd_, which this CL mostly does. Once I submit this CL And the
other one, I will simply just remove BluetoothSocket::fd() method.

[keybuk, 2013-01-28 19:16:39]

would you hate me if I asked you to base one CL on the other, or even merge
them?

I'm having a hard time reviewing this alongside the CL that adds Read/Write
methods - it's just not clear to me what the resulting code will look like with
both CLs in place

[youngki, 2013-01-28 21:12:50]

On 2013/01/28 19:16:39, keybuk wrote:
> would you hate me if I asked you to base one CL on the other, or even merge
> them?
> 
> I'm having a hard time reviewing this alongside the CL that adds Read/Write
> methods - it's just not clear to me what the resulting code will look like
with
> both CLs in place

I think I will just submit https://chromiumcodereview.appspot.com/12039075/ and
sync this. :)

[keybuk, 2013-01-28 21:37:33]

On 2013/01/28 21:12:50, youngki wrote:
> I think I will just submit https://chromiumcodereview.appspot.com/12039075/
and
> sync this. :)

sure that works too :)

[youngki, 2013-01-30 16:42:17]

This CL has been rebased and updated with complete implementation of
BluetoothSocketWin. Please take a look.

[keybuk, 2013-01-30 23:28:00]

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_socket.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_socket.h:27: virtual int fd() const = 0;
Can this go away now? What's it used for now we have Receive() and Send() ?

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_utils.h:18: };
This could have a bdaddr_t member in the union on Chrome OS and then the
conversion code wouldn't be necessary

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_utils.h:21: // of bytes and a uint64. Returns true on
success, false on failure.  The
This should just refer to the union by name, rather than describe it

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_utils_unittest.cc (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_utils_unittest.cc:14:
EXPECT_TRUE(bluetooth_utils::str2ba("B0:D0:9C:04:7D:91", &bluetooth_address));
Why change the test values?

[youngki, 2013-01-31 15:45:42]

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_socket.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_socket.h:27: virtual int fd() const = 0;
On 2013/01/30 23:28:00, keybuk wrote:
> Can this go away now? What's it used for now we have Receive() and Send() ?

It is still used to dedup the socket:
https://cs.corp.google.com/#chrome/src/chrome/browser/extensions/api/bluetoot...

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_utils.h:18: };
On 2013/01/30 23:28:00, keybuk wrote:
> This could have a bdaddr_t member in the union on Chrome OS and then the
> conversion code wouldn't be necessary

I'd rather have this union without depending on any specific platform.. this
union could be used to represent bluetooth address in any platform; having
bdaddr_t instead would make this less reusable for the non-chromeos platforms
that need uint8 array of bluetooth address.

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_utils.h:21: // of bytes and a uint64. Returns true on
success, false on failure.  The
On 2013/01/30 23:28:00, keybuk wrote:
> This should just refer to the union by name, rather than describe it

Done.

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_utils_unittest.cc (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_utils_unittest.cc:14:
EXPECT_TRUE(bluetooth_utils::str2ba("B0:D0:9C:04:7D:91", &bluetooth_address));
On 2013/01/30 23:28:00, keybuk wrote:
> Why change the test values?

Done.

[keybuk, 2013-01-31 19:15:28]

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_socket.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_socket.h:27: virtual int fd() const = 0;
Wouldn't the same socket structure have the same address? It's ref counted after
all

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_utils.h:18: };
But you're already adding platform-specific code ;) the union is there to make
it much easier to do the Windows implementation by having the uint64 format that
you happen to know makes it easier for Windows

Right now this implementation is yucky

It's used in precisely one place, and doesn't provide a return type that is
useful in that place, so has to be converted again to a local type.

[keybuk, 2013-01-31 19:26:41]

https://chromiumcodereview.appspot.com/12052066/diff/14001/device/bluetooth/b...
File device/bluetooth/bluetooth_socket_chromeos.cc (right):

https://chromiumcodereview.appspot.com/12052066/diff/14001/device/bluetooth/b...
device/bluetooth/bluetooth_socket_chromeos.cc:59: sizeof(socket_address));
Ok, let's look at this objectively...

We Have - service_record, and service_record.address() - a stirng

We Want - a struct sockaddr* (specifically a bdaddr_t for it)

https://chromiumcodereview.appspot.com/12052066/diff/14001/device/bluetooth/b...
File device/bluetooth/bluetooth_socket_win.cc (right):

https://chromiumcodereview.appspot.com/12052066/diff/14001/device/bluetooth/b...
device/bluetooth/bluetooth_socket_win.cc:62: sizeof(sa));
And again, objectively...

We Have - service_record, and service_record.address() - a string

We Want - a SOCKADDR* (specifically a uint64 for it)

https://chromiumcodereview.appspot.com/12052066/diff/14001/device/bluetooth/b...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/14001/device/bluetooth/b...
device/bluetooth/bluetooth_utils.h:25: bool str2ba(const std::string&
in_address, bluetooth_address* out_address);
So what should this do?

It should take a string address (or even just a service record), and return a
platform-specific return type.

Since the return-type is platform-specific, this implies that it should be in a
class that we can sub-class per platform.

But then we should also look at where this is used... it's only used in per
platform code (BluetoothSocket*)

So how about this?

  BluetoothServiceRecord::GetAddressBytes(uint8* bytes);

Turns the string of that service record into the reversed byte format.

Then BluetoothSocketChromeOS and BluetoothSocketWin can convert that into their
native format (bdaddr_t or uint64) appropriately.

[youngki, 2013-01-31 23:25:21]

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
File device/bluetooth/bluetooth_socket.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/6002/device/bluetooth/bl...
device/bluetooth/bluetooth_socket.h:27: virtual int fd() const = 0;
On 2013/01/31 19:15:28, keybuk wrote:
> Wouldn't the same socket structure have the same address? It's ref counted
after
> all

Done.

https://chromiumcodereview.appspot.com/12052066/diff/14001/device/bluetooth/b...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/14001/device/bluetooth/b...
device/bluetooth/bluetooth_utils.h:25: bool str2ba(const std::string&
in_address, bluetooth_address* out_address);
On 2013/01/31 19:26:41, keybuk wrote:
> So what should this do?
> 
> It should take a string address (or even just a service record), and return a
> platform-specific return type.
> 
> Since the return-type is platform-specific, this implies that it should be in
a
> class that we can sub-class per platform.
> 
> But then we should also look at where this is used... it's only used in per
> platform code (BluetoothSocket*)
> 
> So how about this?
> 
>   BluetoothServiceRecord::GetAddressBytes(uint8* bytes);
> 
> Turns the string of that service record into the reversed byte format.
> 
> Then BluetoothSocketChromeOS and BluetoothSocketWin can convert that into
their
> native format (bdaddr_t or uint64) appropriately.

So, I actually implemented
BluetoothServiceRecordChromeOS::GetBluetoothAddress(bdaddr_t*) and
BluetoothServiceRecordWin::bth_addr() to return platform-specific bluetooth
address.

[keybuk, 2013-01-31 23:49:46]

lgtm if you're sure about the socket vs socket.get() thing and after the union
is removed

https://chromiumcodereview.appspot.com/12052066/diff/17004/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/12052066/diff/17004/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:76: if
(i->second == socket)
socket or socket.get()?

https://chromiumcodereview.appspot.com/12052066/diff/17004/device/bluetooth/b...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/17004/device/bluetooth/b...
device/bluetooth/bluetooth_utils.h:18: };
not needed now?

[youngki, 2013-02-01 16:56:24]

Mike, could you review the tiny changes on
chrome/browser/extensions/api/bluetooth/ for OWNERS approval?

https://chromiumcodereview.appspot.com/12052066/diff/17004/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/12052066/diff/17004/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:76: if
(i->second == socket)
On 2013/01/31 23:49:46, keybuk wrote:
> socket or socket.get()?

made it i->second.get() == socket.get().

https://chromiumcodereview.appspot.com/12052066/diff/17004/device/bluetooth/b...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/12052066/diff/17004/device/bluetooth/b...
device/bluetooth/bluetooth_utils.h:18: };
On 2013/01/31 23:49:46, keybuk wrote:
> not needed now?

Done.

[miket_OOO, 2013-02-01 18:39:18]

> Mike, could you review the tiny changes on
> chrome/browser/extensions/api/bluetooth/ for OWNERS approval?

OWNERS LGTM

[commit-bot: I haz the power, 2013-02-01 22:09:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12052066/6022

[commit-bot: I haz the power, 2013-02-02 01:31:56]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-02 07:35:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12052066/6022

[commit-bot: I haz the power, 2013-02-02 11:56:28]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-02 19:55:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12052066/6022

[commit-bot: I haz the power, 2013-02-03 00:44:02]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-03 15:59:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12052066/6022

[commit-bot: I haz the power, 2013-02-03 18:24:04]

Change committed as 180315