Skip to content

Cleanups on how we do tasks and promises - #1032

Merged
Manishearth merged 2 commits into
immersive-web:masterfrom
Manishearth:tasks
May 12, 2020
Merged

Manishearth merged 2 commits into
immersive-web:masterfrom
Manishearth:tasks

Conversation

@Manishearth

@Manishearth Manishearth commented May 11, 2020 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

cc @annevk

@toji toji left a comment

Copy link
Copy Markdown
Member

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.

Comment thread index.bs
@@ -313,8 +313,8 @@ When this method is invoked, it MUST run the following steps:
1. Run the following steps [=in parallel=]:
1. [=ensures an immersive XR device is selected|Ensure an immersive XR device is selected=].

@annevk annevk May 12, 2020 •

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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