bluetooth: Refactor how we update based on Advertising Data

device/bluetooth/bluetooth_adapter_android.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "device/bluetooth/bluetooth_adapter_android.h"
 
 #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"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "device/bluetooth/android/wrappers.h"
 #include "device/bluetooth/bluetooth_advertisement.h"
 #include "device/bluetooth/bluetooth_device_android.h"
 #include "device/bluetooth/bluetooth_discovery_session_outcome.h"
 #include "jni/ChromeBluetoothAdapter_jni.h"
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF8;
+using base::android::AppendJavaStringArrayToStringVector;
 using base::android::JavaParamRef;
 
 namespace {
 // The poll interval in ms when there is no active discovery. This
 // matches the max allowed advertisting interval for connectable
 // devices.
 enum { kPassivePollInterval = 11000 };
 // The poll interval in ms when there is an active discovery.
 enum { kActivePollInterval = 1000 };
 // The delay in ms to wait before purging devices when a scan starts.
@@ skipping 146 matching lines @@
     JNIEnv* env,
     const JavaParamRef<jobject>& caller,
     const JavaParamRef<jstring>& address,
     const JavaParamRef<jobject>&
         bluetooth_device_wrapper,  // Java Type: bluetoothDeviceWrapper
     const JavaParamRef<jobjectArray>&
         advertised_uuids) {  // Java Type: List<ParcelUuid>
   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));
   }
 }
 
 BluetoothAdapterAndroid::BluetoothAdapterAndroid() : weak_ptr_factory_(this) {
 }
 
 BluetoothAdapterAndroid::~BluetoothAdapterAndroid() {
   Java_ChromeBluetoothAdapter_onBluetoothAdapterAndroidDestruction(
@@ skipping 58 matching lines @@
   if (num_discovery_sessions_ == 0) {
     VLOG(1) << "RemoveDiscoverySession: No scan in progress.";
     NOTREACHED();
   } else {
     --num_discovery_sessions_;
     session_removed = true;
     if (num_discovery_sessions_ == 0) {
       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.";
     }
   }
 
   if (session_removed) {
     callback.Run();
   } else {
     // TODO(scheib): Eventually wire the SCAN_FAILED result through to here.
     error_callback.Run(UMABluetoothDiscoverySessionOutcome::UNKNOWN);
   }
 }
 
 void BluetoothAdapterAndroid::SetDiscoveryFilter(
     std::unique_ptr<BluetoothDiscoveryFilter> discovery_filter,
     const base::Closure& callback,
     const DiscoverySessionErrorCallback& error_callback) {
   // TODO(scheib): Support filters crbug.com/490401
   NOTIMPLEMENTED();
   error_callback.Run(UMABluetoothDiscoverySessionOutcome::NOT_IMPLEMENTED);
 }
 
 void BluetoothAdapterAndroid::RemovePairingDelegateInternal(
     device::BluetoothDevice::PairingDelegate* pairing_delegate) {
 }
 
 }  // namespace device