Bluetooth: Send UI notifications when the connecting status changes.

Bluetooth: Send UI notifications when the connecting status changes.

This fix sends a DeviceChanged notification to all the
BluetoothAdapter::Observer when a BluetoothDevice::Connect makes the
BluetoothDevice::IsConnecting property change.

BUG=231985
TEST=None

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

[deymo, 2013-04-18 00:45:24]

I need this notification in BlueZ 4 to test that bug.

[keybuk, 2013-04-18 00:52:23]

Bluetooth{Adapter,Device}ExperimentalChromeOS will need this change too
(currently in CQ)

https://chromiumcodereview.appspot.com/12374062/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://chromiumcodereview.appspot.com/12374062/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_chromeos.cc:252: DeviceChanged(this,
device));
There are other instances of this code in BluetoothAdapterChromeOS - they should
be changed to call this function instead

https://chromiumcodereview.appspot.com/12374062/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/12374062/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_chromeos.h:122: 
Since this is called externally, perhaps NotifyDeviceChanged()?

Comment should say something like "notifies observers"

https://chromiumcodereview.appspot.com/12374062/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://chromiumcodereview.appspot.com/12374062/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_chromeos.cc:121: if (connecting_calls_ != 0 &&
!connecting_) {
How can connecting calls be zero if you just incremented it? :)

[deymo, 2013-04-18 01:38:40]

PTAL.
Please remember this is not a long-term fix.

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_ad...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_chromeos.cc:252: DeviceChanged(this,
device));
On 2013/04/18 00:52:24, keybuk wrote:
> There are other instances of this code in BluetoothAdapterChromeOS - they
should
> be changed to call this function instead

In a deprecated implementation? It is worth the refactor?

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_ad...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_chromeos.h:122: 
On 2013/04/18 00:52:24, keybuk wrote:
> Since this is called externally, perhaps NotifyDeviceChanged()?
> 
> Comment should say something like "notifies observers"

Done.

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_de...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:121: if (connecting_calls_ != 0 &&
!connecting_) {
On 2013/04/18 00:52:24, keybuk wrote:
> How can connecting calls be zero if you just incremented it? :)
Probably it can't. I just translated this:
if (invariant is false) { fix it! and notify }

Done.

[keybuk, 2013-04-18 15:54:17]

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_ad...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_chromeos.cc:252: DeviceChanged(this,
device));
On 2013/04/18 01:38:40, deymo wrote:
> On 2013/04/18 00:52:24, keybuk wrote:
> > There are other instances of this code in BluetoothAdapterChromeOS - they
> should
> > be changed to call this function instead
> 
> In a deprecated implementation? It is worth the refactor?

Yes.

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.cc:531: bool prev_connecting =
!!connecting_calls_;
why is this not just:

  bool prev_connecting = connecting_;

?

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.cc:542: bool prev_connecting =
!!connecting_calls_;
ditto

[deymo, 2013-04-18 16:27:43]

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.cc:531: bool prev_connecting =
!!connecting_calls_;
It could be any of those... why one or the other?? Looks the same for me.

[keybuk, 2013-04-18 17:12:05]

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.cc:531: bool prev_connecting =
!!connecting_calls_;
On 2013/04/18 16:27:43, deymo wrote:
> It could be any of those... why one or the other?? Looks the same for me.

You're making an assumption that connecting_ is calculated from
connecting_calls_ - someone may change that in the future and unwittingly break
this code

prev_connecting = connecting_
OR
prev_connecting_calls_ = connecting_calls_

would be both fine

[deymo, 2013-04-19 01:40:30]

PTAL

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_ad...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_chromeos.cc:252: DeviceChanged(this,
device));
On 2013/04/18 15:54:17, keybuk wrote:
> On 2013/04/18 01:38:40, deymo wrote:
> > On 2013/04/18 00:52:24, keybuk wrote:
> > > There are other instances of this code in BluetoothAdapterChromeOS - they
> > should
> > > be changed to call this function instead
> > 
> > In a deprecated implementation? It is worth the refactor?
> 
> Yes.

Done.

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.cc:531: bool prev_connecting =
!!connecting_calls_;
On 2013/04/18 17:12:05, keybuk wrote:
> On 2013/04/18 16:27:43, deymo wrote:
> > It could be any of those... why one or the other?? Looks the same for me.
> 
> You're making an assumption that connecting_ is calculated from
> connecting_calls_ - someone may change that in the future and unwittingly
break
> this code
> 
> prev_connecting = connecting_
> OR
> prev_connecting_calls_ = connecting_calls_
> 
> would be both fine

Done.

https://codereview.chromium.org/12374062/diff/6001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.cc:542: bool prev_connecting =
!!connecting_calls_;
On 2013/04/18 15:54:17, keybuk wrote:
> ditto

Done.

[keybuk, 2013-04-19 01:55:44]

lgtm

please note in the bug that the equivalent ExperimentalChromeOS change is not
yet done

[commit-bot: I haz the power, 2013-04-19 20:45:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/12374062/13001

[keybuk, 2013-04-19 20:48:53]

https://codereview.chromium.org/12374062/diff/13001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/13001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_adapter_chromeos.cc:239: CHECK(device->adapter_ ==
this);
this must be a DCHECK...

please fix in a follow-up commit

[deymo, 2013-04-19 21:05:42]

https://codereview.chromium.org/12374062/diff/13001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/12374062/diff/13001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_adapter_chromeos.cc:239: CHECK(device->adapter_ ==
this);
On 2013/04/19 20:48:53, keybuk wrote:
> this must be a DCHECK...
> 
> please fix in a follow-up commit

Done.

[commit-bot: I haz the power, 2013-04-19 21:06:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/12374062/29001

[commit-bot: I haz the power, 2013-04-20 03:11:11]

Change committed as 195375