Adding support for Low Energy device discovery to BluetoothAdapterMac

Adding support for Low Energy device discovery to BluetoothAdapterMac.

Modified BluetoothAdapterMac so that AddDiscoverySession can start low
energy discovery sessions in addition to classic.  Note that the adapter
cannot yet add LE devices found to |devices_| because the ability to
distinguish LE and classic devices, when they are stored as
BluetoothDevice, is not yet implemented.

Also added corresponding tests to bluetooth_adapter_mac_unittest.mm and
created class MockCentralManager.

BUG=496987
Committed: https://crrev.com/c08decbd80f568e4177a4f82942e2ed04c97735e
Cr-Commit-Position: refs/heads/master@{#336062}

[krstnmnlsn, 2015-06-05 02:52:22]

Patchset #1 (id:1) has been deleted

[krstnmnlsn, 2015-06-05 03:24:09]

Patchset #1 (id:20001) has been deleted

[krstnmnlsn, 2015-06-05 03:25:40]

krstnmnlsn@chromium.org changed reviewers:
+ armansito@chromium.org, scheib@chromium.org

[krstnmnlsn, 2015-06-05 03:32:21]

Hey, here is the first of (at least) 2 cls to allow LE device discovery on mac. 
Sorry there are so many files, a lot of those have very minor changes.  Also 3
of the files are testing-related.

Please look over it whenever you have a chance, thanks!

[scheib, 2015-06-05 16:34:57]

The CQ bit was checked by scheib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 16:36:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/40001

[commit-bot: I haz the power, 2015-06-05 17:11:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 17:11:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[scheib, 2015-06-05 18:10:20]

1/2 way through review, but have to stop for now, here's a few items:

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:220: "test/mock_bluetooth_central_manager.h",
Mac specific files should only be built on that platform. The build systems can
do this implicitly by either having a file suffix _mac, be in a mac directory,
or it can be done explicitly with conditional sections of build files. In GN the
conditionals look like "if (is_mac)". In GYP:
"""
  'conditions': [
    ['OS=="mac"', {
"""

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.h:115: 
Comment.

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

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:178: bool
BluetoothAdapterMac::StartDiscovery(
Move to after SetDiscoveryFilter. "Method definitions in the corresponding .cc
file should be the same as the declaration order, as much as possible."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:186: if ((transport &
BluetoothDiscoveryFilter::Transport::TRANSPORT_CLASSIC) &&
We should find a mix of classic and LE devices and test that starting discovery
on both APIs at the same time will work. I can help with that a bit. 

Or, if there's more info already known about this we should document it here in
comments.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:385: // handle instances of
LowEnergyBluetoothDevice in |devices_|.
Add a reference to the issue number to do that work. Just a crbug.com/000000
after the sentence.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:390: NOTIMPLEMENTED();
We expect LE sessions will be started, so we can't throw an error here because
we haven't implemented this yet. Just remove NOTIMPLEMENTED for now.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:394: // instances of
BluetoothDeviceMac.  Add support for low energy devices.
issue number.

[scheib, 2015-06-06 05:09:49]

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

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:213: if
(!StartDiscovery(discovery_filter)) {
Without seeing the plans for how differing filters will be handled I'm uncertain
about this. Previously if multiple AddDiscoverSessions happen the system was
only asked to scan once. I don't know if the OSX APIs care, but I also don't see
a need to call them more than once unless we have a reason to.

Adding Classic and LE simultaneously gives us a reason to do something if first
a discovery session requested only one, but another AddDiscoverySession
requested another one. Perhaps num_discovery_sessions_ should be duplicated for
both Classic and LE counts?

If filters later try to support scanning for differing sets of service UUIDs in
LE, we'll need e.g. merging logic for the filer, or postponing or failing a new
discovery session if one is already under way.

After clearing up the design intent (put it in the design doc, and add comments
here explaining anything (especially the 'why' that isn't obvious from code))
Then add tests for the different scenarios of multiple AddDiscoverySessions.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:26: // Helper methods for
access BluetoothAdapterMacTest's members.
access to

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:106: [mock_central_manager_
setScanForPeripheralsWasCalled:NO];
Would it be easier and perhaps measure behavior more specifically to track call
counts, e.g. setScanForPeripheralCallCount?

[krstnmnlsn, 2015-06-09 01:08:10]

Thanks!

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:220: "test/mock_bluetooth_central_manager.h",
On 2015/06/05 18:10:20, scheib wrote:
> Mac specific files should only be built on that platform. The build systems
can
> do this implicitly by either having a file suffix _mac, be in a mac directory,
> or it can be done explicitly with conditional sections of build files. In GN
the
> conditionals look like "if (is_mac)". In GYP:
> """
>   'conditions': [
>     ['OS=="mac"', {
> """

Alright, please check if I've got it correct now.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.h:115: 
On 2015/06/05 18:10:20, scheib wrote:
> Comment.

Done.

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

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:178: bool
BluetoothAdapterMac::StartDiscovery(
On 2015/06/05 18:10:20, scheib wrote:
> Move to after SetDiscoveryFilter. "Method definitions in the corresponding .cc
> file should be the same as the declaration order, as much as possible."
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Right.  I went ahead and moved RemovePairingDelegateInteral as well.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:186: if ((transport &
BluetoothDiscoveryFilter::Transport::TRANSPORT_CLASSIC) &&
On 2015/06/05 18:10:20, scheib wrote:
> We should find a mix of classic and LE devices and test that starting
discovery
> on both APIs at the same time will work. I can help with that a bit. 
> 
> Or, if there's more info already known about this we should document it here
in
> comments.

Manually tested and added a short comment (cannot find any information online).

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:213: if
(!StartDiscovery(discovery_filter)) {
On 2015/06/06 05:09:48, scheib wrote:
> Without seeing the plans for how differing filters will be handled I'm
uncertain
> about this. Previously if multiple AddDiscoverSessions happen the system was
> only asked to scan once. I don't know if the OSX APIs care, but I also don't
see
> a need to call them more than once unless we have a reason to.
> 
> Adding Classic and LE simultaneously gives us a reason to do something if
first
> a discovery session requested only one, but another AddDiscoverySession
> requested another one. Perhaps num_discovery_sessions_ should be duplicated
for
> both Classic and LE counts?
> 
> If filters later try to support scanning for differing sets of service UUIDs
in
> LE, we'll need e.g. merging logic for the filer, or postponing or failing a
new
> discovery session if one is already under way.
> 
> After clearing up the design intent (put it in the design doc, and add
comments
> here explaining anything (especially the 'why' that isn't obvious from code))
> Then add tests for the different scenarios of multiple AddDiscoverySessions.

Recap/continuing of offline discussion: 
The plans for handling of differing filters have mostly already been made in the
BluetoothAdapter class (see: GetMergedDiscoveryFilter).  We can handle merging
(and editing) the set of filters, and notifying the system of a filter change
will be consistent with Chromeos and what (I was interpreting to be) the desired
cross-platform behavior.

The LE API does not mind repeated calls to start bluetooth/peripheral discovery,
the classic API might.  That is why the classic part of StartDiscovery currently
does nothing on a repeated call (this was the previous behavior as well). 
Issue: crbug.com/498056

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:385: // handle instances of
LowEnergyBluetoothDevice in |devices_|.
On 2015/06/05 18:10:20, scheib wrote:
> Add a reference to the issue number to do that work. Just a crbug.com/000000
> after the sentence. 

Done.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:390: NOTIMPLEMENTED();
On 2015/06/05 18:10:20, scheib wrote:
> We expect LE sessions will be started, so we can't throw an error here because
> we haven't implemented this yet. Just remove NOTIMPLEMENTED for now.

Done.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:394: // instances of
BluetoothDeviceMac.  Add support for low energy devices.
On 2015/06/05 18:10:20, scheib wrote:
> issue number.

Done.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:26: // Helper methods for
access BluetoothAdapterMacTest's members.
On 2015/06/06 05:09:48, scheib wrote:
> access to

Done.

https://codereview.chromium.org/1165053003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:106: [mock_central_manager_
setScanForPeripheralsWasCalled:NO];
On 2015/06/06 05:09:48, scheib wrote:
> Would it be easier and perhaps measure behavior more specifically to track
call
> counts, e.g. setScanForPeripheralCallCount?

Sure, I was already debating switching it.

[armansito, 2015-06-09 19:32:59]

lgtm with a few nits. The code is getting cleaner already!

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:245: if (is_mac) {
I don't think you need the is_mac check here. The *_mac.* part of the file name
should take care of that, no (see src/build/config/BUILDCONFIG.gn)?

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth.gyp (right):

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth.gyp:235: ['OS=="mac"', {
Is this check needed?

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (left):

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:435: const ErrorCallback& error_callback) =
0;
nit: New line before the comment block (keeps things visually easier to parse).

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:426: // greater than 0 and
AddDiscoverySession (RemoveDiscoverySession) is called
I'm confused by the parentheses? Do you mean "AddDiscoverySession or
RemoveDiscoverySession is called"?

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:439: // bluetooth controller.
nit: s/bluetooth/Bluetooth/

[krstnmnlsn, 2015-06-09 23:09:01]

nits done, thanks Arman

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/BUILD....
device/bluetooth/BUILD.gn:245: if (is_mac) {
On 2015/06/09 19:32:58, armansito wrote:
> I don't think you need the is_mac check here. The *_mac.* part of the file
name
> should take care of that, no (see src/build/config/BUILDCONFIG.gn)?

Done.

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth.gyp (right):

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth.gyp:235: ['OS=="mac"', {
On 2015/06/09 19:32:58, armansito wrote:
> Is this check needed?

Nope, apparently not :)

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (left):

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:435: const ErrorCallback& error_callback) =
0;
On 2015/06/09 19:32:59, armansito wrote:
> nit: New line before the comment block (keeps things visually easier to
parse).

Done.

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:426: // greater than 0 and
AddDiscoverySession (RemoveDiscoverySession) is called
On 2015/06/09 19:32:58, armansito wrote:
> I'm confused by the parentheses? Do you mean "AddDiscoverySession or
> RemoveDiscoverySession is called"?

Yep, switched.

https://codereview.chromium.org/1165053003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:439: // bluetooth controller.
On 2015/06/09 19:32:59, armansito wrote:
> nit: s/bluetooth/Bluetooth/

Done.

[krstnmnlsn, 2015-06-11 18:25:32]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-11 18:25:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps100001
(title: "arman nits")

[commit-bot: I haz the power, 2015-06-11 18:27:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/100001

[commit-bot: I haz the power, 2015-06-11 18:27:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 18:27:29]

Dry run: CLs for remote refs other than refs/pending/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[scheib, 2015-06-11 19:44:04]

The CQ bit was checked by scheib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-11 19:45:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/100001

[commit-bot: I haz the power, 2015-06-11 19:45:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 19:45:17]

Dry run: CLs for remote refs other than refs/pending/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[krstnmnlsn, 2015-06-11 20:52:41]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-11 20:52:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps120001
(title: "trying to fix upstream")

[commit-bot: I haz the power, 2015-06-11 20:54:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/120001

[scheib, 2015-06-11 21:13:31]

Patchset #4 (id:100001) has been deleted

[scheib, 2015-06-11 21:22:00]

LGTM

[commit-bot: I haz the power, 2015-06-11 21:29:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 21:29:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[krstnmnlsn, 2015-06-11 22:17:03]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-11 22:17:03]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps140001
(title: "importing CoreBluetooth to the discovery manager")

[commit-bot: I haz the power, 2015-06-11 22:18:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/140001

[commit-bot: I haz the power, 2015-06-11 22:49:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 22:49:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[krstnmnlsn, 2015-06-12 02:25:47]

Patchset #5 (id:140001) has been deleted

[krstnmnlsn, 2015-06-12 17:02:47]

Patchset #5 (id:160001) has been deleted

[krstnmnlsn, 2015-06-12 18:07:45]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-12 18:07:46]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps180001
(title: "trying new selector")

[commit-bot: I haz the power, 2015-06-12 18:08:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/180001

[commit-bot: I haz the power, 2015-06-12 18:41:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-12 18:41:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[krstnmnlsn, 2015-06-12 22:34:33]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-12 22:34:34]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps200001
(title: "should now compile on < 10.7")

[commit-bot: I haz the power, 2015-06-12 22:37:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/200001

[commit-bot: I haz the power, 2015-06-12 23:04:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-12 23:04:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[krstnmnlsn, 2015-06-13 00:01:14]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-13 00:01:15]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps220001
(title: "test, please ignore.")

[commit-bot: I haz the power, 2015-06-13 00:04:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/220001

[krstnmnlsn, 2015-06-13 00:25:56]

Patchset #7 (id:220001) has been deleted

[krstnmnlsn, 2015-06-13 00:34:13]

Patchset #7 (id:240001) has been deleted

[krstnmnlsn, 2015-06-13 00:35:29]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-13 00:35:30]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps260001
(title: "try 1million")

[commit-bot: I haz the power, 2015-06-13 00:36:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/260001

[krstnmnlsn, 2015-06-13 00:48:17]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-13 00:48:18]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps240002
(title: "3billionth time's the charm")

[commit-bot: I haz the power, 2015-06-13 00:50:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/240002

[krstnmnlsn, 2015-06-13 01:22:47]

Patchset #8 (id:240002) has been deleted

[krstnmnlsn, 2015-06-13 01:23:08]

Patchset #7 (id:260001) has been deleted

[krstnmnlsn, 2015-06-13 01:57:35]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-13 01:57:35]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps290001
(title: "???")

[commit-bot: I haz the power, 2015-06-13 01:58:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/290001

[commit-bot: I haz the power, 2015-06-13 02:23:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-13 02:23:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[krstnmnlsn, 2015-06-16 00:20:01]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-16 00:20:02]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps310001
(title: "adding sdk_forward_declarations to the gyp & gn")

[commit-bot: I haz the power, 2015-06-16 00:21:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/310001

[commit-bot: I haz the power, 2015-06-16 00:39:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-16 00:39:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[krstnmnlsn, 2015-06-16 00:47:15]

Patchset #8 (id:310001) has been deleted

[krstnmnlsn, 2015-06-16 00:49:33]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-16 00:49:34]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps330001
(title: "adding sdk_forward_decl to mock target")

[commit-bot: I haz the power, 2015-06-16 00:49:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/330001

[krstnmnlsn, 2015-06-16 01:14:37]

Patchset #8 (id:330001) has been deleted

[krstnmnlsn, 2015-06-16 01:17:16]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-16 01:17:16]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps350001
(title: "adding sdk_forward_declarations to gyp & gn")

[commit-bot: I haz the power, 2015-06-16 01:18:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/350001

[commit-bot: I haz the power, 2015-06-16 01:43:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-16 01:43:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[krstnmnlsn, 2015-06-16 21:14:48]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-16 21:14:49]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps370001
(title: "fixing gyp and gn paths")

[commit-bot: I haz the power, 2015-06-16 21:16:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/370001

[commit-bot: I haz the power, 2015-06-16 21:53:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-16 21:53:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[krstnmnlsn, 2015-06-17 00:13:15]

Patchset #9 (id:370001) has been deleted

[krstnmnlsn, 2015-06-17 00:13:31]

Patchset #8 (id:350001) has been deleted

[krstnmnlsn, 2015-06-17 00:14:03]

Patchset #7 (id:290001) has been deleted

[krstnmnlsn, 2015-06-17 00:14:42]

Patchset #6 (id:200001) has been deleted

[krstnmnlsn, 2015-06-17 00:16:40]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-17 00:16:40]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps390001
(title: "removing references to CBCentralManager")

[commit-bot: I haz the power, 2015-06-17 00:17:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/390001

[krstnmnlsn, 2015-06-17 00:35:49]

So I think it's finally going to compile on 10.6 (still waiting on a few of the
try bots).  Tried to use OCMockObject but ran into some trouble (commented in
the change) so a slightly ugly workaround was employed instead.

[commit-bot: I haz the power, 2015-06-17 02:08:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-17 02:08:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scheib, 2015-06-17 16:55:08]

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:33: // Cannot use a
OCMockObject because mocking 'state' property gives a
I haven't seen OCMockObject use yet, is this comment better placed at the
definition of MockCentralManager?

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:34: // compiler warning when
mock_central_manager of type id (multiple methods
"when mock_central_manager of type id" confuses me a bit. Should this read "when
mock_central_manager is of type id"

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:38:
adapter_mac_->low_energy_discovery_manager_->SetManager(
I think the logic should clearly skip the test if the manager can't be set. We
can either have SetManagerForTesting return a bool if it is successfull, or just
add the base::mac::IsOSLionOrLater() logic check here up above and emit the
warning and return false.

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:74: virtual void
SetManager(CBCentralManager* manager);
If it is only called in tests, rename to SetManagerForTesting.
https://www.chromium.org/developers/coding-style#Naming

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:143: // This
method should not be called on OS X < 10.7.
How about, "setDelegate is only available in OSX 10.7 and later."

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:148: }
Either return a bool indicating if this was successful, or maybe simpler,
Comment that it's LionOrLater and add a CHECK(base::mac::IsOSLionOrLater())
here, remove the 'if'.

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.h (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.h:16: @interface
MockCentralManager : NSObject
// Mocks a CBCentralManager

[krstnmnlsn, 2015-06-17 20:04:09]

Patchset #7 (id:410001) has been deleted

[krstnmnlsn, 2015-06-17 20:18:41]

Thanks Vincent!

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:33: // Cannot use a
OCMockObject because mocking 'state' property gives a
On 2015/06/17 16:55:08, scheib wrote:
> I haven't seen OCMockObject use yet, is this comment better placed at the
> definition of MockCentralManager?

Done.

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:34: // compiler warning when
mock_central_manager of type id (multiple methods
On 2015/06/17 16:55:08, scheib wrote:
> "when mock_central_manager of type id" confuses me a bit. Should this read
"when
> mock_central_manager is of type id"

Done.

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:38:
adapter_mac_->low_energy_discovery_manager_->SetManager(
On 2015/06/17 16:55:08, scheib wrote:
> I think the logic should clearly skip the test if the manager can't be set. We
> can either have SetManagerForTesting return a bool if it is successfull, or
just
> add the base::mac::IsOSLionOrLater() logic check here up above and emit the
> warning and return false.

The check in SetManagerForTesting() is redundant with the one already here (I
think checking CBCentralManager != nil is actually stronger because Apple might
deprecate CBCentralManager is some OS that is LionOrLater).  I added it in case
some other function calls SetManagerForTesting() without already checking
CoreBluetooth is available.  But yes in that case giving some indication of the
failed manager would be a good idea. Changing the if statement to a
CHECK(LionOrLater) should handle that (?)

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:74: virtual void
SetManager(CBCentralManager* manager);
On 2015/06/17 16:55:08, scheib wrote:
> If it is only called in tests, rename to SetManagerForTesting.
> https://www.chromium.org/developers/coding-style#Naming

Done.

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:143: // This
method should not be called on OS X < 10.7.
On 2015/06/17 16:55:08, scheib wrote:
> How about, "setDelegate is only available in OSX 10.7 and later."

Done.

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:148: }
On 2015/06/17 16:55:08, scheib wrote:
> Either return a bool indicating if this was successful, or maybe simpler,
> Comment that it's LionOrLater and add a CHECK(base::mac::IsOSLionOrLater())
> here, remove the 'if'.

I like the CHECK idea.

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.h (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.h:16: @interface
MockCentralManager : NSObject
On 2015/06/17 16:55:08, scheib wrote:
> // Mocks a CBCentralManager

Done.

[krstnmnlsn, 2015-06-17 20:22:29]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-17 20:22:29]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps430001
(title: "scheib fixes #2, also adding delegate property to mock ")

[commit-bot: I haz the power, 2015-06-17 20:23:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/430001

[commit-bot: I haz the power, 2015-06-18 01:56:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-18 01:56:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scheib, 2015-06-18 18:11:07]

LGTM again

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1165053003/diff/390001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:38:
adapter_mac_->low_energy_discovery_manager_->SetManager(
On 2015/06/17 20:18:41, krstnmnlsn wrote:
> On 2015/06/17 16:55:08, scheib wrote:
> > I think the logic should clearly skip the test if the manager can't be set.
We
> > can either have SetManagerForTesting return a bool if it is successfull, or
> just
> > add the base::mac::IsOSLionOrLater() logic check here up above and emit the
> > warning and return false.
> 
> The check in SetManagerForTesting() is redundant with the one already here (I
> think checking CBCentralManager != nil is actually stronger because Apple
might
> deprecate CBCentralManager is some OS that is LionOrLater).  I added it in
case
> some other function calls SetManagerForTesting() without already checking
> CoreBluetooth is available.  But yes in that case giving some indication of
the
> failed manager would be a good idea. Changing the if statement to a
> CHECK(LionOrLater) should handle that (?)

CHECK handles it. Wouldn't hurt to mention in the .h comment for
SetManagerForTesting the requirement of IsOSLionOrLater.

[krstnmnlsn, 2015-06-19 17:14:41]

Patchset #9 (id:470001) has been deleted

[krstnmnlsn, 2015-06-19 17:14:55]

Patchset #8 (id:450001) has been deleted

[krstnmnlsn, 2015-06-19 18:57:34]

Patchset #8 (id:490001) has been deleted

[krstnmnlsn, 2015-06-19 19:26:49]

Patchset #8 (id:510001) has been deleted

[krstnmnlsn, 2015-06-19 19:50:57]

Patchset #8 (id:530001) has been deleted

[krstnmnlsn, 2015-06-22 22:01:49]

Patchset #9 (id:570001) has been deleted

[krstnmnlsn, 2015-06-22 22:01:56]

Patchset #8 (id:550001) has been deleted

[krstnmnlsn, 2015-06-24 21:48:42]

Patchset #26 (id:930001) has been deleted

[krstnmnlsn, 2015-06-24 21:48:49]

Patchset #25 (id:910001) has been deleted

[krstnmnlsn, 2015-06-24 21:48:57]

Patchset #24 (id:890001) has been deleted

[krstnmnlsn, 2015-06-24 21:49:05]

Patchset #23 (id:870001) has been deleted

[krstnmnlsn, 2015-06-24 21:49:14]

Patchset #22 (id:850001) has been deleted

[krstnmnlsn, 2015-06-24 21:49:29]

Patchset #21 (id:760002) has been deleted

[krstnmnlsn, 2015-06-24 21:49:32]

Patchset #18 (id:780001) has been deleted

[krstnmnlsn, 2015-06-24 21:49:57]

Patchset #19 (id:820001) has been deleted

[krstnmnlsn, 2015-06-24 21:50:02]

Patchset #17 (id:760001) has been deleted

[krstnmnlsn, 2015-06-24 21:50:38]

Patchset #17 (id:800001) has been deleted

[krstnmnlsn, 2015-06-24 21:50:47]

Patchset #16 (id:750001) has been deleted

[krstnmnlsn, 2015-06-24 21:50:55]

Patchset #15 (id:730001) has been deleted

[krstnmnlsn, 2015-06-24 21:51:01]

Patchset #14 (id:710001) has been deleted

[krstnmnlsn, 2015-06-24 21:51:15]

Patchset #13 (id:690001) has been deleted

[krstnmnlsn, 2015-06-24 21:51:19]

Patchset #11 (id:650001) has been deleted

[krstnmnlsn, 2015-06-24 21:51:28]

Patchset #9 (id:610001) has been deleted

[krstnmnlsn, 2015-06-24 21:52:19]

Patchset #10 (id:670001) has been deleted

[krstnmnlsn, 2015-06-24 21:52:28]

Patchset #9 (id:630001) has been deleted

[krstnmnlsn, 2015-06-24 21:52:36]

Patchset #8 (id:590001) has been deleted

[krstnmnlsn, 2015-06-24 22:23:43]

Patchset #8 (id:950001) has been deleted

[krstnmnlsn, 2015-06-24 22:42:59]

The CQ bit was checked by krstnmnlsn@chromium.org to run a CQ dry run

[krstnmnlsn, 2015-06-24 22:43:00]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps990001
(title: "comment nit on SetManagerForTesting.")

[commit-bot: I haz the power, 2015-06-24 22:43:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/990001

[commit-bot: I haz the power, 2015-06-24 23:12:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 23:12:43]

Dry run: Exceeded global retry quota

[scheib, 2015-06-24 23:24:25]

https://codereview.chromium.org/1165053003/diff/990001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right):

https://codereview.chromium.org/1165053003/diff/990001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:83: // Underlying
CoreBluetooth central manager.  Deallocating CBCentralManager
  // Underlying CoreBluetooth central manager.
  // 
  // Note: Intentionally leaked. Deallocating CBCentralManager
  // results in a crash, at least when running OSX 10.9.5 on a
  // mac_chromuim_rel_ng trybot. On the other hand, restricting |manager_| use
  // to 10.10 and later would mean the code is unrun and untested by the current
  // trybots. To work around this we call retain on |manager_| after allocation,
  // so that the object is leaked.

https://codereview.chromium.org/1165053003/diff/990001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right):

https://codereview.chromium.org/1165053003/diff/990001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:174: [manager_
retain];
Add a comment here referencing the explanation in the header.
// Increment reference count, see comment at declaration.

[krstnmnlsn, 2015-06-24 23:42:26]

comments fixed!

https://codereview.chromium.org/1165053003/diff/990001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right):

https://codereview.chromium.org/1165053003/diff/990001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:83: // Underlying
CoreBluetooth central manager.  Deallocating CBCentralManager
On 2015/06/24 23:24:24, scheib wrote:
>   // Underlying CoreBluetooth central manager.
>   // 
>   // Note: Intentionally leaked. Deallocating CBCentralManager
>   // results in a crash, at least when running OSX 10.9.5 on a
>   // mac_chromuim_rel_ng trybot. On the other hand, restricting |manager_| use
>   // to 10.10 and later would mean the code is unrun and untested by the
current
>   // trybots. To work around this we call retain on |manager_| after
allocation,
>   // so that the object is leaked.

Done.

https://codereview.chromium.org/1165053003/diff/990001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right):

https://codereview.chromium.org/1165053003/diff/990001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:174: [manager_
retain];
On 2015/06/24 23:24:25, scheib wrote:
> Add a comment here referencing the explanation in the header.
> // Increment reference count, see comment at declaration.

Done.

[scheib, 2015-06-25 00:06:26]

LGTM

[krstnmnlsn, 2015-06-25 00:31:37]

The CQ bit was checked by krstnmnlsn@chromium.org

[krstnmnlsn, 2015-06-25 00:31:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1165053003/#ps1010001
(title: "comment fixes")

[commit-bot: I haz the power, 2015-06-25 00:31:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165053003/1010001

[commit-bot: I haz the power, 2015-06-25 01:26:25]

Committed patchset #10 (id:1010001)

[commit-bot: I haz the power, 2015-06-25 01:27:24]

Patchset 10 (id:??) landed as
https://crrev.com/c08decbd80f568e4177a4f82942e2ed04c97735e
Cr-Commit-Position: refs/heads/master@{#336062}