Bug 160237 – Split calculateCacheSizes in two methods

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It's used to calculate memory and disk cache sizes, but only the web process is interested in memory caches, and the network process in disk cache. We can also avoid a lot of duplicated code between ports to set the cache model.

[Carlos Garcia Campos]

Created attachment 284681 [details]
Patch

[Carlos Garcia Campos]

Created attachment 284685 [details]
Try to fix mac builds

[Darin Adler]

Comment on attachment 284685 [details]
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.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 284685 [details]
> 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.

Thanks for the review. Note that I haven't changed the variable types at all, I just split the method leaving the same types. I agree this is a perfect moment to fix them if they have always been wrong.

> > 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.

Sure. I think the ForPath is because the method doesn't really operate on the path, but on the volume where the path is. But yes, I think it's clear enough to not need the suffix.

> Is it really acceptable for this function to return 0 to indicate failure?

I don't think we are checking the return value anyway, I'll fix that too.

> > 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?

Right, good point.

> > 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.

Indeed.

> > 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.

Ok, I will check which types to use.

> > 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.

Me neither, I noticed that we were doing this inside a GTK platform ifdef in setCacheModel, and I thought that was not the right place anyway to set that, so when I was to the constructor to put it there, I noticed it was alreasdy there for iOS, so I just added the GTK to the condition. But I don't know why we are doing that in GTK and not in other ports, to be honest, just wanted to make sure this refactoring doesn't change any behavior.

[Carlos Garcia Campos]

Created attachment 284758 [details]
Updated patch

[Darin Adler]

Comment on attachment 284758 [details]
Updated patch

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

> Source/WebCore/platform/mac/FileSystemMac.mm:105
> +    NSDictionary *fileSystemAttributesDictionary = [[NSFileManager defaultManager] attributesOfFileSystemForPath:(NSString *)path error:NULL];
> +    freeSpace = [[fileSystemAttributesDictionary objectForKey:NSFileSystemFreeSize] unsignedLongLongValue];
> +    return true;

If fileSystemAttributesDictionary is nil, then the function failed, and should return false.

It’s funny that Apple API uses the term "file system free size", the same term I asked you to stop using here!

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 284758 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284758&action=review
> 
> > Source/WebCore/platform/mac/FileSystemMac.mm:105
> > +    NSDictionary *fileSystemAttributesDictionary = [[NSFileManager defaultManager] attributesOfFileSystemForPath:(NSString *)path error:NULL];
> > +    freeSpace = [[fileSystemAttributesDictionary objectForKey:NSFileSystemFreeSize] unsignedLongLongValue];
> > +    return true;
> 
> If fileSystemAttributesDictionary is nil, then the function failed, and
> should return false.

Ah, ok.

> It’s funny that Apple API uses the term "file system free size", the same
> term I asked you to stop using here!

Yes, I guess that's where the original function name came from actually, I just added the ForPath suffix to make it even worse :-P

[Carlos Garcia Campos]

Committed r203857: <http://trac.webkit.org/changeset/203857>