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

Consider merging immersive-ar into WebXR #32

Closed
saschanaz opened this issue Sep 26, 2019 · 20 comments · Fixed by #38
Closed

Consider merging immersive-ar into WebXR #32

saschanaz opened this issue Sep 26, 2019 · 20 comments · Fixed by #38
Assignees
Milestone

Comments

@saschanaz
Copy link

Currently there are two enums named XRSessionMode in Web IDL world, one from WebXR Device API and another from WebXR AR module. As they can cause potential confusion, could we consider merging those two?

Relevant issues:

@Manishearth
Copy link
Contributor

Part of the reason behind the module split was so that we could cleanly define all of the AR stuff in a separate module. Moving this back makes modules less useful.

That said, we could do the minimal change here: move immersive-ar into the WebXR spec, define it as erroring early in requestSession() and isSessionSupported(), and define immersive session as containing it. This spec can then change it to a non-error.

@blairmacintyre
Copy link
Contributor

Isn't there a way to say "this is additive to the thing in the main spec"? It's understood that this module build on webxr, so in general we'll need a convention to say "add or change some definition in the main spec"

@saschanaz
Copy link
Author

Currently there is no machine-readable way to say an enum is an extension of another one. There is an existing discussion to allow it: whatwg/webidl#184

@Manishearth
Copy link
Contributor

@blairmacintyre No, see whatwg/webidl#184 . Talking to folks at TPAC it seemed like partial enums weren't really desirable.

This is doable for spec text (which we'll have to do a bunch), but less so for webidls, especially since they're slurped by various tools (which is AIUI the context of this issue). It might be possible to tweak the tooling to support this, idk.

@mounirlamouri
Copy link
Contributor

In this case, it may be possible to simply define immersive-ar in the core spec and say that the feature is defined as part of webx-ar and if the implementation doesn't support this spec, it should say it's not supported.

It's fairly similar to what @Manishearth suggests above with the exception that I do not think we should have one spec requiring to fail and another saying otherwise because the conflict between the two specs would prevent browsers to properly implement one or the other.

@rcabanier
Copy link
Contributor

Wouldn't that make it harder to add other modes later (ie handheld-ar)
What happened to the proposal to just make it a DOMString and not rely in WebIDL to do the parsing?

@Manishearth
Copy link
Contributor

Manishearth commented Sep 27, 2019

I do not think we should have one spec requiring to fail and another saying otherwise because the conflict between the two specs would prevent browsers to properly implement one or the other

I think we have precedent with specs explicitly overriding others, especially if we call it out in the first spec. There will need to be some form of overriding no matter what.

We could also do it as a DOMString, yes. I feel like that's not as good for documentation.

@saschanaz
Copy link
Author

I think @mounirlamouri's suggestion matches what Permissions spec does: it simply describes it's for another spec and skips everything else.

@mounirlamouri
Copy link
Contributor

Wouldn't that make it harder to add other modes later (ie handheld-ar)

No, it wouldn't. The only impact would be that if we send a version of WebXR to REC, it may miss new values that are in the ED. Though, this is mostly an administrative problem, it has no practical impact.

What happened to the proposal to just make it a DOMString and not rely in WebIDL to do the parsing?

DOMString seems to be a counter-pattern here. partial enum are being pushed back because the TAG and the community at large is pushing against specs editing the behaviour of other specs. Trying to make things DOMString because it would facilitate doing that may not be the right approach in spirit.

@Manishearth
Copy link
Contributor

/agenda to figure out what we want to do here

@probot-label probot-label bot added the agenda label Sep 30, 2019
@Manishearth Manishearth added this to the October 2019 milestone Sep 30, 2019
@toji
Copy link
Member

toji commented Oct 1, 2019

Honestly my preference would be to keep enums well scoped to the documents that define their behavior, but given that it appears partial enums are not being considered I don't think we can conscionably continue to write spec text as if they exist.

That leaves us with two clear alternatives:

  • Define all known enum values in the document that the enum is declared in and clearly link out to the appropriate document for the behavioral description.
  • Maintain a separate document that defines just the enum and links to the appropriate behavioral descriptions for each value. (This is, effectively, the Permissions approach.)

The latter approach is certainly more extensible, and it may be prudent for something that we expect to be amended frequently. (Perhaps this is how we should manage the feature descriptors list?) It does feel like overkill for an enum that is expected to only have 3-4 items in it in the forseeable future, though.

That's especially true for an enum defined in a spec outside the IW group's control, like Gamepad. In that case it seems pretty clear that the recommendation is that we work with the host spec to incorporate our desired enum value. It's obviously not desirable to jump into asking a different spec for modifications right away, though, so I would recommend that until a proposal solidifies we leave an ISSUE in the spec text indicating the desired additions to the other spec in lieu of duplicating the enum or faking partial enum support.

Given all that, my suggested changes (though I'm not particularly excited about them) are:

  • Move the "immersive-ar" enum value into the WebXR spec with a link to the webxr-ar-module spec for the definition.
  • Send a pull request to the gamepad API to add the "xr-standard" enum value with a link to the webxr-gamepad-module for the definition.
  • Evaluate if there's any similar lists in the core WebXR spec that we feel will be modified by multiple modules in the future and consider moving them into a separately maintained list (feature descriptors are a strong candidate for this.)

@rcabanier
Copy link
Contributor

What happened to the proposal to just make it a DOMString and not rely in WebIDL to do the parsing?

DOMString seems to be a counter-pattern here. partial enum are being pushed back because the TAG and the community at large is pushing against specs editing the behaviour of other specs. Trying to make things DOMString because it would facilitate doing that may not be the right approach in spirit.

The whole point of modularizing is to change the behavior of the WebXR spec. If the TAG is against this, do we need to revisit that decision?

Another advantage of moving to DOMString is that isSessionSupported won't throw if you pass a value that the UA doesn't recognize. It's a bit weird that a developer will have to test for both the false flag and an exception.

@mounirlamouri
Copy link
Contributor

The whole point of modularizing is to change the behavior of the WebXR spec. If the TAG is against this, do we need to revisit that decision?

This decision happened before I joined this project. I am missing too much context to answer. This said, in principle, having modules for incubation doesn't require us to have different specs when going to REC. The modules could be merged into the main spec when they are ready. They could also be treated as different spec.

Another advantage of moving to DOMString is that isSessionSupported won't throw if you pass a value that the UA doesn't recognize. It's a bit weird that a developer will have to test for both the false flag and an exception.

This is a fair point and a common concern. In practice, we don't really expect too many values here and immersive-ar may as well be the last. Having immersive-ar in the main spec would actually help here as the spec could recommend to return not supported instead of throwing when AR isn't implemented.

@Manishearth
Copy link
Contributor

partial enum are being pushed back because the TAG and the community at large is pushing against specs editing the behaviour of other specs

Is this really the case? My understanding of this when talking to webidl (and one TAG) person, informally, was that they don't like webidl partials (dictionaries and enums, interfaces are kinda necessary) because they cause problems, but specs extending each other in spec text is okay. I was mostly focused on the partial enums though, so I could be wrong here.

@mounirlamouri
Copy link
Contributor

Is this really the case? My understanding of this when talking to webidl (and one TAG) person, informally, was that they don't like webidl partials (dictionaries and enums, interfaces are kinda necessary) because they cause problems, but specs extending each other in spec text is okay. I was mostly focused on the partial enums though, so I could be wrong here.

I was told by one person that they consider partial dictionary to be a mistake and by two others that they would prefer avoiding any type of partial. They can't walk back this very easily but even for partial Navigator, some consider that the Navigator interface should be extended to say that xr (or whatever) else is present and defined in the XR spec. I do not think the TAG had has an official position on this but even TAG members or prominent member of the community have this opinion.

I didn't say that it wasn't okay to extend another spec but it's not okay to modify its behaviour in the manner of a hot fix/patch in the other spec. The line between extending and modifying can be thin and I wouldn't be surprised that some of the guidelines surfacing are to avoid people walking on the wrong side of it: if you can't have partial interfaces/dictionary, you must surface the entry point to your change in the main spec.

@Manishearth
Copy link
Contributor

Yep, what I heard was also that many consider partial dictionary to be a mistake, and that partial enum would be a mistake as well, along with a similarly iffy opinion on partial interface. Having modules that extend each other in a non-webidl sense didn't seem controversial from what I heard, but I can ask again explicitly (and perhaps we should formally ask the TAG to figure out how we should be doing this)

@mounirlamouri
Copy link
Contributor

That's all correct. The important part is what you call "extend" in this context.

@bialpio
Copy link
Contributor

bialpio commented Oct 8, 2019

Related issue: immersive-web/webxr#817.

@AdaRoseCannon
Copy link
Member

Removing the agenda label because it was discussed, if it needs more time feel free to re-agenda it.

@probot-label
Copy link

probot-label bot commented Oct 10, 2019

This issue is fixed by PR #38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants