Introduce 'connecting' field to bluetooth device info for system tray

Introduce 'connecting' field to bluetooth device info for system tray


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

[jennyz, 2013-03-02 01:12:16]

https://codereview.chromium.org/12385064/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12385064/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:529: if
(device->IsConnected() || device->IsConnecting()) {
If the bluetooth is connecting, it could be one of the two cases: if this is
already paired, it is connecting to the device; if this is not paired, it is
pairing with the device. For the pairing process, we probably should call
Disconnect. deymo should knows this better.

[deymo, 2013-03-02 02:31:13]

FYI: I'll be submitting the DeviceChanged notification next week.

https://codereview.chromium.org/12385064/diff/1/ash/system/bluetooth/tray_blu...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/12385064/diff/1/ash/system/bluetooth/tray_blu...
ash/system/bluetooth/tray_bluetooth.cc:137: else if (list[i].connecting)
If the property IsConnecting() is true the device is definitively in a
"connecting" status. It may have the connected property in true or not, for
other reasons, but from the users perspective "connecting" is more important
than "connected". So, this should be checked before list[i].connected

https://codereview.chromium.org/12385064/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12385064/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:529: if
(device->IsConnected() || device->IsConnecting()) {
On 2013/03/02 01:12:16, jennyz wrote:
> If the bluetooth is connecting, it could be one of the two cases: if this is
> already paired, it is connecting to the device; if this is not paired, it is
> pairing with the device. For the pairing process, we probably should call
> Disconnect. deymo should knows this better.

If IsConnecting is true, means that a connection is going on. This can include
the pairing process or not, but in any case, the connection will try to connect
the applications also (so, try to reach the connected=true state). So if the
device is connecting and the user clicks on that device, you may want to
do...hmm.. nothing? There's no other option more than just wait till the
connection finishes.

[deymo, 2013-03-02 03:34:26]

Update:
If you need the notification functionality for testing your part you can pickup
this patch: https://crrev.com/12374062

$ git cl patch 12374062
will commit this patch in your current branch.
I'm holding it because is missing test and I'm working on adding a new
bluetooth_device_unittest.cc before pushing that CL.

[Jun Mukai, 2013-04-09 03:15:08]

Sorry for ignoring so long time this. Please take another look.

https://codereview.chromium.org/12385064/diff/1/ash/system/bluetooth/tray_blu...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/12385064/diff/1/ash/system/bluetooth/tray_blu...
ash/system/bluetooth/tray_bluetooth.cc:137: else if (list[i].connecting)
On 2013/03/02 02:31:13, deymo wrote:
> If the property IsConnecting() is true the device is definitively in a
> "connecting" status. It may have the connected property in true or not, for
> other reasons, but from the users perspective "connecting" is more important
> than "connected". So, this should be checked before list[i].connected

Done.

https://codereview.chromium.org/12385064/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12385064/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:529: if
(device->IsConnected() || device->IsConnecting()) {
On 2013/03/02 02:31:13, deymo wrote:
> On 2013/03/02 01:12:16, jennyz wrote:
> > If the bluetooth is connecting, it could be one of the two cases: if this is
> > already paired, it is connecting to the device; if this is not paired, it is
> > pairing with the device. For the pairing process, we probably should call
> > Disconnect. deymo should knows this better.
> 
> If IsConnecting is true, means that a connection is going on. This can include
> the pairing process or not, but in any case, the connection will try to
connect
> the applications also (so, try to reach the connected=true state). So if the
> device is connecting and the user clicks on that device, you may want to
> do...hmm.. nothing? There's no other option more than just wait till the
> connection finishes.

moved the conditions. now it attempts to connect.

[jennyz, 2013-04-10 16:15:37]

https://codereview.chromium.org/12385064/diff/5001/ash/system/bluetooth/tray_...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/12385064/diff/5001/ash/system/bluetooth/tray_...
ash/system/bluetooth/tray_bluetooth.cc:236: }
It's a little bit unclean to modify the display name here, can you change the
item's display_name when you push the item into connecting_devices_. after line
139?

[Jun Mukai, 2013-04-13 00:05:35]

https://codereview.chromium.org/12385064/diff/5001/ash/system/bluetooth/tray_...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/12385064/diff/5001/ash/system/bluetooth/tray_...
ash/system/bluetooth/tray_bluetooth.cc:236: }
On 2013/04/10 16:15:37, jennyz wrote:
> It's a little bit unclean to modify the display name here, can you change the
> item's display_name when you push the item into connecting_devices_. after
line
> 139?

Done.

https://codereview.chromium.org/12385064/diff/5001/ash/system/bluetooth/tray_...
ash/system/bluetooth/tray_bluetooth.cc:297: } else if (FoundDevice(device_id,
connecting_devices_, &display_name) ||
noticed this is not necessary. since connecting_devices_ already shows
'connecting...' message, no need to update the label.

[deymo, 2013-04-13 01:16:44]

Sorry for the delay reviewing it. I wasn't sure if I had to review it or not =)

https://codereview.chromium.org/12385064/diff/13001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12385064/diff/13001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:563: } else if
(device->IsPaired() || device->IsConnecting()) {
If the device is connecting the "click" action on the list should do nothing. In
ChromeOS if you call Connect while another Connect is going on it will fail with
a "org.bluez.Error.InProgress" error, which is confusing for the user. The
"connecting..." label on the device should be enough because there will be an
error message for the ongoing connection or the "connecting..." message will go
away.

[Jun Mukai, 2013-04-15 17:36:46]

https://codereview.chromium.org/12385064/diff/13001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12385064/diff/13001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:563: } else if
(device->IsPaired() || device->IsConnecting()) {
On 2013/04/13 01:16:44, deymo wrote:
> If the device is connecting the "click" action on the list should do nothing.
In
> ChromeOS if you call Connect while another Connect is going on it will fail
with
> a "org.bluez.Error.InProgress" error, which is confusing for the user. The
> "connecting..." label on the device should be enough because there will be an
> error message for the ongoing connection or the "connecting..." message will
go
> away.

Done.

[deymo, 2013-04-15 17:43:55]

lgtm

lgtm, but jenny must approve.

[jennyz, 2013-04-15 18:47:16]

lgtm with nit.

https://codereview.chromium.org/12385064/diff/20001/ash/system/bluetooth/tray...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/12385064/diff/20001/ash/system/bluetooth/tray...
ash/system/bluetooth/tray_bluetooth.cc:235: string16 display_name =
list[i].display_name;
nit, you don't need to use this variable display_name, since it is only used
once.

[Jun Mukai, 2013-04-15 21:09:45]

https://codereview.chromium.org/12385064/diff/20001/ash/system/bluetooth/tray...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/12385064/diff/20001/ash/system/bluetooth/tray...
ash/system/bluetooth/tray_bluetooth.cc:235: string16 display_name =
list[i].display_name;
On 2013/04/15 18:47:17, jennyz wrote:
> nit, you don't need to use this variable display_name, since it is only used
> once.

Done.

[Jun Mukai, 2013-04-15 21:10:17]

stevenjb, can you review this?
I need owner approval for c/b/chromeos/system

[stevenjb, 2013-04-15 21:21:24]

lgtm

[Jun Mukai, 2013-04-16 20:10:32]

Committed patchset #5 manually as r194432 (presubmit successful).