Issue 1415533006: bluetooth: Implement requestDevice by name or name prefix

ortuno

Description was changed from ========== bluetooth: WIP. name filter Based on: https://github.com/WebBluetoothCG/web-bluetooth/pull/174 Implementation of requestDevice ...

5 years, 2 months ago (2015-10-21 20:45:29 UTC) #1

ortuno

Description was changed from ========== bluetooth: WIP. name filter Based on: https://github.com/WebBluetoothCG/web-bluetooth/pull/174 Implementation of requestDevice ...

5 years, 2 months ago (2015-10-21 20:46:11 UTC) #2

ortuno

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

5 years, 2 months ago (2015-10-21 20:46:28 UTC) #3

ortuno

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

5 years, 2 months ago (2015-10-21 20:47:55 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415533006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415533006/20001

5 years, 2 months ago (2015-10-21 20:48:42 UTC) #6

commit-bot: I haz the power

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

5 years, 2 months ago (2015-10-21 22:49:47 UTC) #7

commit-bot: I haz the power

Dry run: 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_ng/builds/123929)

5 years, 2 months ago (2015-10-21 22:49:48 UTC) #8

scheib

https://codereview.chromium.org/1415533006/diff/20001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/20001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode54 content/browser/bluetooth/bluetooth_dispatcher_host.cc:54: DCHECK(!(filter.name.empty() && filter.namePrefix.empty() && Malformed data from a renderer ...

5 years, 2 months ago (2015-10-22 04:58:42 UTC) #9

ortuno

https://codereview.chromium.org/1415533006/diff/20001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/20001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode54 content/browser/bluetooth/bluetooth_dispatcher_host.cc:54: DCHECK(!(filter.name.empty() && filter.namePrefix.empty() && On 2015/10/22 at 04:58:42, scheib ...

5 years, 2 months ago (2015-10-22 16:21:31 UTC) #10

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

https://codereview.chromium.org/1415533006/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:54:
DCHECK(!(filter.name.empty() && filter.namePrefix.empty() &&
On 2015/10/22 at 04:58:42, scheib wrote:
> Malformed data from a renderer should kill it. That check shouldn't be made
here, it should be in OnRequestDevice. Probably take this logic into a member
function and call it when handling the IPC and kill renderer, then reuse here
with DCHECK and comment that IPC is verified elsewhere.

Done.

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/requestDevice-canonicalize-filter.html
(right):

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice-canonicalize-filter.html:1:
<!DOCTYPE html>
On 2015/10/22 at 04:58:42, scheib wrote:
> You named this file because of the modules/bluetooth/Bluetooth.cpp
canonicalizeFilter method. But, it's a confusing name on its own. All the
filters here don't match. It's a testharness test we hope other browsers will
use, so naming it based on an implementation naming detail isn't great. How
about requestDevice-invalid-filters?

The name comes from the spec algorithm to create a canonicalizedFilter. The
Bluetooth.cpp function is named after that as well.

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/requestDevice-matches-a-filter.html
(right):

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice-matches-a-filter.html:1:
<!DOCTYPE html>
On 2015/10/22 at 04:58:42, scheib wrote:
> I'm confused by file name again. The first 2 blocks have filters that don't
match.

Similarly here. The name comes from the "Matches a filter" algorithm in the
spec.

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice-matches-a-filter.html:102:
}, 'If a present filter doesn\'t match the device doesn\'t match the filter.');
On 2015/10/22 at 04:58:42, scheib wrote:
> If any part of the filter doesn't match ...

Done.

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

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:35: "The services
member must contain at least one service");
On 2015/10/22 at 04:58:42, scheib wrote:
> 'services', if present, must contain ...

Done.

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:49:
canonicalizedFilter.name = filter.name();
On 2015/10/22 at 04:58:42, scheib wrote:
> Shouldn't this also test for empty string ""?

The spec doesn't say anything about empty name. Also I don't see a reason why we
should disallow an empty name.

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:55: "A filter must
restrict the devices in some way.");
On 2015/10/22 at 04:58:42, scheib wrote:
> 'namePrefix', if present, must be non-empty.

Done. This message was copied from the spec. Can you make a comment for Jeffrey
to fix?

scheib

https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right): https://codereview.chromium.org/1415533006/diff/20001/third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp#newcode49 third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:49: canonicalizedFilter.name = filter.name(); On 2015/10/22 16:21:31, ortuno wrote: > ...

5 years, 2 months ago (2015-10-22 18:35:52 UTC) #11

ortuno

https://codereview.chromium.org/1415533006/diff/40001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/40001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode50 content/browser/bluetooth/bluetooth_dispatcher_host.cc:50: bool EmptyFilter(const content::BluetoothScanFilter& filter) { On 2015/10/22 at 18:35:52, ...

5 years, 2 months ago (2015-10-22 20:47:22 UTC) #12

ortuno

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

5 years, 2 months ago (2015-10-22 21:23:58 UTC) #14

ortuno

palmer: PTAL at content/common/bluetooth/. We are sending a string from javascript through IPC to the ...

5 years, 2 months ago (2015-10-22 21:23:59 UTC) #15

ortuno

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

5 years, 1 month ago (2015-10-27 20:34:12 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415533006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415533006/60001

5 years, 1 month ago (2015-10-27 20:35:55 UTC) #18

commit-bot: I haz the power

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

5 years, 1 month ago (2015-10-27 20:48:52 UTC) #19

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/49524) mac_chromium_compile_dbg_ng on ...

5 years, 1 month ago (2015-10-27 20:48:53 UTC) #20

ortuno

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

5 years, 1 month ago (2015-10-27 20:55:30 UTC) #21

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415533006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415533006/80001

5 years, 1 month ago (2015-10-27 20:56:00 UTC) #22

commit-bot: I haz the power

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

5 years, 1 month ago (2015-10-27 21:12:04 UTC) #23

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-generic_chromium_compile_only_ng/builds/49512) linux_chromium_compile_dbg_32_ng on ...

5 years, 1 month ago (2015-10-27 21:12:05 UTC) #24

palmer

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode64 content/browser/bluetooth/bluetooth_dispatcher_host.cc:64: return false; Nit: Maybe this is cleaner/more clear: return ...

5 years, 1 month ago (2015-10-27 21:43:18 UTC) #25

palmer

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode482 content/browser/bluetooth/bluetooth_dispatcher_host.cc:482: VLOG(1) << "Name: " << filter.name; Also, do they ...

5 years, 1 month ago (2015-10-27 21:46:56 UTC) #26

ortuno

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode64 content/browser/bluetooth/bluetooth_dispatcher_host.cc:64: return false; On 2015/10/27 at 21:43:18, palmer wrote: > ...

5 years, 1 month ago (2015-10-28 17:53:25 UTC) #27

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:64: return false;
On 2015/10/27 at 21:43:18, palmer wrote:
> Nit: Maybe this is cleaner/more clear:
> 
> return filters.empty() ? true
>                        : filters.end() != std::find_if(filters.begin(),
>                                                        filters.end(),
>                                                        IsEmptyFilter);

Done.

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:75: if
(!filter.name.empty()) {
On 2015/10/27 at 21:43:18, palmer wrote:
> Nit: I think you can combine these 2 conditions into 1, right?
> 
> if (!filter.name.empty() && device_name != filter_name)
>   return false;
> 
> Same thing with lines 81 and 82 below.

Done.

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: const
std::vector<BluetoothUUID>& device_uuid_list = device.GetUUIDs();
On 2015/10/27 at 21:43:18, palmer wrote:
> This whole block is a good use for auto:
> 
> const auto& device_uuid_list = device.GetUUIDs();
> et c. ...
> 

Done.

> Also I think you can shorten the code by using a reutnr-if-empty,
otherwise-return-find_if strategy like above in the function |HasEmptyFilter|.

I don't think we can use that pattern here since we need to check that every
service in the filter is in the device. I think converting the service uuids
into a set and then checking of all services are in that set is also a fairly
efficient way of doing this.

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:482: VLOG(1) << "Name: "
<< filter.name;
On 2015/10/27 at 21:43:18, palmer wrote:
> Are name and namePrefix ever going to be used for anything else?

We don't have any plans to use it beyond what's implemented here i.e. filtering.

On 2015/10/27 at 21:46:56, palmer wrote:
> Also, do they have any length, lexical, or syntactic constraints that it makes
sense to enforce?

Good point. Device names are UTF-8 strings with a max length of 248 bytes; we
should enforce that. I've raised the issue in the Web Bluetooth Spec [1].

[1]
https://github.com/WebBluetoothCG/web-bluetooth/pull/174#discussion_r43203535

ortuno

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

5 years, 1 month ago (2015-10-30 05:29:24 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415533006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415533006/120001

5 years, 1 month ago (2015-10-30 05:29:44 UTC) #29

commit-bot: I haz the power

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

5 years, 1 month ago (2015-10-30 06:40:52 UTC) #30

commit-bot: I haz the power

Dry run: 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_ng/builds/128078)

5 years, 1 month ago (2015-10-30 06:40:53 UTC) #31

Jeffrey Yasskin

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

5 years, 1 month ago (2015-10-30 22:04:43 UTC) #32

Jeffrey Yasskin

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode482 content/browser/bluetooth/bluetooth_dispatcher_host.cc:482: VLOG(1) << "Name: " << filter.name; On 2015/10/28 17:53:25, ...

5 years, 1 month ago (2015-10-30 22:04:44 UTC) #33

ortuno

https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/80001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode482 content/browser/bluetooth/bluetooth_dispatcher_host.cc:482: VLOG(1) << "Name: " << filter.name; On 2015/10/30 22:04:44, ...

5 years, 1 month ago (2015-10-30 22:36:02 UTC) #34

palmer

> Yup. It's 29 if you don't count the flags which are almost always there. ...

5 years, 1 month ago (2015-10-30 23:44:13 UTC) #35

ortuno

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

5 years, 1 month ago (2015-11-04 02:34:36 UTC) #37

ortuno

palmer: Check for length added. PTAL. nick: ptal at bad_messages.h isherman: ptal at histograms.xml

5 years, 1 month ago (2015-11-04 02:34:37 UTC) #38

ortuno

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

5 years, 1 month ago (2015-11-04 02:35:12 UTC) #39

ncarter (slow)

On 2015/11/04 02:35:13, ortuno wrote: > isherman: PTAL at histograms.xml bad_message rubber stamp lgtm

5 years, 1 month ago (2015-11-04 18:24:16 UTC) #41

palmer

LGTM with nit. https://codereview.chromium.org/1415533006/diff/180001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1415533006/diff/180001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode76 content/browser/bluetooth/bluetooth_dispatcher_host.cc:76: return false; Nit: This is indented ...

5 years, 1 month ago (2015-11-04 21:15:27 UTC) #42

ortuno

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

5 years, 1 month ago (2015-11-04 23:07:17 UTC) #45

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415533006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415533006/200001

5 years, 1 month ago (2015-11-04 23:08:10 UTC) #46

commit-bot: I haz the power

5 years, 1 month ago (2015-11-04 23:20:49 UTC) #48

Message was sent while issue was closed.

Patchset 10 (id:??) landed as
https://crrev.com/9c4494c0090cd89597c9219a48673bf9ff8efc51
Cr-Commit-Position: refs/heads/master@{#357926}

Issue 1415533006: bluetooth: Implement requestDevice by name or name prefix (Closed)

Description

Patch Set 1 #

Patch Set 2 : Tests #

Patch Set 3 : Address scheib's comments #

Patch Set 4 : Change names, added return #

Patch Set 5 : Merge with ToT #

Patch Set 6 : Actually merge with ToT #

Patch Set 7 : Address palmer's comments #

Patch Set 8 : Check name and namePrefix length #

Patch Set 9 : clean up #

Patch Set 10 : Merge with ToT and fix histograms conflict #

Messages