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

content/browser/bluetooth/bluetooth_dispatcher_host.cc

 // Copyright 2014 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.
 
-// 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_,
 // descriptor_to_characteristic_] implies a hostile renderer because a renderer
 // obtains the corresponding ID from this class and it will be added to the map
 // at that time.
 
 #include "content/browser/bluetooth/bluetooth_dispatcher_host.h"
 
 #include "base/bind.h"
@@ skipping 255 matching lines @@
   }
 
   const int thread_id;
   const int request_id;
   const std::vector<BluetoothScanFilter> filters;
   const std::vector<BluetoothUUID> optional_services;
   scoped_ptr<BluetoothChooser> chooser;
   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);
   connections_.clear();
   if (adapter_.get())
     adapter_->RemoveObserver(this);
   adapter_ = adapter;
   if (adapter_.get())
     adapter_->AddObserver(this);
 }
@@ skipping 214 matching lines @@
     int request_id,
     const std::string& device_instance_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction::CONNECT_GATT);
   const base::TimeTicks start_time = base::TimeTicks::Now();
 
   // TODO(ortuno): Right now it's pointless to check if the domain has access to
   // 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),
       base::Bind(&BluetoothDispatcherHost::OnCreateGATTConnectionError,
                  weak_ptr_on_ui_thread_, thread_id, request_id,
                  device_instance_id, start_time));
 }
 
 void BluetoothDispatcherHost::OnGetPrimaryService(
     int thread_id,
@@ skipping 20 matching lines @@
 
 void BluetoothDispatcherHost::OnGetCharacteristic(
     int thread_id,
     int request_id,
     const std::string& service_instance_id,
     const std::string& characteristic_uuid) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   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);
+  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 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));
-    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();
 
       auto insert_result = characteristic_to_service_.insert(
           make_pair(characteristic_instance_id, service_instance_id));
 
       // If  value is already in map, DCHECK it's valid.
       if (!insert_result.second)
         DCHECK(insert_result.first->second == service_instance_id);
@@ skipping 12 matching lines @@
 }
 
 void BluetoothDispatcherHost::OnReadValue(
     int thread_id,
     int request_id,
     const std::string& characteristic_instance_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   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));
+  CacheQueryResult query_result = CacheQueryResult();
+  if (QueryCacheForCharacteristic(characteristic_instance_id, query_result)) {
     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));
+  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;
   }
 
-  BluetoothGattCharacteristic* characteristic =
-      service->GetCharacteristic(characteristic_instance_id);
-  if (characteristic == nullptr) {
-    RecordCharacteristicReadValueOutcome(
-        UMAGATTOperationOutcome::NO_CHARACTERISTIC);
-    Send(new BluetoothMsg_ReadCharacteristicValueError(
-        thread_id, request_id,
-        WebBluetoothError::CharacteristicNoLongerExists));
-    return;
-  }
-
-  characteristic->ReadRemoteCharacteristic(
+  query_result.characteristic->ReadRemoteCharacteristic(
       base::Bind(&BluetoothDispatcherHost::OnCharacteristicValueRead,
                  weak_ptr_on_ui_thread_, thread_id, request_id),
       base::Bind(&BluetoothDispatcherHost::OnCharacteristicReadValueError,
                  weak_ptr_on_ui_thread_, thread_id, request_id));
 }
 
 void BluetoothDispatcherHost::OnWriteValue(
     int thread_id,
     int request_id,
     const std::string& characteristic_instance_id,
     const std::vector<uint8_t>& value) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::CHARACTERISTIC_WRITE_VALUE);
 
   // Length check per step 3 of writeValue algorithm:
   // https://webbluetoothchrome.github.io/web-bluetooth/#dom-bluetoothgattcharacteristic-writevalue
   // We perform the length check on the renderer side. So if we
   // get a value with length > 512, we can assume it's a hostile
   // renderer and kill it.
   if (value.size() > 512) {
     bad_message::ReceivedBadMessage(
         this, bad_message::BDH_INVALID_WRITE_VALUE_LENGTH);
     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,
                  weak_ptr_on_ui_thread_, thread_id, request_id));
 }
 
 void BluetoothDispatcherHost::OnStartNotifications(
     int thread_id,
     int request_id,
     const std::string& characteristic_instance_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::CHARACTERISTIC_START_NOTIFICATIONS);
 
   // BluetoothDispatcher will never send a request for a characteristic
   // already subscribed to notifications.
   if (characteristic_id_to_notify_session_.find(characteristic_instance_id) !=
       characteristic_id_to_notify_session_.end()) {
     bad_message::ReceivedBadMessage(
         this, bad_message::BDH_CHARACTERISTIC_ALREADY_SUBSCRIBED);
     return;
   }
 
   // TODO(ortuno): Check if notify/indicate bit is set.
   // http://crbug.com/538869
 
-  auto characteristic_iter =
-      characteristic_to_service_.find(characteristic_instance_id);
+  CacheQueryResult query_result = CacheQueryResult();
+  QueryCacheForCharacteristic(characteristic_instance_id, query_result);
 
-  // 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);
+  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;
   }
 
-  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;
-  }
-
-  BluetoothGattCharacteristic* characteristic =
-      service->GetCharacteristic(characteristic_instance_id);
-  if (characteristic == nullptr) {
-    RecordStartNotificationsOutcome(UMAGATTOperationOutcome::NO_CHARACTERISTIC);
-    Send(new BluetoothMsg_StartNotificationsError(
-        thread_id, request_id,
-        WebBluetoothError::CharacteristicNoLongerExists));
-    return;
-  }
-
-  characteristic->StartNotifySession(
+  query_result.characteristic->StartNotifySession(
       base::Bind(&BluetoothDispatcherHost::OnStartNotifySessionSuccess,
                  weak_ptr_factory_.GetWeakPtr(), thread_id, request_id),
       base::Bind(&BluetoothDispatcherHost::OnStartNotifySessionFailed,
                  weak_ptr_factory_.GetWeakPtr(), thread_id, request_id));
 }
 
 void BluetoothDispatcherHost::OnStopNotifications(
     int thread_id,
     int request_id,
     const std::string& characteristic_instance_id) {
@@ skipping 196 matching lines @@
                                          TranslateConnectError(error_code)));
 }
 
 void BluetoothDispatcherHost::OnServicesDiscovered(
     int thread_id,
     int request_id,
     const std::string& device_instance_id,
     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.
       // https://crbug.com/495379
       const std::string& service_identifier = service->GetIdentifier();
       auto insert_result = service_to_device_.insert(
           make_pair(service_identifier, device_instance_id));
 
       // If a value is already in map, DCHECK it's valid.
       if (!insert_result.second)
@@ skipping 73 matching lines @@
 }
 
 void BluetoothDispatcherHost::OnStopNotifySession(
     int thread_id,
     int request_id,
     const std::string& characteristic_instance_id) {
   characteristic_id_to_notify_session_.erase(characteristic_instance_id);
   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();
 }
 
 void BluetoothDispatcherHost::ShowBluetoothPairingLink() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   NOTIMPLEMENTED();
 }
 
 void BluetoothDispatcherHost::ShowBluetoothAdapterOffLink() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   NOTIMPLEMENTED();
 }
 
 }  // namespace content