Bug 63916 - Coalesce data array into one NSData before calling didReceiveData
Summary: Coalesce data array into one NSData before calling didReceiveData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-07-04 12:51 PDT by Pratik Solanki
Modified: 2011-07-05 10:35 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.67 KB, patch)
2011-07-04 13:05 PDT, Pratik Solanki
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
<rdar://problem/9715181>
Comment 2 Pratik Solanki 2011-07-04 13:05:44 PDT
Created attachment 99651 [details]
Patch
Comment 3 mitz 2011-07-04 13:12:03 PDT
Comment on attachment 99651 [details]
Patch

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>