Bug 63916

Summary: Coalesce data array into one NSData before calling didReceiveData
Product: WebKit Reporter: Pratik Solanki <psolanki>
Component: PlatformAssignee: Pratik Solanki <psolanki>
Severity: Normal CC: ap, koivisto, mitz, psolanki
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch mitz: review+

Description Pratik Solanki 2011-07-04 12:51:57 PDT
If the ResourceHandleClient does not support accepting data array, we call its didReceiveData() method in a loop. Instead of doing that we should coalesce all the elements in the array and call didReceiveData once.
Comment 1 Pratik Solanki 2011-07-04 12:52:15 PDT
Comment 2 Pratik Solanki 2011-07-04 13:05:44 PDT
Created attachment 99651 [details]
Comment 3 mitz 2011-07-04 13:12:03 PDT
Comment on attachment 99651 [details]

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

> Source/WebCore/ChangeLog:10
> +        the data buffers into one anc all it with all the data at once.

Typo: anc.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:-815
> -        // The call to didReceiveData below could cancel a load, which would result in the delegate
> -        // (self) being released.

Perhaps this is worth replacing with a comment where there is a currently useless return statement saying that at that point self may have been deallocated.
Comment 4 Alexey Proskuryakov 2011-07-04 13:48:59 PDT
> Instead of doing that we should coalesce all the elements

Why should we do that???
Comment 5 Alexey Proskuryakov 2011-07-05 00:00:57 PDT
It seems that it would be more straightforward to remove connection:didReceiveDataArray:, and let CFNetwork do the same coalescing itself before calling connection:didReceiveData:.
Comment 6 Pratik Solanki 2011-07-05 10:01:12 PDT
I discussed this with Alexey offline and explained how we are modifying the else part of

   if (m_handle->client()->supportsDataArray())
       m_handle->client()->didReceiveDataArray(m_handle, reinterpret_cast<CFArrayRef>(dataArray));
   else {
       <other clients that don't handle data arrays>

The patch made more sense to him now. He adds "The problem with not coalescing probably comes from TextResourceDecoder, which scans the source multiple times if it gets small chunks."

I'll check in the patch shortly. Thanks for the review, Dan!
Comment 7 Pratik Solanki 2011-07-05 10:35:03 PDT
Committed r90400: <http://trac.webkit.org/changeset/90400>