146041 2015-06-16 19:12:19 -0700 [GTK] Reimplement webkit_web_context_clear_cache functionality. 2016-03-14 06:36:44 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED https://bugzilla.gnome.org/show_bug.cgi?id=763456 P1 Major --- 1 clopez webkit-unassigned berto cgarcia commit-queue gustavo gyuyoung.kim mcatanzaro mrobinson oldest_to_newest 1102444 0 clopez 2015-06-16 19:12:19 -0700 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). 1102497 1 cgarcia 2015-06-16 23:45:33 -0700 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. 1102509 2 255006 cgarcia 2015-06-17 01:28:36 -0700 Created attachment 255006 Patch This should fix the regression 1102510 3 commit-queue 2015-06-17 01:30:09 -0700 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 1102520 4 clopez 2015-06-17 03:04:15 -0700 I noticed we don't have a regression test for this. Should we add one? 1102521 5 cgarcia 2015-06-17 03:41:19 -0700 (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. 1102545 6 mcatanzaro 2015-06-17 08:13:08 -0700 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. 1105186 7 255006 andersca 2015-06-27 10:46:21 -0700 Comment on attachment 255006 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. 1127501 8 mcatanzaro 2015-09-20 09:00:33 -0700 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. 1127502 9 mcatanzaro 2015-09-20 09:07:00 -0700 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. 1138306 10 mcatanzaro 2015-11-01 06:37:26 -0800 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. 1143066 11 mcatanzaro 2015-11-18 16:20:08 -0800 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. 1143226 12 cgarcia 2015-11-19 00:20:42 -0800 we need to fix this in 2.10 branch in any case. 1152167 13 mcatanzaro 2016-01-03 15:41:36 -0800 (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? 1174431 14 cgarcia 2016-03-14 06:36:44 -0700 Committed r198124: <http://trac.webkit.org/changeset/198124> 255006 2015-06-17 01:28:36 -0700 2016-01-03 15:42:03 -0800 Patch wk2-clear-cache.diff text/plain 2528 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCAzM2FlYzVkLi45ZWZjZWM5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkg QEAKKzIwMTUtMDYtMTcgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIFtHVEtdIFJlaW1wbGVtZW50IHdlYmtpdF93ZWJfY29udGV4dF9jbGVhcl9j YWNoZSBmdW5jdGlvbmFsaXR5LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTQ2MDQxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgVXNlIHRoZSBXZWJQcm9jZXNzUG9vbCBsZWdhY3kgV2Vic2l0ZURhdGFTdG9y ZSB0byBjbGVhciBkaXNrIGFuZAorICAgICAgICBtZW1vcnkgY2FjaGVzLgorCisgICAgICAgICog VUlQcm9jZXNzL0FQSS9ndGsvV2ViS2l0V2ViQ29udGV4dC5jcHA6CisgICAgICAgICh3ZWJraXRf d2ViX2NvbnRleHRfY2xlYXJfY2FjaGUpOgorICAgICAgICAqIFdlYlByb2Nlc3MvV2ViUHJvY2Vz cy5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYlByb2Nlc3M6OmRlbGV0ZVdlYnNpdGVEYXRhKTog QWxzbyByZW1vdmUgdGhlIGRpc2sKKyAgICAgICAgY2FjaGUgZm9yIFNvdXAgaWYgV2Vic2l0ZURh dGFUeXBlRGlza0NhY2hlIGZsYWcgaXMgcHJlc2VudC4KKwogMjAxNS0wNi0xNiAgQ2FybG9zIEdh cmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0dUS10gSW5oaWJp dCBzY3JlZW4gc2F2ZXIgd2hlbiBwbGF5aW5nIGZ1bGwgc2NyZWVuIHZpZGVvCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRXZWJDb250ZXh0LmNwcCBi L1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYkNvbnRleHQuY3BwCmlu ZGV4IGRlNjg4MzYuLmU1NGRmNzcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vz cy9BUEkvZ3RrL1dlYktpdFdlYkNvbnRleHQuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJv Y2Vzcy9BUEkvZ3RrL1dlYktpdFdlYkNvbnRleHQuY3BwCkBAIC01MTUsOCArNTE1LDkgQEAgdm9p ZCB3ZWJraXRfd2ViX2NvbnRleHRfY2xlYXJfY2FjaGUoV2ViS2l0V2ViQ29udGV4dCogY29udGV4 dCkKIHsKICAgICBnX3JldHVybl9pZl9mYWlsKFdFQktJVF9JU19XRUJfQ09OVEVYVChjb250ZXh0 KSk7CiAKLSAgICAvLyBGSVhNRTogaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE0NjA0MQotICAgIC8vIGNvbnRleHQtPnByaXYtPmNvbnRleHQtPnN1cHBsZW1lbnQ8V2Vi UmVzb3VyY2VDYWNoZU1hbmFnZXJQcm94eT4oKS0+Y2xlYXJDYWNoZUZvckFsbE9yaWdpbnMoQWxs UmVzb3VyY2VDYWNoZXMpOworICAgIGF1dG8mIHdlYnNpdGVEYXRhU3RvcmUgPSBjb250ZXh0LT5w cml2LT5jb250ZXh0LT53ZWJzaXRlRGF0YVN0b3JlKCktPndlYnNpdGVEYXRhU3RvcmUoKTsKKyAg ICB3ZWJzaXRlRGF0YVN0b3JlLnJlbW92ZURhdGEoc3RhdGljX2Nhc3Q8V2Vic2l0ZURhdGFUeXBl cz4oV2Vic2l0ZURhdGFUeXBlczo6V2Vic2l0ZURhdGFUeXBlTWVtb3J5Q2FjaGUgfCBXZWJzaXRl RGF0YVR5cGVzOjpXZWJzaXRlRGF0YVR5cGVEaXNrQ2FjaGUpLAorICAgICAgICBzdGQ6OmNocm9u bzo6c3lzdGVtX2Nsb2NrOjp0aW1lX3BvaW50OjptaW4oKSwgW10geyB9KTsKIH0KIAogdHlwZWRl ZiBIYXNoTWFwPERvd25sb2FkUHJveHkqLCBHUmVmUHRyPFdlYktpdERvd25sb2FkPiA+IERvd25s b2Fkc01hcDsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUHJvY2Vz cy5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlByb2Nlc3MuY3BwCmluZGV4IDlj NTg5YjEuLmRkOTgxM2UgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2Vi UHJvY2Vzcy5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNw cApAQCAtMTE3Niw2ICsxMTc2LDEwIEBAIHZvaWQgV2ViUHJvY2Vzczo6ZGVsZXRlV2Vic2l0ZURh dGEoU2Vzc2lvbklEIHNlc3Npb25JRCwgdWludDY0X3Qgd2Vic2l0ZURhdGFUeXBlCiAKICAgICAg ICAgQ3Jvc3NPcmlnaW5QcmVmbGlnaHRSZXN1bHRDYWNoZTo6c2luZ2xldG9uKCkuZW1wdHkoKTsK ICAgICB9CisjaWYgVVNFKFNPVVApCisgICAgaWYgKHdlYnNpdGVEYXRhVHlwZXMgJiBXZWJzaXRl RGF0YVR5cGVEaXNrQ2FjaGUpCisgICAgICAgIHBsYXRmb3JtQ2xlYXJSZXNvdXJjZUNhY2hlcyhB bGxSZXNvdXJjZUNhY2hlcyk7CisjZW5kaWYKIAogICAgIHBhcmVudFByb2Nlc3NDb25uZWN0aW9u KCktPnNlbmQoTWVzc2FnZXM6OldlYlByb2Nlc3NQcm94eTo6RGlkRGVsZXRlV2Vic2l0ZURhdGEo Y2FsbGJhY2tJRCksIDApOwogfQo=