Implement document preloaded resources#39794
Merged
TimvdLippe merged 1 commit intoservo:mainfrom Oct 20, 2025
Merged
Conversation
TimvdLippe
force-pushed
the
implement-document-preloaded-resources
branch
2 times, most recently
from
dc571ee to
219f724
Compare
|
🔨 Triggering try run (#18463506639) for Linux (WPT) |
|
Test results for linux-wpt from try job (#18463506639): Flaky unexpected result (34)
Stable unexpected results that are known to be intermittent (26)
|
|
✨ Try run (#18463506639) succeeded. |
TimvdLippe
force-pushed
the
implement-document-preloaded-resources
branch
from
219f724 to
0a59149
Compare
servo-highfive
added
the
S-awaiting-review
Closed
TimvdLippe
commented
Oct 13, 2025
Comment on lines
+435
to
+438
| // Step 1. Wait until fetchParams’s preloaded response candidate is not "pending". | ||
| // TODO | ||
| // Step 2. Assert: fetchParams’s preloaded response candidate is a response. | ||
| // TODO |
Contributor
Author
There was a problem hiding this comment.
The reason WPT isn't passing more tests is because of this here. I don't know how we can change the structs to be able to handle this case. I think it needs a separate PreloadEntry with enum on how to store this, but I am not sure how we can pass in the fetch_params into that.
Any guidance on how I can implement this in Rust would be great. I did see .await in other parts of the codebase, so I assume that Futures are the appropriate language construct to implement this, are they not?
Member
There was a problem hiding this comment.
Yeah, futures are probably the way to go here. One possibility would be to use a channel for preload responses, so this code could just call await on the receiver, and the code that produces the preload response would be responsible for sending the complete response object over the corresponding sender.
Contributor
Author
There was a problem hiding this comment.
Cool. I think I know already how to tackle that, so will implement that in the next PR
jdm
reviewed
Oct 17, 2025
Member
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
This is a really useful step forward!
Comment on lines
+435
to
+438
| // Step 1. Wait until fetchParams’s preloaded response candidate is not "pending". | ||
| // TODO | ||
| // Step 2. Assert: fetchParams’s preloaded response candidate is a response. | ||
| // TODO |
Member
There was a problem hiding this comment.
Yeah, futures are probably the way to go here. One possibility would be to use a channel for preload responses, so this code could just call await on the receiver, and the code that produces the preload response would be responsible for sending the complete response object over the corresponding sender.
| global: self.global.clone(), | ||
| })); | ||
| let global = self.global.root(); | ||
| let _realm = enter_realm(&*global); |
Contributor
Author
There was a problem hiding this comment.
Ah this should have been part of 9042b68#diff-7732144f217b054c2148f8efd84259dfba8526ca07db0019691d994445c5195e Previously, we would call GlobalScope::current() which would require us to enter the realm here. Now, we pass in the global as an argument, hence this isn't required anymore.
| pub struct RequestClient { | ||
| /// <https://html.spec.whatwg.org/multipage/#map-of-preloaded-resources> | ||
| #[conditional_malloc_size_of] | ||
| #[serde(skip)] // TODO: Figure out what we need to do here to serialize this map |
Contributor
Author
There was a problem hiding this comment.
Yeah. It does support serialization, but it doesn't guarantee ordering. Ordering is important per the spec, so if we were to serialize as-is, then the ordering would be broken and we would violate the spec.
That said, I don't think in the current setup we need serialization for these requests, as we don't transfer them between threads. But I don't know when we will and how to support that, given that the default serialization isn't feasible.
|
|
||
| /// <https://html.spec.whatwg.org/multipage/#traversable-navigable> | ||
| #[derive(Clone, Copy, Default, MallocSizeOf, PartialEq)] | ||
| pub struct TraversableNavigable { |
Member
There was a problem hiding this comment.
I suspect we will want this to live somewhere else and only include a few relevant fields in the net-related data structures (particularly after the TODOs), but I'm fine with dealing with that later.
jdm
approved these changes
Oct 17, 2025
Member
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
I'm willing to merge this with only minor changes to avoid this turning into a sprawling PR of doom.
servo-highfive
added
S-needs-rebase
This aligns the request object more with the specification, since the spec now has a `traversable_for_user_prompts` and a separate field for the client. Before, they were present in the same enum. In doing so, new structs are added that are all required in the new spec. Doing so, we can add support for preloaded resources in this client, which are only populated when we have an applicable Global. Since the spec moved things around a bit, it now has a dedicated method to populate the client from the request. Part of servo#35035 Signed-off-by: Tim van der Lippe <[email protected]>
TimvdLippe
force-pushed
the
implement-document-preloaded-resources
branch
from
0a59149 to
412d562
Compare
servo-highfive
added
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This aligns the request object more with the specification,
since the spec now has a
traversable_for_user_promptsanda separate field for the client. Before, they were present
in the same enum.
In doing so, new structs are added that are all required in
the new spec. With this we can add support for preloaded
resources in this client, which are only populated when
we have an applicable Global.
Since the spec moved things around a bit, it now has a
dedicated method to populate the client from the request.
Unfortunately none of the WPT preload tests pass, since
the requests are received out-of-order. The specification
requires us to wait for that to settle, but I haven't figured
out yet how to do that. Given that this PR is already quite
large, opted to do that in a follow-up.
Part of #35035