Add a multipart HTTP request class.

Add a multipart HTTP request class.

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

[Bob Nystrom, 2012-11-06 22:00:08]

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_file.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:31: bool get finalized => _finalized;
How about "isFinalized" here? I think "if (file.isFinalized)" reads a bit more
naturally to me.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:38: /// [contentType] currently defaults to
`application/octet-stream`, but in the
I try to avoid sentences that start with identifiers just because it makes
capitalization a bit odd. This is OK, but maybe do "The [contentType]..."?

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:57: contentType: contentType);
Style nit. Put the positional args on the same line as new and put the named
args on one line?

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:64: /// [contentType] currently defaults to
`text/plain; charset=utf-8`, but in the
Long line.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:69: new
ContentType.fromString(contentType.toString());
Why the toString / fromString dance here?

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:81: // TODO(nweiz): infer the content-type
from the filename
Should be a sentence: "Infer ... filename."

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:90: return file.length().transform((length)
{
You could do lengthSync() here. Do you think it's better to make this async?

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_request.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:32: ///       // ...
Maybe "print('Uploaded!')" or something a little more concrete?

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:36: /// request body. According to RFC
1341, this can't be longer than 70.
Link to RFC 1341?

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:37: static final int _boundaryLength =
70;
static const _BOUNDARY_LENGTH = 70;

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:57: }
Please explain what the 2 and 4 magic numbers are for here. They don't
necessarily need to be named constants (though that wouldn't hurt), but at least
add a comment.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:119: static final List<int>
_boundaryCharacters = const <int>[
static const _BOUNDARY_CHARS = const <int> [ ...

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:156: var random = new Random();
Why use a random string here?

Creating a new RNG each time here might be asking for trouble if it's only
seeded by time. How about making a single static lazy initialized one?

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dar...
pkg/http/lib/src/utils.dart:83: /// Returns the [Encoding] that corresponds to
[charset]. Throws a [FormatException]
Unrelated, but long line here.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dar...
pkg/http/lib/src/utils.dart:117: final RegExp _asciiOnly = const
RegExp(r"^[\x00-\x7F]+$");
static const _ASCII_ONLY = const ...

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dar...
pkg/http/lib/src/utils.dart:175: /// iteration to stop and will be piped through
the return value.
Add a TODO that Future should support this directly. Maybe file a bug too?

https://codereview.chromium.org/11363094/diff/1001/pkg/http/test/multipart_te...
File pkg/http/test/multipart_test.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/test/multipart_te...
pkg/http/test/multipart_test.dart:35: var contentType = new
ContentType.fromString(item.headers['content-type']);
Long line.

[nweiz, 2012-11-06 23:15:56]

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_file.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:31: bool get finalized => _finalized;
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> How about "isFinalized" here? I think "if (file.isFinalized)" reads a bit more
> naturally to me.

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:38: /// [contentType] currently defaults to
`application/octet-stream`, but in the
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> I try to avoid sentences that start with identifiers just because it makes
> capitalization a bit odd. This is OK, but maybe do "The [contentType]..."?

I think using the definite article only for identifiers that start sentences
reads even weirder.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:57: contentType: contentType);
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Style nit. Put the positional args on the same line as new and put the named
> args on one line?

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:64: /// [contentType] currently defaults to
`text/plain; charset=utf-8`, but in the
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:69: new
ContentType.fromString(contentType.toString());
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Why the toString / fromString dance here?

We want to be able to modify the charset below without affecting the original
object. I've added a comment explaining this.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:81: // TODO(nweiz): infer the content-type
from the filename
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Should be a sentence: "Infer ... filename."

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:90: return file.length().transform((length)
{
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> You could do lengthSync() here. Do you think it's better to make this async?

Yeah, I don't like using synchronous IO in library code.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_request.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:32: ///       // ...
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Maybe "print('Uploaded!')" or something a little more concrete?

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:36: /// request body. According to RFC
1341, this can't be longer than 70.
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Link to RFC 1341?

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:37: static final int _boundaryLength =
70;
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> static const _BOUNDARY_LENGTH = 70;

Done. I couldn't remember what we'd settled on for constant naming conventions.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:37: static final int _boundaryLength =
70;
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> static const _BOUNDARY_LENGTH = 70;

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:57: }
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Please explain what the 2 and 4 magic numbers are for here. They don't
> necessarily need to be named constants (though that wouldn't hurt), but at
least
> add a comment.

I've changed them to .length expressions on the actual strings they represent.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:119: static final List<int>
_boundaryCharacters = const <int>[
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> static const _BOUNDARY_CHARS = const <int> [ ...

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:156: var random = new Random();
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Why use a random string here?

The boundary needs to be a string that will never appear in the uploaded files
or form parameters. The best way to do that (that doesn't involve examining the
entire contents of the files) is to generate a boundary string that's extremely
likely to be unique.

> Creating a new RNG each time here might be asking for trouble if it's only
> seeded by time. How about making a single static lazy initialized one?

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dar...
pkg/http/lib/src/utils.dart:83: /// Returns the [Encoding] that corresponds to
[charset]. Throws a [FormatException]
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Unrelated, but long line here.

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dar...
pkg/http/lib/src/utils.dart:117: final RegExp _asciiOnly = const
RegExp(r"^[\x00-\x7F]+$");
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> static const _ASCII_ONLY = const ...

Done.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dar...
pkg/http/lib/src/utils.dart:175: /// iteration to stop and will be piped through
the return value.
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Add a TODO that Future should support this directly. Maybe file a bug too?

I'll do you one better and send a patch.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/test/multipart_te...
File pkg/http/test/multipart_test.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/test/multipart_te...
pkg/http/test/multipart_test.dart:35: var contentType = new
ContentType.fromString(item.headers['content-type']);
On 2012/11/06 22:00:08, Bob Nystrom wrote:
> Long line.

Done.

[Bob Nystrom, 2012-11-07 00:50:26]

One suggestion but LGTM either way.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_file.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:38: /// [contentType] currently defaults to
`application/octet-stream`, but in the
On 2012/11/06 23:15:56, nweiz wrote:
> On 2012/11/06 22:00:08, Bob Nystrom wrote:
> > I try to avoid sentences that start with identifiers just because it makes
> > capitalization a bit odd. This is OK, but maybe do "The [contentType]..."?
> 
> I think using the definite article only for identifiers that start sentences
> reads even weirder.

Fair enough.

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11363094/diff/1001/pkg/http/lib/src/utils.dar...
pkg/http/lib/src/utils.dart:175: /// iteration to stop and will be piped through
the return value.
On 2012/11/06 23:15:56, nweiz wrote:
> On 2012/11/06 22:00:08, Bob Nystrom wrote:
> > Add a TODO that Future should support this directly. Maybe file a bug too?
> 
> I'll do you one better and send a patch.

I love patches!

https://codereview.chromium.org/11363094/diff/3002/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_file.dart (right):

https://codereview.chromium.org/11363094/diff/3002/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:68: // Make a copy of the original
contentType so we can modify charset.
If this is the motivation, maybe add a clone method to ContentType?

[nweiz, 2012-11-07 20:53:28]

https://codereview.chromium.org/11363094/diff/3002/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_file.dart (right):

https://codereview.chromium.org/11363094/diff/3002/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_file.dart:68: // Make a copy of the original
contentType so we can modify charset.
On 2012/11/07 00:50:26, Bob Nystrom wrote:
> If this is the motivation, maybe add a clone method to ContentType?

I think there's a broader language-level (or at least all-libraries-level) issue
of "how do I clone an object?". I don't think the solution is to add one-off
clone methods to every class that we happen to want to clone somewhere. I
suspect something better will come out of the discussions on serialization,
since the problems are related.