Add support for HTTP proxy in the HTTP client

Add support for HTTP proxy in the HTTP client

An HttpClient instance now has the property findProxy. This
property can be set to a function which can return the proxy
configuration encoded in a string in the same format as is used
by browser PAC (proxy auto-config) scripts.

Currently only the first proxy in the list will be used. I will
address trying multiple proxies in a separate CL.

Also fixed a HTTP input stream bug revealed by the tests. If an
input stream from either a HttpRequest or HttpClientResponse
object is retreived after all data in the body has been received
the HTTP input stream would call the onClosed callback.

R=ajohnsen@google.com

BUG=dart:5468
TEST=tests/standalone/io/http_proxy_test.dart

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

[Anders Johnsen, 2012-10-08 11:05:25]

LGTM. Very nice. Maybe we should create a sample somewhere, with the ProxyServer
you have in the test?

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:1995: int port = int.parse(proxy.substring(colon +
1).trim());
Try around parse (or the hole method)?

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2046: return _openUrl(method, url);
Nice catch :)

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2130: // TODO(sgjesse): Keep a map of these as
normally only a few
That's only valid if it won't change over time. Is that a fair assumption?

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2138: if (proxyConfiguration.proxies[0].direct) {
proxyConfiguration.proxies[0] is used quite a bit. Can you extract it early and
reuse throughout?

https://codereview.chromium.org/11066041/diff/3001/tests/standalone/io/http_p...
File tests/standalone/io/http_proxy_test.dart (right):

https://codereview.chromium.org/11066041/diff/3001/tests/standalone/io/http_p...
tests/standalone/io/http_proxy_test.dart:20: if (proxyHops > 0) {
Indentation.

https://codereview.chromium.org/11066041/diff/3001/tests/standalone/io/http_p...
tests/standalone/io/http_proxy_test.dart:128: int testDirectDoneCount = 0;
Why is this(and those below) defined outside the function?

[Søren Gjesse, 2012-10-08 12:17:30]

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:1995: int port = int.parse(proxy.substring(colon +
1).trim());
On 2012/10/08 11:05:25, ajohnsen wrote:
> Try around parse (or the hole method)?

Done.

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2130: // TODO(sgjesse): Keep a map of these as
normally only a few
On 2012/10/08 11:05:25, ajohnsen wrote:
> That's only valid if it won't change over time. Is that a fair assumption?

In most cases only a few different strings will be returned from findProxy. What
I am suggesting is to have a map from these strings to their _ProxyConfiguration
decoding.

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2138: if (proxyConfiguration.proxies[0].direct) {
On 2012/10/08 11:05:25, ajohnsen wrote:
> proxyConfiguration.proxies[0] is used quite a bit. Can you extract it early
and
> reuse throughout?

Done.

https://codereview.chromium.org/11066041/diff/3001/tests/standalone/io/http_p...
File tests/standalone/io/http_proxy_test.dart (right):

https://codereview.chromium.org/11066041/diff/3001/tests/standalone/io/http_p...
tests/standalone/io/http_proxy_test.dart:20: if (proxyHops > 0) {
On 2012/10/08 11:05:25, ajohnsen wrote:
> Indentation.

Done.

https://codereview.chromium.org/11066041/diff/3001/tests/standalone/io/http_p...
tests/standalone/io/http_proxy_test.dart:128: int testDirectDoneCount = 0;
On 2012/10/08 11:05:25, ajohnsen wrote:
> Why is this(and those below) defined outside the function?

Because otherwise its initial value of 0 is captured by the conn.onResponse
closure and the stopping condition does not work.

[Anders Johnsen, 2012-10-08 12:27:29]

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11066041/diff/3001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2130: // TODO(sgjesse): Keep a map of these as
normally only a few
On 2012/10/08 12:17:30, Søren Gjesse wrote:
> On 2012/10/08 11:05:25, ajohnsen wrote:
> > That's only valid if it won't change over time. Is that a fair assumption?
> 
> In most cases only a few different strings will be returned from findProxy.
What
> I am suggesting is to have a map from these strings to their
_ProxyConfiguration
> decoding.

Ahh okay!

https://codereview.chromium.org/11066041/diff/3001/tests/standalone/io/http_p...
File tests/standalone/io/http_proxy_test.dart (right):

https://codereview.chromium.org/11066041/diff/3001/tests/standalone/io/http_p...
tests/standalone/io/http_proxy_test.dart:128: int testDirectDoneCount = 0;
On 2012/10/08 12:17:30, Søren Gjesse wrote:
> On 2012/10/08 11:05:25, ajohnsen wrote:
> > Why is this(and those below) defined outside the function?
> 
> Because otherwise its initial value of 0 is captured by the conn.onResponse
> closure and the stopping condition does not work.

Okay!

[Mads Ager (google), 2012-10-08 16:59:39]

DBC

https://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:1979: _ProxyConfiguration(String configuration) :
proxies = new List<_Proxy>() {
Funky indentation.

https://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2013: const _ProxyConfiguration.direct()
Maybe add a space between the constructors.

https://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2180: !proxyConfiguration.proxies[0].direct);
Just !proxy.direct?

https://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart...
runtime/bin/http_impl.dart:2188: !proxyConfiguration.proxies[0].direct));
!proxy.direct.

https://codereview.chromium.org/11066041/diff/8001/tests/standalone/io/http_p...
File tests/standalone/io/http_proxy_test.dart (right):

https://codereview.chromium.org/11066041/diff/8001/tests/standalone/io/http_p...
tests/standalone/io/http_proxy_test.dart:16: server.listen("127.0.0.1", 0, 5);
Maybe leave out the backlog parameter and just use the default one (which is
higher)?

https://codereview.chromium.org/11066041/diff/8001/tests/standalone/io/http_p...
tests/standalone/io/http_proxy_test.dart:18: (HttpRequest request, HttpResponse
response) {
Funky indentation. See the

   if (...) {
} else {
}

below.

https://codereview.chromium.org/11066041/diff/8001/tests/standalone/io/http_p...
tests/standalone/io/http_proxy_test.dart:63: server.listen("127.0.0.1", 0, 5);
Ditto for the backlog param?

[Søren Gjesse, 2012-10-09 14:05:22]

DBC comments addressed in https://codereview.chromium.org/11098018.

http://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

http://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart#...
runtime/bin/http_impl.dart:1979: _ProxyConfiguration(String configuration) :
proxies = new List<_Proxy>() {
On 2012/10/08 16:59:39, Mads Ager wrote:
> Funky indentation.

Done.

http://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart#...
runtime/bin/http_impl.dart:2013: const _ProxyConfiguration.direct()
On 2012/10/08 16:59:39, Mads Ager wrote:
> Maybe add a space between the constructors.

Done.

http://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart#...
runtime/bin/http_impl.dart:2180: !proxyConfiguration.proxies[0].direct);
On 2012/10/08 16:59:39, Mads Ager wrote:
> Just !proxy.direct?

Already fixed.

http://codereview.chromium.org/11066041/diff/8001/runtime/bin/http_impl.dart#...
runtime/bin/http_impl.dart:2188: !proxyConfiguration.proxies[0].direct));
On 2012/10/08 16:59:39, Mads Ager wrote:
> !proxy.direct.

Already fixed.

http://codereview.chromium.org/11066041/diff/8001/tests/standalone/io/http_pr...
File tests/standalone/io/http_proxy_test.dart (right):

http://codereview.chromium.org/11066041/diff/8001/tests/standalone/io/http_pr...
tests/standalone/io/http_proxy_test.dart:16: server.listen("127.0.0.1", 0, 5);
On 2012/10/08 16:59:39, Mads Ager wrote:
> Maybe leave out the backlog parameter and just use the default one (which is
> higher)?

Done.

http://codereview.chromium.org/11066041/diff/8001/tests/standalone/io/http_pr...
tests/standalone/io/http_proxy_test.dart:18: (HttpRequest request, HttpResponse
response) {
On 2012/10/08 16:59:39, Mads Ager wrote:
> Funky indentation. See the
> 
>    if (...) {
> } else {
> }
> 
> below.

Done.

http://codereview.chromium.org/11066041/diff/8001/tests/standalone/io/http_pr...
tests/standalone/io/http_proxy_test.dart:63: server.listen("127.0.0.1", 0, 5);
On 2012/10/08 16:59:39, Mads Ager wrote:
> Ditto for the backlog param?

Done.