bluetooth: Remove indicator only when there are no more devices connected

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.
 
 // 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.
@@ skipping 328 matching lines @@
     DCHECK(pending_primary_services_requests_.empty());
 
     // The following data structures are cleaned up when a
     // device/service/characteristic is removed.
     // Since this can happen after the test is done and the cleanup function is
     // called, we clean them here.
     service_to_device_.clear();
     characteristic_to_service_.clear();
     characteristic_id_to_notify_session_.clear();
     active_characteristic_threads_.clear();
     device_id_to_connection_map_.clear();

[ncarter (slow), 2016/04/07 19:48:54]

Do we need to decrement here?

[ortuno, 2016/04/07 22:34:51]

This code is only run during tests. Anyway, the new object cleans up WebContents
before destruction.

     allowed_devices_map_ = BluetoothAllowedDevicesMap();
   }
 
   set_adapter(std::move(mock_adapter));
 }
 
 BluetoothDispatcherHost::~BluetoothDispatcherHost() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Clear adapter, releasing observer references.
   set_adapter(scoped_refptr<device::BluetoothAdapter>());
@@ skipping 340 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT);
 
   // Frames can send a disconnect request after they've started navigating,
   // making calls to GetLastCommitted origin invalid. Because we still need
   // to disconnect the device, otherwise we would leave users with no other
   // option to disconnect than closing the tab, we purposefully don't
   // check if the frame has permission to interact with the device.
 
-  RenderFrameHostImpl* render_frame_host =
-      RenderFrameHostImpl::FromID(render_process_id_, frame_routing_id);
-  WebContents* web_contents =
-      WebContents::FromRenderFrameHost(render_frame_host);
-  if (web_contents) {
-    web_contents->SetBluetoothDeviceConnected(false);
-  }
-
   // The last BluetoothGattConnection for a device closes the connection when
   // it's destroyed.
   if (device_id_to_connection_map_.erase(device_id)) {
     VLOG(1) << "Disconnecting device: " << device_id;
+
+    // This only catches disconnections from the renderer. If the device
+    // disconnects by itself, or the renderer frame has been deleted
+    // due to navigation, we will not hide the indicator.
+    // TODO(ortuno): Once we move to Frame and Mojo we will be able
+    // to observe the frame's lifetime and hide the indicator when necessary.
+    // http://crbug.com/508771
+
+    // Indicate there is one less connected device.
+    RenderFrameHostImpl* render_frame_host =
+        RenderFrameHostImpl::FromID(render_process_id_, frame_routing_id);
+    WebContents* web_contents =
+        WebContents::FromRenderFrameHost(render_frame_host);
+    if (web_contents) {
+      web_contents->DecrementBluetoothConnectedDeviceCount();
+    }
   }
 }
 
 void BluetoothDispatcherHost::OnGetPrimaryService(
     int thread_id,
     int request_id,
     int frame_routing_id,
     const std::string& device_id,
     const std::string& service_uuid) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ skipping 668 matching lines @@
 }
 
 void BluetoothDispatcherHost::OnGATTConnectionCreated(
     int thread_id,
     int request_id,
     int frame_routing_id,
     const std::string& device_id,
     base::TimeTicks start_time,
     scoped_ptr<device::BluetoothGattConnection> connection) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   device_id_to_connection_map_[device_id] = std::move(connection);

[ncarter (slow), 2016/04/07 19:48:54]

If the device was already in the map, then the connection count on the
WebContents is incorrect -- we wouldn't have decremented it for the overwritten
entry. Do we need to defend against that?

It might be worthwhile considering making the values of the
device_id_to_connection_map_ be RAII-style wrapper objects, that hold the
|connection| but also increment/decrement the WebContents count in their ctors &
dtors. My experience is that it is much easier to keep the increments/decrements
balanced that way, especially as the code gets refactored.

[ortuno, 2016/04/07 22:34:51]

Added a new ConnectedDevicesMap. WDYT?

   RecordConnectGATTTimeSuccess(base::TimeTicks::Now() - start_time);
   RecordConnectGATTOutcome(UMAConnectGATTOutcome::SUCCESS);
   RenderFrameHostImpl* render_frame_host =
       RenderFrameHostImpl::FromID(render_process_id_, frame_routing_id);
   WebContents* web_contents =
       WebContents::FromRenderFrameHost(render_frame_host);
   if (web_contents) {
-    web_contents->SetBluetoothDeviceConnected(true);
+    web_contents->IncrementBluetoothConnectedDeviceCount();
   }
   Send(new BluetoothMsg_GATTServerConnectSuccess(thread_id, request_id));
 }
 
 void BluetoothDispatcherHost::OnCreateGATTConnectionError(
     int thread_id,
     int request_id,
     const std::string& device_id,
     base::TimeTicks start_time,
     device::BluetoothDevice::ConnectErrorCode error_code) {
@@ skipping 187 matching lines @@
 
 bool BluetoothDispatcherHost::CanFrameAccessCharacteristicInstance(
     int frame_routing_id,
     const std::string& characteristic_instance_id) {
   return QueryCacheForCharacteristic(GetOrigin(frame_routing_id),
                                      characteristic_instance_id)
              .outcome != CacheQueryOutcome::BAD_RENDERER;
 }
 
 }  // namespace content