Bug 15356

Created attachment 16516 [details] First patch The patch corrects the behavior of the two methods.

+ if (!isValidHeaderValue(name)) { It seems that a new isValidHeaderName() function is needed here - header name and value productions are different. Eric has made some comments on IRC, too.

>+ if (!isValidHeaderValue(name)) { > It seems that a new isValidHeaderName() function is needed here - header name > and value productions are different. Eric has made some comments on IRC, too. I have mistaken the function to call. I should have called isValidToken as it is the case in setResponseHeader (according to the RFC 2616 header-name is a token). Thanks for pointing the error. Concerning the tests suggested by Eric, I have tested with the latest Firefox and it seems that it does not support all the exception added by the patch. I will test on IE7 tomorrow once I can put my hand on it and update the results here.

Comment on attachment 16516 [details] First patch Throwing exceptions in these cases can easily cause a compatibility regression with web pages or widgets that are doing it wrong and "getting away with it". But I think these calls are rarely enough used that it's not going to be a problem. r=me for the feature branch

Comment on attachment 16516 [details] First patch Actually marking r- because of the isValidHeaderValue issue (I should have done that right away).

(In reply to comment #5) > Actually marking r- because of the isValidHeaderValue issue (I should have done > that right away). Good catch. Sorry!

Created attachment 16596 [details] Updated patch Concerning Darin's comment about regression, I did some testing (as I told you I would) and updated the patch and the tests according to the results. For INVALID_STATE_ERR : IE7 : raise an exception when readyState < 4 FF 3 : has a buggy behaviour that depends on the version I have used but i does raise an exception at some point before readyState 3. Opera : raise an exception when readyState < 3 Here raising an exception when readyState < 3 seems the right behaviour and I have not changed that part. For SYNTAX_ERROR : IE 7 & Opera : returns an empty string Firefox : returns null I sided with Opera and IE and returned an empty string if the requested header is not valid.

Comment on attachment 16596 [details] Updated patch + // Should raise a SYNTAX_ERR exception + // but Opera and IE returns an empty string, so we just return an empty string We should get this fixed in the XMLHttpRequest specification if browsers really can't follow it and be compatible. Is there really a need to add the isValidToken check? What's the behavior if we leave it out? Do any of the tests you wrote fail if you leave out the isValidToken check? + //used to call getResponseHeader but could raise unnecessary exception I don't think that's a helpful comment. Please just leave it out. + String getAllResponseHeaders(ExceptionCode& ec) const; + String getResponseHeader(const String& name, ExceptionCode& ec) const; The argument doesn't need a name in the header (see the setRequestHeader above for an example of how we write these). review- mainly because of the isValidToken question

> Is there really a need to add the isValidToken check? What's the behavior if we > leave it out? Do any of the tests you wrote fail if you leave out the > isValidToken check? Without isValidToken we return null as does Firefox. A side effect is to make the test case fail but it was designed to fail if something else that an empty string is returned so it is not relevant. So basically we have the choice to side with Firefox (return null) or Opera/IE (return an empty string).

(In reply to comment #8) > + // Should raise a SYNTAX_ERR exception > + // but Opera and IE returns an empty string, so we just return an > empty string > > We should get this fixed in the XMLHttpRequest specification if browsers really > can't follow it and be compatible. Cc'ing Anne.

Ok, so instead of throwing a SYNTAX_ERR for incorrect header names the empty string would be returned. I guess this is ok, although that is also returned for valid header names that are empty. If you could e-mail the public-webapi list that you'd like getResponseHeader() header changed in that way that would be great.

> If you could e-mail the public-webapi list that you'd like getResponseHeader() > header changed in that way that would be great. Just send an email requesting the modification. Updated patch will follow

Created attachment 16688 [details] Patch updated with Darin's comments and current discussion

Comment on attachment 16688 [details] Patch updated with Darin's comments and current discussion r=me (hope Alexey doesn't notice something I missed this time!)

Landed in r26986. Be wary of tabs, your new test cases contained a few.

I also had to land new test results in r26989 as the results included in the patch didn't match what the test was expected to generate.

I opened a new bug (29140) related to this issue. The issue is a little different than the one in the title of this bug, but it touches on issues discussed in the comments here. https://bugs.webkit.org/show_bug.cgi?id=29140

By the way, the spec says to return null in case the production does not match so you might want to change the code.