Bug 14898

Summary: XMLHttpRequest.getAllResponseHeaders should separate headers with CRLF
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: XMLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P3    
Version: 523.x (Safari 3)   
Hardware: All   
OS: All   
Attachments:
Description Flags
proposed patch.
mrowe: review-
Modified patches
aroben: review+
Updated patch eric: review+

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!