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

Add webgpu IDL spec and generated code #38

Merged
merged 18 commits into from
May 3, 2022
Merged

Add webgpu IDL spec and generated code #38

merged 18 commits into from
May 3, 2022

Conversation

MaxDesiatov
Copy link
Contributor

This allows removing manual OffscreenRenderingContext and RenderingContext declarations.

@MaxDesiatov MaxDesiatov requested a review from a team May 3, 2022 15:55
@MaxDesiatov MaxDesiatov added the enhancement New feature or request label May 3, 2022
@j-f1
Copy link
Member

j-f1 commented May 3, 2022

Looks like this needs to be rebased?

@MaxDesiatov
Copy link
Contributor Author

I've rebased it, but there are still a few commits that don't matter in the end diff, and they will be squashed. Yes, the diff on Generated.swift is huge, because webgpu is apparently that big...

@@ -1,5 +1,5 @@
/// https://github.com/w3c/webidl2.js#iterable-async-iterable-maplike-and-setlike-declarations
public protocol IDLDeclaration: IDLNode, IDLInterfaceMember {
public protocol IDLDeclaration: IDLInterfaceMember {
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 conformance to IDLNode triggered a redundancy warning, as apparently IDLInterfaceMember already conforms to IDLNode.

@@ -117,7 +117,7 @@ func runWebGLDemo() {
// Get A WebGL context
let canvas = HTMLCanvasElement(from: document.createElement(localName: "canvas"))!
_ = document.body?.appendChild(node: canvas)
let context = canvas.getContext(contextId: "webgl2")!.webGL2RenderingContext!
let context = WebGL2RenderingContext.construct(from: canvas.getContext(contextId: "webgl2")!.jsValue)!
Copy link
Member

@j-f1 j-f1 May 3, 2022

Choose a reason for hiding this comment

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

Suggested change
let context = WebGL2RenderingContext.construct(from: canvas.getContext(contextId: "webgl2")!.jsValue)!
let context = canvas.getContext(contextId: "webgl2")!.webGL2RenderingContext!

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 doesn't work unfortunately, as these enum properties aren't public in the generated code. IDK if that's on purpose. Either way, I decided to make it work with the existing generation approach, and those properties could be made public, if needed, in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Oh they should definitely be public — there’s no internal use for them.

MaxDesiatov added a commit to swiftwasm/JavaScriptKit that referenced this pull request May 16, 2022
Force unwrapping in `class var constructor` may crash for classes that are unavailable in the current environment. For example, after swiftwasm/WebAPIKit#38 was merged, it led to crashes in `getCanvas` calls due to these optional casts relying on `class var constructor` always succeeding:

```swift
    public static func construct(from value: JSValue) -> Self? {
        if let canvasRenderingContext2D: CanvasRenderingContext2D = value.fromJSValue() {
            return .canvasRenderingContext2D(canvasRenderingContext2D)
        }
        if let gpuCanvasContext: GPUCanvasContext = value.fromJSValue() {
            return .gpuCanvasContext(gpuCanvasContext)
        }
        if let imageBitmapRenderingContext: ImageBitmapRenderingContext = value.fromJSValue() {
            return .imageBitmapRenderingContext(imageBitmapRenderingContext)
        }
        if let webGL2RenderingContext: WebGL2RenderingContext = value.fromJSValue() {
            return .webGL2RenderingContext(webGL2RenderingContext)
        }
        if let webGLRenderingContext: WebGLRenderingContext = value.fromJSValue() {
            return .webGLRenderingContext(webGLRenderingContext)
        }
        return nil
    }
```

`if let gpuCanvasContext: GPUCanvasContext = value.fromJSValue()` branch crashes on browsers that don't have `GPUCanvasContext` enabled.

As we currently don't have a better way to handle unavailable features, I propose making the result type of `static var constructor` requirement optional. This means you can still declare classes that are unavailable in the host JS environment. Conditional type casts are also available as they were, they will just always return `nil`, and initializers for these classes will return `nil` as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants