[Bluetooth] Improve the adapter's awareness of connected devices.

[Bluetooth] Improve the adapter's awareness of connected devices.

Inform the adapter when a new, potentially unpaired device connects.
In addition to notifying any observers of this event, this prevents a
NULL pointer dereference in the existing logic.

BUG=385493
TEST=Establish a Bluetooth connection from an unpaired device.
     chrome.bluetooth.onDeviceAdded() should be called.
R=keybuk@chromium.org

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

[Ilya Sherman, 2014-06-17 06:42:14]

keybuk for review
others as fyi and extra eyes

Scott, when you connected to L2CAP from your Linux box, do you remember whether
you were prompted to pair or not?  When I connect from my Mac laptop to my Mac
desktop, I'm prompted to pair, and that happens before OnChannelOpened() is
called.  If that's behavior that we can count on, then we could simplify
UpdateDevices() a bit -- we could drop the second for loop.  OTOH, it doesn't
really hurt to keep it, just in case.  WDYT?

[armansito, 2014-06-17 14:56:43]

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:144: // pairing.
I agree with this, since to be consistent with the other platforms, devices that
are signalled through DeviceAdded should be available later via
BluetoothAdapter::GetDevices. Though since we don't seem to have a reliable
"removed" event on Mac, we can only detect this through the polling below and it
looks like it will now effectively end up calling DeviceRemoved on devices that
aren't connected or paired.

So if discovery is running and DeviceFound and PollAdapter run one after
another, we'll call DeviceAdded and then immediately DeviceRemoved on a device
which is technically still available to the system?

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:323: for (const auto& old_device :
old_devices) {
C++11 is allowed now?

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:337: DeviceRemoved(this,
removed_device.second));
Good catch on realizing that this was never called. On Chrome OS, devices are
available via BluetoothAdapter::GetDevices even if they aren't connected or
paired, as long as they have been discovered and are discoverable and within
range, which is ideally what we want here as well (see my comment above in
::DeviceFound).

I wonder which one of [IOBluetoothDevice pairedDevices], [IOBluetoothDevice
recentDevices:], and [IOBluetoothDeviceInquiry foundDevices] correctly returns
what we want in the end. If there is no way to obtain a list of all devices
currently within range, then maybe that's why DeviceRemoved was actually never
called before?

[keybuk, 2014-06-17 21:35:42]

reviewed, no additional comments

[Ilya Sherman, 2014-06-17 21:43:38]

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:144: // pairing.
On 2014/06/17 14:56:43, armansito wrote:
> I agree with this, since to be consistent with the other platforms, devices
that
> are signalled through DeviceAdded should be available later via
> BluetoothAdapter::GetDevices. Though since we don't seem to have a reliable
> "removed" event on Mac, we can only detect this through the polling below and
it
> looks like it will now effectively end up calling DeviceRemoved on devices
that
> aren't connected or paired.
> 
> So if discovery is running and DeviceFound and PollAdapter run one after
> another, we'll call DeviceAdded and then immediately DeviceRemoved on a device
> which is technically still available to the system?

Yeah, all of this logic is kinda wrong.  It's not actually clear to me when
exactly DeviceRemoved should be called.  Suppose a device is paired, but not
connected, and we haven't recently pinged it to check whether it's currently
within range.  The device might be off.  When should DeviceRemoved be called for
this device?  (What does ChromeOS do?)

Maybe we should report devices as removed if they are not connected and their
-getLastInquiryUpdate: and -recentAccessDate: are both a while ago?  I'd need to
investigate whether that does something reasonable.  I'd prefer to punt this
work to a separate CL.  For this CL, I'm primarily interested in making sure
that connected devices are listed among the |devices_|, so that we don't try to
dereference a NULL pointer.  Would you prefer that I hold off on adding
DeviceRemoved notifications until they are more sane?

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:323: for (const auto& old_device :
old_devices) {
On 2014/06/17 14:56:43, armansito wrote:
> C++11 is allowed now?

It is in Mac-only files :D

[keybuk, 2014-06-17 21:48:50]

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:144: // pairing.
CrOS for a device that was found by discovery, and has not been paired, keeps
the device object around for 30s-3m (depending on device type) before removing
it

The idea being that if we don't get an inquiry or advertising packet in that
time, the device isn't in range anymore

[armansito, 2014-06-17 22:07:23]

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:144: // pairing.
> Maybe we should report devices as removed if they are not connected and their
> -getLastInquiryUpdate: and -recentAccessDate: are both a while ago?  I'd need
to
> investigate whether that does something reasonable.  I'd prefer to punt this
> work to a separate CL.  For this CL, I'm primarily interested in making sure
> that connected devices are listed among the |devices_|, so that we don't try
to
> dereference a NULL pointer.  Would you prefer that I hold off on adding
> DeviceRemoved notifications until they are more sane?

Yes, let's not include the DeviceRemoved notifications yet, at least until we
have a better way to report them. As Scott said, on Chrome OS bluetoothd
automatically removes devices if it hasn't heard from them for a period of time.
So perhaps we try to simulate that here (if we can)? Ideally we wouldn't
condition this at all on whether or not the device is paired and/or connected.

Please investigate if the API calls that you mentioned give us a reliable result
overall. It's fine if you choose to do this in another CL, just make sure to add
a TODO here detailing the behavior that we're aiming for.

[Ilya Sherman, 2014-06-17 22:15:55]

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/341433004/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:144: // pairing.
On 2014/06/17 22:07:22, armansito wrote:
> > Maybe we should report devices as removed if they are not connected and
their
> > -getLastInquiryUpdate: and -recentAccessDate: are both a while ago?  I'd
need
> to
> > investigate whether that does something reasonable.  I'd prefer to punt this
> > work to a separate CL.  For this CL, I'm primarily interested in making sure
> > that connected devices are listed among the |devices_|, so that we don't try
> to
> > dereference a NULL pointer.  Would you prefer that I hold off on adding
> > DeviceRemoved notifications until they are more sane?
> 
> Yes, let's not include the DeviceRemoved notifications yet, at least until we
> have a better way to report them. As Scott said, on Chrome OS bluetoothd
> automatically removes devices if it hasn't heard from them for a period of
time.
> So perhaps we try to simulate that here (if we can)? Ideally we wouldn't
> condition this at all on whether or not the device is paired and/or connected.
> 
> Please investigate if the API calls that you mentioned give us a reliable
result
> overall. It's fine if you choose to do this in another CL, just make sure to
add
> a TODO here detailing the behavior that we're aiming for.

Done.

[armansito, 2014-06-17 22:21:25]

lgtm

[keybuk, 2014-06-17 23:30:21]

lgtm

[Ilya Sherman, 2014-06-17 23:30:32]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-06-17 23:32:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/341433004/40001

[commit-bot: I haz the power, 2014-06-18 13:08:54]

Change committed as 278050