search

immersive-web / hit-test

https://immersive-web.github.io/hit-test/
Other
78 stars 31 forks source link

Update XRRay vector constructor to use correct webidl semantics #85

Closed Manishearth closed 4 years ago

Manishearth commented 4 years ago

Fixes https://github.com/immersive-web/hit-test/issues/84

This adds a new initializer type so that the defaults work correctly.

This also throws an error for non-1 w coordinates for the origin, similar to XRRigidTransform.

r? @bialpio

Preview | Diff

bialpio commented 4 years ago

Looks good, merging now.

Manishearth commented 4 years ago

On the Chrome side, y'all ideally should update webidl parsing to the newer thing, however if not you should then make the default behavior work as much as possible and change the error type to TypeError

Manishearth commented 4 years ago

Oh, and the tests need fixing, but I might end up doing that myself if my Servo changes land first.

bialpio commented 4 years ago

Chrome's behavior was incorrect because the implementation relied on multiple overloads for the constructor, I've reached out internally and was told that the change to require = {} in Web IDL was a syntactic change, w/o changing the semantics.

For reference, I've sent out initial CL with the fix (including WPTs): https://crrev.com/c/2145937.

Manishearth commented 4 years ago

@bialpio it changes the semantics in the sense that you can no longer receive "nullable" dictionaries in webidl

Also, you should use assert_throws in the test, see https://github.com/Manishearth/servo/blob/2bb06b338c8dc606078736c43c5fda9c13e6357c/tests/wpt/web-platform-tests/webxr/hit-test/xrRay_constructor.https.html#L91 . I can just directly upstream those test changes if you'd like.

bialpio commented 4 years ago

you can no longer receive "nullable" dictionaries in webidl

IIUC, that's the case even prior to this change, but the reason why we were getting them is because Chrome's XRRay Web IDL was not matching the one in the spec: https://chromium-review.googlesource.com/c/chromium/src/+/2145937/1/third_party/blink/renderer/modules/xr/xr_ray.idl#b11 - note the 0, 1 and 2 parameter overloads. I think I got it working in Chrome in a way that made sense in WPTs but forgot to adjust the specification. :(

If you're ready to upstream your changes to the tests then go for it, I think it'll take me a while to go through CL with my change.

Manishearth commented 4 years ago

Ah, I see! I'd just assumed the spec had the webidl chrome was using :)

My tests can be found in https://github.com/web-platform-tests/wpt/pull/22868 , if you review that PR we can just go ahead and land it!