Summary: | [BlackBerry] We shouldn't set empty Content-Length & Content-Type to request's headers. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Liu <jasonliuwebkit> | ||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | charles.wei, leo.yang, rwlbuis, staikos, tonikitoo, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | Other | ||||||||
Attachments: |
|
Description
Jason Liu
2012-04-05 20:56:05 PDT
Created attachment 135979 [details]
Patch
Comment on attachment 135979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135979&action=review > Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:210 > + m_httpHeaderFields.remove("Content-Length"); > + > + if (url().protocolInHTTPFamily()) > + m_platformRequestUpdated = false; Just ASSERT(url().protocolInHTTPFamily()) instead of *if* branch? > Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:220 > + m_httpHeaderFields.remove("Content-Type"); > + > + if (url().protocolInHTTPFamily()) > + m_platformRequestUpdated = false; Ditto. (In reply to comment #2) > (From update of attachment 135979 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135979&action=review > > > Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:210 > > + m_httpHeaderFields.remove("Content-Length"); > > + > > + if (url().protocolInHTTPFamily()) > > + m_platformRequestUpdated = false; > > Just ASSERT(url().protocolInHTTPFamily()) instead of *if* branch? > > > Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:220 > > + m_httpHeaderFields.remove("Content-Type"); > > + > > + if (url().protocolInHTTPFamily()) > > + m_platformRequestUpdated = false; > > Ditto. Why don't we do as clearHTTPReferrer? If we remove this *if* branch, "updateResourceRequest();" can be moved, too. r? Comment on attachment 135979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135979&action=review OK, let's keep consistent with clearHTTPReferrer. Looks good to me. >>> Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:210 >>> + m_httpHeaderFields.remove("Content-Length"); >>> + >>> + if (url().protocolInHTTPFamily()) >>> + m_platformRequestUpdated = false; >> >> Just ASSERT(url().protocolInHTTPFamily()) instead of *if* branch? > > Why don't we do as clearHTTPReferrer? > If we remove this *if* branch, "updateResourceRequest();" can be moved, too. Just ASSERT(url().protocolInHTTPFamily()) instead of *if* branch? >> Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:220 >> + m_httpHeaderFields.remove("Content-Type"); >> + >> + if (url().protocolInHTTPFamily()) >> + m_platformRequestUpdated = false; > > Ditto. Ditto. Comment on attachment 135979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135979&action=review I agree with Leo's comments. > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:587 > if ((method != "GET") && (method != "HEAD")) { There are extra parentheses here Created attachment 136830 [details]
Patch
(In reply to comment #6) > (From update of attachment 135979 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135979&action=review > > I agree with Leo's comments. These two functions are written according to ResourceRequestBase::clearHTTPReferrer(). And Leo's comment #5 has approved. > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:587 > > if ((method != "GET") && (method != "HEAD")) { > > There are extra parentheses here I amended this issue in the new patch. Comment on attachment 136830 [details] Patch Clearing flags on attachment: 136830 Committed r113950: <http://trac.webkit.org/changeset/113950> All reviewed patches have been landed. Closing bug. |