Annotations for WebBluetooth functions #14407

zakorgy · 2016-11-29T13:27:15Z

Moved the convert_request_device_options function steps into request_bluetooth_devices function, to stay consistent with the current specification.
Updated the existing step annotations for the requestDevice and related methods.
Added step annotations for the implemented WebBluetooth methods.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14324, #12614

This change is

highfive · 2016-11-29T13:27:20Z

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, components/script/dom/bluetoothdevice.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, components/script/dom/bluetoothdevice.rs

highfive · 2016-11-29T13:27:21Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

zakorgy · 2016-11-29T13:27:30Z

r? @jdm

jdm

This is a great improvement! Thanks!

jdm · 2016-11-29T14:32:54Z

components/script/dom/bluetooth.rs

                let manufacturer_id = match u16::from_str(key.as_ref()) {
                    Ok(id) => id,
                    Err(err) => return Err(Type(format!("{} {} {}", KEY_CONVERSION_ERROR, key, err))),
                };
+
+                // Step 7.3: This step is missing,
+                // because we don't necesarry need to make the conversion to an IDL value.


No need to convert to IDL values since this is only used by native code.

jdm · 2016-11-29T14:32:54Z

components/script/dom/bluetooth.rs

                if uuid_is_blocklisted(service.as_ref(), Blocklist::All) {
                    return Err(Security);
                }
+
+                // Step 9.6: This step is missing,
+                // because we don't need necesarry to make the conversion to an IDL value.


No need to convert to IDL values since this is only used by native code.

jdm · 2016-11-29T14:32:54Z

components/bluetooth/lib.rs

@@ -636,18 +639,22 @@ impl BluetoothManager {
                            thread::sleep(Duration::from_millis(CONNECTION_TIMEOUT_MS));
                        },
                    }
+                // TODO: Step 5.1.4: Use the excahnge MTU procedure.


jdm · 2016-11-29T14:32:54Z

components/bluetooth/lib.rs

@@ -664,10 +671,12 @@ impl BluetoothManager {
        }
    }

+    // https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver-getprimaryservice


Shouldn't this be https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren or at least reference it?

jdm · 2016-11-29T14:32:54Z

components/bluetooth/lib.rs

    fn get_primary_service(&mut self,
                           device_id: String,
                           uuid: String,
                           sender: IpcSender<BluetoothResponseResult>) {
+        // Step 2.5.


These numbers don't correspond to step numbers in the spec, given the split between https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren and https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver-getprimaryservice .

jdm · 2016-11-29T14:32:54Z

components/bluetooth/lib.rs

@@ -851,10 +892,12 @@ impl BluetoothManager {
        return drop(sender.send(Err(BluetoothError::NotFound)));
    }

+    // https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattservice-getcharacteristics


Add a link to https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren.

jdm · 2016-11-29T14:32:54Z

components/bluetooth/lib.rs

        if !self.cached_characteristics.contains_key(&characteristic_id) {
            return drop(sender.send(Err(BluetoothError::InvalidState)));
        }
        let mut adapter = get_adapter_or_return_error!(self, sender);
        let descriptors = self.get_gatt_descriptors_by_uuid(&mut adapter, &characteristic_id, &uuid);
+
+        // Step 7: If its argument is empty, throws a NotFoundError.
        if descriptors.is_empty() {
            return drop(sender.send(Err(BluetoothError::NotFound)));
        }


Remove this.

jdm · 2016-11-29T14:32:54Z

components/bluetooth/lib.rs

        if characteristics_vec.is_empty() {
            return drop(sender.send(Err(BluetoothError::NotFound)));
        }

        return drop(sender.send(Ok(BluetoothResponse::GetCharacteristics(characteristics_vec))));
    }

+    // https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattcharacteristic-getdescriptor


Add a link to https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren .

jdm · 2016-11-29T14:32:54Z

components/bluetooth/lib.rs

@@ -922,10 +977,12 @@ impl BluetoothManager {
        return drop(sender.send(Err(BluetoothError::NotFound)));
    }

+    // https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattcharacteristic-getdescriptors


Add a link to https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren .

jdm · 2016-11-29T14:32:54Z

components/bluetooth/lib.rs

@@ -934,9 +991,13 @@ impl BluetoothManager {
            Some(id) => self.get_gatt_descriptors_by_uuid(&mut adapter, &characteristic_id, &id),
            None => self.get_and_cache_gatt_descriptors(&mut adapter, &characteristic_id),
        };
+
+        // Step 7: If its argument is empty, throws a NotFoundError.
        if descriptors.is_empty() {
            return drop(sender.send(Err(BluetoothError::NotFound)));
        }


Remove this.

bors-servo · 2016-11-29T15:06:42Z

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


        Move convert_request_device_options into request_bluetooth_devices.

dati91 · 2016-11-29T16:48:04Z

@jdm How should I number the steps? e.g. Step 2.7 which is the 7. step in gattchildren, or Step 7. but then i need to resolve the numbering in the dom side somehow, because there will be 2 Step 1

I realize this thing is a bit unfortunate, but i could move up the gattchildren implementation in my todo list if you prefer :)

jdm · 2016-11-29T16:51:13Z

@dati91 I would add a comment like // Subsequent steps are relative to https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren


        Update requestDevice step annotations.


        Step annotations for WebBluetooth functions

jdm · 2016-11-29T17:58:56Z

@bors-servo: delegate+
Squash the latest commits into a9237d6 and this can merge.

bors-servo · 2016-11-29T17:58:59Z

✌️ @zakorgy can now approve this pull request

zakorgy · 2016-11-30T07:11:23Z

@bors-servo r=jdm

bors-servo · 2016-11-30T07:11:24Z

📌 Commit 8dd100f has been approved by jdm


        Auto merge of #14407 - szeged:annotations, r=jdm

Annotations for WebBluetooth functions  1. Moved the `convert_request_device_options` function steps into `request_bluetooth_devices` function, to stay consistent with the current specification. 2. Updated the existing step annotations for the requestDevice and related methods. 3. Added step annotations for the implemented WebBluetooth methods. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14324, #12614   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14407)

bors-servo · 2016-11-30T07:49:25Z

💔 Test failed - android

zakorgy · 2016-11-30T13:38:29Z

@bors-servo try

bors-servo · 2016-11-30T13:39:11Z

⌛ Trying commit 8dd100f with merge f2ca433...


        Auto merge of #14407 - szeged:annotations, r=<try>

Annotations for WebBluetooth functions  1. Moved the `convert_request_device_options` function steps into `request_bluetooth_devices` function, to stay consistent with the current specification. 2. Updated the existing step annotations for the requestDevice and related methods. 3. Added step annotations for the implemented WebBluetooth methods. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14324, #12614   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14407)

jdm · 2016-11-30T14:19:12Z

@zakorgy That's just servo/saltfs#545. You should be able to retry like usual.

bors-servo · 2016-11-30T15:09:03Z

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

bors-servo · 2016-11-30T15:09:06Z

⌛ Testing commit 8dd100f with merge 9a902a7...


        Auto merge of #14407 - szeged:annotations, r=jdm

Annotations for WebBluetooth functions  1. Moved the `convert_request_device_options` function steps into `request_bluetooth_devices` function, to stay consistent with the current specification. 2. Updated the existing step annotations for the requestDevice and related methods. 3. Added step annotations for the implemented WebBluetooth methods. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14324, #12614   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14407)

bors-servo · 2016-11-30T15:59:46Z

💔 Test failed - linux-rel-css

KiChjang · 2016-11-30T18:32:18Z

@bors-servo retry

#14425

bors-servo · 2016-11-30T21:55:50Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-css...

bors-servo · 2016-11-30T22:50:10Z

💔 Test failed - linux-rel-css

KiChjang · 2016-11-30T22:54:56Z

@bors-servo retry

#14237

bors-servo · 2016-12-01T00:37:25Z

⌛ Testing commit 8dd100f with merge 16199b4...


        Auto merge of #14407 - szeged:annotations, r=jdm

Annotations for WebBluetooth functions  1. Moved the `convert_request_device_options` function steps into `request_bluetooth_devices` function, to stay consistent with the current specification. 2. Updated the existing step annotations for the requestDevice and related methods. 3. Added step annotations for the implemented WebBluetooth methods. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14324, #12614   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14407)

bors-servo · 2016-12-01T01:31:18Z

☀️ 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 Manishearth Nov 29, 2016

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

highfive assigned jdm and unassigned Manishearth Nov 29, 2016

jdm mentioned this pull request Nov 29, 2016

get_primary_service doesn't check blocklist or allowed services #14409

Closed

jdm requested changes Nov 29, 2016

View changes

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

highfive added the S-needs-rebase label Nov 29, 2016

Move convert_request_device_options into request_bluetooth_devices.

315c57f

dati91 force-pushed the szeged:annotations branch from d652245 to 5cc0117 Nov 29, 2016

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

zakorgy added 2 commits Nov 11, 2016

Update requestDevice step annotations.

89c4219

Step annotations for WebBluetooth functions

Loading status checks…

8dd100f

jdm added S-needs-squash and removed S-awaiting-review S-needs-rebase labels Nov 29, 2016

dati91 force-pushed the szeged:annotations branch from 54f4e12 to 8dd100f Nov 29, 2016

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

highfive added S-awaiting-merge and removed S-needs-squash labels Nov 30, 2016

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

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

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

highfive added the S-tests-failed label Nov 30, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 1, 2016

bors-servo merged commit 8dd100f into servo:master Dec 1, 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 Dec 1, 2016

servo / servo

Annotations for WebBluetooth functions #14407

Annotations for WebBluetooth functions #14407

zakorgy commented Nov 29, 2016 •

edited by larsbergstrom

highfive commented Nov 29, 2016

highfive commented Nov 29, 2016

zakorgy commented Nov 29, 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.

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.

bors-servo commented Nov 29, 2016

dati91 commented Nov 29, 2016

jdm commented Nov 29, 2016

jdm commented Nov 29, 2016

bors-servo commented Nov 29, 2016

zakorgy commented Nov 30, 2016

bors-servo commented Nov 30, 2016

bors-servo commented Nov 30, 2016

zakorgy commented Nov 30, 2016 •

edited

bors-servo commented Nov 30, 2016

jdm commented Nov 30, 2016

bors-servo commented Nov 30, 2016

bors-servo commented Nov 30, 2016

bors-servo commented Nov 30, 2016

KiChjang commented Nov 30, 2016

bors-servo commented Nov 30, 2016

bors-servo commented Nov 30, 2016

KiChjang commented Nov 30, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

servo / servo

Join GitHub today

Annotations for WebBluetooth functions #14407

Annotations for WebBluetooth functions #14407

Conversation

zakorgy commented Nov 29, 2016 • edited by larsbergstrom

highfive commented Nov 29, 2016

highfive commented Nov 29, 2016

zakorgy commented Nov 29, 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.

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.

bors-servo commented Nov 29, 2016

dati91 commented Nov 29, 2016

jdm commented Nov 29, 2016

jdm commented Nov 29, 2016

bors-servo commented Nov 29, 2016

zakorgy commented Nov 30, 2016

bors-servo commented Nov 30, 2016

bors-servo commented Nov 30, 2016

zakorgy commented Nov 30, 2016 • edited

bors-servo commented Nov 30, 2016

jdm commented Nov 30, 2016

bors-servo commented Nov 30, 2016

bors-servo commented Nov 30, 2016

bors-servo commented Nov 30, 2016

KiChjang commented Nov 30, 2016

bors-servo commented Nov 30, 2016

bors-servo commented Nov 30, 2016

KiChjang commented Nov 30, 2016

bors-servo commented Dec 1, 2016

bors-servo commented Dec 1, 2016

zakorgy commented Nov 29, 2016 •

edited by larsbergstrom

zakorgy commented Nov 30, 2016 •

edited