Bug 177424

Summary: Remove ENABLE_NETWORK_CACHE
Product: WebKit Reporter: Yoshiaki Jitsukawa <yoshiaki.jitsukawa>
Component: WebKit2Assignee: Don Olmstead <don.olmstead>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, cgarcia, commit-queue, don.olmstead, ggaren, koivisto, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Yoshiaki Jitsukawa 2017-09-25 00:16:36 PDT
https://trac.webkit.org/changeset/221942/webkit introduced NetworkCache::Key and other things in NetworkCache namespace
into WebKit/NetworkProcess/cache/CacheStorageEngineCache, but NetworkCache::* is not defined unless ENABLE(NETWORK_CACHE) is true
and this causes WebKit to fail to build.

The definition in WebKit/config.h is 
#ifndef ENABLE_NETWORK_CACHE
#if PLATFORM(COCOA) || USE(SOUP)
#define ENABLE_NETWORK_CACHE 1
#else
#define ENABLE_NETWORK_CACHE 0
#endif
#endif

and it seems like all webkit(2) port enable it today.

Should we support the case ENABLE_NETWORK_CACHE==0 or should all webkit(2) port (including upcoming windows one) enable network cache?
Comment 1 Antti Koivisto 2017-09-25 09:52:29 PDT
We should just remove ENABLE_NETWORK_CACHE define and always compile in the code. Cache can still be disabled dynamically.
Comment 2 Yoshiaki Jitsukawa 2017-09-25 14:54:53 PDT
(In reply to Antti Koivisto from comment #1)
> We should just remove ENABLE_NETWORK_CACHE define and always compile in the
> code. Cache can still be disabled dynamically.

I understand. Thank you for the clarification :)
Comment 3 Don Olmstead 2017-10-10 14:22:00 PDT
Created attachment 323349 [details]
Patch
Comment 4 Antti Koivisto 2017-10-10 15:02:13 PDT
Comment on attachment 323349 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:320
>      bool shouldSendDidReceiveResponse = true;

This line can be removed

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:334
>      shouldSendDidReceiveResponse = !m_cacheEntryForValidation;

... and this turned into

bool shouldSendDidReceiveResponse = !m_cacheEntryForValidation

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-362
>      // For main resources, the web process is responsible for sending back a NetworkResourceLoader::ContinueDidReceiveResponse message.
>      bool shouldContinueDidReceiveResponse = !shouldWaitContinueDidReceiveResponse;
> -#if ENABLE(NETWORK_CACHE)
>      shouldContinueDidReceiveResponse = shouldContinueDidReceiveResponse || m_cacheEntryForValidation;
> -#endif

This can be reduced to

bool shouldContinueDidReceiveResponse = !shouldWaitContinueDidReceiveResponse || m_cacheEntryForValidation;
Comment 5 Don Olmstead 2017-10-10 17:36:32 PDT
Created attachment 323365 [details]
Patch

Fixing review comments
Comment 6 Antti Koivisto 2017-10-11 00:01:59 PDT
Comment on attachment 323365 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2017-10-11 08:53:25 PDT
Comment on attachment 323365 [details]
Patch

Clearing flags on attachment: 323365

Committed r223179: <https://trac.webkit.org/changeset/223179>
Comment 8 WebKit Commit Bot 2017-10-11 08:53:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-10-11 08:54:09 PDT
<rdar://problem/34934064>