[webkit-reviews] review canceled: [Bug 118343] [WK2][Soup] Support cache model in NetworkProcess : [Attachment 213980] Patch

[bugzilla-daemon at webkit.org]

Csaba Osztrogonac <ossy at webkit.org> has canceled Csaba Osztrogonac
<ossy at webkit.org>'s request for review:
Bug 118343: [WK2][Soup] Support cache model in NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=118343

Attachment 213980: Patch
https://bugs.webkit.org/attachment.cgi?id=213980&action=review

------- Additional Comments from Csaba Osztrogonac <ossy at webkit.org>
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.