Bug 14898 – XMLHttpRequest.getAllResponseHeaders should separate headers with CRLF

• Component: XML
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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'.

[Julien Chaffraix]

Created attachment 16298 [details]
proposed patch.

[Oliver Hunt]

Comment on attachment 16298 [details]
proposed patch.

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

[Geoffrey Garen]

Please include a regression test with your code change.

[Mark Rowe (bdash)]

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.

[Julien Chaffraix]

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).

[Adam Roben (:aroben)]

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).

[Julien Chaffraix]

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).

[Eric Seidel (no email)]

Comment on attachment 16470 [details]
Updated patch

This looks great to me.  I'll run the tests and land.

[Eric Seidel (no email)]

landed as r25776 on feature-branch.

Thanks for the patch!