Correctly report encoded data length from CFNetwork to NetworkResourceLoader
Created attachment 280676 [details] Patch
Created attachment 280715 [details] Patch
Comment on attachment 280715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280715&action=review > Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:263 > + m_handle->client()->didReceiveBuffer(m_handle, SharedBuffer::wrapNSData(data), static_cast<int>(lengthReceived)); This chops a 64-bit integer and turns it into a 32-bit integer. Why is that OK? > Source/WebKit2/NetworkProcess/NetworkLoad.cpp:252 > +void NetworkLoad::didReceiveData(RefPtr<SharedBuffer>&& buffer, int64_t encodedDataLength) How did you choose the type int64_t? Why signed rather than unsigned? Also, other places in the code use “[unsigned] long long” for this. > Source/WebKit2/NetworkProcess/NetworkLoad.cpp:255 > + m_client.didReceiveBuffer(WTFMove(buffer), static_cast<int>(encodedDataLength)); This chops a 64-bit integer and turns it into a 32-bit integer. Why is that OK?
(In reply to comment #3) > Comment on attachment 280715 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280715&action=review > > > Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:263 > > + m_handle->client()->didReceiveBuffer(m_handle, SharedBuffer::wrapNSData(data), static_cast<int>(lengthReceived)); > > This chops a 64-bit integer and turns it into a 32-bit integer. Why is that > OK? There is great inconsistency in how this is passed around. It should be fixed. > > > Source/WebKit2/NetworkProcess/NetworkLoad.cpp:252 > > +void NetworkLoad::didReceiveData(RefPtr<SharedBuffer>&& buffer, int64_t encodedDataLength) > > How did you choose the type int64_t? Why signed rather than unsigned? Also, > other places in the code use “[unsigned] long long” for this. That's what is used in NSURLSession.h. @property (readonly) int64_t countOfBytesReceived; There's a comment saying it can be 0 or -1.
rdar://problem/21319035
Created attachment 280756 [details] Patch
Created attachment 280769 [details] Patch
Created attachment 280774 [details] Patch
Created attachment 280784 [details] Patch
Comment on attachment 280784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280784&action=review > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:265 > + networkDataTask->didReceiveData(WebCore::SharedBuffer::wrapNSData(data), [dataTask countOfBytesReceived]); [dataTask countOfBytesReceived] returns the decoded size, not the encoded size. That's exactly the same as the sum of the previous data->size()s. This gives us no additional information.
Comment on attachment 280715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280715&action=review >>> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:252 >>> +void NetworkLoad::didReceiveData(RefPtr<SharedBuffer>&& buffer, int64_t encodedDataLength) >> >> How did you choose the type int64_t? Why signed rather than unsigned? Also, other places in the code use “[unsigned] long long” for this. > > That's what is used in NSURLSession.h. > @property (readonly) int64_t countOfBytesReceived; > There's a comment saying it can be 0 or -1. OK, but for future reference, that doesn’t make it the correct type for platform-independent code.
<rdar://problem/27927293>
> Comment on attachment 280784 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280784&action=review > > > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:265 > > + networkDataTask->didReceiveData(WebCore::SharedBuffer::wrapNSData(data), [dataTask countOfBytesReceived]); > > [dataTask countOfBytesReceived] returns the decoded size, not the encoded > size. That's exactly the same as the sum of the previous data->size()s. > This gives us no additional information. There is a new SPI we should be able to use now to get the encoded size: -[NSURLSessionTask _countOfBytesReceivedEncoded] I'll update the patch.
Comment on attachment 280784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280784&action=review > Source/WebKit/WebCoreSupport/WebResourceLoadScheduler.cpp:94 > WebResourceLoadScheduler::~WebResourceLoadScheduler() > { > + // FIXME: Why isn't m_nonHTTPProtocolHost deleted? > } It looks like this should delete the host, but I think the WebResourceLoadScheduler (owned by the PlatformStrategy) is never be destroyed. This could move to a separate issue.
Created attachment 314544 [details] [PATCH] Work in Progress I don't think the new SPI is enough. But I'm attaching my work in progress so it doesn't get lost.
Comment on attachment 314544 [details] [PATCH] Work in Progress View in context: https://bugs.webkit.org/attachment.cgi?id=314544&action=review > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:405 > + if ([dataTask respondsToSelector:@selector(_countOfBytesReceivedEncoded)]) { > + WTFLogAlways(">>> didReceiveData (%u) (%u)", (unsigned)dataTask._countOfBytesReceivedEncoded, (unsigned)dataTask.countOfBytesReceived); > + bytesReceived = dataTask._countOfBytesReceivedEncoded ?: dataTask.countOfBytesReceived; > + } This is not good enough. 1. If the response is encoded (compressed) data, then _countOfBytesReceivedEncoded starts with zero and doesn't include headers... 2. If the response is uncompressed data, then countOfBytesReceived starts with non-zero and includes the header size. So without knowing if we are receiving compressed / uncompressed data we can't know which set of APIs to use.