Add an HTTP client based on Curl.

Add an HTTP client based on Curl.

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

[Bob Nystrom, 2012-11-07 23:15:15]

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
File pkg/http/lib/src/curl_client.dart (right):

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:16: /// temporarily until [dart:io] natively
supports requests over HTTPS.
Given this, maybe it should be put in a sublibrary instead of exporting it from
the main http one?

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:22: CurlClient([String executable])
It's seems a bit redundant, but document the constructor param here too please.

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:93: Future _waitForHeaders(Process process,
bool noResponseBody) {
How about "expectBody" instead of "noResponseBody", and I would make a named
parameter too since it's a bool.

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:124: var resetCallbacks = () {
resetCallbacks() {
  ...
}

Dart has nested function statements. :)

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:167: headers[split[0].toLowerCase()] =
split[1].trim();
What's the toLowercase here for?

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

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:189: /// will be deleted.
Very nice.

https://codereview.chromium.org/11394003/diff/1/pkg/http/test/curl_client_tes...
File pkg/http/test/curl_client_test.dart (right):

https://codereview.chromium.org/11394003/diff/1/pkg/http/test/curl_client_tes...
pkg/http/test/curl_client_test.dart:20: test('head', () {
Are these tests essentially the same as for the other client? If so, it would be
good to reuse them and just pass in the client (or a client factory function)
instead of copy/paste.

[nweiz, 2012-11-08 00:33:37]

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
File pkg/http/lib/src/curl_client.dart (right):

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:16: /// temporarily until [dart:io] natively
supports requests over HTTPS.
On 2012/11/07 23:15:15, Bob Nystrom wrote:
> Given this, maybe it should be put in a sublibrary instead of exporting it
from
> the main http one?

I don't think that adds any utility. It just makes it more confusing as to what
prefix to use when importing it.

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:22: CurlClient([String executable])
On 2012/11/07 23:15:15, Bob Nystrom wrote:
> It's seems a bit redundant, but document the constructor param here too
please.

Done.

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:93: Future _waitForHeaders(Process process,
bool noResponseBody) {
On 2012/11/07 23:15:15, Bob Nystrom wrote:
> How about "expectBody" instead of "noResponseBody", and I would make a named
> parameter too since it's a bool.

Done.

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:124: var resetCallbacks = () {
On 2012/11/07 23:15:15, Bob Nystrom wrote:
> resetCallbacks() {
>   ...
> }
> 
> Dart has nested function statements. :)

Done.

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:167: headers[split[0].toLowerCase()] =
split[1].trim();
On 2012/11/07 23:15:15, Bob Nystrom wrote:
> What's the toLowercase here for?

Matching dart:io's convention of lower-casing header names.

https://codereview.chromium.org/11394003/diff/1/pkg/http/test/curl_client_tes...
File pkg/http/test/curl_client_test.dart (right):

https://codereview.chromium.org/11394003/diff/1/pkg/http/test/curl_client_tes...
pkg/http/test/curl_client_test.dart:20: test('head', () {
On 2012/11/07 23:15:15, Bob Nystrom wrote:
> Are these tests essentially the same as for the other client? If so, it would
be
> good to reuse them and just pass in the client (or a client factory function)
> instead of copy/paste.

These tests come from the top-level function tests, which can't be
dependency-injected in that way.

[Bob Nystrom, 2012-11-08 00:49:13]

LGTM!

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
File pkg/http/lib/src/curl_client.dart (right):

https://codereview.chromium.org/11394003/diff/1/pkg/http/lib/src/curl_client....
pkg/http/lib/src/curl_client.dart:167: headers[split[0].toLowerCase()] =
split[1].trim();
On 2012/11/08 00:33:37, nweiz wrote:
> On 2012/11/07 23:15:15, Bob Nystrom wrote:
> > What's the toLowercase here for?
> 
> Matching dart:io's convention of lower-casing header names.

Works for me. A comment to that effect would help here.

https://codereview.chromium.org/11394003/diff/1/pkg/http/test/curl_client_tes...
File pkg/http/test/curl_client_test.dart (right):

https://codereview.chromium.org/11394003/diff/1/pkg/http/test/curl_client_tes...
pkg/http/test/curl_client_test.dart:20: test('head', () {
On 2012/11/08 00:33:37, nweiz wrote:
> On 2012/11/07 23:15:15, Bob Nystrom wrote:
> > Are these tests essentially the same as for the other client? If so, it
would
> be
> > good to reuse them and just pass in the client (or a client factory
function)
> > instead of copy/paste.
> 
> These tests come from the top-level function tests, which can't be
> dependency-injected in that way.

SGTM.

[Bob Nystrom, 2012-11-08 00:53:10]

LGTM.

Note that this won't currently work on windows, so you should either disable it
(boo) or figure out a way to make it work (yay!).