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

Update WebGPU WebIDL #1896

Closed
wants to merge 1 commit into from
Closed

Conversation

grovesNL
Copy link
Contributor

@grovesNL grovesNL commented Dec 4, 2019

This was updated using the WebIDL for WebGPU available at https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/WebGPU.webidl because it looks like most WebIDL in this repository was vendored from mozilla-central originally.

The only change I made to the WebIDL was to remove a trailing comma that seems to be invalid which caused weedle parsing to fail (which will be fixed upstream in Gecko).

Note that WebGPU is being worked on actively in Gecko, so parts of the API are commented out until they're available there. If we'd prefer to avoid these comments, I could add the WebIDL from the WebGPU specification itself. cc @kvark

@alexcrichton
Copy link
Contributor

Thanks for the PR! This is a little tricky to land though because it's a breaking change. We've occasionally made breaking changes to web-sys methods in the past, but they've been pretty minor.

Are the bindings, as listed today, totally unusable?

Ideally this would never have been exposed in the first place and would have been gated. There's discussion on #1712 about how the gating would work, although it has not yet been implemented.

@grovesNL
Copy link
Contributor Author

grovesNL commented Dec 4, 2019

I don't think the current bindings will be very useful to people because they're relatively outdated at this point.

The WebGPU specification also isn't finished yet so there will still be (probably many) breaking changes before it stabilizes. Gating it sounds alright to me if we could find a good way to do that. We were basically hoping to start using web-sys with wgpu-rs releases (when targeting wasm instead of native) even before the WebGPU specification stabilizes.

@alexcrichton
Copy link
Contributor

Ok if that's the case then I'm tempted to prefer a course of action that looks like:

  • Don't land this as-is as a breaking change
  • Implement the system described in add WebUSB WebIDL #1712 to gate unstable features at compile time
  • Update the WebIDL, and provide it behind the gate implemented

(or at least that's how I'd personally prefer to proceed here)

@alexlapa
Copy link
Contributor

@grovesNL ,

Are you still working on this? I also need to add some unstable webidls so might try to implement web_sys_unstable_apis feature.

@grovesNL
Copy link
Contributor Author

@alexlapa I started working on it but the changes are a bit more involved than I hoped. For example, we have to update the code generator in a few places to output the #[cfg(web_sys_unstable_apis)] attribute in several places (struct definitions, impl blocks, etc.).

I plan to continue working on it sometime over the next couple weeks, but please feel free to implement it in the meantime if you'd like

@alexlapa
Copy link
Contributor

alexlapa commented Dec 19, 2019

@alexcrichton ,

So i've tried the most straightforward web_sys_unstable_apis implementation:

  1. add websys/webidls/unstable dir
  2. if web_sys_unstable_apis is on, then web_sys's build.rs will lookup that directory and override all enabled webidls with unstable ones looking at the file name. So if we have enabled/MediaDevices.webidl, and unstable/MediaDevices.webidl only the unstable one will be passed to wasm_bindgen_webidl::compile.

Now i am trying to add all webidls that i need, and it seems that it is hard to get right:

  1. Some changes to webidl language are not support by current bindings generator, so i have to modify them manually.
  2. Some changes in webidls are causing 'chain reaction', e.g. some item was just moved to other file, so i also have to override file that it was moved from.
  3. Some things that are in webidls are not implemented by UA's at all, so it make no sense to add them, so i am modifying webidls once again.

I have also thought about passing every file to compiler and overriding stable apis based on item names, that would definitely be more precise, but still has some drawbacks.

Another solution might be to allow users to add their own webidls.

  1. Add some USER_WEBIDLS_PATH env variable.
  2. web-sys will look by that path and include all webidls to sources passed to binding generator.
  3. binding generator will add every item, overriding items from enabled list based on their name.
  4. Current feature-based binding generation wont work in this case, so every item from webidls in that dir will be included to ouput.

In this way, every user would be able to generate any binding that he needs for his use-case.

UPD: another interesting way that such feature can be used, is to allow users to make bindings for any JS libraries that they need to use. Not sure how to make that work atm, but that would be pretty awesome.

@alexcrichton
Copy link
Contributor

Hm now that you mention it I wonder if that's the best way to do this? We could publish a stable crate whose purpose is solely to generate a crate from webidl files. That way we wouldn't have to change web sys at all here and users could maintain their own webidl in their own crates?

@grovesNL
Copy link
Contributor Author

I think that would work well for our use case, at least until WebGPU stabilizes.

@alexlapa
Copy link
Contributor

@alexcrichton ,

Hm now that you mention it I wonder if that's the best way to do this?

I guess so.

Another option might be releasing some web-sys-unstable crate, that would contain all the webidls from https://github.com/mozilla/gecko-dev/tree/master/dom/webidl, and update them every month or so. That would be enough for many cases, i believe. But, obviously, that would require to keep binding generator up to date with latest webidl language changes.

We could publish a stable crate whose purpose is solely to generate a crate from webidl files.

Would work for me! My only concern is interop with web-sys, but, i guess it could be achieved via JSCast with no problem.

@alexcrichton
Copy link
Contributor

Ah that's a good point about interop, but I we could probably have crates depend on web-sys automatically and also auto-enable features for classes that the WebIDL inherits from?

@alexcrichton
Copy link
Contributor

I've gone ahead and opened up #1950 to track longer-term strategies of how to support unstable WebIDL files (like this)

I'm going to go ahead and close this because I don't think we want to change this in web-sys right now, but rather I think we need to figure out a general scheme for unstable WebIDL files going forward.

@grovesNL grovesNL deleted the sync-webgpu-dec-4 branch January 10, 2020 02:23
@525c1e21-bd67-4735-ac99-b4b0e5262290
Copy link

525c1e21-bd67-4735-ac99-b4b0e5262290 commented Jan 21, 2020

FWIW using the webidl embedded in the spec itself (and accompanying tool to extract it) gives you near perfect bindings for the current implementations. See #1968

From there, you need only make minor alterations to support vendor-specific extensions such as Chrome's GPUComputePipeline::getBindGroupLayout and Safari's WHLSL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants