Nowadays GTK+ always uses the network cache, and never the soup cache. It would be nice to get rid of our soup cache support entirely, which implies switching the EFL port to use ENABLE_NETWORK_CACHE. I have a speculative patch; let's see if the bots like it. See also: bug #146041.
Created attachment 268157 [details] Patch
Created attachment 268160 [details] Patch
It's failing because EFL enables more warnings than we do, and uses -Werror. In particular, it's failing on -Werror=missing-field-initializers and -Werror=unused-result. For example: ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:109:18: error: missing initializer for member 'WebKit::NetworkCache::BlobStorage::Blob::data' [-Werror=missing-field-initializers] return { }; ^ (There are a couple of these.) This seems to be a GCC bug, since gcc(1) says: In C++ this option does not warn either about the empty { } initializer, for example: struct s { int f, g, h; }; s x = { }; I think this warning is bogus. Maybe we should just turn it off if it's going to complain about "issues" like this. But the -Wunused-result warnings seem legit: ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:101:51: error: ignoring return value of 'int link(const char*, const char*)', declared with attribute warn_unused_result [-Werror=unused-result] link(blobPath.data(), linkPath.data()); ^ We should probably check the return value of link() and print a warning with WTFLogAlways() if it fails.
Comment on attachment 268160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268160&action=review I would say this is EFL specific, since it's dead code for GTK+ port. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:48 > + , m_sourceStorageRecord() This shouldn't be needed, since it's initialized when declared. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h:92 > - Storage::Record m_sourceStorageRecord { }; > + Storage::Record m_sourceStorageRecord; Ah, you are removing this, I don't understand why, we are changing from constructor initializations to this. > Source/WebKit2/UIProcess/API/gtk/APIWebsiteDataStoreGtk.cpp:39 > +// FIXME: The other directories in this file are shared between all applications using WebKitGTK+. > +// Why is only this directory namespaced to a particular application? Because in this cache the directory is not a globla dir for different cached, but represents the cache itself. The soup cache doesn't support multiple applications using the same cache concurrently, and I'm not sure about the webkit one, I don't think it has been tested.
(In reply to comment #4) > Ah, you are removing this, I don't understand why, we are changing from > constructor initializations to this. Because it triggers -Werror=missing-field-initializers, failing the build. But there are many other places that trigger this, so we should disable that warning instead of trying to avoid it everywhere.
Created attachment 287955 [details] Patch
Created attachment 287957 [details] Patch
Created attachment 287958 [details] Patch
Created attachment 287959 [details] Patch
Created attachment 287960 [details] Patch
Created attachment 287961 [details] Patch
I need a WebKit2 owner to approve the change in NetworkCacheBlobStorage.cpp.
Created attachment 287963 [details] Patch
Created attachment 287964 [details] Patch
Created attachment 287965 [details] Patch
Comment on attachment 287965 [details] Patch Attachment 287965 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2012908 New failing tests: http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html
Created attachment 287968 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287965 [details] Patch Seems the Mac EWS is going red and uploading test results even after determining the failure is not caused by the patch. I don't think it used to do that....
Comment on attachment 287965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287965&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:101 > + auto ret = link(blobPath.data(), linkPath.data()); > + if (ret == -1) There's no need for a named variable here. If we have one, it shouldn't be named ret.
(In reply to comment #19) > There's no need for a named variable here. If we have one, it shouldn't be > named ret. Good point.
Created attachment 288024 [details] Patch
Comment on attachment 288024 [details] Patch OK, let's do it. Gyuyoung knows about it, so it can be rolled out if it causes problems for EFL, and it's the start of a new release cycle for them anyway.
Comment on attachment 288024 [details] Patch Clearing flags on attachment: 288024 Committed r205556: <http://trac.webkit.org/changeset/205556>
All reviewed patches have been landed. Closing bug.