Summary: | [EFL] Switch to ENABLE_NETWORK_CACHE | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, gyuyoung.kim, koivisto, lucas.de.marchi, mcatanzaro | ||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||
Version: | Other | ||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2016-01-03 16:44:27 PST
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. |