<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Split calculateCacheSizes in two methods"
href="https://bugs.webkit.org/show_bug.cgi?id=160237">bug 160237</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #284685 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Split calculateCacheSizes in two methods"
href="https://bugs.webkit.org/show_bug.cgi?id=160237#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Split calculateCacheSizes in two methods"
href="https://bugs.webkit.org/show_bug.cgi?id=160237">bug 160237</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=284685&action=diff" name="attach_284685" title="Try to fix mac builds">attachment 284685</a> <a href="attachment.cgi?id=284685&action=edit" title="Try to fix mac builds">[details]</a></span>
Try to fix mac builds
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284685&action=review">https://bugs.webkit.org/attachment.cgi?id=284685&action=review</a>
This introduces potential problems with values that don’t fit in 32-bits so that, at least, needs to be fixed.
<span class="quote">> Source/WebCore/platform/FileSystem.h:206
> +WEBCORE_EXPORT uint64_t volumeFreeSizeForPath(const String&);</span >
No need for two blanks lines after this function. I think the right term for this is "volume free space", not "volume free size". The "ForPath" in this function name is peculiar, other functions in the file don’t use that naming. Having this down at the bottom with the platform-specific functions isn’t good. It should be moved up above into the platform-independent section.
Is it really acceptable for this function to return 0 to indicate failure?
<span class="quote">> Source/WebCore/platform/efl/FileSystemEfl.cpp:93
> + CString fsRep = fileSystemRepresentation(path);</span >
I believe fileSystemRepresentation can fail and it returns an empty string in that case. Does this function do the right thing in that case?
<span class="quote">> Source/WebCore/platform/glib/FileSystemGlib.cpp:237
> + GUniquePtr<gchar> filename = unescapedFilename(path);</span >
Other functions in this file check for null in the result of this function and handle it. This function should probably do the same.
<span class="quote">> Source/WebKit2/NetworkProcess/NetworkProcess.h:200
> + void platformSetURLCacheSize(unsigned long urlCacheMemoryCapacity, unsigned long urlCacheDiskCapacity);</span >
The type here is wrong. "unsigned long" is the same size as "unsigned" and is 32-bit even on 64-bit systems with the compilers and environments we use. So the type here should either be "unsigned" if we want 32-bit, or a type like uint64_t or unsigned long long if we want 64-bit.
<span class="quote">> Source/WebKit2/Shared/CacheModel.h:41
> +void calculateURLCacheSizes(CacheModel, uint64_t diskFreeSize, unsigned long& urlCacheMemoryCapacity, unsigned long& urlCacheDiskCapacity);</span >
Same problem here. The type "unsigned long" should almost never be used anywhere in WebKit.
<span class="quote">> Source/WebKit2/WebProcess/WebProcess.cpp:210
> -#if PLATFORM(IOS)
> +#if PLATFORM(IOS) || PLATFORM(GTK)
> PageCache::singleton().setShouldClearBackingStores(true);
> #endif</span >
Don’t understand the rationale for doing this on iOS and GTK but not Mac. Probably needs a comment.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>