Bug 152676 - [EFL] Switch to ENABLE_NETWORK_CACHE
Summary: [EFL] Switch to ENABLE_NETWORK_CACHE
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-03 16:44 PST by Michael Catanzaro
Modified: 2016-09-07 11:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.62 KB, patch)
2016-01-03 16:48 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2016-01-03 17:33 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (9.92 KB, patch)
2016-09-05 07:37 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (10.79 KB, patch)
2016-09-05 07:47 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (9.96 KB, patch)
2016-09-05 07:52 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (11.45 KB, patch)
2016-09-05 08:11 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2016-09-05 08:23 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (13.36 KB, patch)
2016-09-05 08:31 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (15.48 KB, patch)
2016-09-05 08:47 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (16.78 KB, patch)
2016-09-05 08:54 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (17.47 KB, patch)
2016-09-05 09:07 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (1.49 MB, application/zip)
2016-09-05 10:36 PDT, Build Bot
no flags Details
Patch (17.41 KB, patch)
2016-09-06 07:52 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-01-03 16:44:27 PST
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.
Comment 1 Michael Catanzaro 2016-01-03 16:48:24 PST
Created attachment 268157 [details]
Patch
Comment 2 Michael Catanzaro 2016-01-03 17:33:17 PST
Created attachment 268160 [details]
Patch
Comment 3 Michael Catanzaro 2016-01-03 18:11:51 PST
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 4 Carlos Garcia Campos 2016-01-04 00:50:04 PST
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.
Comment 5 Michael Catanzaro 2016-01-04 07:32:03 PST
(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.
Comment 6 Michael Catanzaro 2016-09-05 07:37:27 PDT
Created attachment 287955 [details]
Patch
Comment 7 Michael Catanzaro 2016-09-05 07:47:12 PDT
Created attachment 287957 [details]
Patch
Comment 8 Michael Catanzaro 2016-09-05 07:52:30 PDT
Created attachment 287958 [details]
Patch
Comment 9 Michael Catanzaro 2016-09-05 08:11:26 PDT
Created attachment 287959 [details]
Patch
Comment 10 Michael Catanzaro 2016-09-05 08:23:51 PDT
Created attachment 287960 [details]
Patch
Comment 11 Michael Catanzaro 2016-09-05 08:31:10 PDT
Created attachment 287961 [details]
Patch
Comment 12 Michael Catanzaro 2016-09-05 08:37:41 PDT
I need a WebKit2 owner to approve the change in NetworkCacheBlobStorage.cpp.
Comment 13 Michael Catanzaro 2016-09-05 08:47:46 PDT
Created attachment 287963 [details]
Patch
Comment 14 Michael Catanzaro 2016-09-05 08:54:19 PDT
Created attachment 287964 [details]
Patch
Comment 15 Michael Catanzaro 2016-09-05 09:07:06 PDT
Created attachment 287965 [details]
Patch
Comment 16 Build Bot 2016-09-05 10:36:10 PDT
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
Comment 17 Build Bot 2016-09-05 10:36:13 PDT
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 18 Michael Catanzaro 2016-09-05 11:31:25 PDT
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 19 Alex Christensen 2016-09-05 22:57:22 PDT
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.
Comment 20 Michael Catanzaro 2016-09-06 07:52:09 PDT
(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.
Comment 21 Michael Catanzaro 2016-09-06 07:52:39 PDT
Created attachment 288024 [details]
Patch
Comment 22 Michael Catanzaro 2016-09-07 10:53:41 PDT
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 23 WebKit Commit Bot 2016-09-07 11:15:08 PDT
Comment on attachment 288024 [details]
Patch

Clearing flags on attachment: 288024

Committed r205556: <http://trac.webkit.org/changeset/205556>
Comment 24 WebKit Commit Bot 2016-09-07 11:15:16 PDT
All reviewed patches have been landed.  Closing bug.