Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

content/browser/bluetooth/web_bluetooth_service_impl.cc

 // Copyright 2016 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.
 
 // ID Not In Map Note: A service, characteristic, or descriptor ID not in the
 // corresponding WebBluetoothServiceImpl map [service_id_to_device_address_,
 // characteristic_id_to_service_id_, descriptor_id_to_characteristic_id_]
 // 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.
 
@@ skipping 215 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (device_chooser_controller_.get()) {
     device_chooser_controller_->AddFilteredDevice(*device);
   }
 
   if (!device->IsGattConnected()) {
     base::Optional<WebBluetoothDeviceId> device_id =
         connected_devices_->CloseConnectionToDeviceWithAddress(
             device->GetAddress());
-    if (device_id && client_) {
-      client_->GattServerDisconnected(device_id.value());
-    }
   }
 }
 
 void WebBluetoothServiceImpl::GattServicesDiscovered(
     device::BluetoothAdapter* adapter,
     device::BluetoothDevice* device) {
   if (device_chooser_controller_.get()) {
     device_chooser_controller_->AddFilteredDevice(*device);
   }
 
@@ skipping 36 matching lines @@
           base::Bind(&WebBluetoothServiceImpl::NotifyCharacteristicValueChanged,
                      weak_ptr_factory_.GetWeakPtr(),
                      characteristic->GetIdentifier(), value))) {
     LOG(WARNING) << "No TaskRunner.";
   }
 }
 
 void WebBluetoothServiceImpl::NotifyCharacteristicValueChanged(
     const std::string& characteristic_instance_id,
     const std::vector<uint8_t>& value) {
-  if (client_) {
-    client_->RemoteCharacteristicValueChanged(characteristic_instance_id,
-                                              value);
+  auto iter =
+      characteristic_id_to_notify_session_.find(characteristic_instance_id);
+  if (iter != characteristic_id_to_notify_session_.end()) {
+    iter->second.second->RemoteCharacteristicValueChanged(value);
   }
 }
 
-void WebBluetoothServiceImpl::SetClient(
-    blink::mojom::WebBluetoothServiceClientAssociatedPtrInfo client) {
-  DCHECK(!client_.get());
-  client_.Bind(std::move(client));
-}
-
 void WebBluetoothServiceImpl::RequestDevice(
     blink::mojom::WebBluetoothRequestDeviceOptionsPtr options,
     const RequestDeviceCallback& callback) {
   RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction::REQUEST_DEVICE);
   RecordRequestDeviceOptions(options);
 
   if (!GetAdapter()) {
     if (BluetoothAdapterFactoryWrapper::Get().IsLowEnergyAvailable()) {
       BluetoothAdapterFactoryWrapper::Get().AcquireAdapter(
           this, base::Bind(&WebBluetoothServiceImpl::RequestDeviceImpl,
@@ skipping 14 matching lines @@
 void WebBluetoothServiceImpl::RemoteServerConnect(
     const WebBluetoothDeviceId& device_id,
     const RemoteServerConnectCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction::CONNECT_GATT);
 
   const CacheQueryResult query_result = QueryCacheForDevice(device_id);
 
   if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
     RecordConnectGATTOutcome(query_result.outcome);
-    callback.Run(query_result.GetWebResult());
+    callback.Run(query_result.GetWebResult(), nullptr);

[ortuno, 2017/02/24 03:28:52]

nit: comment what nullptr is. Same below.

[juncai, 2017/03/01 02:04:12]

Done.

     return;
   }
 
   if (connected_devices_->IsConnectedToDeviceWithId(device_id)) {
     DVLOG(1) << "Already connected.";
-    callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
+    callback.Run(blink::mojom::WebBluetoothResult::SUCCESS, nullptr);
     return;
   }
 
   // It's possible for WebBluetoothServiceImpl to issue two successive
   // connection requests for which it would get two successive responses
   // and consequently try to insert two BluetoothGattConnections for the
   // same device. WebBluetoothServiceImpl should reject or queue connection
   // requests if there is a pending connection already, but the platform
   // abstraction doesn't currently support checking for pending connections.
   // TODO(ortuno): CHECK that this never happens once the platform
@@ skipping 324 matching lines @@
 void WebBluetoothServiceImpl::RemoteCharacteristicStartNotifications(
     const std::string& characteristic_instance_id,
     const RemoteCharacteristicStartNotificationsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::CHARACTERISTIC_START_NOTIFICATIONS);
 
   auto iter =
       characteristic_id_to_notify_session_.find(characteristic_instance_id);
   if (iter != characteristic_id_to_notify_session_.end() &&
-      iter->second->IsActive()) {
+      iter->second.first->IsActive()) {
     // If the frame has already started notifications and the notifications
     // are active we return SUCCESS.
-    callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
+    callback.Run(blink::mojom::WebBluetoothResult::SUCCESS, nullptr);
     return;
   }
 
   const CacheQueryResult query_result =
       QueryCacheForCharacteristic(characteristic_instance_id);
 
   if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
     return;
   }
 
   if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
     RecordStartNotificationsOutcome(query_result.outcome);
-    callback.Run(query_result.GetWebResult());
+    callback.Run(query_result.GetWebResult(), nullptr);
     return;
   }
 
   device::BluetoothRemoteGattCharacteristic::Properties notify_or_indicate =
       query_result.characteristic->GetProperties() &
       (device::BluetoothRemoteGattCharacteristic::PROPERTY_NOTIFY |
        device::BluetoothRemoteGattCharacteristic::PROPERTY_INDICATE);
   if (!notify_or_indicate) {
-    callback.Run(blink::mojom::WebBluetoothResult::GATT_NOT_SUPPORTED);
+    callback.Run(blink::mojom::WebBluetoothResult::GATT_NOT_SUPPORTED, nullptr);
     return;
   }
 
   query_result.characteristic->StartNotifySession(
       base::Bind(&WebBluetoothServiceImpl::OnStartNotifySessionSuccess,
                  weak_ptr_factory_.GetWeakPtr(), callback),
       base::Bind(&WebBluetoothServiceImpl::OnStartNotifySessionFailed,
                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
@@ skipping 12 matching lines @@
   }
 
   auto notify_session_iter =
       characteristic_id_to_notify_session_.find(characteristic_instance_id);
   if (notify_session_iter == characteristic_id_to_notify_session_.end()) {
     // If the frame hasn't subscribed to notifications before we just
     // run the callback.
     callback.Run();
     return;
   }
-  notify_session_iter->second->Stop(base::Bind(
+  notify_session_iter->second.first->Stop(base::Bind(
       &WebBluetoothServiceImpl::OnStopNotifySessionComplete,
       weak_ptr_factory_.GetWeakPtr(), characteristic_instance_id, callback));
 }
 
 void WebBluetoothServiceImpl::RemoteDescriptorReadValue(
     const std::string& descriptor_instance_id,
     const RemoteDescriptorReadValueCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::DESCRIPTOR_READ_VALUE);
@@ skipping 189 matching lines @@
 
 void WebBluetoothServiceImpl::OnCreateGATTConnectionSuccess(
     const WebBluetoothDeviceId& device_id,
     base::TimeTicks start_time,
     const RemoteServerConnectCallback& callback,
     std::unique_ptr<device::BluetoothGattConnection> connection) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordConnectGATTTimeSuccess(base::TimeTicks::Now() - start_time);
   RecordConnectGATTOutcome(UMAConnectGATTOutcome::SUCCESS);
 
-  connected_devices_->Insert(device_id, std::move(connection));
-  callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
+  blink::mojom::WebBluetoothServerClientAssociatedPtr client;
+  callback.Run(blink::mojom::WebBluetoothResult::SUCCESS,
+               mojo::MakeRequest(&client));
+  connected_devices_->Insert(device_id, std::move(connection),
+                             std::move(client));

[ortuno, 2017/02/24 03:28:52]

Would it make sense to remove the RemoteGATTServerDisconnect method and just use
error handlers?

// web_bluetooth_service_impl.cc

// TODO: Maybe we could use a better name.
void WebBluetoothServiceImpl::HandlerServerClientError(const
WebBluetoothDeviceId& id) {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  RecordWebBluetoothFunctionCall(
      UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT);

  if (connected_devices_->IsConnectedToDeviceWithId(device_id)) {
    DVLOG(1) << "Disconnecting device: " << device_id.str();
    connected_devices_->CloseConnectionToDeviceWithId(device_id);
  }
}


void WebBluetoothServiceImpl::OnCreateGATTConnectionSuccess(...) {
  // ...
  blink::mojom::WebBluetoothServerClientAssociatedPtr client;
  client.set_connection_error_handler(base::Bind(
    &WebBluetoothServiceImpl::HandlerServerClientError,
    weak_ptr_factory_.GetWeakPtr(),
    device_id));
}

// Then in frame_connected_bluetooth_devices.cc we can just close the pipe.

FrameConnectedBluetoothDevices::CloseConnectionToDeviceWithAddress(
    const std::string& device_address) {
  // ...
  CHECK(device_address_to_id_map_.erase(device_address));
  CHECK(device_id_to_connection_map_.erase(device_id));
}

// And in BluetoothRemoteGATTServer.cpp

void BluetoothRemoteGATTServer::disconnect(ScriptState* scriptState) {
  // ...
  if (m_clientBinding.is_bound())
    m_clientBinding.Close();
}

void BluetoothRemoteGATTServer::HandleClientConnectionError() {
  m_device->dispatchGattServerDisconnected();
}

void BluetoothRemoteGATTServer::ConnectCallback(...) {

  // Bind binding and set error handler to handleClientConnectionError();
}

[juncai, 2017/03/01 02:04:12]

Making sense, since it also needs to change the web_bluetooth.mojom, how about
doing it in a follow-up CL?

[juncai, 2017/03/02 03:23:38]

I opened an issue for that:
https://bugs.chromium.org/p/chromium/issues/detail?id=697700

 }
 
 void WebBluetoothServiceImpl::OnCreateGATTConnectionFailed(
     base::TimeTicks start_time,
     const RemoteServerConnectCallback& callback,
     device::BluetoothDevice::ConnectErrorCode error_code) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordConnectGATTTimeFailed(base::TimeTicks::Now() - start_time);
-  callback.Run(TranslateConnectErrorAndRecord(error_code));
+  callback.Run(TranslateConnectErrorAndRecord(error_code), nullptr);
 }
 
 void WebBluetoothServiceImpl::OnCharacteristicReadValueSuccess(
     const RemoteCharacteristicReadValueCallback& callback,
     const std::vector<uint8_t>& value) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordCharacteristicReadValueOutcome(UMAGATTOperationOutcome::SUCCESS);
   callback.Run(blink::mojom::WebBluetoothResult::SUCCESS, value);
 }
 
@@ skipping 22 matching lines @@
 }
 
 void WebBluetoothServiceImpl::OnStartNotifySessionSuccess(
     const RemoteCharacteristicStartNotificationsCallback& callback,
     std::unique_ptr<device::BluetoothGattNotifySession> notify_session) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Copy Characteristic Instance ID before passing a unique pointer because
   // compilers may evaluate arguments in any order.
   std::string characteristic_instance_id =
       notify_session->GetCharacteristicIdentifier();
+
+  blink::mojom::WebBluetoothCharacteristicClientAssociatedPtr client;
+  callback.Run(blink::mojom::WebBluetoothResult::SUCCESS,
+               mojo::MakeRequest(&client));
   // Saving the BluetoothGattNotifySession keeps notifications active.
   characteristic_id_to_notify_session_[characteristic_instance_id] =
-      std::move(notify_session);
-  callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
+      std::make_pair(std::move(notify_session), std::move(client));
 }
 
 void WebBluetoothServiceImpl::OnStartNotifySessionFailed(
     const RemoteCharacteristicStartNotificationsCallback& callback,
     device::BluetoothRemoteGattService::GattErrorCode error_code) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   callback.Run(TranslateGATTErrorAndRecord(
-      error_code, UMAGATTOperation::START_NOTIFICATIONS));
+                   error_code, UMAGATTOperation::START_NOTIFICATIONS),
+               nullptr);
 }
 
 void WebBluetoothServiceImpl::OnStopNotifySessionComplete(
     const std::string& characteristic_instance_id,
     const RemoteCharacteristicStopNotificationsCallback& callback) {
   characteristic_id_to_notify_session_.erase(characteristic_instance_id);
   callback.Run();
 }
 
 void WebBluetoothServiceImpl::OnDescriptorReadValueSuccess(
@@ skipping 172 matching lines @@
   descriptor_id_to_characteristic_id_.clear();
   characteristic_id_to_service_id_.clear();
   service_id_to_device_address_.clear();
   connected_devices_.reset(
       new FrameConnectedBluetoothDevices(render_frame_host_));
   device_chooser_controller_.reset();
   BluetoothAdapterFactoryWrapper::Get().ReleaseAdapter(this);
 }
 
 }  // namespace content