bluetooth: Refactor how we update based on Advertising Data

device/bluetooth/bluetooth_adapter_android.cc

Index: device/bluetooth/bluetooth_adapter_android.cc
diff --git a/device/bluetooth/bluetooth_adapter_android.cc b/device/bluetooth/bluetooth_adapter_android.cc
index ec9d0f88f0de201263362f88c46a6fb0356983b8..397257303e21b03b254ffc3c85b10a13d1f6e52f 100644
--- a/device/bluetooth/bluetooth_adapter_android.cc
+++ b/device/bluetooth/bluetooth_adapter_android.cc
@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/android/jni_android.h"
+#include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
 #include "base/location.h"
@@ -21,6 +22,7 @@
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF8;
+using base::android::AppendJavaStringArrayToStringVector;
 using base::android::JavaParamRef;
 
 namespace {
@@ -187,23 +189,32 @@ void BluetoothAdapterAndroid::CreateOrUpdateDeviceOnScan(
   std::string device_address = ConvertJavaStringToUTF8(env, address);
   DevicesMap::const_iterator iter = devices_.find(device_address);
 
+  bool is_new_device = false;
+  BluetoothDeviceAndroid* device_android;

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

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();
} ...

[ortuno, 2016/08/19 20:50:33]

Done.

+
   if (iter == devices_.end()) {
     // New device.
-    BluetoothDeviceAndroid* device_android =
+    is_new_device = true;
+    device_android =
         BluetoothDeviceAndroid::Create(this, bluetooth_device_wrapper);
-    device_android->UpdateAdvertisedUUIDs(advertised_uuids);
-    device_android->UpdateTimestamp();
+  } else {
+    // Existing device.
+    device_android = static_cast<BluetoothDeviceAndroid*>(iter->second);
+  }
+  DCHECK(device_android);
+  std::vector<std::string> advertised_uuids_strings;
+  AppendJavaStringArrayToStringVector(env, advertised_uuids,

[Jeffrey Yasskin, 2016/08/18 16:04:32]

You're passing a List<ParcelUuid> to a function that says it takes a
StringArray? Which of those is wrong?

[ortuno, 2016/08/19 20:50:33]

Changed the comment to String[]. Done.

+                                      &advertised_uuids_strings);
+
+  device_android->UpdateAdvertisementData(std::move(advertised_uuids_strings),

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

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?

[ortuno, 2016/08/19 20:50:33]

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

[Jeffrey Yasskin, 2016/08/19 22:08:09]

'k.

+                                          {} /* service_data */);
+
+  if (is_new_device) {
     devices_.add(device_address,
                  std::unique_ptr<BluetoothDevice>(device_android));
     FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
                       DeviceAdded(this, device_android));
   } else {
-    // Existing device.
-    BluetoothDeviceAndroid* device_android =
-        static_cast<BluetoothDeviceAndroid*>(iter->second);
-    device_android->UpdateTimestamp();
-    device_android->UpdateAdvertisedUUIDs(advertised_uuids);
-
     FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
                       DeviceChanged(this, device_android));
   }
@@ -282,6 +293,9 @@ void BluetoothAdapterAndroid::RemoveDiscoverySession(
       VLOG(1) << "RemoveDiscoverySession: Now 0 sessions. Stopping scan.";
       session_removed = Java_ChromeBluetoothAdapter_stopScan(
           AttachCurrentThread(), j_adapter_);
+      for (const auto& device_id_object_pair : devices_) {
+        device_id_object_pair.second->ClearAdvertisementData();
+      }
     } else {
       VLOG(1) << "RemoveDiscoverySession: Now "
               << unsigned(num_discovery_sessions_) << " sessions.";