-
Notifications
You must be signed in to change notification settings - Fork 42
Clarify prefetch vs. activation navigable issues #391
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
Conversation
Preview: |
e326dd8
to
ede03fe
Compare
ede03fe
to
355fd7b
Compare
The PR itself looks good to explain the current spec behavior, while I'm also not sure about expected COOP behavior. Anyway brainstorming... |
As discussed in #384 (comment) I think the current spec is broken and I don't like it. For Chromium's behavior, in internal chat Taiyo (cannot find his GitHub handle) said:
So, if you think it is OK, I think I would rather update this PR to remove all prefetch-time COOP checking and move it all to activation time. |
355fd7b
to
81ca693
Compare
Recent commits fixed the COOP issues. I've pushed an update to this PR which expands on how those fixes work, and in general on the different navigables, in a larger non-normative note. @hiroshige-g and/or @domfarolino, can you review this? |
Ping for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just mention in the OP that the "issues" referenced in the title are mostly closed by #394?
prefetch.bs
Outdated
|
||
This is, actually, fine. During the [=navigate|navigation=] that results in prefetch activation, the target navigable will still be used for most of the important checks, which are performed earlier in the process before <a spec=HTML>attempt to populate the history entry's document</a> is called. And, looking at all the ways in which the navigable impacts the prefetch-time [=create navigation params by fetching=]: | ||
|
||
* Setting the prefetch request's [=request/reserved client=] to |document|'s [=node navigable=] is expected. We have to pick one at prefetch time, and this is the only sensible choice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked a little more detail:
navigable
doesn't affect the reserved client other than
https://html.spec.whatwg.org/multipage/webappapis.html#concept-environment-target-browsing-context, because "prefetches are only supported in top-level navigables", and- https://html.spec.whatwg.org/multipage/webappapis.html#concept-environment-target-browsing-context is never used in the ServiceWorker spec for clients with execution ready flag not set. (I'm not sure whether it is used outside ServiceWorker/HTML spec though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I analyzed this in #384 (comment) a bit. I'll maybe add a link to that comment here or something. Thank you for double-checking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Closes #384
Obsolete original discussion
I'm actually unsure whether we perform the COOP checks correctly now. I think this might have gotten messed up #358. So keeping this as draft while I figure that out.