Issue 2718583002: Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-02-23 23:21:19 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/1

3 years, 10 months ago (2017-02-23 23:22:11 UTC) #2

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-23 23:47:41 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/315720)

3 years, 10 months ago (2017-02-23 23:47:41 UTC) #4

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-02-23 23:52:59 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/20001

3 years, 10 months ago (2017-02-23 23:53:55 UTC) #6

juncai

juncai@chromium.org changed reviewers: + ortuno@chromium.org

3 years, 10 months ago (2017-02-23 23:54:11 UTC) #7

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-24 01:08:53 UTC) #9

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/396774)

3 years, 10 months ago (2017-02-24 01:08:54 UTC) #10

ortuno

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right): https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc#newcode40 content/browser/bluetooth/frame_connected_bluetooth_devices.cc:40: if (device_id_to_connection_map_.find(device_id) != This is another place where we ...

3 years, 10 months ago (2017-02-24 03:28:52 UTC) #11

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:40: if
(device_id_to_connection_map_.find(device_id) !=
This is another place where we are missing tests. We should add a test for:

device.gatt.connect();
device.gatt.connect();

Right now that causes OnCreateGATTConnectionSuccess to call Insert twice and
then send the request for that pointer. But because Insert ignores the second
value the pointer gets destroyed.

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:333:
callback.Run(query_result.GetWebResult(), nullptr);
nit: comment what nullptr is. Same below.

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:956: std::move(client));
Would it make sense to remove the RemoteGATTServerDisconnect method and just use
error handlers?

// web_bluetooth_service_impl.cc

// TODO: Maybe we could use a better name.
void WebBluetoothServiceImpl::HandlerServerClientError(const
WebBluetoothDeviceId& id) {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  RecordWebBluetoothFunctionCall(
      UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT);

  if (connected_devices_->IsConnectedToDeviceWithId(device_id)) {
    DVLOG(1) << "Disconnecting device: " << device_id.str();
    connected_devices_->CloseConnectionToDeviceWithId(device_id);
  }
}


void WebBluetoothServiceImpl::OnCreateGATTConnectionSuccess(...) {
  // ...
  blink::mojom::WebBluetoothServerClientAssociatedPtr client;
  client.set_connection_error_handler(base::Bind(
    &WebBluetoothServiceImpl::HandlerServerClientError,
    weak_ptr_factory_.GetWeakPtr(),
    device_id));
}

// Then in frame_connected_bluetooth_devices.cc we can just close the pipe.

FrameConnectedBluetoothDevices::CloseConnectionToDeviceWithAddress(
    const std::string& device_address) {
  // ...
  CHECK(device_address_to_id_map_.erase(device_address));
  CHECK(device_id_to_connection_map_.erase(device_id));
}

// And in BluetoothRemoteGATTServer.cpp

void BluetoothRemoteGATTServer::disconnect(ScriptState* scriptState) {
  // ...
  if (m_clientBinding.is_bound())
    m_clientBinding.Close();
}

void BluetoothRemoteGATTServer::HandleClientConnectionError() {
  m_device->dispatchGattServerDisconnected();
}

void BluetoothRemoteGATTServer::ConnectCallback(...) {

  // Bind binding and set error handler to handleClientConnectionError();
}

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:86: void
BluetoothDevice::disconnectGATTIfConnected() {
Would it make sense to move disconnectGATTIfConnected,
cleanupDisconnectedDeviceAndFireEvent and dispatchGattServerDisconnected to the
server?

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:66:
// The pipe to this object must be closed when is marked unreachable to
notifyCharacteristicObjectRemoved is used so that when the context gets
destroyed e.g. when reloading the page or when the object is garbage collected
we stop notifications. If you only close the pipe when the object is garbage
collected i.e. dispose is called then the characteristic will still be
subscribed to notifications if you reload a page and GC hasn't run yet.

You should move this code to notifyCharacteristicObjectRemoved(). The binding
being bound will work as the m_stopped so you don't need it anymore.

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:57:
void BluetoothRemoteGATTServer::dispose() {
Similar to BluetoothRemoteGATTCharacteristic, we want the device to disconnect
as soon as you navigate away. dispose only gets called when the object gets
garbage collected which could happen many seconds after we've navigated away.
For that reason we should also implement contextDestroyed.

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:77:
m_clientBinding.Bind(std::move(request));
Hmm seems we are missing tests. We need a test to make sure that the following
works:

device.gatt.connect().then(gatt => gatt.connect());


Right now that would result in the second callback getting called with a null
request. And the client would no longer receive a disconnection event. I would
first write the test and then implement the fix that way you know that the test
is working correctly.

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:223:
associated WebBluetoothCharacteristicClient&? client_request);
As you mentioned now that we no longer have a global client we are missing
events for read requests. We could solve this by simply firing an event on the
readValue callback. But we would introduce a bug in which if you subscribed to
notifications and then called readValue you would get notified twice. I think
there are two ways to solve this:

1. Add logic to web_bluetooth_service_impl to not notify if there are pending
reads.
2. Change GattCharacteristicValueChanged to only get called for notifications.

I think option 2 is the way to go: For Android[1], macOS[2] and Windows[3] it's
only removing a line. For BlueZ there is a bit more work to do since we would
have to keep track of pending requests and run callbacks when the value changes.
Similar to what we do on macOS[4]

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
[2]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
[3]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
[4]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-01 00:07:47 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/40001

3 years, 9 months ago (2017-03-01 00:08:47 UTC) #13

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-01 01:22:31 UTC) #14

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/241465)

3 years, 9 months ago (2017-03-01 01:22:32 UTC) #15

juncai

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right): https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc#newcode40 content/browser/bluetooth/frame_connected_bluetooth_devices.cc:40: if (device_id_to_connection_map_.find(device_id) != On 2017/02/24 03:28:52, ortuno wrote: > ...

3 years, 9 months ago (2017-03-01 02:04:12 UTC) #16

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:40: if
(device_id_to_connection_map_.find(device_id) !=
On 2017/02/24 03:28:52, ortuno wrote:
> This is another place where we are missing tests. We should add a test for:
> 
> device.gatt.connect();
> device.gatt.connect();
> 
> Right now that causes OnCreateGATTConnectionSuccess to call Insert twice and
> then send the request for that pointer. But because Insert ignores the second
> value the pointer gets destroyed.

Done.

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:333:
callback.Run(query_result.GetWebResult(), nullptr);
On 2017/02/24 03:28:52, ortuno wrote:
> nit: comment what nullptr is. Same below.

Done.

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:956: std::move(client));
On 2017/02/24 03:28:52, ortuno wrote:
> Would it make sense to remove the RemoteGATTServerDisconnect method and just
use
> error handlers?
> 
> // web_bluetooth_service_impl.cc
> 
> // TODO: Maybe we could use a better name.
> void WebBluetoothServiceImpl::HandlerServerClientError(const
> WebBluetoothDeviceId& id) {
>   DCHECK_CURRENTLY_ON(BrowserThread::UI);
>   RecordWebBluetoothFunctionCall(
>       UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT);
> 
>   if (connected_devices_->IsConnectedToDeviceWithId(device_id)) {
>     DVLOG(1) << "Disconnecting device: " << device_id.str();
>     connected_devices_->CloseConnectionToDeviceWithId(device_id);
>   }
> }
> 
> 
> void WebBluetoothServiceImpl::OnCreateGATTConnectionSuccess(...) {
>   // ...
>   blink::mojom::WebBluetoothServerClientAssociatedPtr client;
>   client.set_connection_error_handler(base::Bind(
>     &WebBluetoothServiceImpl::HandlerServerClientError,
>     weak_ptr_factory_.GetWeakPtr(),
>     device_id));
> }
> 
> // Then in frame_connected_bluetooth_devices.cc we can just close the pipe.
> 
> FrameConnectedBluetoothDevices::CloseConnectionToDeviceWithAddress(
>     const std::string& device_address) {
>   // ...
>   CHECK(device_address_to_id_map_.erase(device_address));
>   CHECK(device_id_to_connection_map_.erase(device_id));
> }
> 
> // And in BluetoothRemoteGATTServer.cpp
> 
> void BluetoothRemoteGATTServer::disconnect(ScriptState* scriptState) {
>   // ...
>   if (m_clientBinding.is_bound())
>     m_clientBinding.Close();
> }
> 
> void BluetoothRemoteGATTServer::HandleClientConnectionError() {
>   m_device->dispatchGattServerDisconnected();
> }
> 
> void BluetoothRemoteGATTServer::ConnectCallback(...) {
> 
>   // Bind binding and set error handler to handleClientConnectionError();
> }

Making sense, since it also needs to change the web_bluetooth.mojom, how about
doing it in a follow-up CL?

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:86: void
BluetoothDevice::disconnectGATTIfConnected() {
On 2017/02/24 03:28:52, ortuno wrote:
> Would it make sense to move disconnectGATTIfConnected,
> cleanupDisconnectedDeviceAndFireEvent and dispatchGattServerDisconnected to
the
> server?

Done.

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:66:
// The pipe to this object must be closed when is marked unreachable to
On 2017/02/24 03:28:52, ortuno wrote:
> notifyCharacteristicObjectRemoved is used so that when the context gets
> destroyed e.g. when reloading the page or when the object is garbage collected
> we stop notifications. If you only close the pipe when the object is garbage
> collected i.e. dispose is called then the characteristic will still be
> subscribed to notifications if you reload a page and GC hasn't run yet.
> 
> You should move this code to notifyCharacteristicObjectRemoved(). The binding
> being bound will work as the m_stopped so you don't need it anymore.

Done.

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:57:
void BluetoothRemoteGATTServer::dispose() {
On 2017/02/24 03:28:52, ortuno wrote:
> Similar to BluetoothRemoteGATTCharacteristic, we want the device to disconnect
> as soon as you navigate away. dispose only gets called when the object gets
> garbage collected which could happen many seconds after we've navigated away.
> For that reason we should also implement contextDestroyed.

Done.

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:77:
m_clientBinding.Bind(std::move(request));
On 2017/02/24 03:28:52, ortuno wrote:
> Hmm seems we are missing tests. We need a test to make sure that the following
> works:
> 
> device.gatt.connect().then(gatt => gatt.connect());
> 
> 
> Right now that would result in the second callback getting called with a null
> request. And the client would no longer receive a disconnection event. I would
> first write the test and then implement the fix that way you know that the
test
> is working correctly.

Done.

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:223:
associated WebBluetoothCharacteristicClient&? client_request);
On 2017/02/24 03:28:52, ortuno wrote:
> As you mentioned now that we no longer have a global client we are missing
> events for read requests. We could solve this by simply firing an event on the
> readValue callback. But we would introduce a bug in which if you subscribed to
> notifications and then called readValue you would get notified twice. I think
> there are two ways to solve this:
> 
> 1. Add logic to web_bluetooth_service_impl to not notify if there are pending
> reads.
> 2. Change GattCharacteristicValueChanged to only get called for notifications.
> 
> I think option 2 is the way to go: For Android[1], macOS[2] and Windows[3]
it's
> only removing a line. For BlueZ there is a bit more work to do since we would
> have to keep track of pending requests and run callbacks when the value
changes.
> Similar to what we do on macOS[4]
> 
> [1]
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> [2]
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> [3]
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> [4]
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...

I will do this in a separate CL.

ortuno

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right): https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc#newcode81 content/browser/bluetooth/frame_connected_bluetooth_devices.cc:81: if (device_id_iter->second.second) { When would a device have a ...

3 years, 9 months ago (2017-03-01 04:52:06 UTC) #17

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-02 02:08:12 UTC) #18

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/60001

3 years, 9 months ago (2017-03-02 02:09:41 UTC) #19

juncai

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetooth/web_bluetooth_service_impl.cc File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetooth/web_bluetooth_service_impl.cc#newcode956 content/browser/bluetooth/web_bluetooth_service_impl.cc:956: std::move(client)); On 2017/03/01 02:04:12, juncai wrote: > On 2017/02/24 ...

3 years, 9 months ago (2017-03-02 03:23:50 UTC) #20

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2718583002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:956: std::move(client));
On 2017/03/01 02:04:12, juncai wrote:
> On 2017/02/24 03:28:52, ortuno wrote:
> > Would it make sense to remove the RemoteGATTServerDisconnect method and just
> use
> > error handlers?
> > 
> > // web_bluetooth_service_impl.cc
> > 
> > // TODO: Maybe we could use a better name.
> > void WebBluetoothServiceImpl::HandlerServerClientError(const
> > WebBluetoothDeviceId& id) {
> >   DCHECK_CURRENTLY_ON(BrowserThread::UI);
> >   RecordWebBluetoothFunctionCall(
> >       UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT);
> > 
> >   if (connected_devices_->IsConnectedToDeviceWithId(device_id)) {
> >     DVLOG(1) << "Disconnecting device: " << device_id.str();
> >     connected_devices_->CloseConnectionToDeviceWithId(device_id);
> >   }
> > }
> > 
> > 
> > void WebBluetoothServiceImpl::OnCreateGATTConnectionSuccess(...) {
> >   // ...
> >   blink::mojom::WebBluetoothServerClientAssociatedPtr client;
> >   client.set_connection_error_handler(base::Bind(
> >     &WebBluetoothServiceImpl::HandlerServerClientError,
> >     weak_ptr_factory_.GetWeakPtr(),
> >     device_id));
> > }
> > 
> > // Then in frame_connected_bluetooth_devices.cc we can just close the pipe.
> > 
> > FrameConnectedBluetoothDevices::CloseConnectionToDeviceWithAddress(
> >     const std::string& device_address) {
> >   // ...
> >   CHECK(device_address_to_id_map_.erase(device_address));
> >   CHECK(device_id_to_connection_map_.erase(device_id));
> > }
> > 
> > // And in BluetoothRemoteGATTServer.cpp
> > 
> > void BluetoothRemoteGATTServer::disconnect(ScriptState* scriptState) {
> >   // ...
> >   if (m_clientBinding.is_bound())
> >     m_clientBinding.Close();
> > }
> > 
> > void BluetoothRemoteGATTServer::HandleClientConnectionError() {
> >   m_device->dispatchGattServerDisconnected();
> > }
> > 
> > void BluetoothRemoteGATTServer::ConnectCallback(...) {
> > 
> >   // Bind binding and set error handler to handleClientConnectionError();
> > }
> 
> Making sense, since it also needs to change the web_bluetooth.mojom, how about
> doing it in a follow-up CL?

I opened an issue for that:
https://bugs.chromium.org/p/chromium/issues/detail?id=697700

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2718583002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:223:
associated WebBluetoothCharacteristicClient&? client_request);
On 2017/03/01 02:04:12, juncai wrote:
> On 2017/02/24 03:28:52, ortuno wrote:
> > As you mentioned now that we no longer have a global client we are missing
> > events for read requests. We could solve this by simply firing an event on
the
> > readValue callback. But we would introduce a bug in which if you subscribed
to
> > notifications and then called readValue you would get notified twice. I
think
> > there are two ways to solve this:
> > 
> > 1. Add logic to web_bluetooth_service_impl to not notify if there are
pending
> > reads.
> > 2. Change GattCharacteristicValueChanged to only get called for
notifications.
> > 
> > I think option 2 is the way to go: For Android[1], macOS[2] and Windows[3]
> it's
> > only removing a line. For BlueZ there is a bit more work to do since we
would
> > have to keep track of pending requests and run callbacks when the value
> changes.
> > Similar to what we do on macOS[4]
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> > [2]
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> > [3]
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> > [4]
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> 
> I will do this in a separate CL. 

I opened an issue at:
https://bugs.chromium.org/p/chromium/issues/detail?id=697702

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:81: if
(device_id_iter->second.second) {
On 2017/03/01 04:52:05, ortuno wrote:
> When would a device have a connection but no client?

In the test code at:
frame_connected_bluetooth_devices_unittest.cc

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc
(right):

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc:125:
map0_->Insert(kDeviceId0, GetConnection(kDeviceAddress0), nullptr);
On 2017/03/01 04:52:05, ortuno wrote:
> nit: please comment nullptr.

Done.

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:335: nullptr /*
blink::mojom::WebBluetoothServerClientAssociatedRequest */);
On 2017/03/01 04:52:05, ortuno wrote:
> optional: you can just use client_request.

Done.

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:962:
blink::mojom::WebBluetoothServerClientAssociatedPtr client;
On 2017/03/01 04:52:05, ortuno wrote:
> Seems a bit wasteful to be sending request when there is a client already :/

Done.

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:1021:
blink::mojom::WebBluetoothCharacteristicClientAssociatedPtr client;
On 2017/03/01 04:52:05, ortuno wrote:
> Seems a bit wasteful to be sending request when there is a client already :/

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/server/connect/connect-twice.html
(right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/connect-twice.html:8:
return setBluetoothFakeAdapter('DisconnectingHeartRateAdapter')
On 2017/03/01 04:52:05, ortuno wrote:
> nit: Can you change this to DisconnectingHealthThermometerAdapter? We are
trying
> to move away from HeartRate.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/server/connect/same-gatt-server-both-receive-disconnect-event.html
(right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/same-gatt-server-both-receive-disconnect-event.html:9:
return setBluetoothFakeAdapter('DisconnectingHeartRateAdapter')
On 2017/03/01 04:52:06, ortuno wrote:
> DisconnectingHealthThermometer please.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/same-gatt-server-both-receive-disconnect-event.html:18:
return Promise.all([get_request_disconnection(gattServers[0]),
get_request_disconnection(gattServers[1])]);
On 2017/03/01 04:52:06, ortuno wrote:
> No need to get two requests or the bubbles check:
> 
> })
> .then([gattServer,] => get_request_disconnection(gattServer))
> .then(requestDisconnection => {
>   let disconnected = eventPromise(device, 'gattserverdisconnected');
>   return Promise.all([requestDisconnection(), disconnected]);
> });

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/same-gatt-server-both-receive-disconnect-event.html:25:
}, 'Same gatt server should both receive disconnect event.');
On 2017/03/01 04:52:05, ortuno wrote:
> A device disconnecting after two consecutive connect requests should fire an
> event.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:78: void
BluetoothDevice::dispose() {
On 2017/03/01 04:52:06, ortuno wrote:
> Let's just do both on BluetoothRemoteGATTServer.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(left):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:93:
if (eventType == EventTypeNames::characteristicvaluechanged) {
On 2017/03/01 04:52:06, ortuno wrote:
> Forgot about this. We should keep an eye out for http://crbug.com/541390 as
that
> would mean we have to tie the client to this. Nothing to do for now.

Thanks!

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:69:
void BluetoothRemoteGATTCharacteristic::notifyCharacteristicObjectRemoved() {
On 2017/03/01 04:52:06, ortuno wrote:
> Can you change the name of this function? It doesn't seem appropriate now that
> we are not notifying anyone that the object was removed.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:110:
dispatchEvent(Event::create(EventTypeNames::characteristicvaluechanged));
On 2017/03/01 04:52:06, ortuno wrote:
> This test
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
> should make sure that the event is fired before the promise but it currently
> just ignores the order. Could you please fix the test to make sure the event
is
> fire before the promise per step 5.5.4 of readValue()
>
https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattcharac...

I opened an issue for it:
https://bugs.chromium.org/p/chromium/issues/detail?id=697698
And will fix it in a follow-up CL.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.h
(right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.h:64:
void RemoteCharacteristicValueChanged(
On 2017/03/01 04:52:06, ortuno wrote:
> nit: Move this above contextDestroyed so that both functions that are supposed
> to be called at the end of the the object's lifetime are closer.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.h:139:
mojo::AssociatedBinding<mojom::blink::WebBluetoothCharacteristicClient>
On 2017/03/01 04:52:06, ortuno wrote:
> nit: move this to the end.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:57:
void BluetoothRemoteGATTServer::disconnectIfConnected() {
On 2017/03/01 04:52:06, ortuno wrote:
> What do you think of the following:
> 
> void disconnect() {
>   ClearActiveAlgorithms();
>   if (!connected()) {
>     return;
>   }
> 
>   cleanupDisconnectedDevice();
>   m_device->dispatchEvent(...);
> 
>   mojom::blink::WebBluetoothService* service =
m_device->bluetooth()->service();
>   service->RemoteServerDisconnect(device()->id());
> 
>   DCHECK(m_clientBinding.is_bound());
>   m_clientBinding.Close();
> }
> 
> void GATTServerDisconnected() {
>   // This function can only be called if m_clientBinding is bound and
> m_clientBinding can
>   // only be bound if connected() is true. Therefore we skip Step 2 of
> Responding to
>   // Disconnection.
> https://webbluetoothcg.github.io/web-bluetooth/#disconnection-events
>   cleanupDisconnectedDevice();
>   m_device->dispatchEvent(...);
> }
> 
> void cleanupDisconnectedDevice() {
>   DCHECK(connected());
>   connected_ = false;
>   m_activeAlgorithms.clear();
>   m_device->clearAttributeInstanceMap();
> }
> 
> 
> void dispose() {
>   disconnectIfConnected();
> }
> 
> void contextDestroyed(...) {
>   disconnectIfConnected();
> }
> 
> void disconnectIfConnected() {
>   if (!connected())
>     return;
> 
>   cleanupDisconnectedDevice();
>   mojom::blink::WebBluetoothService* service =
m_device->bluetooth()->service();
>   service->RemoteServerDisconnect(device()->id());
>   DCHECK(m_clientBinding.is_bound())
>   m_clientBinding.Close();
> }

I think this change can be combined in a follow-up CL that also removes the
RemoteServerDisconnect() from web_bluetooth.mojom that you suggested in a
previous comment.
I opened an issue for that:
https://bugs.chromium.org/p/chromium/issues/detail?id=697700

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:101:
if (!m_connected)
On 2017/03/01 04:52:06, ortuno wrote:
> Why not just send a nullptr for already connected devices?

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:265:
interface WebBluetoothCharacteristicClient {
On 2017/03/01 04:52:06, ortuno wrote:
> Please add comments to both functions.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:270:
GattServerDisconnected();
On 2017/03/01 04:52:06, ortuno wrote:
> nit: GATTServerDisconnected();

Done.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-02 05:43:58 UTC) #21

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/242665)

3 years, 9 months ago (2017-03-02 05:43:59 UTC) #22

ortuno

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right): https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc#newcode81 content/browser/bluetooth/frame_connected_bluetooth_devices.cc:81: if (device_id_iter->second.second) { On 2017/03/02 at 03:23:38, juncai wrote: ...

3 years, 9 months ago (2017-03-06 11:31:20 UTC) #23

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:81: if
(device_id_iter->second.second) {
On 2017/03/02 at 03:23:38, juncai wrote:
> On 2017/03/01 04:52:05, ortuno wrote:
> > When would a device have a connection but no client?
> 
> In the test code at:
> frame_connected_bluetooth_devices_unittest.cc

hmm we should try to avoid polluting prod code because of our testing. Is there
anyway we can use a fake ptr for our tests? I would probably ask ken.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:78: void
BluetoothDevice::dispose() {
On 2017/03/02 at 03:23:47, juncai wrote:
> On 2017/03/01 04:52:06, ortuno wrote:
> > Let's just do both on BluetoothRemoteGATTServer.
> 
> Done.

Also add contextDestroyed to BluetoothRemoteGATTServer. After that we can remove
both of these from BluetoothDevice.

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:72:
FrameConnectedBluetoothDevices::CloseConnectionToDeviceWithAddress(
Maybe for follow up: We returned the device id because we needed it to send the
event but now that we send the event from here we don't need the device id. Two
options:

1. Return the associated ptr and keep sending the event from
web_bluetooth_service_impl
2. Just make this a void. Though I'm slightly sad that we will no longer have a
unittest to make sure the right thing was happening. Maybe we can change our
tests to make sure that an event is sent when this method is called?

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:293:
characteristic_id_to_notify_session_.find(characteristic_instance_id);
You can move all this logic up to GattCharacteristicValueChanged now that reads
don't fire this event.

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:955: if
(connected_devices_->IsConnectedToDeviceWithId(device_id)) {
// It's possible for WebBluetoothServiceImpl to issue two successive
  // connection requests for which it would get two successive responses
  // and consequently try to insert two BluetoothGattConnections for the
  // same device. WebBluetoothServiceImpl should reject or queue connection
  // requests if there is a pending connection already, but the platform
  // abstraction doesn't currently support checking for pending connections.
  // TODO(crbug.com/583544): CHECK that this never happens once the platform
  // abstraction allows to check for pending connections.

I wonder if we should remove this comment from
frame_connected_bluetooth_devices.cc since we already perform this check here.
WDYT?

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:225:
m_clientBinding.Bind(std::move(request));
Wouldn't we bind an invalid request for:

startNotifications();
startNotifications();

? We should probably add a test if there is a bug here.

Also please move inside SUCCESS if statement, so that it's obvious that we only
bind for success.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:102:
if (!m_connected)
Binding the request based on tha value of m_connected forces readers to think of
all the cases in which m_connected could be true and makes the intent harder to
understand i.e. we only bind valid requests. Furthermore m_connected and binding
seem like two unrelated operations which could lead to bugs. Case in point:

gatt.connect().then(() => gatt.disconnect());
gatt.connect();

1. Dispatch two connection requests.
2. First connect response arrives to the browser so
OnCreateGATTConnectionSuccess is called. Since there are no other connections in
the map, the ptr is added to the map, we create request based on the ptr and
sent it to the renderer.
3. Second connect response arrives to the browser so
OnCreateGATTConnectionSuccess is called. Since there is already a connection in
the map we just send nullptr.
4. First connect response arrives on the renderer. We bind the request and set
m_connected to true and resolve the promise.
5. Now that the promise has resolve we call gatt.disconnect() which sets
m_connected to false.
6. Second connect response arrives on the render. Since m_connected is false we
bind the invalid request :(

Change this to:

// WebBluetoothService sends an invalid request and SUCCESS if there is a client
// already.
if (request.is_valid())
  m_clientBinding.Bind(std::move(request));

And also please move inside SUCCESS if statement to make it more obvious that
the request should be bound on success.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:169: //
connection alive.
// Returns the result of the connection request and a client request if the
connection was successful and there was no previous WebBluetoothServerClient.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:222: //
|characteristic_instance_id|.
// Returns the result of the start notifications request and a client request if
the gatt operation was successful and there was no previous
WebBluetoothCharacteristicClient.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:265: //
Classes should implement this interface and will be notified of device
// Classes that implement this interface will be notified of characteristic
events.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:269:
RemoteCharacteristicValueChanged(array<uint8> value);
Called when we receive a notification for the characteristic.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:272: //
Classes should implement this interface and will be notified of device
Classes that implement this interface will be notified of device events.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:275: //
Device has been disconnected.
Called when a device disconnects.

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-09 01:51:47 UTC) #24

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/80001

3 years, 9 months ago (2017-03-09 01:52:18 UTC) #25

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-09 02:57:47 UTC) #26

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/336862)

3 years, 9 months ago (2017-03-09 02:57:48 UTC) #27

juncai

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right): https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetooth/frame_connected_bluetooth_devices.cc#newcode81 content/browser/bluetooth/frame_connected_bluetooth_devices.cc:81: if (device_id_iter->second.second) { On 2017/03/06 11:31:20, ortuno wrote: > ...

3 years, 9 months ago (2017-03-09 07:30:57 UTC) #28

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/2718583002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:81: if
(device_id_iter->second.second) {
On 2017/03/06 11:31:20, ortuno wrote:
> On 2017/03/02 at 03:23:38, juncai wrote:
> > On 2017/03/01 04:52:05, ortuno wrote:
> > > When would a device have a connection but no client?
> > 
> > In the test code at:
> > frame_connected_bluetooth_devices_unittest.cc
> 
> hmm we should try to avoid polluting prod code because of our testing. Is
there
> anyway we can use a fake ptr for our tests? I would probably ask ken.

Done.

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right):

https://codereview.chromium.org/2718583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:78: void
BluetoothDevice::dispose() {
On 2017/03/06 11:31:20, ortuno wrote:
> On 2017/03/02 at 03:23:47, juncai wrote:
> > On 2017/03/01 04:52:06, ortuno wrote:
> > > Let's just do both on BluetoothRemoteGATTServer.
> > 
> > Done.
> 
> Also add contextDestroyed to BluetoothRemoteGATTServer. After that we can
remove
> both of these from BluetoothDevice.

Done.

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:72:
FrameConnectedBluetoothDevices::CloseConnectionToDeviceWithAddress(
On 2017/03/06 11:31:20, ortuno wrote:
> Maybe for follow up: We returned the device id because we needed it to send
the
> event but now that we send the event from here we don't need the device id.
Two
> options:
> 
> 1. Return the associated ptr and keep sending the event from
> web_bluetooth_service_impl
> 2. Just make this a void. Though I'm slightly sad that we will no longer have
a
> unittest to make sure the right thing was happening. Maybe we can change our
> tests to make sure that an event is sent when this method is called?

I will do this in a follow-up CL. I opened an issue for this:
https://bugs.chromium.org/p/chromium/issues/detail?id=699823

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:293:
characteristic_id_to_notify_session_.find(characteristic_instance_id);
On 2017/03/06 11:31:20, ortuno wrote:
> You can move all this logic up to GattCharacteristicValueChanged now that
reads
> don't fire this event.

Done.

https://codereview.chromium.org/2718583002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:955: if
(connected_devices_->IsConnectedToDeviceWithId(device_id)) {
On 2017/03/06 11:31:20, ortuno wrote:
> // It's possible for WebBluetoothServiceImpl to issue two successive
>   // connection requests for which it would get two successive responses
>   // and consequently try to insert two BluetoothGattConnections for the
>   // same device. WebBluetoothServiceImpl should reject or queue connection
>   // requests if there is a pending connection already, but the platform
>   // abstraction doesn't currently support checking for pending connections.
>   // TODO(crbug.com/583544): CHECK that this never happens once the platform
>   // abstraction allows to check for pending connections.
> 
> I wonder if we should remove this comment from
> frame_connected_bluetooth_devices.cc since we already perform this check here.
> WDYT?

Done.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:225:
m_clientBinding.Bind(std::move(request));
On 2017/03/06 11:31:20, ortuno wrote:
> Wouldn't we bind an invalid request for:
> 
> startNotifications();
> startNotifications();
> 
> ? We should probably add a test if there is a bug here.
> 
> Also please move inside SUCCESS if statement, so that it's obvious that we
only
> bind for success.

Done.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:102:
if (!m_connected)
On 2017/03/06 11:31:20, ortuno wrote:
> Binding the request based on tha value of m_connected forces readers to think
of
> all the cases in which m_connected could be true and makes the intent harder
to
> understand i.e. we only bind valid requests. Furthermore m_connected and
binding
> seem like two unrelated operations which could lead to bugs. Case in point:
> 
> gatt.connect().then(() => gatt.disconnect());
> gatt.connect();
> 
> 1. Dispatch two connection requests.
> 2. First connect response arrives to the browser so
> OnCreateGATTConnectionSuccess is called. Since there are no other connections
in
> the map, the ptr is added to the map, we create request based on the ptr and
> sent it to the renderer.
> 3. Second connect response arrives to the browser so
> OnCreateGATTConnectionSuccess is called. Since there is already a connection
in
> the map we just send nullptr.
> 4. First connect response arrives on the renderer. We bind the request and set
> m_connected to true and resolve the promise.
> 5. Now that the promise has resolve we call gatt.disconnect() which sets
> m_connected to false.
> 6. Second connect response arrives on the render. Since m_connected is false
we
> bind the invalid request :(
> 
> Change this to:
> 
> // WebBluetoothService sends an invalid request and SUCCESS if there is a
client
> // already.
> if (request.is_valid())
>   m_clientBinding.Bind(std::move(request));
> 
> And also please move inside SUCCESS if statement to make it more obvious that
> the request should be bound on success.

Done.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:169: //
connection alive.
On 2017/03/06 11:31:20, ortuno wrote:
> // Returns the result of the connection request and a client request if the
> connection was successful and there was no previous WebBluetoothServerClient.

Done.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:222: //
|characteristic_instance_id|.
On 2017/03/06 11:31:20, ortuno wrote:
> // Returns the result of the start notifications request and a client request
if
> the gatt operation was successful and there was no previous
> WebBluetoothCharacteristicClient.

Done.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:265: //
Classes should implement this interface and will be notified of device
On 2017/03/06 11:31:20, ortuno wrote:
> // Classes that implement this interface will be notified of characteristic
> events.

Done.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:269:
RemoteCharacteristicValueChanged(array<uint8> value);
On 2017/03/06 11:31:20, ortuno wrote:
> Called when we receive a notification for the characteristic.

Done.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:272: //
Classes should implement this interface and will be notified of device
On 2017/03/06 11:31:20, ortuno wrote:
> Classes that implement this interface will be notified of device events.

Done.

https://codereview.chromium.org/2718583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:275: //
Device has been disconnected.
On 2017/03/06 11:31:20, ortuno wrote:
> Called when a device disconnects.

Done.

ortuno

lgtm! https://codereview.chromium.org/2718583002/diff/80001/third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h (right): https://codereview.chromium.org/2718583002/diff/80001/third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h#newcode34 third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h:34: public ContextLifecycleObserver { Does BluetoothDevice still need to ...

3 years, 9 months ago (2017-03-09 23:16:38 UTC) #29

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-10 01:58:07 UTC) #30

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/100001

3 years, 9 months ago (2017-03-10 01:58:42 UTC) #31

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-10 03:22:24 UTC) #32

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-10 03:22:25 UTC) #33

juncai

https://codereview.chromium.org/2718583002/diff/80001/third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h (right): https://codereview.chromium.org/2718583002/diff/80001/third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h#newcode34 third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h:34: public ContextLifecycleObserver { On 2017/03/09 23:16:38, ortuno wrote: > ...

3 years, 9 months ago (2017-03-10 03:57:02 UTC) #34

juncai

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the ...

3 years, 9 months ago (2017-03-10 03:57:22 UTC) #36

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/100001

3 years, 9 months ago (2017-03-10 03:57:44 UTC) #37

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-10 04:05:22 UTC) #38

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/382641)

3 years, 9 months ago (2017-03-10 04:05:23 UTC) #39

juncai

juncai@chromium.org changed reviewers: + dcheng@chromium.org

3 years, 9 months ago (2017-03-10 04:10:57 UTC) #40

juncai

dcheng@chromium.org: Please review changes in //third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom

3 years, 9 months ago (2017-03-10 04:10:58 UTC) #41

ortuno

https://codereview.chromium.org/2718583002/diff/80001/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2718583002/diff/80001/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp#newcode130 third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:130: cleanupDisconnectedDeviceAndFireEvent(); On 2017/03/10 at 03:57:02, juncai wrote: > On ...

3 years, 9 months ago (2017-03-10 04:37:09 UTC) #42

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-10 20:39:39 UTC) #43

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/120001

3 years, 9 months ago (2017-03-10 20:40:07 UTC) #44

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-10 22:30:50 UTC) #45

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-10 22:30:51 UTC) #46

juncai

https://codereview.chromium.org/2718583002/diff/80001/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2718583002/diff/80001/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp#newcode130 third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:130: cleanupDisconnectedDeviceAndFireEvent(); On 2017/03/10 04:37:09, ortuno wrote: > On 2017/03/10 ...

3 years, 9 months ago (2017-03-10 22:31:34 UTC) #47

dcheng

It feels unusual to pass back an interface request rather than just passing an interface ...

3 years, 9 months ago (2017-03-13 07:15:57 UTC) #48

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-13 18:55:25 UTC) #49

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/140001

3 years, 9 months ago (2017-03-13 18:55:57 UTC) #50

juncai

On 2017/03/13 07:15:57, dcheng wrote: > It feels unusual to pass back an interface request ...

3 years, 9 months ago (2017-03-13 20:00:07 UTC) #51

juncai

https://codereview.chromium.org/2718583002/diff/120001/content/browser/bluetooth/frame_connected_bluetooth_devices.h File content/browser/bluetooth/frame_connected_bluetooth_devices.h (right): https://codereview.chromium.org/2718583002/diff/120001/content/browser/bluetooth/frame_connected_bluetooth_devices.h#newcode75 content/browser/bluetooth/frame_connected_bluetooth_devices.h:75: blink::mojom::WebBluetoothServerClientAssociatedPtr>, On 2017/03/13 07:15:57, dcheng wrote: > Nit: I'd ...

3 years, 9 months ago (2017-03-13 20:00:17 UTC) #52

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-13 20:29:31 UTC) #53

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/382270)

3 years, 9 months ago (2017-03-13 20:29:33 UTC) #54

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-15 17:54:12 UTC) #55

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/160001

3 years, 9 months ago (2017-03-15 17:54:41 UTC) #56

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-15 19:18:35 UTC) #57

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-15 19:18:38 UTC) #58

juncai

On 2017/03/13 07:15:57, dcheng wrote: > It feels unusual to pass back an interface request ...

3 years, 9 months ago (2017-03-16 01:27:09 UTC) #59

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-16 18:02:54 UTC) #60

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/200001

3 years, 9 months ago (2017-03-16 18:03:39 UTC) #61

juncai

juncai@chromium.org changed reviewers: + haraken@chromium.org, scheib@chromium.org

3 years, 9 months ago (2017-03-16 18:07:39 UTC) #62

juncai

I created a new patch in this CL which passes an interface pointer to the ...

3 years, 9 months ago (2017-03-16 18:10:53 UTC) #64

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-16 20:49:59 UTC) #66

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-16 20:50:00 UTC) #67

dcheng

Sorry for the review delay. Mostly nits and one question to help me understand how ...

3 years, 9 months ago (2017-03-16 22:20:30 UTC) #68

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-17 00:51:00 UTC) #69

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/220001

3 years, 9 months ago (2017-03-17 00:51:35 UTC) #70

juncai

scheib@, I answered the question in the comment. Can you correct it if the answer ...

3 years, 9 months ago (2017-03-17 00:53:57 UTC) #71

scheib

https://codereview.chromium.org/2718583002/diff/200001/content/browser/bluetooth/web_bluetooth_service_impl.cc File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2718583002/diff/200001/content/browser/bluetooth/web_bluetooth_service_impl.cc#newcode1020 content/browser/bluetooth/web_bluetooth_service_impl.cc:1020: iter->second->gatt_notify_session->IsActive()) { On 2017/03/17 00:53:56, juncai wrote: > On ...

3 years, 9 months ago (2017-03-17 01:25:15 UTC) #72

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-17 02:33:58 UTC) #73

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-17 02:33:59 UTC) #74

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-17 17:18:07 UTC) #75

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/240001

3 years, 9 months ago (2017-03-17 17:18:54 UTC) #76

juncai

https://codereview.chromium.org/2718583002/diff/200001/content/browser/bluetooth/web_bluetooth_service_impl.cc File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2718583002/diff/200001/content/browser/bluetooth/web_bluetooth_service_impl.cc#newcode1020 content/browser/bluetooth/web_bluetooth_service_impl.cc:1020: iter->second->gatt_notify_session->IsActive()) { On 2017/03/17 01:25:15, scheib wrote: > On ...

3 years, 9 months ago (2017-03-17 17:52:59 UTC) #77

https://codereview.chromium.org/2718583002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2718583002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:1020:
iter->second->gatt_notify_session->IsActive()) {
On 2017/03/17 01:25:15, scheib wrote:
> On 2017/03/17 00:53:56, juncai wrote:
> > On 2017/03/16 22:20:29, dcheng wrote:
> > > Just to help me understand... what is IsActive(), and why do we check
this?
> > > Furthermore, why do we keep inactive sessions around in the map?
> > > 
> > > (I'm wondering how this interfaces with the Blink side - is it possible to
> end
> > > up with multiple DOM objects that want to listen for changes on the same
> > > characteristic?)
> > 
> > This is similar to:
> >
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > 
> > I'll try answering here.
> > scheib@, can you correct me if the following is incorrect? Thanks!
> > 
> > It is possible that when
> WebBluetoothServiceImpl::OnStartNotifySessionSuccess()
> > is called, BluetoothGattNotifySession::IsActive() returns false, since
> > BluetoothGattNotifySession::Stop() may just be called:
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_gatt_notify_s...
> > but if BluetoothGattNotifySession::Stop()'s callback is not called yet:
> >
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > And the callback is supposed to remove the inactive session from the map:
> >
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > 
> > So in this case, the inactive session is still in the map. We still want to
> > update the new |notify_session| in the map. So we check IsActive() here.
> > 
> > But I think when the callback is called after the new |notify_session| is
> > updated in the map, this |notify_session| is removed. I saw there is already
> an
> > issue filed for this:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
> > 
> > Based on this:
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> > I think it is possible that multiple DOM objects listen for the same
> > characteristic.
> 
> Yes, the description for why IsActive may be false sounds accurate.
> 
> Thinking further though, I'm not sure why we need this logic block.
> 
> We hopefully already avoided requesting a duplicate session in
> RemoteCharacteristicStartNotifications[1].
> 
> If we do somehow get a double success then we always want to return SUCCESS
> anyway, and in both cases (active or not) it would be OK to replace the
session
> in the map with a new one.
> 
> So, probably we should remove this if -- unless juncai you can explain why we
> need it?
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

Done.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-17 19:42:06 UTC) #78

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-17 19:42:08 UTC) #79

ortuno

Do we remove sessions from the map if a device disconnects? On Sat, Mar 18, ...

3 years, 9 months ago (2017-03-17 20:58:14 UTC) #80

Do we remove sessions from the map if a device disconnects?

On Sat, Mar 18, 2017, 4:53 AM <juncai@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2718583002/diff/200001/content/browser/blueto...
> File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):
>
>
>
https://codereview.chromium.org/2718583002/diff/200001/content/browser/blueto...
> content/browser/bluetooth/web_bluetooth_service_impl.cc:1020:
> iter->second->gatt_notify_session->IsActive()) {
> On 2017/03/17 01:25:15, scheib wrote:
> > On 2017/03/17 00:53:56, juncai wrote:
> > > On 2017/03/16 22:20:29, dcheng wrote:
> > > > Just to help me understand... what is IsActive(), and why do we
> check this?
> > > > Furthermore, why do we keep inactive sessions around in the map?
> > > >
> > > > (I'm wondering how this interfaces with the Blink side - is it
> possible to
> > end
> > > > up with multiple DOM objects that want to listen for changes on
> the same
> > > > characteristic?)
> > >
> > > This is similar to:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > >
> > > I'll try answering here.
> > > scheib@, can you correct me if the following is incorrect? Thanks!
> > >
> > > It is possible that when
> > WebBluetoothServiceImpl::OnStartNotifySessionSuccess()
> > > is called, BluetoothGattNotifySession::IsActive() returns false,
> since
> > > BluetoothGattNotifySession::Stop() may just be called:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_gatt_notify_s...
> > > but if BluetoothGattNotifySession::Stop()'s callback is not called
> yet:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > > And the callback is supposed to remove the inactive session from the
> map:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > >
> > > So in this case, the inactive session is still in the map. We still
> want to
> > > update the new |notify_session| in the map. So we check IsActive()
> here.
> > >
> > > But I think when the callback is called after the new
> |notify_session| is
> > > updated in the map, this |notify_session| is removed. I saw there is
> already
> > an
> > > issue filed for this:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
> > >
> > > Based on this:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> > > I think it is possible that multiple DOM objects listen for the same
> > > characteristic.
> >
> > Yes, the description for why IsActive may be false sounds accurate.
> >
> > Thinking further though, I'm not sure why we need this logic block.
> >
> > We hopefully already avoided requesting a duplicate session in
> > RemoteCharacteristicStartNotifications[1].
> >
> > If we do somehow get a double success then we always want to return
> SUCCESS
> > anyway, and in both cases (active or not) it would be OK to replace
> the session
> > in the map with a new one.
> >
> > So, probably we should remove this if -- unless juncai you can explain
> why we
> > need it?
> >
> > [1]
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
>
> Done.
>
> https://codereview.chromium.org/2718583002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

ortuno

Do we remove sessions from the map if a device disconnects? On Sat, Mar 18, ...

3 years, 9 months ago (2017-03-17 20:58:15 UTC) #81

Do we remove sessions from the map if a device disconnects?

On Sat, Mar 18, 2017, 4:53 AM <juncai@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2718583002/diff/200001/content/browser/blueto...
> File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):
>
>
>
https://codereview.chromium.org/2718583002/diff/200001/content/browser/blueto...
> content/browser/bluetooth/web_bluetooth_service_impl.cc:1020:
> iter->second->gatt_notify_session->IsActive()) {
> On 2017/03/17 01:25:15, scheib wrote:
> > On 2017/03/17 00:53:56, juncai wrote:
> > > On 2017/03/16 22:20:29, dcheng wrote:
> > > > Just to help me understand... what is IsActive(), and why do we
> check this?
> > > > Furthermore, why do we keep inactive sessions around in the map?
> > > >
> > > > (I'm wondering how this interfaces with the Blink side - is it
> possible to
> > end
> > > > up with multiple DOM objects that want to listen for changes on
> the same
> > > > characteristic?)
> > >
> > > This is similar to:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > >
> > > I'll try answering here.
> > > scheib@, can you correct me if the following is incorrect? Thanks!
> > >
> > > It is possible that when
> > WebBluetoothServiceImpl::OnStartNotifySessionSuccess()
> > > is called, BluetoothGattNotifySession::IsActive() returns false,
> since
> > > BluetoothGattNotifySession::Stop() may just be called:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_gatt_notify_s...
> > > but if BluetoothGattNotifySession::Stop()'s callback is not called
> yet:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > > And the callback is supposed to remove the inactive session from the
> map:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > >
> > > So in this case, the inactive session is still in the map. We still
> want to
> > > update the new |notify_session| in the map. So we check IsActive()
> here.
> > >
> > > But I think when the callback is called after the new
> |notify_session| is
> > > updated in the map, this |notify_session| is removed. I saw there is
> already
> > an
> > > issue filed for this:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
> > >
> > > Based on this:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
> > > I think it is possible that multiple DOM objects listen for the same
> > > characteristic.
> >
> > Yes, the description for why IsActive may be false sounds accurate.
> >
> > Thinking further though, I'm not sure why we need this logic block.
> >
> > We hopefully already avoided requesting a duplicate session in
> > RemoteCharacteristicStartNotifications[1].
> >
> > If we do somehow get a double success then we always want to return
> SUCCESS
> > anyway, and in both cases (active or not) it would be OK to replace
> the session
> > in the map with a new one.
> >
> > So, probably we should remove this if -- unless juncai you can explain
> why we
> > need it?
> >
> > [1]
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
>
> Done.
>
> https://codereview.chromium.org/2718583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

juncai

On 2017/03/17 20:58:15, ortuno wrote: > Do we remove sessions from the map if a ...

3 years, 9 months ago (2017-03-17 21:12:07 UTC) #82

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-20 18:44:34 UTC) #83

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/260001

3 years, 9 months ago (2017-03-20 18:45:07 UTC) #84

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-20 19:19:37 UTC) #85

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/253653) chromeos_amd64-generic_chromium_compile_only_ng on ...

3 years, 9 months ago (2017-03-20 19:19:38 UTC) #86

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-21 00:36:39 UTC) #87

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/260001

3 years, 9 months ago (2017-03-21 00:37:33 UTC) #88

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-21 03:07:25 UTC) #89

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-21 03:07:27 UTC) #90

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-22 01:14:17 UTC) #91

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/280001

3 years, 9 months ago (2017-03-22 01:15:11 UTC) #92

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-22 03:17:37 UTC) #94

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started ...

3 years, 9 months ago (2017-03-22 03:17:38 UTC) #95

dcheng

Sorry for the review delay. I've been thinking about this, and I feel like I ...

3 years, 9 months ago (2017-03-22 10:25:22 UTC) #96

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-22 19:49:42 UTC) #97

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/300001

3 years, 9 months ago (2017-03-22 19:50:10 UTC) #98

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-22 21:40:14 UTC) #99

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-22 21:40:15 UTC) #100

juncai

On 2017/03/22 10:25:22, dcheng wrote: > Sorry for the review delay. > > I've been ...

3 years, 9 months ago (2017-03-22 22:04:29 UTC) #101

dcheng

(talked with juncai@ about the semantics, which are basically 'last one to connect / request ...

3 years, 9 months ago (2017-03-23 00:16:57 UTC) #102

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-23 01:10:34 UTC) #103

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/320001

3 years, 9 months ago (2017-03-23 01:11:17 UTC) #104

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-23 03:03:32 UTC) #105

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/406290)

3 years, 9 months ago (2017-03-23 03:03:33 UTC) #106

juncai

https://codereview.chromium.org/2718583002/diff/300001/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.h File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.h (right): https://codereview.chromium.org/2718583002/diff/300001/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.h#newcode140 third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.h:140: mojo::AssociatedBinding<mojom::blink::WebBluetoothCharacteristicClient>>> On 2017/03/23 00:16:57, dcheng wrote: > Can this ...

3 years, 9 months ago (2017-03-23 05:34:19 UTC) #107

dcheng

LGTM Honestly, I'm not actually sure that we need to keep the set of bindings ...

3 years, 9 months ago (2017-03-23 05:42:13 UTC) #108

juncai

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-23 17:47:25 UTC) #109

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/340001

3 years, 9 months ago (2017-03-23 17:48:03 UTC) #110

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-23 19:07:29 UTC) #111

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/415242)

3 years, 9 months ago (2017-03-23 19:07:31 UTC) #112

juncai

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org, scheib@chromium.org, dcheng@chromium.org Link ...

3 years, 9 months ago (2017-03-23 20:22:49 UTC) #114

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718583002/340001

3 years, 9 months ago (2017-03-23 20:23:30 UTC) #115

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 340001, "attempt_start_ts": 1490300569000500, "parent_rev": "344035e6942d313099c3ceb741f2eb8231de1a2c", "commit_rev": "5fbf7e618bca38528d65a5f673b030fb0b2cd684"}

3 years, 9 months ago (2017-03-23 21:22:02 UTC) #116

commit-bot: I haz the power

Description was changed from ========== Refactor WebBluetoothServiceClient in the web_bluetooth.mojom This CL changes the WebBluetoothServiceClient ...

3 years, 9 months ago (2017-03-23 21:22:53 UTC) #117

commit-bot: I haz the power

Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/5fbf7e618bca38528d65a5f673b030fb0b2cd684

3 years, 9 months ago (2017-03-23 21:22:55 UTC) #118

ortuno

fwiw I think we should change to the original approach and keep returning the client ...

3 years, 9 months ago (2017-03-28 09:34:29 UTC) #119

juncai

3 years, 8 months ago (2017-03-29 17:31:14 UTC) #120

Message was sent while issue was closed.

On 2017/03/28 09:34:29, ortuno wrote:
> fwiw I think we should change to the original approach and keep returning the
> client from the browser. I see a couple of problems with this approach:
> 
> 1. Both CreateGattConnection and StartNotifySession are async calls which
means
> that it's possible to send events i.e. notifications and disconnection events
> before callbacks are run and promises resolve.
> 2. We are holding a set of mostly obsolete clients which I think is
unnecessary
> and wasteful.
> 3. Even when CreateGattConnection/StartNotifySession fail we end up with a
bound
> client.
> 
> We could obviously add logic to fix all of these issues i.e. close bindings
when
> connection/notifications fail, add logic to remove obsolete clients, etc. but
I
> think the other approach results in less code and avoids subtle race
conditions.

For the binding set that was added in this CL, the following CL changes it back:
https://codereview.chromium.org/2752663002/

Issue 2718583002: Refactor WebBluetoothServiceClient in the web_bluetooth.mojom (Closed)

Description

Patch Set 1 : refactor WebBluetoothServiceClient in the web_bluetooth.mojom #

Patch Set 2 : fix content unit tests #

Patch Set 3 : address comments #

Patch Set 4 : address more comments #

Patch Set 5 : address more comments #

Patch Set 6 : address more comments #

Patch Set 7 : added new layout test #

Patch Set 8 : address dcheng@'s comments #

Patch Set 9 : merge master and resolve conflicts #

Patch Set 10 : pass interface pointer to the Bluetooth service implementation #

Patch Set 11 : address comments from: https://codereview.chromium.org/2751293002/ #

Patch Set 12 : address dcheng@'s comments #

Patch Set 13 : address scheib@'s comment #

Patch Set 14 : rebase #

Patch Set 15 : rebase #

Patch Set 16 : address dcheng@'s comment #

Patch Set 17 : use AssociatedBindingSet #

Patch Set 18 : rebase #

Messages