Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have sites explicitly watch for advertisements on paired devices. #235

Merged
merged 3 commits into from May 6, 2016

Conversation

@jyasskin
Copy link
Member

jyasskin commented May 3, 2016

This removes the enforcement that a site can only see service data for services
from its allowed services list. Since an advertisement is broadcast and
read-only, I think there's neither any private data in the advertisement nor a
need to protect the device from hostile sites.

Fixes #117 and fixes #212.

Preview at https://rawgit.com/jyasskin/web-bluetooth-1/watch-advertisements/index.html#advertising-events.

This removes the enforcement that a site can only see service data for services
from its allowed services list. Since an advertisement is broadcast and
read-only, I think there's neither any private data in the advertisement nor a
need to protect the device from hostile sites.

Fixes #117 and fixes #212.
@beaufortfrancois

This comment has been minimized.

Copy link
Member

beaufortfrancois commented May 4, 2016

Thank you for spec'ing this @jyasskin!

Here are my two cents: Do we really need the added this new boolean attribute watchingAdvertisements? Can't we simply set it internally to true when developer calls device.addEventListener('advertisementreceived', foo);?

It would be as simple and nice as:

var known_service = "A service in the iBeacon’s GATT server";
return navigator.bluetooth.requestDevice({
  filters: [{services: [known_service]}]
}).then(device => {
  // Set `watchingAdvertisements` to true
  device.addEventListener('advertisementreceived', interpretIBeacon);
});

// Later on...
// Set `watchingAdvertisements` to false
device.removeEventListener('advertisementreceived', interpretIBeacon);
@beaufortfrancois

This comment has been minimized.

Copy link
Member

beaufortfrancois commented May 4, 2016

If we really want these two steps, I'd advise for consistency with startNotifications()/stopNotifications() and make watchingAdvertisements a function like startWatchingAdvertisements()/stopWatchingAdvertisements():

var known_service = "A service in the iBeacon’s GATT server";
return navigator.bluetooth.requestDevice({
  filters: [{services: [known_service]}]
}).then(device => {
  device.startWatchingAdvertisements();
  device.addEventListener('advertisementreceived', interpretIBeacon);
});

// Later on...
device.stopWatchingAdvertisements();
@jefBinomed

This comment has been minimized.

Copy link

jefBinomed commented May 4, 2016

I shared the point of view of @beaufortfrancois with the fact that the device.watchingAdvertisement is useless.

I you look at other sensors, you subscribe the app to an event if you want to receive it and remove the listener when you want to continue to listen to it... So If i could vote, i will vote for option 1 of francois

Also fix up the "BluetoothDevice attributes" note.
index.bs Outdated
readonly attribute BluetoothRemoteGATTServer gatt;
readonly attribute FrozenArray<UUID> uuids;

void watchAdvertisements();

This comment has been minimized.

Copy link
@jyasskin

jyasskin May 4, 2016

Author Member

Maybe @beaufortfrancois's startWatchingAdvertisements() is better since it avoids the nonword "unwatch".

This comment has been minimized.

Copy link
@g-ortuno

g-ortuno May 6, 2016

Contributor

Hmm I was expecting this to be a promise. The main reason is that starting to scan could fail for example if the adapter is off.

This comment has been minimized.

Copy link
@jyasskin

jyasskin May 6, 2016

Author Member

Mhmm. I was thinking that we'd remember that a scan was supposed to be happening, and start it when the adapter comes on. [Android](http://developer.android.com/reference/android/bluetooth/le/BluetoothLeScanner.html#startScan%28java.util.List<android.bluetooth.le.ScanFilter>, android.bluetooth.le.ScanSettings, android.bluetooth.le.ScanCallback%29) can fail asynchronously, but it doesn't report success, so we can't use that to resolve a Promise, and CoreBluetooth doesn't fail at all. It looks like Windows 10 does report both success and failure synchronously, and has a reasonable list of errors.

Ok, let's make this a Promise, and just have a bunch of platforms never fail.

interface BluetoothAdvertisingData {
[Constructor(DOMString type, BluetoothAdvertisingEventInit init)]
interface BluetoothAdvertisingEvent : Event {
readonly attribute BluetoothDevice device;

This comment has been minimized.

Copy link
@jyasskin

jyasskin May 4, 2016

Author Member

This could just be the target of the Event, since these are always dispatched to a BluetoothDevice. However, "target" is a little weird, since it's actually the source of the advertisement.

index.bs Outdated
set <code><var>event</var>.name</code> to the result.

Note: We don't expose whether the name is complete
because Android and Core Bluetooth don't.

This comment has been minimized.

Copy link
@g-ortuno

g-ortuno May 6, 2016

Contributor

Actually I think we can get this from Android because it exposes the raw bytes: http://developer.android.com/reference/android/bluetooth/le/ScanRecord.html#getBytes()

This comment has been minimized.

Copy link
@jyasskin

jyasskin May 6, 2016

Author Member

Yeah, it's more about following precedent. If most users don't need this (shown by the existing platforms not providing it), we shouldn't provide it? If we do provide it, I think it should be a boolean rather than two distinct fields. My comment could be better though.

This comment has been minimized.

Copy link
@g-ortuno

g-ortuno May 6, 2016

Contributor

Yeah, I'm OK with not providing it, just wanted to correct the Note.

This comment has been minimized.

Copy link
@jyasskin

jyasskin May 6, 2016

Author Member

Done.

@g-ortuno

This comment has been minimized.

Copy link
Contributor

g-ortuno commented May 6, 2016

lgtm!

@jyasskin

This comment has been minimized.

Copy link
Member Author

jyasskin commented May 6, 2016

As discussed in #236 (comment), we can't make addEventListener have side-effects, so it has to be an attribute or a function, and Gio's point making it a Promise forces a function.

@jyasskin jyasskin merged commit 725dbf7 into WebBluetoothCG:gh-pages May 6, 2016
@jyasskin jyasskin deleted the jyasskin:watch-advertisements branch May 6, 2016
dstockwell pushed a commit to dstockwell/chromium that referenced this pull request May 7, 2016
The spec is changing the way in which users can receive advertisement
data: WebBluetoothCG/web-bluetooth#235

BUG=568137

Review-Url: https://codereview.chromium.org/1945823003
Cr-Commit-Position: refs/heads/master@{#392222}
@beaufortfrancois beaufortfrancois mentioned this pull request May 9, 2016
@Emill

This comment has been minimized.

Copy link

Emill commented Jan 10, 2017

What's really the point of the public watchingAdvertisements property?
If you know the promise has resolved then it's watching until you call unwatchAdvertisements().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.