[Webkit-unassigned] [Bug 147783] Expand network cache tests to cover memory cache behavior

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147783

--- Comment #2 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 258501
  --> https://bugs.webkit.org/attachment.cgi?id=258501
patch

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

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:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=cache%2F

> Source/WebCore/loader/cache/CachedRawResource.cpp:144
> +        if (validationInProgress())
> +            response.setSource(ResourceResponse::Source::MemoryCacheAfterValidation);

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?

> Source/WebCore/loader/cache/CachedRawResource.cpp:184
> -        c->responseReceived(this, m_response);
> +        c->responseReceived(this, response);

This change is not mentioned in ChangeLog, what does it do?

> Source/WebCore/platform/network/ResourceResponseBase.h:110
> +    // This is primarily for testing support. It is not necessarily accurate in all scenarios.

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150807/af986648/attachment-0001.html>