Re-implement support for client redirects.

Re-implement support for client redirects.

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

[Søren Gjesse, 2013-01-09 08:43:49]

LGTM! - but please address the comments

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:919: int maxRedirects;
Place maxRedirects after followRedirects.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:980: * Returns the series of redirects this connection has
been through.
Document that no redirects is the empty list and not null.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:996: * even if was already visited. Default value is
[false].
Maybe we need to add some additional documentation on the logic with respect to
maxRedirects when using manual redirects and that the [redirects] is updated
both for manual and automatic redirects.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:130: List<RedirectInfo> this.redirects)
Do you need to pass this? Isn't it just _httpRequest._responseRedirects?

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:157: * part of the redirection request(s).
No need for this comment in the implementation file.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:163: if (statusCode == HttpStatus.SEE_OTHER &&
_httpRequest.method == "POST") {
Add comment with reference to RFC 2616 section 10.3.4.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:170: if (!isRedirect) throw new StateError("Response
is not redirecting");
"Response is not redirecting" => Response has no Location header for redirect"

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:170: if (!isRedirect) throw new StateError("Response
is not redirecting");
We should probably only check for the Location header and not use isRedirect
here.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:185: // Allow one less redirect.
The "one less" in the comment is not reflected in the code.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:187: // Copy headers
Terminate comment with .

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:194: var redirects = [];
Just operate directly on request._responseRedirects.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:437: void _setResponseRedirects(List<RedirectInfo>
redirects) {
Why do you need this method?

https://codereview.chromium.org/11783034/diff/9001/tests/standalone/io/http_r...
File tests/standalone/io/http_redirect_test.dart (right):

https://codereview.chromium.org/11783034/diff/9001/tests/standalone/io/http_r...
tests/standalone/io/http_redirect_test.dart:164: (_) {},
Maybe add Expect.fail here as no data is expected.

https://codereview.chromium.org/11783034/diff/9001/tests/standalone/io/http_r...
tests/standalone/io/http_redirect_test.dart:193: response.listen((_) {}, onDone:
() {
Ditto.

https://codereview.chromium.org/11783034/diff/9001/tests/standalone/io/http_r...
tests/standalone/io/http_redirect_test.dart:322: // 
testManualRedirectWithHeaders();
Why are these tests still commented out?

[Anders Johnsen, 2013-01-09 10:03:00]

Thank you, very helpful comments! Landing!

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:919: int maxRedirects;
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Place maxRedirects after followRedirects.

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:980: * Returns the series of redirects this connection has
been through.
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Document that no redirects is the empty list and not null.

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http.dart#newco...
sdk/lib/io/http.dart:996: * even if was already visited. Default value is
[false].
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Maybe we need to add some additional documentation on the logic with respect
to
> maxRedirects when using manual redirects and that the [redirects] is updated
> both for manual and automatic redirects.

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:130: List<RedirectInfo> this.redirects)
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Do you need to pass this? Isn't it just _httpRequest._responseRedirects?

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:157: * part of the redirection request(s).
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> No need for this comment in the implementation file.

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:163: if (statusCode == HttpStatus.SEE_OTHER &&
_httpRequest.method == "POST") {
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Add comment with reference to RFC 2616 section 10.3.4.

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:170: if (!isRedirect) throw new StateError("Response
is not redirecting");
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> "Response is not redirecting" => Response has no Location header for redirect"

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:170: if (!isRedirect) throw new StateError("Response
is not redirecting");
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> We should probably only check for the Location header and not use isRedirect
> here.

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:185: // Allow one less redirect.
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> The "one less" in the comment is not reflected in the code.

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:187: // Copy headers
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Terminate comment with .

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:194: var redirects = [];
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Just operate directly on request._responseRedirects.

Done.

https://codereview.chromium.org/11783034/diff/9001/sdk/lib/io/http_impl.dart#...
sdk/lib/io/http_impl.dart:437: void _setResponseRedirects(List<RedirectInfo>
redirects) {
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Why do you need this method?

Done.

https://codereview.chromium.org/11783034/diff/9001/tests/standalone/io/http_r...
File tests/standalone/io/http_redirect_test.dart (right):

https://codereview.chromium.org/11783034/diff/9001/tests/standalone/io/http_r...
tests/standalone/io/http_redirect_test.dart:164: (_) {},
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Maybe add Expect.fail here as no data is expected.

Done.

https://codereview.chromium.org/11783034/diff/9001/tests/standalone/io/http_r...
tests/standalone/io/http_redirect_test.dart:193: response.listen((_) {}, onDone:
() {
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Ditto.

Done.

https://codereview.chromium.org/11783034/diff/9001/tests/standalone/io/http_r...
tests/standalone/io/http_redirect_test.dart:322: // 
testManualRedirectWithHeaders();
On 2013/01/09 08:43:49, Søren Gjesse wrote:
> Why are these tests still commented out?

Argh, fixed!!