device/bluetooth: Clean up classic discovery in BluetoothAdapterMac.

device/bluetooth: Clean up classic discovery in BluetoothAdapterMac.

This CL cleans up the way the IOBluetoothDeviceInquiry object's state
and the callbacks passed to AddDiscoverySession and
RemoveDiscoverySession are managed by BluetoothAdapterMac. Since the API
functions start and stop already report their result synchronously,
success/failure can be determined without complex tracking of delegate
methods.

This CL moves the explicit restarting of inquiry sessions into separate
class and makes AddDiscoverySession and RemoveDiscoverySession behave in
a way that is consistent with the overall DiscoverySession API.

BUG=381980, 345007
TEST=Tested several parallel discovery sessions on a MacBook Pro with OS
10.9.2

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

[keybuk, 2014-06-09 17:57:39]

https://codereview.chromium.org/319183010/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/319183010/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_mac.h:136: class
BluetoothMacClassicDiscoveryManager {
I dislike having multiple classes in a header like this. Either it needs to be a
class within BluetoothAdapterMac or needs a file of its own

[Ilya Sherman, 2014-06-10 01:11:00]

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth.gyp (right):

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth.gyp:53: 'bluetooth_mac_discovery_manager.h',
nit: "_mac" should be the last component in the file path, so that gyp can wave
its magic wand most magically.

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.h:111: // Discovery manager for classic
discovery.
Optional nit: I'd replace the second "discovery" with "Bluetooth", so that the
five word comment doesn't list "discovery" twice.

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:166: }
Can this be a DCHECK rather than a VLOG?

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:168: VLOG(1) << "Discovery stopped
unexpectedly";
Out of curiousity, why do we use VLOG rather than DVLOG?  In general, debug-only
logging is preferred in Chrome.  Is release logging more useful for Bluetooth
development?

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:188: DCHECK(num_discovery_sessions_ ==
0);
nit: DCHECK_EQ

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:209: // There are active sessions
other than the one currently being removed.
nit: Please move this into the if-stmt.  Typically, comments above if-stmts are
of the form "Check for X", and inside of if-stmts are of the form "X [therefore
Y]".

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_mac_discovery_manager.h (right):

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.h:28: // too
BluetoothMacDiscoveryManager::StopDiscovery or an unexpected reason,
nit: "too" -> "to"

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.h:41: // is already running.
nit: It visually looks like this comment applies to both methods below it; but
it really only describes one of the two.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.h:56: ObserverList<Observer>
observers_;
Do we really need a list of observers, or can we just have a single observer
that's set during construction?

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:48: class
BluetoothMacDiscoveryManagerClassicImpl
nit: "Impl" seems unnecessary, since there's no non-impl version of the class
(other than the superclass which I assume is intended to be the superclass for
both Classic and LE).

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:70: }
Could this be a DCHECK instead?

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:78: }
Why is this reachable?  Could this be a DCHECK instead?

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:101: }
Ditto.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:108: }
This seems wrong, since inquiry_running_ can be set asynchronously.  It seems
like calling StopDiscovery() immediately after StartDiscovery() would currently
not actually stop the discovery.  Or am I missing something?

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:154: // Automatically
restart device inquiry if discovery is still desired.
nit: This comment seems either misplaced, or like an "Otherwise, ..." addition
could help clarify.

[keybuk, 2014-06-10 01:13:12]

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:168: VLOG(1) << "Discovery stopped
unexpectedly";
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> Out of curiousity, why do we use VLOG rather than DVLOG?  In general,
debug-only
> logging is preferred in Chrome.  Is release logging more useful for Bluetooth
> development?

CHROME OS debug builds haven't worked for something like 14+ months, so on that
platform DVLOG is useless

[Ilya Sherman, 2014-06-10 01:14:05]

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:168: VLOG(1) << "Discovery stopped
unexpectedly";
On 2014/06/10 01:13:12, keybuk wrote:
> On 2014/06/10 01:10:59, Ilya Sherman wrote:
> > Out of curiousity, why do we use VLOG rather than DVLOG?  In general,
> debug-only
> > logging is preferred in Chrome.  Is release logging more useful for
Bluetooth
> > development?
> 
> CHROME OS debug builds haven't worked for something like 14+ months, so on
that
> platform DVLOG is useless

Ok, good to know -- thanks :)

[keybuk, 2014-06-10 01:21:03]

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:168: VLOG(1) << "Discovery stopped
unexpectedly";
but this is _mac, so doesn't really apply, I was just explaining why arman and I
might be in the habit of using VLOG ;-)

[Ilya Sherman, 2014-06-10 01:25:13]

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:31: #endif  //
MAC_OS_X_VERSION_10_7
I discovered while rebasing a CL today that apparently this has moved to
base/mac/sdk_forward_declarations.h

[armansito, 2014-06-10 21:56:51]

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth.gyp (right):

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth.gyp:53: 'bluetooth_mac_discovery_manager.h',
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> nit: "_mac" should be the last component in the file path, so that gyp can
wave
> its magic wand most magically.

Done.

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.h:111: // Discovery manager for classic
discovery.
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> Optional nit: I'd replace the second "discovery" with "Bluetooth", so that the
> five word comment doesn't list "discovery" twice.

Done.

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:166: }
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> Can this be a DCHECK rather than a VLOG?

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:168: VLOG(1) << "Discovery stopped
unexpectedly";
On 2014/06/10 01:21:03, keybuk wrote:
> but this is _mac, so doesn't really apply, I was just explaining why arman and
I
> might be in the habit of using VLOG ;-)

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:188: DCHECK(num_discovery_sessions_ ==
0);
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:209: // There are active sessions
other than the one currently being removed.
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> nit: Please move this into the if-stmt.  Typically, comments above if-stmts
are
> of the form "Check for X", and inside of if-stmts are of the form "X
[therefore
> Y]".

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_mac_discovery_manager.h (right):

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.h:28: // too
BluetoothMacDiscoveryManager::StopDiscovery or an unexpected reason,
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> nit: "too" -> "to"

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.h:41: // is already running.
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> nit: It visually looks like this comment applies to both methods below it; but
> it really only describes one of the two.

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.h:56: ObserverList<Observer>
observers_;
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> Do we really need a list of observers, or can we just have a single observer
> that's set during construction?

We can have a single observer, certainly. This just seemed a bit more generic
and cleaner to me but I don't have a preference, so I'll change it if you
insist.

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

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:31: #endif  //
MAC_OS_X_VERSION_10_7
On 2014/06/10 01:25:13, Ilya Sherman wrote:
> I discovered while rebasing a CL today that apparently this has moved to
> base/mac/sdk_forward_declarations.h

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:48: class
BluetoothMacDiscoveryManagerClassicImpl
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> nit: "Impl" seems unnecessary, since there's no non-impl version of the class
> (other than the superclass which I assume is intended to be the superclass for
> both Classic and LE).

You assumed correctly. Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:70: }
On 2014/06/10 01:11:00, Ilya Sherman wrote:
> Could this be a DCHECK instead?

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:78: }
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> Why is this reachable?  Could this be a DCHECK instead?

No, please see my comment below.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:101: }
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> Ditto.

Done.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:108: }
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> This seems wrong, since inquiry_running_ can be set asynchronously.  It seems
> like calling StopDiscovery() immediately after StartDiscovery() would
currently
> not actually stop the discovery.  Or am I missing something?

[inquiry_ start] and [inquiry_ stop] are synchronous, so it shouldn't be
possible to call StopDiscovery before StartDiscovery has returned successfully,
since I assume all of these operations are running on the same thread (the UI
thread?).  With that assumption, if StopDiscovery gets called within 10 seconds
after the call to StartDiscovery, |inquiry_running_| should be true. If
StopDiscovery is called exactly after 10 seconds and we get a race where the
system notifies us that inquiry has stopped and sets |inquiry_running_| to
false, since this method here sets |should_do_discovery_| to false, the inquiry
won't be restarted.

If we ever hit a case where we end up restarting the inquiry, and this method
returns while |inquiry_running_| is still false, then yes, we will be in a
situation where inquiry will keep running for another 10 seconds. Even so, we
won't restart inquiry as long as |should_do_discovery_| is false when inquiry
ends. I think this behavior is OK. Us returning success from StopDiscovery
basically means that we have requested the underlying system to stop discovering
new devices and the request went through successfully, and it will eventually
stop.

This race condition is also the reason why we don't want to have a
DCHECK(!inquiry_running_) in StartDiscovery. If inquiry is still running during
a call to StartDiscovery, we just return true and set |should_do_discovery_| to
true, so that when inquiry ends, it gets automatically restarted.

P.S.: I just tested this by running startDiscovery and stopDiscovery in a loop
from JS, and things seem to work as expected.

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.mm:154: // Automatically
restart device inquiry if discovery is still desired.
On 2014/06/10 01:10:59, Ilya Sherman wrote:
> nit: This comment seems either misplaced, or like an "Otherwise, ..." addition
> could help clarify.

Done.

[Ilya Sherman, 2014-06-10 23:23:01]

LGTM % a couple of nits.  Thanks, Arman!

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_mac_discovery_manager.h (right):

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.h:56: ObserverList<Observer>
observers_;
On 2014/06/10 21:56:50, armansito wrote:
> On 2014/06/10 01:10:59, Ilya Sherman wrote:
> > Do we really need a list of observers, or can we just have a single observer
> > that's set during construction?
> 
> We can have a single observer, certainly. This just seemed a bit more generic
> and cleaner to me but I don't have a preference, so I'll change it if you
> insist.

I won't insist, but IMO it's better to avoid being generic when it's not
necessary.

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

https://codereview.chromium.org/319183010/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:163: DCHECK(manager ==
classic_discovery_manager_.get());
nit: DCHECK_EQ

https://codereview.chromium.org/319183010/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_discovery_manager_mac.mm (right):

https://codereview.chromium.org/319183010/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_discovery_manager_mac.mm:32: #endif  //
MAC_OS_X_VERSION_10_7
To clarify my previous comment, the idea is that all of the forward declarations
should move into sdk_forward_declarations.h.

[armansito, 2014-06-13 02:14:55]

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_mac_discovery_manager.h (right):

https://codereview.chromium.org/319183010/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_mac_discovery_manager.h:56: ObserverList<Observer>
observers_;
On 2014/06/10 23:23:01, Ilya Sherman wrote:
> On 2014/06/10 21:56:50, armansito wrote:
> > On 2014/06/10 01:10:59, Ilya Sherman wrote:
> > > Do we really need a list of observers, or can we just have a single
observer
> > > that's set during construction?
> > 
> > We can have a single observer, certainly. This just seemed a bit more
generic
> > and cleaner to me but I don't have a preference, so I'll change it if you
> > insist.
> 
> I won't insist, but IMO it's better to avoid being generic when it's not
> necessary.

Done.

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

https://codereview.chromium.org/319183010/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_mac.mm:163: DCHECK(manager ==
classic_discovery_manager_.get());
On 2014/06/10 23:23:01, Ilya Sherman wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/319183010/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_discovery_manager_mac.mm (right):

https://codereview.chromium.org/319183010/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_discovery_manager_mac.mm:32: #endif  //
MAC_OS_X_VERSION_10_7
On 2014/06/10 23:23:01, Ilya Sherman wrote:
> To clarify my previous comment, the idea is that all of the forward
declarations
> should move into sdk_forward_declarations.h.

Ah, I misunderstood. Done.

[armansito, 2014-06-13 02:15:03]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-06-13 02:20:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/armansito@chromium.org/319183010/60001

[commit-bot: I haz the power, 2014-06-13 08:37:51]

Change committed as 276973