Bug 14059 - [CURL] Does not call didReceiveResponse()
Summary: [CURL] Does not call didReceiveResponse()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 5954 14061
Blocks: 14058
  Show dependency treegraph
 
Reported: 2007-06-10 01:00 PDT by Alp Toker
Modified: 2007-07-17 13:55 PDT (History)
1 user (show)

See Also:


Attachments
Parse header data and call didReceiveResponse() when appropriate (2.83 KB, patch)
2007-06-10 01:25 PDT, Alp Toker
mjs: review-
Details | Formatted Diff | Diff
CURL rework header handling (17.20 KB, patch)
2007-07-05 01:22 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
CURL rework headerCallback (11.06 KB, patch)
2007-07-10 17:35 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
CURL rework headerCallback (11.09 KB, patch)
2007-07-11 11:57 PDT, Holger Freyther
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2007-06-10 01:00:59 PDT
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.
Comment 1 Alp Toker 2007-06-10 01:25:57 PDT
Created attachment 14924 [details]
Parse header data and call didReceiveResponse() when appropriate
Comment 2 Holger Freyther 2007-06-13 08:05:21 PDT
+  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?


Comment 3 Alp Toker 2007-06-13 11:24:43 PDT
Note: I've had second thoughts about strtok_r and plan to update this patch to replace it with more conventional/portable header parsing.
Comment 4 Maciej Stachowiak 2007-06-26 00:22:10 PDT
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.
Comment 5 Alp Toker 2007-06-26 07:03:42 PDT
(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.
Comment 6 Holger Freyther 2007-06-30 18:27:27 PDT
I'm going to rediff now and parse all headers.
Comment 7 Holger Freyther 2007-07-05 01:22:45 PDT
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(-)
Comment 8 Holger Freyther 2007-07-05 09:54:27 PDT
(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.
Comment 9 Holger Freyther 2007-07-10 17:35:40 PDT
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.
Comment 10 Holger Freyther 2007-07-11 11:57:30 PDT
Created attachment 15477 [details]
CURL rework headerCallback

Catch up with the renaming of the parse functions.
Comment 11 Nikolas Zimmermann 2007-07-16 02:51:44 PDT
(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
Comment 12 Holger Freyther 2007-07-16 10:32:43 PDT
(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.
Comment 13 Kevin Ollivier 2007-07-16 21:11:16 PDT
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
Comment 14 Darin Adler 2007-07-16 22:22:37 PDT
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.
Comment 15 Holger Freyther 2007-07-17 01:49:35 PDT
(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.

Comment 16 Holger Freyther 2007-07-17 01:53:32 PDT
(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. 

Comment 17 Maciej Stachowiak 2007-07-17 01:55:12 PDT
Comment on attachment 15477 [details]
CURL rework headerCallback

r=me
Comment 18 Holger Freyther 2007-07-17 13:55:11 PDT
Landed in r24372 and a new bug regarding m_isNull = false in the setters of ResourceResponse will be filed.