[webkit-reviews] review denied: [Bug 22700] ApplicationCache should have size limit : [Attachment 25801] Patch to enforce size limit on app cache

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Aaron Golden <aegolden at gmail.com>'s
request for review:
Bug 22700: ApplicationCache should have size limit
https://bugs.webkit.org/show_bug.cgi?id=22700

Attachment 25801: Patch to enforce size limit on app cache
https://bugs.webkit.org/attachment.cgi?id=25801&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +#include <sys/statvfs.h>

I don't believe that statvfs exists on all the platforms that WebKit supports,
so the function to get this information needs to go into FileSystem.h in the
platform directory.

> +static long long _maximumCacheDatabaseSize(void)
> +{
> +    static long long maximumCacheDatabaseSize = -1LL;
> +    if (maximumCacheDatabaseSize == -1LL) {
> +	struct statvfs statistics;
> +	if (!statvfs("/", &statistics)) {
> +	       // Just for readability, pull the relevant bits out of the
struct.
> +	       long long blocksOnFileSystem = statistics.f_blocks;
> +	       long long blocksAvailable = statistics.f_bavail;
> +	       long long blockSize = statistics.f_frsize;
> +	       
> +	       // We limit the cache size to the size of the file system
divided by 32 or the size of the remaining
> +	       // free space in the file system divided by four, whichever is
smaller.
> +	       maximumCacheDatabaseSize = MIN(blocksOnFileSystem * blockSize /
32LL, blocksAvailable * blockSize / 4LL);
> +	} else {
> +	       // Fallback strategy:  If for any reason statvfs fails then we
will simply use an extremely
> +	       // conservative limit for the cache size, 256MB.
> +	       maximumCacheDatabaseSize = 256 * 1024 * 1024;
> +	   }
> +    }
> +    return maximumCacheDatabaseSize;
> +}

We don't use underscores in our private function names.

There's a cleaner way to write this, which is:

    static long long calculateMaximumCacheDatabaseSize()
    {
	// code goes here; no need to use -1LL as a magic value
    }

    static long long maximumCacheDatabaseSize()
    {
	static long long size = calculateMaximumCacheDatabaseSize();
	return size;
    }

> +	   // If the cache has grown too large, wipe it out and start
rebuilding.
> +	   long long fileSize = 0;
> +	   getFileSize(applicationCachePath, fileSize);
> +	   if (fileSize > _maximumCacheDatabaseSize())
> +	       deleteFile(applicationCachePath);

Is wiping out the entire cache the policy we want? This could be really
inconvenient if you're starting up the web browser and the application you
expect to be present is entirely gone.

Can we do better?

review- because this needs to be written in a way that will not break
compilation on platforms that don't have a <sys/statvfs.h> header.