Bug 14059 – [CURL] Does not call didReceiveResponse()

• Component: New Bugs
• Status: RESOLVED FIXED

[Alp Toker]

The curl network backend should fire off didReceiveResponse() when a response is received. This is important for providing the Content-Type and other information for further processing down the line.

[Alp Toker]

Created attachment 14924 [details]
Parse header data and call didReceiveResponse() when appropriate

[Holger Freyther]

+  if (strcmp("\r\n", ptr))
+        return totalSize;

A comment describing why this guard is the right one would be awesome. We wen through this on irc and I agree that it is sufficient.




+    KURL url(hdr);
+    if (hdr) {

a) could you move KURL url handling further down? It is only used at the last line of this method and it would increase readability
b) Is it documented how KURL(const char*) is handling possible NULL strings? Or is it only possible that the second call where hdr is used returns a NULL string?
c) Would it be too much work to not reuse the char* hdr but have a variable name describing this thing better?


+        char* buf = strdup(hdr);
I think we don't care for the case where this fails?

[Alp Toker]

Note: I've had second thoughts about strtok_r and plan to update this patch to replace it with more conventional/portable header parsing.

[Maciej Stachowiak]

Comment on attachment 14924 [details]
Parse header data and call didReceiveResponse() when appropriate

Good improvement, but while you're at it, how about parsing the response headers too? Also, the curl docs imply that the callback you are changing gets called once per received header line, seems like that would be bad. According to libcurl docs:

"This function gets called by libcurl as soon as it has received header data. The header callback will be called once for each header and only complete header lines are passed on to the callback."

r- to revise in light of these comments.

[Alp Toker]

(In reply to comment #4)
> (From update of attachment 14924 [details] [edit])
> Good improvement, but while you're at it, how about parsing the response
> headers too? Also, the curl docs imply that the callback you are changing gets
> called once per received header line, seems like that would be bad. According
> to libcurl docs:
> 
> "This function gets called by libcurl as soon as it has received header data.
> The header callback will be called once for each header and only complete
> header lines are passed on to the callback."

    if (strcmp("\r\n", ptr))
        return totalSize;

ie. the code that follows will only be run once, after the final header is processed, so your concern is not valid but it's true the code could be less obscurely written/commented.

Parsing the other response headers sounds like an interesting idea. I'll see how it's meant to be done from the other http backends.

> 
> r- to revise in light of these comments.
> 

I'll get round to adding the comment and response header parsing in a few days, but it's worth noting that the patch works as-is and unblocks a series of other patches/bugs all the way up to making SVG support work.

[Holger Freyther]

I'm going to rediff now and parse all headers.

[Holger Freyther]

Created attachment 15396 [details]
CURL rework header handling

The question I have is:
   Should the parsing of Content-Type, Content-Dispo... go into a CURL specific ResourceResponse::doUpdateResourceResponse?


 ChangeLog                                       |   99 ++++++++++++++++++++++++
 platform/network/HTTPParsers.cpp                |   48 +++++++++++
 platform/network/HTTPParsers.h                  |    2 
 platform/network/ResourceHandleInternal.h       |    4 
 platform/network/ResourceResponse.cpp           |    7 +
 platform/network/ResourceResponse.h             |    1 
 platform/network/cf/ResourceResponseCFNet.cpp   |   29 -------
 platform/network/curl/ResourceHandleCurl.cpp    |    2 
 platform/network/curl/ResourceHandleManager.cpp |   77 +++++++++++++++++-
 platform/network/curl/ResourceHandleManager.h   |    7 -
 10 files changed, 235 insertions(+), 41 deletions(-)

[Holger Freyther]

(In reply to comment #7)
ap's Comments from IRC on the HTTPParser changes:
    -Using split(';') is bad for two reasons. It will not honor quotations and is supposed to be slow. This affects my parsing method for the Content-Type and the one moved from network/cf/ResourceResponseCFNet.cpp
    -There are two places parsing the Content-Type already. This is http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/xml/xmlhttprequest.cpp#L90 and TextResourceLoader.

Also the change of the CURL timeout was not documented in the ChangeLog.

So the next steps of the patch regarding will be to remove the split from the code and follow the xhtmlhttprequest examples and move one Content-Type parsing method into HTTPParsers.

[Holger Freyther]

Created attachment 15470 [details]
CURL rework headerCallback

Simplified headerCallback on the cost of adding more setters to ResourceResponse. The added setters set m_isNull to false to work for the CURL backend.

This patch requires the patch for HTTPParsers from the bugreport related to this one.

[Holger Freyther]

Created attachment 15477 [details]
CURL rework headerCallback

Catch up with the renaming of the parse functions.

[Nikolas Zimmermann]

(In reply to comment #10)
> Created an attachment (id=15477) [edit]
> CURL rework headerCallback
> 
> Catch up with the renaming of the parse functions.

Any news on this patch after talking to Maciej?
If all issues are resolved, I'd like to r+ it.

Greetings,
Niko

[Holger Freyther]

(In reply to comment #11)
> (In reply to comment #10)

> Any news on this patch after talking to Maciej?
> If all issues are resolved, I'd like to r+ it.

Not yet. I will try to ask Maciej to take another look.

[Kevin Ollivier]

I've just tried this patch on the wx port, and it does fire didReceiveResponse as expected. However, it only sends it for HTTP downloads. Page loads from disk (e.g. file:// URLs) won't get a didReceiveResponse message. I'm not sure how best to resolve this though, as CURL does not seem to have a way to send notification when a connection is established. 

Ideas? 

Thanks,

Kevin

[Darin Adler]

Comment on attachment 15477 [details]
CURL rework headerCallback

Instead of adding all those setters to ResourceResponse, I think you should use the constructor that takes all of the values as parameters.

    d->m_response = ResourceResponse(x, x, x, x, x);

I suppose there's no major harm in adding the setters, but there's no major benefit either.

[Holger Freyther]

(In reply to comment #14)
> (From update of attachment 15477 [details] [edit])
> Instead of adding all those setters to ResourceResponse, I think you should use
> the constructor that takes all of the values as parameters.
> 
>     d->m_response = ResourceResponse(x, x, x, x, x);
> 
> I suppose there's no major harm in adding the setters, but there's no major
> benefit either.

Well, in this case I should have a HTTPHeaderMap in my ResourceHandleInternal as well and either add a setHTTPHeaderMap to ResourceResponse (like in a previous version of this patch), but if you compare the two versions I think the current one (requring more setters) is better to read.

[Holger Freyther]

(In reply to comment #15)
> the current one (requring more setters) is better to read.
read that as slighly better to read. Check http://bugs.webkit.org/attachment.cgi?id=15396 (ignore the hunks that went into 5954) and compare with the latest one. I'm happy to get review of the two approaches. 

[Maciej Stachowiak]

Comment on attachment 15477 [details]
CURL rework headerCallback

r=me

[Holger Freyther]

Landed in r24372 and a new bug regarding m_isNull = false in the setters of ResourceResponse will be filed.