Issue 2510323002: bluetooth: Implement acceptAllDevices

ortuno

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

4 years, 1 month ago (2016-11-18 04:21:02 UTC) #1

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 04:22:02 UTC) #2

ortuno

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

4 years, 1 month ago (2016-11-18 05:17:51 UTC) #3

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 05:55:07 UTC) #5

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 05:55:08 UTC) #6

haraken

haraken@chromium.org changed reviewers: + haraken@chromium.org

4 years, 1 month ago (2016-11-22 03:58:41 UTC) #8

haraken

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right): https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp#newcode114 third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:114: if (options.filters().isEmpty()) { Is it possible that isEmpty returns ...

4 years, 1 month ago (2016-11-22 03:58:42 UTC) #9

ortuno

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right): https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp#newcode114 third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:114: if (options.filters().isEmpty()) { On 2016/11/22 at 03:58:42, haraken wrote: ...

4 years, 1 month ago (2016-11-22 04:00:12 UTC) #10

scheib

https://codereview.chromium.org/2510323002/diff/20001/content/browser/bluetooth/bluetooth_device_chooser_controller.cc File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2510323002/diff/20001/content/browser/bluetooth/bluetooth_device_chooser_controller.cc#newcode122 content/browser/bluetooth/bluetooth_device_chooser_controller.cc:122: (options->filters.is_null() && !options->accept_all_devices)) { 122 is redundant with 125,126 ...

4 years ago (2016-11-22 21:22:44 UTC) #11

ortuno

https://codereview.chromium.org/2510323002/diff/20001/content/browser/bluetooth/bluetooth_device_chooser_controller.cc File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2510323002/diff/20001/content/browser/bluetooth/bluetooth_device_chooser_controller.cc#newcode122 content/browser/bluetooth/bluetooth_device_chooser_controller.cc:122: (options->filters.is_null() && !options->accept_all_devices)) { On 2016/11/22 at 21:22:44, scheib ...

4 years ago (2016-11-23 06:03:08 UTC) #12

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

https://codereview.chromium.org/2510323002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:122:
(options->filters.is_null() && !options->accept_all_devices)) {
On 2016/11/22 at 21:22:44, scheib wrote:
> 122 is redundant with 125,126 I think. Would this whole method be easier to
read as:
> 
> if (options->accept_all_devices)
>   return !options->filters.is_null();
> else
>   return HasEmptyOrInvalidFilter(options->filters);

Nice. Done.

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/requestDevice/acceptAllDevices/no-optional-services.html
(right):

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice/acceptAllDevices/no-optional-services.html:1:
<!DOCTYPE html>
On 2016/11/22 at 21:22:44, scheib wrote:
> it's OK, but fyi I try to name the files so that variants are adjacent, e.g.
optional-services-found, optional-services-missing

That's much better, thanks. Done

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/requestDevice/canonicalizeFilter/filters-and-acceptAllDevices.html
(right):

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice/canonicalizeFilter/filters-and-acceptAllDevices.html:1:
<!DOCTYPE html>
On 2016/11/22 at 21:22:44, scheib wrote:
> filename maybe: filters-xor-acceptAllDevices.html

Done.

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

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:103: if
((options.hasFilters() && options.acceptAllDevices()) ||
On 2016/11/22 at 21:22:44, scheib wrote:
> Do you find either of these easier to read?
> 
>   if (!(options.hasFilters() ^ options.acceptAllDevices()))
> 
>   if (options.hasFilters() == options.acceptAllDevices())

Went with the first one. Done.

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:207:
WebRequestDeviceOptions webOptions;
On 2016/11/22 at 21:22:44, scheib wrote:
> Some of these comments helped explain why these steps were happening :|

hmm I think the code is pretty clear already but either way I added the comments
back and removed the number since those are no longer correct.

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/bluetooth/WebRequestDeviceOptions.h
(right):

https://codereview.chromium.org/2510323002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebRequestDeviceOptions.h:33:
bool hasFilters;
On 2016/11/22 at 21:22:44, scheib wrote:
> I don't think we need hasFilters in WebRequestDeviceOptions, just use
acceptAllDevices.

hmm I was trying to make things in bluetooth_type_converters.cc more explicit.
If we remove hasFilters bluetooth_type_converters.cc becomes:

if (!options->accept_all_devices) {
  options->filters = mojo::Array<...>::From(web_options.filters);
}

as opposed to:

if (web_options.hasFilters) {
  options->filters = mojo::Array<...>::From(web_options.filters);
}

Both WebRequestDeviceOptions.cc and bluetooth_type_converters.cc are going away
with Onion Soup so I don't mind either way. Up to you.

commit-bot: I haz the power

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

4 years ago (2016-11-23 06:03:28 UTC) #14

commit-bot: I haz the power

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

4 years ago (2016-11-23 06:05:57 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/341358)

4 years ago (2016-11-23 06:05:58 UTC) #16

ortuno

ortuno@chromium.org changed reviewers: + isherman@chromium.org, nick@chromium.org

4 years ago (2016-11-23 21:55:35 UTC) #18

ortuno

nick: PTAL a content/browser/bad_message.h isherman: PTAL at t/m/h/histograms.xml

4 years ago (2016-11-23 21:55:36 UTC) #19

commit-bot: I haz the power

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

4 years ago (2016-11-23 22:42:26 UTC) #22

commit-bot: I haz the power

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

4 years ago (2016-11-23 23:34:21 UTC) #23

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/341902)

4 years ago (2016-11-23 23:34:23 UTC) #24

commit-bot: I haz the power

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

4 years ago (2016-11-24 03:06:19 UTC) #26

commit-bot: I haz the power

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

4 years ago (2016-11-24 04:50:56 UTC) #27

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/342079)

4 years ago (2016-11-24 04:50:57 UTC) #28

Ilya Sherman

https://codereview.chromium.org/2510323002/diff/80001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2510323002/diff/80001/tools/metrics/histograms/histograms.xml#newcode4834 tools/metrics/histograms/histograms.xml:4834: + enum="Boolean"> Please provide a custom boolean enum, with ...

4 years ago (2016-11-29 03:21:40 UTC) #29

ortuno

https://codereview.chromium.org/2510323002/diff/80001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2510323002/diff/80001/tools/metrics/histograms/histograms.xml#newcode4834 tools/metrics/histograms/histograms.xml:4834: + enum="Boolean"> On 2016/11/29 at 03:21:40, Ilya Sherman wrote: ...

4 years ago (2016-11-29 04:48:10 UTC) #30

commit-bot: I haz the power

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

4 years ago (2016-11-29 04:48:38 UTC) #32

commit-bot: I haz the power

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

4 years ago (2016-11-29 04:51:15 UTC) #33

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/114545) mac_chromium_rel_ng on ...

4 years ago (2016-11-29 04:51:17 UTC) #34

Ilya Sherman

metrics lgtm https://codereview.chromium.org/2510323002/diff/80001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2510323002/diff/80001/tools/metrics/histograms/histograms.xml#newcode4834 tools/metrics/histograms/histograms.xml:4834: + enum="Boolean"> On 2016/11/29 04:48:10, ortuno wrote: ...

4 years ago (2016-11-29 04:54:15 UTC) #35

ortuno

scheib: PTAL again since we've changed to stl containers. dcheng: Security review please.

4 years ago (2016-11-30 23:32:03 UTC) #36

commit-bot: I haz the power

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

4 years ago (2016-11-30 23:32:48 UTC) #38

commit-bot: I haz the power

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

4 years ago (2016-12-01 01:45:43 UTC) #39

dcheng

mojo lgtm https://codereview.chromium.org/2510323002/diff/120001/content/renderer/bluetooth/bluetooth_type_converters.cc File content/renderer/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2510323002/diff/120001/content/renderer/bluetooth/bluetooth_type_converters.cc#newcode46 content/renderer/bluetooth/bluetooth_type_converters.cc:46: options->filters = base::make_optional( I think it would ...

4 years ago (2016-12-03 01:23:00 UTC) #44

ortuno

Thanks! https://codereview.chromium.org/2510323002/diff/120001/content/renderer/bluetooth/bluetooth_type_converters.cc File content/renderer/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2510323002/diff/120001/content/renderer/bluetooth/bluetooth_type_converters.cc#newcode46 content/renderer/bluetooth/bluetooth_type_converters.cc:46: options->filters = base::make_optional( On 2016/12/03 at 01:23:00, dcheng ...

4 years ago (2016-12-04 22:33:38 UTC) #45

ortuno

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

4 years ago (2016-12-04 22:33:42 UTC) #47

commit-bot: I haz the power

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

4 years ago (2016-12-04 22:33:58 UTC) #48

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480890821862590, "parent_rev": "76630420d64b6975cb7fc6a47999efea231557fe", "commit_rev": "a4967febaf82572adce803a49e1e3372448462f3"}

4 years ago (2016-12-05 00:24:45 UTC) #49

commit-bot: I haz the power

Description was changed from ========== bluetooth: Implement acceptAllDevices When acceptAllDevices is true the chooser will ...

4 years ago (2016-12-05 00:27:06 UTC) #51

commit-bot: I haz the power

4 years ago (2016-12-05 00:27:07 UTC) #52

Message was sent while issue was closed.

Patchset 8 (id:??) landed as
https://crrev.com/cfcd3337a6ede677e625c08f10417569c94d46a6
Cr-Commit-Position: refs/heads/master@{#436209}

Issue 2510323002: bluetooth: Implement acceptAllDevices (Closed)

Description

Patch Set 1 #

Patch Set 2 : Clean up #

Patch Set 3 : Address scheib's comments #

Patch Set 4 : Fix merge conflicts #

Patch Set 5 : Fix mac #

Patch Set 6 : Fix histograms #

Patch Set 7 : Rebase (now uses stl containers) #

Patch Set 8 : clean up #

Messages