New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 682050 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocking:
issue 597053



Sign in to add a comment

Enforce 1:1 between WebBluetoothService & Client

Project Member Reported by scheib@chromium.org, Jan 18 2017

Issue description

WebBluetoothServiceClient is only used in interface
WebBluetoothService's SetClient which comments that
"Sets the client for this WebBluetoothService." 

It is always 1:1. dcheng requested in https://codereview.chromium.org/2635473004/
that we explore making sure this invariant always holds.

The current way to do that is using a factory interface,
though Mojo may explore alternatives if there's enough demand

See https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/j-kiK17V-Qc


 

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

Components: Blink>Bluetooth

Comment 2 by juncai@chromium.org, Feb 11 2017

Cc: juncai@chromium.org

Comment 3 by juncai@chromium.org, Feb 13 2017

Cc: -juncai@chromium.org
Owner: juncai@chromium.org
Status: Assigned (was: Available)

Comment 4 by juncai@chromium.org, Feb 14 2017

Status: Started (was: Assigned)

Comment 5 by ortuno@chromium.org, Feb 15 2017

I don't mind the Factory pattern but I wonder if we are really going to keep the current form for our client interface. More specifically, would more specialized clients make more sense?

We currently hold two maps in Bluetooth: m_connectedDevices and m_activeCharacteristics. When we get a response for connection/startNotifications, we add the device/characteristic to the respective map. When we receive an event on the client interface we have to first look for the device/characteristic in the map and then dispatch the event to that device/characteristic.

This seems unnecessary. We could instead return a pipe in the connect and startNotifications callbacks and have the BluetoothDevice and BluetoothRemoteGATTCharacteristic hold the references to those pipes. Then the events would be dispatched to the respective objects directly and we would avoid that lookup.

Comment 6 by ortuno@chromium.org, Feb 15 2017

Cc: scheib@chromium.org

Comment 7 by juncai@chromium.org, Feb 15 2017

Does that mean the WebBluetoothService mojom interface can be multiple interfaces instead of just one giant interface?

Comment 8 by ortuno@chromium.org, Feb 15 2017

I don't think we want to split WebBluetooth just yet. I can't think of any benefits.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 23 2017

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

commit 5fbf7e618bca38528d65a5f673b030fb0b2cd684
Author: juncai <juncai@chromium.org>
Date: Thu Mar 23 21:21:56 2017

Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

This CL changes the WebBluetoothServiceClient in the web_bluetooth.mojom
into two clients:
WebBluetoothCharacteristicClient
WebBluetoothServerClient

BUG= 682050 

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

[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/content/browser/bluetooth/frame_connected_bluetooth_devices.cc
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/content/browser/bluetooth/frame_connected_bluetooth_devices.h
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/content/browser/bluetooth/web_bluetooth_service_impl.cc
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/content/browser/bluetooth/web_bluetooth_service_impl.h
[add] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/concurrent-starts-and-receive-notification.html
[add] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/LayoutTests/bluetooth/server/connect/connect-twice.html
[add] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/LayoutTests/bluetooth/server/connect/same-gatt-server-both-receive-disconnect-event.html
[add] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/LayoutTests/bluetooth/server/disconnect/disconnect-after-request-disconnection.html
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/Source/modules/bluetooth/Bluetooth.h
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.h
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.h
[modify] https://crrev.com/5fbf7e618bca38528d65a5f673b030fb0b2cd684/third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom

Status: Fixed (was: Started)

Sign in to add a comment