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

Implement GetGATTChildren #14593

Merged
merged 1 commit into from Dec 16, 2016
Merged

Implement GetGATTChildren #14593

merged 1 commit into from Dec 16, 2016
+258 −705

Conversation

@dati91
Copy link
Contributor

dati91 commented Dec 14, 2016 •
edited by larsbergstrom

Implement the GetGATTChildren function for webbluetooth.

  • ./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 Reviewable
@highfive
Copy link

highfive commented Dec 14, 2016

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/bluetoothremotegattserver.rs, components/script/dom/bluetoothremotegattservice.rs, components/script/dom/bluetooth.rs, components/script/dom/bluetoothremotegattdescriptor.rs, components/script/dom/bluetoothremotegattcharacteristic.rs
  • @KiChjang: components/script/dom/bluetoothremotegattserver.rs, components/script/dom/bluetoothremotegattservice.rs, components/script/dom/bluetooth.rs, components/script/dom/bluetoothremotegattdescriptor.rs, components/script/dom/bluetoothremotegattcharacteristic.rs
@dati91
Copy link
Contributor Author

dati91 commented Dec 14, 2016

Note: it depends on #14567 and also needs a rebase.
@dati91
Copy link
Contributor Author

dati91 commented Dec 14, 2016

r? @jdm
@highfive highfive assigned jdm and unassigned KiChjang Dec 14, 2016
@jdm
Copy link
Member

jdm commented Dec 14, 2016

What does WIP in the title mean?
@dati91
Copy link
Contributor Author

dati91 commented Dec 14, 2016

work in progress, maybe i should have used [do not merge yet] or something
I just wanted to indicate somehow that the first patch doesn't belong here, just a placeholder before the other pr lands.
@jdm
Copy link
Member

jdm commented Dec 15, 2016

We can use the S-blocked-on-external label for that. WIP usually means that it's not necessarily ready for review yet.
@dati91 dati91 force-pushed the szeged:getgattchildren branch from 4343278 to 42134a4 Dec 15, 2016
@dati91 dati91 changed the title [WIP] Implement GetGATTChildren Implement GetGATTChildren Dec 15, 2016
@jdm
jdm requested changes Dec 15, 2016
View changes
Copy link
Member

jdm left a comment

This is a nice improvement that reduces code complexity.
components/bluetooth/lib.rs Outdated
if !self.device_is_cached(&device_id) {
return drop(sender.send(Err(BluetoothError::InvalidState)));
}
fn get_gatt_children(&mut self,

This comment has been minimized.

Sign in to view
@jdm

jdm Dec 15, 2016

Member

Let's add the https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren link back.
components/script/dom/bluetooth.rs Outdated
let p_cx = p.global().get_cx();

let mut result_uuid: Option<String> = None;
if let Some(u) = uuid {

This comment has been minimized.

Sign in to view
@jdm

jdm Dec 15, 2016

Member 
let result_uuid = if let Some(u) = uuid {
    let canonicalized = match uuid_canonicalizer(u) {
        Ok(canonicalized_uuid) => canonicalized_uuid.to_string(),
        Err(e) => {
            ...
        }
    };
    if uuid_is_blocklisted(canonicalized.as_ref(), Blocklist::All) {
        ...
    }
    Some(canonicalized)
} else {
    None
};
@bors-servo
Copy link
Contributor

bors-servo commented Dec 15, 2016

☔ The latest upstream changes (presumably #14604) made this pull request unmergeable. Please resolve the merge conflicts.
@dati91 dati91 force-pushed the szeged:getgattchildren branch from 42134a4 to f31ae71 Dec 16, 2016
@jdm
Copy link
Member

jdm commented Dec 16, 2016

@bors-servo: delegate+
Squash and r=jdm.
@bors-servo
Copy link
Contributor

bors-servo commented Dec 16, 2016

✌️ @dati91 can now approve this pull request
@dati91
Implement GetGATTChildren
Loading status checks…
b010368
@dati91 dati91 force-pushed the szeged:getgattchildren branch from f31ae71 to b010368 Dec 16, 2016
@dati91
Copy link
Contributor Author

dati91 commented Dec 16, 2016

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

bors-servo commented Dec 16, 2016

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

bors-servo commented Dec 16, 2016

⌛ Testing commit b010368 with merge 6efea39...
bors-servo added a commit that referenced this pull request Dec 16, 2016
@bors-servo
Auto merge of #14593 - szeged:getgattchildren, r=jdm
Loading status checks…
6efea39 
Implement GetGATTChildren

<!-- Please describe your changes on the following line: -->
Implement the [GetGATTChildren](https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren) function for webbluetooth.

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

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

bors-servo commented Dec 16, 2016

☀️ 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-dev
@bors-servo bors-servo merged commit b010368 into servo:master Dec 16, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Dec 16, 2016
Merged
3 of 3 tasks complete
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

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