Skip to content

Implement GetAvailability for Bluetooth #15368

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 13, 2017
Merged

Implement GetAvailability for Bluetooth #15368

merged 1 commit into from
Feb 13, 2017

Conversation

dati91
Copy link
Contributor

@dati91 dati91 commented Feb 3, 2017 •
edited by larsbergstrom
Loading

This implements the getAvailability function from the spec.

  • ./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 Feb 3, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/webidls/Bluetooth.webidl, components/script/dom/bluetooth.rs
  • @KiChjang: components/script/dom/webidls/Bluetooth.webidl, components/script/dom/bluetooth.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 3, 2017
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Feb 3, 2017
nox
nox suggested changes Feb 7, 2017
View reviewed changes
components/script/dom/bluetooth.rs Outdated
let sender = response_async(&p, self);
self.get_bluetooth_thread().send(
BluetoothRequest::GetAvailability(sender)).unwrap();
return p;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return is useless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still need to change this.

components/script/dom/bluetooth.rs
@@ -443,6 +443,18 @@ impl BluetoothMethods for Bluetooth {
return p;
}

#[allow(unrooted_must_root)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required for the Rc<Promise>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdm That sounds very weird, why does Rc<Promise> need unrooted_must_root be allowed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because our rooting lint doesn't understand that Rc<Promise> is safe. I thought there was an issue about it, but I can't find it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

components/bluetooth/lib.rs
@@ -924,4 +927,12 @@ impl BluetoothManager {
// TODO: Implement this when supported in lower level
return Err(BluetoothError::NotSupported);
}

// https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetooth-getavailability
fn get_availability(&mut self) -> BluetoothResponseResult {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this return a Result? It doesn't seem to return any error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called via an async promise and the handle function requires a result, because usually a promise could fail, but this will only resolve. Step 2 - 3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

components/bluetooth/lib.rs Outdated
match self.get_adapter() {
Ok(_) => Ok(BluetoothResponse::GetAvailability(true)),
Err(_) => Ok(BluetoothResponse::GetAvailability(false)),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

BluetoothResponse::GetAvailability(self.get_adapter().is_ok())

@nox nox removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Feb 7, 2017
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Feb 7, 2017
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 8, 2017
nox
nox suggested changes Feb 8, 2017
View reviewed changes
Copy link
Contributor

@nox nox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash and remove that superfluous return.

components/script/dom/bluetooth.rs
@@ -443,6 +443,18 @@ impl BluetoothMethods for Bluetooth {
return p;
}

#[allow(unrooted_must_root)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

components/script/dom/bluetooth.rs Outdated
let sender = response_async(&p, self);
self.get_bluetooth_thread().send(
BluetoothRequest::GetAvailability(sender)).unwrap();
return p;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still need to change this.

@nox nox added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-needs-rebase There are merge conflict errors. S-awaiting-review There is new code that needs to be reviewed. labels Feb 8, 2017
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 8, 2017
@wilzbach wilzbach mentioned this pull request Feb 12, 2017
Closed
@nox nox removed the S-needs-squash Some (or all) of the commits in the PR should be combined. label Feb 13, 2017
@nox
Copy link
Contributor

nox commented Feb 13, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit ca7aa6b has been approved by nox

@highfive 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 Feb 13, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit ca7aa6b with merge e394334...

bors-servo pushed a commit that referenced this pull request Feb 13, 2017
Auto merge of #15368 - szeged:get_availability, r=nox
e394334 
Implement GetAvailability for Bluetooth

<!-- Please describe your changes on the following line: -->
This implements the [getAvailability](https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetooth-getavailability) function from the spec.

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

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: nox
Pushing e394334 to master...

@bors-servo bors-servo merged commit ca7aa6b into servo:master Feb 13, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm jdm left review comments

+1 more reviewer

@nox nox nox approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@nox nox

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dati91 @highfive @bors-servo @nox @jdm