Code cleanup (mostly io lib and some http lib).

Code cleanup (mostly io lib and some http lib).

- Unused function removed,
- Make properties always appear before methods,
- Use dart idioms where possible.

BUG=
R=ajohnsen@google.com

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

[Lasse Reichstein Nielsen, 2014-01-06 09:29:29]

This is great stuff. Are you testing a linter, or was this manual work? :)

We do need you to sign the contributor license agreement before we can commit
your changes (https://developers.google.com/open-source/cla/individual - the
electronic version at the bottom is the easiest way).

Cheers

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

https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/io_client.d...
pkg/http/lib/src/io_client.dart:31: ..followRedirects = request.followRedirects
Indent by 4.

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

https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/mock_client...
pkg/http/lib/src/mock_client.dart:37: ..persistentConnection =
baseRequest.persistentConnection
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/common.dart
File sdk/lib/io/common.dart (right):

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/common.dart#newco...
sdk/lib/io/common.dart:80: ..write(errorCode.toString());
Not to self: We should probably remove the toString call here.
If the StringBuffer gets smarter, it might know a smart way to generate the
integer directly into the buffer without an intermediate string. This code
prevents such optimizations.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/directory_impl.dart
File sdk/lib/io/directory_impl.dart (right):

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/directory_impl.da...
sdk/lib/io/directory_impl.dart:233: bool _isErrorResponse(response) =>
Style guide isn't prescriptive about this - there are people who prefer the
return when the expression can't stay on the same line as the =>.
(Personally, I'm fine with this change).

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

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http.dart#newcode98
sdk/lib/io/http.dart:98: * completed, the connection is droped.
droped -> dropped

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http.dart#newcode...
sdk/lib/io/http.dart:1588: ..write('HttpException: ')
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_date.dart
File sdk/lib/io/http_date.dart (right):

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_date.dart#ne...
sdk/lib/io/http_date.dart:47: ..write(wkday[d.weekday - 1])
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart
File sdk/lib/io/http_headers.dart (right):

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
sdk/lib/io/http_headers.dart:46: value.forEach((v) => _add(name, v));
This may be a little slower than the original.
Probably not noticeably so, so I'm fine with it.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
sdk/lib/io/http_headers.dart:425: ..write(": ");
Hmm, technically the second line should be indented by 4, but I really don't
want to do that here.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
sdk/lib/io/http_headers.dart:709: String name;
Are none of these really final?

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

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:36: _HttpIncoming(_HttpHeaders this.headers,
this._transferLength, this._stream);
Remove _HttpHeaders too.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:94: ..chunkedTransferEncoding = true
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:229: ..addAll(this.redirects)
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:281: List<String> authChallenge() => proxyAuth ?
I prefer using a block with a return here. Perhaps: 
  List<String> authChallenge() {
    return headers[proxyAuth ? HttpHeaders.PROXY_AUTHENTICATE
                             : HttpHeaders.WWW_AUTHENTICATE];
  }

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:287: _httpClient._findCredentials(_httpRequest.uri,
scheme);
Ditto here.
Using => with expressions that span more than one line is likely to look better
using a block.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:785: ..value = session.id
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:794: ..httpOnly = true
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1266: ..host = uri.host
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:2341: ..add(UTF8.encode(creds.username))
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:2461: ..add(request.method.codeUnits)
Indent by 4 (and several similar below).

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_parser.dart
File sdk/lib/io/http_parser.dart (right):

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_parser.dart#...
sdk/lib/io/http_parser.dart:122: ..resume()
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dar...
sdk/lib/io/secure_socket.dart:611: int available() => _status != CONNECTED ?
I'd prefer the block-and-return here.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dar...
sdk/lib/io/secure_socket.dart:1102: int get length => start > end ?
Use block-and-return for these three too.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.dart
File sdk/lib/io/websocket_impl.dart (right):

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
sdk/lib/io/websocket_impl.dart:415: ..close();
Indent by 4, ditto twice below.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
sdk/lib/io/websocket_impl.dart:562: createFrame(opcode, data,
webSocket._serverSide).forEach(_eventSink.add);
Indent by 4.

https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
sdk/lib/io/websocket_impl.dart:806: ..add(HttpHeaders.CONNECTION, "upgrade")
Indent by 4.

[vicb, 2014-01-06 09:41:03]

On 2014/01/06 09:29:29, Lasse Reichstein Nielsen wrote:
> This is great stuff. Are you testing a linter, or was this manual work? :)
> 
> We do need you to sign the contributor license agreement before we can commit
> your changes (https://developers.google.com/open-source/cla/individual - the
> electronic version at the bottom is the easiest way).
> 
> Cheers
> 
>
https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/io_client.dart
> File pkg/http/lib/src/io_client.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/io_client.d...
> pkg/http/lib/src/io_client.dart:31: ..followRedirects =
request.followRedirects
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/mock_client...
> File pkg/http/lib/src/mock_client.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/mock_client...
> pkg/http/lib/src/mock_client.dart:37: ..persistentConnection =
> baseRequest.persistentConnection
> Indent by 4.
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/common.dart
> File sdk/lib/io/common.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/common.dart#newco...
> sdk/lib/io/common.dart:80: ..write(errorCode.toString());
> Not to self: We should probably remove the toString call here.
> If the StringBuffer gets smarter, it might know a smart way to generate the
> integer directly into the buffer without an intermediate string. This code
> prevents such optimizations.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/directory_impl.dart
> File sdk/lib/io/directory_impl.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/directory_impl.da...
> sdk/lib/io/directory_impl.dart:233: bool _isErrorResponse(response) =>
> Style guide isn't prescriptive about this - there are people who prefer the
> return when the expression can't stay on the same line as the =>.
> (Personally, I'm fine with this change).
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http.dart
> File sdk/lib/io/http.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http.dart#newcode98
> sdk/lib/io/http.dart:98: * completed, the connection is droped.
> droped -> dropped
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http.dart#newcode...
> sdk/lib/io/http.dart:1588: ..write('HttpException: ')
> Indent by 4.
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_date.dart
> File sdk/lib/io/http_date.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_date.dart#ne...
> sdk/lib/io/http_date.dart:47: ..write(wkday[d.weekday - 1])
> Indent by 4.
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart
> File sdk/lib/io/http_headers.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
> sdk/lib/io/http_headers.dart:46: value.forEach((v) => _add(name, v));
> This may be a little slower than the original.
> Probably not noticeably so, so I'm fine with it.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
> sdk/lib/io/http_headers.dart:425: ..write(": ");
> Hmm, technically the second line should be indented by 4, but I really don't
> want to do that here.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
> sdk/lib/io/http_headers.dart:709: String name;
> Are none of these really final?
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart
> File sdk/lib/io/http_impl.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:36: _HttpIncoming(_HttpHeaders this.headers,
> this._transferLength, this._stream);
> Remove _HttpHeaders too.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:94: ..chunkedTransferEncoding = true
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:229: ..addAll(this.redirects)
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:281: List<String> authChallenge() => proxyAuth ?
> I prefer using a block with a return here. Perhaps: 
>   List<String> authChallenge() {
>     return headers[proxyAuth ? HttpHeaders.PROXY_AUTHENTICATE
>                              : HttpHeaders.WWW_AUTHENTICATE];
>   }
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:287: _httpClient._findCredentials(_httpRequest.uri,
> scheme);
> Ditto here.
> Using => with expressions that span more than one line is likely to look
better
> using a block.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:785: ..value = session.id
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:794: ..httpOnly = true
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:1266: ..host = uri.host
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:2341: ..add(UTF8.encode(creds.username))
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:2461: ..add(request.method.codeUnits)
> Indent by 4 (and several similar below).
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_parser.dart
> File sdk/lib/io/http_parser.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_parser.dart#...
> sdk/lib/io/http_parser.dart:122: ..resume()
> Indent by 4.
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dart
> File sdk/lib/io/secure_socket.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dar...
> sdk/lib/io/secure_socket.dart:611: int available() => _status != CONNECTED ?
> I'd prefer the block-and-return here.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dar...
> sdk/lib/io/secure_socket.dart:1102: int get length => start > end ?
> Use block-and-return for these three too.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.dart
> File sdk/lib/io/websocket_impl.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
> sdk/lib/io/websocket_impl.dart:415: ..close();
> Indent by 4, ditto twice below.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
> sdk/lib/io/websocket_impl.dart:562: createFrame(opcode, data,
> webSocket._serverSide).forEach(_eventSink.add);
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
> sdk/lib/io/websocket_impl.dart:806: ..add(HttpHeaders.CONNECTION, "upgrade")
> Indent by 4.

Thanks for the review, I'll update later today.

That's all manual work... it allows me better understanding the libs.

Cheers,
Victor

[Søren Gjesse, 2014-01-06 10:00:39]

On 2014/01/06 09:29:29, Lasse Reichstein Nielsen wrote:
> This is great stuff. Are you testing a linter, or was this manual work? :)
> 
> We do need you to sign the contributor license agreement before we can commit
> your changes (https://developers.google.com/open-source/cla/individual - the
> electronic version at the bottom is the easiest way).
> 
> Cheers
> 
>
https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/io_client.dart
> File pkg/http/lib/src/io_client.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/io_client.d...
> pkg/http/lib/src/io_client.dart:31: ..followRedirects =
request.followRedirects
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/mock_client...
> File pkg/http/lib/src/mock_client.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/pkg/http/lib/src/mock_client...
> pkg/http/lib/src/mock_client.dart:37: ..persistentConnection =
> baseRequest.persistentConnection
> Indent by 4.
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/common.dart
> File sdk/lib/io/common.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/common.dart#newco...
> sdk/lib/io/common.dart:80: ..write(errorCode.toString());
> Not to self: We should probably remove the toString call here.
> If the StringBuffer gets smarter, it might know a smart way to generate the
> integer directly into the buffer without an intermediate string. This code
> prevents such optimizations.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/directory_impl.dart
> File sdk/lib/io/directory_impl.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/directory_impl.da...
> sdk/lib/io/directory_impl.dart:233: bool _isErrorResponse(response) =>
> Style guide isn't prescriptive about this - there are people who prefer the
> return when the expression can't stay on the same line as the =>.
> (Personally, I'm fine with this change).
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http.dart
> File sdk/lib/io/http.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http.dart#newcode98
> sdk/lib/io/http.dart:98: * completed, the connection is droped.
> droped -> dropped
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http.dart#newcode...
> sdk/lib/io/http.dart:1588: ..write('HttpException: ')
> Indent by 4.
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_date.dart
> File sdk/lib/io/http_date.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_date.dart#ne...
> sdk/lib/io/http_date.dart:47: ..write(wkday[d.weekday - 1])
> Indent by 4.
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart
> File sdk/lib/io/http_headers.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
> sdk/lib/io/http_headers.dart:46: value.forEach((v) => _add(name, v));
> This may be a little slower than the original.
> Probably not noticeably so, so I'm fine with it.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
> sdk/lib/io/http_headers.dart:425: ..write(": ");
> Hmm, technically the second line should be indented by 4, but I really don't
> want to do that here.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_headers.dart...
> sdk/lib/io/http_headers.dart:709: String name;
> Are none of these really final?
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart
> File sdk/lib/io/http_impl.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:36: _HttpIncoming(_HttpHeaders this.headers,
> this._transferLength, this._stream);
> Remove _HttpHeaders too.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:94: ..chunkedTransferEncoding = true
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:229: ..addAll(this.redirects)
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:281: List<String> authChallenge() => proxyAuth ?
> I prefer using a block with a return here. Perhaps: 
>   List<String> authChallenge() {
>     return headers[proxyAuth ? HttpHeaders.PROXY_AUTHENTICATE
>                              : HttpHeaders.WWW_AUTHENTICATE];
>   }
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:287: _httpClient._findCredentials(_httpRequest.uri,
> scheme);
> Ditto here.
> Using => with expressions that span more than one line is likely to look
better
> using a block.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:785: ..value = session.id
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:794: ..httpOnly = true
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:1266: ..host = uri.host
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:2341: ..add(UTF8.encode(creds.username))
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_impl.dart#ne...
> sdk/lib/io/http_impl.dart:2461: ..add(request.method.codeUnits)
> Indent by 4 (and several similar below).
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_parser.dart
> File sdk/lib/io/http_parser.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/http_parser.dart#...
> sdk/lib/io/http_parser.dart:122: ..resume()
> Indent by 4.
> 
> https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dart
> File sdk/lib/io/secure_socket.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dar...
> sdk/lib/io/secure_socket.dart:611: int available() => _status != CONNECTED ?
> I'd prefer the block-and-return here.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/secure_socket.dar...
> sdk/lib/io/secure_socket.dart:1102: int get length => start > end ?
> Use block-and-return for these three too.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.dart
> File sdk/lib/io/websocket_impl.dart (right):
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
> sdk/lib/io/websocket_impl.dart:415: ..close();
> Indent by 4, ditto twice below.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
> sdk/lib/io/websocket_impl.dart:562: createFrame(opcode, data,
> webSocket._serverSide).forEach(_eventSink.add);
> Indent by 4.
> 
>
https://codereview.chromium.org/124753002/diff/1/sdk/lib/io/websocket_impl.da...
> sdk/lib/io/websocket_impl.dart:806: ..add(HttpHeaders.CONNECTION, "upgrade")
> Indent by 4.

As far as I can see victor.berchet@gmail.com already signed the CLA in 2012.

[Lasse Reichstein Nielsen, 2014-01-06 13:53:39]

On 2014/01/06 10:00:39, Søren Gjesse wrote:

> As far as I can see mailto:victor.berchet@gmail.com already signed the CLA in
2012.

Right you are! In that case, if you address the comments, and Anders doesn't
have further comments, I'll land it!

[vicb, 2014-01-06 13:57:15]

On 2014/01/06 13:53:39, Lasse Reichstein Nielsen wrote:

> 
> Right you are! In that case, if you address the comments, and Anders doesn't
> have further comments, I'll land it!

I have one more patch to come and I'll be ok (unit tests running).

I do have one OOT question: I have a branch with updates to the Zlib c++/dart
extensions,
but I have some pending questions, which would be the most suitable ML to ask
questions ?

Thanks for your help,
Victor

[Anders Johnsen, 2014-01-06 14:01:52]

Hi Victor,

I did the ZLib impl, so I suppose you could just upload a CL and add me as
reviewer :)

This is LGTM, with a few comments.

- Anders

https://codereview.chromium.org/124753002/diff/150001/sdk/lib/io/file_system_...
File sdk/lib/io/file_system_entity.dart (right):

https://codereview.chromium.org/124753002/diff/150001/sdk/lib/io/file_system_...
sdk/lib/io/file_system_entity.dart:146: ..add(codes[(permissions >> 6) & 0x7])
Indentation.

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

https://codereview.chromium.org/124753002/diff/150001/sdk/lib/io/http_impl.da...
sdk/lib/io/http_impl.dart:2462: _MD5 hasher = new _MD5()
Indentation, and below.

https://codereview.chromium.org/124753002/diff/150001/sdk/lib/io/secure_socke...
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/124753002/diff/150001/sdk/lib/io/secure_socke...
sdk/lib/io/secure_socket.dart:539: ..onData(_eventDispatcher)
Indentation.

https://codereview.chromium.org/124753002/diff/150001/sdk/lib/io/secure_socke...
sdk/lib/io/secure_socket.dart:1103: int get length {
=> ?

[vicb, 2014-01-06 14:16:14]

On 2014/01/06 14:01:52, Anders Johnsen wrote:
> Hi Victor,
> 
> I did the ZLib impl, so I suppose you could just upload a CL and add me as
> reviewer :)
>

I'll do that in the coming days. Basically I want to expose more 
zlib options (windowsBits, raw, ...).

> 
> This is LGTM, with a few comments.
> 
> - Anders

Most your comments are fixed in the latest patch (Set 3 and 4 are the same,
I got a 500 error while uploading the first time)

> 
>
https://codereview.chromium.org/124753002/diff/150001/sdk/lib/io/secure_socke...
> sdk/lib/io/secure_socket.dart:1103: int get length {
> => ?

It actually was a "=>" on my first patch and has been changed to {} after
Lasse review (to be complete, they were 2 liner before). Should I change again ?

Cheers,
Victor

[vicb, 2014-01-06 14:52:25]

I forgot to mention that I am done with this patch in my latest msg.

[Søren Gjesse, 2014-01-06 14:57:38]

On 2014/01/06 13:53:39, Lasse Reichstein Nielsen wrote:
> On 2014/01/06 10:00:39, Søren Gjesse wrote:
> 
> > As far as I can see mailto:victor.berchet@gmail.com already signed the CLA
in
> 2012.
> 
> Right you are! In that case, if you address the comments, and Anders doesn't
> have further comments, I'll land it!

Lasse, before landing please add victor.berchet@gmail.com to the AUTHORS file.

[Lasse Reichstein Nielsen, 2014-01-07 12:31:28]

On 2014/01/06 14:16:14, vicb wrote:
> On 2014/01/06 14:01:52, Anders Johnsen wrote:
>
https://codereview.chromium.org/124753002/diff/150001/sdk/lib/io/secure_socke...
> > sdk/lib/io/secure_socket.dart:1103: int get length {
> > => ?
> 
> It actually was a "=>" on my first patch and has been changed to {} after
> Lasse review (to be complete, they were 2 liner before). Should I change again
?

If you want to. I'm fine with either.
As long as the expression fits in one line, I'll accept =>, but I won't require
it :)

Cheers.

[vicb, 2014-01-07 12:39:29]

let's use the shorter version (included in set 5)

[Lasse Reichstein Nielsen, 2014-01-08 12:03:34]

Committed patchset #7 manually as r31602 (presubmit successful).

[Ivan Posva, 2014-01-08 17:35:37]

https://codereview.chromium.org/124753002/diff/510001/sdk/lib/io/http_date.dart
File sdk/lib/io/http_date.dart (right):

https://codereview.chromium.org/124753002/diff/510001/sdk/lib/io/http_date.da...
sdk/lib/io/http_date.dart:46: StringBuffer sb = new StringBuffer()
Lasse,

Can you please explain the motivation of this kind of change? I am failing to
see the increase in readability, you are not saving a local variable declaration
and not saving any keystrokes.

Thanks,
-Ivan

[vicb, 2014-01-08 18:44:19]

On 2014/01/08 17:35:37, Ivan Posva wrote:
> Can you please explain the motivation of this kind of change? I am failing to
> see the increase in readability, you are not saving a local variable
declaration
> and not saving any keystrokes.

Ivan, you are right in the sense that it almost saves no keystrokes. My
motivation was to introduce more Dart idioms which I think is important in the
core lib - some devs like myself use them to learn about Dart.

I could have gone further, saving more keystrokes and a local variable by
chaining the '.toString()', I missed that.

If there is a consensus, I could revert the code.

Cheers,
Victor

[nweiz, 2014-01-28 20:56:54]

Please don't commit changes to Bob's and my pkg packages without including us on
the review.

[vicb, 2014-01-28 21:23:27]

Is there any place where package owners are listed ?

I had trouble ccing a reviewer for my changes (& blaming w/ git does not work,
the history has a depth of 1 only)

Thanks for your help,
Victor

[ahe, 2014-02-13 04:04:14]

Hi Victor,

I think it is great that you corrected spelling errors and removed dead code.
Thank you for doing that!

However, some of the changes made concern me. For example, changing code to use
cascades, moving functions around, and replacing "return" with "=>". I think it
is unnecessary. It doesn't fix bugs, and might add new bugs. It also creates
noise in the version history.

I think we should consider having a policy in Dart that you shouldn't make
formatting-only changes to code you're not maintaining.

On 2014/01/28 21:23:27, vicb wrote:
> Is there any place where package owners are listed ?

I prefer the word "maintainer" to "owner". It is considered polite to CC
maintainers.

Most often, the most recent committers or reviewers are the maintainers, and you
can find them by browsing the sources via the project page, for example:

https://code.google.com/p/dart/source/list?path=/branches/bleeding_edge/dart/...

Cheers,
Peter

> I had trouble ccing a reviewer for my changes (& blaming w/ git does not work,
> the history has a depth of 1 only)
> 
> Thanks for your help,
> Victor

[vicb, 2014-02-13 08:55:46]

On 2014/02/13 04:04:14, ahe wrote:
> Hi Victor,
> 
> I think it is great that you corrected spelling errors and removed dead code.
> Thank you for doing that!

Thanks

> However, some of the changes made concern me. For example, changing code to
use
> cascades, moving functions around, and replacing "return" with "=>". I think
it
> is unnecessary. It doesn't fix bugs, and might add new bugs. It also creates
> noise in the version history.

You're right, moving chunks of code might not be a good thing as it could cause
headache when merging changes. However smaller changes should not be a problem 
with version control ? IMO "noise" is inherent to VCS, not a bad thing.

I read the core lib code to learn Dart - I expect it to follow best practices. 
When I think it could be improved, I try to improve. If some(/all) changes
should 
not be merged, let me know and I will revert.

Regarding potential bug introduction, Dart has UTs and wonderful reviewers but
once again, I would be happy to revert any un-relevant changes.
 
> I think we should consider having a policy in Dart that you shouldn't make
> formatting-only changes to code you're not maintaining.

Good point. A CONTRIBUTING[.md] file with guidelines at the repo root would 
be great !
 
> On 2014/01/28 21:23:27, vicb wrote:
> > Is there any place where package owners are listed ?
> 
> I prefer the word "maintainer" to "owner". It is considered polite to CC
> maintainers.
> 
> Most often, the most recent committers or reviewers are the maintainers, and
you
> can find them by browsing the sources via the project page, for example:
> 
>
https://code.google.com/p/dart/source/list?path=/branches/bleeding_edge/dart/...

Thanks for the tip. I'll try to do better in the future, still learning and
you guys are great at providing useful advice. codereview is a bit hard at
th beginning, I really miss some github features (ie a pull request can be
composed of multiple commits, you can see all the PRs related to one project
easily, ...).
 
> Cheers,
> Peter

Thank you Peter and all for being helpful. Let me know what you think about a
CONTRIBUTING[.md] a the root. If it can help, I could draft something.

Cheers,
Victor