Skip to content

Fix webidl for XRRay #83

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Fix webidl for XRRay #83

wants to merge 3 commits into from

Conversation

@Manishearth
Copy link
Contributor

@Manishearth Manishearth commented Apr 9, 2020 •
edited by pr-preview bot
Loading

r? @bialpio

Preview | Diff

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020

Actually it seems like one of the tests asserts that this throws an error, so i'm just going to switch that over to an error

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020 •
edited
Loading

Actually, ugh, this isn't right either, because now new XRRay(point) doesn't work, and new XRRay(point, {}) will throw an error.

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020

At least in Servo the overload isn't working when we pass a single argument, either.

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020 •
edited
Loading

Yeah so the current model is fundamentally broken, it relies on obsolete webidl behavior (which is no longer allowed by Firefox and Servo's webidl parser and cannot be implemented).

It is not possible to distinguish between a null and initial-value dict argument.

Furthermore, new XRRay(domPoint) will not work if domPoint is a DomPoint object (or DomPointReadOnly) as tested by xrRay_constructor.html. The overload resolution algorithm step 12, substep 4, will notice that domPoint is a "platform object" and focus on resolving against the XRRigidTransform constructor, and at that point fail. It is a bug in Chrome's webidl implementation that new XRRay(new DOMPoint({x: 1, y: 0, z: 0})) works at all.

But I kind of want that to work -- we might need to introduce an additional single-argument DOMPointReadonly overload.

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020 •
edited
Loading

The overload resolution algorithm step 12, substep 4, will notice that domPoint is a "platform object" and focus on resolving against the XRRigidTransform constructor, and at that point fail. It is a bug in Chrome's webidl implementation that new XRRay(new DOMPoint({x: 1, y: 0, z: 0})) works at all.

Hmm, actually, maybe not, it requires that V implement it. Could be a bug on our side, but I'm not sure.

Update: yeah that's a bug on our side.

The original change in this PR still stands.

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 9, 2020
@Manishearth
Update xrRay_constructor test to not throw
cc6cfe6 
See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.
@bialpio
Copy link
Contributor

bialpio commented Apr 9, 2020

I'm not sure if we should silently accept new XRRay({}, {}); - it is equivalent to new XRRay({}, {x:0, x:0, y:0, w:1}), and I think this should be rejected instead of automatically fixed by us. Would updating the spec to accept DOMPointReadOnlys instead of DOMPointInits solve this issue in a non-API-breaking way?

And now it occurred to me that we ignore ws of the passed in dictionaries - it feels wrong that we don't even validate it, but then we won't be able to correctly validate the default direction (since w needs to be set to 0 there...).

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020

@bialpio there is no way to distinguish between new XRRay({}, {}) and new XRRay({})

Using DOMPointReadOnlys is an API breaking change, because currently you can pass a dictionary and making it use DOMPointReadOnly will force you to use an interface.

And yeah, we ignore the ws, i'd noticed that but wasn't sure how best to handle that. We can divide by w for position and error in the zero case, but there's not much we can do for direction.

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020

Oh, another option we have available is to define a new dictionary for direction that has different defaults. Thoughts?

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020

It's not pretty, but it would work.

@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020

Should I file an issue for this with the various options?

@bialpio
Copy link
Contributor

bialpio commented Apr 9, 2020

I like the idea of having a new dictionary - I think it checks all the boxes (should not be a breaking change, we won't be auto-fixing incorrect parameters, allows us to correctly validate & handle w both for position and direction). Sure, go ahead and file a new issue.

@Manishearth Manishearth mentioned this pull request Apr 9, 2020
Closed
@Manishearth
Copy link
Contributor Author

Manishearth commented Apr 9, 2020

I'm closing this and will reopen a fresh PR

@Manishearth Manishearth closed this Apr 9, 2020
Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 11, 2020
@Manishearth
Update xrRay_constructor test to not throw
f91b4dd 
See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.
servo-wpt-sync pushed a commit to servo-wpt-sync/web-platform-tests that referenced this pull request Apr 11, 2020
@Manishearth
Update xrRay_constructor test to not throw
f8806b9 
See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.
servo-wpt-sync pushed a commit to servo-wpt-sync/web-platform-tests that referenced this pull request Apr 13, 2020
@Manishearth
Update xrRay_constructor test to not throw
2702d44 
See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.
Manishearth added a commit to web-platform-tests/wpt that referenced this pull request Apr 14, 2020
@Manishearth
Update xrRay_constructor test to not throw
7513c92 
See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Apr 16, 2020
@Manishearth @moz-wptsync-bot
Bug 1629229 [wpt PR 22868] - Implement hit testing API, a=testonly
f7bc0fe 
Automatic update from web-platform-tests
Update xrRay_constructor test to not throw

See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.

--
Update XRRay semantics

Implements changes from immersive-web/hit-test#85

--
Fix world generation to have vertices member

--

wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814
wpt-pr: 22868
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 16, 2020
@Manishearth @moz-wptsync-bot
Bug 1629229 [wpt PR 22868] - Implement hit testing API, a=testonly
09d92b2 
Automatic update from web-platform-tests
Update xrRay_constructor test to not throw

See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.

--
Update XRRay semantics

Implements changes from immersive-web/hit-test#85

--
Fix world generation to have vertices member

--

wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814
wpt-pr: 22868
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Apr 20, 2020
@marco-c
Bug 1629229 [wpt PR 22868] - Implement hit testing API, a=testonly
847da81 
Automatic update from web-platform-tests
Update xrRay_constructor test to not throw

See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.

--
Update XRRay semantics

Implements changes from immersive-web/hit-test#85

--
Fix world generation to have vertices member

--

wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814
wpt-pr: 22868

UltraBlame original commit: 309322031e57b9f606c20274ad42711703753086
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Apr 20, 2020
@marco-c
Bug 1629229 [wpt PR 22868] - Implement hit testing API, a=testonly
d2f2a97 
Automatic update from web-platform-tests
Update xrRay_constructor test to not throw

See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.

--
Update XRRay semantics

Implements changes from immersive-web/hit-test#85

--
Fix world generation to have vertices member

--

wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814
wpt-pr: 22868

UltraBlame original commit: 309322031e57b9f606c20274ad42711703753086
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Apr 20, 2020
@marco-c
Bug 1629229 [wpt PR 22868] - Implement hit testing API, a=testonly
f5bb256 
Automatic update from web-platform-tests
Update xrRay_constructor test to not throw

See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.

--
Update XRRay semantics

Implements changes from immersive-web/hit-test#85

--
Fix world generation to have vertices member

--

wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814
wpt-pr: 22868

UltraBlame original commit: 309322031e57b9f606c20274ad42711703753086
bhearsum pushed a commit to mozilla-releng/staging-firefox that referenced this pull request May 1, 2025
@Manishearth @moz-wptsync-bot
Bug 1629229 [wpt PR 22868] - Implement hit testing API, a=testonly
f3b5a6b 
Automatic update from web-platform-tests
Update xrRay_constructor test to not throw

See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification.

--
Update XRRay semantics

Implements changes from immersive-web/hit-test#85

--
Fix world generation to have vertices member

--

wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814
wpt-pr: 22868
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Manishearth @bialpio