Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752663002:
Remove RemoteServerDisconnect() from web_bluetooth.mojom (Closed)
|
Created:
3 years, 9 months ago by juncai Modified:
3 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, haraken, jam, ortuno+watch_chromium.org, qsr+mojo_chromium.org, scheib+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove RemoteServerDisconnect() from web_bluetooth.mojom
When either of the following happens:
1. When a device is disconnected on the browser side.
2. When the renderer intentionally wants to disconnect from a device.
It is currently handled by RemoteServerDisconnect() which is
implemented on the browser side and called on the renderer side.
This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and
uses error handlers on both browser and renderer side instead. It
cleans up the code and make the disconnection logic simpler and more
straightforward.
BUG=697700
Patch Set 1 : remove RemoteServerDisconnect() from web_bluetooth.mojom #Patch Set 2 : updated function name #Patch Set 3 : update comment #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : rebase #
Total comments: 10
Patch Set 8 : address scheib@'s comments #
Total comments: 15
Patch Set 9 : rebase #Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : merge master #Patch Set 13 : code cleanup #Patch Set 14 : address scheib@'s comments #
Total comments: 17
Patch Set 15 : address ortuno@'s comments #
Total comments: 1
Messages
Total messages: 65 (48 generated)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and use error handlers instead. BUG=697700 ========== to ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and use error handlers instead. BUG=697700 ==========
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
juncai@chromium.org changed reviewers: + ortuno@chromium.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and use error handlers instead. BUG=697700 ========== to ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and uses error handlers instead. BUG=697700 ==========
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
juncai@chromium.org changed reviewers: + scheib@chromium.org
Please take a look.
Explain more in the change description about what is being changed. I read up some on the linked bug and 2 comments from previous reviews, but that explanation should be distilled into the summary here. https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:358: void WebBluetoothServiceImpl::HandleServerClientError( Keep .h and .cc file methods in same order please. https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:149: void HandleServerClientError(const WebBluetoothDeviceId& device_id); Explain in comments when this is expected to happen, e.g. when we loose connection to a renderer and also when the renderer intentionally is disconnecting. https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:40: // This function can only be called if m_clientBinding is bound and Can we assert these claims with DCHECKs? https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:45: m_device->dispatchEvent( Nearly all locations CleanupDisconnectedDevice is called create this event. Should it be created in CleanupDisconnectedDevice? Perhaps an argument to control that? Let's understand why we handle it differently when we don't, and make that clear. https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:69: CleanupDisconnectedDevice(); This location doesn't create a disconnected event. Let's understand why not and explain it.
Description was changed from ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and uses error handlers instead. BUG=697700 ========== to ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom When either of the following happens: 1. When a device is disconnected on the browser side. 2. When the renderer intentionally wants to disconnect from a device. It is currently handled by RemoteServerDisconnect() which is implemented on the browser side and called on the renderer side This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and uses error handlers on both browser and renderer sides instead. It cleans up the code and make the disconnection logic simpler and more straightforward on both browser and renderer sides. BUG=697700 ==========
Description was changed from ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom When either of the following happens: 1. When a device is disconnected on the browser side. 2. When the renderer intentionally wants to disconnect from a device. It is currently handled by RemoteServerDisconnect() which is implemented on the browser side and called on the renderer side This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and uses error handlers on both browser and renderer sides instead. It cleans up the code and make the disconnection logic simpler and more straightforward on both browser and renderer sides. BUG=697700 ========== to ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom When either of the following happens: 1. When a device is disconnected on the browser side. 2. When the renderer intentionally wants to disconnect from a device. It is currently handled by RemoteServerDisconnect() which is implemented on the browser side and called on the renderer side. This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and uses error handlers on both browser and renderer side instead. It cleans up the code and make the disconnection logic simpler and more straightforward. BUG=697700 ==========
Description was changed from ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom When either of the following happens: 1. When a device is disconnected on the browser side. 2. When the renderer intentionally wants to disconnect from a device. It is currently handled by RemoteServerDisconnect() which is implemented on the browser side and called on the renderer side. This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and uses error handlers on both browser and renderer side instead. It cleans up the code and make the disconnection logic simpler and more straightforward. BUG=697700 ========== to ========== Remove RemoteServerDisconnect() from web_bluetooth.mojom When either of the following happens: 1. When a device is disconnected on the browser side. 2. When the renderer intentionally wants to disconnect from a device. It is currently handled by RemoteServerDisconnect() which is implemented on the browser side and called on the renderer side. This CL removes RemoteServerDisconnect() from web_bluetooth.mojom and uses error handlers on both browser and renderer side instead. It cleans up the code and make the disconnection logic simpler and more straightforward. BUG=697700 ==========
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:358: void WebBluetoothServiceImpl::HandleServerClientError( On 2017/03/20 21:39:13, scheib wrote: > Keep .h and .cc file methods in same order please. Done. https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:149: void HandleServerClientError(const WebBluetoothDeviceId& device_id); On 2017/03/20 21:39:13, scheib wrote: > Explain in comments when this is expected to happen, e.g. when we loose > connection to a renderer and also when the renderer intentionally is > disconnecting. Done. https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:40: // This function can only be called if m_clientBinding is bound and On 2017/03/20 21:39:14, scheib wrote: > Can we assert these claims with DCHECKs? Added a "DCHECK(m_clientBinding.is_bound());" here. And CleanupDisconnectedDevice() has "DCHECK(m_connected);". Done. https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:45: m_device->dispatchEvent( On 2017/03/20 21:39:14, scheib wrote: > Nearly all locations CleanupDisconnectedDevice is called create this event. > Should it be created in CleanupDisconnectedDevice? Perhaps an argument to > control that? Let's understand why we handle it differently when we don't, and > make that clear. Added an extra parameter to CleanupDisconnectedDevice() and it controls if dispatching an event. Also added comments to places where no event is dispatched. Done. https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:69: CleanupDisconnectedDevice(); On 2017/03/20 21:39:14, scheib wrote: > This location doesn't create a disconnected event. Let's understand why not and > explain it. This function DisconnectIfConnected() is removed since it is only called by Dispose(), so moved its content to Dispose(). And added comment there to explain why no event is dispatched. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Fwiw a lot of these functions are pretty much copies of the spec functions so it's easier to follow the implementation. On Tue, Mar 21, 2017, 11:52 AM <juncai@chromium.org> wrote: > > > https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... > File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): > > > https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... > content/browser/bluetooth/web_bluetooth_service_impl.cc:358: void > WebBluetoothServiceImpl::HandleServerClientError( > On 2017/03/20 21:39:13, scheib wrote: > > Keep .h and .cc file methods in same order please. > > Done. > > > > https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... > File content/browser/bluetooth/web_bluetooth_service_impl.h (right): > > > https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... > content/browser/bluetooth/web_bluetooth_service_impl.h:149: void > HandleServerClientError(const WebBluetoothDeviceId& device_id); > On 2017/03/20 21:39:13, scheib wrote: > > Explain in comments when this is expected to happen, e.g. when we > loose > > connection to a renderer and also when the renderer intentionally is > > disconnecting. > > Done. > > > > https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp > (right): > > > https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:40: > // This function can only be called if m_clientBinding is bound and > On 2017/03/20 21:39:14, scheib wrote: > > Can we assert these claims with DCHECKs? > > Added a "DCHECK(m_clientBinding.is_bound());" here. > And CleanupDisconnectedDevice() has "DCHECK(m_connected);". > > Done. > > > > https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:45: > m_device->dispatchEvent( > On 2017/03/20 21:39:14, scheib wrote: > > Nearly all locations CleanupDisconnectedDevice is called create this > event. > > Should it be created in CleanupDisconnectedDevice? Perhaps an argument > to > > control that? Let's understand why we handle it differently when we > don't, and > > make that clear. > > Added an extra parameter to CleanupDisconnectedDevice() and it controls > if dispatching an event. > > Also added comments to places where no event is dispatched. > > Done. > > > > https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:69: > CleanupDisconnectedDevice(); > On 2017/03/20 21:39:14, scheib wrote: > > This location doesn't create a disconnected event. Let's understand > why not and > > explain it. > > This function DisconnectIfConnected() is removed since it is only called > by Dispose(), so moved its content to Dispose(). > > And added comment there to explain why no event is dispatched. > > Done. > > https://codereview.chromium.org/2752663002/ > -- 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.
Fwiw a lot of these functions are pretty much copies of the spec functions so it's easier to follow the implementation. On Tue, Mar 21, 2017, 11:52 AM <juncai@chromium.org> wrote: > > > https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... > File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): > > > https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... > content/browser/bluetooth/web_bluetooth_service_impl.cc:358: void > WebBluetoothServiceImpl::HandleServerClientError( > On 2017/03/20 21:39:13, scheib wrote: > > Keep .h and .cc file methods in same order please. > > Done. > > > > https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... > File content/browser/bluetooth/web_bluetooth_service_impl.h (right): > > > https://codereview.chromium.org/2752663002/diff/120001/content/browser/blueto... > content/browser/bluetooth/web_bluetooth_service_impl.h:149: void > HandleServerClientError(const WebBluetoothDeviceId& device_id); > On 2017/03/20 21:39:13, scheib wrote: > > Explain in comments when this is expected to happen, e.g. when we > loose > > connection to a renderer and also when the renderer intentionally is > > disconnecting. > > Done. > > > > https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp > (right): > > > https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:40: > // This function can only be called if m_clientBinding is bound and > On 2017/03/20 21:39:14, scheib wrote: > > Can we assert these claims with DCHECKs? > > Added a "DCHECK(m_clientBinding.is_bound());" here. > And CleanupDisconnectedDevice() has "DCHECK(m_connected);". > > Done. > > > > https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:45: > m_device->dispatchEvent( > On 2017/03/20 21:39:14, scheib wrote: > > Nearly all locations CleanupDisconnectedDevice is called create this > event. > > Should it be created in CleanupDisconnectedDevice? Perhaps an argument > to > > control that? Let's understand why we handle it differently when we > don't, and > > make that clear. > > Added an extra parameter to CleanupDisconnectedDevice() and it controls > if dispatching an event. > > Also added comments to places where no event is dispatched. > > Done. > > > > https://codereview.chromium.org/2752663002/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:69: > CleanupDisconnectedDevice(); > On 2017/03/20 21:39:14, scheib wrote: > > This location doesn't create a disconnected event. Let's understand > why not and > > explain it. > > This function DisconnectIfConnected() is removed since it is only called > by Dispose(), so moved its content to Dispose(). > > And added comment there to explain why no event is dispatched. > > Done. > > https://codereview.chromium.org/2752663002/ > -- 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.
https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:842: UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT); This UMA was intended to count how often Javascript called disconnect, but now it also counts when the device disconnects and the browser is informed, or for any other reason when the client is disconnected. Perhaps move this UMA to BluetoothRemoteGATTServer::disconnect, with a comment here and there explaining why we do this. Another alternative is a patch before this one that moves all Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit. https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:150: // 1. When a device is disconnected on the browser side. Let's reword to be a bit clearer, something like: Renderer has closed the pipe because it no longer needs a connection to this device. Because: 1) Content script code has called disconnect() 2) The actual device disconnected, WebBluetoothServiceImpl informed the renderer, and renderer reacted. https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:152: void HandleServerClientError(const WebBluetoothDeviceId& device_id); Perhaps: OnGATTServerClientDisconnection https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:63: void BluetoothRemoteGATTServer::CleanupDisconnectedDevice(bool dispatchEvent) { ortuno reminds us that this implementation is intended to follow https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device. Please add a comment on this implementation to make that clear // This implements the specification "clean up the disconnected device" algorithm // https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device I think it is warranted in both declaration and here in definition because it's being used in a very specific way. https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:67: m_device->ClearAttributeInstanceMap(); Below this line there are steps in the specification which we are not explicitly handling here (clearing representedService, notification & indications). We should comment here explaining why no explicit actions are needed. OK to mark this as a TODO and follow up. https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:87: CleanupDisconnectedDevice(false /* dispatchEvent */); Thanks! So much clearer. https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:139: m_clientBinding.Close(); It now becomes less clear how we are signaling to the WBService that disconnect should happen. Perhaps a short comment here, or less so on the member m_clientBinding. // Implicitly signals disconnect via the connection_error_handler https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.h (right): https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.h:64: void CleanupDisconnectedDevice(bool dispatchEvent); Let's cite the specification algorithm this method is implementing. https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:842: UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT); On 2017/03/22 01:11:07, scheib wrote: > This UMA was intended to count how often Javascript called disconnect, but now > it also counts when the device disconnects and the browser is informed, or for > any other reason when the client is disconnected. > > Perhaps move this UMA to BluetoothRemoteGATTServer::disconnect, with a comment > here and there explaining why we do this. > > Another alternative is a patch before this one that moves all > Bluetooth.Web.FunctionCall.Count UMA from browser to WebKit. Filed an issue: https://bugs.chromium.org/p/chromium/issues/detail?id=703970 https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:150: // 1. When a device is disconnected on the browser side. On 2017/03/22 01:11:08, scheib wrote: > Let's reword to be a bit clearer, something like: > > Renderer has closed the pipe because it no longer needs a connection > to this device. Because: > 1) Content script code has called disconnect() > 2) The actual device disconnected, WebBluetoothServiceImpl informed the > renderer, and renderer reacted. Done. https://codereview.chromium.org/2752663002/diff/140001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:152: void HandleServerClientError(const WebBluetoothDeviceId& device_id); On 2017/03/22 01:11:08, scheib wrote: > Perhaps: OnGATTServerClientDisconnection Done. https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:63: void BluetoothRemoteGATTServer::CleanupDisconnectedDevice(bool dispatchEvent) { On 2017/03/22 01:11:08, scheib wrote: > ortuno reminds us that this implementation is intended to follow > https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device. > > > Please add a comment on this implementation to make that clear > > // This implements the specification "clean up the disconnected device" > algorithm > // > https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device > > I think it is warranted in both declaration and here in definition because it's > being used in a very specific way. Done. https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:67: m_device->ClearAttributeInstanceMap(); On 2017/03/22 01:11:08, scheib wrote: > Below this line there are steps in the specification which we are not explicitly > handling here (clearing representedService, notification & indications). We > should comment here explaining why no explicit actions are needed. OK to mark > this as a TODO and follow up. Done. https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:139: m_clientBinding.Close(); On 2017/03/22 01:11:08, scheib wrote: > It now becomes less clear how we are signaling to the WBService that disconnect > should happen. Perhaps a short comment here, or less so on the member > m_clientBinding. > > // Implicitly signals disconnect via the connection_error_handler Done. https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.h (right): https://codereview.chromium.org/2752663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.h:64: void CleanupDisconnectedDevice(bool dispatchEvent); On 2017/03/22 01:11:08, scheib wrote: > Let's cite the specification algorithm this method is implementing. > https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2752663002/diff/260001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2752663002/diff/260001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:841: RecordWebBluetoothFunctionCall( This will incorrectly record UMA for disconnects that were not caused by JavaScript. This patch should come after a patch that resolves the UMA move: https://codereview.chromium.org/2771893002/
https://codereview.chromium.org/2752663002/diff/260001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2752663002/diff/260001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:841: RecordWebBluetoothFunctionCall( On 2017/03/27 21:51:46, scheib wrote: > This will incorrectly record UMA for disconnects that were not caused by > JavaScript. This patch should come after a patch that resolves the UMA move: > https://codereview.chromium.org/2771893002/ Sorry for not explicitly mentioning that in previous comments. Yes, the original plan was to wait until the CL that resolves UMA is landed, then this CL can proceed.
juncai@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@chromium.org, please review changes in: third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom This CL also addresses the comment at: https://codereview.chromium.org/2718583002/ by using mojo::AssociatedBinding instead of mojo::AssociatedBindingSet in the BluetoothRemoteGATTServer.
lgtm https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:146: ClearActiveAlgorithms(); Just curious: how come we need to ClearActiveAlgorithms() here before the early return? Might be worth a comment, since CleanupDisconnectedDevice() also does that.
https://codereview.chromium.org/2752663002/diff/260001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2752663002/diff/260001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:841: RecordWebBluetoothFunctionCall( On 2017/03/27 23:21:14, juncai wrote: > On 2017/03/27 21:51:46, scheib wrote: > > This will incorrectly record UMA for disconnects that were not caused by > > JavaScript. This patch should come after a patch that resolves the UMA move: > > https://codereview.chromium.org/2771893002/ > > Sorry for not explicitly mentioning that in previous comments. Yes, the original > plan was to wait until the CL that resolves UMA is landed, then this CL can > proceed. It is clearer if you e.g. create the UMA change branch, upload the UMA patch, merge that branch into this branch, and then upload. codereview tool will then see the branch is dependent and the diff will no longer include this UMA change.
https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:41: // |m_clientBinding| can only be bound if |m_connected| is true. I might be missing something here but m_clientBinding is bound as soon as we call connect during which m_connected stays false. https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:73: // notification & indications). Need to add comment here explaining why no Clearing the represented services, characteristics and descriptors is done by ClearAttributeInstanceMap(). The notification context is implementing in the browser so it cleans up up itself. https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:124: if (m_connected) { hmm I think this is breaking spec'ed behavior e.g. connect().then(() => { connect(); disconnect(); }); Based on the spec'ed algorithm the second call to connect should reject: Step 5.2 says to queue a task, at which point disconnect runs, the algorithm for disconnect immediately removes the promise from the active algorithms map so when step 5.2.1 runs the promise should reject. https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver... https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:131: m_clientBinding.set_connection_error_handler(convertToBaseCallback( Since you are binding this here and in the browser wouldn't you get a notification if the connection fails since you are closing the binding? https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:146: ClearActiveAlgorithms(); On 2017/03/28 at 02:25:01, dcheng wrote: > Just curious: how come we need to ClearActiveAlgorithms() here before the early return? Might be worth a comment, since CleanupDisconnectedDevice() also does that. I asked the same in the spec a while ago :) The answer is in the spec: "1. Clear this.[[activeAlgorithms]] to abort any active connect() calls." https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver... Basically connect() calls will add a promise to this active algorithms map. When connect's callback is run we check that the promise is still in this map and if not we reject the promise. During this, m_connected stays false so if we want to actually cancel the connection we need to do so before the m_connected check.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:41: // |m_clientBinding| can only be bound if |m_connected| is true. On 2017/03/30 04:20:48, ortuno wrote: > I might be missing something here but m_clientBinding is bound as soon as we > call connect during which m_connected stays false. Done. https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:73: // notification & indications). Need to add comment here explaining why no On 2017/03/30 04:20:48, ortuno wrote: > Clearing the represented services, characteristics and descriptors is done by > ClearAttributeInstanceMap(). The notification context is implementing in the > browser so it cleans up up itself. Done. https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:124: if (m_connected) { On 2017/03/30 04:20:48, ortuno wrote: > hmm I think this is breaking spec'ed behavior e.g. > > connect().then(() => { > connect(); > disconnect(); > }); > > Based on the spec'ed algorithm the second call to connect should reject: Step > 5.2 says to queue a task, at which point disconnect runs, the algorithm for > disconnect immediately removes the promise from the active algorithms map so > when step 5.2.1 runs the promise should reject. > > https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver... hmm... I am not sure I understand it. When the second connect() is called, the connect() code is not aware of the following code has a disconnect() call, so it can't act upon it. Similar to the following: connect().then(() => { connect(); ... ... }); And since this is the second call connect(), it makes sense to just resolve the promise if |m_connected| is true. https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:131: m_clientBinding.set_connection_error_handler(convertToBaseCallback( On 2017/03/30 04:20:48, ortuno wrote: > Since you are binding this here and in the browser wouldn't you get a > notification if the connection fails since you are closing the binding? Yes, only if |m_connected| is true. And in that situation, a notification is necessary, right? https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:146: ClearActiveAlgorithms(); On 2017/03/28 02:25:01, dcheng wrote: > Just curious: how come we need to ClearActiveAlgorithms() here before the early > return? Might be worth a comment, since CleanupDisconnectedDevice() also does > that. comment added. Done. https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:146: ClearActiveAlgorithms(); On 2017/03/30 04:20:48, ortuno wrote: > On 2017/03/28 at 02:25:01, dcheng wrote: > > Just curious: how come we need to ClearActiveAlgorithms() here before the > early return? Might be worth a comment, since CleanupDisconnectedDevice() also > does that. > > I asked the same in the spec a while ago :) The answer is in the spec: > "1. Clear this.[[activeAlgorithms]] to abort any active connect() calls." > > https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver... > > Basically connect() calls will add a promise to this active algorithms map. When > connect's callback is run we check that the promise is still in this map and if > not we reject the promise. > > During this, m_connected stays false so if we want to actually cancel the > connection we need to do so before the m_connected check. Thanks Gio for the explanation!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2752663002/diff/280001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2752663002/diff/280001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:841: RecordWebBluetoothFunctionCall( Please address the UMA patch, and make this change based after that one. . checkout this git branch, . set it to track the UMA branch with git branch --set-upstream-to= . merge in that branch . upload again. This change should not show an incorrect UMA usage.
https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp (right): https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:124: if (m_connected) { On 2017/03/30 at 18:44:57, juncai wrote: > On 2017/03/30 04:20:48, ortuno wrote: > > hmm I think this is breaking spec'ed behavior e.g. > > > > connect().then(() => { > > connect(); > > disconnect(); > > }); > > > > Based on the spec'ed algorithm the second call to connect should reject: Step > > 5.2 says to queue a task, at which point disconnect runs, the algorithm for > > disconnect immediately removes the promise from the active algorithms map so > > when step 5.2.1 runs the promise should reject. > > > > https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver... > > hmm... I am not sure I understand it. When the second connect() is called, the connect() code is not aware of the following code has a disconnect() call, so it can't act upon it. Similar to the following: > connect().then(() => { > connect(); > ... > ... > }); > And since this is the second call connect(), it makes sense to just resolve the promise if |m_connected| is true. According to step 5.2 of the connect() algorithm in the spec we *have* to post a task before resolving the promise. If we don't then the bug I described before will happen. This is what should happen in the code above based on the spec'ed functions: // When the second connect is called: 1. Steps 1, 2, 3, 4, and 5.1 of the connect()[1] algorithm run 2. Step 5.2 queues a task to execute steps 5.2.* // disconnect is called 3. Step 1 of the disconnect()[2] algorithm immediately clears the active algorithms map 4. Steps 2, 3, 4, 5 of the disconnect() algorithm run 5. The task posted by 5.2 of the connect() algorithm starts running 6. Step 5.2.1 checks the active algorithms map and since the connect() promise is no longer there (it was removed by step 1 of the disconnect() algorithm), then the promise rejects. But as is this code will resolve the promise immediately and the spec'ed behavior will not happen. Before the "queue a task" part of the algorithm was being done by calling RemoteGATTServerConnect. [1] https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver... [2] https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver... https://codereview.chromium.org/2752663002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:131: m_clientBinding.set_connection_error_handler(convertToBaseCallback( On 2017/03/30 at 18:44:57, juncai wrote: > On 2017/03/30 04:20:48, ortuno wrote: > > Since you are binding this here and in the browser wouldn't you get a > > notification if the connection fails since you are closing the binding? > > Yes, only if |m_connected| is true. And in that situation, a notification is necessary, right? I think the fact that the client is receiving connection notifications before an actual connection is made doesn't make much sense, is very subtle and could end up causing bugs. We should refactor the code so that we only get notifications after the connection is made. |
