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.

Sign up for GitHub

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

Use sourceDocument if set for prefetch lookup #267

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Use sourceDocument if set for prefetch lookup #267

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d97b00
Use sourceDocument for prefetch lookup in prerendering navigation
hiroshige-g Apr 10, 2023
d3d7be8
Always use sourceDocument if set
hiroshige-g Apr 17, 2023
c7658e8
Remove obsolete anchor
hiroshige-g Apr 17, 2023
dc87b98
Rename to sourceDocumentForPrefetches
hiroshige-g Apr 17, 2023
4474dc5
Plumb via source snapshot params
hiroshige-g Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions prefetch.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,19 +230,18 @@ The user agent may [=prefetch record/cancel and discard=] records from the [=Doc
</div>

<div algorithm="find a matching prefetch record">
To <dfn export>find a matching prefetch record</dfn> given a {{Document}} |document|, [=URL=] |url|, and [=sandboxing flag set=] |sandboxFlags|, perform the following steps.
To <dfn export>find a matching prefetch record</dfn> given a [=source snapshot params=] |sourceSnapshotParams|, [=URL=] |url|, and [=sandboxing flag set=] |sandboxFlags|, perform the following steps.

1. [=Assert=]: |document| is [=Document/fully active=].
1. Let |currentTime| be the [=current high resolution time=] for the [=relevant global object=] of |document|.
1. [=list/For each=] |record| of |document|'s [=Document/prefetch records=]:
1. Let |currentTime| be the [=current high resolution time=] for |sourceSnapshotParams|'s [=source snapshot params/fetch client=]'s [=environment settings object/global object=].
1. [=list/For each=] |record| of |sourceSnapshotParams|'s [=source snapshot params/prefetch records=]:
1. If |record|'s [=prefetch record/URL=] is not equal to |url|, then [=iteration/continue=].
1. If |record|'s [=prefetch record/state=] is not "`completed`", then [=iteration/continue=].
1. If |record|'s [=prefetch record/sandboxing flag set=] is empty and |sandboxFlags| is not empty, then [=iteration/continue=].

<div class="note">
Strictly speaking, it would still be possible for this to be valid if sandbox flags have been added to the container since prefetch but those flags would not cause an error due to cross origin opener policy. This is expected to be rare and so isn't handled.
</div>
1. [=list/Remove=] |record| from |document|'s [=Document/prefetch records=].
1. [=list/Remove=] |record| from |sourceSnapshotParams|'s [=source snapshot params/prefetch records=].
1. If |record|'s [=prefetch record/expiry time=] is less than |currentTime|, return null.
1. If |record|'s [=prefetch record/redirect chain=] [=redirect chain/has updated credentials=], return null.
1. Return |record|.
Expand Down Expand Up @@ -411,6 +410,14 @@ Update all creation sites to supply an empty string, except for any in this docu

<hr>

Add an additional item to [=source snapshot params=] as follows:
: <dfn for="source snapshot params">prefetch records</dfn>
:: a a [=list=] of [=prefetch records=].

When to [=snapshot source snapshot params=], set [=source snapshot params/prefetch records=] to <var ignore>sourceDocument</var>'s [=Document/prefetch records=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably make a [=list/clone=] here (https://infra.spec.whatwg.org/#list-clone) to reflect the idea we're snapshotting.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not so sure: in general creating a snapshot would be a good idea to avoid unintentional race conditions, while in this case not taking a clone might be also an option,

  • As [=list/clone=] is a shallow copy, this isn't a complete frozen snapshot of the prefetch records.
  • When a prefetch is still ongoing at the time of navigation start and we want to use the prefetch result after response is received after navigation start, we might need intentional interactions via (non-snapshot) prefetch records (IIUC such case is not yet specified so though)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I can see your concerns. However, I think it'd most fit the spirit of source snapshot params to start with a shallow clone for now, reflecting the idea that any prefetches added after the prerender navigation starts, should not influence the prerender navigation. Similar to how, e.g., changes to the page's origin or sandboxing flags after navigation starts, should not affect the (normal, non-prerender) navigation.

I agree that it's a little strange that we're not doing a deep clone. But that's kind of the nature of prefetch records being things that are updated over time. I think that part is OK.


<hr>

<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:

Expand All @@ -422,7 +429,7 @@ Update all creation sites to supply an empty string, except for any in this docu

1. Let |request| be the result of [=creating a navigation request=] given <var ignore>entry</var>, <var ignore>sourceSnapshotParams</var>'s [=source snapshot params/fetch client=], <var ignore>navigable</var>'s [=navigable/container=], and <var ignore>sourceSnapshotParams</var>'s [=source snapshot params/has transient activation=].
1. Set |request|'s [=request/replaces client id=] to <var ignore>navigable</var>'s [=navigable/active document=]'s [=relevant settings object=]'s [=environment/id=].
1. Let |prefetchRecord| be the result of [=finding a matching prefetch record=] given <var ignore>navigable</var>'s [=navigable/active document=], <var ignore>entry</var>'s [=session history entry/URL=], and <var ignore>targetSnapshotParams</var>'s [=target snapshot params/sandboxing flags=].
1. Let |prefetchRecord| be the result of [=finding a matching prefetch record=] given <var ignore>sourceSnapshotParams</var>, <var ignore>entry</var>'s [=session history entry/URL=], and <var ignore>targetSnapshotParams</var>'s [=target snapshot params/sandboxing flags=].
1. If <var ignore>documentResource</var> is null and |prefetchRecord| is not null:
1. Set <var ignore>navigationParams</var> to the result of [=creating navigation params from a prefetch record=] given <var ignore>navigable</var>, <var ignore>entry</var>'s [=session history entry/document state=], <var ignore>navigationId</var>, <var ignore>navTimingType</var>, <var ignore>request</var>, |prefetchRecord|, <var ignore>targetSnapshotParams</var>, and <var ignore>sourceSnapshotParams</var>.
1. [=Copy prefetch cookies=] given |prefetchRecord|'s [=prefetch record/isolated partition key=] and <var ignore>navigationParams</var>'s [=navigation params/reserved environment=].
Expand Down