Summary: | REGRESSION: XMLHttpRequest lowercase post requests broken | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonas Munk <jonasmunk> | ||||||
Component: | DOM | Assignee: | Alexey Proskuryakov <ap> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez | ||||||
Priority: | P1 | Keywords: | Regression | ||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
Attachments: |
|
Description
Jonas Munk
2006-03-31 01:28:21 PST
Created attachment 7447 [details]
proposed fix
Firefox does uppercase the method.
Comment on attachment 7447 [details]
proposed fix
Whose job should it be to do this uppercasing? Should this be in TransferJob so we don't have to do it two different places in XMLHttpRequest?
Can m_method be changed into a String instead of a DeprecatedString so that we can do comparisons on it using equalIgnoringCase instead of constantly calling lower() on it?
Anyway, r=me.
Created attachment 7458 [details] revised fix > Whose job should it be to do this uppercasing? Should this be in TransferJob so > we don't have to do it two different places in XMLHttpRequest? "Get" or "put" are valid methods, different from "GET" and "PUT", so TransferJob probably shouldn't change their case. However, I have now found that Firefox has more complex logic - it uppercases methods it knows about, and leaves others untouched. This is a bit weird, but since we are matching their quirk here, we should probably honor this further complication, too. This new patch uses a list of headers from Firefox (nsHttpAtomList.h). > Can m_method be changed into a String instead of a DeprecatedString so that we > can do comparisons on it using equalIgnoringCase instead of constantly calling > lower() on it? This new patch does comparisons via ==; since there are many other uses of DeprecatedString in this class, I think that they should rather be all changed is a separate cleanup patch. Comment on attachment 7458 [details]
revised fix
The number of different methods listed in XMLHttpRequest::open is great enought that we might even want to use a HashSet. If we use CaseInsensitiveHash, we won't even need to call upper! You can call find, then get the value out of the hash, something like this:
HashSet<String, CaseInsensitiveHash>::const_iterator it = knownMethods.find(method);
if (it != knownMethods.end())
m_method = *it;
The only obstacle is that currently we only have CaseInsensitiveHash defined for StringImpl*, not String or RefPtr<StringImpl*>. But since this is only a single global set, we could just ref the StringImpl* instances and then use a HashSet<StringImpl*, CaseInsensitiveHash>.
I suggest the comment:
+ // Methods names are case-sensitive, but Firefox uppercases methods it knows
be changed to something more like:
// Method names are case sensitive. But since Firefox uppercases method names it knows, we'll do the same.
The code is good as-is. r=me
Perhaps it would also be possible to use AtomicStrings in a way similar to HTMLNames - but since the HTTP request itself is so slow, I am not sure if optimizing these comparisons is necessary. Landed with a changed comment. Mass moving XML DOM bugs to the "DOM" Component. |