Don't to do HTML entity decoding for multi-part form fields

Don't to do HTML entity decoding for multi-part form fields

The code was trying to do HTML entity decoding on text field values
in multipart form posts. This has been removed as it did not work.

The parsing was expecting HTML entities to always be used, but that is
not the case. Chrome, Firefox and Safari will do it if e.g. the attribute
accept-charset="latin1" is set on the HTML form element, but the default
when accept-charset is not set or is set to utf8 UTF-8 encoding will be
used.

Also when HTML entity encoding is used, the encoding done by the browser
cannot be consistently  decoded. E.g. if the value '&#<some large char>' is
entered in a form with accept-charset="latin1" the value in the
multipart/form-data response is '&#&#12402;' (<some large char> is HTML
entity encoded as &#12402;), so '&#' is not escaped in any way.

Passing the value returned by the browser into user code without trying to
be clever is the right thing to do.

R=kustermann@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=41821

[kustermann, 2014-11-18 16:34:20]

lgtm with comments

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/lib/src/http...
File pkg/http_server/lib/src/http_multipart_form_data_impl.dart (right):

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/lib/src/http...
pkg/http_server/lib/src/http_multipart_form_data_impl.dart:35: StringBuffer
buffer = new StringBuffer();
This StringBuffer is unused now, remove it?

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/lib/src/http...
pkg/http_server/lib/src/http_multipart_form_data_impl.dart:38: encoding =
Encoding.getByName(contentType.charset);
This should be only done

if (contentType != null && contentType.charset != null) {
  ...
}

right?

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
File pkg/http_server/test/http_multipart_test.dart (right):

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:14: final value;
Document that 'value' is either String or List<int> here.

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:33: }
If this one overrides operator== it should also override hashCode!

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:57: .then((b) => b.toString());
Maybe just do a

future = multipart.join('')

here?

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:60: .fold([], (b, s) =>
b..addAll(s));
future = multipart.toList()

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:76: .fold([], (l, f) => l..add(f))
Just do a '.toList()' instead ?

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:222: 116, 66, 114, 99, 106, 120,
104, 101, 75, 101, 78, 54, 105, 48, 45, 45, 13,
long line

[Søren Gjesse, 2014-11-19 07:41:06]

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/lib/src/http...
File pkg/http_server/lib/src/http_multipart_form_data_impl.dart (right):

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/lib/src/http...
pkg/http_server/lib/src/http_multipart_form_data_impl.dart:35: StringBuffer
buffer = new StringBuffer();
On 2014/11/18 16:34:20, kustermann wrote:
> This StringBuffer is unused now, remove it?

Done.

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/lib/src/http...
pkg/http_server/lib/src/http_multipart_form_data_impl.dart:38: encoding =
Encoding.getByName(contentType.charset);
On 2014/11/18 16:34:20, kustermann wrote:
> This should be only done
> 
> if (contentType != null && contentType.charset != null) {
>   ...
> }
> 
> right?

Encoding.getByName returns null if the argument is null. Changed anyway.

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
File pkg/http_server/test/http_multipart_test.dart (right):

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:14: final value;
On 2014/11/18 16:34:20, kustermann wrote:
> Document that 'value' is either String or List<int> here.

Done.

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:33: }
On 2014/11/18 16:34:20, kustermann wrote:
> If this one overrides operator== it should also override hashCode!

Oops. Just used name.hashCode.

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:57: .then((b) => b.toString());
On 2014/11/18 16:34:20, kustermann wrote:
> Maybe just do a
> 
> future = multipart.join('')
> 
> here?

Done.

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:60: .fold([], (b, s) =>
b..addAll(s));
On 2014/11/18 16:34:20, kustermann wrote:
> future = multipart.toList()

toList does not have the addAll semantics.

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:76: .fold([], (l, f) => l..add(f))
On 2014/11/18 16:34:20, kustermann wrote:
> Just do a '.toList()' instead ?

Done.

https://codereview.chromium.org/730203008/diff/1/pkg/http_server/test/http_mu...
pkg/http_server/test/http_multipart_test.dart:222: 116, 66, 114, 99, 106, 120,
104, 101, 75, 101, 78, 54, 105, 48, 45, 45, 13,
On 2014/11/18 16:34:20, kustermann wrote:
> long line 

Done.

[Søren Gjesse, 2014-11-19 07:41:30]

Committed patchset #2 (id:20001) manually as 41821 (presubmit successful).