Use sequence instead of map for data filters #545

beaufortfrancois · 2021-04-27T13:26:30Z

The more I'm playing with manufacturer data filters, the more I think we should go with a sequence instead of a map in BluetoothLEScanFilterInit for both manufacturerData and serviceData.

Because users can have different strings to express the same uint16 ('0' and '00' for instance), we'll have to check for the unicity of the key which is what we're doing already with a sequence.
Moreover, having a required companyIdentifier dictionary key seems more readable than a simple object key.

I've added some code samples below to give you an idea of what I'm experiencing.

// It's not easy to understand what we're filtering on at first glance and the
// empty object isn't nice to read as well.
let manufacturerData = { 0x00e0: {} };
navigator.bluetooth.requestDevice({ filters: [{ manufacturerData }] });

// And with the base-10 serialization of a uint16, it's not much better.
// We would have to handle the `0x` case among others or let developers do it.
let manufacturerData = { "224": {} };
navigator.bluetooth.requestDevice({ filters: [{ manufacturerData }] });

And here's what I'm proposing:

// [NEW] Simplest form.
let manufacturerData = [{ companyIdentifier: 0x00e0 }];
navigator.bluetooth.requestDevice({ filters: [{ manufacturerData }] });

// [NEW] Full filtering on manufacturer data.
let manufacturerData = [
  {
    companyIdentifier: 0x00e0,
    dataPrefix: new Uint8Array([0x01, 0x02, 0x03, 0x04]),
    mask: new Uint8Array([0xff, 0xff, 0xff, 0x00]),
  },
];
navigator.bluetooth.requestDevice({ filters: [{ manufacturerData }] });

@reillyeon What do you think?

Preview | Diff

beaufortfrancois · 2021-04-27T13:44:26Z

Note that CI should succeed once #546 is merged.

This CL adds support for new manufacturerData filter so that developers can request Bluetooth LE devices based on manufacturer specific data (company identifier and data). Spec: WebBluetoothCG/web-bluetooth#545 Test: https://manufacturer-data.glitch.me/ Bug: 707635 Change-Id: I63b80812f35c8f0f557ceaf53632d0d6d2d52b9b

reillyeon · 2021-04-27T23:02:27Z

index.bs

-    1. Let |canonicalizedDataFilter| be the result of <a
-        for="BluetoothDataFilterInit">canonicalizing</a> |dataFilter|,
+    present and 
+    <code>|filter|.{{BluetoothLEScanFilterInit/manufacturerData}}.length === 0</code>,


A nit, as was pointed out to me on the most recent specifications I worked on, the correct syntax for referring to a member of a dictionary is filter["manufacturerData"] rather than filter.manufacturerData. We should do a pass to fix this over the whole specification but might as well avoid introducing new examples of the old style.

reillyeon · 2021-04-27T23:04:57Z

index.bs

+[
+  {
+    manufacturerData: [
+      { companyIdentifier: 17, dataPrefix: new Uint8Array([1, 2, 3]) },


Put companyIdentifier and dataPrefix on separate lines in this and the next two examples to avoid this table column becoming too wide.

reillyeon · 2021-04-27T23:10:29Z

index.bs

+1. For each |manufacturerData| in
+    <code>|filter|.{{BluetoothLEScanFilterInit/manufacturerData}}</code>, do the following sub-steps:
+    1. If <code>|manufacturerData|.{{BluetoothManufacturerDataFilterInit/companyIdentifier}}</code>
+        is present in |canonicalizedFilter|.manufacturerData,


reillyeon · 2021-04-27T23:15:19Z

index.bs

+        is present in |canonicalizedFilter|.manufacturerData,
+        throw a {{TypeError}} and abort these steps.
+    1. Let |canonicalizedManufacturerDataFilter| be the result of <a
+        for="BluetoothDataFilterInit">canonicalizing</a> |manufacturerData|,
        <a>converted to an ECMAScript value</a>. If this throws an exception,
        propagate that exception and abort these steps.
    1. Call <a abstract-op>CreateDataProperty</a>


Since canonicalizedFilter["manufacturerData"] is now a sequence rather than an object we can append to it.

reillyeon · 2021-04-27T23:15:55Z

index.bs

@@ -1400,51 +1431,42 @@ returned from the following steps:
        <code><var>filter</var>.namePrefix</code>.
 1. Set <code>|canonicalizedFilter|.manufacturerData</code> to `{}`.


Initialize canonicalizedFilter["manufacturerData"] to an empty array rather than an empty object.

reillyeon · 2021-04-27T23:16:36Z

index.bs

-        |canonicalizedDataFilter|).
+        (|canonicalizedFilter|.manufacturerData,
+        <code>|manufacturerData|.{{BluetoothManufacturerDataFilterInit/companyIdentifier}}</code>,
+        |canonicalizedManufacturerDataFilter|).
 1. Set <code>|canonicalizedFilter|.serviceData</code> to `{}`.


reillyeon · 2021-04-27T23:16:54Z

index.bs

-    1. Let |canonicalizedDataFilter| be the result of <a
-        for="BluetoothDataFilterInit">canonicalizing</a> |dataFilter|,
+    1. Let |canonicalizedServiceDataFilter| be the result of <a
+        for="BluetoothDataFilterInit">canonicalizing</a> |serviceData|,
        <a>converted to an ECMAScript value</a>. If this throws an exception,
        propagate that exception and abort these steps.
    1. Call <a
        abstract-op>CreateDataProperty</a>(|canonicalizedFilter|.serviceData,


reillyeon · 2021-04-27T23:20:29Z

scanning.bs

 spec: permissions-1
    type: enum-value
+        text: "bluetooth"


In #546 you only removed the conflicting enum value above. Is this change also necessary?

It's not anymore. Thanks for catching!

beaufortfrancois

Thanks for the review!

beaufortfrancois · 2021-04-28T06:11:29Z

index.bs

+[
+  {
+    manufacturerData: [
+      { companyIdentifier: 17, dataPrefix: new Uint8Array([1, 2, 3]) },


beaufortfrancois · 2021-04-28T06:25:05Z

index.bs

-    1. Let |canonicalizedDataFilter| be the result of <a
-        for="BluetoothDataFilterInit">canonicalizing</a> |dataFilter|,
+    present and 
+    <code>|filter|.{{BluetoothLEScanFilterInit/manufacturerData}}.length === 0</code>,


beaufortfrancois · 2021-04-28T06:33:43Z

index.bs

+1. For each |manufacturerData| in
+    <code>|filter|.{{BluetoothLEScanFilterInit/manufacturerData}}</code>, do the following sub-steps:
+    1. If <code>|manufacturerData|.{{BluetoothManufacturerDataFilterInit/companyIdentifier}}</code>
+        is present in |canonicalizedFilter|.manufacturerData,


beaufortfrancois · 2021-04-28T06:34:15Z

scanning.bs

 spec: permissions-1
    type: enum-value
+        text: "bluetooth"


It's not anymore. Thanks for catching!

beaufortfrancois · 2021-04-28T06:50:45Z

index.bs

+        is present in |canonicalizedFilter|.manufacturerData,
+        throw a {{TypeError}} and abort these steps.
+    1. Let |canonicalizedManufacturerDataFilter| be the result of <a
+        for="BluetoothDataFilterInit">canonicalizing</a> |manufacturerData|,
        <a>converted to an ECMAScript value</a>. If this throws an exception,
        propagate that exception and abort these steps.
    1. Call <a abstract-op>CreateDataProperty</a>


beaufortfrancois · 2021-04-28T06:50:50Z

index.bs

-        |canonicalizedDataFilter|).
+        (|canonicalizedFilter|.manufacturerData,
+        <code>|manufacturerData|.{{BluetoothManufacturerDataFilterInit/companyIdentifier}}</code>,
+        |canonicalizedManufacturerDataFilter|).
 1. Set <code>|canonicalizedFilter|.serviceData</code> to `{}`.


beaufortfrancois · 2021-04-28T06:50:53Z

index.bs

-    1. Let |canonicalizedDataFilter| be the result of <a
-        for="BluetoothDataFilterInit">canonicalizing</a> |dataFilter|,
+    1. Let |canonicalizedServiceDataFilter| be the result of <a
+        for="BluetoothDataFilterInit">canonicalizing</a> |serviceData|,
        <a>converted to an ECMAScript value</a>. If this throws an exception,
        propagate that exception and abort these steps.
    1. Call <a
        abstract-op>CreateDataProperty</a>(|canonicalizedFilter|.serviceData,


beaufortfrancois · 2021-04-28T06:51:05Z

index.bs

@@ -1400,51 +1431,42 @@ returned from the following steps:
        <code><var>filter</var>.namePrefix</code>.
 1. Set <code>|canonicalizedFilter|.manufacturerData</code> to `{}`.


This CL adds support for new manufacturerData filter so that developers can request Bluetooth LE devices based on manufacturer specific data (company identifier and data). Spec: WebBluetoothCG/web-bluetooth#545 Test: https://manufacturer-data.glitch.me/ Bug: 707635 Change-Id: I63b80812f35c8f0f557ceaf53632d0d6d2d52b9b

@beaufortfrancois

SHA: d9eeae5 Reason: push, by @beaufortfrancois Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@beaufortfrancois

SHA: d9eeae5 Reason: push, by @beaufortfrancois Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@beaufortfrancois

SHA: d9eeae5 Reason: push, by @beaufortfrancois Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@beaufortfrancois

SHA: d9eeae5 Reason: push, by @beaufortfrancois Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

This CL adds support for new manufacturerData filter so that developers can request Bluetooth LE devices based on manufacturer specific data (company identifier and data). Spec: WebBluetoothCG/web-bluetooth#545 Test: https://manufacturer-data.glitch.me/ Bug: 707635 Change-Id: I63b80812f35c8f0f557ceaf53632d0d6d2d52b9b

This CL adds support for new manufacturerData filter so that developers can request Bluetooth LE devices based on manufacturer specific data (company identifier and data). Spec: WebBluetoothCG/web-bluetooth#545 Test: https://manufacturer-data.glitch.me/ Bug: 707635 Change-Id: I63b80812f35c8f0f557ceaf53632d0d6d2d52b9b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2830933 Commit-Queue: François Beaufort <beaufort.francois@gmail.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/heads/master@{#879284}

…ilter when requesting device, a=testonly Automatic update from web-platform-tests [Web Bluetooth] Add `manufacturerData` filter when requesting device This CL adds support for new manufacturerData filter so that developers can request Bluetooth LE devices based on manufacturer specific data (company identifier and data). Spec: WebBluetoothCG/web-bluetooth#545 Test: https://manufacturer-data.glitch.me/ Bug: 707635 Change-Id: I63b80812f35c8f0f557ceaf53632d0d6d2d52b9b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2830933 Commit-Queue: François Beaufort <beaufort.francois@gmail.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/heads/master@{#879284} -- wpt-commits: d50983bfca33b58be57894725b190b8c1a011ddc wpt-pr: 28718

This CL adds support for new manufacturerData filter so that developers can request Bluetooth LE devices based on manufacturer specific data (company identifier and data). Spec: WebBluetoothCG/web-bluetooth#545 Test: https://manufacturer-data.glitch.me/ Bug: 707635 Change-Id: I63b80812f35c8f0f557ceaf53632d0d6d2d52b9b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2830933 Commit-Queue: François Beaufort <beaufort.francois@gmail.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/heads/master@{#879284} NOKEYCHECK=True GitOrigin-RevId: 33578ed2957f2568a1bb9da1222ff964887a7ed0

beaufortfrancois force-pushed the data-filter branch from 02ba744 to 3e350a0 Compare April 27, 2021 13:31

This was referenced Apr 27, 2021

Accept filters on manufacturer and service data. #297

Merged

Adding simple manufacturer id filtering support #544

Closed

beaufortfrancois force-pushed the data-filter branch from 3e350a0 to e3139c1 Compare April 27, 2021 13:48

chromium-wpt-export-bot mentioned this pull request Apr 27, 2021

[Web Bluetooth] Add manufacturerData filter when requesting device web-platform-tests/wpt#28718

Merged

reillyeon reviewed Apr 27, 2021

View reviewed changes

Use sequence instead of map for data filters

509e088

beaufortfrancois force-pushed the data-filter branch from e3139c1 to 509e088 Compare April 28, 2021 06:08

beaufortfrancois commented Apr 28, 2021

View reviewed changes

Address @reillyeon feedback

e7a6e1a

beaufortfrancois force-pushed the data-filter branch from 953f24d to e7a6e1a Compare April 28, 2021 09:39

reillyeon approved these changes Apr 28, 2021

View reviewed changes

beaufortfrancois merged commit d9eeae5 into main Apr 29, 2021

beaufortfrancois deleted the data-filter branch April 29, 2021 07:23

github-actions bot added a commit that referenced this pull request Apr 29, 2021

Use sequence instead of map for data filters (#545)

9bd70ac

SHA: d9eeae5 Reason: push, by @beaufortfrancois Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 29, 2021

Use sequence instead of map for data filters (#545)

9fcc4b9

SHA: d9eeae5 Reason: push, by @beaufortfrancois Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 29, 2021

Use sequence instead of map for data filters (#545)

ddf681d

SHA: d9eeae5 Reason: push, by @beaufortfrancois Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 29, 2021

Use sequence instead of map for data filters (#545)

7740f52

SHA: d9eeae5 Reason: push, by @beaufortfrancois Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

beaufortfrancois mentioned this pull request Apr 29, 2021

Add Manufacturer Data Filter Web Bluetooth sample GoogleChrome/samples#719

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use sequence instead of map for data filters #545

Use sequence instead of map for data filters #545

beaufortfrancois commented Apr 27, 2021 •

edited by pr-preview bot

Loading

beaufortfrancois commented Apr 27, 2021

reillyeon Apr 27, 2021

beaufortfrancois Apr 28, 2021

reillyeon Apr 27, 2021

beaufortfrancois Apr 28, 2021

reillyeon Apr 27, 2021 •

edited

Loading

beaufortfrancois Apr 28, 2021

reillyeon Apr 27, 2021

beaufortfrancois Apr 28, 2021

reillyeon Apr 27, 2021

beaufortfrancois Apr 28, 2021

reillyeon Apr 27, 2021

beaufortfrancois Apr 28, 2021

reillyeon Apr 27, 2021

beaufortfrancois Apr 28, 2021

reillyeon Apr 27, 2021

beaufortfrancois Apr 28, 2021

beaufortfrancois left a comment

beaufortfrancois Apr 28, 2021

beaufortfrancois Apr 28, 2021

beaufortfrancois Apr 28, 2021

beaufortfrancois Apr 28, 2021

beaufortfrancois Apr 28, 2021

beaufortfrancois Apr 28, 2021

beaufortfrancois Apr 28, 2021

beaufortfrancois Apr 28, 2021

		@@ -1400,51 +1431,42 @@ returned from the following steps:
		<code><var>filter</var>.namePrefix</code>.
		1. Set <code>\|canonicalizedFilter\|.manufacturerData</code> to `{}`.

Use sequence instead of map for data filters #545

Use sequence instead of map for data filters #545

Conversation

beaufortfrancois commented Apr 27, 2021 • edited by pr-preview bot Loading

beaufortfrancois commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reillyeon Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaufortfrancois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaufortfrancois commented Apr 27, 2021 •

edited by pr-preview bot

Loading

reillyeon Apr 27, 2021 •

edited

Loading