Bug 68928 – CachedResourceRequest should support didReceiveDataArray

• Status: RESOLVED
• Resolution: INVALID
• Component: Platform

[Pratik Solanki]

Currently, we loop around in SubresourceLoader calling didReceiveData for each CFData that we have. This can lead to CachedResourceRequest being called multiple times which, for images, can lead to multiple calls to BitmapImage::setData(). This is unnecessary and we can fix it by having CachedResourceRequest implement a didReceiveDataArray() method.

[Pratik Solanki]

Created attachment 108891 [details]
Patch

[Darin Adler]

Comment on attachment 108891 [details]
Patch

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

> Source/WebCore/loader/cf/SubresourceLoaderCF.cpp:47
> +        if (m_client && m_client->supportsDataArray()) {
> +            m_client->didReceiveDataArray(this, dataArray);
> +            return;
> +        }

Why not just construct and pass a SharedBuffer here?

[Pratik Solanki]

(In reply to comment #2)
> (From update of attachment 108891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108891&action=review
> 
> > Source/WebCore/loader/cf/SubresourceLoaderCF.cpp:47
> > +        if (m_client && m_client->supportsDataArray()) {
> > +            m_client->didReceiveDataArray(this, dataArray);
> > +            return;
> > +        }
> 
> Why not just construct and pass a SharedBuffer here?

That won't really buy us anything. didReceiveDataArray() does

    if (m_multipart) {
        // The loader delivers the data in a multipart section all at once, send eof.
        // The resource data will change as the next part is loaded, so we need to make a copy.
        RefPtr<SharedBuffer> copiedData = SharedBuffer::create(dataArray);
        m_resource->data(copiedData.release(), true);
    } else if (m_incremental)
        m_resource->data(loader->resourceData(), false);

Moving the creation of SharedBuffer to SubresourceLoader would simply result in an extra SharedBuffer object being created for the !m_multipart case. Is there any benefit to creating SharedBuffer?

[Sam Weinig]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 108891 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=108891&action=review
> > 
> > > Source/WebCore/loader/cf/SubresourceLoaderCF.cpp:47
> > > +        if (m_client && m_client->supportsDataArray()) {
> > > +            m_client->didReceiveDataArray(this, dataArray);
> > > +            return;
> > > +        }
> > 
> > Why not just construct and pass a SharedBuffer here?
> 
> That won't really buy us anything. didReceiveDataArray() does
> 
>     if (m_multipart) {
>         // The loader delivers the data in a multipart section all at once, send eof.
>         // The resource data will change as the next part is loaded, so we need to make a copy.
>         RefPtr<SharedBuffer> copiedData = SharedBuffer::create(dataArray);
>         m_resource->data(copiedData.release(), true);
>     } else if (m_incremental)
>         m_resource->data(loader->resourceData(), false);
> 
> Moving the creation of SharedBuffer to SubresourceLoader would simply result in an extra SharedBuffer object being created for the !m_multipart case. Is there any benefit to creating SharedBuffer?

If we move it up high enough in the stack, we can avoid having a platform specific type, CFArray, littering the code.  It would also let transition to other data types, such as dispatch_data_t when the time comes.

[Pratik Solanki]

I obsoleted the patch since the code has bitrotted and the same could probably be done a bit more efficiently.

(In reply to comment #4)
> If we move it up high enough in the stack, we can avoid having a platform specific type, CFArray, littering the code.  It would also let transition to other data types, such as dispatch_data_t when the time comes.

Where would you want to move this? How high up the stack?

[Pratik Solanki]

Much of CachedResourceRequest was merged into SubresourceLoader in bug 71149. I don't think this bug makes sense any more.