Cleanup: Remove the support for IPv4 and IPv6 addressing types in the SOCKS5 ...

Cleanup: Remove the support for IPv4 and IPv6 addressing types in the SOCKS5 implementation.
This is no longer used by chrome, and it is unlikely that embedders would want to use this.
The default will just use the DOMAIN addressing type.

BUG=29914
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34928

[wtc, 2009-12-18 01:33:00]

LGTM.

http://codereview.chromium.org/507048/diff/2001/3003
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/507048/diff/2001/3003#newcode60
net/socket/socks5_client_socket.cc:60: buffer_.clear();
Why do we need this?  The original code (lines 68-69) doesn't
do this.

http://codereview.chromium.org/507048/diff/2001/3003#newcode257
net/socket/socks5_client_socket.cc:257: return ERR_ADDRESS_INVALID;
Is ERR_ADDRESS_INVALID the right error code?

ERR_ADDRESS_INVALID means:

// The IP address or port number is invalid (e.g., cannot connect to the IP
// address 0 or the port 0).

http://codereview.chromium.org/507048/diff/2001/3003#newcode350
net/socket/socks5_client_socket.cc:350: else if (address_type ==
kEndPointResolvedIPv4)
Do we still need to handle the kEndPointResolvedIPv4 and
kEndPointResolvedIPv6 cases?

http://codereview.chromium.org/507048/diff/2001/3004
File net/socket/socks5_client_socket.h (left):

http://codereview.chromium.org/507048/diff/2001/3004#oldcode35
net/socket/socks5_client_socket.h:35: // SOCKS5 supports three modes of
specifying connection endpoints:
Can we keep the comments on lines 35-38 in the original code?
I find them to be informative.

http://codereview.chromium.org/507048/diff/2001/3004
File net/socket/socks5_client_socket.h (right):

http://codereview.chromium.org/507048/diff/2001/3004#newcode77
net/socket/socks5_client_socket.h:77: enum SocksEndPointAddressType {
Do we still need this enum type?

http://codereview.chromium.org/507048/diff/2001/3004#newcode136
net/socket/socks5_client_socket.h:136: HostResolver::RequestInfo
host_request_info_;
Can we remove this, too?

http://codereview.chromium.org/507048/diff/2001/3002
File net/socket/socks5_client_socket_unittest.cc (right):

http://codereview.chromium.org/507048/diff/2001/3002#newcode24
net/socket/socks5_client_socket_unittest.cc:24: namespace net {
Nit: add a blank line.

http://codereview.chromium.org/507048/diff/2001/3002#newcode302
net/socket/socks5_client_socket_unittest.cc:302: }  // namespace
Add a blank line.

[eroman, 2009-12-18 01:53:37]

http://codereview.chromium.org/507048/diff/2001/3003
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/507048/diff/2001/3003#newcode60
net/socket/socks5_client_socket.cc:60: buffer_.clear();
On 2009/12/18 01:33:00, wtc wrote:
> Why do we need this?  The original code (lines 68-69) doesn't
> do this.

Clearing |buffer_| used to be a side-effect of running the HostResolve step (it
would get cleared in DoResolveHostComplete()). So this simply copies that logic
over.

It was a mistake that in my earlier change I did not copy that over.

I am not sure if this will repro as bug, I will follow-up with a unittest either
way. Conceptually the situation I would be concerned for is:

socket->Connect();
socket->Disconnect();
socket->Connect();

Since the first Connect() will mutate buffer_, and I don't think it will be
cleared by the time Connect() runs again.

http://codereview.chromium.org/507048/diff/2001/3003#newcode257
net/socket/socks5_client_socket.cc:257: return ERR_ADDRESS_INVALID;
On 2009/12/18 01:33:00, wtc wrote:
> Is ERR_ADDRESS_INVALID the right error code?
> 
> ERR_ADDRESS_INVALID means:
> 
> // The IP address or port number is invalid (e.g., cannot connect to the IP
> // address 0 or the port 0).

How about ERR_INVALID_URL ?

(Since in some sense the URL is "invalid" in that the hostname is super long).

http://codereview.chromium.org/507048/diff/2001/3003#newcode350
net/socket/socks5_client_socket.cc:350: else if (address_type ==
kEndPointResolvedIPv4)
On 2009/12/18 01:33:00, wtc wrote:
> Do we still need to handle the kEndPointResolvedIPv4 and
> kEndPointResolvedIPv6 cases?

Yes, we still need that to know how large the response is.

http://codereview.chromium.org/507048/diff/2001/3004
File net/socket/socks5_client_socket.h (right):

http://codereview.chromium.org/507048/diff/2001/3004#newcode77
net/socket/socks5_client_socket.h:77: enum SocksEndPointAddressType {
On 2009/12/18 01:33:00, wtc wrote:
> Do we still need this enum type?

We still need this, to test the address type of the proxy's response (we need to
know this to figure out how large the response will be).

http://codereview.chromium.org/507048/diff/2001/3004#newcode136
net/socket/socks5_client_socket.h:136: HostResolver::RequestInfo
host_request_info_;
On 2009/12/18 01:33:00, wtc wrote:
> Can we remove this, too?

Yes, this is what contains the hostname and port of the connect request.

[wtc, 2009-12-18 02:05:24]

http://codereview.chromium.org/507048/diff/2001/3003
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/507048/diff/2001/3003#newcode60
net/socket/socks5_client_socket.cc:60: buffer_.clear();
On 2009/12/18 01:53:37, eroman wrote:
>
> It was a mistake that in my earlier change I did not copy that over.

I actually noticed that when I reviewed your earlier CL.
This is why I asked about it when I reviewed this CL.

http://codereview.chromium.org/507048/diff/2001/3003#newcode257
net/socket/socks5_client_socket.cc:257: return ERR_ADDRESS_INVALID;
On 2009/12/18 01:53:37, eroman wrote:
>
> How about ERR_INVALID_URL ?

Good.