Bug 83349 - [BlackBerry] We shouldn't set empty Content-Length & Content-Type to request's headers.
Summary: [BlackBerry] We shouldn't set empty Content-Length & Content-Type to request...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-05 20:56 PDT by Jason Liu
Modified: 2012-04-12 01:37 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.71 KB, patch)
2012-04-05 21:26 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (3.86 KB, patch)
2012-04-11 23:13 PDT, Jason Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Liu 2012-04-05 20:56:05 PDT
When there is a redirection for a POST request, we change POST to GET and make Content-Length/Content-Type to empty string.
We should remove these headers instead of adding empty strings.
Comment 1 Jason Liu 2012-04-05 21:26:43 PDT
Created attachment 135979 [details]
Patch
Comment 2 Leo Yang 2012-04-06 00:54:41 PDT
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.
Comment 3 Jason Liu 2012-04-06 01:20:38 PDT
(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.
Comment 4 Jason Liu 2012-04-09 02:06:57 PDT
r?
Comment 5 Leo Yang 2012-04-09 19:16:21 PDT
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 6 George Staikos 2012-04-11 22:52:43 PDT
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
Comment 7 Jason Liu 2012-04-11 23:13:44 PDT
Created attachment 136830 [details]
Patch
Comment 8 Jason Liu 2012-04-11 23:19:46 PDT
(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 9 WebKit Review Bot 2012-04-12 01:37:43 PDT
Comment on attachment 136830 [details]
Patch

Clearing flags on attachment: 136830

Committed r113950: <http://trac.webkit.org/changeset/113950>
Comment 10 WebKit Review Bot 2012-04-12 01:37:48 PDT
All reviewed patches have been landed.  Closing bug.