Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775953004:
bluetooth: Move writeValue to mojo (Closed)
|
Created:
4 years, 9 months ago by ortuno Modified:
4 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, Anand Mistry (off Chromium), ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, ortuno+watch_chromium.org, qsr+mojo_chromium.org, rouslan+watch_chromium.org, scheib+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@my-origin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Move writeValue to mojo
First step towards moving Web Bluetooth from IPC::Message to Mojo. We will
move each functions from BluetoothDispatcherHost to the new BluetoothService one
by one.
1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace
BluetoothDispatcherHost.
2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl.
For now it only implements RemoteCharacteristicWrite.
3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can
acquire an InterfacePtr to the new BluetoothService.
Since we are using mojo there could be races in our tests that rely on
IPC:Message ordering. The follow up patch addresses this issue: http://crrev.com/1815483003
BUG=508771
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/ad6b0feaf45b761f180d0b19a1dfae072e0df3b5
Cr-Commit-Position: refs/heads/master@{#384345}
Patch Set 1 #Patch Set 2 : GYP and clean up #Patch Set 3 : Moar cleanup #
Total comments: 16
Patch Set 4 : Move mojom interface to public/platform #Patch Set 5 : Move mojom interface to public/platform #Patch Set 6 : Move mojom interface to public/platform #Patch Set 7 : Move WebBluetoothService to RenderFrameHost #Patch Set 8 : Rebase #Patch Set 9 : Small cleanup #Patch Set 10 : Moar clean up #
Total comments: 2
Patch Set 11 : Moar clean up #
Total comments: 10
Patch Set 12 : Address rockot's comments #Patch Set 13 : Lint and format #Patch Set 14 : moar clean up #Patch Set 15 : Fix typoe #Patch Set 16 : Moar clean up #
Total comments: 26
Patch Set 17 : Address jyasskin's comments #
Total comments: 12
Patch Set 18 : Address jyasskin's comment #Patch Set 19 : Merge with ToT #
Total comments: 2
Patch Set 20 : Address palmer's comments #
Total comments: 13
Patch Set 21 : Address scheib's comments #Depends on Patchset: Messages
Total messages: 56 (27 generated)
Description was changed from ========== WIP. bluetooth: Move writeValue to mojo BUG= ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds a new Mojo interface to content/common/bluetooth: BluetoothService. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUIlD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG= ==========
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds a new Mojo interface to content/common/bluetooth: BluetoothService. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUIlD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG= ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds a new Mojo interface to content/common/bluetooth: BluetoothService. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUIlD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG=508771 ==========
ortuno@chromium.org changed reviewers: + rockot@chromium.org
rockot: PTAL at mojo stuff :) I also left a bunch of questions. https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... content/renderer/bluetooth/bluetooth_dispatcher.cc:145: BluetoothDispatcher::~BluetoothDispatcher() { FWIW I tried setting a error handler but it never gets called even when the Service was destroyed because the renderer process died. https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... content/renderer/bluetooth/bluetooth_dispatcher.cc:308: base::Bind(&BluetoothDispatcher::OnWriteValue, base::Unretained(this), This is safe because if the class gets destroyed the InterfacePtr will also be destroyed and no messages will arrive right? Or do I need to make a WeakPtrFactory? https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... content/renderer/bluetooth/bluetooth_dispatcher.cc:316: if (error == blink::mojom::WebBluetoothError::SUCCESS) { It would be nice if there was a way we could do this without having to check for SUCCESS. Otherwise we will have this logic in all of our functions.
https://codereview.chromium.org/1775953004/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1775953004/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.h:63: device::BluetoothDevice* device; instead of intializers in each constructor just set to nullptr here: device::BluetoothDevice* device = nullptr; etc https://codereview.chromium.org/1775953004/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_service_impl.cc (right): https://codereview.chromium.org/1775953004/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_service_impl.cc:142: query_result.characteristic->WriteRemoteCharacteristic( persisting offline discussion: need to make sure these callbacks are never passed to other threads, because that would mean they may be destroyed on other threads, and mojo callbacks can't be destroyed on a different thread from where they were created https://codereview.chromium.org/1775953004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1775953004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1107: mojo_application_host_->service_registry()->AddService( Use GetServiceRegistry() instead of mojo_application_host_->service_registry() https://codereview.chromium.org/1775953004/diff/40001/content/common/bluetoot... File content/common/bluetooth/bluetooth_service.mojom (right): https://codereview.chromium.org/1775953004/diff/40001/content/common/bluetoot... content/common/bluetooth/bluetooth_service.mojom:5: module content.mojom; Hmm. I understand you want to port to mojo first, then move to blink, but I wonder if there's any harm in just starting this mojom in blink. Even if the impl is in content/browser and the client is in content/renderer, it's going to be much less churn later (namespace changes, build changes, header changes...). https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... content/renderer/bluetooth/bluetooth_dispatcher.cc:308: base::Bind(&BluetoothDispatcher::OnWriteValue, base::Unretained(this), On 2016/03/09 at 01:21:23, ortuno wrote: > This is safe because if the class gets destroyed the InterfacePtr will also be destroyed and no messages will arrive right? Or do I need to make a WeakPtrFactory? Correct, the callback will never be invoked if the BluetoothServerPtr has been reset or destroyed. https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... content/renderer/bluetooth/bluetooth_dispatcher.cc:316: if (error == blink::mojom::WebBluetoothError::SUCCESS) { On 2016/03/09 at 01:21:23, ortuno wrote: > It would be nice if there was a way we could do this without having to check for SUCCESS. Otherwise we will have this logic in all of our functions. Can you elaborate on what a more ideal situation would look like? https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... File content/renderer/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... content/renderer/bluetooth/bluetooth_type_converters.h:13: struct TypeConverter<String, blink::WebString> { This should move somewhere more generic (somewhere non-bluetooth in content/child or third_party/WebKit).
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds a new Mojo interface to content/common/bluetooth: BluetoothService. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUIlD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG=508771 ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUIlD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG=508771 ==========
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUIlD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG=508771 ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUIlD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
rockot: PTAL again. I've moved the Service from the RenderProcessHost to RenderFrameHost. RenderFrameHost now owns the service. https://codereview.chromium.org/1775953004/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1775953004/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.h:63: device::BluetoothDevice* device; On 2016/03/09 at 19:31:26, Ken Rockot wrote: > instead of intializers in each constructor just set to nullptr here: > > device::BluetoothDevice* device = nullptr; > > etc Done. https://codereview.chromium.org/1775953004/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_service_impl.cc (right): https://codereview.chromium.org/1775953004/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_service_impl.cc:142: query_result.characteristic->WriteRemoteCharacteristic( On 2016/03/09 at 19:31:26, Ken Rockot wrote: > persisting offline discussion: need to make sure these callbacks are never passed to other threads, because that would mean they may be destroyed on other threads, and mojo callbacks can't be destroyed on a different thread from where they were created As discussed offline: BluetoothServiceImpl lives on the UI thread which outlives the DBus thread to which the mojo callback is getting passed to. https://codereview.chromium.org/1775953004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1775953004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1107: mojo_application_host_->service_registry()->AddService( On 2016/03/09 at 19:31:26, Ken Rockot wrote: > Use GetServiceRegistry() instead of mojo_application_host_->service_registry() Done. https://codereview.chromium.org/1775953004/diff/40001/content/common/bluetoot... File content/common/bluetooth/bluetooth_service.mojom (right): https://codereview.chromium.org/1775953004/diff/40001/content/common/bluetoot... content/common/bluetooth/bluetooth_service.mojom:5: module content.mojom; On 2016/03/09 at 19:31:26, Ken Rockot wrote: > Hmm. I understand you want to port to mojo first, then move to blink, but I wonder if there's any harm in just starting this mojom in blink. Even if the impl is in content/browser and the client is in content/renderer, it's going to be much less churn later (namespace changes, build changes, header changes...). Done. https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... content/renderer/bluetooth/bluetooth_dispatcher.cc:316: if (error == blink::mojom::WebBluetoothError::SUCCESS) { On 2016/03/09 at 19:31:26, Ken Rockot wrote: > On 2016/03/09 at 01:21:23, ortuno wrote: > > It would be nice if there was a way we could do this without having to check for SUCCESS. Otherwise we will have this logic in all of our functions. > > Can you elaborate on what a more ideal situation would look like? Jeffrey started a thread about it a while ago: https://groups.google.com/a/chromium.org/forum/#!msg/mojo-dev/7kNfPXtVLxM/Qea... https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... File content/renderer/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/1775953004/diff/40001/content/renderer/blueto... content/renderer/bluetooth/bluetooth_type_converters.h:13: struct TypeConverter<String, blink::WebString> { On 2016/03/09 at 19:31:26, Ken Rockot wrote: > This should move somewhere more generic (somewhere non-bluetooth in content/child or third_party/WebKit). Done.
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUIlD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUILD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: BluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that BluetoothDispatcher can acquire an InterfacePtr to the new BluetoothService. 4. We need an enum to be accessible from blink and content so this patch adds a new mojo enum to third_party/WebKit/public/platform/modules/bluetooth and modifies the necessary BUILD.gn's and gyp files so that classes can access the generated files. 5. Modifies blink classes to use the new mojo enum. BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
rouslan@chromium.org changed reviewers: + rouslan@chromium.org
Sorry for the drive-by. https://codereview.chromium.org/1775953004/diff/180001/content/child/mojo/typ... File content/child/mojo/type_converters.h (right): https://codereview.chromium.org/1775953004/diff/180001/content/child/mojo/typ... content/child/mojo/type_converters.h:25: struct TypeConverter<String, blink::WebString> { This should not be necessary now that https://codereview.chromium.org/1751563002/ landed. You should add "'for_blink': true" variable to the mojo target instead. That will make mojo use WTF::String directly. This avoids string copies.
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
rouslan@chromium.org changed reviewers: - rouslan@chromium.org
https://codereview.chromium.org/1775953004/diff/180001/content/child/mojo/typ... File content/child/mojo/type_converters.h (right): https://codereview.chromium.org/1775953004/diff/180001/content/child/mojo/typ... content/child/mojo/type_converters.h:25: struct TypeConverter<String, blink::WebString> { On 2016/03/16 at 22:09:30, Rouslan wrote: > This should not be necessary now that https://codereview.chromium.org/1751563002/ landed. You should add "'for_blink': true" variable to the mojo target instead. That will make mojo use WTF::String directly. This avoids string copies. That's for WTF types. This is a WebString so I still need to use the type converter. Once we get rid of the content/renderer layer we will be able to use WTF types directly.
Exciting :) This generally looks good, but I still have concerns over callback lifetime. https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:90: weak_ptr_on_ui_thread_ = weak_ptr_factory_.GetWeakPtr(); nit: I think this is a weird thing to do, but it seems like this mechanism is already being used elsewhere in bluetooth code. If the impl is the only thing vending WeakPtrs, and the impl always checks that it's being called on the right thread, this doesn't add any real value as far as I can tell. It's possible that someone might run a weakptr-bound callback on the wrong thread, but the DCHECK_CURRENTLY_ONs will catch that. https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:112: return; You should still invoke the callback here. At least for now its internal state will DCHECK if it's destroyed while the pipe is still open and the callback hasn't been invoked. The pipe may still appear to be open briefly after the renderer crashes. https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:139: weak_ptr_on_ui_thread_, callback), As discussed previously, it's generally unsafe to pass response callbacks to other threads, since they are not allowed to be destroyed on those threads. Previously I suggested that we're safe in this case because the service impl lives on the main thread which outlive whatever background thread this goes to. I don't think that actually makes sense. +amistry since I know he's thought a good bit about bindings threading issues. We need to address this ASAP as it seems most services will have to deal with it in some form or another. https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:29: mojo::InterfaceRequest<blink::mojom::WebBluetoothService> request); nit: we now have type aliases for requests, so |request|'s type can simply be blink::mojom::WebBluetoothServiceRequest. https://codereview.chromium.org/1775953004/diff/200001/content/renderer/bluet... File content/renderer/bluetooth/web_bluetooth_impl.cc (right): https://codereview.chromium.org/1775953004/diff/200001/content/renderer/bluet... content/renderer/bluetooth/web_bluetooth_impl.cc:80: base::Bind(&WebBluetoothImpl::OnWriteValue, base::Unretained(this), value, Here's a fun problem. If you can't get a WebBluetoothService (say the flag's not enabled), this call to RemoteCharacteristicWriteValue will silently fail -- there's not much else it can do really, it's writing to a dead pipe. In such a case, your |callbacks| will be destroyed without either onSuccess or onError being called. If callbacks comes from a promise adapter (it does, right?), this is a problem, because promise adapter callbacks must not be destroyed until one of the callbacks is called. I introduced ScopedWebCallbacks (in content/child) specifically for this purpose. Essentially it's just a scoped_ptr which allows you to take back ownership of the WebCallbacks; but if it's destroyed before you do that, it invokes a custom destruction callback which you can use to, say, run the WebCallbacks with a default error code. WebUSB uses this. It adds a layer of ugliness to wrap callbacks like this, but I haven't come up with a better alternative yet.
https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:90: weak_ptr_on_ui_thread_ = weak_ptr_factory_.GetWeakPtr(); On 2016/03/17 at 18:16:13, Ken Rockot wrote: > nit: I think this is a weird thing to do, but it seems like this mechanism is already being used elsewhere in bluetooth code. If the impl is the only thing vending WeakPtrs, and the impl always checks that it's being called on the right thread, this doesn't add any real value as far as I can tell. It's possible that someone might run a weakptr-bound callback on the wrong thread, but the DCHECK_CURRENTLY_ONs will catch that. I agree. I'll bring it up with scheib. https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:112: return; On 2016/03/17 at 18:16:13, Ken Rockot wrote: > You should still invoke the callback here. At least for now its internal state will DCHECK if it's destroyed while the pipe is still open and the callback hasn't been invoked. The pipe may still appear to be open briefly after the renderer crashes. I call binding_.Close() on CrashRenderAndClosePipe now. https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:139: weak_ptr_on_ui_thread_, callback), On 2016/03/17 at 18:16:13, Ken Rockot wrote: > As discussed previously, it's generally unsafe to pass response callbacks to other threads, since they are not allowed to be destroyed on those threads. Previously I suggested that we're safe in this case because the service impl lives on the main thread which outlive whatever background thread this goes to. > > I don't think that actually makes sense. +amistry since I know he's thought a good bit about bindings threading issues. We need to address this ASAP as it seems most services will have to deal with it in some form or another. As discussed offline, it's ok to do this for the time being. I opened an issue that blocks our transition on it. Will wrap the callbacks when the wrapper is available. http://crbug.com/595940 https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:29: mojo::InterfaceRequest<blink::mojom::WebBluetoothService> request); On 2016/03/17 at 18:16:13, Ken Rockot wrote: > nit: we now have type aliases for requests, so |request|'s type can simply be blink::mojom::WebBluetoothServiceRequest. Done. https://codereview.chromium.org/1775953004/diff/200001/content/renderer/bluet... File content/renderer/bluetooth/web_bluetooth_impl.cc (right): https://codereview.chromium.org/1775953004/diff/200001/content/renderer/bluet... content/renderer/bluetooth/web_bluetooth_impl.cc:80: base::Bind(&WebBluetoothImpl::OnWriteValue, base::Unretained(this), value, On 2016/03/17 at 18:16:13, Ken Rockot wrote: > Here's a fun problem. If you can't get a WebBluetoothService (say the flag's not enabled), this call to RemoteCharacteristicWriteValue will silently fail -- there's not much else it can do really, it's writing to a dead pipe. > > In such a case, your |callbacks| will be destroyed without either onSuccess or onError being called. If callbacks comes from a promise adapter (it does, right?), this is a problem, because promise adapter callbacks must not be destroyed until one of the callbacks is called. > > I introduced ScopedWebCallbacks (in content/child) specifically for this purpose. Essentially it's just a scoped_ptr which allows you to take back ownership of the WebCallbacks; but if it's destroyed before you do that, it invokes a custom destruction callback which you can use to, say, run the WebCallbacks with a default error code. WebUSB uses this. > > It adds a layer of ugliness to wrap callbacks like this, but I haven't come up with a better alternative yet. As discussed offline, ScriptPromiseResolver only DCHECKS that resolve or reject have been called. In release builds the only consequence of a service dropping callbacks is that a promise will hang. In tests if a callback is dropped then the test will fail, which is basically the same as us returning an error when the callback is dropped. I think the extra layer would add little benefit.
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. Since we are no longer using mojo there could be races in our tests that rely on IPC:Message ordering. The follow up patch addresses this issue: http://crrev.com/1815483003 BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. Since we are no longer using mojo there could be races in our tests that rely on IPC:Message ordering. The follow up patch addresses this issue: http://crrev.com/1815483003 BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. Since we are no longer using mojo there could be races in our tests that rely on IPC:Message ordering. The follow up patch addresses this issue: http://crrev.com/1815483003 BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin: PTAL
mojo stuff lgtm On 2016/03/18 at 01:19:20, ortuno wrote: > https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... > File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): > > https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... > content/browser/bluetooth/web_bluetooth_service_impl.cc:90: weak_ptr_on_ui_thread_ = weak_ptr_factory_.GetWeakPtr(); > On 2016/03/17 at 18:16:13, Ken Rockot wrote: > > nit: I think this is a weird thing to do, but it seems like this mechanism is already being used elsewhere in bluetooth code. If the impl is the only thing vending WeakPtrs, and the impl always checks that it's being called on the right thread, this doesn't add any real value as far as I can tell. It's possible that someone might run a weakptr-bound callback on the wrong thread, but the DCHECK_CURRENTLY_ONs will catch that. > > I agree. I'll bring it up with scheib. > > https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... > content/browser/bluetooth/web_bluetooth_service_impl.cc:112: return; > On 2016/03/17 at 18:16:13, Ken Rockot wrote: > > You should still invoke the callback here. At least for now its internal state will DCHECK if it's destroyed while the pipe is still open and the callback hasn't been invoked. The pipe may still appear to be open briefly after the renderer crashes. > > I call binding_.Close() on CrashRenderAndClosePipe now. > > https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... > content/browser/bluetooth/web_bluetooth_service_impl.cc:139: weak_ptr_on_ui_thread_, callback), > On 2016/03/17 at 18:16:13, Ken Rockot wrote: > > As discussed previously, it's generally unsafe to pass response callbacks to other threads, since they are not allowed to be destroyed on those threads. Previously I suggested that we're safe in this case because the service impl lives on the main thread which outlive whatever background thread this goes to. > > > > I don't think that actually makes sense. +amistry since I know he's thought a good bit about bindings threading issues. We need to address this ASAP as it seems most services will have to deal with it in some form or another. > > As discussed offline, it's ok to do this for the time being. I opened an issue that blocks our transition on it. Will wrap the callbacks when the wrapper is available. http://crbug.com/595940 > Actually the wrapper would only have been a temporary solution, and I think we can just fix Callback very soon. I don't want lots of code landing which relies on this to be safe until it's actually safe. I think in this case it's low risk though. > https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... > File content/browser/bluetooth/web_bluetooth_service_impl.h (right): > > https://codereview.chromium.org/1775953004/diff/200001/content/browser/blueto... > content/browser/bluetooth/web_bluetooth_service_impl.h:29: mojo::InterfaceRequest<blink::mojom::WebBluetoothService> request); > On 2016/03/17 at 18:16:13, Ken Rockot wrote: > > nit: we now have type aliases for requests, so |request|'s type can simply be blink::mojom::WebBluetoothServiceRequest. > > Done. > > https://codereview.chromium.org/1775953004/diff/200001/content/renderer/bluet... > File content/renderer/bluetooth/web_bluetooth_impl.cc (right): > > https://codereview.chromium.org/1775953004/diff/200001/content/renderer/bluet... > content/renderer/bluetooth/web_bluetooth_impl.cc:80: base::Bind(&WebBluetoothImpl::OnWriteValue, base::Unretained(this), value, > On 2016/03/17 at 18:16:13, Ken Rockot wrote: > > Here's a fun problem. If you can't get a WebBluetoothService (say the flag's not enabled), this call to RemoteCharacteristicWriteValue will silently fail -- there's not much else it can do really, it's writing to a dead pipe. > > > > In such a case, your |callbacks| will be destroyed without either onSuccess or onError being called. If callbacks comes from a promise adapter (it does, right?), this is a problem, because promise adapter callbacks must not be destroyed until one of the callbacks is called. > > > > I introduced ScopedWebCallbacks (in content/child) specifically for this purpose. Essentially it's just a scoped_ptr which allows you to take back ownership of the WebCallbacks; but if it's destroyed before you do that, it invokes a custom destruction callback which you can use to, say, run the WebCallbacks with a default error code. WebUSB uses this. > > > > It adds a layer of ugliness to wrap callbacks like this, but I haven't come up with a better alternative yet. > > As discussed offline, ScriptPromiseResolver only DCHECKS that resolve or reject have been called. In release builds the only consequence of a service dropping callbacks is that a promise will hang. In tests if a callback is dropped then the test will fail, which is basically the same as us returning an error when the callback is dropped. I think the extra layer would add little benefit.
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. Since we are no longer using mojo there could be races in our tests that rely on IPC:Message ordering. The follow up patch addresses this issue: http://crrev.com/1815483003 BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. Since we are using mojo there could be races in our tests that rely on IPC:Message ordering. The follow up patch addresses this issue: http://crrev.com/1815483003 BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:411: : outcome(CacheQueryOutcome::SUCCESS) {} I'd probably move this to a member initializer too, but that's just a matter of taste. https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:18: #include "content/common/bluetooth/bluetooth_messages.h" I don't think you're using this #include? https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:58: struct CacheQueryResult { Are these only temporarily here? If so, put a TODO saying that. https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:21: blink::mojom::WebBluetoothError GetWebError(CacheQueryOutcome outcome) { I think this duplication will also go away if you 'using' the mojo enum into blink. https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:155: callback.Run(TranslateGATTErrorAndRecord( Probably keep the comment that "// TranslateGATTError calls RecordGATTOperationOutcome." https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:52: mojo::Binding<blink::mojom::WebBluetoothService> binding_; Why is this just a 'Binding' while the testing service was a 'StrongBinding'? https://codereview.chromium.org/1775953004/diff/300001/content/child/mojo/typ... File content/child/mojo/type_converters.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/child/mojo/typ... content/child/mojo/type_converters.h:25: struct TypeConverter<String, blink::WebString> { There was something about blink-friendly bindings at https://docs.google.com/document/d/13v1Y0KkijNIc3wgiu6PCwACohoc1FYn53PIGHC4SH.... Does it apply here? https://codereview.chromium.org/1775953004/diff/300001/content/renderer/bluet... File content/renderer/bluetooth/web_bluetooth_impl.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/renderer/bluet... content/renderer/bluetooth/web_bluetooth_impl.h:81: void OnWriteValue( Maybe "OnWriteValueComplete"? https://codereview.chromium.org/1775953004/diff/300001/content/renderer/bluet... content/renderer/bluetooth/web_bluetooth_impl.h:89: ServiceRegistry* service_registry_; Make this a 'ServiceRegistry* const' if possible. https://codereview.chromium.org/1775953004/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h (right): https://codereview.chromium.org/1775953004/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h:16: enum class WebBluetoothError { Try to move all of these to the .mojom at once by replacing this file's definition with 'using mojom::WebBluetoothError;'. https://codereview.chromium.org/1775953004/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom (right): https://codereview.chromium.org/1775953004/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:34: RemoteCharacteristicWriteValue( Comment what this does.
Patchset #17 (id:320001) has been deleted
https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:411: : outcome(CacheQueryOutcome::SUCCESS) {} On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > I'd probably move this to a member initializer too, but that's just a matter of taste. Done. https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:18: #include "content/common/bluetooth/bluetooth_messages.h" On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > I don't think you're using this #include? We need both: - bluetooth_messages for blink::WebBluetoothError in GetWebError for CacheQueryResult. - bluetooth_metrics for CacheQueryOutcome in CacheQueryResult. https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:58: struct CacheQueryResult { On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > Are these only temporarily here? If so, put a TODO saying that. Done. https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:21: blink::mojom::WebBluetoothError GetWebError(CacheQueryOutcome outcome) { On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > I think this duplication will also go away if you 'using' the mojo enum into blink. Done. \o/ https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:155: callback.Run(TranslateGATTErrorAndRecord( On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > Probably keep the comment that "// TranslateGATTError calls RecordGATTOperationOutcome." It's now called TranslateGATTErrorAndRecord. Do you think the comment is still necessary? https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:52: mojo::Binding<blink::mojom::WebBluetoothService> binding_; On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > Why is this just a 'Binding' while the testing service was a 'StrongBinding'? StrongBinding will delete the service if the connection is terminated or an error occurs. I rather explicitly manage the lifetime of the service than to rely on the pipe closing. For that reason I decided to use Binding and have the Render Frame Host own this service. I added a comment for the class and for this specific member. https://codereview.chromium.org/1775953004/diff/300001/content/child/mojo/typ... File content/child/mojo/type_converters.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/child/mojo/typ... content/child/mojo/type_converters.h:25: struct TypeConverter<String, blink::WebString> { On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > There was something about blink-friendly bindings at https://docs.google.com/document/d/13v1Y0KkijNIc3wgiu6PCwACohoc1FYn53PIGHC4SH.... Does it apply here? No :( We are converting from a WebString to a mojo::String. The new bindings are for WTF::String. https://codereview.chromium.org/1775953004/diff/300001/content/renderer/bluet... File content/renderer/bluetooth/web_bluetooth_impl.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/renderer/bluet... content/renderer/bluetooth/web_bluetooth_impl.h:81: void OnWriteValue( On 2016/03/25 at 00:48:24, Jeffrey Yasskin wrote: > Maybe "OnWriteValueComplete"? Done. https://codereview.chromium.org/1775953004/diff/300001/content/renderer/bluet... content/renderer/bluetooth/web_bluetooth_impl.h:89: ServiceRegistry* service_registry_; On 2016/03/25 at 00:48:24, Jeffrey Yasskin wrote: > Make this a 'ServiceRegistry* const' if possible. Done. https://codereview.chromium.org/1775953004/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h (right): https://codereview.chromium.org/1775953004/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h:16: enum class WebBluetoothError { On 2016/03/25 at 00:48:24, Jeffrey Yasskin wrote: > Try to move all of these to the .mojom at once by replacing this file's definition with 'using mojom::WebBluetoothError;'. Done in http://crrev.com/1837253002 https://codereview.chromium.org/1775953004/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom (right): https://codereview.chromium.org/1775953004/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:34: RemoteCharacteristicWriteValue( On 2016/03/25 at 00:48:24, Jeffrey Yasskin wrote: > Comment what this does. Done.
lgtm https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:18: #include "content/common/bluetooth/bluetooth_messages.h" On 2016/03/29 17:58:34, ortuno wrote: > On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > > I don't think you're using this #include? > > We need both: > - bluetooth_messages for blink::WebBluetoothError in GetWebError for > CacheQueryResult. > - bluetooth_metrics for CacheQueryOutcome in CacheQueryResult. Now that everything's using the mojo enum, try including just third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom.h instead of the whole messages file. DEPS needs to allow that regardless, in order for WebBluetoothServiceImpl to work. https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:155: callback.Run(TranslateGATTErrorAndRecord( On 2016/03/29 17:58:34, ortuno wrote: > On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > > Probably keep the comment that "// TranslateGATTError calls > RecordGATTOperationOutcome." > > It's now called TranslateGATTErrorAndRecord. Do you think the comment is still > necessary? Good point, probably not. https://codereview.chromium.org/1775953004/diff/300001/content/child/mojo/typ... File content/child/mojo/type_converters.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/child/mojo/typ... content/child/mojo/type_converters.h:25: struct TypeConverter<String, blink::WebString> { On 2016/03/29 17:58:34, ortuno wrote: > On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > > There was something about blink-friendly bindings at > https://docs.google.com/document/d/13v1Y0KkijNIc3wgiu6PCwACohoc1FYn53PIGHC4SH.... > Does it apply here? > > No :( We are converting from a WebString to a mojo::String. The new bindings are > for WTF::String. Too bad. https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:417: blink::WebBluetoothError You don't need the blink:: here, due to the 'using' above. https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:32: // class and save it in a scope pointer, thus making the lifetime of the s/scope pointer/scoped_ptr/? You could make this a bit shorter by saying "RenderFrameHostImpl will create an instance of this class and keep ownership of it.", which implies the lifetime and avoids saying exactly how it owns the instance. https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:37: // |request|: The instance will be bound to this request. Used for mojo "this request's pipe", I think, looking at https://code.google.com/p/chromium/codesearch/#chromium/src/mojo/public/cpp/b.... I don't think you need to say this is used for mojo setup: the object just needs to be constructed with this sort of pipe. https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:66: // owns it. Using a Binding instead of a StrongBinding means that this "... owns it, so we can't use the self-deleting behavior of a StrongBinding."? https://codereview.chromium.org/1775953004/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1775953004/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2715: web_bluetooth_service_.reset( Should we DCHECK that web_bluetooth_service_ hasn't been filled in yet? https://codereview.chromium.org/1775953004/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775953004/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:26: #include "content/browser/bluetooth/web_bluetooth_service_impl.h" You only need a forward declaration in the header, as long as you include the full definition in the file that defines the destructor. You'll probably also need to add a forward declaration of blink::mojom::WebBluetoothServiceRequest and the full #include in the .cc.
https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1775953004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:18: #include "content/common/bluetooth/bluetooth_messages.h" On 2016/03/29 at 20:30:02, Jeffrey Yasskin wrote: > On 2016/03/29 17:58:34, ortuno wrote: > > On 2016/03/25 at 00:48:23, Jeffrey Yasskin wrote: > > > I don't think you're using this #include? > > > > We need both: > > - bluetooth_messages for blink::WebBluetoothError in GetWebError for > > CacheQueryResult. > > - bluetooth_metrics for CacheQueryOutcome in CacheQueryResult. > > Now that everything's using the mojo enum, try including just third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom.h instead of the whole messages file. DEPS needs to allow that regardless, in order for WebBluetoothServiceImpl to work. I do that in content/renderer/web_bluetooth_impl and content/browser/web_bluetooth_service_impl. I haven't done the same for old files e.g. bluetooth_dispatcher_host because they shouldn't know anything about mojo. https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:417: blink::WebBluetoothError On 2016/03/29 at 20:30:02, Jeffrey Yasskin wrote: > You don't need the blink:: here, due to the 'using' above. Done. https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:32: // class and save it in a scope pointer, thus making the lifetime of the On 2016/03/29 at 20:30:02, Jeffrey Yasskin wrote: > s/scope pointer/scoped_ptr/? > > You could make this a bit shorter by saying "RenderFrameHostImpl will create an instance of this class and keep ownership of it.", which implies the lifetime and avoids saying exactly how it owns the instance. Done. https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:37: // |request|: The instance will be bound to this request. Used for mojo On 2016/03/29 at 20:30:02, Jeffrey Yasskin wrote: > "this request's pipe", I think, looking at https://code.google.com/p/chromium/codesearch/#chromium/src/mojo/public/cpp/b.... > > I don't think you need to say this is used for mojo setup: the object just needs to be constructed with this sort of pipe. Done. https://codereview.chromium.org/1775953004/diff/340001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:66: // owns it. Using a Binding instead of a StrongBinding means that this On 2016/03/29 at 20:30:02, Jeffrey Yasskin wrote: > "... owns it, so we can't use the self-deleting behavior of a StrongBinding."? Done. https://codereview.chromium.org/1775953004/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1775953004/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2715: web_bluetooth_service_.reset( On 2016/03/29 at 20:30:02, Jeffrey Yasskin wrote: > Should we DCHECK that web_bluetooth_service_ hasn't been filled in yet? Done. https://codereview.chromium.org/1775953004/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775953004/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:26: #include "content/browser/bluetooth/web_bluetooth_service_impl.h" On 2016/03/29 at 20:30:02, Jeffrey Yasskin wrote: > You only need a forward declaration in the header, as long as you include the full definition in the file that defines the destructor. You'll probably also need to add a forward declaration of blink::mojom::WebBluetoothServiceRequest and the full #include in the .cc. Done.
ortuno@chromium.org changed reviewers: + jam@chromium.org, palmer@chromium.org
jam: PTAL at content/ palmer: PTAL at content/common and third_party/WebKit/public/platform/modules/bluetooth/
lgtm
lgtm https://codereview.chromium.org/1775953004/diff/380001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1775953004/diff/380001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:58: // TODO(ortuno): We temporarily make this a public struct so that Attach a bug ID to every TODO. Also typo: capital H in BluetoothDispatcherHost on line 60.
The CQ bit was checked by ortuno@chromium.org
Thanks! https://codereview.chromium.org/1775953004/diff/380001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1775953004/diff/380001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:58: // TODO(ortuno): We temporarily make this a public struct so that On 2016/03/30 at 21:34:51, palmer wrote: > Attach a bug ID to every TODO. Also typo: capital H in BluetoothDispatcherHost on line 60. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, jyasskin@chromium.org, palmer@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1775953004/#ps400001 (title: "Address palmer's comments")
The CQ bit was unchecked by ortuno@chromium.org
ortuno@chromium.org changed reviewers: + scheib@chromium.org
@scheib: PTAL
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775953004/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775953004/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
https://codereview.chromium.org/1775953004/diff/400001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/1775953004/diff/400001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:32: // ownership of it. Not thread safe, and created on the UI thread as required by device/bluetooth. https://codereview.chromium.org/1775953004/diff/400001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775953004/diff/400001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:893: scoped_ptr<WebBluetoothServiceImpl> web_bluetooth_service_; WebBluetoothServiceImpl implies that it would prefer to have a StrongBinding that had the Impl destruct when the pipe closes. It could, it seems, if RenderFrameHostImpl a weak pointer here and destroyed the WebBluetoothServiceImpl in ~RenderFrameHostImpl. But, current code pattern seems fine, which makes me think the comment is too strong in WebBluetoothServiceImpl "so we can't use the self-deleting behavior of a StrongBinding" -- it implies that there's a deficiency to not using StrongBinding. https://codereview.chromium.org/1775953004/diff/400001/content/renderer/bluet... File content/renderer/bluetooth/web_bluetooth_impl.cc (right): https://codereview.chromium.org/1775953004/diff/400001/content/renderer/bluet... content/renderer/bluetooth/web_bluetooth_impl.cc:81: base::Unretained(this), value, Unretained always makes a reader wonder, "Is it clear that this class will outlive the callback?" Well, to me it isn't 100% clear. I'm guessing it is because mojo wouldn't dispatch the callback if the class didn't still exist, but I'm not clear on when/why a reader knows that. I didn't see it in the mojo primer, it's not obvious elsewhere. We should ping mojo folk so that this pattern is either documented in mojo primer, or to have the code read in a way where we don't have this (e.g. weak pointer). https://codereview.chromium.org/1775953004/diff/400001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom (right): https://codereview.chromium.org/1775953004/diff/400001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:13: enum WebBluetoothError { Not always an Error, so Result? https://codereview.chromium.org/1775953004/diff/400001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:75: interface WebBluetoothService { In person we discussed and I thought you said the plan was a mojo interface each for adapter, device, characteristic, descriptor? https://codereview.chromium.org/1775953004/diff/400001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:83: array<uint8> value) => (WebBluetoothError error); error -> result
https://codereview.chromium.org/1775953004/diff/400001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775953004/diff/400001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:893: scoped_ptr<WebBluetoothServiceImpl> web_bluetooth_service_; On 2016/03/31 04:17:51, scheib wrote: > WebBluetoothServiceImpl implies that it would prefer to have a StrongBinding > that had the Impl destruct when the pipe closes. It could, it seems, if > RenderFrameHostImpl a weak pointer here and destroyed the > WebBluetoothServiceImpl in ~RenderFrameHostImpl. > > But, current code pattern seems fine, which makes me think the comment is too > strong in WebBluetoothServiceImpl "so we can't use the self-deleting behavior of > a StrongBinding" -- it implies that there's a deficiency to not using > StrongBinding. That wording comes from me, I think. My idea was that it's always nicer not to have a half-working object: if the pipe is closed but the object still exists, that's one extra state to deal with. If we can use a StrongBinding, then that state doesn't exist by construction. But here, we need to call into the object via the RenderFrameHost, so we either have the half-working state in WebBluetoothServiceImpl or in RenderFrameHost, and maybe it's simpler to encapsulate it in WebBluetoothServiceImpl. I don't have a strong opinion about which to use, but the comments should explain why we used the one we did.
https://codereview.chromium.org/1775953004/diff/400001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.h (right): https://codereview.chromium.org/1775953004/diff/400001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.h:32: // ownership of it. On 2016/03/31 at 04:17:51, scheib wrote: > Not thread safe, and created on the UI thread as required by device/bluetooth. Done. https://codereview.chromium.org/1775953004/diff/400001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775953004/diff/400001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:893: scoped_ptr<WebBluetoothServiceImpl> web_bluetooth_service_; On 2016/03/31 at 15:26:16, Jeffrey Yasskin wrote: > On 2016/03/31 04:17:51, scheib wrote: > > WebBluetoothServiceImpl implies that it would prefer to have a StrongBinding > > that had the Impl destruct when the pipe closes. It could, it seems, if > > RenderFrameHostImpl a weak pointer here and destroyed the > > WebBluetoothServiceImpl in ~RenderFrameHostImpl. > > > > But, current code pattern seems fine, which makes me think the comment is too > > strong in WebBluetoothServiceImpl "so we can't use the self-deleting behavior of > > a StrongBinding" -- it implies that there's a deficiency to not using > > StrongBinding. > > That wording comes from me, I think. My idea was that it's always nicer not to have a half-working object: if the pipe is closed but the object still exists, that's one extra state to deal with. If we can use a StrongBinding, then that state doesn't exist by construction. But here, we need to call into the object via the RenderFrameHost, so we either have the half-working state in WebBluetoothServiceImpl or in RenderFrameHost, and maybe it's simpler to encapsulate it in WebBluetoothServiceImpl. I don't have a strong opinion about which to use, but the comments should explain why we used the one we did. When I was designing this I had to decide between using a "Binding" and having the frame own the Service or using a StrongBinding and juggle the lifetime of the service between the pipe closing and the frame being removed (I would have had to do this by implementing a couple of functions of WebContentsObserver; I hadn't considered using a WeakPtr in RenderFrameHostImpl) I thought that making Frame own the service would make it much easier to reason about the lifetime of the service and would result in less code. I changed the comment for binding_. https://codereview.chromium.org/1775953004/diff/400001/content/renderer/bluet... File content/renderer/bluetooth/web_bluetooth_impl.cc (right): https://codereview.chromium.org/1775953004/diff/400001/content/renderer/bluet... content/renderer/bluetooth/web_bluetooth_impl.cc:81: base::Unretained(this), value, On 2016/03/31 at 04:17:51, scheib wrote: > Unretained always makes a reader wonder, "Is it clear that this class will outlive the callback?" Well, to me it isn't 100% clear. I'm guessing it is because mojo wouldn't dispatch the callback if the class didn't still exist, but I'm not clear on when/why a reader knows that. I didn't see it in the mojo primer, it's not obvious elsewhere. We should ping mojo folk so that this pattern is either documented in mojo primer, or to have the code read in a way where we don't have this (e.g. weak pointer). I asked the same thing in patchset #3 bluetooth_dispatcher.cc: On 2016/03/09 at 01:21:23, ortuno wrote: >> This is safe because if the class gets destroyed the InterfacePtr will also be destroyed and no messages will arrive right? Or do I need to make a WeakPtrFactory? > Correct, the callback will never be invoked if the BluetoothServerPtr has been reset or destroyed. I'll talk to rockot to get this in the mojo primer. Also opened http://crbug.com/599498 so that I don't forget. https://codereview.chromium.org/1775953004/diff/400001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom (right): https://codereview.chromium.org/1775953004/diff/400001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:13: enum WebBluetoothError { On 2016/03/31 at 04:17:51, scheib wrote: > Not always an Error, so Result? Opened http://crbug.com/599489 https://codereview.chromium.org/1775953004/diff/400001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:75: interface WebBluetoothService { On 2016/03/31 at 04:17:51, scheib wrote: > In person we discussed and I thought you said the plan was a mojo interface each for adapter, device, characteristic, descriptor? Yes. That's the long term plan. Since that + mojo would be too much to refactor at once, I started by only converting from IPC::Message to Mojo. https://codereview.chromium.org/1775953004/diff/400001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:83: array<uint8> value) => (WebBluetoothError error); On 2016/03/31 at 04:17:51, scheib wrote: > error -> result Will do in http://crbug.com/599489
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775953004/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775953004/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, jyasskin@chromium.org, rockot@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1775953004/#ps420001 (title: "Address scheib's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775953004/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775953004/420001
Message was sent while issue was closed.
Committed patchset #21 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. Since we are using mojo there could be races in our tests that rely on IPC:Message ordering. The follow up patch addresses this issue: http://crrev.com/1815483003 BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== bluetooth: Move writeValue to mojo First step towards moving Web Bluetooth from IPC::Message to Mojo. We will move each functions from BluetoothDispatcherHost to the new BluetoothService one by one. 1. Adds WebBluetoothService to web_bluetooth.mojom. This service will replace BluetoothDispatcherHost. 2. Implements the interface in content/browser/bluetooth: WebBluetoothServiceImpl. For now it only implements RemoteCharacteristicWrite. 3. Modifies WebBluetoothImpl/BluetoothDispatcher so that WebBluetoothImpl can acquire an InterfacePtr to the new BluetoothService. Since we are using mojo there could be races in our tests that rely on IPC:Message ordering. The follow up patch addresses this issue: http://crrev.com/1815483003 BUG=508771 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/ad6b0feaf45b761f180d0b19a1dfae072e0df3b5 Cr-Commit-Position: refs/heads/master@{#384345} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/ad6b0feaf45b761f180d0b19a1dfae072e0df3b5 Cr-Commit-Position: refs/heads/master@{#384345} |
