Bringing 4 more BluetoothTest.* unit tests to Mac

Modified BluetoothTestMac to support device discovery tests.

Also implemented utility function FindCBUUIDForHashTarget() to find
relatively simple CBUUIDs mapping (under SHA256 plus truncating) to
a 6 byte address with the first 3 bytes specified.  Used to choose
CBUUID / bluetooth address pairs for testing.

BUG=511008
Committed: https://crrev.com/5a0eceb28201302bcb45621015119bd73351d659
Cr-Commit-Position: refs/heads/master@{#341400}

Committed: https://crrev.com/41c13ac0fcc9dbd8077a109df59b12eb6a8e27f8
Cr-Commit-Position: refs/heads/master@{#341433}

[krstnmnlsn, 2015-07-22 23:09:23]

Patchset #1 (id:1) has been deleted

[krstnmnlsn, 2015-07-22 23:20:56]

Patchset #1 (id:20001) has been deleted

[krstnmnlsn, 2015-07-22 23:23:17]

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

[krstnmnlsn, 2015-07-22 23:23:17]

Ok here's the patch at last!

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_unittest.cc (left):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_unittest.cc:514: EXPECT_EQ(0,
observer.device_changed_count());
As previously discussed, it's not clear if / how we efficiently check if a
device has changed when BluetoothLowEnergyDeviceMac::Update is called.  Could
make it check only the fields we care about, or else this line has to go..

[scheib, 2015-07-23 18:15:25]

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_unittest.cc (left):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_unittest.cc:514: EXPECT_EQ(0,
observer.device_changed_count());
On 2015/07/22 23:23:17, krstnmnlsn wrote:
> As previously discussed, it's not clear if / how we efficiently check if a
> device has changed when BluetoothLowEnergyDeviceMac::Update is called.  Could
> make it check only the fields we care about, or else this line has to go..

Well, in the implementation of Update (and all methods it calls) the new data
would need to be compared with the previously stored data to see if it were
different. It's a bit of work, and I'm not sure that we need it now. The
difficulty is that it will be hard to notice later if it produces a lot of
unnecessary work. But, OK with removing this line and sending update for now.

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

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.h:122: UUIDList uuids_;
For clarity rename this to advertised_uuids_ -- we'll need to later merge this
with discovered uuids as well.

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

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.mm:57: for (CBUUID* uuid in
service_data) {
ClearServiceData(); before setting values, so that old service data isn't kept?

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:20: const std::string
BluetoothTestBase::kTestUUID1 = "1800";
These have better names, they're from
https://developer.bluetooth.org/gatt/services/Pages/ServicesHome.aspx

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:54: //      Address AA:00:00:00:00:01 on
Android. 01:00:00:90:1E:BE on Mac.
Please change the address on Android as well so that only one is used.

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:63: // Check if Low Energy is available.
On Mac, we require OS X >= 10.10.
Logically this is used very early, so it fits better higher up. Move this up to
just after the destructor (and update order in other files too).

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:174: //
http://www.cs.berkeley.edu/~jfc/cs174/lecs/lec10/lec10.pdf.
Nice try at nerd sniping https://xkcd.com/356/
I still live and you only stole 10 minutes of life.

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:187: break;
Don't need a break; after a return;

[krstnmnlsn, 2015-07-23 22:03:29]

Java functional, comments addressed!

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_unittest.cc (left):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_unittest.cc:514: EXPECT_EQ(0,
observer.device_changed_count());
On 2015/07/23 18:15:25, scheib wrote:
> On 2015/07/22 23:23:17, krstnmnlsn wrote:
> > As previously discussed, it's not clear if / how we efficiently check if a
> > device has changed when BluetoothLowEnergyDeviceMac::Update is called. 
Could
> > make it check only the fields we care about, or else this line has to go..
> 
> Well, in the implementation of Update (and all methods it calls) the new data
> would need to be compared with the previously stored data to see if it were
> different. It's a bit of work, and I'm not sure that we need it now. The
> difficulty is that it will be hard to notice later if it produces a lot of
> unnecessary work. But, OK with removing this line and sending update for now. 

Yes, and the reason I'm hesitant on it is because my understanding from the
Apple doc is that the didDiscoverPeripheral: message should only be sent when a
peripheral has been discovered for the first time or really has an update.  See
the CBCentralManagerScanOptionAllowDuplicatesKey scanning option (which defaults
to NO)
https://developer.apple.com/library/prerelease/ios/documentation/CoreBluetoot...

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

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.h:122: UUIDList uuids_;
On 2015/07/23 18:15:25, scheib wrote:
> For clarity rename this to advertised_uuids_ -- we'll need to later merge this
> with discovered uuids as well.

sure.  And you mean we will put uuids we discover later in |advertised_uuids_|
as well?

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

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.mm:57: for (CBUUID* uuid in
service_data) {
On 2015/07/23 18:15:25, scheib wrote:
> ClearServiceData(); before setting values, so that old service data isn't
kept?

The call was further above.  Moved it down for clarity though.

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:20: const std::string
BluetoothTestBase::kTestUUID1 = "1800";
On 2015/07/23 18:15:25, scheib wrote:
> These have better names, they're from
> https://developer.bluetooth.org/gatt/services/Pages/ServicesHome.aspx

Done.

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:54: //      Address AA:00:00:00:00:01 on
Android. 01:00:00:90:1E:BE on Mac.
On 2015/07/23 18:15:25, scheib wrote:
> Please change the address on Android as well so that only one is used.

Done.

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:63: // Check if Low Energy is available.
On Mac, we require OS X >= 10.10.
On 2015/07/23 18:15:25, scheib wrote:
> Logically this is used very early, so it fits better higher up. Move this up
to
> just after the destructor (and update order in other files too). 

Done.

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:174: //
http://www.cs.berkeley.edu/~jfc/cs174/lecs/lec10/lec10.pdf.
On 2015/07/23 18:15:25, scheib wrote:
> Nice try at nerd sniping https://xkcd.com/356/
> I still live and you only stole 10 minutes of life.

Haha! next time..

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_mac.mm:187: break;
On 2015/07/23 18:15:25, scheib wrote:
> Don't need a break; after a return; 

:P

[scheib, 2015-07-23 22:05:39]

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

[commit-bot: I haz the power, 2015-07-23 22:06:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246913006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246913006/60001

[scheib, 2015-07-23 22:15:39]

LGTM

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

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.h:122: UUIDList uuids_;
On 2015/07/23 22:03:29, krstnmnlsn wrote:
> On 2015/07/23 18:15:25, scheib wrote:
> > For clarity rename this to advertised_uuids_ -- we'll need to later merge
this
> > with discovered uuids as well.
> 
> sure.  And you mean we will put uuids we discover later in |advertised_uuids_|
> as well?

GetUUIDs will need to return the merge of advertised_uuids_ and discovered
uuids.

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

https://codereview.chromium.org/1246913006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.mm:57: for (CBUUID* uuid in
service_data) {
On 2015/07/23 22:03:29, krstnmnlsn wrote:
> On 2015/07/23 18:15:25, scheib wrote:
> > ClearServiceData(); before setting values, so that old service data isn't
> kept?
> 
> The call was further above.  Moved it down for clarity though.

Oh! great. Hmm, yeah, line breaks for sections would have helped a bit too here,
no need to change though.

[commit-bot: I haz the power, 2015-07-23 22:18:11]

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

[commit-bot: I haz the power, 2015-07-23 22:18:11]

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

[krstnmnlsn, 2015-07-24 20:17:51]

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

[krstnmnlsn, 2015-07-24 20:17:51]

Arman:
Continuing to bring up Vincent's bluetooth testing on mac.  Please review when
you get a chance!

[armansito, 2015-07-28 00:55:57]

lgtm

[krstnmnlsn, 2015-07-28 16:56:54]

Patchset #4 (id:100001) has been deleted

[krstnmnlsn, 2015-07-28 17:48:34]

krstnmnlsn@chromium.org changed reviewers:
+ davidben@chromium.org

[krstnmnlsn, 2015-07-28 17:48:34]

Hey David, check this over if you get a chance (waiting to hear back from
rsleevi).  Crypto is used at the end of
device/bluetooth/test/bluetooth_test_mac.mm in a function that is never called,
and just there if Vincent wants to generate more test addresses that correspond
to nice UUIDs in the future.

[davidben, 2015-07-28 18:03:15]

On 2015/07/28 17:48:34, krstnmnlsn wrote:
> Hey David, check this over if you get a chance (waiting to hear back from
> rsleevi).  Crypto is used at the end of
> device/bluetooth/test/bluetooth_test_mac.mm in a function that is never
called,
> and just there if Vincent wants to generate more test addresses that
correspond
> to nice UUIDs in the future.

I'm confused. Why do you need to find strings that end up with a particular
prefix in the hash? (If it's unused and just used to generate test data, seems a
standalone Python script or something would be simpler.)

The function you checked in also seems to be totally deterministic. So you may
as well turn it into return "whatever the answer works out to be".

[scheib, 2015-07-28 18:23:14]

On 2015/07/28 18:03:15, David Benjamin wrote:
> On 2015/07/28 17:48:34, krstnmnlsn wrote:
> > Hey David, check this over if you get a chance (waiting to hear back from
> > rsleevi).  Crypto is used at the end of
> > device/bluetooth/test/bluetooth_test_mac.mm in a function that is never
> called,
> > and just there if Vincent wants to generate more test addresses that
> correspond
> > to nice UUIDs in the future.
> 
> I'm confused. Why do you need to find strings that end up with a particular
> prefix in the hash? (If it's unused and just used to generate test data, seems
a
> standalone Python script or something would be simpler.)

I've asked that the code to generate new pairs of IDs and hashes be kept close
to the test logic that relies on this functionality. This improves code
discover-ability and usability IMHO. 

> 
> The function you checked in also seems to be totally deterministic. So you may
> as well turn it into return "whatever the answer works out to be".

The function is designed to be used as necessary to generate new ID/Hash pairs.
Developers would modify as needed when generating new pairs.

[krstnmnlsn, 2015-07-28 18:31:38]

On 2015/07/28 18:03:15, David Benjamin wrote:
> On 2015/07/28 17:48:34, krstnmnlsn wrote:
> > Hey David, check this over if you get a chance (waiting to hear back from
> > rsleevi).  Crypto is used at the end of
> > device/bluetooth/test/bluetooth_test_mac.mm in a function that is never
> called,
> > and just there if Vincent wants to generate more test addresses that
> correspond
> > to nice UUIDs in the future.
> 
> I'm confused. Why do you need to find strings that end up with a particular
> prefix in the hash? (If it's unused and just used to generate test data, seems
a
> standalone Python script or something would be simpler.)
> 
> The function you checked in also seems to be totally deterministic. So you may
> as well turn it into return "whatever the answer works out to be".

Yeah, so the background is on Mac we only get 128-bit bluetooth device
identifiers, but within Chrome we need 48 bit bluetooth device addresses.  We
decided to temporarily create fake addresses by hashing on Mac (eventually we
will move all platforms over to device identifiers instead).  For testing, then
we need 128-bit identifiers that we know the hash of, and it would be nice if
both the id and the hash were man-made looking (and not completely random). 
Vincent wanted me to check in the code I used to generate the hashes, in case he
needs it after I finish this internship.  (and yes, he would have to tell it to
look at a different range because of the determinism)

[davidben, 2015-07-30 23:13:25]

On 2015/07/28 18:31:38, krstnmnlsn wrote:
> On 2015/07/28 18:03:15, David Benjamin wrote:
> > On 2015/07/28 17:48:34, krstnmnlsn wrote:
> > > Hey David, check this over if you get a chance (waiting to hear back from
> > > rsleevi).  Crypto is used at the end of
> > > device/bluetooth/test/bluetooth_test_mac.mm in a function that is never
> > called,
> > > and just there if Vincent wants to generate more test addresses that
> > correspond
> > > to nice UUIDs in the future.
> > 
> > I'm confused. Why do you need to find strings that end up with a particular
> > prefix in the hash? (If it's unused and just used to generate test data,
seems
> a
> > standalone Python script or something would be simpler.)
> > 
> > The function you checked in also seems to be totally deterministic. So you
may
> > as well turn it into return "whatever the answer works out to be".
> 
> Yeah, so the background is on Mac we only get 128-bit bluetooth device
> identifiers, but within Chrome we need 48 bit bluetooth device addresses.  We
> decided to temporarily create fake addresses by hashing on Mac (eventually we
> will move all platforms over to device identifiers instead).  For testing,
then
> we need 128-bit identifiers that we know the hash of, and it would be nice if
> both the id and the hash were man-made looking (and not completely random). 
> Vincent wanted me to check in the code I used to generate the hashes, in case
he
> needs it after I finish this internship.  (and yes, he would have to tell it
to
> look at a different range because of the determinism)

Why do they need to be man-made looking? Hashes with 00s in the front are still
possible, so you probably don't want to be doing anything programmatic with
them.


> The function is designed to be used as necessary to generate new ID/Hash
pairs.
> Developers would modify as needed when generating new pairs.

Without knowing that, it seems someone reading the code would be very confused
what that function is for. It's totally deterministic, does quite a bit of
computation, and never called. Probably at least wants a comment that it's just
some sample code. (If it's just sample code, maybe just put it in a comment
somewhere?)

[krstnmnlsn, 2015-07-31 00:21:00]

On 2015/07/30 23:13:25, David Benjamin wrote:
> On 2015/07/28 18:31:38, krstnmnlsn wrote:
> > On 2015/07/28 18:03:15, David Benjamin wrote:
> > > On 2015/07/28 17:48:34, krstnmnlsn wrote:
> > > > Hey David, check this over if you get a chance (waiting to hear back
from
> > > > rsleevi).  Crypto is used at the end of
> > > > device/bluetooth/test/bluetooth_test_mac.mm in a function that is never
> > > called,
> > > > and just there if Vincent wants to generate more test addresses that
> > > correspond
> > > > to nice UUIDs in the future.
> > > 
> > > I'm confused. Why do you need to find strings that end up with a
particular
> > > prefix in the hash? (If it's unused and just used to generate test data,
> seems
> > a
> > > standalone Python script or something would be simpler.)
> > > 
> > > The function you checked in also seems to be totally deterministic. So you
> may
> > > as well turn it into return "whatever the answer works out to be".
> > 
> > Yeah, so the background is on Mac we only get 128-bit bluetooth device
> > identifiers, but within Chrome we need 48 bit bluetooth device addresses. 
We
> > decided to temporarily create fake addresses by hashing on Mac (eventually
we
> > will move all platforms over to device identifiers instead).  For testing,
> then
> > we need 128-bit identifiers that we know the hash of, and it would be nice
if
> > both the id and the hash were man-made looking (and not completely random). 
> > Vincent wanted me to check in the code I used to generate the hashes, in
case
> he
> > needs it after I finish this internship.  (and yes, he would have to tell it
> to
> > look at a different range because of the determinism)
> 
> Why do they need to be man-made looking? Hashes with 00s in the front are
still
> possible, so you probably don't want to be doing anything programmatic with
> them.

Maybe man-made is the wrong word. We want to use addresses of the form
010000XXXXXX, 020000XXXXXX, 030000XXXXXX, ... (or similar) in our tests to
indicate to the reader these are testing values (and not magic).

> > The function is designed to be used as necessary to generate new ID/Hash
> pairs.
> > Developers would modify as needed when generating new pairs.
> 
> Without knowing that, it seems someone reading the code would be very confused
> what that function is for. It's totally deterministic, does quite a bit of
> computation, and never called. Probably at least wants a comment that it's
just
> some sample code. (If it's just sample code, maybe just put it in a comment
> somewhere?)

Added a comment and commented out the function.

[krstnmnlsn, 2015-07-31 19:23:09]

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

[commit-bot: I haz the power, 2015-07-31 19:23:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246913006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246913006/160001

[commit-bot: I haz the power, 2015-07-31 20:00:35]

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

[commit-bot: I haz the power, 2015-07-31 20:00:35]

Dry run: This issue passed the CQ dry run.

[krstnmnlsn, 2015-07-31 20:09:22]

The CQ bit was checked by krstnmnlsn@chromium.org

[krstnmnlsn, 2015-07-31 20:09:22]

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/1246913006/#ps160001
(title: "removing dependency on crypto")

[commit-bot: I haz the power, 2015-07-31 20:09:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246913006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246913006/160001

[commit-bot: I haz the power, 2015-07-31 20:15:21]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-07-31 20:15:48]

Patchset 6 (id:??) landed as
https://crrev.com/5a0eceb28201302bcb45621015119bd73351d659
Cr-Commit-Position: refs/heads/master@{#341400}

[dmurph, 2015-07-31 21:05:46]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/1265023004/ by dmurph@chromium.org.

The reason for reverting is:
https://code.google.com/p/chromium/issues/detail?id=516032

Linking failure on mac debug bot..

[Avi (use Gerrit), 2015-07-31 21:32:35]

On 2015/07/31 21:05:46, dmurph wrote:
> A revert of this CL (patchset #6 id:160001) has been created in
> https://codereview.chromium.org/1265023004/ by mailto:dmurph@chromium.org.
> 
> The reason for reverting is:
> https://code.google.com/p/chromium/issues/detail?id=516032
> 
> Linking failure on mac debug bot..

Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$_CBUUID", referenced from:
      objc-class-ref in device_unittests.bluetooth_test_mac.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

CBUUID isn't available everywhere; you'll need to dynamically look it up.

[krstnmnlsn, 2015-07-31 21:54:45]

The CQ bit was checked by krstnmnlsn@chromium.org

[krstnmnlsn, 2015-07-31 21:54: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/1246913006/#ps180001
(title: "CBUUID dyanamically used now")

[commit-bot: I haz the power, 2015-07-31 21:55:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246913006/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246913006/180001

[commit-bot: I haz the power, 2015-07-31 23:11:23]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-07-31 23:12:01]

Patchset 7 (id:??) landed as
https://crrev.com/41c13ac0fcc9dbd8077a109df59b12eb6a8e27f8
Cr-Commit-Position: refs/heads/master@{#341433}