bluetooth: Refactor how we update based on Advertising Data

bluetooth: Refactor how we update based on Advertising Data

Changes:

 1. GetUUIDs now returns a set. Most clients check if the device contains an
    specific UUID so it makes more sense to return a set rather than force
    clients to iterate over all services.
 2. Service Data is now stored as a map from UUIDs to std::vector<uint8_t>
 3. Change how we expose Advertisement Data:

    The old model: Advertisement Data was kept indefinitely meaning it would never
    expire. This could lead to exposing stale data to clients of the API.
    The new model: Every time a packet arrives we discard the old advertising data
    and replace it with the received Advertisement Data. Once discovery finishes
    we clear the data. This matches what BlueZ does and gives clients the most
    up to date information regarding the Advertisement Data.

BUG=568135
Committed: https://crrev.com/7a8399b1e00b3cd796396021c99472ef43d5f7a7
Cr-Commit-Position: refs/heads/master@{#413780}

[ortuno, 2016-08-12 22:10:48]

Description was changed from

==========
bluetooth: Refactor how we update based on Advertising Data

BUG=568135
==========

to

==========
bluetooth: Refactor how we update based on Advertising Data

Changes:

 1. GetUUIDs now returns a set. Most clients check if the device contains an
    specific UUID so it makes more sense to return a set rather than force
    clients to iterate over all services.
 2. Service Data is now stored as a map from UUIDs to std::vector<uint8_t>
 3. Change how we expose Advertisement Data:

    The old model: Advertisement Data was kept indefinitely meaning it would
never
    expire. This could lead to exposing stale data to clients of the API.
    The new model: Every time a packet arrives we discard the old advertising
data
    and replace it with the received Advertisement Data. Once discovery finishes
    we clear the data. This matches what BlueZ does and gives clients the most
    up to date information regarding the Advertisement Data.

BUG=568135
==========

[ortuno, 2016-08-12 23:28:39]

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

[commit-bot: I haz the power, 2016-08-12 23:29:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 23:47:54]

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

[commit-bot: I haz the power, 2016-08-12 23:47:55]

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

[ortuno, 2016-08-13 00:03:51]

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

[ortuno, 2016-08-13 00:04:11]

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

[ortuno, 2016-08-13 00:04:12]

scheib: PTAL

[commit-bot: I haz the power, 2016-08-13 00:04:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 00:15:59]

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

[commit-bot: I haz the power, 2016-08-13 00:16:00]

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

[ortuno, 2016-08-15 17:55:59]

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

[commit-bot: I haz the power, 2016-08-15 17:56:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ortuno, 2016-08-15 17:56:48]

ortuno@chromium.org changed reviewers:
+ jyasskin@chromium.org

[ortuno, 2016-08-15 17:56:49]

jyasskin: Could you take a look since scheib is OOO?

[commit-bot: I haz the power, 2016-08-15 18:08:20]

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

[commit-bot: I haz the power, 2016-08-15 18:08:21]

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

[ortuno, 2016-08-15 19:05:25]

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

[commit-bot: I haz the power, 2016-08-15 19:06:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 19:35:02]

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

[commit-bot: I haz the power, 2016-08-15 19:35:03]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ortuno, 2016-08-16 16:11:11]

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

[commit-bot: I haz the power, 2016-08-16 16:11:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 16:14:05]

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

[commit-bot: I haz the power, 2016-08-16 16:14:06]

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

[ortuno, 2016-08-16 16:18:31]

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

[commit-bot: I haz the power, 2016-08-16 16:18:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 16:29:37]

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

[commit-bot: I haz the power, 2016-08-16 16:29:38]

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

[ortuno, 2016-08-16 18:55:14]

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

[commit-bot: I haz the power, 2016-08-16 18:55:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 19:37:54]

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

[commit-bot: I haz the power, 2016-08-16 19:37:54]

Dry run: Exceeded global retry quota

[ortuno, 2016-08-16 20:37:49]

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

[commit-bot: I haz the power, 2016-08-16 20:38:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 22:42:47]

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

[commit-bot: I haz the power, 2016-08-16 22:42:48]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2016-08-18 16:04:33]

More later.

https://codereview.chromium.org/2244693002/diff/250001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/bluetooth/device_info/runtest.js
(right):

https://codereview.chromium.org/2244693002/diff/250001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/bluetooth/device_info/runtest.js:18: let
uuids = new Set(devices[0].uuids);
We should alert the extensions/apps team that the order of UUIDs is changing,
which might break some apps. (Even though the behavior is allowed by
https://developer.chrome.com/apps/bluetooth#property-Device-uuids)

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1415: uuid_results[i] =
mojom::BluetoothUUID::From(uuid);
Too bad they don't have tests. You need to i++ in this loop.

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1546: BluetoothDevice::UUIDSet
uuid_set = device->GetUUIDs();
This indentation's wrong.

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1553: advertised_uuids[i] =
mojom::BluetoothUUID::From(uuid);
Another missing i++.

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1560: for (auto& uuid :
device->GetServiceDataUUIDs()) {
Not from this change, but can this be a 'const auto&', or even a 'const
BluetoothUUID&'?

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1573: for (const uint8_t& byte
: *data) {
I think you can use "service_data->data.assign(data->begin(), data->end())" here
instead of the loop.

https://codereview.chromium.org/2244693002/diff/250001/components/proximity_a...
File
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/2244693002/diff/250001/components/proximity_a...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:169:
BluetoothDevice::UUIDSet uuids;
I believe you can use

  BluetoothDevice::UUIDSet uuids = {device::BluetoothUUID(uuid)};

now:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/andro...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java
(right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:245:
String[] uuid_strings;
Was this just an improvement you happened to notice while writing the change, or
was it needed by something I'm not seeing?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:193: BluetoothDeviceAndroid*
device_android;
To track ownership more clearly, I might write this as:

std::unique_ptr<BluetoothDeviceAndroid> device_android_owner;
BluetoothDeviceAndroid* device_android;

if (iter == devices_.end()) {
  ...
  device_android_owner.reset(BluetoothDeviceAndroid::Create(...));
  device_android = device_android_owner.get();
} ...

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:206:
AppendJavaStringArrayToStringVector(env, advertised_uuids,
You're passing a List<ParcelUuid> to a function that says it takes a
StringArray? Which of those is wrong?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:209:
device_android->UpdateAdvertisementData(std::move(advertised_uuids_strings),
I'm skeptical of having a single function that updates all the advertising data:
I think it's likely to have to grow too many parameters to be clear. Would it
also work to explicitly clear the data in this caller, and then update each
field in a separate call? Or to have an AdvertisementData object, which you
build and then move into the UpdateAdvertisementData() call?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:539: std::piecewise_construct,
I wouldn't bother with the complexity of piecewise_construct here. Strings and
vectors are cheap to move, so you don't need to avoid constructing them outside
of the constructor. So, .emplace([[uuid UUIDString] UTF8String],
std::vector<uint8_t>(bytes, bytes + length)) should be fine.

[Jeffrey Yasskin, 2016-08-18 19:56:31]

Another partial review:

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:490: bool is_new_device = device_mac
== nullptr;
Consider making this const, so it's really clear it doesn't change over the
course of the function.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:531: std::unordered_map<std::string,
std::vector<uint8_t>> service_data_map;
Would it make sense to use a BluetoothUUID as the key of this map?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_unittest.cc (left):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:637:
EXPECT_TRUE(base::ContainsValue(device->GetUUIDs(),
Why don't we need to check which UUIDs are listed anymore?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:57: BluetoothDevice::UUIDSet
BluetoothDevice::DeviceUUIDs::GetUUIDs() const {
Since you're returning an object that keeps living after the call, consider
returning a const BluetoothDevice::UUIDSet&.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:121: typedef
std::unordered_set<BluetoothUUID, BluetoothUUIDHash> UUIDSet;
This is where I'd really like to have a vector-backed set, which could be much
cheaper to create and use. However, Bluetooth probably isn't a big enough
fraction of browser time to make it worth adding that type just for us.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:129: typedef
base::ScopedPtrHashMap<std::string,
Use unordered_map instead:
https://cs.chromium.org/chromium/src/base/containers/scoped_ptr_hash_map.h?l=21

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:320: // is not discoverying.
sp: discoverying->discovering

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:324: // Returns an empty set if the adapter
is not discoverying.
I think I'd rather return something consistent between GetServiceData() and
GetServiceDataUUIDs() if the adapter isn't discovering. Empty seems fine for
both.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:542: // Helper class to easily update the
sets of UUIDs and them in sycn with the
sp: sycn

[Jeffrey Yasskin, 2016-08-19 15:09:53]

Ok, that's all my comments.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:289: BluetoothDevice::UUIDSet
BluetoothDevice::GetUUIDs() const {
If device_uuids_.GetUUIDs() starts returning a reference, this should too.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:294: return &service_data_;
The interface says this'll be null when not scanning, but it's actually empty.
That seems fine, but then you can return a reference.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:392:
service_data_.insert({BluetoothUUID(it.first), std::move(it.second)});
Maybe this should just take an unordered_map of the right type, and move-assign
it?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_unittest.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:128: TEST_F(BluetoothTest,
AdvertisementData_Discovery) {
Nice tests!

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:150: #if defined(OS_MACOSX)
Comment why this only works for Mac?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:173:
!!device->GetServiceDataForUUID(BluetoothUUID(kTestUUIDHeartRate)));
Use ==nullptr instead of !! to check for null.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_win.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:240: UUIDSet known_services = uuids_;
Try not to make a copy here, if uuids_ doesn't change before you use
known_services.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_bluez_unittest.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_bluez_unittest.cc:2694: ASSERT_EQ(1U,
uuids.size());
You can change these to EXPECT_EQ now that the inspection won't crash if the
set's too small.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.cc:361: uuids.insert(uuid);
Try to std::move() BluetoothUUIDs around, since they contain a string. It
doesn't help yet, but we can give BluetoothUUID a move constructor later.

[ortuno, 2016-08-19 20:50:31]

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

[ortuno, 2016-08-19 20:50:34]

https://codereview.chromium.org/2244693002/diff/250001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/bluetooth/device_info/runtest.js
(right):

https://codereview.chromium.org/2244693002/diff/250001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/bluetooth/device_info/runtest.js:18: let
uuids = new Set(devices[0].uuids);
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> We should alert the extensions/apps team that the order of UUIDs is changing,
which might break some apps. (Even though the behavior is allowed by
https://developer.chrome.com/apps/bluetooth#property-Device-uuids)

I'll add rkc as a reviewer of extensions code.

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1415: uuid_results[i] =
mojom::BluetoothUUID::From(uuid);
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> Too bad they don't have tests. You need to i++ in this loop.

Done.

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1546: BluetoothDevice::UUIDSet
uuid_set = device->GetUUIDs();
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> This indentation's wrong.

Done.

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1553: advertised_uuids[i] =
mojom::BluetoothUUID::From(uuid);
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> Another missing i++.

Done.

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1560: for (auto& uuid :
device->GetServiceDataUUIDs()) {
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> Not from this change, but can this be a 'const auto&', or even a 'const
BluetoothUUID&'?

Done.

https://codereview.chromium.org/2244693002/diff/250001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1573: for (const uint8_t& byte
: *data) {
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> I think you can use "service_data->data.assign(data->begin(), data->end())"
here instead of the loop.

Close. service_data->data is a mojo::Array and that doesn't have an assign
method. But I copied the data to another vector and used mojo::Array::Swap.

https://codereview.chromium.org/2244693002/diff/250001/components/proximity_a...
File
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/2244693002/diff/250001/components/proximity_a...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:169:
BluetoothDevice::UUIDSet uuids;
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> I believe you can use
> 
>   BluetoothDevice::UUIDSet uuids = {device::BluetoothUUID(uuid)};
> 
> now:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/andro...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java
(right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:245:
String[] uuid_strings;
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> Was this just an improvement you happened to notice while writing the change,
or was it needed by something I'm not seeing?

Before we were putting UUIDs in a set since the c++ side just put the strings in
an array. Now that we put it in an array we don't need to put the UUIDs in a set
here.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:193: BluetoothDeviceAndroid*
device_android;
On 2016/08/18 at 16:04:33, Jeffrey Yasskin wrote:
> To track ownership more clearly, I might write this as:
> 
> std::unique_ptr<BluetoothDeviceAndroid> device_android_owner;
> BluetoothDeviceAndroid* device_android;
> 
> if (iter == devices_.end()) {
>   ...
>   device_android_owner.reset(BluetoothDeviceAndroid::Create(...));
>   device_android = device_android_owner.get();
> } ...

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:206:
AppendJavaStringArrayToStringVector(env, advertised_uuids,
On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> You're passing a List<ParcelUuid> to a function that says it takes a
StringArray? Which of those is wrong?

Changed the comment to String[]. Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:209:
device_android->UpdateAdvertisementData(std::move(advertised_uuids_strings),
On 2016/08/18 at 16:04:33, Jeffrey Yasskin wrote:
> I'm skeptical of having a single function that updates all the advertising
data: I think it's likely to have to grow too many parameters to be clear. Would
it also work to explicitly clear the data in this caller, and then update each
field in a separate call? Or to have an AdvertisementData object, which you
build and then move into the UpdateAdvertisementData() call?

I like having a compile time check to make sure all the advertisement data
fields have been parsed.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:490: bool is_new_device = device_mac
== nullptr;
On 2016/08/18 at 19:56:30, Jeffrey Yasskin wrote:
> Consider making this const, so it's really clear it doesn't change over the
course of the function.

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:531: std::unordered_map<std::string,
std::vector<uint8_t>> service_data_map;
On 2016/08/18 at 19:56:30, Jeffrey Yasskin wrote:
> Would it make sense to use a BluetoothUUID as the key of this map?

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:539: std::piecewise_construct,
On 2016/08/18 at 16:04:33, Jeffrey Yasskin wrote:
> I wouldn't bother with the complexity of piecewise_construct here. Strings and
vectors are cheap to move, so you don't need to avoid constructing them outside
of the constructor. So, .emplace([[uuid UUIDString] UTF8String],
std::vector<uint8_t>(bytes, bytes + length)) should be fine.

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_unittest.cc (left):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:637:
EXPECT_TRUE(base::ContainsValue(device->GetUUIDs(),
On 2016/08/18 at 19:56:30, Jeffrey Yasskin wrote:
> Why don't we need to check which UUIDs are listed anymore?

The purpose of this test is just to make sure that no new devices were created
for devices we've seen in the past. The GetUUIDs* tests on
bluetooth_device_unittest.cc test that the UUIDs are updated correctly.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:57: BluetoothDevice::UUIDSet
BluetoothDevice::DeviceUUIDs::GetUUIDs() const {
On 2016/08/18 at 19:56:30, Jeffrey Yasskin wrote:
> Since you're returning an object that keeps living after the call, consider
returning a const BluetoothDevice::UUIDSet&.

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:289: BluetoothDevice::UUIDSet
BluetoothDevice::GetUUIDs() const {
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> If device_uuids_.GetUUIDs() starts returning a reference, this should too.

Both BluetoothClassicDeviceMac and BluetoothDeviceBluez don't hold the set of
uuids themselves but rather just construct them when GetUUIDs is called. We
could refactor BluetoothDeviceBlueZ to hold to the uuids and update them when
they changed but there is no indication that the UUIDs changed for classic mac
devices :( So we either keep returning a copy or we remove the const qualifier.
Which one do you think it's better?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:294: return &service_data_;
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> The interface says this'll be null when not scanning, but it's actually empty.
That seems fine, but then you can return a reference.

The interface says this will be empty :) GetServiceDataForUUID will be nullptr.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:392:
service_data_.insert({BluetoothUUID(it.first), std::move(it.second)});
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> Maybe this should just take an unordered_map of the right type, and
move-assign it?

Hmm I wanted to have UpdateAdvertisementData take only strings so that each
function that parsed the data didn't have to create the BluetoothUUIDs
themselves. But I agree that moving the map of BluetoothUUIDs would be better,
so I changed the function to take BluetoothUUIDs rather than strings. That said
I'm sad that I have to convert all the strings to BluetoothUUIDs in each
platform :(

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:121: typedef
std::unordered_set<BluetoothUUID, BluetoothUUIDHash> UUIDSet;
On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> This is where I'd really like to have a vector-backed set, which could be much
cheaper to create and use. However, Bluetooth probably isn't a big enough
fraction of browser time to make it worth adding that type just for us.

Is that so that we would always return the uuids in the order they were added?
Wouldn't std::set work for that?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:129: typedef
base::ScopedPtrHashMap<std::string,
On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> Use unordered_map instead:
https://cs.chromium.org/chromium/src/base/containers/scoped_ptr_hash_map.h?l=21

I had to fight the urge to convert this to an unordered_map, but I'm just moving
this from below because DeviceUUIDs::ReplaceServiceUUIDs takes a GattServiceMap.
I opened an issue to convert all of these to std::unordered_map:
http://crbug.com/639089

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:320: // is not discoverying.
On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> sp: discoverying->discovering

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:324: // Returns an empty set if the adapter
is not discoverying.
On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> I think I'd rather return something consistent between GetServiceData() and
GetServiceDataUUIDs() if the adapter isn't discovering. Empty seems fine for
both.

An empty array means that the service just has no data but it's still in the
service data field e.g. [0x3 /* length */, 0x16 /* service data */, 0xAA, 0xFE
/* service uuid */]. Whereas a nullptr means the service uuid is not in the
service data field or we are not advertising.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:542: // Helper class to easily update the
sets of UUIDs and them in sycn with the
On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> sp: sycn

Done. Heh, I was also missing "keep".

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_unittest.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:128: TEST_F(BluetoothTest,
AdvertisementData_Discovery) {
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> Nice tests!

Thanks :)

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:150: #if defined(OS_MACOSX)
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> Comment why this only works for Mac?

Done here and in all instances below.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:173:
!!device->GetServiceDataForUUID(BluetoothUUID(kTestUUIDHeartRate)));
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> Use ==nullptr instead of !! to check for null.

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_win.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_win.cc:240: UUIDSet known_services = uuids_;
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> Try not to make a copy here, if uuids_ doesn't change before you use
known_services.

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_bluez_unittest.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_bluez_unittest.cc:2694: ASSERT_EQ(1U,
uuids.size());
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> You can change these to EXPECT_EQ now that the inspection won't crash if the
set's too small.

Done.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.cc:361: uuids.insert(uuid);
On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> Try to std::move() BluetoothUUIDs around, since they contain a string. It
doesn't help yet, but we can give BluetoothUUID a move constructor later.

Done.

[Jeffrey Yasskin, 2016-08-19 22:08:10]

LGTM after you check the nits and see if making GetServiceData() return a
reference makes sense. Thanks!

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/andro...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java
(right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:245:
String[] uuid_strings;
On 2016/08/19 20:50:32, ortuno wrote:
> On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> > Was this just an improvement you happened to notice while writing the
change,
> or was it needed by something I'm not seeing?
> 
> Before we were putting UUIDs in a set since the c++ side just put the strings
in
> an array. Now that we put it in an array we don't need to put the UUIDs in a
set
> here.

... because there might be duplicates in the UUIDs reported here?

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:209:
device_android->UpdateAdvertisementData(std::move(advertised_uuids_strings),
On 2016/08/19 20:50:33, ortuno wrote:
> On 2016/08/18 at 16:04:33, Jeffrey Yasskin wrote:
> > I'm skeptical of having a single function that updates all the advertising
> data: I think it's likely to have to grow too many parameters to be clear.
Would
> it also work to explicitly clear the data in this caller, and then update each
> field in a separate call? Or to have an AdvertisementData object, which you
> build and then move into the UpdateAdvertisementData() call?
> 
> I like having a compile time check to make sure all the advertisement data
> fields have been parsed.

'k.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:289: BluetoothDevice::UUIDSet
BluetoothDevice::GetUUIDs() const {
On 2016/08/19 20:50:33, ortuno wrote:
> On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> > If device_uuids_.GetUUIDs() starts returning a reference, this should too.
> 
> Both BluetoothClassicDeviceMac and BluetoothDeviceBluez don't hold the set of
> uuids themselves but rather just construct them when GetUUIDs is called. We
> could refactor BluetoothDeviceBlueZ to hold to the uuids and update them when
> they changed but there is no indication that the UUIDs changed for classic mac
> devices :( So we either keep returning a copy or we remove the const
qualifier.
> Which one do you think it's better?

Returning a copy sounds fine, if that's what Mac and Bluez will need to do
anyway.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:294: return &service_data_;
On 2016/08/19 20:50:33, ortuno wrote:
> On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> > The interface says this'll be null when not scanning, but it's actually
empty.
> That seems fine, but then you can return a reference.
> 
> The interface says this will be empty :) GetServiceDataForUUID will be
nullptr.

From
https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluet...

// Returns the last advertised Service Data. Returns nullptr if the adapter
// is not discovering.
const ServiceDataMap* GetServiceData() const;

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:392:
service_data_.insert({BluetoothUUID(it.first), std::move(it.second)});
On 2016/08/19 20:50:33, ortuno wrote:
> On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> > Maybe this should just take an unordered_map of the right type, and
> move-assign it?
> 
> Hmm I wanted to have UpdateAdvertisementData take only strings so that each
> function that parsed the data didn't have to create the BluetoothUUIDs
> themselves. But I agree that moving the map of BluetoothUUIDs would be better,
> so I changed the function to take BluetoothUUIDs rather than strings. That
said
> I'm sad that I have to convert all the strings to BluetoothUUIDs in each
> platform :(

I think I'm relatively happy about that: it does force you to duplicate a bit of
code, but it establishes type safety earlier. One purpose of the platform layer
is to convert from untyped OS interfaces to typed Chrome interfaces, so it seems
like a reasonable place to do this conversion.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:121: typedef
std::unordered_set<BluetoothUUID, BluetoothUUIDHash> UUIDSet;
On 2016/08/19 20:50:33, ortuno wrote:
> On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> > This is where I'd really like to have a vector-backed set, which could be
much
> cheaper to create and use. However, Bluetooth probably isn't a big enough
> fraction of browser time to make it worth adding that type just for us.
> 
> Is that so that we would always return the uuids in the order they were added?
> Wouldn't std::set work for that?

No, I'm thinking of the implementation guts of an unordered_set: it needs an
allocated node for each entry, and for small sets like the ones in advertising
data, a linear search can be faster and more memory efficient than a hash
lookup. We do have
https://cs.chromium.org/chromium/src/base/containers/small_map.h, but this is
one of the cases where I wouldn't want to spend the extra code size.

It's possible that using Blink's HashMap (which doesn't use separate nodes)
would beat the linear search. But anyway, it's not worth adding something to
Chrome for this purpose.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:129: typedef
base::ScopedPtrHashMap<std::string,
On 2016/08/19 20:50:33, ortuno wrote:
> On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> > Use unordered_map instead:
>
https://cs.chromium.org/chromium/src/base/containers/scoped_ptr_hash_map.h?l=21
> 
> I had to fight the urge to convert this to an unordered_map, but I'm just
moving
> this from below because DeviceUUIDs::ReplaceServiceUUIDs takes a
GattServiceMap.
> I opened an issue to convert all of these to std::unordered_map:
> http://crbug.com/639089

Oh, ok.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:324: // Returns an empty set if the adapter
is not discoverying.
On 2016/08/19 20:50:33, ortuno wrote:
> On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> > I think I'd rather return something consistent between GetServiceData() and
> GetServiceDataUUIDs() if the adapter isn't discovering. Empty seems fine for
> both.
> 
> An empty array means that the service just has no data but it's still in the
> service data field e.g. [0x3 /* length */, 0x16 /* service data */, 0xAA, 0xFE
> /* service uuid */]. Whereas a nullptr means the service uuid is not in the
> service data field or we are not advertising.

I think that's for GetServiceDataForUUID()? I was talking about
GetServiceData(), which doesn't identify a particular service.

https://codereview.chromium.org/2244693002/diff/290001/components/arc/bluetoo...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2244693002/diff/290001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1661: std::vector<uint8_t>
data_copy(data->begin(), data->end());
This can just be std::vector<uint8_t> data_copy = *data;

https://codereview.chromium.org/2244693002/diff/290001/components/proximity_a...
File
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/2244693002/diff/290001/components/proximity_a...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:349:
uuids.insert(device::BluetoothUUID(kServiceUUID));
{} here too.

https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:321: const ServiceDataMap* GetServiceData()
const;
That is, can GetServiceData() return a const ServiceDataMap&?

https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.h:223: BluetoothDevice::UUIDSet
uuids_;
I think you don't use this in the .cc file.

[ortuno, 2016-08-22 22:02:52]

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

[commit-bot: I haz the power, 2016-08-22 22:04:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 22:07:34]

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

[commit-bot: I haz the power, 2016-08-22 22:07:35]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ortuno, 2016-08-22 22:55:21]

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

[commit-bot: I haz the power, 2016-08-22 22:55:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ortuno, 2016-08-23 00:38:04]

Thanks!

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/andro...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java
(right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:245:
String[] uuid_strings;
On 2016/08/19 at 22:08:09, Jeffrey Yasskin wrote:
> On 2016/08/19 20:50:32, ortuno wrote:
> > On 2016/08/18 at 16:04:32, Jeffrey Yasskin wrote:
> > > Was this just an improvement you happened to notice while writing the
change,
> > or was it needed by something I'm not seeing?
> > 
> > Before we were putting UUIDs in a set since the c++ side just put the
strings in
> > an array. Now that we put it in an array we don't need to put the UUIDs in a
set
> > here.
> 
> ... because there might be duplicates in the UUIDs reported here?

Oh, mainly because users of GetUUIDs used it to check if the device contained a
specific UUID.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:392:
service_data_.insert({BluetoothUUID(it.first), std::move(it.second)});
On 2016/08/19 at 22:08:09, Jeffrey Yasskin wrote:
> On 2016/08/19 20:50:33, ortuno wrote:
> > On 2016/08/19 at 15:09:53, Jeffrey Yasskin wrote:
> > > Maybe this should just take an unordered_map of the right type, and
> > move-assign it?
> > 
> > Hmm I wanted to have UpdateAdvertisementData take only strings so that each
> > function that parsed the data didn't have to create the BluetoothUUIDs
> > themselves. But I agree that moving the map of BluetoothUUIDs would be
better,
> > so I changed the function to take BluetoothUUIDs rather than strings. That
said
> > I'm sad that I have to convert all the strings to BluetoothUUIDs in each
> > platform :(
> 
> I think I'm relatively happy about that: it does force you to duplicate a bit
of code, but it establishes type safety earlier. One purpose of the platform
layer is to convert from untyped OS interfaces to typed Chrome interfaces, so it
seems like a reasonable place to do this conversion.

That makes sense.

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/2244693002/diff/250001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:324: // Returns an empty set if the adapter
is not discoverying.
On 2016/08/19 at 22:08:09, Jeffrey Yasskin wrote:
> On 2016/08/19 20:50:33, ortuno wrote:
> > On 2016/08/18 at 19:56:31, Jeffrey Yasskin wrote:
> > > I think I'd rather return something consistent between GetServiceData()
and
> > GetServiceDataUUIDs() if the adapter isn't discovering. Empty seems fine for
> > both.
> > 
> > An empty array means that the service just has no data but it's still in the
> > service data field e.g. [0x3 /* length */, 0x16 /* service data */, 0xAA,
0xFE
> > /* service uuid */]. Whereas a nullptr means the service uuid is not in the
> > service data field or we are not advertising.
> 
> I think that's for GetServiceDataForUUID()? I was talking about
GetServiceData(), which doesn't identify a particular service.

Ah right. Changed GetServiceData to return an empty map.

https://codereview.chromium.org/2244693002/diff/290001/components/arc/bluetoo...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2244693002/diff/290001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1661: std::vector<uint8_t>
data_copy(data->begin(), data->end());
On 2016/08/19 at 22:08:10, Jeffrey Yasskin wrote:
> This can just be std::vector<uint8_t> data_copy = *data;

Done.

https://codereview.chromium.org/2244693002/diff/290001/components/proximity_a...
File
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/2244693002/diff/290001/components/proximity_a...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:349:
uuids.insert(device::BluetoothUUID(kServiceUUID));
On 2016/08/19 at 22:08:10, Jeffrey Yasskin wrote:
> {} here too.

Done.

https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:321: const ServiceDataMap* GetServiceData()
const;
On 2016/08/19 at 22:08:10, Jeffrey Yasskin wrote:
> That is, can GetServiceData() return a const ServiceDataMap&?

Done.

https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/2244693002/diff/290001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.h:223: BluetoothDevice::UUIDSet
uuids_;
On 2016/08/19 at 22:08:10, Jeffrey Yasskin wrote:
> I think you don't use this in the .cc file.

Ah right. This was leftover for when I was trying to return a reference.

[ortuno, 2016-08-23 00:43:39]

ortuno@chromium.org changed reviewers:
+ puthik@chromium.org, rkc@chromium.org, sacomoto@chromium.org,
stevenjb@chromium.org

[ortuno, 2016-08-23 00:43:40]

puthik: PTAL at components/arc
sacomoto: PTAL at components/proximity_auth
stevenjb: PTAL at extensions
rkc: FYI this changes the behavior of the BLE API in that GetUUIDs will no
longer return the UUIDs in the same order. The API docs doesn't give any
guarantees with respect to the order.

[puthik_chromium, 2016-08-23 01:08:52]

components/arc LGTM
Didn't look at other code

[ortuno, 2016-08-23 01:10:58]

ortuno@chromium.org changed reviewers:
+ lhchavez@chromium.org

[ortuno, 2016-08-23 01:11:00]

lhchavez: PTAL at components/arc

[commit-bot: I haz the power, 2016-08-23 01:21:19]

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

[commit-bot: I haz the power, 2016-08-23 01:21:21]

Dry run: This issue passed the CQ dry run.

[sacomoto, 2016-08-23 09:58:27]

components/proximity_auth LGTM.

[stevenjb, 2016-08-23 15:36:41]

extensions/ lgtm

[Luis Héctor Chávez, 2016-08-23 15:49:27]

lgtm with a nit

https://codereview.chromium.org/2244693002/diff/350001/components/arc/bluetoo...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2244693002/diff/350001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1480: for (const auto& uuid :
uuids) {
nit: can this be

for (size_t i = 0; i < uuids.size(); i++)
  uuid_results[i] = mojom::BluetoothUUID::From(uuids[i]);

? Same in L1640.

[ortuno, 2016-08-23 17:40:19]

https://codereview.chromium.org/2244693002/diff/350001/components/arc/bluetoo...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2244693002/diff/350001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1480: for (const auto& uuid :
uuids) {
On 2016/08/23 at 15:49:27, Luis Héctor Chávez wrote:
> nit: can this be
> 
> for (size_t i = 0; i < uuids.size(); i++)
>   uuid_results[i] = mojom::BluetoothUUID::From(uuids[i]);
> 
> ? Same in L1640.

We can't do that because uuids is not a vector, it's a set.

[Luis Héctor Chávez, 2016-08-23 17:44:28]

https://codereview.chromium.org/2244693002/diff/350001/components/arc/bluetoo...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2244693002/diff/350001/components/arc/bluetoo...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1480: for (const auto& uuid :
uuids) {
On 2016/08/23 17:40:19, ortuno wrote:
> On 2016/08/23 at 15:49:27, Luis Héctor Chávez wrote:
> > nit: can this be
> > 
> > for (size_t i = 0; i < uuids.size(); i++)
> >   uuid_results[i] = mojom::BluetoothUUID::From(uuids[i]);
> > 
> > ? Same in L1640.
> 
> We can't do that because uuids is not a vector, it's a set.

Fine then :(

[ortuno, 2016-08-23 17:45:28]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-08-23 17:45:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/2244693002/#ps350001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-23 17:46:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 17:51:10]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2016-08-23 17:52:49]

Description was changed from

==========
bluetooth: Refactor how we update based on Advertising Data

Changes:

 1. GetUUIDs now returns a set. Most clients check if the device contains an
    specific UUID so it makes more sense to return a set rather than force
    clients to iterate over all services.
 2. Service Data is now stored as a map from UUIDs to std::vector<uint8_t>
 3. Change how we expose Advertisement Data:

    The old model: Advertisement Data was kept indefinitely meaning it would
never
    expire. This could lead to exposing stale data to clients of the API.
    The new model: Every time a packet arrives we discard the old advertising
data
    and replace it with the received Advertisement Data. Once discovery finishes
    we clear the data. This matches what BlueZ does and gives clients the most
    up to date information regarding the Advertisement Data.

BUG=568135
==========

to

==========
bluetooth: Refactor how we update based on Advertising Data

Changes:

 1. GetUUIDs now returns a set. Most clients check if the device contains an
    specific UUID so it makes more sense to return a set rather than force
    clients to iterate over all services.
 2. Service Data is now stored as a map from UUIDs to std::vector<uint8_t>
 3. Change how we expose Advertisement Data:

    The old model: Advertisement Data was kept indefinitely meaning it would
never
    expire. This could lead to exposing stale data to clients of the API.
    The new model: Every time a packet arrives we discard the old advertising
data
    and replace it with the received Advertisement Data. Once discovery finishes
    we clear the data. This matches what BlueZ does and gives clients the most
    up to date information regarding the Advertisement Data.

BUG=568135

Committed: https://crrev.com/7a8399b1e00b3cd796396021c99472ef43d5f7a7
Cr-Commit-Position: refs/heads/master@{#413780}
==========

[commit-bot: I haz the power, 2016-08-23 17:52:51]

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