Refactor BluetoothAllowedDevicesMap

content/public/browser/bluetooth_allowed_devices_map_base.cc

Index: content/public/browser/bluetooth_allowed_devices_map_base.cc
diff --git a/content/public/browser/bluetooth_allowed_devices_map_base.cc b/content/public/browser/bluetooth_allowed_devices_map_base.cc
new file mode 100644
index 0000000000000000000000000000000000000000..03ed82a1f462cc9598a411678695d7b195398ba8
--- /dev/null
+++ b/content/public/browser/bluetooth_allowed_devices_map_base.cc
@@ -0,0 +1,30 @@
+// Copyright 2017 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.
+
+#include "content/public/browser/bluetooth_allowed_devices_map_base.h"
+
+#include <utility>
+
+#include "content/browser/bluetooth/bluetooth_allowed_devices.h"
+
+namespace content {
+
+BluetoothAllowedDevicesMapBase::BluetoothAllowedDevicesMapBase() {}
+
+BluetoothAllowedDevicesMapBase::~BluetoothAllowedDevicesMapBase() {}
+
+content::BluetoothAllowedDevices*
+BluetoothAllowedDevicesMapBase::GetOrCreateAllowedDevices(url::Origin origin) {

[jam, 2017/02/01 01:18:24]

See https://www.chromium.org/developers/content-module/content-api for
guidelines on content/public. Namely, they should be structs or interfaces.

Also why is this a new separate interface, instead of just one method on
WebContentsDelegate?

[juncai, 2017/02/01 05:19:27]

Here are some context:
This CL makes each browser profile have one bluetooth allowed devices map, and
in the map it has a 1:1 mapping of origin to BluetoothAllowedDevices. So it
needs to be a KeyedService and needs to be implemented on chrome side. And the
map is created by BluetoothAllowedDevicesMapFactory. Though
BluetoothAllowedDevices is implemented at //content/browser/bluetooth, and the
chrome side code can not use it, so it has a BluetoothAllowedDevicesMapBase
class at //content/public/browser and it actually contains the mapping of origin
to BluetoothAllowedDevices. And on chrome side, the BluetoothAllowedDevicesMap
just needs to be a subclass of both KeyedService and
BluetoothAllowedDevicesMapBase. I am not sure if it can be only a method of
WebContentsDelegate. I know it is a workaround, so any suggestions to make this
design better are welcome. Thanks a lot!

[jam, 2017/02/02 06:13:27]

This doesn't appear to be used outside of content right? If the intent is to
make this per-profile, this can be done inside content by hanging this class off
StoragePartitionImpl. See
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h...
as an example.

[juncai, 2017/02/03 04:47:55]

Thanks for the suggestion!

Done.

+  // "Unique" Origins generate the same key in maps, therefore are not
+  // supported.
+  CHECK(!origin.unique());

[dcheng, 2017/02/02 05:26:03]

Why is this a CHECK()? This appears to be directly reachable via a mojo
interface (WebBluetoothService::RemoteServerGetPrimaryServices), and isn't
explicitly checked for. If a renderer makes this mojo IPC call, won't it kill
the browser?

[juncai, 2017/02/03 04:47:55]

When the WebBluetooth requestDevice() function is called, the code:
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_devi...
already checks if it is a unique origin. If so, it will display some error
message. And the check happens before
BluetoothAllowedDevicesMap::GetOrCreateAllowedDevices() is called.

+  auto iter = origin_to_allowed_devices_map_.find(origin);
+  if (iter == origin_to_allowed_devices_map_.end()) {
+    iter = origin_to_allowed_devices_map_.insert(
+        iter, std::make_pair(origin, content::BluetoothAllowedDevices()));
+  }
+  return &(iter->second);
+}
+
+}  //  namespace content