<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Expand network cache tests to cover memory cache behavior"
href="https://bugs.webkit.org/show_bug.cgi?id=147783#c2">Comment # 2</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Expand network cache tests to cover memory cache behavior"
href="https://bugs.webkit.org/show_bug.cgi?id=147783">bug 147783</a>
from <span class="vcard"><a class="email" href="mailto:ap@webkit.org" title="Alexey Proskuryakov <ap@webkit.org>"> <span class="fn">Alexey Proskuryakov</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=258501&action=diff" name="attach_258501" title="patch">attachment 258501</a> <a href="attachment.cgi?id=258501&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=258501&action=review">https://bugs.webkit.org/attachment.cgi?id=258501&action=review</a>
Very nicely expanded testing! I have one general question, and some nits about the code.
What makes these tests reliable and repeatable? Could these resources get purged randomly? I just checked, and we have quite a few existing cache tests that are flaky:
<a href="https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=cache%2F">https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=cache%2F</a>
<span class="quote">> Source/WebCore/loader/cache/CachedRawResource.cpp:144
> + if (validationInProgress())
> + response.setSource(ResourceResponse::Source::MemoryCacheAfterValidation);</span >
This code looks quite strange. Do we really call client->responseReceived before we know that revalidation succeeded? Or is validationInProgress a misnomer, and validation is actually complete and successful when this code runs?
<span class="quote">> Source/WebCore/loader/cache/CachedRawResource.cpp:184
> - c->responseReceived(this, m_response);
> + c->responseReceived(this, response);</span >
This change is not mentioned in ChangeLog, what does it do?
<span class="quote">> Source/WebCore/platform/network/ResourceResponseBase.h:110
> + // This is primarily for testing support. It is not necessarily accurate in all scenarios.</span >
Generally, we prefer adding such information to function name, to make sure that it's not overlooked (e.g. "deprecatedFoo", or "barForTesting"). Also, another engineer wouldn't know how much to trust the source even for testing.
Adding a comment is an improvement over having an unreliable function with no comment though, so I'm not asking for this to be necessarily improved further now.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>