Webbluetooth Async behaviour #13909

dati91 · 2016-10-24T15:38:32Z

Note: depends on #13612

The current WBT communication is synchronous. With this, it should work properly (except the disconnect function, which will need some more work, because the current implementation differ from the spec).

./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

highfive · 2016-10-24T15:38:39Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs, components/constellation/pipeline.rs
@fitzgen: components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/script_thread.rs, components/script/dom/mod.rs, components/script/dom/webidls/Window.webidl, components/script/dom/window.rs, components/script/dom/bluetoothremotegattserver.rs, components/script/dom/bluetoothremotegattservice.rs, components/script/dom/webidls/TestRunner.webidl, components/script/network_listener.rs, components/script/dom/testrunner.rs, components/script/dom/bluetooth.rs, components/script/dom/bluetoothremotegattdescriptor.rs, components/script/dom/bluetoothremotegattcharacteristic.rs
@KiChjang: components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/script_thread.rs, components/net/lib.rs, components/script/dom/mod.rs, components/script/dom/webidls/Window.webidl, components/net/bluetooth_thread.rs, components/script/dom/window.rs, components/script/dom/bluetoothremotegattserver.rs, components/net/bluetooth_test.rs, components/script/dom/bluetoothremotegattservice.rs, components/net_traits/bluetooth_thread.rs, components/net_traits/bluetooth_thread.rs, components/script/dom/webidls/TestRunner.webidl, components/script/network_listener.rs, components/script/dom/testrunner.rs, components/script/dom/bluetooth.rs, components/script/dom/bluetoothremotegattdescriptor.rs, components/script/dom/bluetoothremotegattcharacteristic.rs, components/net/Cargo.toml

dati91 · 2016-10-24T15:38:48Z

@jdm r?

jdm

Great work! It's exciting to see all of this synchronous code become asynchronous.

jdm · 2016-11-01T17:26:19Z

components/script/dom/bluetooth.rs

+    }));
+    let listener = NetworkListener {
+        context: context,
+        //script_chan: receiver.root().global().r().networking_task_source(),


Remove this.

jdm · 2016-11-01T17:26:19Z

components/script/dom/bluetooth.rs

        self.get_bluetooth_thread().send(BluetoothMethodMsg::RequestDevice(option, sender)).unwrap();
    }
 }

+pub fn response_async<T: AsyncBluetoothListener + Reflectable + 'static>(
+        //chan: Box<ScriptChan + Send>,


Remove this.

jdm · 2016-11-01T17:26:19Z

components/script/dom/bluetooth.rs

+pub fn response_async<T: AsyncBluetoothListener + Reflectable + 'static>(
+        //chan: Box<ScriptChan + Send>,
+        promise: &Rc<Promise>,
+        receiver: Trusted<T>) -> IpcSender<BluetoothResponseMsg> {


It would make more sense to take a &T argument and store the result of Trusted::new(receiver) instead.

jdm · 2016-11-01T17:26:19Z

components/net_traits/bluetooth_thread.rs

    RequestDevice(BluetoothDeviceMsg),
    GATTServerConnect(bool),
-    GATTServerDisconnect(bool),
+    //GATTServerDisconnect(bool),


Can we remove this?

jdm · 2016-11-01T17:26:19Z

components/net_traits/bluetooth_thread.rs

@@ -85,20 +85,19 @@ pub enum BluetoothRequest {

 #[derive(Deserialize, Serialize)]
 pub enum BluetoothResponse {
-    RequestDevice(BluetoothDeviceMsg),
-    GATTServerConnect(bool),
+    RequestDevice(BluetoothResult<BluetoothDeviceMsg>),


Given that every response includes a result value, it probably makes more sense to use BluetoothResult<BluetoothResponse> instead. This will mean less code duplication for handling errors in places like bluetoothremotegattcharacteristic.rs. It could even be handled inside BluetoothResponseListener::response, so none of the rest of the implementations of AsyncBluetoothListener need to care about errors.

jdm · 2016-11-02T13:38:13Z

The changes in e550880 are a good improvement. There are still a couple unaddressed comments from my last review, too.

dati91 · 2016-11-02T14:37:51Z

The last thing on my list is to remove the Option from Option<TrustedPromise>
But i having trouble with accessing it

let promise = self.promise.root();
              ^^^^ cannot move out of borrowed content

any idea how should i do it?

jdm · 2016-11-02T14:49:53Z

You're right, I looked at the code and I don't see a way to make that change right now. It's fine as-is.

dati91 · 2016-11-07T10:24:20Z

Note: There was a commit which moved bluetooth to a new folder, and Action can be implemented in its own crate, so i moved the impl to net_traits. Everything else should be the same.

jdm · 2016-11-07T13:58:08Z

@bors-servo: r+

bors-servo · 2016-11-07T13:58:09Z

📌 Commit d9e1cf3 has been approved by jdm

bors-servo · 2016-11-07T15:20:31Z

⌛ Testing commit d9e1cf3 with merge 5770105...


        Auto merge of #13909 - dati91:promise-queue, r=jdm

Webbluetooth Async behaviour  Note: depends on #13612 The current WBT communication is synchronous. With this, it should work properly (except the disconnect function, which will need some more work, because the current implementation differ from the spec).  - [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   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13909)

bors-servo · 2016-11-07T15:50:12Z

💔 Test failed - linux-dev

dati91 · 2016-11-07T15:52:48Z

oh.. i forget the ports.. :/

jdm · 2016-11-07T15:56:07Z

@bors-servo: r+

bors-servo · 2016-11-07T15:56:09Z

📌 Commit a18490b has been approved by jdm

bors-servo · 2016-11-07T16:53:15Z

🔒 Merge conflict

bors-servo · 2016-11-07T16:54:39Z

☔ The latest upstream changes (presumably #13918) made this pull request unmergeable. Please resolve the merge conflicts.


        Webbluetooth Async behaviour

jdm · 2016-11-08T14:19:41Z

@bors-servo: r+

bors-servo · 2016-11-08T14:19:46Z

📌 Commit e7e7c74 has been approved by jdm

bors-servo · 2016-11-08T15:05:13Z

⌛ Testing commit e7e7c74 with merge 1153ca9...


        Auto merge of #13909 - dati91:promise-queue, r=jdm

Webbluetooth Async behaviour  Note: depends on #13612 The current WBT communication is synchronous. With this, it should work properly (except the disconnect function, which will need some more work, because the current implementation differ from the spec).  - [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   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13909)

bors-servo · 2016-11-08T15:56:42Z

☀️ 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

highfive assigned jdm Oct 24, 2016

highfive added the S-awaiting-review label Oct 24, 2016

jdm requested changes Nov 1, 2016

View changes

jdm added S-needs-code-changes S-needs-rebase and removed S-awaiting-review labels Nov 1, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 2, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 2, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 2, 2016

jdm added S-blocked-on-external and removed S-awaiting-review labels Nov 2, 2016

dati91 force-pushed the dati91:promise-queue branch from 629ab62 to d9e1cf3 Nov 7, 2016

highfive added the S-awaiting-review label Nov 7, 2016

dati91 changed the title ~~[WIP] WebBluetooth async behaviour~~ Webbluetooth Async behaviour Nov 7, 2016

jdm removed the S-blocked-on-external label Nov 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Nov 7, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 7, 2016

dati91 force-pushed the dati91:promise-queue branch from d9e1cf3 to a18490b Nov 7, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Nov 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 7, 2016

highfive added S-needs-rebase and removed S-awaiting-merge labels Nov 7, 2016

Webbluetooth Async behaviour

Loading status checks…

e7e7c74

dati91 force-pushed the dati91:promise-queue branch from a18490b to e7e7c74 Nov 8, 2016

highfive added the S-awaiting-review label Nov 8, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Nov 8, 2016

bors-servo merged commit e7e7c74 into servo:master Nov 8, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 8, 2016

servo / servo

Webbluetooth Async behaviour #13909

Webbluetooth Async behaviour #13909

dati91 commented Oct 24, 2016 •

edited by larsbergstrom

highfive commented Oct 24, 2016

dati91 commented Oct 24, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Nov 2, 2016

dati91 commented Nov 2, 2016 •

edited

jdm commented Nov 2, 2016

dati91 commented Nov 7, 2016

jdm commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

dati91 commented Nov 7, 2016

jdm commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

jdm commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

servo / servo

Join GitHub today

Webbluetooth Async behaviour #13909

Webbluetooth Async behaviour #13909

Conversation

dati91 commented Oct 24, 2016 • edited by larsbergstrom

highfive commented Oct 24, 2016

dati91 commented Oct 24, 2016

jdm left a comment

This comment has been minimized.

jdm Nov 1, 2016

This comment has been minimized.

jdm Nov 1, 2016

This comment has been minimized.

jdm Nov 1, 2016

This comment has been minimized.

jdm Nov 1, 2016

This comment has been minimized.

jdm Nov 1, 2016

jdm commented Nov 2, 2016

dati91 commented Nov 2, 2016 • edited

jdm commented Nov 2, 2016

dati91 commented Nov 7, 2016

jdm commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

dati91 commented Nov 7, 2016

jdm commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

bors-servo commented Nov 7, 2016

jdm commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

dati91 commented Oct 24, 2016 •

edited by larsbergstrom

dati91 commented Nov 2, 2016 •

edited