[webkit-reviews] review denied: [Bug 19079] Send the submissions character encoding in hidden _charset_ field : [Attachment 113138] proposed patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>'s request for review:
Bug 19079: Send the submissions character encoding in hidden _charset_ field
https://bugs.webkit.org/show_bug.cgi?id=19079

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113138&action=review


> Source/WebCore/loader/FormSubmission.cpp:188
> +	       if (equalIgnoringCase(input->name(), "_charset_") &&
input->isInputTypeHidden())
> +		       input->setValueForUser(domFormData->encoding().name());

* Wrong indentation
* According to the specification, we should set the character encoding only if
the hidden input has no value attribute.
* I don't think we should update HTMLInputELement::value.  How does IE work?
* It's dangerous to use setValueForUser(), which dispatches an event and
execute JavaScript code, which might update the associated element list.

> LayoutTests/ChangeLog:14
> +	   *
http/tests/misc/char_encoding_in_hidden_charset_field_01-expected.txt: Added.
> +	   * http/tests/misc/char_encoding_in_hidden_charset_field_01.html:
Added. For Default Encoding.

We usually use '-' to concatenate words, not '_'.

Please assign meaningful names instead of numbers.  e.g.
char_encoding_in_hidden_charset_field_02.html should be
char-encoding-in-hidden-charset-field-with-accept-charset.html

We should have tests for Chinese/Japanese encodings such as Big5, Shift_JIS,
EUC-JP, ISO-2022-JP.  This feature is important in these countries.