Bug 118343

Created attachment 205980 [details] Patch

Comment on attachment 205980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205980&action=review > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:81 > + g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE, > + SOUP_SESSION_SSL_STRICT, FALSE, 0); The last argument of g_object_set should be NULL instead of 0. Otherwise, the compiler warns "missing sentinel in function call [-Wformat]"

just a note: this patch might conflict with the patch in https://bugs.webkit.org/show_bug.cgi?id=118520. Only the Source/WebKit2/GNUmakefile.list.am, it depends on which patch lands eaarlier. :)

(In reply to comment #1) > Created an attachment (id=205980) [details] > Patch The patch is still up-to-date. Could you review it please?

Comment on attachment 205980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205980&action=review > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:79 > + SoupSession* session = WebCore::ResourceHandle::defaultSession(); Just a note: bug118388 is going to add a session named variable to the same block. This one or the other one should be modified/removed before landing. Maybe the best thing would be to let the other bug define and initialize this variable and simple use it here.

Created attachment 212715 [details] patch updated: one more include added, avoid the conflict with bug118388

Now it depends on bug118388 too because of session variable.

Created attachment 213006 [details] patch (In reply to comment #2) > (From update of attachment 205980 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205980&action=review > > > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:81 > > + g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE, > > + SOUP_SESSION_SSL_STRICT, FALSE, 0); > > The last argument of g_object_set should be NULL instead of 0. Otherwise, the compiler warns "missing sentinel in function call [-Wformat]" Fixed this warning too.

ping?

Comment on attachment 213006 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=213006&action=review I'm fine with this if Soup reviewers are. > Source/WebKit2/ChangeLog:15 > + The following things were fixed by Csaba Osztrogonác: Encoding issue in the ChangeLog?

(In reply to comment #10) > (From update of attachment 213006 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=213006&action=review > > I'm fine with this if Soup reviewers are. cc Carlos as SOUP reviewer. Any objection? > > Source/WebKit2/ChangeLog:15 > > + The following things were fixed by Csaba Osztrogonác: > > Encoding issue in the ChangeLog? No, the bug is in the pretty diff script, not in my name :)

Comment on attachment 213006 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=213006&action=review > Source/WebKit2/GNUmakefile.list.am:483 > + Source/WebKit2/Shared/soup/CacheModelHelper.cpp \ > + Source/WebKit2/Shared/soup/CacheModelHelper.h \ I'm going to add the new files to cmake buildsystem too. > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:38 > +#include <libsoup/soup.h> ditto > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:80 > + // Despite using system CAs to validate certificates we're > + // accepting invalid certificates by default. New API will be > + // added later to let client accept/discard invalid certificates. > + g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE, > + SOUP_SESSION_SSL_STRICT, FALSE, NULL); > + Now all unix ports use soup, but it won't be true in the future after Nix will switch to curl. That's why I'm going to add a USE(SOUP) guard for this block. > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:86 > + if (SoupSessionFeature* soupCache = soup_session_get_feature(session, SOUP_TYPE_CACHE)) { > + soup_cache_flush(SOUP_CACHE(soupCache)); > + soup_cache_dump(SOUP_CACHE(soupCache)); > + } ditto

Created attachment 213980 [details] Patch blind fix, will be built locally a little bit later

(In reply to comment #10) > > Source/WebKit2/ChangeLog:15 > > + The following things were fixed by Csaba Osztrogonác: > > Encoding issue in the ChangeLog? Ooops, you were right, it was really buggy, I fixed it too.

Attachment 213980 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp', u'Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp', u'Source/WebKit2/PlatformEfl.cmake', u'Source/WebKit2/PlatformGTK.cmake', u'Source/WebKit2/Shared/soup/CacheModelHelper.cpp', u'Source/WebKit2/Shared/soup/CacheModelHelper.h', u'Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp']" exit_code: 1 Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:82: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #15) > Attachment 213980 [details] did not pass style-queue: > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:82: Use 0 instead of NULL. [readability/null] [5] > Total errors found: 1 in 9 files False positive report, we have to use NULL here to avoid build failure, see Comment #2 for details.

(In reply to comment #13) > Created an attachment (id=213980) [details] > Patch > > blind fix, will be built locally a little bit later I tested, it builds locally with enabled network process.

Carlos, could you review it, please?

Comment on attachment 213980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213980&action=review Looks good to me in general, I have a few comments. > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:32 > +#include "Logging.h" What is logging used for? > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:64 > + SoupSession* session = WebCore::ResourceHandle::defaultSession(); You don't need the WebCore:: prefix. > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:87 > + SoupSession* session = WebCore::ResourceHandle::defaultSession(); Ditto. > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:81 > + g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE, Where does this session came from? > Source/WebKit2/Shared/soup/CacheModelHelper.cpp:48 > + return WebCore::getVolumeFreeSizeForPath(cacheDir.get()); You don't need the WebCore:: prefix here either. > Source/WebKit2/Shared/soup/CacheModelHelper.cpp:68 > +uint64_t getMemorySize() > +{ > + static uint64_t kDefaultMemorySize = 512; > +#if !OS(WINDOWS) > + long pageSize = sysconf(_SC_PAGESIZE); > + if (pageSize == -1) > + return kDefaultMemorySize; > + > + long physPages = sysconf(_SC_PHYS_PAGES); > + if (physPages == -1) > + return kDefaultMemorySize; > + > + return ((pageSize / 1024) * physPages) / 1024; > +#else > + // Fallback to default for other platforms. > + return kDefaultMemorySize; > +#endif > +} Mac also has a function to get the memory size duplicated in WebProcessMac.mm and NetworkProcessMac.mm, I think we could add a common interface to WebCore platform with both implementations there, since this method doesn't depend on any cache model. getCacheDiskFreeSize() is simple enough that we could have it duplicated in WebProcessSoup and NetworkProcessSoup so that we don't need this new file just for a simple function. > Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:-80 > - I think you should also check in the web process if the network process is in use to not use the disk cache or other network stuff.

Comment on attachment 213980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213980&action=review >> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:32 >> +#include "Logging.h" > > What is logging used for? We don't need it, I'll remove. >> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:64 >> + SoupSession* session = WebCore::ResourceHandle::defaultSession(); > > You don't need the WebCore:: prefix. Will remove it. >> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:81 >> + g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE, > > Where does this session came from? Tricky. :) It comes from http://trac.webkit.org/changeset/157406 >> Source/WebKit2/Shared/soup/CacheModelHelper.cpp:48 >> + return WebCore::getVolumeFreeSizeForPath(cacheDir.get()); > > You don't need the WebCore:: prefix here either. Will remove. >> Source/WebKit2/Shared/soup/CacheModelHelper.cpp:68 >> +} > > Mac also has a function to get the memory size duplicated in WebProcessMac.mm and NetworkProcessMac.mm, I think we could add a common interface to WebCore platform with both implementations there, since this method doesn't depend on any cache model. getCacheDiskFreeSize() is simple enough that we could have it duplicated in WebProcessSoup and NetworkProcessSoup so that we don't need this new file just for a simple function. I checked the Mac implementation, they use exactly same memorySize function, and similar volumeFreeSize() functions. The only difference is the parameter type (NSString* vs const String&) Additionally they have a same volumeFreeSize implementation (as the WebProcess one) in Source/WebKit/mac/Misc/WebKitSystemBits.m and an ancient version of memorySize. I don't think if we should do this unification here. It can be done as a followup patch, it must be a low hanging fruit for somebody with Mac build environment. >> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:-80 >> - > > I think you should also check in the web process if the network process is in use to not use the disk cache or other network stuff. Good point, I'll fix it.

Created attachment 214269 [details] patch on top of attachment213980 [details] I'll integrate it to https://bugs.webkit.org/attachment.cgi?id=213980 immediately.

Created attachment 214274 [details] Patch

Attachment 214274 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp', u'Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp', u'Source/WebKit2/PlatformEfl.cmake', u'Source/WebKit2/PlatformGTK.cmake', u'Source/WebKit2/Shared/soup/CacheModelHelper.cpp', u'Source/WebKit2/Shared/soup/CacheModelHelper.h', u'Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp']" exit_code: 1 Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:95: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

ping?

ping for review please

ping

ping

(In reply to comment #20) > >> Source/WebKit2/Shared/soup/CacheModelHelper.cpp:68 > >> +} > > > > Mac also has a function to get the memory size duplicated in WebProcessMac.mm and NetworkProcessMac.mm, I think we could add a common interface to WebCore platform with both implementations there, since this method doesn't depend on any cache model. getCacheDiskFreeSize() is simple enough that we could have it duplicated in WebProcessSoup and NetworkProcessSoup so that we don't need this new file just for a simple function. > > I checked the Mac implementation, they use exactly same memorySize function, and similar > volumeFreeSize() functions. The only difference is the parameter type (NSString* vs const String&) > Additionally they have a same volumeFreeSize implementation (as the WebProcess one) in > Source/WebKit/mac/Misc/WebKitSystemBits.m and an ancient version of memorySize. > > I don't think if we should do this unification here. It can be done as a followup patch, > it must be a low hanging fruit for somebody with Mac build environment. > If we are going to do it in a follow up patch, I prefer to duplicate the code for now, instead of adding new files that we are going to remove later. It will make this patch simpler too.

Comment on attachment 214274 [details] Patch Would it be possible to have m_usesNetworkProcess unconditionally and initialize it to false when !ENABLE(NETWORK_PROCESS) so that we don't need an #if every time the variable is used?

(In reply to comment #29) > (From update of attachment 214274 [details]) > Would it be possible to have m_usesNetworkProcess unconditionally and initialize it to false when !ENABLE(NETWORK_PROCESS) so that we don't need an #if every time the variable is used? It sounds good, but it should be a separate refactoring patch because the current convention of guarding m_usesNetworkProcess with ENABLE(NETWORK_PROCESS) is all over WebKit2.

Created attachment 218743 [details] Patch

(In reply to comment #28) > If we are going to do it in a follow up patch, I prefer to duplicate the code for now, instead of adding new files that we are going to remove later. It will make this patch simpler too. Done!

Attachment 218743 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp', u'Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp', u'Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp', '--commit-queue']" exit_code: 1 ERROR: Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:95: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 218743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218743&action=review > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:36 > + Please, remove this empty line > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:40 > +#if USE(SOUP) > +#include <libsoup/soup.h> > +#endif I think we don't need to keep the alphabetic order for headers added inside an #if block, so I think it's better to move this below right after the #if PLATFORM(EFL) block > Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:104 > +#if ENABLE(NETWORK_PROCESS) > + if (!m_usesNetworkProcess) { > +#endif > + SoupSession* session = WebCore::ResourceHandle::defaultSession(); > + cache = SOUP_CACHE(soup_session_get_feature(session, SOUP_TYPE_CACHE)); > + diskFreeSize = getCacheDiskFreeSize(cache) / 1024 / 1024; > +#if ENABLE(NETWORK_PROCESS) > + } > +#endif I think we could define those unconditionally and return false when network process is not enabled, but if we can probably do it in a follow up patch to not block this one.

Committed r160307: <http://trac.webkit.org/changeset/160307>