[webkit-reviews] review denied: [Bug 20795] text/plain form encoding ignored and incorrectly specified in request header : [Attachment 73453] Patch that handles enctype="text/plain" correctly

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Philip Tellis
<philip.tellis at gmail.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 73453: Patch that handles enctype="text/plain" correctly
https://bugs.webkit.org/attachment.cgi?id=73453&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73453&action=review

Please clarify what exactly this patch implements, and add test coverage.
Besides what is mentioned below, there should be a test with non-ASCII data.

> WebCore/ChangeLog:13
> +	   Implementation algorithm defined here:
> +	  
http://dev.w3.org/html5/spec/Overview.html#text-plain-encoding-algorithm

You reference the HTML5 algorithm, but is it fully implemented? We don't
implement accept-charset, and we don't implement _charset_ entries. There is no
test for sending a file input as text/plain.

> WebCore/loader/FormSubmission.cpp:168
> +	   formData =
FormData::create(*(static_cast<FormDataList*>(domFormData.get())),
domFormData->encoding(), isMailtoForm || attributes.method() == GetMethod ?
"application/x-www-form-urlencoded" : encodingType);

This doesn't seem to match what HTML5 says. GET/mailto is always
"application/x-www-form-urlencoded", but POST/mailto uses "appropriate form
encoding algorithm".

> WebCore/platform/network/FormData.h:98
> +    static PassRefPtr<FormData> create(const FormDataList&, const
TextEncoding&, const String encodingType =
"application/x-www-form-urlencoded");

The encoding type definitely shouldn't be passed as string copy. Maybe String
reference, maybe an AtomicString, or maybe an enum constant. The constant is
probably most appropriate.

> WebCore/platform/network/FormDataBuilder.cpp:190
> +	   //
http://dev.w3.org/html5/spec/Overview.html#text-plain-encoding-algorithm

I don't think that a link to an interim version of HTML5 is useful. That's
where one would look by default anyway.

> LayoutTests/http/tests/misc/form-post-textplain.html:13
> +    <input type="hidden" name="f1" value="This is field #1 &!@$%'<>">
> +    <input type="hidden" name="f2" value='This is field #2 ""'>

Interesting characters to test should include '=' and newlines.