Bug 31140

Summary: [Qt] Allow setting HTTP headers with empty value in XMLHTTPRequest
Product: WebKit Reporter: Yael <yael>
Component: XMLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, hausmann, kenneth, laszlo.gombos, thiago.macieira, tonikitoo, vestbo
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 29799    
Attachments:
Description Flags
Patch
zecke: review-
Patch none

Yael
Reported 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.
Attachments
Patch (1.40 KB, patch)
2009-11-04 13:57 PST, Yael 		zecke: review-
DetailsFormatted DiffDiff
Patch (4.04 KB, patch)
2009-11-09 12:34 PST, Yael 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yael
Comment 1 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.
Simon Hausmann
Comment 2 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 :)
Yael
Comment 3 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.
Antonio Gomes
Comment 4 2009-11-05 09:24:49 PST
thiago, they need you (see comment :)
Eric Seidel (no email)
Comment 5 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?
Holger Freyther
Comment 6 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.
Thiago Macieira
Comment 7 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.
Kenneth Rohde Christiansen
Comment 8 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.
Yael
Comment 9 2009-11-09 12:34:02 PST
Created attachment 42782 [details] Patch Added regression test and a comment for this patch.
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2009-11-09 12:46:28 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.