Bug 29140

Intend to submit patch soon.

I have read the discussion on bug 15356, and I saw that some changes were made to the spec probably because of those discussions, but I believe that WebKit is still not conforming to the spec and not only that, but due to its behavior makes it difficult for JavaScript developers to distinguish between missing and empty headers, while the spec makes that easy and I also do not believe that changing this behavior should cause major compatibility issues. If nobody dares to break with a bad legacy, progress will die, and furthermore things will become worse and worse, as mistakes will keep creeping in over the years.

That's very interesting - from my reading of the code, this doesn't seem to be the case for JS bindings at least. > If nobody dares to break with a bad legacy, progress will die This is not the kind of argument that helps move a patch forward - instead, many people will avoid bugs with politically charged discussions. Please do test what IE and Firefox return, both for blacklisted headers like Set-Cookie, and for ones that just aren't in the response.

(In reply to comment #3) > That's very interesting - from my reading of the code, this doesn't seem to be > the case for JS bindings at least. Oh sorry, my reading was incorrect :)

Created attachment 39698 [details] Proposed patch I have verified that Firefox returns null for the valid states (>=2), but does not throw an exception for the 0 state, and in some versions not for the 1 state either. I was unable to install a recent version of IE (7.0 or better) on the computers that I have access to (download restrictions and failure of internal install packages) thus I am not sure what recent IE behavior is but IE 6.0 is very far from the standard: has redefined states, headers being accessible only after the entire document is loaded, and then sometimes it returns an empty string, sometimes it throws an exception (like in the case of null).

I have checked IE and IE is inconsistent in handling XMLHttpRequest between major versions, but for the most part it returns an empty string for missing, unsafe. empty, and malformed header names. I believe that we should go with the specification, not only because it is the specification, but because it allows distinguishing a missing header from a header with an empty value in it.

Here is the response from the editor of the XMLHttpRequest Level 2 specification to the question that this bug report is trying to address: Re: XMLHttpRequest: IE compatibility: null versus empty string for requesting missing, unsafe, or malformed header names To: carol.szabo@nokia.com Cc: ap@webkit.org On Mon, Sep 21, 2009 at 6:57 PM, <carol.szabo@nokia.com> wrote: > IE 8.0 returns an empty string, except that it throws an exception if the > header name argument is null. > The standard that I read specifies null for the cases mentioned in the > subject. Is this difference intentional? > I personally prefer to have it the way the standard specifies the handling > of these cases, as it allows a distinction between a header not being > present and having an empty value, but I would like to know whether the > current state of the standard is likely to remain as is, or it is likely to > change to emulate IE, which while providing less information makes it easier > for the developers to treat missing and empty headers the same way, which is > probably ok in most cases. > > Any thoughts that you might have on this issue are greatly appreciated. The standard at one point returned the empty string. We changed it for exactly the reason you mention and I do not plan to change that. By the way, it's better to use annevk@opera.com if you want to reach me on such matters. annevankesteren+webkit@gmail.com is just for WebKit Bugzilla. Cheers, -- Anne van Kesteren http://annevankesteren.nl/

Comment on attachment 39698 [details] Proposed patch > - if (!isValidToken(name)) > - return ""; <...> > + //if name is not a "Valid token" it shall not match any valid header name thus the check for isValidToken is not needed. > + //since httpHeaderField(name) returns a null string anyway. > return m_response.httpHeaderField(name); It seems fine to remove the check - I think that we should suggest removing this clause form XMLHttpRequest spec, too. I do not think that the comment is helpful - it talks about code that used to be there, but isn't any more. Also, sentences should start with capital letters, and there should be a space after "//". I suggest just removing the comment, and e-mailing W3C WebApps working group about changing the spec. > + * http/tests/xmlhttprequest/xmlhttprequest-invalidHeader-getRequestHeader.html: Removed. > + getResponseHeader.html now covers this case as well. You should remove expected results for this test, as well.

Created attachment 39943 [details] Proposed Patch: Addresses AP's concerns.

Comment on attachment 39943 [details] Proposed Patch: Addresses AP's concerns. r=me

Comment on attachment 39943 [details] Proposed Patch: Addresses AP's concerns. Carol is not a committer, so assuming he wanted this auto-committed given no indication otherwise.

Comment on attachment 39943 [details] Proposed Patch: Addresses AP's concerns. Rejecting patch 39943 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11318 test cases. http/tests/xmlhttprequest/getResponseHeader.html -> failed Exiting early after 1 failures. 8974 tests run. 252.41s total testing time 8973 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output

The test failure looks related to your change, so this rejection is real (not a flakey test).

(In reply to comment #13) > The test failure looks related to your change, so this rejection is real (not a > flakey test). Eric, I have run this test in my environment and it succeeded. I think the output is apparently different on Mac than on Linux, I assume that there probably are extra warnings or something. Can you provide the actual output of the failed test such that I can address the issue. Thanks, Carol

Created attachment 40076 [details] Fixed a problem in the getResonseHeader-expected.txt. The test should succeed now. I fixed the expected test result for the failing test. The difference was in the output formatting.

OK. Best to let ap set r+ again since he reviewed the original patch.

Comment on attachment 40076 [details] Fixed a problem in the getResonseHeader-expected.txt. The test should succeed now. r=me

Comment on attachment 40076 [details] Fixed a problem in the getResonseHeader-expected.txt. The test should succeed now. Clearing flags on attachment: 40076 Committed r48739: <http://trac.webkit.org/changeset/48739>

All reviewed patches have been landed. Closing bug.