Skip to content

Defer initial inputsourcechange event till after the promise resolves #1002

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

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

Manishearth
Copy link
Contributor

Fixes #961

As mentioned in #961, this is not the only way to fix this issue; the other way to fix it involves changing the spec so that inputSources can start off non-null, however that involves ensuring the ecosystem respects that as well.

r? @toji

cc @cabanier

@@ -382,6 +382,13 @@ When this method is invoked, the user agent MUST run the following steps:
<dd> Append |session| to the [=list of inline sessions=].
</dl>
1. [=/Resolve=] |promise| with |session|.
1. [=Queue a task=] to perform the following steps:
Copy link
Member

Choose a reason for hiding this comment

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

Does the Resolve step wait until the task is done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the other way around, resolve uses a microtask, and queue a task uses a regular task, so we can guarantee that the callback attached in requestSession(..).resolve(callback) will execute before the task below does.

Copy link
Member

Choose a reason for hiding this comment

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

ok. Resolve actually calls the then function. I think this looks good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

er, i should have written then instead of resolve in my comment above, but you get the idea

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! I think this is a pretty clean way of resolving the timing ambiguities.

@Manishearth Manishearth merged commit ab433a3 into immersive-web:master Apr 21, 2020
@Manishearth
Copy link
Contributor Author

@klausw you might want to ensure the impl in chrome does this deferring properly

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.

Timing of initial inputsourceschange event
3 participants