Webbluetooth Async behaviour by dati91 · Pull Request #13909 · servo/servo

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:13:14Z

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:13:18Z

components/script/dom/bluetooth.rs

 }

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


Remove this.

jdm · 2016-11-01T17:14:17Z

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:19:07Z

components/net_traits/bluetooth_thread.rs

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


Can we remove this?

jdm · 2016-11-01T17:23:34Z

components/net_traits/bluetooth_thread.rs

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

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.

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

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 There is new code that needs to be reviewed. label Oct 24, 2016

jdm requested changes Nov 1, 2016

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-rebase There are merge conflict errors. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 1, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 2, 2016

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 2, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 2, 2016

jdm added S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 2, 2016

dati91 force-pushed the promise-queue branch from 629ab62 to d9e1cf3 Compare November 7, 2016 10:14

highfive added the S-awaiting-review There is new code that needs to be reviewed. 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 Something, somewhere else, needs to happen before this PR can be merged. label Nov 7, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Nov 7, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 7, 2016

dati91 force-pushed the promise-queue branch from d9e1cf3 to a18490b Compare November 7, 2016 15:54

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 7, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 7, 2016

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 7, 2016

Webbluetooth Async behaviour

e7e7c74

dati91 force-pushed the promise-queue branch from a18490b to e7e7c74 Compare November 8, 2016 12:36

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 8, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Nov 8, 2016

bors-servo merged commit e7e7c74 into servo:master Nov 8, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 8, 2016

Uh oh!

Conversation

dati91 commented Oct 24, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Oct 24, 2016

Uh oh!

dati91 commented Oct 24, 2016

Uh oh!

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

jdm Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

jdm Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

jdm Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

jdm Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

jdm Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

jdm commented Nov 2, 2016

Uh oh!

dati91 commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdm commented Nov 2, 2016

Uh oh!

dati91 commented Nov 7, 2016

Uh oh!

jdm commented Nov 7, 2016

Uh oh!

bors-servo commented Nov 7, 2016

Uh oh!

bors-servo commented Nov 7, 2016

Uh oh!

bors-servo commented Nov 7, 2016

Uh oh!

dati91 commented Nov 7, 2016

Uh oh!

jdm commented Nov 7, 2016

Uh oh!

bors-servo commented Nov 7, 2016

Uh oh!

bors-servo commented Nov 7, 2016

Uh oh!

bors-servo commented Nov 7, 2016

Uh oh!

jdm commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dati91 commented Oct 24, 2016 •

edited by larsbergstrom

Loading

dati91 commented Nov 2, 2016 •

edited

Loading