You need to
before you can comment on or make changes to this bug.
Think this is a easy to fix regression – from the latest request method fixes :-)
When using req.open("post",url,true) (with lowercase "post" instead of uppercase "POST" the request method is sent as lowercase "post".
Of course there is no such thing as a lowercase http POST request and at least PHP doesn't understand it.
Safari 2.0.3 (417.9.2) translates req.open("post",url,true) into a uppercase POST.
I noticed this because Prototype and Scriptaculous sometimes calls open() with lowercase "post".
I know you SHOULD use uppercase when calling open() but it seems not everyone does it :-( and Prototype gets used by many people – so I think its a good idea to translate post into POST
Note: haven't checked GET, PUT etc.
Created an attachment (id=7447) [details]
Firefox does uppercase the method.
(From update of attachment 7447 [details])
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?
Created an attachment (id=7458) [details]
> 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.
(From update of attachment 7458 [details])
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.