Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1120373004:
bluetooth: Browser-side implementation of connectGATT. (Closed)
|
Created:
5 years, 7 months ago by ortuno Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, Jeffrey Yasskin, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bluetooth-request-device-implementation Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Partial implementation of connectGATT.
Now connectGATT connects to the BLE device. This is the first
step towards fully implementing connectGATT.
Also adds BluetoothMsg_ConnectGATTError.
Layout tests are in:
http://crrev.com/1150523004
BUG=421668
Committed: https://crrev.com/87896a75a8d3e794a5ab7333fc0d249854d3124e
Cr-Commit-Position: refs/heads/master@{#330883}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added ConnectGATTError #Patch Set 3 : Moved function to match header #Patch Set 4 : Tests and cleanup #
Total comments: 13
Patch Set 5 : Address scheib's comments #
Total comments: 18
Patch Set 6 : Remove connection variable #Patch Set 7 : Address jyasskin's comments #
Messages
Total messages: 30 (12 generated)
ortuno@chromium.org changed reviewers: + scheib@chromium.org
@scheib: Please review.
Patches with lots of TODO comments are a hint that the other patches perhaps should be done sooner. Exception for e.g. the larger item of dealing with origin security. https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... content/browser/bluetooth/bluetooth_dispatcher_host.cc:133: // TODO(ortuno): Right now it's pointless to check if the domain has access to Find or file an issue for this security concept, reference it in comment, mark it as blocking https://code.google.com/p/chromium/issues/detail?id=436283 https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... content/browser/bluetooth/bluetooth_dispatcher_host.cc:140: // a set and update it based on events from the adapter. Probably we'll keep a map of origins, each with a map to allowed devices. I'm not sure we'll need to keep a general set of devices the adapter has discovered here, as we'll wait for scan timeout and then might as well use the set of devices the adapter is already storing. However, the UI implementation will need to track devices as they're discovered. https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... content/browser/bluetooth/bluetooth_dispatcher_host.cc:176: // TODO(ortuno): Change to NetworkError once it's implemented. As we start to implement the algorithm details, let's keep references spec sections that define the behavior and add a note in http://webbluetoothchrome.github.io/web-bluetooth/ about the reference. Same above. https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... content/browser/bluetooth/bluetooth_dispatcher_host.h:53: ?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
@scheib: Please review again. On 2015/05/05 at 04:00:43, scheib wrote: > Patches with lots of TODO comments are a hint that the other patches perhaps should be done sooner. Exception for e.g. the larger item of dealing with origin security. I removed one of the TODOs. I just didn't want this to be blocked on a 3-way patch for NetworkError. > https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... > File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): > > https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:133: // TODO(ortuno): Right now it's pointless to check if the domain has access to > Find or file an issue for this security concept, reference it in comment, mark it as blocking https://code.google.com/p/chromium/issues/detail?id=436283 Done. > https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:140: // a set and update it based on events from the adapter. > Probably we'll keep a map of origins, each with a map to allowed devices. I'm not sure we'll need to keep a general set of devices the adapter has discovered here, as we'll wait for scan timeout and then might as well use the set of devices the adapter is already storing. However, the UI implementation will need to track devices as they're discovered. Done. > https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:176: // TODO(ortuno): Change to NetworkError once it's implemented. > As we start to implement the algorithm details, let's keep references spec sections that define the behavior and add a note in http://webbluetoothchrome.github.io/web-bluetooth/ about the reference. Same above. Done. I have a PR ready for when this CL gets committed. > https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... > File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): > > https://codereview.chromium.org/1120373004/diff/1/content/browser/bluetooth/b... > content/browser/bluetooth/bluetooth_dispatcher_host.h:53: > ? Done.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Put this patch on hold until we have a solution to implement mock adapters to test with.
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
@scheib: Tests added.
LGTM with a few requests: https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:55: ACTION_P(GetMockDevice, adapter) { Comment https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:184: BluetoothDevice* device_ptr = device.get(); This is OK, but it may be simpler to read the block below by extracting the device address here closing over it instead of the device and then extracting the address inside the closure. https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:79: // - |CreateGattConnection| runs first argument with ... runs success callback ... https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:86: // - |CreateGattConnection| runs second argument with ... runs error callback ...
https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:55: ACTION_P(GetMockDevice, adapter) { On 2015/05/20 at 20:14:08, scheib wrote: > Comment Done. https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:184: BluetoothDevice* device_ptr = device.get(); On 2015/05/20 at 20:14:08, scheib wrote: > This is OK, but it may be simpler to read the block below by extracting the device address here closing over it instead of the device and then extracting the address inside the closure. Hmm this way we avoid creating a copy of the address though. https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:79: // - |CreateGattConnection| runs first argument with On 2015/05/20 at 20:14:08, scheib wrote: > ... runs success callback ... Done. https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:86: // - |CreateGattConnection| runs second argument with On 2015/05/20 at 20:14:08, scheib wrote: > ... runs error callback ... Done.
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org, tsepez@chromium.org
@tsepez: Please OWNERS review for common/bluetooth/bluetooth_messages.h @jyasskin: PTAL.
https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:184: BluetoothDevice* device_ptr = device.get(); On 2015/05/20 20:54:47, ortuno wrote: > On 2015/05/20 at 20:14:08, scheib wrote: > > This is OK, but it may be simpler to read the block below by extracting the > device address here closing over it instead of the device and then extracting > the address inside the closure. > > Hmm this way we avoid creating a copy of the address though. To elaborate, we have to capture the address by value because capturing it by reference is only valid if a particular C++ defect is resolved in the right way: http://open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2011. I suggested that we always close over the effective "this" pointer in ON_CALL expressions instead of trying to pick the right fields to extract from it separately at each use, but I'm not dead set on that.
https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:184: BluetoothDevice* device_ptr = device.get(); On 2015/05/20 21:14:07, Jeffrey Yasskin wrote: > On 2015/05/20 20:54:47, ortuno wrote: > > On 2015/05/20 at 20:14:08, scheib wrote: > > > This is OK, but it may be simpler to read the block below by extracting the > > device address here closing over it instead of the device and then extracting > > the address inside the closure. > > > > Hmm this way we avoid creating a copy of the address though. > > To elaborate, we have to capture the address by value because capturing it by > reference is only valid if a particular C++ defect is resolved in the right way: > http://open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2011. > > I suggested that we always close over the effective "this" pointer in ON_CALL > expressions instead of trying to pick the right fields to extract from it > separately at each use, but I'm not dead set on that. I'm fine either way. I thought capturing the address string by value would be just fine (I don't think it'll change, and I don't think the copy is excessive to do). I figured it would simplify the code a bit in the closure, thinking that it's short enough and nested in this code blob below, that the shorter we make it the easier it is to read. If multiple fields are used I agree it seems simpler to close over just the object pointer and extract inline. Anyhoo, it would save a line in this somewhat dense and hard to read bit. Also, we can return the scope_ptr directly: device_ptr: ON_CALL(*device, CreateGattConnection(_, _)) .WillByDefault( RunCallbackWithResult<0 /* success_callback */>([device_ptr]() { return scoped_ptr<NiceMock<MockBluetoothGattConnection>> ( new NiceMock<MockBluetoothGattConnection>( device_ptr->GetAddress())); })); address: ON_CALL(*device, CreateGattConnection(_, _)) .WillByDefault( RunCallbackWithResult<0 /* success_callback */>([address]() { return scoped_ptr<NiceMock<MockBluetoothGattConnection>> ( new NiceMock<MockBluetoothGattConnection>(address)); }));
lgtm Messages LGTM.
https://codereview.chromium.org/1120373004/diff/240001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1120373004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:110: // NetworkError. Is a device_instance_id that doesn't map to a device something that a valid renderer would do? It seems like renderers should really just be reflecting instance IDs back to the browser, and if they send an instance ID that the browser has never seen before, the right thing to do is to infer that the renderer is hostile and kill it. Maybe comment what would cause a null device given a valid instance ID? https://codereview.chromium.org/1120373004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:208: // NetworkError. Please give the eventual Javascript error a descriptive message so that developers can debug their connection errors. The message isn't specified in the standard, but that's a place each browser should try to do its best. https://codereview.chromium.org/1120373004/diff/240001/content/child/bluetoot... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1120373004/diff/240001/content/child/bluetoot... content/child/bluetooth/bluetooth_dispatcher.cc:174: WebBluetoothErrorFromBluetoothError(error_type), "")); "" is bad here, since it leaves developers guessing. At least add a TODO to fill in real strings. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (left): https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:122: LayoutTestBluetoothAdapterProvider::GetDiscoverySession() { No need to revert it here, but try not to reorder functions in the same change that you're also adding new functions. It makes it harder to see what's changed. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:59: for (auto& device : adapter->GetMockDevices()) { Can you spell out "const BluetoothDevice*" here instead of using 'auto&'? Using 'auto&' to capture a pointer is a little misleading, and there are few enough types in an ACTION that it's helpful to write one down to ground the rest of the function. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:112: ON_CALL(*adapter, GetDevice(_)).WillByDefault(GetMockDevice(adapter.get())); I would probably have GetMockDevice() take *adapter rather than adapter.get(), since you don't need a non-const or null adapter. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:9: #include "base/strings/utf_string_conversions.h" Do you still need this in the header? https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:79: // - |CreateGattConnection| runs sucess callback with a sp: sucess
https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:184: BluetoothDevice* device_ptr = device.get(); On 2015/05/20 21:32:13, scheib wrote: > On 2015/05/20 21:14:07, Jeffrey Yasskin wrote: > > On 2015/05/20 20:54:47, ortuno wrote: > > > On 2015/05/20 at 20:14:08, scheib wrote: > > > > This is OK, but it may be simpler to read the block below by extracting > the > > > device address here closing over it instead of the device and then > extracting > > > the address inside the closure. > > > > > > Hmm this way we avoid creating a copy of the address though. > > > > To elaborate, we have to capture the address by value because capturing it by > > reference is only valid if a particular C++ defect is resolved in the right > way: > > http://open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2011. > > > > I suggested that we always close over the effective "this" pointer in ON_CALL > > expressions instead of trying to pick the right fields to extract from it > > separately at each use, but I'm not dead set on that. > > I'm fine either way. I thought capturing the address string by value would be > just fine (I don't think it'll change, and I don't think the copy is excessive > to do). I figured it would simplify the code a bit in the closure, thinking that > it's short enough and nested in this code blob below, that the shorter we make > it the easier it is to read. > > If multiple fields are used I agree it seems simpler to close over just the > object pointer and extract inline. > > Anyhoo, it would save a line in this somewhat dense and hard to read bit. Also, > we can return the scope_ptr directly: > > device_ptr: > ON_CALL(*device, CreateGattConnection(_, _)) > .WillByDefault( > RunCallbackWithResult<0 /* success_callback */>([device_ptr]() { > return scoped_ptr<NiceMock<MockBluetoothGattConnection>> ( > new NiceMock<MockBluetoothGattConnection>( > device_ptr->GetAddress())); > })); > > address: > ON_CALL(*device, CreateGattConnection(_, _)) > .WillByDefault( > RunCallbackWithResult<0 /* success_callback */>([address]() { > return scoped_ptr<NiceMock<MockBluetoothGattConnection>> ( > new NiceMock<MockBluetoothGattConnection>(address)); > })); make_scoped_ptr() would also make this shorter: https://code.google.com/p/chromium/codesearch/#chromium/src/base/memory/scope... It might also be more readable to put the [whatever]() on the next line, so that it's clearer we're writing a function here.
Ready for review again. https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:184: BluetoothDevice* device_ptr = device.get(); On 2015/05/20 at 22:29:25, Jeffrey Yasskin wrote: > On 2015/05/20 21:32:13, scheib wrote: > > On 2015/05/20 21:14:07, Jeffrey Yasskin wrote: > > > On 2015/05/20 20:54:47, ortuno wrote: > > > > On 2015/05/20 at 20:14:08, scheib wrote: > > > > > This is OK, but it may be simpler to read the block below by extracting > > the > > > > device address here closing over it instead of the device and then > > extracting > > > > the address inside the closure. > > > > > > > > Hmm this way we avoid creating a copy of the address though. > > > > > > To elaborate, we have to capture the address by value because capturing it by > > > reference is only valid if a particular C++ defect is resolved in the right > > way: > > > http://open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2011. > > > > > > I suggested that we always close over the effective "this" pointer in ON_CALL > > > expressions instead of trying to pick the right fields to extract from it > > > separately at each use, but I'm not dead set on that. > > > > I'm fine either way. I thought capturing the address string by value would be > > just fine (I don't think it'll change, and I don't think the copy is excessive > > to do). I figured it would simplify the code a bit in the closure, thinking that > > it's short enough and nested in this code blob below, that the shorter we make > > it the easier it is to read. > > > > If multiple fields are used I agree it seems simpler to close over just the > > object pointer and extract inline. > > > > Anyhoo, it would save a line in this somewhat dense and hard to read bit. Also, > > we can return the scope_ptr directly: > > > > device_ptr: > > ON_CALL(*device, CreateGattConnection(_, _)) > > .WillByDefault( > > RunCallbackWithResult<0 /* success_callback */>([device_ptr]() { > > return scoped_ptr<NiceMock<MockBluetoothGattConnection>> ( > > new NiceMock<MockBluetoothGattConnection>( > > device_ptr->GetAddress())); > > })); > > > > address: > > ON_CALL(*device, CreateGattConnection(_, _)) > > .WillByDefault( > > RunCallbackWithResult<0 /* success_callback */>([address]() { > > return scoped_ptr<NiceMock<MockBluetoothGattConnection>> ( > > new NiceMock<MockBluetoothGattConnection>(address)); > > })); > > make_scoped_ptr() would also make this shorter: > https://code.google.com/p/chromium/codesearch/#chromium/src/base/memory/scope... > Done. > It might also be more readable to put the [whatever]() on the next line, so that it's clearer we're writing a function here. I tried to but git cl format always puts it in the same line. https://codereview.chromium.org/1120373004/diff/240001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1120373004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:110: // NetworkError. On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > Is a device_instance_id that doesn't map to a device something that a valid renderer would do? It seems like renderers should really just be reflecting instance IDs back to the browser, and if they send an instance ID that the browser has never seen before, the right thing to do is to infer that the renderer is hostile and kill it. > > Maybe comment what would cause a null device given a valid instance ID? Yes, the renderer could send a device_instance_id that is no longer in BluetoothAdapter. For example a user chooses a device which is then returned by requestDevice. If the user walks away from the device and the device is no longer in range the device will no longer be in BluetoothAdapter by the time JS tries to connect to the device. https://codereview.chromium.org/1120373004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:208: // NetworkError. On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > Please give the eventual Javascript error a descriptive message so that developers can debug their connection errors. The message isn't specified in the standard, but that's a place each browser should try to do its best. Added a todo and opened an issue: http://crbug.com/490419 https://codereview.chromium.org/1120373004/diff/240001/content/child/bluetoot... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1120373004/diff/240001/content/child/bluetoot... content/child/bluetooth/bluetooth_dispatcher.cc:174: WebBluetoothErrorFromBluetoothError(error_type), "")); On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > "" is bad here, since it leaves developers guessing. At least add a TODO to fill in real strings. TODO added. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (left): https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:122: LayoutTestBluetoothAdapterProvider::GetDiscoverySession() { On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > No need to revert it here, but try not to reorder functions in the same change that you're also adding new functions. It makes it harder to see what's changed. Sorry. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:59: for (auto& device : adapter->GetMockDevices()) { On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > Can you spell out "const BluetoothDevice*" here instead of using 'auto&'? Using 'auto&' to capture a pointer is a little misleading, and there are few enough types in an ACTION that it's helpful to write one down to ground the rest of the function. It's actually only BluetoothDevice*. Done. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:112: ON_CALL(*adapter, GetDevice(_)).WillByDefault(GetMockDevice(adapter.get())); On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > I would probably have GetMockDevice() take *adapter rather than adapter.get(), since you don't need a non-const or null adapter. GetMockDevice(*adapter) tries to copy adapter but you can't copy a NiceMock so it fails to compile. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:9: #include "base/strings/utf_string_conversions.h" On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > Do you still need this in the header? Done. https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:79: // - |CreateGattConnection| runs sucess callback with a On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > sp: sucess Done.
LGTM, thanks! https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/220001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:184: BluetoothDevice* device_ptr = device.get(); On 2015/05/20 23:35:19, ortuno wrote: > On 2015/05/20 at 22:29:25, Jeffrey Yasskin wrote: > > It might also be more readable to put the [whatever]() on the next line, so > that it's clearer we're writing a function here. > > I tried to but git cl format always puts it in the same line. Good point. I don't think it's worth diverging from `git cl format`. https://codereview.chromium.org/1120373004/diff/240001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1120373004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:110: // NetworkError. On 2015/05/20 23:35:19, ortuno wrote: > On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > > Is a device_instance_id that doesn't map to a device something that a valid > renderer would do? It seems like renderers should really just be reflecting > instance IDs back to the browser, and if they send an instance ID that the > browser has never seen before, the right thing to do is to infer that the > renderer is hostile and kill it. > > > > Maybe comment what would cause a null device given a valid instance ID? > > Yes, the renderer could send a device_instance_id that is no longer in > BluetoothAdapter. For example a user chooses a device which is then returned by > requestDevice. If the user walks away from the device and the device is no > longer in range the device will no longer be in BluetoothAdapter by the time JS > tries to connect to the device. And walking out of range is exactly when we'd want to return NetworkError. Sounds good. :) https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1120373004/diff/240001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:112: ON_CALL(*adapter, GetDevice(_)).WillByDefault(GetMockDevice(adapter.get())); On 2015/05/20 23:35:19, ortuno wrote: > On 2015/05/20 at 22:19:48, Jeffrey Yasskin wrote: > > I would probably have GetMockDevice() take *adapter rather than adapter.get(), > since you don't need a non-const or null adapter. > > GetMockDevice(*adapter) tries to copy adapter but you can't copy a NiceMock so > it fails to compile. Aha! Never mind then.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1120373004/#ps280001 (title: "Address jyasskin's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1120373004/280001
Message was sent while issue was closed.
Committed patchset #7 (id:280001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/87896a75a8d3e794a5ab7333fc0d249854d3124e Cr-Commit-Position: refs/heads/master@{#330883} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
