Bug 14898 - XMLHttpRequest.getAllResponseHeaders should separate headers with CRLF
Summary: XMLHttpRequest.getAllResponseHeaders should separate headers with CRLF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-08-07 22:04 PDT by Alexey Proskuryakov
Modified: 2007-09-30 09:15 PDT (History)
0 users

See Also:


Attachments
proposed patch. (1.29 KB, patch)
2007-09-15 17:58 PDT, Julien Chaffraix
mrowe: review-
Details | Formatted Diff | Diff
Modified patches (3.14 KB, patch)
2007-09-29 19:48 PDT, Julien Chaffraix
aroben: review+
Details | Formatted Diff | Diff
Updated patch (3.20 KB, patch)
2007-09-30 05:58 PDT, Julien Chaffraix
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2007-08-07 22:04:08 PDT
From the draft spec: "Return all the HTTP headers, as a single string, with each header line separated by a U+000D CR U+000A LF pair excluding the status line."

WebKit just appends '\n'.
Comment 1 Julien Chaffraix 2007-09-15 17:58:47 PDT
Created attachment 16298 [details]
proposed patch.
Comment 2 Oliver Hunt 2007-09-17 22:11:46 PDT
Comment on attachment 16298 [details]
proposed patch.

Wouldn't UChar('\r'), UChar('\n') work just as well, and be slightly clearer?
Comment 3 Geoffrey Garen 2007-09-25 11:00:33 PDT
Please include a regression test with your code change.
Comment 4 Mark Rowe (bdash) 2007-09-28 23:04:03 PDT
Comment on attachment 16298 [details]
proposed patch.

Marking as r- due to the lack of test case.  It should be possible to create a HTTP layout test for this patch.  I think adopting Oliver's suggested change would be good too.
Comment 5 Julien Chaffraix 2007-09-29 19:48:08 PDT
Created attachment 16462 [details]
Modified patches

> Wouldn't UChar('\r'), UChar('\n') work just as well, and be slightly clearer?

Yes, totally true. I have updated that part.

> Please include a regression test with your code change.

The regression test is attached that should validate the patch. The problem is I cannot get the results on my platform due to the lack of ResourceResponse on Qt (merely getAllResponseHeaders returns an empty string).
Comment 6 Adam Roben (:aroben) 2007-09-29 23:34:32 PDT
Comment on attachment 16462 [details]
Modified patches

I wonder if this test will really work without being in the http directory? (Tests in the http directory are loaded through a local Apache server; all other tests are loaded as local files).

You probably also meant to say "crlf" in the test name instead of "crcf".

r=me for landing on feature-branch.

Whoever commits this is going to have to make sure to generate test results (and ensure they are correct, of course).
Comment 7 Julien Chaffraix 2007-09-30 05:58:25 PDT
Created attachment 16470 [details]
Updated patch

> (From update of attachment 16462 [details] [edit])
> I wonder if this test will really work without being in the http directory?
> (Tests in the http directory are loaded through a local Apache server; all
> other tests are loaded as local files).

I cannot say but I would think it would hit other bugs. Therefore, I moved the file in the http directory to be sure.

> You probably also meant to say "crlf" in the test name instead of "crcf".

Ops, I did not catch that one. Corrected in the new patch.

Thanks

> r=me for landing on feature-branch.

> Whoever commits this is going to have to make sure to generate test results
(and ensure they are correct, of course).
Comment 8 Eric Seidel (no email) 2007-09-30 08:49:13 PDT
Comment on attachment 16470 [details]
Updated patch

This looks great to me.  I'll run the tests and land.
Comment 9 Eric Seidel (no email) 2007-09-30 09:15:47 PDT
landed as r25776 on feature-branch.

Thanks for the patch!