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

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
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/20160728/d1412211/attachment-0001.html>