Implement Logical HTTP Cache Invalidation for instant Clear-Site-Data response. [chromium/src : main]
Deepak Ravichandran (Gerrit)
Deepak Ravichandran added 2 comments![]( "Open in Gerrit")
Christian - I got around to fixing this CL finally after ~8 months! This CL was heavily Gemini assisted - but I did spend quite a bit of time testing and verifying code and adjusting them.
A base::DoNothing() callback is used, so the browser doesn't wait for the cache to be
cleared. This makes the UI more responsive during browsing data removal.
The original synchronous behavior is preserved when the flag is disabled.Deepak RavichandranI don't think we can make http cache deletion async. If we report a Clear-Site-Data or ClearBrowsingData deletion as done before the deletion actually happened, a website could already start loading a new page where it expects not to get any responses from the cache but if the timing goes wrong, it would still get responses from the cache that is supposed to be deleted. This would be quite confusing?
Websites can already do Clear-Site-Data asynchronous by sending it with a subresource request instead of with the main pageload, so I don't think this change would be good?
I tried the solution again with Gemini 3.0 this time and with a few prompt engineering and testing - I found a async solution with consistency guarantee. I have flag guarded this change and tested it extenstively.
Related details
- Christian Dullweber
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Ravichandran (Gerrit)
Deepak Ravichandran added 1 comment![]( "Open in Gerrit")
This CL attempts to speed up ClearHttpCache deletion when called fromDeepak RavichandranPlease also format with a single line for the title
Done
Related details
- Christian Dullweber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Christian Dullweber added 6 comments![]( "Open in Gerrit")
This looks great, I'm excited to see the experiment results!
bool IsInvalidated(disk_cache::Entry* entry);Should we make this a method of InvalidationFilter? Then we could also more easily test the filter logic with various entries
if (entry->opened() && IsInvalidated(entry->GetEntry())) {Do we also need to check if non-opened entries are invalidated and return nullptr? (I don't really know what "opened" means here. If this is correct, then maybe add a comment why opened entries can be skipped?)
if (opened && IsInvalidated(disk_entry)) {same here
invalidation_filters_.push_back(std::move(filter));Should we enforce that end_time is at most base::Time::Now() so you can't accidentially delete future cache entries, e.g. with base::Time::Max() as end_time?
Related details
- Deepak Ravichandran
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Ravichandran (Gerrit)
Deepak Ravichandran added 5 comments![]( "Open in Gerrit")
Should we make this a method of InvalidationFilter? Then we could also more easily test the filter logic with various entries
I created a function called Matches() to address your comment for testability harness. We will still keep this function because it handles the early exit which will be true for 99,9999+% of the case.
Deepak RavichandranPlease autoformat the CL
Done
if (entry->opened() && IsInvalidated(entry->GetEntry())) {Do we also need to check if non-opened entries are invalidated and return nullptr? (I don't really know what "opened" means here. If this is correct, then maybe add a comment why opened entries can be skipped?)
added a comment to that effect - because unopened/new entries are guaranteed to be fresh and we should not invalidate them,.
if (opened && IsInvalidated(disk_entry)) {Deepak Ravichandransame here
as reason as above - added a comment to that effect.
invalidation_filters_.push_back(std::move(filter));Should we enforce that end_time is at most base::Time::Now() so you can't accidentially delete future cache entries, e.g. with base::Time::Max() as end_time?
great idea - added a line below to reflect this comment:
invalidation_filter.end_time = std::min(end_time, base::Time::Now());
Related details
- Christian Dullweber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Christian Dullweber voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christian Dullweber
| Code-Review | +1 |
2 comments
std::string url_str = GetResourceURLFromHttpCacheKey(entry->GetKey());Could you parse this as GURL already so we don't need to parse it for every filter?
Related details
- Deepak Ravichandran
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Ravichandran (Gerrit)
Deepak Ravichandran added 1 comment![]( "Open in Gerrit")
std::string url_str = GetResourceURLFromHttpCacheKey(entry->GetKey());Could you parse this as GURL already so we don't need to parse it for every filter?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Schinazi (Gerrit)
David Schinazi voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Schinazi
| Code-Review | +1 |
3 comments
Nice, this looks like a solid improvement. LGTM modulo a couple extra safety checks
if (entry->opened() && IsInvalidated(entry->GetEntry())) {Can we also protect this check behind the feature flag to be extra safe?
if (opened && IsInvalidated(disk_entry)) {Same
Related details
- Deepak Ravichandran
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |