Bug 143872 – [SOUP] Add initial implementation of NetworkProcess disk cache

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Add initial implementation of NetworkProcess disk cache for Soup.

[Carlos Garcia Campos]

Created attachment 251014 [details]
Patch

It's enabled by default only for EWS, but I don't plan to enable it in this patch.

[WebKit Commit Bot]

Attachment 251014 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:123:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:137:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:170:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:194:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Found 14 tests; running 14, skipping 0.
Running 1 WebKitTestRunner.     

All 14 tests ran as expected.

All tests pass, but I haven't tested it much more than the layout tests and some navigation with the MiniBrowser to dump the cache to a json file.

[Chris Dumez]

Comment on attachment 251014 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:114
> +    Data(SoupBuffer*, Backing = Backing::Buffer);

Could this be a GRefPtr<SourceBuffer>&& to make memory management clearer?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:56
> +    return { buffer.get() };

then this would be a return { WTF::move(buffer) };

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 251014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251014&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:114
> > +    Data(SoupBuffer*, Backing = Backing::Buffer);
> 
> Could this be a GRefPtr<SourceBuffer>&& to make memory management clearer?

Yes.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:56
> > +    return { buffer.get() };
> 
> then this would be a return { WTF::move(buffer) };

Yes.

[Carlos Garcia Campos]

Created attachment 251151 [details]
Updated patch

[WebKit Commit Bot]

Attachment 251151 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:123:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:137:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:170:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:194:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Comment on attachment 251014 [details]
Patch

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

Awesome stuff Carlos!

> Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:34
> +#include <fcntl.h>

Why this?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:29
> +#if ENABLE(NETWORK_CACHE)

Don't we need some USE(SOUP) or PLATFORM(LINUX) and the like?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:129
> +    ASSERT(map && map != MAP_FAILED);

Perhaps better to split this in 2 ASSERTs in order to better spot the actual cause of failure.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:58
> +    ASSERT(m_outputStream || m_inputStream || m_ioStream);

Same here about splitting the ASSERT

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:82
> +    }

Can't we use Data directly instead of having to copy it from a SoupBuffer?

[Carlos Garcia Campos]

(In reply to comment #8)
> Comment on attachment 251014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251014&action=review
> 
> Awesome stuff Carlos!

Thanks.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:34
> > +#include <fcntl.h>
> 
> Why this?

It's needed for open and its flags (O_CREAT, O_RDWR, etc.) in Linux, I guess it's not needed in mac, but doesn't hurt anyway.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:29
> > +#if ENABLE(NETWORK_CACHE)
> 
> Don't we need some USE(SOUP) or PLATFORM(LINUX) and the like?

I assumed NETWORK_CACHE is already enabled depending on the port.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:129
> > +    ASSERT(map && map != MAP_FAILED);
> 
> Perhaps better to split this in 2 ASSERTs in order to better spot the actual
> cause of failure.

Ok, I think I copied this one from mac impl :-)

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:58
> > +    ASSERT(m_outputStream || m_inputStream || m_ioStream);
> 
> Same here about splitting the ASSERT

Sure.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:82
> > +    }
> 
> Can't we use Data directly instead of having to copy it from a SoupBuffer?

I'm not sure I understand the question. We create a Data with a SoupBuffer to avoid copying the data. The read soup buffer will be reused on next read, so we need to copy that data appending it to the Data.

[Carlos Garcia Campos]

Created attachment 251154 [details]
Another update

[WebKit Commit Bot]

Attachment 251154 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:138:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:171:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:195:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 251154 [details]
Another update

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

Looks good to me, but we should chat briefly about benefits and drawbacks of using WebKit's builtin memory manager.

> Source/WebCore/platform/network/soup/GRefPtrSoup.cpp:24
> +template <> SoupBuffer* refGPtr(SoupBuffer* ptr)

Nit: Do you mind using 'buffer' here instead of 'ptr'?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:41
> +    : m_buffer(adoptGRef(soup_buffer_new(SOUP_MEMORY_COPY, data, size)))

We should probably discuss whether we want to use the WebKit memory allocator.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:94
> +    uint8_t* data = static_cast<uint8_t*>(g_malloc(size));

Ditto.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:101
> +    if (bytesRead == -1) {
> +        asyncData->completionHandler(asyncData->data, -1);
> +        delete asyncData;
> +        return;
> +    }

It is annoying that GFileError cannot be used here for the error code, because the enum starts at zero. Perhaps a comment here about that could be informative.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:144
> +    gssize bytesRead;

This can move into the do-while loop, I think.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:149
> +        bytesRead = g_input_stream_read(m_inputStream.get(), const_cast<char*>(readBuffer->data), bytesToRead, nullptr, nullptr);

Same issue here with GFileError. :(

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:181
> +    if (bytesWritten == -1) {
> +        asyncData->completionHandler(-1);
> +        delete asyncData;
> +        return;
> +    }

Ditto.

[Carlos Garcia Campos]

(In reply to comment #12)
> Comment on attachment 251154 [details]
> Another update
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251154&action=review
> 
> Looks good to me, but we should chat briefly about benefits and drawbacks of
> using WebKit's builtin memory manager.

Please note that this is a first implementation of a feature that is disabled, it's a first step to continue working on it.

> > Source/WebCore/platform/network/soup/GRefPtrSoup.cpp:24
> > +template <> SoupBuffer* refGPtr(SoupBuffer* ptr)
> 
> Nit: Do you mind using 'buffer' here instead of 'ptr'?

Sure, we use ptr in all GRefPtrFoo implementations, though.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:41
> > +    : m_buffer(adoptGRef(soup_buffer_new(SOUP_MEMORY_COPY, data, size)))
> 
> We should probably discuss whether we want to use the WebKit memory
> allocator.

I'm not sure I get what you mean.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:94
> > +    uint8_t* data = static_cast<uint8_t*>(g_malloc(size));
> 
> Ditto.

Soup uses g_free unconditionally, so when it takes the memory it needs to be allocated with g_malloc(). 

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:101
> > +    if (bytesRead == -1) {
> > +        asyncData->completionHandler(asyncData->data, -1);
> > +        delete asyncData;
> > +        return;
> > +    }
> 
> It is annoying that GFileError cannot be used here for the error code,
> because the enum starts at zero. Perhaps a comment here about that could be
> informative.

It can be used, but I didn't care about that yet, I don't even know if those errors are actually handled, or just used to check whether it failed or not.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:144
> > +    gssize bytesRead;
> 
> This can move into the do-while loop, I think.

Yes, I think the first impl used that in the while comparison.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:149
> > +        bytesRead = g_input_stream_read(m_inputStream.get(), const_cast<char*>(readBuffer->data), bytesToRead, nullptr, nullptr);
> 
> Same issue here with GFileError. :(
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:181
> > +    if (bytesWritten == -1) {
> > +        asyncData->completionHandler(-1);
> > +        delete asyncData;
> > +        return;
> > +    }
> 
> Ditto.

[Martin Robinson]

Comment on attachment 251154 [details]
Another update

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

>>> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:41
>>> +    : m_buffer(adoptGRef(soup_buffer_new(SOUP_MEMORY_COPY, data, size)))
>> 
>> We should probably discuss whether we want to use the WebKit memory allocator.
> 
> I'm not sure I get what you mean.

I'm simply suggesting that it might be worth it to use bmalloc here and then to free the memory properly by using soup_buffer_new_with_owner instead.

>>> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:94
>>> +    uint8_t* data = static_cast<uint8_t*>(g_malloc(size));
>> 
>> Ditto.
> 
> Soup uses g_free unconditionally, so when it takes the memory it needs to be allocated with g_malloc().

We would need to use soup_buffer_new_with_owner.

>>> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:101
>>> +    }
>> 
>> It is annoying that GFileError cannot be used here for the error code, because the enum starts at zero. Perhaps a comment here about that could be informative.
> 
> It can be used, but I didn't care about that yet, I don't even know if those errors are actually handled, or just used to check whether it failed or not.

Hrm. Isn't the first enum member of GFileError (G_FILE_ERROR_EXIST) defined to be 0?

[Carlos Garcia Campos]

(In reply to comment #14)
> Comment on attachment 251154 [details]
> Another update
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251154&action=review
> 
> >>> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:41
> >>> +    : m_buffer(adoptGRef(soup_buffer_new(SOUP_MEMORY_COPY, data, size)))
> >> 
> >> We should probably discuss whether we want to use the WebKit memory allocator.
> > 
> > I'm not sure I get what you mean.
> 
> I'm simply suggesting that it might be worth it to use bmalloc here and then
> to free the memory properly by using soup_buffer_new_with_owner instead.

Ah, ok, I thought you were suggesting to replace soup buffer somehow.

> >>> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:94
> >>> +    uint8_t* data = static_cast<uint8_t*>(g_malloc(size));
> >> 
> >> Ditto.
> > 
> > Soup uses g_free unconditionally, so when it takes the memory it needs to be allocated with g_malloc().
> 
> We would need to use soup_buffer_new_with_owner.

Got it.

> >>> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:101
> >>> +    }
> >> 
> >> It is annoying that GFileError cannot be used here for the error code, because the enum starts at zero. Perhaps a comment here about that could be informative.
> > 
> > It can be used, but I didn't care about that yet, I don't even know if those errors are actually handled, or just used to check whether it failed or not.
> 
> Hrm. Isn't the first enum member of GFileError (G_FILE_ERROR_EXIST) defined
> to be 0?

My point is that I haven't cared about the actual error codes at this point because the caller use the error as a boolean (just to check whether it failed) or for the debug logs, so it doesn't really matter.

[Carlos Garcia Campos]

Created attachment 251543 [details]
Updated to use fast malloc/free

[WebKit Commit Bot]

Attachment 251543 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:138:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:170:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:194:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 251543 [details]
Updated to use fast malloc/free

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:129
> +    GRefPtr<SoupBuffer> buffer = adoptGRef(soup_buffer_new(SOUP_MEMORY_TAKE, static_cast<char*>(g_malloc(bufferSize)), bufferSize));

Here it might be better to use fastMalloc as well.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:142
> +    GRefPtr<SoupBuffer> readBuffer = adoptGRef(soup_buffer_new(SOUP_MEMORY_TAKE, static_cast<char*>(g_malloc(bufferSize)), bufferSize));

Ditto.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:162
> +    size_t bytesToRead = bufferSize;
> +    do {
> +        // FIXME: implement offset.
> +        gssize bytesRead = g_input_stream_read(m_inputStream.get(), const_cast<char*>(readBuffer->data), bytesToRead, nullptr, nullptr);
> +        if (bytesRead == -1) {
> +            completionHandler(data, -1);
> +            return;
> +        }
> +
> +        if (!bytesRead)
> +            break;
> +
> +        ASSERT(bytesRead > 0);
> +        fillDataFromReadBuffer(readBuffer.get(), static_cast<size_t>(bytesRead), data);
> +
> +        pendingBytesToRead = size - data.size();
> +        bytesToRead = std::min(pendingBytesToRead, readBuffer->length);
> +    } while (pendingBytesToRead);

Sorry I missed this in the first review. I have a quick question about the synchronous read case. Why is it better to read chunks of data and constantly fill up small buffers? Why not create a buffer the full size of the data and only create a single Data object? Couldn't you avoid all of the concatenation that way?

Otherwise, I suggest a small rename here for the sake of clarity: pendingBytesToRead -> bytesToRead, bytesToRead -> bytesToReadForCurrentChunk.

[Carlos Garcia Campos]

(In reply to comment #18)
> Comment on attachment 251543 [details]
> Updated to use fast malloc/free
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251543&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:129
> > +    GRefPtr<SoupBuffer> buffer = adoptGRef(soup_buffer_new(SOUP_MEMORY_TAKE, static_cast<char*>(g_malloc(bufferSize)), bufferSize));
> 
> Here it might be better to use fastMalloc as well.

Right, I forgot we were also allocating memory here.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:142
> > +    GRefPtr<SoupBuffer> readBuffer = adoptGRef(soup_buffer_new(SOUP_MEMORY_TAKE, static_cast<char*>(g_malloc(bufferSize)), bufferSize));
> 
> Ditto.

Right.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:162
> > +    size_t bytesToRead = bufferSize;
> > +    do {
> > +        // FIXME: implement offset.
> > +        gssize bytesRead = g_input_stream_read(m_inputStream.get(), const_cast<char*>(readBuffer->data), bytesToRead, nullptr, nullptr);
> > +        if (bytesRead == -1) {
> > +            completionHandler(data, -1);
> > +            return;
> > +        }
> > +
> > +        if (!bytesRead)
> > +            break;
> > +
> > +        ASSERT(bytesRead > 0);
> > +        fillDataFromReadBuffer(readBuffer.get(), static_cast<size_t>(bytesRead), data);
> > +
> > +        pendingBytesToRead = size - data.size();
> > +        bytesToRead = std::min(pendingBytesToRead, readBuffer->length);
> > +    } while (pendingBytesToRead);
> 
> Sorry I missed this in the first review. I have a quick question about the
> synchronous read case. Why is it better to read chunks of data and
> constantly fill up small buffers? Why not create a buffer the full size of
> the data and only create a single Data object? Couldn't you avoid all of the
> concatenation that way?

Because we don't know the size, NetworkCacheStorage passes std::numeric_limits<size_t>::max() to ensure we read until end of stream. Note that this readSync method is only used for debugging, when a dump.json file is requested to be created by the user, it's not used when retrieving cache resources for loading.

> Otherwise, I suggest a small rename here for the sake of clarity:
> pendingBytesToRead -> bytesToRead, bytesToRead -> bytesToReadForCurrentChunk.

[Carlos Garcia Campos]

Created attachment 251553 [details]
Use fast malloc/free in IOChannel too

[WebKit Commit Bot]

Attachment 251553 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:139:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:172:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:196:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 251553 [details]
Use fast malloc/free in IOChannel too

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:201
> +    WriteAsyncData* asyncData = new WriteAsyncData { this, data.soupBuffer(), completionHandler };

It would be nicer and safer to use std::unique_ptr instead of explicit new/delete for memory management.

auto asyncData = std::make_unique<WriteAsyncData>(...)

[Carlos Garcia Campos]

(In reply to comment #22)
> Comment on attachment 251553 [details]
> Use fast malloc/free in IOChannel too
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251553&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:201
> > +    WriteAsyncData* asyncData = new WriteAsyncData { this, data.soupBuffer(), completionHandler };
> 
> It would be nicer and safer to use std::unique_ptr instead of explicit
> new/delete for memory management.
> 
> auto asyncData = std::make_unique<WriteAsyncData>(...)

I didn't do this because the pointer is passed as user data of a C callback.

[Carlos Garcia Campos]

(In reply to comment #23)
> (In reply to comment #22)
> > Comment on attachment 251553 [details]
> > Use fast malloc/free in IOChannel too
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=251553&action=review
> > 
> > > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:201
> > > +    WriteAsyncData* asyncData = new WriteAsyncData { this, data.soupBuffer(), completionHandler };
> > 
> > It would be nicer and safer to use std::unique_ptr instead of explicit
> > new/delete for memory management.
> > 
> > auto asyncData = std::make_unique<WriteAsyncData>(...)
> 
> I didn't do this because the pointer is passed as user data of a C callback.

Well, I can adopt the pointer at the beginning of the clabback and release it before calling the read/write method method again. It's still tricky, because we can't use the wrapped pointer in the read/write call, so we need to save the buffer to a local variable and then release the unique_ptr. I'll update the patch.

[Carlos Garcia Campos]

Created attachment 251679 [details]
Patch

[WebKit Commit Bot]

Attachment 251679 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:90:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:139:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:172:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:197:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheDataSoup.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 251679 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119
> +    userData = asyncData.release();

You should be able to avoid this assignment by passing ayncData.release() directly.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:192
> +    userData = asyncData.release();

Ditto.

[Carlos Garcia Campos]

(In reply to comment #27)
> Comment on attachment 251679 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251679&action=review

Thanks for the review!

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119
> > +    userData = asyncData.release();
> 
> You should be able to avoid this assignment by passing ayncData.release()
> directly.

Right, I moved it to make it explicit that using asyncData in the method call would end up in a crash, to avoid a future refactoring thinking, why are we using a variable for the buffer? I'll rather add a comment there, and move the release to the method call.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:192
> > +    userData = asyncData.release();
> 
> Ditto.

[Carlos Garcia Campos]

Committed r183387: <http://trac.webkit.org/changeset/183387>