Move MimeMultipartTransformer and HttpBodyHandler to mime and http_server packages.

Move MimeMultipartTransformer and HttpBodyHandler to mime and http_server packages.

BUG=
R=whesse@google.com

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

[Anders Johnsen, 2013-07-02 14:55:42]

I'll send this CL out to a broader audience once we agree on how this should be.

I see a potential to expose _BufferList (or some abbreviation thereof) and
HttpUtils.

[Bill Hesse, 2013-07-03 11:30:27]

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
File pkg/http_server/lib/src/http_body_impl.dart (right):

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:209: // TODO(ajohnsen): Provide in
dart:io?
Do you now mean "Provide in pkg/http_server?"  Do you mean "make this not
private"?

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:219: // the provided end.
I don't think you need this function.  You can, in the while loop, just find the
next '=' in the string using indexOf('=', start).  Then, when you have a
separator, you can just check if the = position is before it, and use it if it
is.

I also think you could even use either RegExp('[;&]').allMatches or both
indexOf(';', lastSemi) and indexOf('&', lastAmp)
to use higher-level functions.

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:273: while (!encoded && index <
urlEncoded.length) {
.indexOf(RegExp('[%+]'))?

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:357: void add(List<int> buffer) {
buffer and _buffer in the same function is confusing.

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:376: for (int i = 0; i <
bufferLength; i++) {
Why aren't we using setRange for both cases?

https://codereview.chromium.org/18438005/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/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_multipart_form_data_impl.dart:110: int end =
input.lastIndexOf(';');
or indexOf(';', amp)

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/test/http_bod...
File pkg/http_server/test/http_body_test.dart (right):

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/test/http_bod...
pkg/http_server/test/http_body_test.dart:116: expect(
expect(mimeType, anyOf(equals('multipart/form-data'),    
equals('application/x-www-form-urlencoded')));

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
File pkg/mime/test/mime_multipart_transformer_test.dart (right):

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
pkg/mime/test/mime_multipart_transformer_test.dart:30:
futures.add(partCompleter.future);
Why not
futures.add(multipart.fold( ... ).then((data) { ... }));
and drop the partCompleter.

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
pkg/mime/test/mime_multipart_transformer_test.dart:45: .then((values) =>
!values.any((b) => !b))
Is this stray code?  All the values are true by construction, and
the result is not used.

Also, values.every((b) => b)

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
pkg/mime/test/mime_multipart_transformer_test.dart:71: .then((values) =>
!values.any((b) => !b)),
All the values are true anyway.

completion(anything)?

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
pkg/mime/test/mime_multipart_transformer_test.dart:102:
"content-transfer-encoding": "base64"};
Indentation

[Anders Johnsen, 2013-07-12 10:45:57]

PTAL,

I think the cleanup is sufficient for this to land.

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
File pkg/http_server/lib/src/http_body_impl.dart (right):

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:209: // TODO(ajohnsen): Provide in
dart:io?
On 2013/07/03 11:30:27, Bill Hesse wrote:
> Do you now mean "Provide in pkg/http_server?"  Do you mean "make this not
> private"?

Removed.

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:219: // the provided end.
On 2013/07/03 11:30:27, Bill Hesse wrote:
> I don't think you need this function.  You can, in the while loop, just find
the
> next '=' in the string using indexOf('=', start).  Then, when you have a
> separator, you can just check if the = position is before it, and use it if it
> is.
> 
> I also think you could even use either RegExp('[;&]').allMatches or both
> indexOf(';', lastSemi) and indexOf('&', lastAmp)
> to use higher-level functions.
> 

This is gone.

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:273: while (!encoded && index <
urlEncoded.length) {
On 2013/07/03 11:30:27, Bill Hesse wrote:
> .indexOf(RegExp('[%+]'))?

This is gone.

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:357: void add(List<int> buffer) {
On 2013/07/03 11:30:27, Bill Hesse wrote:
> buffer and _buffer in the same function is confusing.

Also gone.

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_body_impl.dart:376: for (int i = 0; i <
bufferLength; i++) {
On 2013/07/03 11:30:27, Bill Hesse wrote:
> Why aren't we using setRange for both cases?

We are now.

https://codereview.chromium.org/18438005/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/18438005/diff/1/pkg/http_server/lib/src/http_...
pkg/http_server/lib/src/http_multipart_form_data_impl.dart:110: int end =
input.lastIndexOf(';');
On 2013/07/03 11:30:27, Bill Hesse wrote:
> or indexOf(';', amp)

This is gone.

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/test/http_bod...
File pkg/http_server/test/http_body_test.dart (right):

https://codereview.chromium.org/18438005/diff/1/pkg/http_server/test/http_bod...
pkg/http_server/test/http_body_test.dart:116: expect(
On 2013/07/03 11:30:27, Bill Hesse wrote:
> expect(mimeType, anyOf(equals('multipart/form-data'),    
> equals('application/x-www-form-urlencoded')));

Done.

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
File pkg/mime/test/mime_multipart_transformer_test.dart (right):

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
pkg/mime/test/mime_multipart_transformer_test.dart:30:
futures.add(partCompleter.future);
On 2013/07/03 11:30:27, Bill Hesse wrote:
> Why not
> futures.add(multipart.fold( ... ).then((data) { ... }));
> and drop the partCompleter.

Done.

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
pkg/mime/test/mime_multipart_transformer_test.dart:45: .then((values) =>
!values.any((b) => !b))
On 2013/07/03 11:30:27, Bill Hesse wrote:
> Is this stray code?  All the values are true by construction, and
> the result is not used.
> 
> Also, values.every((b) => b)

Done.

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
pkg/mime/test/mime_multipart_transformer_test.dart:71: .then((values) =>
!values.any((b) => !b)),
On 2013/07/03 11:30:27, Bill Hesse wrote:
> All the values are true anyway.
> 
> completion(anything)?

Done.

https://codereview.chromium.org/18438005/diff/1/pkg/mime/test/mime_multipart_...
pkg/mime/test/mime_multipart_transformer_test.dart:102:
"content-transfer-encoding": "base64"};
On 2013/07/03 11:30:27, Bill Hesse wrote:
> Indentation

Done.

[Bill Hesse, 2013-07-12 11:43:33]

lgtm

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/lib/src/ht...
File pkg/http_server/lib/src/http_body_impl.dart (right):

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/lib/src/ht...
pkg/http_server/lib/src/http_body_impl.dart:163: .listen((data) => string =
data,
What about an input that ends in half of an encoded character?

I would prefer 
.listen((data) => strings.add(data),
onError: ... ,
onDone: () => string = strings.join());

Then we are sure that the controller has sent close on its output stream.

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
File pkg/http_server/test/http_body_test.dart (right):

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
pkg/http_server/test/http_body_test.dart:34: if (shouldFail) throw "Error
expected";
expect(shouldFail, isFalse)

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
pkg/http_server/test/http_body_test.dart:44: throw "bad body type";
fail("bad body type");

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
pkg/http_server/test/http_body_test.dart:96: if (shouldFail) throw "Error
expected";
ditto

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
pkg/http_server/test/http_body_test.dart:121: for (var key in expectedBody.keys)
{
var found = body.body[key];
var expected = expectedBody[key];
would make the rest read easier.

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
File pkg/mime/test/mime_multipart_transformer_test.dart (right):

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
pkg/mime/test/mime_multipart_transformer_test.dart:29: var partCompleter = new
Completer();
partCompleter is unused.

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
pkg/mime/test/mime_multipart_transformer_test.dart:32: if (expectedParts[part]
!= null) {
Is this really needed?  I see only one test with expectedParts containing nulls,
and that has an error thrown from the stream of parts.  But these streams should
have something happen to them - are they closed, do they have errors?  Test what
they should do.

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
pkg/mime/test/mime_multipart_transformer_test.dart:38: }, onDone: () {
expect(expectError, isFalse);

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
pkg/mime/test/mime_multipart_transformer_test.dart:67: completion(anything));
completes is the same as completion(anything).

[Anders Johnsen, 2013-07-15 08:21:32]

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/lib/src/ht...
File pkg/http_server/lib/src/http_body_impl.dart (right):

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/lib/src/ht...
pkg/http_server/lib/src/http_body_impl.dart:163: .listen((data) => string =
data,
On 2013/07/12 11:43:33, Bill Hesse wrote:
> What about an input that ends in half of an encoded character?
> 
> I would prefer 
> .listen((data) => strings.add(data),
> onError: ... ,
> onDone: () => string = strings.join());
> 
> Then we are sure that the controller has sent close on its output stream.

Yes, see line 166.

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
File pkg/http_server/test/http_body_test.dart (right):

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
pkg/http_server/test/http_body_test.dart:34: if (shouldFail) throw "Error
expected";
On 2013/07/12 11:43:33, Bill Hesse wrote:
> expect(shouldFail, isFalse)

Done.

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
pkg/http_server/test/http_body_test.dart:44: throw "bad body type";
On 2013/07/12 11:43:33, Bill Hesse wrote:
> fail("bad body type");

Done.

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
pkg/http_server/test/http_body_test.dart:96: if (shouldFail) throw "Error
expected";
On 2013/07/12 11:43:33, Bill Hesse wrote:
> ditto

Done.

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/test/http_...
pkg/http_server/test/http_body_test.dart:121: for (var key in expectedBody.keys)
{
On 2013/07/12 11:43:33, Bill Hesse wrote:
> var found = body.body[key];
> var expected = expectedBody[key];
> would make the rest read easier.

Done.

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
File pkg/mime/test/mime_multipart_transformer_test.dart (right):

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
pkg/mime/test/mime_multipart_transformer_test.dart:29: var partCompleter = new
Completer();
On 2013/07/12 11:43:33, Bill Hesse wrote:
> partCompleter is unused.

Done.

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
pkg/mime/test/mime_multipart_transformer_test.dart:32: if (expectedParts[part]
!= null) {
On 2013/07/12 11:43:33, Bill Hesse wrote:
> Is this really needed?  I see only one test with expectedParts containing
nulls,
> and that has an error thrown from the stream of parts.  But these streams
should
> have something happen to them - are they closed, do they have errors?  Test
what
> they should do.

Testing for 'null' is a way to specify in the test that we don't care about the
data.

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
pkg/mime/test/mime_multipart_transformer_test.dart:38: }, onDone: () {
On 2013/07/12 11:43:33, Bill Hesse wrote:
> expect(expectError, isFalse);

No, I still get close (cancelOnError is not set).

https://codereview.chromium.org/18438005/diff/9001/pkg/mime/test/mime_multipa...
pkg/mime/test/mime_multipart_transformer_test.dart:67: completion(anything));
On 2013/07/12 11:43:33, Bill Hesse wrote:
> completes is the same as completion(anything).

Done.

[Bill Hesse, 2013-07-15 08:26:33]

lgtm

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/lib/src/ht...
File pkg/http_server/lib/src/http_body_impl.dart (right):

https://codereview.chromium.org/18438005/diff/9001/pkg/http_server/lib/src/ht...
pkg/http_server/lib/src/http_body_impl.dart:163: .listen((data) => string =
data,
On 2013/07/12 11:43:33, Bill Hesse wrote:
> What about an input that ends in half of an encoded character?
> 
> I would prefer 
> .listen((data) => strings.add(data),
> onError: ... ,
> onDone: () => string = strings.join());
> 
> Then we are sure that the controller has sent close on its output stream.

OK, so controller.close is guaranteed to synchronously close all the way
through, of course.

[Anders Johnsen, 2013-07-15 08:40:56]

Committed patchset #6 manually as r24991 (presubmit successful).