[Webkit-unassigned] [Bug 160237] Split calculateCacheSizes in two methods

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=160237

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #284685|review?                     |review-
              Flags|                            |

--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 284685
  --> https://bugs.webkit.org/attachment.cgi?id=284685
Try to fix mac builds

View in context: https://bugs.webkit.org/attachment.cgi?id=284685&action=review

This introduces potential problems with values that don’t fit in 32-bits so that, at least, needs to be fixed.

> Source/WebCore/platform/FileSystem.h:206
> +WEBCORE_EXPORT uint64_t volumeFreeSizeForPath(const String&);

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?

> Source/WebCore/platform/efl/FileSystemEfl.cpp:93
> +    CString fsRep = fileSystemRepresentation(path);

I believe fileSystemRepresentation can fail and it returns an empty string in that case. Does this function do the right thing in that case?

> Source/WebCore/platform/glib/FileSystemGlib.cpp:237
> +    GUniquePtr<gchar> filename = unescapedFilename(path);

Other functions in this file check for null in the result of this function and handle it. This function should probably do the same.

> Source/WebKit2/NetworkProcess/NetworkProcess.h:200
> +    void platformSetURLCacheSize(unsigned long urlCacheMemoryCapacity, unsigned long urlCacheDiskCapacity);

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.

> Source/WebKit2/Shared/CacheModel.h:41
> +void calculateURLCacheSizes(CacheModel, uint64_t diskFreeSize, unsigned long& urlCacheMemoryCapacity, unsigned long& urlCacheDiskCapacity);

Same problem here. The type "unsigned long" should almost never be used anywhere in WebKit.

> Source/WebKit2/WebProcess/WebProcess.cpp:210
> -#if PLATFORM(IOS)
> +#if PLATFORM(IOS) || PLATFORM(GTK)
>      PageCache::singleton().setShouldClearBackingStores(true);
>  #endif

Don’t understand the rationale for doing this on iOS and GTK but not Mac. Probably needs a comment.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160727/3ca34163/attachment.html>