Fix webidl for XRRay

[Manishearth]

• [Constructor] is now constructor()
  • Optional dictionary arguments need to be initialized to {} in the webidl https://github.com/heycam/webidl/pull/750
  • Given that the dictionary argument is initialized to {}, we can't check if it's null, we have to check if all fields are zero (which we should have been checking anyway).

r? @bialpio

---

Preview | Diff

[Manishearth]

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]

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

[Manishearth]

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

[Manishearth]

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]

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.

[bialpio]

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]

@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]

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

[Manishearth]

It's not pretty, but it would work.

[Manishearth]

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

[bialpio]

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]

I'm closing this and will reopen a fresh PR