[webkit-reviews] review denied: [Bug 20795] text/plain form encoding ignored and incorrectly specified in request header : [Attachment 112559] updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>'s request for review:
Bug 20795: text/plain form encoding ignored and incorrectly specified in
request header
https://bugs.webkit.org/show_bug.cgi?id=20795

Attachment 112559: updated patch
https://bugs.webkit.org/attachment.cgi?id=112559&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112559&action=review


Is this correct behavior? Do other browsers do this? What websites use this
feature?

> Source/WebCore/platform/network/FormData.cpp:102
> +PassRefPtr<FormData> FormData::create(const FormDataList& list, const
TextEncoding& encoding, const EncodingType encodingType)

This should just be EncodingType, not const EncodingType.

> Source/WebCore/platform/network/FormData.cpp:183
> +void FormData::appendKeyValuePairItems(const FormDataList& list, const
TextEncoding& encoding, bool isMultiPartForm, Document* document, const
EncodingType encodingType)

No const here either.

> Source/WebCore/platform/network/FormData.h:104
> +    static PassRefPtr<FormData> create(const FormDataList&, const
TextEncoding&, const EncodingType encodingType = FormURLEncoded);

This should be:

    EncodingType = FormURLEncoded

The const is not needed or helpful, and the argument name is not helpful.

> Source/WebCore/platform/network/FormData.h:138
> +    static EncodingType getEncodingType(const String& type)

Functions like this are not named with the word “get” in WebKit. This is
covered in the coding style guide.

This should probably be named “parseEncodingType”.

> Source/WebCore/platform/network/FormData.h:143
> +		return MultipartFormData;

Extra space here.

> Source/WebCore/platform/network/FormData.h:151
> +    void appendKeyValuePairItems(const FormDataList&, const TextEncoding&,
bool isMultiPartForm, Document*, const EncodingType encodingType =
FormURLEncoded);

Same comment as above.

> Source/WebCore/platform/network/FormDataBuilder.cpp:184
> +void FormDataBuilder::addKeyValuePairAsFormData(Vector<char>& buffer, const
CString& key, const CString& value, const FormData::EncodingType encodingType)

Same comment as above.

> Source/WebCore/platform/network/FormDataBuilder.h:47
> +    static void addKeyValuePairAsFormData(Vector<char>&, const CString& key,
const CString& value, FormData::EncodingType encodingType =
FormData::FormURLEncoded);

Should omit the argument name here.