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

Export terms referenced from w3c/ServiceWorker#1545 #6325

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yutakahirano
Copy link
Member

@yutakahirano yutakahirano commented Jan 25, 2021

@yutakahirano
Copy link
Member Author

w3c/ServiceWorker#1545 has not been approved yet. Probably reviewing it and this change together is good.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

One quick comment, but I'll let @annevk guide this into merging since it seems he has opinions on w3c/ServiceWorker#1545 (review) . Let me know if you want me to take back over though; we really should get w3c/ServiceWorker#1545 merged.

source Outdated
@@ -2744,7 +2744,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<ul class="brief">
<li><dfn data-x-href="https://tc39.es/ecma262/#active-function-object">active function object</dfn></li>
<li><dfn data-x-href="https://tc39.es/ecma262/#sec-agents">agent</dfn> and
<dfn data-x-href="https://tc39.es/ecma262/#sec-agent-clusters">agent cluster</dfn></li>
<dfn export data-x-href="https://tc39.es/ecma262/#sec-agent-clusters">agent cluster</dfn></li>
Copy link
Member

Choose a reason for hiding this comment

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

This should not be exported; instead you should reference the JavaScript spec directly.

@domenic
Copy link
Member

domenic commented Feb 26, 2021

Ping @annevk

@annevk
Copy link
Member

annevk commented Feb 27, 2021

I need a status update. As far as I can tell w3c/ServiceWorker#1545 never merged. Is there something I missed?

@yutakahirano
Copy link
Member Author

Sorry I don't remember why I closed the PR... Given https://github.com/w3c/ServiceWorker/pull/1545/files#r563704060, is it probably good to remake this PR for the definition of the operation referred in the comment?

@annevk
Copy link
Member

annevk commented Mar 1, 2021

Yeah, if that's not too much trouble I'd appreciate it. It seems nice to contain that somehow in case we can ever properly define (or remove) it.

@yutakahirano yutakahirano force-pushed the yhirano/export-cross-origin-isolation-mode branch from 0ff6aaf to 40ecfd8 Compare March 1, 2021 10:50
@yutakahirano
Copy link
Member Author

Updated.

@@ -2720,7 +2720,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<ul class="brief">
<li><dfn data-x-href="https://tc39.es/ecma262/#active-function-object">active function object</dfn></li>
<li><dfn data-x-href="https://tc39.es/ecma262/#sec-agents">agent</dfn> and
<dfn data-x-href="https://tc39.es/ecma262/#sec-agent-clusters">agent cluster</dfn></li>
<dfn export data-x-href="https://tc39.es/ecma262/#sec-agent-clusters">agent cluster</dfn></li>
Copy link
Member

Choose a reason for hiding this comment

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

If service workers needs to talk about agent clusters it should also reference JavaScript directly.

Copy link
Member

Choose a reason for hiding this comment

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

This still applies.

source Outdated
<span>implementation-defined</span>.</p>
data-x="agent-cluster-cross-origin-isolation">cross-origin isolation mode</span> to the result
of <span data-x="choose a cross-origin isolation mode">choosing a cross-origin isolateion
mode</span> with <var>worker global scope</var>.

<p class="XXX">This really ought to be set when the agent cluster is created, which requires a
redesign of this section.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Okay, I guess I have to take my suggestion back. As the real answer here is that this should be part of "obtain a worker/worklet agent" once this has been redone. I'm sorry. Would this just require reverting the second commit here? I'm happy to help.

@yutakahirano yutakahirano force-pushed the yhirano/export-cross-origin-isolation-mode branch from 40ecfd8 to 5539aeb Compare March 5, 2021 08:36
@yutakahirano
Copy link
Member Author

Reverted 40ecfd8, and now HEAD is 5539aeb.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Is there a new corresponding SW PR?

@@ -2720,7 +2720,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<ul class="brief">
<li><dfn data-x-href="https://tc39.es/ecma262/#active-function-object">active function object</dfn></li>
<li><dfn data-x-href="https://tc39.es/ecma262/#sec-agents">agent</dfn> and
<dfn data-x-href="https://tc39.es/ecma262/#sec-agent-clusters">agent cluster</dfn></li>
<dfn export data-x-href="https://tc39.es/ecma262/#sec-agent-clusters">agent cluster</dfn></li>
Copy link
Member

Choose a reason for hiding this comment

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

This still applies.

@@ -87210,7 +87213,7 @@ interface <dfn>BeforeUnloadEvent</dfn> : <span>Event</span> {

<div w-nodev>

<p>An <span>agent cluster</span> has an associated <dfn
<p>An <span>agent cluster</span> has an associated <dfn export
Copy link
Member

Choose a reason for hiding this comment

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

This needs for="agent cluster", right?

@domenic
Copy link
Member

domenic commented Aug 17, 2021

Ping @yutakahirano on resolving review comments.

@yutakahirano
Copy link
Member Author

Sorry I lost the context. This was needed for w3c/ServiceWorker#1545 which I closed. We should probably close this, but it seems we still lack logic to set cross-origin isolated correctly for service workers. @ArthurSonzogni, is my understanding correct? Should I create another PR?

@ArthurSonzogni
Copy link
Member

Sorry I lost the context. This was needed for w3c/ServiceWorker#1545 which I closed. We should probably close this, but it seems we still lack logic to set cross-origin isolated correctly for service workers. @ArthurSonzogni, is my understanding correct? Should I create another PR?

I asked offline @yutakahirano how this question relates to me, and this was about:
#6060. The latter was about giving me the possibility to enforce COOP on Android webview, despite not being able to grant the cross-origin-isolation capability at the end on this platform. That's because we can't spawn more than one process.

we still lack logic to set cross-origin isolated correctly for service workers. @ArthurSonzogni, is my understanding correct?

Yes, I believe this is correct. I guess you want to export the 3 values for cross-origin isolation mode (e.g. this patch) so that it can be used in the ServiceWorker spec to define what it means to be COI from the ServiceWorker (e.g. w3c/ServiceWorker#1545). Not sure why the latter has been closed. You may want to re-open it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants