Reimplement BluetoothSocketChromeOS

Reimplement BluetoothSocketChromeOS

Since Chrome OS uses a socket-based interface much of the code already in
BluetoothSocketWin is suitable. Separate out that common code into a new
BluetoothSocketNet class and have both implementations derive from that
class with their differing Connect implementations remaining separate.

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

[keybuk, 2014-04-30 23:44:33]

rpaquay: for _win changes
isherman: for _mac changes
armansito: for /bluetooth/

[Ilya Sherman, 2014-04-30 23:57:42]

_mac changes LGTM with compilation fixed

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_mac.mm (right):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_mac.mm:241: callback.Run();
You should probably update the name here, too ;)

[Ilya Sherman, 2014-04-30 23:58:40]

/cc Xiyuan

[armansito, 2014-05-01 00:14:25]

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device_chromeos.h (right):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_chromeos.h:102:
scoped_refptr<base::SequencedTaskRunner> ui_task_runner_,
Shouldn't have underscores at the end of function arguments, i.e.:
s/ui_task_runner_/ui_task_runner/, s/socket_thread_/socket_thread/g. You have it
correct in the source file, so it works.

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket.h (left):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket.h:44: // Gracefully disconnects the socket and
calls |callback| upon completion.
If you're set on using |success_callback| as the name, then update the comment.

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

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket.h:46: virtual void Disconnect(const
base::Closure& success_callback) = 0;
Might as well leave it as |callback|, since it is a best effort operation. If
things internally failed, we just don't report it, so "success" may not be true.

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_net.cc (right):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_net.cc:127: void
BluetoothSocketNet::ResetData() {
I would at least log something here, maybe VLOG(1), saying that nothing
happened.

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

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_net.h:61: void ResetData();
You should make this virtual if the intention is to have subclasses override it.

[keybuk, 2014-05-01 00:24:09]

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device_chromeos.h (right):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_chromeos.h:102:
scoped_refptr<base::SequencedTaskRunner> ui_task_runner_,
On 2014/05/01 00:14:25, armansito wrote:
> Shouldn't have underscores at the end of function arguments, i.e.:
> s/ui_task_runner_/ui_task_runner/, s/socket_thread_/socket_thread/g. You have
it
> correct in the source file, so it works.

Done.

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket.h (left):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket.h:44: // Gracefully disconnects the socket and
calls |callback| upon completion.
On 2014/05/01 00:14:25, armansito wrote:
> If you're set on using |success_callback| as the name, then update the
comment.

ACK

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

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket.h:46: virtual void Disconnect(const
base::Closure& success_callback) = 0;
On 2014/05/01 00:14:25, armansito wrote:
> Might as well leave it as |callback|, since it is a best effort operation. If
> things internally failed, we just don't report it, so "success" may not be
true.

yeah I had added an error_callback but decided not do to... I guess I forgot I
renamed this ;-)

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_mac.mm (right):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_mac.mm:241: callback.Run();
On 2014/04/30 23:57:42, Ilya Sherman wrote:
> You should probably update the name here, too ;)

The success_callback thing was a holdover from a change I decided not to do -
reverted back to just callback

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_net.cc (right):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_net.cc:127: void
BluetoothSocketNet::ResetData() {
On 2014/05/01 00:14:25, armansito wrote:
> I would at least log something here, maybe VLOG(1), saying that nothing
> happened.

NAK, this is just for subclasses - Win happens to need to do something, but
Chrome OS doesn't ... no need for a log in Chrome OS saying "Nothing happens in
Chrome OS"

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

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_net.h:61: void ResetData();
On 2014/05/01 00:14:25, armansito wrote:
> You should make this virtual if the intention is to have subclasses override
it.

Done.

[armansito, 2014-05-01 00:54:17]

lgtm

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_net.cc (right):

https://codereview.chromium.org/267633003/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_net.cc:127: void
BluetoothSocketNet::ResetData() {
On 2014/05/01 00:24:09, keybuk wrote:
> On 2014/05/01 00:14:25, armansito wrote:
> > I would at least log something here, maybe VLOG(1), saying that nothing
> > happened.
> 
> NAK, this is just for subclasses - Win happens to need to do something, but
> Chrome OS doesn't ... no need for a log in Chrome OS saying "Nothing happens
in
> Chrome OS"

sgtm

[rpaquay, 2014-05-01 15:23:58]

https://codereview.chromium.org/267633003/diff/10001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_profile_chromeos.cc (right):

https://codereview.chromium.org/267633003/diff/10001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_chromeos.cc:310: VLOG(1) <<
object_path_.value() << ": Profile connection complete";
|callback| is not used here, shouldn't there be a "callback.Run(SUCCESS)"
somewhere?

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

https://codereview.chromium.org/267633003/diff/10001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.h:23: // The BluetoothSocketChromeOS class
implements BluetoothSocket for the
BluetoothSocketWin?

https://codereview.chromium.org/267633003/diff/10001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.h:25: class BluetoothSocketWin : public
BluetoothSocket {
BluetoothSocketNet?

[keybuk, 2014-05-01 18:07:15]

https://codereview.chromium.org/267633003/diff/10001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_profile_chromeos.cc (right):

https://codereview.chromium.org/267633003/diff/10001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_profile_chromeos.cc:310: VLOG(1) <<
object_path_.value() << ": Profile connection complete";
On 2014/05/01 15:23:58, rpaquay wrote:
> |callback| is not used here, shouldn't there be a "callback.Run(SUCCESS)"
> somewhere?

Done.

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

https://codereview.chromium.org/267633003/diff/10001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.h:23: // The BluetoothSocketChromeOS class
implements BluetoothSocket for the
On 2014/05/01 15:23:58, rpaquay wrote:
> BluetoothSocketWin?

Done.

https://codereview.chromium.org/267633003/diff/10001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_socket_win.h:25: class BluetoothSocketWin : public
BluetoothSocket {
On 2014/05/01 15:23:58, rpaquay wrote:
> BluetoothSocketNet?

Done.

[rpaquay, 2014-05-01 18:54:24]

lgtm

[keybuk, 2014-05-02 00:45:39]

The CQ bit was checked by keybuk@chromium.org

[commit-bot: I haz the power, 2014-05-02 00:46:29]

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

[commit-bot: I haz the power, 2014-05-02 16:07:33]

Change committed as 267804