[webkit-reviews] review granted: [Bug 189760] Resource Load Statistics: Relax cap on partitioned cache max age if reloads result in the same bytes : [Attachment 353896] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 189760: Resource Load Statistics: Relax cap on partitioned cache max age if
reloads result in the same bytes
https://bugs.webkit.org/show_bug.cgi?id=189760

Attachment 353896: Patch

https://bugs.webkit.org/attachment.cgi?id=353896&action=review




--- Comment #24 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 353896
  --> https://bugs.webkit.org/attachment.cgi?id=353896
Patch

LGTM with comments (I am not sure we should introduce replaceRedirect in
particular).

We should probably bump the network cache revision number.
Since it was done one week ago, maybe this is not needed.
If we do not bump it, please add a comment in the ChangeLog for why we are not
doing it.

As of the check of the conditional request, the server php script could check
whether any of the request contains some If-Modified-Since/If-None-Match..
header. In such a case, it could generate a response that would make the test
fail.

View in context: https://bugs.webkit.org/attachment.cgi?id=353896&action=review

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:601
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)

Maybe we could put this new code in a new method called
validateCacheEntryForMaxAgeCapValidation.
It would return a std::optional<Seconds>
If RESOURCE_LOAD_STATISTICS is not defined, it would return nullopt.
If m_cacheEntryForMaxAgeCapValidation is null, it would return
networkStorageSession->maxAgeCacheCap(request) .
In case of replacing the entry, it would not use replaceRedirect, but would
just call remove().
The readding of the redirect would happen later on, in the regular code path.

That would remove the need for replaceRedirect, which I am not sure is correct,
since the new redirectResponse might not be cacheable.

Some comments below if you do not like the idea of the utility function/removal
of replaceRedirect.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:609
> +	       m_cacheEntryForMaxAgeCapValidation =
m_cache->replaceRedirect(request, *m_cacheEntryForMaxAgeCapValidation,
redirectResponse, redirectRequest, std::nullopt);

No need to set m_cacheEntryForMaxAgeCapValidation since we nullify it shortly
after without using it.
So maybe replaceRedirect should return void.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:611
> +	       m_cache->remove(m_cacheEntryForMaxAgeCapValidation->key());

If we remove the previous entry from the cache, do we still want to store the
new one?
The patch does so which might be safer.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:617
> +    if (!existingCacheEntryIsValid && redirectResponse.source() ==
ResourceResponse::Source::Network && canUseCachedRedirect(request)) {

I would rename existingCacheEntryIsValid to shouldTryCachingEntry.
That way we would write:
if (shouldTryCachingEntry && ...)
That seems to read better to me.

> Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:82
> +#endif

I would reduce the code difference by doing:
std::optional<Seconds> maxAgeCap;
#if ENABLE(RESOURCE_LOAD_STATISTICS)
if (auto networkStorageSession =
WebCore::NetworkStorageSession::storageSession(PAL::SessionID::defaultSessionID
()))
    maxAgeCap = networkStorageSession->maxAgeCacheCap(request);
 #endif
m_cacheEntry = m_cache->storeRedirect(request, redirectResponse,
redirectRequest, maxAgeCap);