Bug 134732 - Move resource buffering from SynchronousLoaderClient to NetworkResourceLoader
Summary: Move resource buffering from SynchronousLoaderClient to NetworkResourceLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords:
Depends on:
Blocks: 134560
  Show dependency treegraph
 
Reported: 2014-07-08 10:39 PDT by Pratik Solanki
Modified: 2014-07-09 13:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.96 KB, patch)
2014-07-08 10:46 PDT, Pratik Solanki
darin: 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 2014-07-08 10:39:02 PDT
We want to buffer JS and CSS and resources as well - see bug 134560. In order to share code, we should move the buffering code SynchronousLoaderClient to NetworkResourceLoader.
Comment 1 Pratik Solanki 2014-07-08 10:46:44 PDT
Created attachment 234572 [details]
Patch
Comment 2 Darin Adler 2014-07-08 16:19:36 PDT
Comment on attachment 234572 [details]
Patch

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

> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp:104
> +    ASSERT(loader);

Assertion that an argument is non-null is a clue that the argument should be a reference rather than a pointer.

> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp:116
> +    Vector<char> responseData;
> +    SharedBuffer* buffer = loader->bufferedData();
> +    if (buffer && buffer->size())
> +        responseData.append(buffer->data(), buffer->size());
> +
> +    m_delayedReply->send(m_error, m_response, responseData);

It’s annoying to have to put this into a Vector. What does send do with the Vector? Can we somehow avoid the copy by using either move or getting the bits directly from the SharedBuffer.

> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.h:63
> +    void sendDelayedReply(NetworkResourceLoader*);

This should take a reference rather than a pointer.
Comment 3 Pratik Solanki 2014-07-08 17:34:36 PDT
Comment on attachment 234572 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp:104
>> +    ASSERT(loader);
> 
> Assertion that an argument is non-null is a clue that the argument should be a reference rather than a pointer.

True. I'll make this a reference.

>> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp:116
>> +    m_delayedReply->send(m_error, m_response, responseData);
> 
> It’s annoying to have to put this into a Vector. What does send do with the Vector? Can we somehow avoid the copy by using either move or getting the bits directly from the SharedBuffer.

I didn't like this as well but it didn't seem much worse than we had before. send() seems to be generated code and it looks like it just passes the Vector down to the encoder. I think we should be able to modify it to take SharedBuffer but it's not straightforward. I'll ask Anders.

>> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.h:63
>> +    void sendDelayedReply(NetworkResourceLoader*);
> 
> This should take a reference rather than a pointer.

Ok.
Comment 4 Pratik Solanki 2014-07-09 13:56:16 PDT
Committed r170928: <http://trac.webkit.org/changeset/170928>