Skip to content
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

Methods that take a WebIDL sequence argument are missing #1038

Closed
RSSchermer opened this issue Nov 18, 2018 · 12 comments
Closed

Methods that take a WebIDL sequence argument are missing #1038

RSSchermer opened this issue Nov 18, 2018 · 12 comments
Labels
frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen

Comments

@RSSchermer
Copy link
Contributor

Some methods seem to be missing from the WebGl2RenderingContext:

  • invalidateFramebuffer
  • invalidateSubFramebuffer
  • drawBuffers
  • transformFeedbackVaryings
  • getUniformIndices
  • getActiveUniforms

What they all seem to have in common is that they all take a WebIDL sequence as an argument. I've not checked other APIs, so I'm not sure whether or not this affects other APIs besides the WebGl2 API.

@alexcrichton alexcrichton added the frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen label Nov 27, 2018
@jpmartin2
Copy link

I just noticed this when looking at the SubtleCrypto api, which has the same problem:

  • generateKey
  • deriveKey
  • importKey
  • unwrapKey

are all missing, and all have sequence parameters.

@alexcrichton
Copy link
Contributor

Er sorry for taking awhile to get back to this! I believe this is handled here where we don't handle it right now. I suspect that we probably want to return either an Array or some sort of Rust-defined iterator here perhaps. It'd definitely be nice to bind these!

This'll need some work to figure out what the best interface is, an implementation, and then a PR!

@derekdreery
Copy link
Contributor

derekdreery commented Jan 6, 2019

Relevant webidl spec: https://heycam.github.io/webidl/#es-sequence

It seems like the best thing to do for now is require a js_sys::Array, and then maybe expand to supporting iterators later.

@derekdreery
Copy link
Contributor

derekdreery commented Jan 6, 2019

I've added a patch to use Array, because I think all sequences are converted to arrays in ECMAScript. PR: #1152. I'm going to see if I get any errors with it like this - not 100% sure it's right.

@derekdreery
Copy link
Contributor

derekdreery commented Jan 7, 2019

Update: I've re-read the spec and it seems ambiguous. It says:

IDL sequence values are represented by ECMAScript Array values.

but then says some other things about iterators. Does this mean "values of type sequence<T>", or "values of the sequence (T)". I think Array is a safe option since passing arrays has to work, and I'm pretty sure you get arrays back.

@Pauan
Copy link
Contributor

Pauan commented Jan 7, 2019

Rather than guessing, we should test and see what the browsers actually do.

@derekdreery
Copy link
Contributor

derekdreery commented Jan 7, 2019

I've experimented with DocumentOrShadowRoot elementsFromPoint in the console window (Firefox):

$ temp0 = document.elementsFromPoint(1.0, 1.0);
Array [ html ]
$ temp0 instanceof Array
true

EDIT Chrome is the same

$ temp1 = document.elementsFromPoint(1.0, 1.0)
(3) [div#TZA4S, body.default-theme.des-mat, html]
$ temp1 instanceof Array
true

EDIT 2 In argument position it's the same again (from the SubtleCrypto interface, available at window.crypto.subtle, emphasis added)

var result = crypto.subtle.generateKey(algorithm, extractable, keyUsages);

...

  • keyUsages is an Array indicating what can be done with the newly generated key. Possible values for array elements are:

...

@alexcrichton
Copy link
Contributor

@derekdreery it may be worthwhile testing a few sequences in argument position too, I think it makes sense that coming out they're an array, but going in it may be anything iterable

@derekdreery
Copy link
Contributor

@alexcrichton that's what I thought too, they might accept stuff that's iterable. I'll do some tests.

@derekdreery
Copy link
Contributor

Here's a test case showing that for at least indexedDB, iterables are supported in argument position:

const keys = ['first', 'second'];
var iterable = {
    [Symbol.iterator]() {
        let idx = 0;

        return {
            next: function() {
                if (idx >= keys.length) {
                    return { done: true };
                }
                idx += 1;
                return {value: keys[idx-1], done: false};
            }
        };
    }
}

var request = indexedDB.open("test2", 1);
request.onupgradeneeded = function(evt) {
    var db = request.result;
    var store = db.createObjectStore("test", {keyPath: iterable});
    console.log(store.keyPath);
}

request.onerror = function(evt) {
    console.warn("Error");
    console.warn(evt);
}

request.onsuccess = function(event) {
    console.log("js Success");
}

How should we handle this in web-sys? Should we emit different types depending on whether the sequence is in argument position, or return position?

@alexcrichton
Copy link
Contributor

Ok thanks for the investigation! In that case I think we'd ideally use &JsValue in argument position and js_sys::Array in return position, but it may also be possible to just use JsValue everywhere.

@alexcrichton
Copy link
Contributor

Fixed in #1152!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen
Projects
None yet
Development

No branches or pull requests

5 participants