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

Queue a task on the permissions task source #101

Merged
merged 8 commits into from
Apr 28, 2020

Conversation

yoavweiss
Copy link
Collaborator

@yoavweiss yoavweiss commented Apr 27, 2020

Closes #77


Preview | Diff

@yoavweiss yoavweiss requested a review from domenic April 27, 2020 11:08
index.bs Outdated Show resolved Hide resolved
@yoavweiss
Copy link
Collaborator Author

Also added a few changes to the processing model here based on feedback from @dtapuska on the relevant Chromium CL

index.bs Outdated

2. Run the following steps [=in parallel=]:
2. If [=this=]'s [=relevant global object=] is not identical to {{NavigatorUAData}}'s [=relevant global object=], then:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dtapuska - Is this a good equivalent to !GetExecutionContext()?

Choose a reason for hiding this comment

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

I don't think this is quite right. Isn't this the same as NavigatorUAData? I think what you mean is that the RelevantRealm != IncumbentRealm then throw WrongDocument ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Incumbent realm should not be used by new specs.

If you are trying to make sure that the document is alive, then you need to check if the relevant global object is a Window and its associated Document is fully active.

What are you actually trying to express here, in terms of developer-facing benefit, not in terms of Chromium implementation details?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you are trying to make sure that the document is alive, then you need to check if the relevant global object is a Window and its associated Document is fully active.

Makes sense, thanks!

What are you actually trying to express here, in terms of developer-facing benefit, not in terms of Chromium implementation details?

I'm trying to express that detached frames should reject here.
The developer-facing benefit is reduced confusion as the rest of UA data returns nullyfied values in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the rest of UA data returns nullyfied values in that case.

That doesn't appear to be true, at least for anything in https://html.spec.whatwg.org/multipage/system-state.html#client-identification .

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ugh, that's bizarre. I hope we can align Chromium with the spec? There's just no reason to make these getters dependent on state like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe the easiest path forward would be to downsize this PR to the task queueing only, drop the Promise rejection, find a different ExecutionContext to queue the task on in the Chromium implementation (from the ScriptState) and open an issue on the HTML spec to figure out the nullification of attributes on detached documents.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense to me.

Note that the spec as written rejects the promise but the rejection will never be seen, since the event loop is inactive for those documents.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the promise rejection, but left the realm on which the promise is created.

Note that the spec as written rejects the promise but the rejection will never be seen, since the event loop is inactive for those documents.

Is this because of the realm the promise is created with?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it's because the event loop just doesn't run for inactive documents.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Co-Authored-By: Domenic Denicola <d@domenic.me>
@yoavweiss yoavweiss merged commit f0bbe16 into WICG:master Apr 28, 2020
@yoavweiss yoavweiss deleted the task_queueing branch April 28, 2020 21:08
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.

What task queue should the Promise be queued on?
3 participants