Bug 64130 - Implement didReceiveDataArray callback for CFNetwork based loader
Summary: Implement didReceiveDataArray callback for CFNetwork based loader
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:
Depends on:
Blocks: 51836
  Show dependency treegraph
 
Reported: 2011-07-07 16:16 PDT by Pratik Solanki
Modified: 2011-07-12 11:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.17 KB, patch)
2011-07-07 16:46 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch.v2 (8.77 KB, patch)
2011-07-07 18:09 PDT, Pratik Solanki
ddkilzer: 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-07 16:16:42 PDT
Same as <https://bugs.webkit.org/show_bug.cgi?id=56838> but for the CFNetwork side.
Comment 1 Pratik Solanki 2011-07-07 16:46:51 PDT
Created attachment 100049 [details]
Patch
Comment 2 Pratik Solanki 2011-07-07 18:09:38 PDT
Created attachment 100056 [details]
Patch.v2

Updated patch that should apply cleanly. It includes some of the header file changes from patch in bug 63674.
Comment 3 Jessie Berlin 2011-07-11 11:49:29 PDT
Comment on attachment 100056 [details]
Patch.v2

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

Unofficial r=me!

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:449
> +    CFURLConnectionClient_V6 client = { 6, this, 0, 0, 0, WebCore::willSendRequest, didReceiveResponse, didReceiveData, 0, didFinishLoading, didFail, willCacheResponse, didReceiveChallenge, didSendBodyData, shouldUseCredentialStorageCallback, 0, 0, 0, didReceiveDataArray};

Is there a reason to name this "V6"? Are there versions 4 and 5 somewhere?

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:877
> +        client()->didReceiveDataArray(this, dataArray);

I know you are just moving this code, but instead of the extra nesting of and if { ... } else { ... } clause here, it would be better to just do an early return:

if (client()->supportsDataArray()) {
    client()->didReceiveDataArray(this, dataArray);
    return;
}

CFIndex count = CFArrayGetCount(....

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:885
> +        } else {

Ditto about an early return being better here.
Comment 4 Pratik Solanki 2011-07-11 11:55:01 PDT
Comment on attachment 100056 [details]
Patch.v2

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

>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:449
>> +    CFURLConnectionClient_V6 client = { 6, this, 0, 0, 0, WebCore::willSendRequest, didReceiveResponse, didReceiveData, 0, didFinishLoading, didFail, willCacheResponse, didReceiveChallenge, didSendBodyData, shouldUseCredentialStorageCallback, 0, 0, 0, didReceiveDataArray};
> 
> Is there a reason to name this "V6"? Are there versions 4 and 5 somewhere?

Yes. There's CFURLConnectionClient_V4 and CFURLConnectionClient_V5 in CFURLConnection.h.

>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:877
>> +        client()->didReceiveDataArray(this, dataArray);
> 
> I know you are just moving this code, but instead of the extra nesting of and if { ... } else { ... } clause here, it would be better to just do an early return:
> 
> if (client()->supportsDataArray()) {
>     client()->didReceiveDataArray(this, dataArray);
>     return;
> }
> 
> CFIndex count = CFArrayGetCount(....

Ok. I can do that.

>> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:885
>> +        } else {
> 
> Ditto about an early return being better here.

Ok.
Comment 5 David Kilzer (:ddkilzer) 2011-07-11 13:02:10 PDT
Comment on attachment 100056 [details]
Patch.v2

r=me with jessieberlin's comments addressed.
Comment 6 Pratik Solanki 2011-07-12 11:47:34 PDT
Committed r90834: <http://trac.webkit.org/changeset/90834>