Bug 146041 – [GTK] Reimplement webkit_web_context_clear_cache functionality.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

r185615 <http://trac.webkit.org/changeset/185615> removed WebResourceCacheManager, which was used by the GTK port in webkit_web_context_clear_cache() at Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp

The functionality should be reimplemented (in terms of WebsiteDataStore).

[Carlos Garcia Campos]

Maybe it's time to deprecate this method. It's very confusing, and untested. It says clear cache, as if there were only one cache. Epiphany uses it to clear the disk cache, but as a side effect, the memory cache is cleared as well. We could probably expose the WebsiteDataStore in our API. It would allow to manage all caches and storage (memory cache, disk cache, local storage and indexedDB databases, etc.), and with some more control, like clearing only a cache for a particular security origin.

[Carlos Garcia Campos]

Created attachment 255006 [details]
Patch

This should fix the regression

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Alberto Lopez Perez]

I noticed we don't have a regression test for this. Should we add one?

[Carlos Garcia Campos]

(In reply to comment #4)
> I noticed we don't have a regression test for this. Should we add one?

I don't think it's worth it, we could check that the disk cache is cleared, but not that the memory cache is, at least I don't know how to do that. I think it's better to deprecate this, and expose WebsiteDataStore with proper unit tests.

[Michael Catanzaro]

We should do that anyway, so that we can fix the clear personal data dialog in Epiphany, that's currently only able to clear a subset of your personal data.

[Anders Carlsson]

Comment on attachment 255006 [details]
Patch

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

> Source/WebKit2/WebProcess/WebProcess.cpp:1182
> +#endif

Disk cache isn't the same as all resource caches.

[Michael Catanzaro]

I forgot that this API is broken.

Anders, the problem is that WebProcess::deleteWebsiteData thinks the web process is only responsible for clearing the memory cache, which is true on Macs, but on the Soup port it is also needs to clear the soup disk cache if the network process is enabled. (Our thinking is to eventually mandate running the network process and get rid of the soup cache entirely, but that hasn't happened yet.) platformClearResourceCaches will only ever clear the disk cache on the soup port, and the code is in an ifdef, so platformClearResourceCaches(AllResourceCaches) is the right thing to do; the only other flag would be ResourceCaches::InMemoryResourceCachesOnly, which would cause the function to do nothing.

Alternatives:

* We could add a new function with a more specific name, but then we are just going to need another #ifdef in the header file.

* We could add a new platformClearDiskCache function, which would be a no-op on all other ports. This is my preference, since it avoids ifdefs in the cross-platform files.

* We could copypaste the contents of platformClearResourceCaches into the ifdef, but that rather defeats the purpose of having the platform functions.

* We could clear the disk cache in webkit_web_context_clear_cache, but that would leave WebProcess::deleteWebsiteData broken.

Regardless, we should change both WebProcess::deleteWebsiteData and WebProcess::deleteWebsiteDataForOrigins. Carlos, you fixed WebProcess::deleteWebsiteData, but it seems WebProcess::deleteWebsiteDataForOrigins is what the WebsiteDataManager uses.

[Michael Catanzaro]

I guess it would be odd for WebProcess::deleteWebsiteDataForOrigins to delete the entire disk cache, but it must, since there is no tracking of which resources correspond to which origin. This is fine since applications should be prepared for the entire cache to disappear at any time.

[Michael Catanzaro]

Carlos, got time to deal with this? I think your original patch, which currently has r-, is the best approach, except:

(In reply to comment #8)
> Regardless, we should change both WebProcess::deleteWebsiteData and
> WebProcess::deleteWebsiteDataForOrigins. Carlos, you fixed
> WebProcess::deleteWebsiteData, but it seems
> WebProcess::deleteWebsiteDataForOrigins is what the WebsiteDataManager uses.

[Michael Catanzaro]

We agreed at the Contributors Meeting to make network process mandatory; that will unblock this, since we'll no longer need to worry about the soup cache.

[Carlos Garcia Campos]

we need to fix this in 2.10 branch in any case.

[Michael Catanzaro]

(In reply to comment #12)
> we need to fix this in 2.10 branch in any case.

My vote is to not bother. The GTK+ port does not use the soup cache anymore; only EFL does. I think EFL should switch to use the network cache (which will hopefully be painless) so that USE(SOUP) implies ENABLE(NETWORK_CACHE); then we can remove all our soup cache code. You already taught the network process to clear the soup cache on startup when the network cache is enabled, so we won't have to worry about it anymore.

In summary:

 * This patch's changes to WebKitWebContext.cpp are good as-is, r=me.
 * I do not particularly want the change to WebProcess.cpp in trunk.
    * That change is fine for the 2.10 branch, but you must be sure to change WebProcess::deleteWebsiteDataForOrigins as well; otherwise, the change is broken!
 * I am changing this to r+ on the basis that I have asked you to remove from this patch the change that Anders objected to when he gave this r-.

Sound good?

[Carlos Garcia Campos]

Committed r198124: <http://trac.webkit.org/changeset/198124>