bluetooth: Refactor repeated code used to find a Bluetooth object from its ID.

content/browser/bluetooth/bluetooth_dispatcher_host.cc

Index: content/browser/bluetooth/bluetooth_dispatcher_host.cc
diff --git a/content/browser/bluetooth/bluetooth_dispatcher_host.cc b/content/browser/bluetooth/bluetooth_dispatcher_host.cc
index 7a63827e0234f19158ad0f3bdc2b50ac051cf3db..0f7f7a25bddef0aeab53ebf8b693b27645ed21a2 100644
--- a/content/browser/bluetooth/bluetooth_dispatcher_host.cc
+++ b/content/browser/bluetooth/bluetooth_dispatcher_host.cc
@@ -2,12 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// NETWORK_ERROR Note:
-// When a device can't be found in the BluetoothAdapter, that generally
-// indicates that it's gone out of range. We reject with a NetworkError in that
-// case.
-// https://webbluetoothchrome.github.io/web-bluetooth/#dom-bluetoothdevice-connectgatt
-
 // ID Not In Map Note:
 // A service, characteristic, or descriptor ID not in the corresponding
 // BluetoothDispatcherHost map [service_to_device_, characteristic_to_service_,
@@ -283,6 +277,37 @@ struct BluetoothDispatcherHost::RequestDeviceSession {
   scoped_ptr<device::BluetoothDiscoverySession> discovery_session;
 };
 
+struct BluetoothDispatcherHost::CacheQueryResult {
+  CacheQueryResult()
+      : device(nullptr),
+        service(nullptr),
+        characteristic(nullptr),
+        outcome(CacheQueryOutcome::SUCCESS) {}
+  ~CacheQueryResult() {}
+  void SetError(CacheQueryOutcome error_outcome) {
+    outcome = error_outcome;
+    switch (error_outcome) {
+      case CacheQueryOutcome::NO_DEVICE:
+        error = WebBluetoothError::DeviceNoLongerInRange;
+        return;
+      case CacheQueryOutcome::NO_SERVICE:
+        error = WebBluetoothError::ServiceNoLongerExists;
+        return;
+      case CacheQueryOutcome::NO_CHARACTERISTIC:
+        error = WebBluetoothError::CharacteristicNoLongerExists;
+        return;
+      case CacheQueryOutcome::SUCCESS:
+        return;
+    }
+  }
+
+  device::BluetoothDevice* device;
+  device::BluetoothGattService* service;
+  device::BluetoothGattCharacteristic* characteristic;
+  CacheQueryOutcome outcome;
+  blink::WebBluetoothError error;

[scheib, 2015/10/19 23:15:54]

Instead of storing 'error', just have an accessor that can get a
WebBluetoothError from the 'outcome' member.

[ortuno, 2015/10/20 17:44:03]

Done.

+};
+
 void BluetoothDispatcherHost::set_adapter(
     scoped_refptr<device::BluetoothAdapter> adapter) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -517,14 +542,19 @@ void BluetoothDispatcherHost::OnConnectGATT(
   // the device, because any domain can connect to any device. But once
   // permissions are implemented we should check that the domain has access to
   // the device. https://crbug.com/484745
-  device::BluetoothDevice* device = adapter_->GetDevice(device_instance_id);
-  if (device == nullptr) {  // See "NETWORK_ERROR Note" above.
-    RecordConnectGATTOutcome(UMAConnectGATTOutcome::NO_DEVICE);
-    Send(new BluetoothMsg_ConnectGATTError(
-        thread_id, request_id, WebBluetoothError::DeviceNoLongerInRange));
+
+  CacheQueryResult query_result = CacheQueryResult();
+  QueryCacheForDevice(device_instance_id, query_result);
+
+  if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
+    RecordOutcomeWithCacheQueryResult(UMAWebBluetoothFunction::CONNECT_GATT,
+                                      query_result.outcome);
+    Send(new BluetoothMsg_ConnectGATTError(thread_id, request_id,
+                                           query_result.error));
     return;
   }
-  device->CreateGattConnection(
+
+  query_result.device->CreateGattConnection(
       base::Bind(&BluetoothDispatcherHost::OnGATTConnectionCreated,
                  weak_ptr_on_ui_thread_, thread_id, request_id,
                  device_instance_id, start_time),
@@ -565,38 +595,21 @@ void BluetoothDispatcherHost::OnGetCharacteristic(
   RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction::GET_CHARACTERISTIC);
   RecordGetCharacteristicCharacteristic(characteristic_uuid);
 
-  auto device_iter = service_to_device_.find(service_instance_id);
-  // Kill the renderer, see "ID Not In Map Note" above.
-  if (device_iter == service_to_device_.end()) {
-    bad_message::ReceivedBadMessage(this, bad_message::BDH_INVALID_SERVICE_ID);
-    return;
-  }
-
-  // TODO(ortuno): Check if domain has access to device.
-  // https://crbug.com/493459
-  device::BluetoothDevice* device =
-      adapter_->GetDevice(device_iter->second /* device_instance_id */);
-
-  if (device == nullptr) {  // See "NETWORK_ERROR Note" above.
-    RecordGetCharacteristicOutcome(UMAGetCharacteristicOutcome::NO_DEVICE);
-    Send(new BluetoothMsg_GetCharacteristicError(
-        thread_id, request_id, WebBluetoothError::DeviceNoLongerInRange));
+  CacheQueryResult query_result = CacheQueryResult();
+  if (QueryCacheForService(service_instance_id, query_result)) {

[scheib, 2015/10/19 23:15:54]

Just feels like it should be
if(!QueryCache...)
  return; 
Because, we're returning because we failed to get the object out of cache.

[ortuno, 2015/10/20 17:44:03]

Removed the confusing return value and added an enum for the Renderer Crashed
outcome.

     return;
   }
 
-  // TODO(ortuno): Check if domain has access to service
-  // http://crbug.com/493460
-  device::BluetoothGattService* service =
-      device->GetGattService(service_instance_id);
-  if (!service) {
-    RecordGetCharacteristicOutcome(UMAGetCharacteristicOutcome::NO_SERVICE);
-    Send(new BluetoothMsg_GetCharacteristicError(
-        thread_id, request_id, WebBluetoothError::ServiceNoLongerExists));
+  if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
+    RecordOutcomeWithCacheQueryResult(

[scheib, 2015/10/19 23:15:54]

We already have to list the specific function (e.g. 'GetChacteristic'), so why
not make a call directly to a function named that way.

I propose RecordGetCharacteristicOutcome have an overload that takes the
query_result.outcome. That breaks up RecordOutcomeWithCacheQueryResult into many
functions that are more directly named, and don't have a switch of switches.

[ortuno, 2015/10/20 17:44:03]

Done.

+        UMAWebBluetoothFunction::GET_CHARACTERISTIC, query_result.outcome);
+    Send(new BluetoothMsg_GetCharacteristicError(thread_id, request_id,
+                                                 query_result.error));
     return;
   }
 
   for (BluetoothGattCharacteristic* characteristic :
-       service->GetCharacteristics()) {
+       query_result.service->GetCharacteristics()) {
     if (characteristic->GetUUID().canonical_value() == characteristic_uuid) {
       const std::string& characteristic_instance_id =
           characteristic->GetIdentifier();
@@ -629,50 +642,21 @@ void BluetoothDispatcherHost::OnReadValue(
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::CHARACTERISTIC_READ_VALUE);
 
-  auto characteristic_iter =
-      characteristic_to_service_.find(characteristic_instance_id);
-
-  // Kill the renderer, see "ID Not In Map Note" above.
-  if (characteristic_iter == characteristic_to_service_.end()) {
-    bad_message::ReceivedBadMessage(this,
-                                    bad_message::BDH_INVALID_CHARACTERISTIC_ID);
-    return;
-  }
-  const std::string& service_instance_id = characteristic_iter->second;
-
-  auto device_iter = service_to_device_.find(service_instance_id);
-
-  CHECK(device_iter != service_to_device_.end());
-
-  device::BluetoothDevice* device =
-      adapter_->GetDevice(device_iter->second /* device_instance_id */);
-  if (device == nullptr) {  // See "NETWORK_ERROR Note" above.
-    RecordCharacteristicReadValueOutcome(UMAGATTOperationOutcome::NO_DEVICE);
-    Send(new BluetoothMsg_ReadCharacteristicValueError(
-        thread_id, request_id, WebBluetoothError::DeviceNoLongerInRange));
-    return;
-  }
-
-  BluetoothGattService* service = device->GetGattService(service_instance_id);
-  if (service == nullptr) {
-    RecordCharacteristicReadValueOutcome(UMAGATTOperationOutcome::NO_SERVICE);
-    Send(new BluetoothMsg_ReadCharacteristicValueError(
-        thread_id, request_id, WebBluetoothError::ServiceNoLongerExists));
+  CacheQueryResult query_result = CacheQueryResult();
+  if (QueryCacheForCharacteristic(characteristic_instance_id, query_result)) {
     return;
   }
 
-  BluetoothGattCharacteristic* characteristic =
-      service->GetCharacteristic(characteristic_instance_id);
-  if (characteristic == nullptr) {
-    RecordCharacteristicReadValueOutcome(
-        UMAGATTOperationOutcome::NO_CHARACTERISTIC);
-    Send(new BluetoothMsg_ReadCharacteristicValueError(
-        thread_id, request_id,
-        WebBluetoothError::CharacteristicNoLongerExists));
+  if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
+    RecordOutcomeWithCacheQueryResult(
+        UMAWebBluetoothFunction::CHARACTERISTIC_READ_VALUE,
+        query_result.outcome);
+    Send(new BluetoothMsg_ReadCharacteristicValueError(thread_id, request_id,
+                                                       query_result.error));
     return;
   }
 
-  characteristic->ReadRemoteCharacteristic(
+  query_result.characteristic->ReadRemoteCharacteristic(
       base::Bind(&BluetoothDispatcherHost::OnCharacteristicValueRead,
                  weak_ptr_on_ui_thread_, thread_id, request_id),
       base::Bind(&BluetoothDispatcherHost::OnCharacteristicReadValueError,
@@ -699,49 +683,21 @@ void BluetoothDispatcherHost::OnWriteValue(
     return;
   }
 
-  auto characteristic_iter =
-      characteristic_to_service_.find(characteristic_instance_id);
-
-  // Kill the renderer, see "ID Not In Map Note" above.
-  if (characteristic_iter == characteristic_to_service_.end()) {
-    bad_message::ReceivedBadMessage(this,
-                                    bad_message::BDH_INVALID_CHARACTERISTIC_ID);
-    return;
-  }
-  const std::string& service_instance_id = characteristic_iter->second;
-
-  auto device_iter = service_to_device_.find(service_instance_id);
-
-  CHECK(device_iter != service_to_device_.end());
-
-  device::BluetoothDevice* device =
-      adapter_->GetDevice(device_iter->second /* device_instance_id */);
-  if (device == nullptr) {  // See "NETWORK_ERROR Note" above.
-    RecordCharacteristicWriteValueOutcome(UMAGATTOperationOutcome::NO_DEVICE);
-    Send(new BluetoothMsg_WriteCharacteristicValueError(
-        thread_id, request_id, WebBluetoothError::DeviceNoLongerInRange));
+  CacheQueryResult query_result = CacheQueryResult();
+  if (QueryCacheForCharacteristic(characteristic_instance_id, query_result)) {
     return;
   }
 
-  BluetoothGattService* service = device->GetGattService(service_instance_id);
-  if (service == nullptr) {
-    RecordCharacteristicWriteValueOutcome(UMAGATTOperationOutcome::NO_SERVICE);
-    Send(new BluetoothMsg_WriteCharacteristicValueError(
-        thread_id, request_id, WebBluetoothError::ServiceNoLongerExists));
+  if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
+    RecordOutcomeWithCacheQueryResult(
+        UMAWebBluetoothFunction::CHARACTERISTIC_WRITE_VALUE,
+        query_result.outcome);
+    Send(new BluetoothMsg_WriteCharacteristicValueError(thread_id, request_id,
+                                                        query_result.error));
     return;
   }
 
-  BluetoothGattCharacteristic* characteristic =
-      service->GetCharacteristic(characteristic_instance_id);
-  if (characteristic == nullptr) {
-    RecordCharacteristicWriteValueOutcome(
-        UMAGATTOperationOutcome::NO_CHARACTERISTIC);
-    Send(new BluetoothMsg_WriteCharacteristicValueError(
-        thread_id, request_id,
-        WebBluetoothError::CharacteristicNoLongerExists));
-    return;
-  }
-  characteristic->WriteRemoteCharacteristic(
+  query_result.characteristic->WriteRemoteCharacteristic(
       value, base::Bind(&BluetoothDispatcherHost::OnWriteValueSuccess,
                         weak_ptr_on_ui_thread_, thread_id, request_id),
       base::Bind(&BluetoothDispatcherHost::OnWriteValueFailed,
@@ -768,48 +724,19 @@ void BluetoothDispatcherHost::OnStartNotifications(
   // TODO(ortuno): Check if notify/indicate bit is set.
   // http://crbug.com/538869
 
-  auto characteristic_iter =
-      characteristic_to_service_.find(characteristic_instance_id);
-
-  // Kill the renderer, see "ID Not In Map Note" above.
-  if (characteristic_iter == characteristic_to_service_.end()) {
-    bad_message::ReceivedBadMessage(this,
-                                    bad_message::BDH_INVALID_CHARACTERISTIC_ID);
-    return;
-  }
-
-  const std::string& service_instance_id = characteristic_iter->second;
-  auto device_iter = service_to_device_.find(service_instance_id);
-  CHECK(device_iter != service_to_device_.end());
-
-  device::BluetoothDevice* device =
-      adapter_->GetDevice(device_iter->second /* device_instance_id */);
-  if (device == nullptr) {  // See "NETWORK_ERROR Note" above.
-    RecordStartNotificationsOutcome(UMAGATTOperationOutcome::NO_DEVICE);
-    Send(new BluetoothMsg_StartNotificationsError(
-        thread_id, request_id, WebBluetoothError::DeviceNoLongerInRange));
-    return;
-  }
-
-  BluetoothGattService* service = device->GetGattService(service_instance_id);
-  if (service == nullptr) {
-    RecordStartNotificationsOutcome(UMAGATTOperationOutcome::NO_SERVICE);
-    Send(new BluetoothMsg_StartNotificationsError(
-        thread_id, request_id, WebBluetoothError::ServiceNoLongerExists));
-    return;
-  }
+  CacheQueryResult query_result = CacheQueryResult();
+  QueryCacheForCharacteristic(characteristic_instance_id, query_result);
 
-  BluetoothGattCharacteristic* characteristic =
-      service->GetCharacteristic(characteristic_instance_id);
-  if (characteristic == nullptr) {
-    RecordStartNotificationsOutcome(UMAGATTOperationOutcome::NO_CHARACTERISTIC);
-    Send(new BluetoothMsg_StartNotificationsError(
-        thread_id, request_id,
-        WebBluetoothError::CharacteristicNoLongerExists));
+  if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
+    RecordOutcomeWithCacheQueryResult(
+        UMAWebBluetoothFunction::CHARACTERISTIC_START_NOTIFICATIONS,
+        query_result.outcome);
+    Send(new BluetoothMsg_StartNotificationsError(thread_id, request_id,
+                                                  query_result.error));
     return;
   }
 
-  characteristic->StartNotifySession(
+  query_result.characteristic->StartNotifySession(
       base::Bind(&BluetoothDispatcherHost::OnStartNotifySessionSuccess,
                  weak_ptr_factory_.GetWeakPtr(), thread_id, request_id),
       base::Bind(&BluetoothDispatcherHost::OnStartNotifySessionFailed,
@@ -1026,16 +953,19 @@ void BluetoothDispatcherHost::OnServicesDiscovered(
     const std::string& service_uuid) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  device::BluetoothDevice* device = adapter_->GetDevice(device_instance_id);
-  if (device == nullptr) {  // See "NETWORK_ERROR Note" above.
-    RecordGetPrimaryServiceOutcome(UMAGetPrimaryServiceOutcome::NO_DEVICE);
-    Send(new BluetoothMsg_GetPrimaryServiceError(
-        thread_id, request_id, WebBluetoothError::DeviceNoLongerInRange));
+  CacheQueryResult query_result = CacheQueryResult();
+  QueryCacheForDevice(device_instance_id, query_result);
+
+  if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
+    RecordOutcomeWithCacheQueryResult(
+        UMAWebBluetoothFunction::GET_PRIMARY_SERVICE, query_result.outcome);
+    Send(new BluetoothMsg_GetPrimaryServiceError(thread_id, request_id,
+                                                 query_result.error));
     return;
   }
 
   VLOG(1) << "Looking for service: " << service_uuid;
-  for (BluetoothGattService* service : device->GetGattServices()) {
+  for (BluetoothGattService* service : query_result.device->GetGattServices()) {
     VLOG(1) << "Service in cache: " << service->GetUUID().canonical_value();
     if (service->GetUUID().canonical_value() == service_uuid) {
       // TODO(ortuno): Use generated instance ID instead.
@@ -1129,6 +1059,77 @@ void BluetoothDispatcherHost::OnStopNotifySession(
   Send(new BluetoothMsg_StopNotificationsSuccess(thread_id, request_id));
 }
 
+void BluetoothDispatcherHost::QueryCacheForDevice(
+    const std::string& device_instance_id,
+    CacheQueryResult& result) {
+  result.device = adapter_->GetDevice(device_instance_id);
+  // When a device can't be found in the BluetoothAdapter, that generally
+  // indicates that it's gone out of range. We reject with a NetworkError in
+  // that case.
+  // https://webbluetoothchrome.github.io/web-bluetooth/#dom-bluetoothdevice-connectgatt
+  if (result.device == nullptr) {
+    result.SetError(CacheQueryOutcome::NO_DEVICE);
+  }
+}
+
+bool BluetoothDispatcherHost::QueryCacheForService(
+    const std::string& service_instance_id,
+    CacheQueryResult& result) {
+  auto device_iter = service_to_device_.find(service_instance_id);
+
+  // Kill the renderer, see "ID Not In Map Note" above.
+  if (device_iter == service_to_device_.end()) {
+    bad_message::ReceivedBadMessage(this, bad_message::BDH_INVALID_SERVICE_ID);
+    return true;
+  }
+
+  // TODO(ortuno): Check if domain has access to device.
+  // https://crbug.com/493459
+
+  QueryCacheForDevice(device_iter->second, result);
+
+  if (result.outcome != CacheQueryOutcome::SUCCESS) {
+    return false;
+  }
+
+  result.service = result.device->GetGattService(service_instance_id);
+
+  if (result.service == nullptr) {
+    result.SetError(CacheQueryOutcome::NO_SERVICE);
+  }
+  return false;
+}
+
+bool BluetoothDispatcherHost::QueryCacheForCharacteristic(
+    const std::string& characteristic_instance_id,
+    CacheQueryResult& result) {
+  auto characteristic_iter =
+      characteristic_to_service_.find(characteristic_instance_id);
+
+  // Kill the renderer, see "ID Not In Map Note" above.
+  if (characteristic_iter == characteristic_to_service_.end()) {
+    bad_message::ReceivedBadMessage(this,
+                                    bad_message::BDH_INVALID_CHARACTERISTIC_ID);
+    return true;
+  }
+
+  if (QueryCacheForService(characteristic_iter->second, result)) {
+    return true;
+  }
+
+  if (result.outcome != CacheQueryOutcome::SUCCESS) {
+    return false;
+  }
+
+  result.characteristic =
+      result.service->GetCharacteristic(characteristic_instance_id);
+
+  if (result.characteristic == nullptr) {
+    result.SetError(CacheQueryOutcome::NO_CHARACTERISTIC);
+  }
+  return false;
+}
+
 void BluetoothDispatcherHost::ShowBluetoothOverviewLink() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   NOTIMPLEMENTED();