|Summary:||[Qt] Allow setting HTTP headers with empty value in XMLHTTPRequest|
|Severity:||Normal||CC:||commit-queue, eric, hausmann, kenneth, laszlo.gombos, thiago.macieira, tonikitoo, vestbo|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Yael 2009-11-04 13:24:23 PST
The following script fragment should add a request header to xhr, but in Qt it does not xhr.setRequestHeader("Custom", ""); The reason is that a NULL value causes QNetworkRequest::setRawHeader to remove the header, instead of adding it.
Comment 1 Yael 2009-11-04 13:57:05 PST
Created attachment 42518 [details] Patch This patch converts the header value from null string to empty string before passing the header to QtNetwork. QtNetwork would add headers with empty strings, but not with null strings.
Comment 2 Simon Hausmann 2009-11-05 08:12:41 PST
Hm, we should talk to Thiago about this. Clearly calling setRawHeader() with a null value is currently the only way to _remove_ a header field from a request. But I think it would be cleaner if QNetworkRequest had a removeRawHeader() function and would allow empty header values. Your use-case is a good example of why supporting it would make sense, and therefore it would be nice to not have to work around Qt's behaviour in WebKit if possible. But yes, the last resort is your patch, which looks fine otherwise :)
Comment 3 Yael 2009-11-05 08:29:17 PST
(In reply to comment #2) > Hm, we should talk to Thiago about this. Clearly calling setRawHeader() with a > null value is currently the only way to _remove_ a header field from a request. > But I think it would be cleaner if QNetworkRequest had a removeRawHeader() > function and would allow empty header values. > > Your use-case is a good example of why supporting it would make sense, and > therefore it would be nice to not have to work around Qt's behaviour in WebKit > if possible. > > But yes, the last resort is your patch, which looks fine otherwise :) Simon, I completely agree with you that this is a workaround for a QtNetwork issue. However, if we change QtNetwork, we will break its backwards compatibility, and I did not think Thiago will agree to that.
Comment 4 Antonio Gomes 2009-11-05 09:24:49 PST
thiago, they need you (see comment :)
Comment 5 Eric Seidel (no email) 2009-11-05 15:31:20 PST
If this is a workaround for a QTNetwork issue, shouldn't it have a link to the QtNetwork bug in a comment?
Comment 6 Holger Freyther 2009-11-07 00:27:54 PST
Comment on attachment 42518 [details] Patch I agree with Eric that we at least need some documentation here, and it would be nice if you could come up with a simple http test. E.g. I think we already have a perl script that will return the http header... we should have a dump as text test for that.
Comment 7 Thiago Macieira 2009-11-09 02:28:26 PST
setSomething with a null value is the way to remove that something. This pattern appears in multiple places in Qt. If you need an empty value, please use an empty-but-not-null QString. So attachment 42518 [details] looks correct for me.
Comment 8 Kenneth Rohde Christiansen 2009-11-09 05:02:17 PST
Would be nice to add a comment above the change as well. Something along the lines of what Thiago said.
Comment 9 Yael 2009-11-09 12:34:02 PST
Created attachment 42782 [details] Patch Added regression test and a comment for this patch.
Comment 10 WebKit Commit Bot 2009-11-09 12:46:18 PST
Comment on attachment 42782 [details] Patch Clearing flags on attachment: 42782 Committed r50679: <http://trac.webkit.org/changeset/50679>
Comment 11 WebKit Commit Bot 2009-11-09 12:46:28 PST
All reviewed patches have been landed. Closing bug.