Bug 83349 – [BlackBerry] We shouldn't set empty Content-Length & Content-Type to request's headers.

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jason Liu]

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.

[Jason Liu]

Created attachment 135979 [details]
Patch

[Leo Yang]

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.

[Jason Liu]

(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.

[Jason Liu]

r?

[Leo Yang]

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.

[George Staikos]

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

[Jason Liu]

Created attachment 136830 [details]
Patch

[Jason Liu]

(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.

[WebKit Review Bot]

Comment on attachment 136830 [details]
Patch

Clearing flags on attachment: 136830

Committed r113950: <http://trac.webkit.org/changeset/113950>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.