Bug 126577

Summary: [SOUP] [WK2] - Disable MemoryCache when the DOCUMENT_VIEWER cache model is set
Product: WebKit Reporter: Tomas Popela <tpopela>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, gns, gyuyoung.kim, ryuan.choi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
none
Proposed patch v2
none
Proposed patch v3 none

Description Tomas Popela 2014-01-07 06:08:40 PST
As in http://trac.webkit.org/changeset/152483, but this time for WK2. When WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER is set we need to disable the MemoryCache completely (based on cacheMinDeadCapacity and cacheMaxDeadCapacity values) to prevent caching the resources.
Comment 1 Tomas Popela 2014-01-07 06:18:22 PST
Created attachment 220518 [details]
Proposed patch
Comment 2 Carlos Garcia Campos 2014-01-07 06:25:57 PST
In r152483 we also check that cacheTotalCapacity is 0, don't we need that in wk2 as well?
Comment 3 Carlos Garcia Campos 2014-01-07 06:27:43 PST
Comment on attachment 220518 [details]
Proposed patch

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

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:107
> +    bool disableCache = !cacheMinDeadCapacity && !cacheMaxDeadCapacity;
> +    WebCore::memoryCache()->setDisabled(disableCache);

This is not specific to GTK, it also affects EFL, so I would confirm with EFL guys that this is the desired behaviour for them too, otherwise we should need to do this inside a platform ifdef.
Comment 4 Tomas Popela 2014-01-07 06:33:17 PST
(In reply to comment #2)
> In r152483 we also check that cacheTotalCapacity is 0, don't we need that in wk2 as well?

Actually the calculateCacheSizes will return != 0 for cacheTotalCapacity even for WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER. Another solution will be just asking straight for cache model.
Comment 5 Carlos Garcia Campos 2014-01-12 01:23:57 PST
(In reply to comment #4)
> (In reply to comment #2)
> > In r152483 we also check that cacheTotalCapacity is 0, don't we need that in wk2 as well?
> 
> Actually the calculateCacheSizes will return != 0 for cacheTotalCapacity even for WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER. Another solution will be just asking straight for cache model.

Yes, in the end what we want is to make sure that memory cache is disabled for DocumentViewer cache model, because that's what our public API documentation says. So, I would do something like this:

#if PLATFORM(GTK)
WebCore::memoryCache()->setDisabled(cacheModel == CacheModelDocumentViewer);
#endif

It's also more obvious and it's the only model that returns cacheMinDeadCapacity = cacheMaxDeadCapacity = 0 after all.
Comment 6 Tomas Popela 2014-01-14 06:57:10 PST
Created attachment 221158 [details]
Proposed patch v2
Comment 7 Sergio Villar Senin 2014-01-14 23:15:04 PST
Comment on attachment 221158 [details]
Proposed patch v2

Same comment, have you checked with the EFL guys if they want this behavior? I'm almost sure they want, so we wouldn't need the ifdef
Comment 8 Ryuan Choi 2014-01-14 23:39:50 PST
(In reply to comment #7)
> (From update of attachment 221158 [details])
> Same comment, have you checked with the EFL guys if they want this behavior? I'm almost sure they want, so we wouldn't need the ifdef

I also think that macro is not necessary.
Comment 9 Tomas Popela 2014-01-15 00:45:30 PST
Created attachment 221237 [details]
Proposed patch v3

Disable MemoryCache even for EFL (remove the GTK ifdef)
Comment 10 Carlos Garcia Campos 2014-01-15 00:52:14 PST
Comment on attachment 221237 [details]
Proposed patch v3

Thanks
Comment 11 WebKit Commit Bot 2014-01-15 01:41:19 PST
Comment on attachment 221237 [details]
Proposed patch v3

Clearing flags on attachment: 221237

Committed r162059: <http://trac.webkit.org/changeset/162059>
Comment 12 WebKit Commit Bot 2014-01-15 01:41:22 PST
All reviewed patches have been landed.  Closing bug.