Closed Bug 1469376 Opened 6 years ago Closed 6 years ago

[webgpu] Implement DOM bindings for sketch IDL

Categories

(Core :: Graphics: CanvasWebGL, enhancement, P1)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 2 obsolete files)

Bug 1469376 - Initial stubs for WebGPU sketch API. -
59 bytes, text/x-review-board-request
kvark
: review+
qdot
: review+
Details 
      No description provided.
Comment on attachment 8986046 [details]
Bug 1469376 - Initial stubs for WebGPU sketch API. -

https://reviewboard.mozilla.org/r/251498/#review258016

Got a few questions/concerns, otherwise good to go.

::: dom/webgpu/Buffer.cpp:44
(Diff revision 1)
> +webgpu::Buffer::WrapObject(JSContext* cx, JS::Handle<JSObject*> givenProto)
> +{
> +    return dom::WebGPUBufferBinding::Wrap(cx, this, givenProto);
> +}
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(Buffer)

why is this cycle collection implemented here and not, say, for BlendState?

::: dom/webgpu/moz.build:8
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +with Files('**'):
> +    BUG_COMPONENT = ('Core', 'Canvas: WebGL')

WebGL?

::: dom/webidl/WebGPU.webidl:1
(Diff revision 1)
> +typedef unsigned long u32;

can we vendor this file as a dependency from Github?
Attachment #8986046 - Flags: review?(kvark) → review+
Comment on attachment 8986044 [details]
Bug 1469376 - Formatting fixes for dom/bindings/Codegen.py.

https://reviewboard.mozilla.org/r/251494/#review258294
Attachment #8986044 - Flags: review+
Comment on attachment 8986046 [details]
Bug 1469376 - Initial stubs for WebGPU sketch API. -

https://reviewboard.mozilla.org/r/251498/#review258016

> why is this cycle collection implemented here and not, say, for BlendState?

mMapping unlink handling seems to need to be non-trivial, so it can't be handled by the normal macros. This is really rare, since we rarely hold on to actual JSObjects on the C++ side of things.

> WebGL?

We don't have a separate module yet, and WebGL is close enough for now.

> can we vendor this file as a dependency from Github?

We can't, unfortunately. We still have some deviations, and will for some time. It's TBD whether we can ever consume it directly, though that's a minor goal.
Comment on attachment 8986046 [details]
Bug 1469376 - Initial stubs for WebGPU sketch API. -

https://reviewboard.mozilla.org/r/251498/#review258638
Comment on attachment 8986045 [details]
Bug 1469376 - Don't require Window re-creation after webgpu pref change. -

https://reviewboard.mozilla.org/r/251496/#review259366
Attachment #8986045 - Flags: review?(kyle) → review+
Comment on attachment 8986046 [details]
Bug 1469376 - Initial stubs for WebGPU sketch API. -

https://reviewboard.mozilla.org/r/251498/#review259370

WebIDL file needs a license header and spec ref, but I don't think that's a reason to r-. You could probably also add these interfaces to dom/tests/mochitests/general/test_interfaces.js also, and mark as disabled for now.

::: dom/webidl/WebGPU.webidl:1
(Diff revision 1)
> +typedef unsigned long u32;

Missing license header and reference to spec this is from
Attachment #8986046 - Flags: review?(kyle) → review+
Comment on attachment 8986045 [details]
Bug 1469376 - Don't require Window re-creation after webgpu pref change. -

https://reviewboard.mozilla.org/r/251496/#review259392

Retracting review because I was writing tests for this and found a problem with Union conversions and includes. Need to whittle down how we can let this in.
Attachment #8986045 - Flags: review+ → review?(kyle)
bz, can you check patch 2 on this bug? It does seem to compile fine, but I'm having problems writing tests for it in our test webidl due to TestBindingHeader inclusion with UnionDeclarations, and I'm not sure if this is only compiling because of luck via unified compilation and header order, or if we can actually remove this safely.
Flags: needinfo?(bzbarsky)
So there are two possible cases.

If the union lives in UnionTypes.h, because it's used in multiple bindings, then we will in fact have a circularity problem here: the binding header will try to include UnionTypes.h for the dictionary member, which will try to include the binding header for the dictionary inside the union, and bad things will happen.  Most importantly, those bad things will lead to really hard to diagnose compilation failures, which is why we have this check in codegen: it makes it simple to tell what the problem is and fix it.

The second case is that the union lives in a single binding header: it's only used from this dictionary.  In that case, things will in fact work correctly, because in bug 1068740 we added unions to the set of things we pass to dependencySortObjects, so the unions and dictionaries within that single header will be sorted in an order that makes everything work.

Testing this would involve using a union not used anywhere else in TestCodeGen.webidl; not sure whether you were doing that.

Anyway, what I think we should do here is factor out the logic in getUnionDeclarationFilename so we have a predicate for "does this union live in UnionTypes.h?" and use that predicate from both getUnionDeclarationFilename and here.  If that tests false, then we can skip the rest of the checking being done here.
Flags: needinfo?(bzbarsky)
See Also: → 1470325
I've created bug 1471429 to continue the union creation work elsewhere. Once the WebIDL files here are redone to fit the current system, this can probably land, and I'll deal with combining the interfaces and dicts there once I figure out how to get codegen working with these rules.
Does this new patch make sense? It's sort of a stretch, but the issue is that `window` gets created very infrequently, and having the Window member annotated with Pref in webidl means that after pref change, we still can't access window.webgpu until `window` is recreated. The acute issue here is changing the pref is usually how we test these things in mochitest-plain, which is ideally where we'd do it.

Having `window.webgpu` be a permanent member (albeit usually null) makes the pref useful for testing again.

I'm open to other suggestions, but this would fix the issue for me without being /too/ bad. (though `'webgpu' in window` would always be `true`...)
Flags: needinfo?(kyle)
One option is to test using the window of an iframe that is created after setting the pref.  See https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/dom/manifest/test/test_window_onappinstalled_event.html#16-17 for an example.

Another option is to put all the relevant tests in a single dir and add the relevant prefs= annotation to the mochitest.ini in that dir so it's set before the tests start, if that's the behavior you want.
Yeah, I'd go with the mochitest answer in comment 17, if tests are all you're looking for right now. Keeping the pref in the webidl is definitely cleaner.
Flags: needinfo?(kyle)
Attachment #8986045 - Attachment is obsolete: true
Attachment #8986045 - Flags: review?(kyle)
Comment on attachment 8986046 [details]
Bug 1469376 - Initial stubs for WebGPU sketch API. -

https://reviewboard.mozilla.org/r/251498/#review260808
Attachment #8986044 - Attachment is obsolete: true
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d7f2fdc5bf7
Initial stubs for WebGPU sketch API. - r=kvark,qdot
https://hg.mozilla.org/mozilla-central/rev/3d7f2fdc5bf7
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1473465
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: