Skip to content

/ servo

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

Add Start/Stop notifications #14276

Merged
merged 1 commit into from Nov 23, 2016
Merged

Add Start/Stop notifications #14276

merged 1 commit into from Nov 23, 2016
+364 −4

Conversation

@dati91
Copy link
Contributor

dati91 commented Nov 18, 2016 •
edited by larsbergstrom

Add support for Start and Stop Notifications for WebBluetooth

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes

This change is Reviewable
@highfive
Copy link

highfive commented Nov 18, 2016

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/webidls/BluetoothRemoteGATTCharacteristic.webidl, components/script/dom/bluetoothremotegattcharacteristic.rs
  • @KiChjang: components/script/dom/webidls/BluetoothRemoteGATTCharacteristic.webidl, components/script/dom/bluetoothremotegattcharacteristic.rs
@dati91
Copy link
Contributor Author

dati91 commented Nov 18, 2016

r? @jdm
@highfive highfive assigned jdm and unassigned nox Nov 18, 2016
@dati91
Copy link
Contributor Author

dati91 commented Nov 18, 2016 •
edited

Note: These changes will only enable the ability to start/stop notifications, you can't subscribe for these (yet). This is useful for those devices which requires registering for a notification as a handshake mechanism (e.g. Parrot drones).
@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2016

☔ The latest upstream changes (presumably #14206) made this pull request unmergeable. Please resolve the merge conflicts.
@dati91 dati91 force-pushed the szeged:notify branch from 490cee6 to cc20fc8 Nov 22, 2016
@jdm
jdm requested changes Nov 22, 2016
View changes
components/script/dom/bluetoothremotegattcharacteristic.rs
#[allow(unrooted_must_root)]
// https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattcharacteristic-startnotifications
fn StartNotifications(&self) -> Rc<Promise> {
let p = Promise::new(&self.global());

This comment has been minimized.

Sign in to view
@jdm

jdm Nov 22, 2016

Member

Let's add spec step comments to this method.
components/script/dom/bluetoothremotegattcharacteristic.rs Outdated
let p = Promise::new(&self.global());
let p_cx = p.global().get_cx();

if !self.Service().Device().Gatt().Connected() {

This comment has been minimized.

Sign in to view
@jdm

jdm Nov 22, 2016

Member

What step is this implementing? The spec doesn't say to reject with a network error.

This comment has been minimized.

Sign in to view
@dati91

dati91 Nov 22, 2016

Author Contributor

You are right about this. We differ from the spec, because we do not support any notification context (yet). The reason I put this here to have at least one "early return". But i can remove it if you prefer.

This comment has been minimized.

Sign in to view
@jdm

jdm Nov 22, 2016

Member

I think it's better to stick to the spec when possible.
components/script/dom/bluetoothremotegattcharacteristic.rs
#[allow(unrooted_must_root)]
// https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattcharacteristic-stopnotifications
fn StopNotifications(&self) -> Rc<Promise> {
let p = Promise::new(&self.global());

This comment has been minimized.

Sign in to view
@jdm

jdm Nov 22, 2016

Member

Let's add spec step comments.
tests/wpt/mozilla/tests/mozilla/bluetooth/startNotifications/notify-failure.html Outdated
.then(service => service.getCharacteristic(device_name.name))
.then(characteristic => promise_rejects(t, 'NotSupportedError', characteristic.startNotifications()));
});
}, 'startNotifications should throw NotSupportedError without Notify or Indent flag.');

This comment has been minimized.

Sign in to view
@jdm

jdm Nov 22, 2016

Member

s/Indent/Indicate/
tests/wpt/mozilla/tests/mozilla/bluetooth/startNotifications/notify-succeeds.html Outdated
return characteristic.startNotifications()
.then(result => assert_equals(result, characteristic));
});
});

This comment has been minimized.

Sign in to view
@jdm

jdm Nov 22, 2016

Member 
    .then(gattServer => gattServer.getPrimaryService(heart_rate.name))
    .then(service => service.getCharacteristic(heart_rate_measurement.name))
    .then(characteristic => {
        return characteristic.startNotifications()
        .then(result => assert_equals(result, characteristic));
    });
tests/wpt/mozilla/tests/mozilla/bluetooth/stopNotifications/notify-succeeds.html Outdated
return characteristic.stopNotifications()
.then(result => assert_equals(result, characteristic));
});
});

This comment has been minimized.

Sign in to view
@jdm

jdm Nov 22, 2016

Member 
    .then(gattServer => gattServer.getPrimaryService(heart_rate.name))
    .then(service => service.getCharacteristic(heart_rate_measurement.name))
    .then(characteristic => characteristic.startNotifications())
    .then(characteristic => {
        return characteristic.stopNotifications()
        .then(result => assert_equals(result, characteristic));
    });
@jdm
jdm requested changes Nov 22, 2016
View changes
components/bluetooth/lib.rs Outdated
@@ -953,10 +953,13 @@ impl BluetoothManager {
false => c.stop_notify(),
};
match result {
// Step 11.

This comment has been minimized.

Sign in to view
@jdm

jdm Nov 22, 2016

Member

We'll need a comment containing a link to the appropriate specification, otherwise these steps are mysterious :)
@jdm
Copy link
Member

jdm commented Nov 22, 2016

@bors-servo: delegate+
Squash the commits and this can merge.
@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2016

✌️ @dati91 can now approve this pull request
@dati91 dati91 force-pushed the szeged:notify branch from 9166467 to f5f40c1 Nov 22, 2016
@dati91
Copy link
Contributor Author

dati91 commented Nov 22, 2016

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2016

📌 Commit f5f40c1 has been approved by dati91
@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2016

⌛ Testing commit f5f40c1 with merge 76c92ae...
bors-servo added a commit that referenced this pull request Nov 22, 2016
@bors-servo
Auto merge of #14276 - szeged:notify, r=dati91
Loading status checks…
76c92ae 
Add Start/Stop notifications

Add support for Start and Stop Notifications for WebBluetooth

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14276)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 23, 2016

💔 Test failed - linux-rel-css
@KiChjang
Copy link
Member

KiChjang commented Nov 23, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Nov 23, 2016

⌛ Trying commit f5f40c1 with merge eada866...
bors-servo added a commit that referenced this pull request Nov 23, 2016
@bors-servo
Auto merge of #14276 - szeged:notify, r=<try>
Loading status checks…
eada866 
Add Start/Stop notifications

Add support for Start and Stop Notifications for WebBluetooth

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14276)
<!-- Reviewable:end -->
@KiChjang KiChjang closed this Nov 23, 2016
@KiChjang KiChjang reopened this Nov 23, 2016
@KiChjang KiChjang force-pushed the szeged:notify branch from f5f40c1 to fd7cdbf Nov 23, 2016
@dati91 @KiChjang
Add Start/Stop notifications
Loading status checks…
fd7cdbf
@KiChjang
Copy link
Member

KiChjang commented Nov 23, 2016

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Nov 23, 2016

📌 Commit fd7cdbf has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Nov 23, 2016

⌛ Testing commit fd7cdbf with merge c4b7cc8...
bors-servo added a commit that referenced this pull request Nov 23, 2016
@bors-servo
Auto merge of #14276 - szeged:notify, r=jdm
Loading status checks…
c4b7cc8 
Add Start/Stop notifications

Add support for Start and Stop Notifications for WebBluetooth

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14276)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 23, 2016

💔 Test failed - linux-rel-wpt
@jdm
Copy link
Member

jdm commented Nov 23, 2016

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Nov 23, 2016

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 23, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit fd7cdbf into servo:master Nov 23, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@dati91 @highfive @bors-servo @jdm @KiChjang @nox
You can’t perform that action at this time.