Refactor BluetoothAllowedDevicesMap

content/browser/bluetooth/web_bluetooth_service_impl.cc

Index: content/browser/bluetooth/web_bluetooth_service_impl.cc
diff --git a/content/browser/bluetooth/web_bluetooth_service_impl.cc b/content/browser/bluetooth/web_bluetooth_service_impl.cc
index be6853b1fd7cd9adee5a7648d7a385c54499a973..f7d2c89d4c6660e804ca5653c042d089b484b9fb 100644
--- a/content/browser/bluetooth/web_bluetooth_service_impl.cc
+++ b/content/browser/bluetooth/web_bluetooth_service_impl.cc
@@ -19,7 +19,9 @@
 #include "content/browser/bluetooth/bluetooth_metrics.h"
 #include "content/browser/bluetooth/frame_connected_bluetooth_devices.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/browser/storage_partition_impl.h"
 #include "content/common/bluetooth/web_bluetooth_device_id.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
@@ -190,8 +192,7 @@ void WebBluetoothServiceImpl::SetClientConnectionErrorHandler(
 
 bool WebBluetoothServiceImpl::IsDevicePaired(
     const std::string& device_address) {
-  return allowed_devices_map_.GetDeviceId(GetOrigin(), device_address) !=
-         nullptr;
+  return allowed_devices()->GetDeviceId(device_address) != nullptr;
 }
 
 void WebBluetoothServiceImpl::DidFinishNavigation(
@@ -389,8 +390,7 @@ void WebBluetoothServiceImpl::RemoteServerGetPrimaryServices(
           : UMAWebBluetoothFunction::GET_PRIMARY_SERVICES);
   RecordGetPrimaryServicesServices(quantity, services_uuid);
 
-  if (!allowed_devices_map_.IsOriginAllowedToAccessAtLeastOneService(
-          GetOrigin(), device_id)) {
+  if (!allowed_devices()->IsAllowedToAccessAtLeastOneService(device_id)) {
     callback.Run(
         blink::mojom::WebBluetoothResult::NOT_ALLOWED_TO_ACCESS_ANY_SERVICE,
         base::nullopt /* service */);
@@ -398,8 +398,8 @@ void WebBluetoothServiceImpl::RemoteServerGetPrimaryServices(
   }
 
   if (services_uuid &&
-      !allowed_devices_map_.IsOriginAllowedToAccessService(
-          GetOrigin(), device_id, services_uuid.value())) {
+      !allowed_devices()->IsAllowedToAccessService(device_id,
+                                                   services_uuid.value())) {
     callback.Run(
         blink::mojom::WebBluetoothResult::NOT_ALLOWED_TO_ACCESS_SERVICE,
         base::nullopt /* service */);
@@ -855,8 +855,8 @@ void WebBluetoothServiceImpl::RemoteServerGetPrimaryServicesImpl(
 
   std::vector<blink::mojom::WebBluetoothRemoteGATTServicePtr> response_services;
   for (device::BluetoothRemoteGattService* service : services) {
-    if (!allowed_devices_map_.IsOriginAllowedToAccessService(
-            GetOrigin(), device_id, service->GetUUID())) {
+    if (!allowed_devices()->IsAllowedToAccessService(device_id,
+                                                     service->GetUUID())) {
       continue;
     }
     std::string service_instance_id = service->GetIdentifier();
@@ -913,14 +913,14 @@ void WebBluetoothServiceImpl::OnGetDeviceSuccess(
     return;
   }
 
-  const WebBluetoothDeviceId device_id_for_origin =
-      allowed_devices_map_.AddDevice(GetOrigin(), device_address, options);
+  const WebBluetoothDeviceId device_id =
+      allowed_devices()->AddDevice(device_address, options);
 
   DVLOG(1) << "Device: " << device->GetNameForDisplay();
 
   blink::mojom::WebBluetoothDevicePtr device_ptr =
       blink::mojom::WebBluetoothDevice::New();
-  device_ptr->id = device_id_for_origin;
+  device_ptr->id = device_id;
   device_ptr->name = device->GetName();
 
   RecordRequestDeviceOutcome(UMARequestDeviceOutcome::SUCCESS);
@@ -1055,7 +1055,7 @@ void WebBluetoothServiceImpl::OnDescriptorWriteValueFailed(
 CacheQueryResult WebBluetoothServiceImpl::QueryCacheForDevice(
     const WebBluetoothDeviceId& device_id) {
   const std::string& device_address =
-      allowed_devices_map_.GetDeviceAddress(GetOrigin(), device_id);
+      allowed_devices()->GetDeviceAddress(device_id);
   if (device_address.empty()) {
     CrashRendererAndClosePipe(bad_message::BDH_DEVICE_NOT_ALLOWED_FOR_ORIGIN);
     return CacheQueryResult(CacheQueryOutcome::BAD_RENDERER);
@@ -1084,7 +1084,7 @@ CacheQueryResult WebBluetoothServiceImpl::QueryCacheForService(
   }
 
   const WebBluetoothDeviceId* device_id =
-      allowed_devices_map_.GetDeviceId(GetOrigin(), device_iter->second);
+      allowed_devices()->GetDeviceId(device_iter->second);
   // Kill the renderer if origin is not allowed to access the device.
   if (device_id == nullptr) {
     CrashRendererAndClosePipe(bad_message::BDH_DEVICE_NOT_ALLOWED_FOR_ORIGIN);
@@ -1099,8 +1099,8 @@ CacheQueryResult WebBluetoothServiceImpl::QueryCacheForService(
   result.service = result.device->GetGattService(service_instance_id);
   if (result.service == nullptr) {
     result.outcome = CacheQueryOutcome::NO_SERVICE;
-  } else if (!allowed_devices_map_.IsOriginAllowedToAccessService(
-                 GetOrigin(), *device_id, result.service->GetUUID())) {
+  } else if (!allowed_devices()->IsAllowedToAccessService(
+                 *device_id, result.service->GetUUID())) {
     CrashRendererAndClosePipe(bad_message::BDH_SERVICE_NOT_ALLOWED_FOR_ORIGIN);
     return CacheQueryResult(CacheQueryOutcome::BAD_RENDERER);
   }
@@ -1180,6 +1180,20 @@ url::Origin WebBluetoothServiceImpl::GetOrigin() {
   return render_frame_host_->GetLastCommittedOrigin();
 }
 
+BluetoothAllowedDevices* WebBluetoothServiceImpl::allowed_devices() {
+  if (!allowed_devices_) {
+    StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(

[scheib, 2017/02/03 23:28:13]

I suppose check with jam@, but whenever I see a static_cast I think something is
broken. Why isn't the GetBluetoothAllowedDevicesMap method on the
StoragePartition interface?

[juncai, 2017/02/08 06:43:49]

Hi, jam@, the three functions:
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h...
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h...
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h...
are not part of the StoragePartition interface, just wondering why. Since
GetBluetoothAllowedDevicesMap uses the similar pattern as the above three
functions, and here uses static_cast, similar to:
https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/servic...

[jam1, 2017/02/10 05:24:39]

the public interface is for getters that are exposed outside content.

for getters that are only used inside content, we cast to the impl since it's
the only implementation.

[scheib, 2017/02/10 06:33:59]

That's a bit unfortunate, as it's not clear that this is the only impl possible
without global knowledge of the code base. (BTW, there is a test subclass as
well).

Perhaps we can at least add a comment at StoragePartition that it's safe for
other content code to static_cast it.

Or, do something such as:
"""
// In public headers
class Impl;
class Interface {
    public:
        virtual Impl* GetImpl() = 0;
};

// In implementation only headers
class Impl : public Interface {
    public:
        Impl* GetImpl() override { return this; }

        std::string ImplOnlyMethod() { return "ImplOnlyMethod\n"; }
};

// Elsewhere in implementation
{
    Interface* i = new Impl();
    // ...
    std::cout << i->GetImpl()->ImplOnlyMethod();
}
"""

Test subclasses would return null.
Pro: the pattern is very clear that internals are available to anyone who has
access to the Impl class definition. 
Con: A virtual dispatch.

[juncai, 2017/02/10 22:30:43]

What about adding some comments about this in a separate CL? If so, I can start
a CL that does that.

Or if we decide to implement them using the approach that scheib@ mentioned, I
can also help with that, probably also in a separate CL.

What do you think?

[scheib, 2017/02/10 23:31:21]

Separate patch is OK, need to float to jam@ for thoughts, but using the existing
pattern is fine.

+        BrowserContext::GetDefaultStoragePartition(
+            web_contents()->GetBrowserContext()));
+    scoped_refptr<BluetoothAllowedDevicesMap> allowed_devices_map =
+        partition->GetBluetoothAllowedDevicesMap();
+    allowed_devices_ =
+        allowed_devices_map->GetOrCreateAllowedDevices(GetOrigin());
+    DCHECK(allowed_devices_);
+  }
+  return allowed_devices_;

[ortuno, 2017/02/03 20:16:50]

Can this be null? If so we need to DCHECK that it isn't before using it. If not
then just return a reference.

[juncai, 2017/02/08 06:43:49]

It can't be null. Changed it to return a reference.

Done.

+}
+
 void WebBluetoothServiceImpl::ClearState() {
   characteristic_id_to_notify_session_.clear();
   pending_primary_services_requests_.clear();
@@ -1188,7 +1202,6 @@ void WebBluetoothServiceImpl::ClearState() {
   service_id_to_device_address_.clear();
   connected_devices_.reset(
       new FrameConnectedBluetoothDevices(render_frame_host_));
-  allowed_devices_map_ = BluetoothAllowedDevicesMap();
   device_chooser_controller_.reset();
   BluetoothAdapterFactoryWrapper::Get().ReleaseAdapter(this);
 }