pkg/http tweaks

pkg/http tweaks

locking down MultipartRequest a bit
Removed superfluous import of dart:io and dart:isolate

R=nweiz@google.com

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

[kevmoo-old, 2013-07-23 02:27:10]

Tweaks done as I pondered using http in a browser app

[Bob Nystrom, 2013-07-23 16:12:58]

On 2013/07/23 02:27:10, kevmoo wrote:
> Tweaks done as I pondered using http in a browser app

Removed myself as a reviewer since I'll just be parroting anything Nathan would
say. :)

[nweiz, 2013-07-23 20:18:49]

I like the removal of now-spurious dart:io imports, although unfortunately
making this actually work in the browser will require either
configuration-specific libraries or the sort of nasty mirror hack we're using in
pkg/path.

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_request.dart (left):

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:81: }
Why is this being removed? It shouldn't be possible to directly set the
contentLength of multipart requests.

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

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:130: static String
_headerForField(String name, String value) {
I'm not a fan of making methods static just because they happen to access no
instance state. It adds a useless token and misleadingly communicates that
they'll be accessed from a static context.

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:157: const prefix =
"dart-http-boundary-";
Also not a fan of const local variables, for roughly the same reason that I
don't like final or type-annotated local variables: its extra text that provides
no value, since locals are inherently of limited scope anyway.

[kevmoo-old, 2013-07-23 21:08:56]

Comments inline

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
File pkg/http/lib/src/multipart_request.dart (left):

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:81: }
On 2013/07/23 20:18:49, nweiz wrote:
> Why is this being removed? It shouldn't be possible to directly set the
> contentLength of multipart requests.

Didn't read the type hierarchy correctly. Oops.

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

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:130: static String
_headerForField(String name, String value) {
On 2013/07/23 20:18:49, nweiz wrote:
> I'm not a fan of making methods static just because they happen to access no
> instance state. It adds a useless token and misleadingly communicates that
> they'll be accessed from a static context.

I think it correctly communicates it *could* be accessed via a static state,
which making it clear that no instance state is modified, let alone accessed.

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:157: const prefix =
"dart-http-boundary-";
On 2013/07/23 20:18:49, nweiz wrote:
> Also not a fan of const local variables, for roughly the same reason that I
> don't like final or type-annotated local variables: its extra text that
provides
> no value, since locals are inherently of limited scope anyway.

Fair.

[nweiz, 2013-07-23 21:27:33]

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

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:130: static String
_headerForField(String name, String value) {
On 2013/07/23 21:08:56, kevmoo wrote:
> On 2013/07/23 20:18:49, nweiz wrote:
> > I'm not a fan of making methods static just because they happen to access no
> > instance state. It adds a useless token and misleadingly communicates that
> > they'll be accessed from a static context.
> 
> I think it correctly communicates it *could* be accessed via a static state,
> which making it clear that no instance state is modified, let alone accessed.

It's an implementation detail that it doesn't access instance state. There could
be configuration added later that this cares about. If it were something that
was meant for use outside of the an instance of MultipartRequest, it would be in
utils. In fact, it's a helper method for [finalize], and should inhabit the same
scope as that method.

[kevmoo-old, 2013-07-23 21:36:37]

PTAL

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

https://codereview.chromium.org/19866007/diff/3001/pkg/http/lib/src/multipart...
pkg/http/lib/src/multipart_request.dart:130: static String
_headerForField(String name, String value) {
On 2013/07/23 21:27:34, nweiz wrote:
> On 2013/07/23 21:08:56, kevmoo wrote:
> > On 2013/07/23 20:18:49, nweiz wrote:
> > > I'm not a fan of making methods static just because they happen to access
no
> > > instance state. It adds a useless token and misleadingly communicates that
> > > they'll be accessed from a static context.
> > 
> > I think it correctly communicates it *could* be accessed via a static state,
> > which making it clear that no instance state is modified, let alone
accessed.
> 
> It's an implementation detail that it doesn't access instance state. There
could
> be configuration added later that this cares about. If it were something that
> was meant for use outside of the an instance of MultipartRequest, it would be
in
> utils. In fact, it's a helper method for [finalize], and should inhabit the
same
> scope as that method.

I'm cool w/ philosophical differences here. Not a big deal. :-)

[nweiz, 2013-07-23 22:19:58]

lgtm

https://codereview.chromium.org/19866007/diff/13001/pkg/http/lib/src/multipar...
File pkg/http/lib/src/multipart_request.dart (right):

https://codereview.chromium.org/19866007/diff/13001/pkg/http/lib/src/multipar...
pkg/http/lib/src/multipart_request.dart:165: growable: false);
style nit: indent -2

[kevmoo-old, 2013-07-23 22:47:20]

Committed patchset #5 manually as r25376 (presubmit successful).