[Webkit-unassigned] [Bug 144380] [SOUP] Network Cache: Implement ShareableResource for Soup and enable it for GTK platform

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #2)
> Comment on attachment 251937 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251937&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:130
> > -    auto mappedData = Data::adoptMap(map, size);
> > +    auto mappedData = Data::adoptMap(map, size, fd);
> 
> It would be good to encapsulate the whole "create disk-backed map from
> existing Data" operation and make it a Data function.

Ok.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:42
> >  namespace WebKit {
> > +class SharedMemory;
> 
> empty line after namespace

Sure.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataCocoa.mm:112
> >  Data mapFile(int fd, size_t offset, size_t size)
> >  {
> > -    if (!size)
> > +    if (!size) {
> > +        close(fd);
> >          return Data::empty();
> > +    }
> 
> It is surprising this function now closes the file. At minimum it should be
> renamed to something like adoptAndMapFile. There are a bunch of clients that
> call this and close the file manually.

I agree it's not that obvious that mapFile takes ownership of the fd. I think, I updated all callers to not close the fd, or what clients do you mean?

> We really need better abstactions though.
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataCocoa.mm:157
> > +RefPtr<SharedMemory> Data::createSharedMemory() const
> 
> tryCreateSharedMemory?

It makes sense, yes.

> > Source/WebKit2/Shared/soup/ShareableResourceSoup.cpp:43
> > +PassRefPtr<SharedBuffer> ShareableResource::Handle::tryWrapInSharedBuffer() const
> 
> Adding this Soup-specific stuff is going to make it harder to get rid of
> these ugly abstractions. :(

Yes, this is because current tryWrapInSharedBuffer() method is CF specific, but implemented in the main cpp file. I just provided a soup implementation, but using its own platform file, I could move it to the cpp file though.

-- 
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/20150505/7e98aee1/attachment.html>