Skip to content

Conversation

@mark-buer
Copy link
Contributor

Expose cache status for requests in network monitor.

Testing: Net tests pass. No existing test infrastructure for devtools, adding them seems outside the scope of this PR.
Fixes: #40237

@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 1, 2025
Signed-off-by: Mark Buer <flux.syntax@proton.me>
Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

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

Great work! Did you verify that cached requests appear correctly in the network monitor?

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 2, 2025
@jdm jdm added the S-awaiting-answer Someone asked a question that requires an answer. label Nov 2, 2025
@mark-buer
Copy link
Contributor Author

Yes, the network requests appear with their correct cache_state in firefox devtools...
except when they don't - but that isn't a problem with this PR per se.

I noticed that cross domain requests show the state as not-from-the-cache when they in fact are from the cache.

I've tracked the issue down to https://github.com/servo/servo/blob/main/components/shared/net/response.rs#L286 which filters the response for cross domain requests. Since the cache_state field isn't exposed to the page or JS (it is used for devtools only) it seems that line (and the one a few lines further down) could be safely removed.

What do you suggest - remove those lines in this PR, in another PR, or something else?

@jschwe
Copy link
Member

jschwe commented Nov 2, 2025

Net tests pass. No existing test infrastructure for devtools, adding them seems outside the scope of this PR.

There is ./mach test-devtools, so that statement doesn't seem accurate (I'm not familiar with what exactly is tested there though).

@jdm
Copy link
Member

jdm commented Nov 2, 2025

Net tests pass. No existing test infrastructure for devtools, adding them seems outside the scope of this PR.

There is ./mach test-devtools, so that statement doesn't seem accurate (I'm not familiar with what exactly is tested there though).

There's no support yet for devtools tests that don't involve the debugger, do that's not something I want to gate this PR on.

@jdm
Copy link
Member

jdm commented Nov 2, 2025

What do you suggest - remove those lines in this PR, in another PR, or something else?

Unfortunately this is used in the fetch specification for some steps that we haven't implemented yet (https://fetch.spec.whatwg.org/#ref-for-concept-response-cache-state), so I think we'll need to do this in a separate PR with some more planning first.

@jdm jdm added this pull request to the merge queue Nov 2, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 2, 2025
Merged via the queue into servo:main with commit 2a18c6b Nov 2, 2025
35 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-awaiting-answer Someone asked a question that requires an answer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose cache status for requests in network monitor

4 participants