New issue
Advanced search Search tips

Issue 610343 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature

Blocking:
issue 508771



Sign in to add a comment

bluetooth: Refactor BluetoothAllowedDevicesMap to accept only one origin.

Project Member Reported by ortuno@chromium.org, May 9 2016

Issue description

There can only be one BluetoothAllowedDevicesMap per WebBluetoothService and we clean the map when navigating so the map will ever only deal with a single origin. We can get rid of some of the mapping and logic because of this.

 

Comment 1 by scheib@chromium.org, Jan 11 2017

Owner: juncai@chromium.org
Status: Assigned (was: Available)
As of today there are multiple BluetoothAllowedDevicesMap objects created for multiple frames:
. Renderer has a document with 3 same origin iframes:
. Each iframe calls navigator.bluetooth.requestDevice()
. Each iframe will construct a navigator.bluetooth blink::Bluetooth object
. Each blink::Bluetooth object will call 
  interfaceProvider->getInterface(mojo::MakeRequest(&m_service));
  from within Bluetooth::requestDevice
. Each request will cause RenderFrameHostImpl::CreateWebBluetoothService
  to be called, which will construct a new WebBluetoothServiceImpl object.
. Each service will create a BluetoothAllowedDevicesMap.

This isn't good, because multiple maps won't share knowledge of what is allowed between multiple frames.

It's debatable if there should be a 1:1 or a n:1 mapping of:
WebBluetoothServiceImpl :: origin in a profile.

In discussion, juncai and I are favoring n:1, as it is a bit more flexible in being able to have unique state per binding if ever necessary.

Both options need a new KeyedService & factory to provide common state that is tied to the single origin.

E.g. BluetoothAllowedDevicesMap will have a 1:1 mapping to to the origin in a profile. The keyed service can find or create one BluetoothAllowedDevicesMap object when the WebBluetoothServiceImpl instances are being created in RenderFrameHostImpl.

Then, the origin concept can be removed from BluetoothAllowedDevicesMap's implementation.

Comment 2 by juncai@chromium.org, Jan 23 2017

Status: Started (was: Assigned)

Comment 3 by ortuno@chromium.org, Jan 23 2017

Why not just move over to permissions like WebUSB?

Comment 4 by ortuno@chromium.org, Jan 23 2017

i.e. store Web Bluetooth permissions in WebSite settings.

Comment 5 by juncai@chromium.org, Jan 23 2017

I guess that will eventually be done, it seems there are two issues filed that are related to this:
issue 589228
issue 638721

Comment 6 by ortuno@chromium.org, Jan 23 2017

Right. So why should we refactor this instead of just implementing what's on Issue 589228?
Labels: DeviceService
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f70c51172084e2166d9e56acb3e13ce562a1cc45

commit f70c51172084e2166d9e56acb3e13ce562a1cc45
Author: juncai <juncai@chromium.org>
Date: Fri Feb 10 23:49:17 2017

Refactor BluetoothAllowedDevicesMap

This CL makes each browser profile have a BluetoothAllowedDevicesMap. And
in the map it has 1:1 mapping from origin to BluetoothAllowedDevices.

BUG= 610343 

Review-Url: https://codereview.chromium.org/2658473002
Cr-Commit-Position: refs/heads/master@{#449793}

[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/BUILD.gn
[add] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/bluetooth/bluetooth_allowed_devices.cc
[add] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/bluetooth/bluetooth_allowed_devices.h
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/bluetooth/bluetooth_allowed_devices_map.cc
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/bluetooth/bluetooth_allowed_devices_map.h
[delete] https://crrev.com/31c92fe8317757707c7c910c7d67bc04fb6938be/content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
[add] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/bluetooth/web_bluetooth_service_impl.cc
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/bluetooth/web_bluetooth_service_impl.h
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/browser/storage_partition_impl.h
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/public/browser/storage_partition.h
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/shell/browser/layout_test/blink_test_controller.cc
[modify] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/content/test/BUILD.gn
[add] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/third_party/WebKit/LayoutTests/bluetooth/server/getPrimaryService/two-iframes-from-same-origin.html
[add] https://crrev.com/f70c51172084e2166d9e56acb3e13ce562a1cc45/third_party/WebKit/LayoutTests/resources/bluetooth/heart-rate-two-iframes.html

Comment 9 by juncai@chromium.org, Feb 10 2017

Status: Fixed (was: Started)
Components: Internals>Services>Device

Sign in to add a comment