-
Notifications
You must be signed in to change notification settings - Fork 33
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
Update prefetch spec after whatwg/html#6315. #212
Conversation
…changes for redirect handling, process-a-navigate-fetch, etc
It builds locally; once the prerendering.bs fixes land I'll retrigger the preview bot. In the meantime at least this still works: https://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/jeremyroman/nav-speculation/prefetch-6315/prefetch.bs |
@domenic ptal when you get the chance? I know it's kinda a huge diff. |
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.
Nice work!
prefetch.bs
Outdated
1. If |request|'s [=request/URL=] is not [=potentially trustworthy URL|potentially trustworthy=], then return "`Blocked`". | ||
1. Let |proposedPartitionKey| be the result of [=determining the network partition key=] given |request|'s [=request/reserved client=]. | ||
1. If |partitioningScheme| is "`same-partition`" and |proposedPartitionKey| is not equal to |sourcePartitionKey|, then return "`Blocked`". | ||
1. Otherwise, if |partitioningScheme| is "`uncredentialed`" and |proposedPartitionKey| is equal to |sourcePartitionKey|, then return "`Blocked`". |
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 think this blocks same-origin prefetches on totally-uncredentialed origins. Is that intentional?
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 don't think that's the case, given this "uncredentialed"
is only used when we've asked for a cross-partition prefetch.
In resolving your other comment this has ended up being perhaps more clear that "uncredentialed" here really means "we're crossing partitions, so we have to isolate ourselves from any existing credentials".
Co-authored-by: Domenic Denicola <d@domenic.me>
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9
Co-authored-by: Domenic Denicola <d@domenic.me>
Co-authored-by: Domenic Denicola <d@domenic.me>
…ulation into prefetch-6315
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.
Responding to most comments. The question about whether to just make "create navigation params by fetching" explicitly aware of prefetch is an interesting one which I'll need to think a little more about.
<p class="issue">At the moment, how IP anonymization is achieved is handwaved. This will probably be done in an [=implementation-defined=] manner using some kind of proxy or relay. Ideally this would be plumbed down to [=obtain a connection=], and possibly even the mechanism could be further standardized.</p> | ||
1. [=header list/Set a structured field value=] given (<a http-header>`` `Sec-Purpose` ``</a>, |purpose|) in |request|'s [=request/header list=]. | ||
<div algorithm="attempt to populate the history entry's document"> | ||
In <a spec=HTML>attempt to populate the history entry's document</a>, replace the step which invokes <a spec=HTML>create navigation params by fetching</a> with the following: |
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.
What a fun edge case. It seems the Chromium implementation does currently admit this for both traversal and reload navigations.
Adding a WPT for this: https://chromium-review.googlesource.com/c/chromium/src/+/4062248
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077079}
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077079}
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077079}
…d reload navigations." This reverts commit 1b56f238b3405214bbb11c8caf451735690219b4. Reason for revert: Test is failing on many Mac bots, see crbug.com/1394569 Original change's description: > prefetch: Add WPT showing that prefetch can match traverse and reload navigations. > > This came up during a spec PR review: > WICG/nav-speculation#212 > > Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 > Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> > Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> > Auto-Submit: Jeremy Roman <jbroman@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1077079} Bug: 1394569 Change-Id: I0943c75654ec4f8639713994cb390c7ed4e2e636 No-Presubmit: true No-Tree-Checks: true No-Try: true
…d reload navigations." This reverts commit 1b56f23. Reason for revert: Test is failing on many Mac bots, see crbug.com/1394569 Original change's description: > prefetch: Add WPT showing that prefetch can match traverse and reload navigations. > > This came up during a spec PR review: > WICG/nav-speculation#212 > > Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 > Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> > Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> > Auto-Submit: Jeremy Roman <jbroman@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1077079} Bug: 1394569 Change-Id: I0943c75654ec4f8639713994cb390c7ed4e2e636 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4064343 Owners-Override: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077358}
…d reload navigations." This reverts commit 1b56f238b3405214bbb11c8caf451735690219b4. Reason for revert: Test is failing on many Mac bots, see crbug.com/1394569 Original change's description: > prefetch: Add WPT showing that prefetch can match traverse and reload navigations. > > This came up during a spec PR review: > WICG/nav-speculation#212 > > Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 > Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> > Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> > Auto-Submit: Jeremy Roman <jbroman@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1077079} Bug: 1394569 Change-Id: I0943c75654ec4f8639713994cb390c7ed4e2e636 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4064343 Owners-Override: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077358}
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.
Looks like there's still some substantial feedback outstanding. Let me know if you want to box those away into <p class="issue">
boxes so we can move forward; I think that's very reasonable.
<p class="issue">At the moment, how IP anonymization is achieved is handwaved. This will probably be done in an [=implementation-defined=] manner using some kind of proxy or relay. Ideally this would be plumbed down to [=obtain a connection=], and possibly even the mechanism could be further standardized.</p> | ||
1. [=header list/Set a structured field value=] given (<a http-header>`` `Sec-Purpose` ``</a>, |purpose|) in |request|'s [=request/header list=]. | ||
<div algorithm="attempt to populate the history entry's document"> | ||
In <a spec=HTML>attempt to populate the history entry's document</a>, replace the step which invokes <a spec=HTML>create navigation params by fetching</a> with the following: |
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.
Fun!
Looks like the test got reverted; I hope we can add it back. It's weird that it would be failing on Macs only?
…d reload navigations." This reverts commit 1b56f238b3405214bbb11c8caf451735690219b4. Reason for revert: Test is failing on many Mac bots, see crbug.com/1394569 Original change's description: > prefetch: Add WPT showing that prefetch can match traverse and reload navigations. > > This came up during a spec PR review: > WICG/nav-speculation#212 > > Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 > Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> > Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> > Auto-Submit: Jeremy Roman <jbroman@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1077079} Bug: 1394569 Change-Id: I0943c75654ec4f8639713994cb390c7ed4e2e636 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4064343 Owners-Override: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077358}
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.
This looks great, I think merging into "create navigation params by fetching" worked well.
Giving you to the LGTM so that you can merge this at will, but I guess there might still be something worth checking about how we thread through the prefetch record.
prefetch.bs
Outdated
1. [=redirect chain/Append=] |request| to |prefetchRecord|'s [=prefetch record/redirect chain=]. | ||
1. Return "`Allowed`". | ||
Add the following [=struct/item=] to [=session history entry=]: | ||
* <dfn for="session history entry">prefetch record</dfn>, a [=prefetch record=] or null. If non-null, it indicates that a prefetch, not a normal navigation, is in progress. |
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.
It feels weird to store this in session history entry. It seems like it doesn't need to be there long term, and instead is just used because the relevant algorithms take a SHE. Can we instead just thread it to the relevant algorithms as an independent, sometimes-null parameter?
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.
The entire SHE here is admittedly a little fictitious because we're not actually putting it in session history just yet.
Made the suggested change.
…can match traverse and reload navigations., a=testonly Automatic update from web-platform-tests prefetch: Add WPT showing that prefetch can match traverse and reload navigations. This came up during a spec PR review: WICG/nav-speculation#212 Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077079} -- wpt-commits: 611dc0411c43984c7ab2abfa1810d97d065f7c71 wpt-pr: 37234
…refetch can match traverse and reload navigations.", a=testonly Automatic update from web-platform-tests Revert "prefetch: Add WPT showing that prefetch can match traverse and reload navigations." This reverts commit 1b56f238b3405214bbb11c8caf451735690219b4. Reason for revert: Test is failing on many Mac bots, see crbug.com/1394569 Original change's description: > prefetch: Add WPT showing that prefetch can match traverse and reload navigations. > > This came up during a spec PR review: > WICG/nav-speculation#212 > > Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 > Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> > Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> > Auto-Submit: Jeremy Roman <jbroman@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1077079} Bug: 1394569 Change-Id: I0943c75654ec4f8639713994cb390c7ed4e2e636 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4064343 Owners-Override: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077358} -- wpt-commits: db6deadcb828b9f872cc5284667d95ab523e698e wpt-pr: 37248
…d reload navigations." This reverts commit 1b56f238b3405214bbb11c8caf451735690219b4. Reason for revert: Test is failing on many Mac bots, see crbug.com/1394569 Original change's description: > prefetch: Add WPT showing that prefetch can match traverse and reload navigations. > > This came up during a spec PR review: > WICG/nav-speculation#212 > > Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 > Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> > Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> > Auto-Submit: Jeremy Roman <jbroman@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1077079} Bug: 1394569 Change-Id: I0943c75654ec4f8639713994cb390c7ed4e2e636 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4064343 Owners-Override: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077358}
…can match traverse and reload navigations., a=testonly Automatic update from web-platform-tests prefetch: Add WPT showing that prefetch can match traverse and reload navigations. This came up during a spec PR review: WICG/nav-speculation#212 Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077079} -- wpt-commits: 611dc0411c43984c7ab2abfa1810d97d065f7c71 wpt-pr: 37234
…refetch can match traverse and reload navigations.", a=testonly Automatic update from web-platform-tests Revert "prefetch: Add WPT showing that prefetch can match traverse and reload navigations." This reverts commit 1b56f238b3405214bbb11c8caf451735690219b4. Reason for revert: Test is failing on many Mac bots, see crbug.com/1394569 Original change's description: > prefetch: Add WPT showing that prefetch can match traverse and reload navigations. > > This came up during a spec PR review: > WICG/nav-speculation#212 > > Change-Id: Ia8fb3d6c9b164fa35b196b99f1feb5750ec359f9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062248 > Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> > Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> > Auto-Submit: Jeremy Roman <jbroman@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1077079} Bug: 1394569 Change-Id: I0943c75654ec4f8639713994cb390c7ed4e2e636 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4064343 Owners-Override: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/main@{#1077358} -- wpt-commits: db6deadcb828b9f872cc5284667d95ab523e698e wpt-pr: 37248
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 This is a reland of the following, except for extending the timeout. https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Fixed: 1394569 Change-Id: I1ed952f6e1fc065a7213d262dfe5dc86f722f6c8
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 This is a reland of the following, except for extending the timeout. https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Fixed: 1394569 Change-Id: I1ed952f6e1fc065a7213d262dfe5dc86f722f6c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4184810 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Cr-Commit-Position: refs/heads/main@{#1095624}
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 This is a reland of the following, except for extending the timeout. https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Fixed: 1394569 Change-Id: I1ed952f6e1fc065a7213d262dfe5dc86f722f6c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4184810 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Cr-Commit-Position: refs/heads/main@{#1095624}
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 This is a reland of the following, except for extending the timeout. https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Fixed: 1394569 Change-Id: I1ed952f6e1fc065a7213d262dfe5dc86f722f6c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4184810 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Cr-Commit-Position: refs/heads/main@{#1095624}
…can match traverse and reload navigations., a=testonly Automatic update from web-platform-tests prefetch: Add WPT showing that prefetch can match traverse and reload navigations. This came up during a spec PR review: WICG/nav-speculation#212 This is a reland of the following, except for extending the timeout. https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Fixed: 1394569 Change-Id: I1ed952f6e1fc065a7213d262dfe5dc86f722f6c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4184810 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Cr-Commit-Position: refs/heads/main@{#1095624} -- wpt-commits: eb8a9702e01eb3ce887ab3ceae934625b46a2632 wpt-pr: 38097
…can match traverse and reload navigations., a=testonly Automatic update from web-platform-tests prefetch: Add WPT showing that prefetch can match traverse and reload navigations. This came up during a spec PR review: WICG/nav-speculation#212 This is a reland of the following, except for extending the timeout. https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Fixed: 1394569 Change-Id: I1ed952f6e1fc065a7213d262dfe5dc86f722f6c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4184810 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Cr-Commit-Position: refs/heads/main@{#1095624} -- wpt-commits: eb8a9702e01eb3ce887ab3ceae934625b46a2632 wpt-pr: 38097
… navigations. This came up during a spec PR review: WICG/nav-speculation#212 This is a reland of the following, except for extending the timeout. https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Fixed: 1394569 Change-Id: I1ed952f6e1fc065a7213d262dfe5dc86f722f6c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4184810 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Cr-Commit-Position: refs/heads/main@{#1095624}
…can match traverse and reload navigations., a=testonly Automatic update from web-platform-tests prefetch: Add WPT showing that prefetch can match traverse and reload navigations. This came up during a spec PR review: WICG/nav-speculation#212 This is a reland of the following, except for extending the timeout. https://chromium-review.googlesource.com/c/chromium/src/+/4062248 Fixed: 1394569 Change-Id: I1ed952f6e1fc065a7213d262dfe5dc86f722f6c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4184810 Reviewed-by: Adithya Srinivasan <adithyas@chromium.org> Auto-Submit: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Cr-Commit-Position: refs/heads/main@{#1095624} -- wpt-commits: eb8a9702e01eb3ce887ab3ceae934625b46a2632 wpt-pr: 38097
This is a fairly comprehensive rewrite so hopefully it both respects the new navigables/etc infrastructure and reflects the previously specified functionality.