KEMBAR78
Cleanups on how we do tasks and promises by Manishearth · Pull Request #1032 · immersive-web/webxr · GitHub
Skip to content

Conversation

Manishearth
Copy link
Contributor

@Manishearth Manishearth commented May 11, 2020

fixes #1020, fixes #1022

I didn't update every instance of "queue a task" because both WebGL and HTML's Websocket section specify the task source used once in the document.


Preview | Diff

@Manishearth Manishearth requested a review from toji May 11, 2020 22:12
@Manishearth
Copy link
Contributor Author

cc @annevk

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I'd wait for @annevk to ack prior to merging.

1. If |mode| is {{XRSessionMode/"inline"}}, [=/resolve=] |promise| with <code>true</code> and return it.
1. If the requesting document's origin is not allowed to use the "xr-spatial-tracking" [[#feature-policy|feature policy]], [=reject=] |promise| with a "{{SecurityError}}" {{DOMException}} and return it.
1. Run the following steps [=in parallel=]:
1. [=ensures an immersive XR device is selected|Ensure an immersive XR device is selected=].
Copy link

@annevk annevk May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step is running in a parallel thread but then you invoke something that grabs objects from the main thread. I think you need to make that more clear and separate what happens on the main thread and what happens in parallel.

Also, the selection that happens here presumably updates the state on the main thread. That should happen in the same task that eventually resolves the promise. This also goes for the step below here where you resolve with false.

Does that makes sense?

(I suspect something similar applies to the other in parallel algorithm, but I did not look in detail.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so that's a little bit tricky: there are some parts of the ensure selected algorithm that need to run on the main thread, and some that don't. There doesn't seem to be an easy way to chain async algorithms in spec text.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to merge this PR because this is an unrelated issue, but filing a new one.

@Manishearth Manishearth merged commit b3a95eb into immersive-web:master May 12, 2020
@Manishearth Manishearth deleted the tasks branch May 12, 2020 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

All tasks should be queued with a task source Queue a task before resolving a promise

3 participants