Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2030973002:
bluetooth: Handle two consecutives requestDevice calls. (Closed)
|
Created:
4 years, 6 months ago by ortuno Modified:
4 years, 6 months ago Reviewers:
Jeffrey Yasskin CC:
blink-reviews, chromium-reviews, darin-cc_chromium.org, jam, ortuno+watch_chromium.org, scheib+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: Handle two consecutives requestDevice calls.
WebBluetoothServiceImpl will close any open choosers when calling
RequestDevice. BluetoothDeviceChooserController runs its error callback
when being destroyed if there are any open chooser.
BUG=616222
Committed: https://crrev.com/8eba1a9d135b69979dd20c1c8c49f4cfdb3ee6d7
Cr-Commit-Position: refs/heads/master@{#399836}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : Address jyasskin's comments #Patch Set 4 : Fix typo #Patch Set 5 : Improve comment #
Messages
Total messages: 34 (14 generated)
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/2030973002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin: PTAL
https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:223: error_callback_.Run(blink::mojom::WebBluetoothError::CHOOSER_CANCELLED); DCHECK(!error_callback_.is_null())? It'll crash in .Run() anyway, but it's nice to see that we think there's an invariant that chooser_ implies a non-null callback. https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/web_bluetooth_service_impl.cc:656: device_chooser_controller_.reset(); Can you just delete this line? That'll wind up destroying the old controller after the new one is created, but I don't think that has an effect. If it is important to keep the destroy, create order, comment why. https://codereview.chromium.org/2030973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/consecutive-calls.html (right): https://codereview.chromium.org/2030973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/requestDevice/consecutive-calls.html:8: let assert_expected_events = events => { Write this as function assert_expected_events(events) { ... } The let = => form is most useful when you need to capture 'this'.
lgtm
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/2030973002/40001
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/2030973002/60001
Thanks! https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:223: error_callback_.Run(blink::mojom::WebBluetoothError::CHOOSER_CANCELLED); On 2016/06/10 at 00:49:22, Jeffrey Yasskin wrote: > DCHECK(!error_callback_.is_null())? It'll crash in .Run() anyway, but it's nice to see that we think there's an invariant that chooser_ implies a non-null callback. Done. https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/web_bluetooth_service_impl.cc:656: device_chooser_controller_.reset(); On 2016/06/10 at 00:49:22, Jeffrey Yasskin wrote: > Can you just delete this line? That'll wind up destroying the old controller after the new one is created, but I don't think that has an effect. If it is important to keep the destroy, create order, comment why. I revised the comment. Let me know if you don't think that's enough. https://codereview.chromium.org/2030973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/consecutive-calls.html (right): https://codereview.chromium.org/2030973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/requestDevice/consecutive-calls.html:8: let assert_expected_events = events => { On 2016/06/10 at 00:49:22, Jeffrey Yasskin wrote: > Write this as > function assert_expected_events(events) { ... } > > The let = => form is most useful when you need to capture 'this'. Done.
https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/web_bluetooth_service_impl.cc:656: device_chooser_controller_.reset(); On 2016/06/10 18:10:07, ortuno wrote: > On 2016/06/10 at 00:49:22, Jeffrey Yasskin wrote: > > Can you just delete this line? That'll wind up destroying the old controller > after the new one is created, but I don't think that has an effect. If it is > important to keep the destroy, create order, comment why. > > I revised the comment. Let me know if you don't think that's enough. Well, the second chooser doesn't open until you call ->GetDevice(), which definitely happens after the old one is destroyed. It's only the construction of the object that would happen earlier, and that doesn't look like it has any side-effects. We could say something like "so we don't DCHECK that the old one is closed. We destroy the old chooser before constructing the new one to make sure they can't conflict."
https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2030973002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/web_bluetooth_service_impl.cc:656: device_chooser_controller_.reset(); On 2016/06/10 at 18:47:18, Jeffrey Yasskin wrote: > On 2016/06/10 18:10:07, ortuno wrote: > > On 2016/06/10 at 00:49:22, Jeffrey Yasskin wrote: > > > Can you just delete this line? That'll wind up destroying the old controller > > after the new one is created, but I don't think that has an effect. If it is > > important to keep the destroy, create order, comment why. > > > > I revised the comment. Let me know if you don't think that's enough. > > Well, the second chooser doesn't open until you call ->GetDevice(), which definitely happens after the old one is destroyed. It's only the construction of the object that would happen earlier, and that doesn't look like it has any side-effects. > > We could say something like "so we don't DCHECK that the old one is closed. We destroy the old chooser before constructing the new one to make sure they can't conflict." Done. Thanks for the suggestion!
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/2030973002/#ps80001 (title: "Improve comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030973002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030973002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030973002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030973002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Handle two consecutives requestDevice calls. WebBluetoothServiceImpl will close any open choosers when calling RequestDevice. BluetoothDeviceChooserController runs its error callback when being destroyed if there are any open chooser. BUG=616222 ========== to ========== bluetooth: Handle two consecutives requestDevice calls. WebBluetoothServiceImpl will close any open choosers when calling RequestDevice. BluetoothDeviceChooserController runs its error callback when being destroyed if there are any open chooser. BUG=616222 Committed: https://crrev.com/8eba1a9d135b69979dd20c1c8c49f4cfdb3ee6d7 Cr-Commit-Position: refs/heads/master@{#399836} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8eba1a9d135b69979dd20c1c8c49f4cfdb3ee6d7 Cr-Commit-Position: refs/heads/master@{#399836} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
