Don't check XR compat flag for inline sessions #705
Conversation
|
I don't think this is the complete solution. This doesn't address the case where there are multiple inline sessions present, since it invalidates the other inline sessions. A fix for that is to only unset the flag for It feels like Alternatively, we can make inline sessions skip this flag completely. I might just do that. |
This was cleaner, ended up doing it. cc @asajeffrey |
|
I don't think this is the right approach. As long as the underlying XR device hasn't changed there's no reason that the compatibility bit would become invalid between one XR session and the next. The only time that this would be problematic is if the XR device is changed, and that should already be covered by the algorithm here: http://localhost:8080/webxr/#select-an-xr-device In retrospect I'm not sure why we left #607 open, as it likely could have been easily explained and closed. |
|
In that case should I keep the portion of this PR that ignores the flag for inline sessions? |
|
Since that's a problem regardless of this. |
Manishearth
force-pushed
the
session-shutdown-xrcompat
branch
from
64add9d
to
06597e1
Compare
|
Editors talked this over and we do want to keep the text about ignoring the flag for inline sessions, but we feel the current context compatibility set/unset logic is correct and should not be changed. If you can refactor the PR to only include the change to the inline check, we'll review it again. Thanks! |
Manishearth
force-pushed
the
session-shutdown-xrcompat
branch
from
06597e1
to
05f8dcd
Compare
|
Done! |
Manishearth
changed the title
There was a problem hiding this comment.
Sorry. Looks like your PR has merge conflicts due to a copious amount of recent merges from checks notes @Manishearth. To prevent future conflicts please contact the offending contributor and instruct him to stop being so damn productive. 😉
index.bs
Outdated
| @@ -1605,7 +1605,7 @@ The <dfn constructor for="XRWebGLLayer">XRWebGLLayer(|session|, |context|, |laye | |||
| 1. Let |layer| be a new {{XRWebGLLayer}} | |||
| 1. If |session|'s [=ended=] value is <code>true</code>, throw an {{InvalidStateError}} and abort these steps. | |||
| 1. If |context| is lost, throw an {{InvalidStateError}} and abort these steps. | |||
| 1. If |context|'s [=XR compatible=] boolean is false, throw an {{InvalidStateError}} and abort these steps. | |||
| 1. If |context|'s [=XR compatible=] boolean is false and |session|'s [=XRSession/mode=] is not {{XRSessionMode/"inline"}}, throw an {{InvalidStateError}} and abort these steps. | |||
There was a problem hiding this comment.
Let's change this to use the [=immersive session=] definition instead, which should be a bit more future-proof.
If |session| is an [=immersive session=] and |context|'s [=XR compatible=] boolean is <code>false</code>, throw an {{InvalidStateError}} and abort these steps.
Manishearth
force-pushed
the
session-shutdown-xrcompat
branch
from
05f8dcd
to
9a4e4a1
Compare
Manishearth
force-pushed
the
session-shutdown-xrcompat
branch
from
9a4e4a1
to
c7e48a4
Compare
|
🤣 done! |
|
👍 |
fixes #607
r? @NellWaliczek