Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUpdating GATTServer's Connect/Disconnect calls #14612
Conversation
highfive
commented
Dec 16, 2016
|
Heads up! This PR modifies the following files: |
|
r? @jdm |
|
|
dati91
force-pushed the
szeged:connect-disconnect-update
branch
from
291dec8
to
3360085
|
Do we have any plans to improve the maintainability of the tests? I'm please that we're adding so many, but it's getting really hard to review them well when each file is so similar. |
| } | ||
|
|
||
| // https://webbluetoothcg.github.io/web-bluetooth/#garbage-collect-the-connection | ||
| #[allow(unrooted_must_root)] |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zakorgy
Dec 21, 2016
Author
Contributor
Without this the following error shows up for line 61:
error: Expression of type std::collections::hash_map::Iter<'_, std::string::String, dom::bindings::js::MutJS<dom::bluetoothdevice::BluetoothDevice>> must be rooted, #[deny(unrooted_must_root)] on by default
--> /home/zakorgy/servo/components/script/dom/bluetoothremotegattserver.rs:60:9
|
60 | for (id, device) in context.get_device_map().borrow().iter() {
| ^
The problem is, that device should be rooted here, but i didn't find another way to solve this correctly.
|
|
||
| // https://webbluetoothcg.github.io/web-bluetooth/#garbage-collect-the-connection | ||
| #[allow(unrooted_must_root)] | ||
| fn garbage_collect_the_connection(&self) -> ErrorResult { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zakorgy
Dec 21, 2016
Author
Contributor
I put this function here because in the specification this function is in the section of the BluetoothRemoteGATTServer methods, but it can be moved to BluetoothDevice methods as well.
| if let Err(e) = self.garbage_collect_the_connection() { | ||
| promise.reject_error(promise_cx, Error::from(e)); | ||
| } | ||
| promise.reject_error(promise_cx, Error::Network); |
This comment has been minimized.
This comment has been minimized.
jdm
Dec 20, 2016
Member
The spec says to abort the steps here, otherwise we'll also resolve the promise.
| @@ -0,0 +1,39 @@ | |||
| <!doctype html> | |||
This comment has been minimized.
This comment has been minimized.
jdm
Dec 20, 2016
Member
Given how similar this test and disconnect-invalidates-objects.html are, it would be nice to combine the with/without uuids tests in the same file rather than duplicating so much code in each of them.
|
I have removed the with-uuid tests, because the main goal here is not to test the with-uuid functionalities, which are already tested, and the without uuids tests also contain those cases . |
|
@bors-servo: delegate+ |
|
|
zakorgy
force-pushed the
szeged:connect-disconnect-update
branch
from
deaeb83
to
6e02cb2
|
@bors-servo: r=jdm |
|
|
Updating GATTServer's Connect/Disconnect calls <!-- Please describe your changes on the following line: --> Added the missing [Step 5.2.3](master...szeged:connect-disconnect-updatediff-1dbe29f87740f5aec93f37adbecace6cR213) to the `connect` function. Updated the [disconnect](https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver-disconnect) function to its current state in the specification, including the `clean_up_disconnected_device` and the `garbage_collect_the connection` functions. Added new tests for checking the invalid state of JS objects after disconnection. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [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 <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/14612) <!-- Reviewable:end -->
|
|
zakorgy commentedDec 16, 2016
•
edited by larsbergstrom
Added the missing Step 5.2.3 to the
connectfunction.Updated the disconnect function to its current state in the specification, including the
clean_up_disconnected_deviceand thegarbage_collect_the connectionfunctions.Added new tests for checking the invalid state of JS objects after disconnection.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is