Bug 152676 – [EFL] Switch to ENABLE_NETWORK_CACHE

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 268157 [details]
Patch

[Michael Catanzaro]

Created attachment 268160 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Created attachment 287955 [details]
Patch

[Michael Catanzaro]

Created attachment 287957 [details]
Patch

[Michael Catanzaro]

Created attachment 287958 [details]
Patch

[Michael Catanzaro]

Created attachment 287959 [details]
Patch

[Michael Catanzaro]

Created attachment 287960 [details]
Patch

[Michael Catanzaro]

Created attachment 287961 [details]
Patch

[Michael Catanzaro]

I need a WebKit2 owner to approve the change in NetworkCacheBlobStorage.cpp.

[Michael Catanzaro]

Created attachment 287963 [details]
Patch

[Michael Catanzaro]

Created attachment 287964 [details]
Patch

[Michael Catanzaro]

Created attachment 287965 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Michael Catanzaro]

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....

[Alex Christensen]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Created attachment 288024 [details]
Patch

[Michael Catanzaro]

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.

[WebKit Commit Bot]

Comment on attachment 288024 [details]
Patch

Clearing flags on attachment: 288024

Committed r205556: <http://trac.webkit.org/changeset/205556>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.