Bug 14651 - [CURL] didReceiveResponse() only called for HTTP loads
Summary: [CURL] didReceiveResponse() only called for HTTP loads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Curl, Gtk, Wx
: 14583 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-07-17 19:10 PDT by Kevin Ollivier
Modified: 2007-12-08 16:36 PST (History)
1 user (show)

See Also:


Attachments
Patch to call didReceiveResponse and set the URL for local files (1.63 KB, patch)
2007-12-07 17:42 PST, Kevin Ollivier
alp: review-
Details | Formatted Diff | Diff
New attempt that checks a bool value (3.14 KB, patch)
2007-12-08 15:41 PST, Kevin Ollivier
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Ollivier 2007-07-17 19:10:50 PDT
The current implementation of the CURL loader calls didReceiveResponse only when receiving HTTP headers. This doesn't handle URLs using other protocols, such as file://. This causes problems with loading css files from disk because WebCore/loader/CachedCSSStyleSheet.cpp depends on m_response being set to determine the absolute URL of the css file.

I don't have a patch yet, as I'm not quite sure where the best place to fire didReceiveResponse would be for other protocols. Ideally, it would be from the first successful curl_multi_perform call for each handle, but I'm not sure if there's currently a way to track that.

Anyone more familiar with the CURL loader have any suggestions on how best to implement this?
Comment 1 Kevin Ollivier 2007-12-07 17:42:49 PST
Created attachment 17785 [details]
Patch to call didReceiveResponse and set the URL for local files
Comment 2 Alp Toker 2007-12-08 06:48:10 PST
*** Bug 14583 has been marked as a duplicate of this bug. ***
Comment 3 Alp Toker 2007-12-08 07:08:12 PST
Comment on attachment 17785 [details]
Patch to call didReceiveResponse and set the URL for local files

This bug fix works and is much needed.

I'm concerned about one thing in the implementation though:

writeCallback() is a hot path. We should first research if we can call didReceiveResponse() earlier than writeCallback(). If this isn't an option, the didReceiveResponse() call in writeCallback() needs to be made as lightweight as possible -- perhaps a boolean check, rather than querying CURL for the effective URL, constructing a KURL and updating the response again and again. It only needs to be done once, right?

Think you could devise a lighter solution?
Comment 4 Kevin Ollivier 2007-12-08 15:41:45 PST
Created attachment 17792 [details]
New attempt that checks a bool value

I've updated the implementation to check a bool value before running any of the local files code. It now no longer runs multiple times during one resource load, nor will it call CURL APIs, etc. unless necessary.

I added a TODO in the comments about finding a better approach, but so far I've been unable to find anything in the CURL API that would suggest a better solution, and the only other way I can see to approach this would be to avoid CURL entirely for local files, which I think is just going to add to the number of code paths we need to support. I'd really like to take this route for now so that we can get the fix in there, as I know it is biting existing wx port users at least.
Comment 5 Alp Toker 2007-12-08 16:06:39 PST
Comment on attachment 17792 [details]
New attempt that checks a bool value

r=me
Comment 6 Alp Toker 2007-12-08 16:36:21 PST
Landed in r28569.

I removed this extraneous line before landing:
  KURL url(hdr);

(qmake-based ports may need to rebuild since the ResourceRequest.h change leaves the build in an inconsistent/crashy state.)