I have a mac with safari 5.0.5 installed Scenario: 1. build recent webkit 2. run-safari 3. open web inspector's network panel 4. set Disable Caches in Develop menu 5. go to webkit.org 6. refresh 7. observe 304 statuses of css stylesheets I can also see that the first refresh after setting Disable Caches option shows 200 OK, which probably means that Disable Caches clears cache instead of disabling it.
<rdar://problem/9770590>
OK, here are the two bugs I found: 1) Setting the cache model to CacheModelDocumentViewer does not turn off MemoryCache. It turns off caching of dead resources, but it sets a non-zero value for the total size of the memory cache, which means that live resources are indeed cached. This policy is implemented in the calculateCacheSizes function and the +[WebView _setCacheModel:] method. I tried setting cacheTotalCapacity to 0 and indeed this fixed the original reported symptom. It might be bad, however, for clients using CacheModelDocumentViewer for something other than Safari’s Disable Caches. Performance might be suprisingly bad when the memory cache is entirely turned off. Presumably this regression dates back to when we Antti created the MemoryCache. 2) Setting the cache model to CacheModelDocumentViewer does not turn off the NSURLCache disk cache when NETWORK_CACHE is disabled. This policy is in the NetworkProcess::platformSetCacheModel function and the +[WebView _setCacheModel:] method. Neither will ever shrink disk capacity, only make it larger. Comment says, “Don't shrink a big disk cache, since that would cause churn.” I don’t understand the thinking behind this. This has always been the case <http://trac.webkit.org/changeset/25430>. Safari attempts to work around this by calling -[NSURLCache setDiskCapacity:] directly but this has not had any effect on WebKit networking since we moved it from the UI process to the web process and then later to the network process.
Created attachment 274376 [details] Patch
Here’s my patch: I didn't add a test yet. This won’t fix the problem with Safari until a new version of Safari comes out that uses the new SPI. If we decide this is important, we could put an app-specific hack in to make this do the right thing in old versions of Safari. I'm not sure this covers all the code paths. There are comments that seem to indicate some loads bypass FrameLoader::addExtraFieldsToRequest.
This doesn't disable the page cache's caching of back/forward history. Maybe it should. Any other caches it should disable?
Disabling page cache seems desirable too.
Comment on attachment 274376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274376&action=review > Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:1518 > +void WKPreferencesSetAllCachesDisabled(WKPreferencesRef preferencesRef, bool flag) > +{ > + toImpl(preferencesRef)->setAllCachesDisabled(flag); > +} > + > +bool WKPreferencesGetAllCachesDisabled(WKPreferencesRef preferencesRef) > +{ > + return toImpl(preferencesRef)->allCachesDisabled(); > +} If I understand correctly we don't really want to add more C API. We should add ObjC SPI to WKPreferencesPrivate. I'm not sure we need these C functions at all.
Comment on attachment 274376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274376&action=review >> Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:1518 >> +} > > If I understand correctly we don't really want to add more C API. We should add ObjC SPI to WKPreferencesPrivate. I'm not sure we need these C functions at all. I asked Sam today what to do, and he told me to do this. Safari is currently using the C SPI, and I don’t think it can mix the C SPI with the Objective-C API/SPI. Same said it would be trivial to add the Objective-C API or SPI for this later and so I should not worry about it now.
Comment on attachment 274376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274376&action=review >>> Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:1518 >>> +} >> >> If I understand correctly we don't really want to add more C API. We should add ObjC SPI to WKPreferencesPrivate. I'm not sure we need these C functions at all. > > I asked Sam today what to do, and he told me to do this. Safari is currently using the C SPI, and I don’t think it can mix the C SPI with the Objective-C API/SPI. Same said it would be trivial to add the Objective-C API or SPI for this later and so I should not worry about it now. Sam said, not "Same said".
Comment on attachment 274376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274376&action=review > Source/WebCore/page/Settings.in:255 > +allCachesDisabled initial=false We of course have many caches that this setting is not meant to disable. I suppose it is reasonable to assume that "all caches" here refers to resource caches specifically. Still I wonder if there would be a better name, "resourceCachesDisabled" or something.
> I asked Sam today what to do, and he told me to do this. Safari is currently > using the C SPI, and I don’t think it can mix the C SPI with the Objective-C > API/SPI. Same said it would be trivial to add the Objective-C API or SPI for > this later and so I should not worry about it now. Ok. I assumed we can mix SPIs.
Comment on attachment 274376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274376&action=review >> Source/WebCore/page/Settings.in:255 >> +allCachesDisabled initial=false > > We of course have many caches that this setting is not meant to disable. I suppose it is reasonable to assume that "all caches" here refers to resource caches specifically. Still I wonder if there would be a better name, "resourceCachesDisabled" or something. I’ll make a new version of this patch that covers the page cache too. The page cache caches resources, but it also caches more than that. I guess the name should be resourceCachingDisabled.
Created attachment 274388 [details] Patch
Committed r198393: <http://trac.webkit.org/changeset/198393>
Anders asked me to move this from WKPreferences to WKPage.
Created attachment 274501 [details] Patch
Here’s a new patch that moves this off of preferences.
Created attachment 274502 [details] Patch
Comment on attachment 274502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274502&action=review > Source/WebCore/page/Page.h:518 > + bool isResourceCachingDisabled() const { return m_resourceCachingDisabled; } > + void setResourceCachingDisabled(bool disabled) { m_resourceCachingDisabled = disabled; } Why do we prefer Page here? If this is not appropriate part of Settings then what is?
Anders, could you answer Antti’s question?
Comment on attachment 274502 [details] Patch In any case no need to hold this.
Comment on attachment 274502 [details] Patch Clearing flags on attachment: 274502 Committed r198476: <http://trac.webkit.org/changeset/198476>
All reviewed patches have been landed. Closing bug.