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

Update WebBluetooth to use Promises #13428

Merged
merged 1 commit into from Oct 1, 2016
Merged

Update WebBluetooth to use Promises #13428

merged 1 commit into from Oct 1, 2016

Conversation

@dati91
Copy link
Contributor

dati91 commented Sep 26, 2016 •
edited by larsbergstrom

Initial patch to support promises in WebBluetooth.
This will allow us to use the expected syntax in webpages.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Sep 26, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/webidls/Bluetooth.webidl, components/script/dom/bluetoothremotegattserver.rs, components/script/dom/webidls/BluetoothRemoteGATTServer.webidl, components/script/dom/bluetoothremotegattservice.rs, components/script/dom/bluetooth.rs, components/script/dom/bluetoothremotegattdescriptor.rs, components/script/dom/bluetoothremotegattcharacteristic.rs, components/script/dom/webidls/BluetoothRemoteGATTCharacteristic.webidl, components/script/dom/webidls/BluetoothRemoteGATTService.webidl, components/script/dom/webidls/BluetoothRemoteGATTDescriptor.webidl
@highfive
Copy link

highfive commented Sep 26, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@dati91
Copy link
Contributor Author

dati91 commented Sep 26, 2016

r? @jdm
@highfive highfive assigned jdm and unassigned glennw Sep 26, 2016
@jdm
jdm requested changes Sep 26, 2016
View changes
Copy link
Member

jdm left a comment

Great improvement!
components/script/dom/bluetooth.rs Outdated
pub fn result_to_promise<T: ToJSValConvertible>(global_ref: GlobalRef,
bluetooth_result: Fallible<T>)
-> Fallible<Rc<Promise>> {
match bluetooth_result {

This comment has been minimized.

Sign in to view
@jdm

jdm Sep 26, 2016

Member 
let p = Promise::new(global_ref);
match bluetooth_result {
    Ok(v) => p.resolve_native(v),
    Err(e) => p.reject_error(e),
}
components/script/dom/bluetoothremotegattcharacteristic.rs Outdated
}

#[allow(unrooted_must_root)]
#[allow(unsafe_code)]

This comment has been minimized.

Sign in to view
@jdm

jdm Sep 26, 2016 •
edited

Member

Don't need this and similar ones in this file and other files.
components/script/dom/bluetooth.rs Outdated
#[allow(unsafe_code)]
pub fn result_to_promise<T: ToJSValConvertible>(global_ref: GlobalRef,
bluetooth_result: Fallible<T>)
-> Fallible<Rc<Promise>> {

This comment has been minimized.

Sign in to view
@jdm

jdm Sep 26, 2016

Member

nit: indentation
components/script/dom/webidls/Bluetooth.webidl Outdated
@@ -23,9 +23,8 @@ interface Bluetooth {
// [SecureContext]
// readonly attribute BluetoothDevice? referringDevice;
// [SecureContext]
// Promise<BluetoothDevice> requestDevice(RequestDeviceOptions options);
[Throws]

This comment has been minimized.

Sign in to view
@jdm

jdm Sep 26, 2016 •
edited

Member

This doesn't need to be here anymore, or most (all?) of the other ones in other bluetooth webidl files.

This comment has been minimized.

Sign in to view
@dati91

dati91 Sep 26, 2016

Author Contributor

Do you mean we should remove the Throws/Fallible from the return types?

This comment has been minimized.

Sign in to view
@jdm

jdm Sep 26, 2016

Member

Specifically, the [Throws] annotation, since none of the methods throw an exception any more.
@jdm
Copy link
Member

jdm commented Sep 26, 2016

@bors-servo: delegate+
Just say @bors-servo: r=jdm after squashing the commits into one. Thanks!
@bors-servo
Copy link
Contributor

bors-servo commented Sep 26, 2016

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

bors-servo commented Sep 26, 2016

✌️ @dati91 can now approve this pull request
@jdm
Copy link
Member

jdm commented Sep 26, 2016

@bors-servo: r-
@jdm jdm added S-needs-squash and removed S-awaiting-merge labels Sep 26, 2016
@mmatyas @dati91
Update WebBluetooth to use Promises
Loading status checks…
e05a839
@dati91 dati91 force-pushed the szeged:wbt_promise branch from da1d9f9 to e05a839 Sep 26, 2016
@dati91
Copy link
Contributor Author

dati91 commented Sep 26, 2016

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

bors-servo commented Sep 26, 2016

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

bors-servo commented Sep 27, 2016

⌛ Testing commit e05a839 with merge 1ea0924...
bors-servo added a commit that referenced this pull request Sep 27, 2016
@bors-servo
Auto merge of #13428 - szeged:wbt_promise, r=jdm
Loading status checks…
1ea0924 
Update WebBluetooth to use Promises

<!-- Please describe your changes on the following line: -->
Initial patch to support promises in WebBluetooth.
This will allow us to use the expected syntax in webpages.

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/13428)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2016

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

jdm commented Sep 27, 2016

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

bors-servo commented Sep 28, 2016

⌛ Testing commit e05a839 with merge 04f5155...
bors-servo added a commit that referenced this pull request Sep 28, 2016
@bors-servo
Auto merge of #13428 - szeged:wbt_promise, r=jdm
Loading status checks…
04f5155 
Update WebBluetooth to use Promises

<!-- Please describe your changes on the following line: -->
Initial patch to support promises in WebBluetooth.
This will allow us to use the expected syntax in webpages.

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/13428)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2016

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

KiChjang commented Sep 28, 2016

@bors-servo retry
1 similar comment
@KiChjang
Copy link
Member

KiChjang commented Oct 1, 2016

@bors-servo retry
bors-servo added a commit that referenced this pull request Oct 1, 2016
@bors-servo
Auto merge of #13428 - szeged:wbt_promise, r=jdm
Loading status checks…
0c80ac9 
Update WebBluetooth to use Promises

<!-- Please describe your changes on the following line: -->
Initial patch to support promises in WebBluetooth.
This will allow us to use the expected syntax in webpages.

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/13428)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 1, 2016

⌛ Testing commit e05a839 with merge 0c80ac9...
@bors-servo
Copy link
Contributor

bors-servo commented Oct 1, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit e05a839 into servo:master Oct 1, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
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
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

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