Reimplements net::AddressList without struct addrinfo.

Reimplements net::AddressList without struct addrinfo.

net::AddressList extends std::vector<std::IPEndPoint> by canonical name. (Canonical name is planned to be removed as well.)

Removes dependency on sys_addrinfo.h throughout the codebase.

Introduces net::SockaddrStorage for convenience.

BUG=125696
TEST=green waterfall


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

[szym, 2012-05-02 06:26:01]

AddressList is used everywhere! PTAL before I seek reviews from OWNERS.

There's a few TODO(szym-addr) sprinkled here and there that I want to address
before landing. In particular, I think GetPeerAddress(AddressList*) should be
replaced with GetPeerAddress(IPEndPoint*) throughout the codebase.

[Do not use (sergeyu), 2012-05-02 17:54:45]

Can we encapsulate std::vector<> instead of inheriting from it? Inheriting from
STL classes always looks awkward. E.g. it would expose all the modifier methods
from std::vector<> that we don't really need in AddressList.

On 2012/05/02 06:26:01, szym wrote:
> AddressList is used everywhere! PTAL before I seek reviews from OWNERS.
> 
> There's a few TODO(szym-addr) sprinkled here and there that I want to address
> before landing. In particular, I think GetPeerAddress(AddressList*) should be
> replaced with GetPeerAddress(IPEndPoint*) throughout the codebase.

[szym, 2012-05-02 18:04:57]

On 2012/05/02 17:54:45, sergeyu1 wrote:
> Can we encapsulate std::vector<> instead of inheriting from it? Inheriting
from
> STL classes always looks awkward. E.g. it would expose all the modifier
methods
> from std::vector<> that we don't really need in AddressList.

I'm okay with that. (Extending vector without overriding clear() and swap()
seems like trouble on the horizon.) 

Would you expose operator[] or iterators though? If AddressList exposes
begin/end then TransportConnectJob::MakeAddressListStartWithIPv4 is as easy as:

std::rotate(list->begin(), i, list->end());

[Do not use (sergeyu), 2012-05-02 18:56:51]

On Wed, May 2, 2012 at 11:04 AM, <szym@chromium.org> wrote:

> On 2012/05/02 17:54:45, sergeyu1 wrote:
>
>> Can we encapsulate std::vector<> instead of inheriting from it? Inheriting
>>
> from
>
>> STL classes always looks awkward. E.g. it would expose all the modifier
>>
> methods
>
>> from std::vector<> that we don't really need in AddressList.
>>
>
> I'm okay with that. (Extending vector without overriding clear() and swap()
> seems like trouble on the horizon.)
>
> Would you expose operator[] or iterators though?


iterators - yes, but only const version, operator[] - probably no.


> If AddressList exposes
> begin/end then TransportConnectJob::**MakeAddressListStartWithIPv4 is as
> easy as:
>
> std::rotate(list->begin(), i, list->end());
>

I don't think that mutating the content of the list outside of the class is
a good idea. For that reason we shouldn't expose non-const interators. You
can still add a method in that class that would call std::rotate()
internally.


>
>
>
>
http://codereview.chromium.**org/10309002/<http://codereview.chromium.org/103...
>



-- 
Sergey Ulanov

[szym, 2012-05-02 19:12:00]

> iterators - yes, but only const version, operator[] - probably no.
>
> I don't think that mutating the content of the list outside of the class is
> a good idea. For that reason we shouldn't expose non-const interators. You
> can still add a method in that class that would call std::rotate()
> internally.

What is the benefit in sealing AddressList like that? There's only one
customer for std::rotate (TransportConnectJob), so I'd avoid adding it
to AddressList. In fact, I plan to remove AddressList::StartsWithIPv6,
::EnsurePort and ::GetPort for similar reasons. In general, I'd prefer
a lightweight and dumb struct.

[szym, 2012-05-02 19:21:34]

On 2012/05/02 19:12:00, szym wrote:
> I plan to remove AddressList::StartsWithIPv6,
> ::EnsurePort and ::GetPort for similar reasons. In general, I'd prefer
> a lightweight and dumb struct.

Whether GetPort/SetPort should belong to AddressList depends on whether we are
sticking to AddressList as vector of IPEndPoint (the de facto net::Address in
Chromium) or eventually convert it to list of IPAddressNumber.

If we go with the IPEndPoint, the AddressList(IPAddressList, port) constructor
would also be removed (only used in net/dns/dns_response.cc).

[eroman, 2012-05-02 19:57:45]

Before reviewing any of the other files I'd like to hammer out the API in
address_list.h

Could you describe what you expect the API to look like in the _final_
iteration?

Some initial comments:

(1) I don't like that AddressList inherits from std::vector<>. Please use
composition instead. How about a simple wrapper like this:

class AddressList {
 public:
 typedef std::vector<IPEndPoint> IPEndPoints;

 const IPEndPoints& list() const;
 IPEndPoints* mutable_list();

 private:
  IPEndPoints list_;
};

BTW, I strongly discourage trying to hide the std::vector as an implementation
detail (and doing fancy things like exposing a bunch of iterators). I think that
would undermine the benefits of what we are trying to do, which is make it
easier for clients to build and manipulate these lists!

(2) This is more stylistic so feel free to disregard. I really dislike
overloaded constructors, since I find it makes the code hard to read, and hard
to select the right constructor. I recommend only having a single constructor
for AddressList, and exclusively relying on factory methods for all the other
variants. This helps readability in my opinion since the name describes what it
is doing.

(3) Do you intend for the "canonical name" to be a part of the final version of
the API? I don't believe the canonical name belongs in AddressList (it was just
an oddity of addrinfo). Rather, I think in the future we can make it an explicit
out parameter of the HostResolver API.


Otherwise the overall approach here looksgood, and a lot of the calling code
already looks simpler.

[szym, 2012-05-02 20:21:25]

On 2012/05/02 19:57:45, eroman wrote:

> BTW, I strongly discourage trying to hide the std::vector as an implementation
> detail (and doing fancy things like exposing a bunch of iterators). I think
that
> would undermine the benefits of what we are trying to do, which is make it
> easier for clients to build and manipulate these lists!

Agreed on that one.

> Could you describe what you expect the API to look like in the _final_
> iteration?

I'm torn between IPEndPoint and IPAddress as the core building block. Although
redundant with port numbers, IPEndPoint is what ends up used eventually at most
call-sites and using that would make the overall change smaller.

Suppose we stick to IPEndPoint for now.

> (3) Do you intend for the "canonical name" to be a part of the final version
of
> the API? Rather, I think in the future we can make it an explicit
> out parameter of the HostResolver API.

After review, there's exactly one call site:
net/http/http_auth_handler_negotiate.cc so I'm all for removing canonical name
from AddressList. Internally to HostResolver*/HostCache implementations I'd
probably have a helper struct though.

Consequently, I believe AddressList should be:

typedef std::vector<IPEndPoint> AddressList;

...with the caveat below.

> (2) This is more stylistic so feel free to disregard. I really dislike
> overloaded constructors, since I find it makes the code hard to read, and hard
> to select the right constructor. I recommend only having a single constructor
> for AddressList, and exclusively relying on factory methods for all the other
> variants. This helps readability in my opinion since the name describes what
it
> is doing.

After initial surge to replace AddressList::CreateFromIPAddress with a
constructor, I realized how much more convenient it is than an equivalent
constructor, so I'm sold on that one. However, if AddressList is a typedef, then
there's no way to add static methods. On the other hand, if class AddressList :
public std::vector<IPEndPoint> {} ...

[eroman, 2012-05-02 20:29:48]

Alright I'm sold -- extending std::vector seems like a good strategy.

I won't worry about the cname and port questions right now, we can tackle those
in later iterations.

Should I give this a full review now?

[szym, 2012-05-02 20:32:58]

On 2012/05/02 20:21:25, szym wrote:
> if AddressList is a typedef, then
> there's no way to add static methods. On the other hand, if class AddressList
:
> public std::vector<IPEndPoint> {} ...

BTW, other advantages of dumb class extending std::vector:
1) disables all constructors but the default and copy.
2) can be forward declared.

And another "feature" which would probably need an explicit constructor or a
static conversion method.
*) can convert AddressList to vector<IPEndPoint> but not the other way.

[szym, 2012-05-03 19:45:18]

Ready for review. PTAL.

[eroman, 2012-05-04 01:08:41]

Generally looks good, nice simplifications. I want to see responses to my
comments marked by "IMPORTANT:" before signing off on this.

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy.cc (right):

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy.cc:1487: net::AddressList addrlist =
net::AddressList(endpoint);
nit: can avoid the assignment here and just do
AddressList addrlist(endpoint);

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dns/dns_api.cc (right):

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/extensions/a...
chrome/browser/extensions/api/dns/dns_api.cc:15: #include "net/base/net_util.h"
can this be removed?

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:152: const net::AddressList& GetAddressList()
const { return addresses_; }
might want to add a TODO to rename this to addresses(), since it is a simple
getter.

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:955: for
(net::AddressList::const_iterator addr_it = entry.addrlist.begin();
[optional] style nit: For vectors I actually much prefer the  pattern:

for (size_t i = 0; i < entry.addrlist.size(); ++i) {
  ...
}

You are well within your rights to do either way, just saying since I find the
iterator approach less readable.

http://codereview.chromium.org/10309002/diff/5163/content/browser/renderer_ho...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

http://codereview.chromium.org/10309002/diff/5163/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:75: if
(addresses_.empty()) {
Yuck.

I am surprised to keep seeing these checks...

HostResolver returns an error when it fails to resolve. The API specification is
pretty ambiguous about what the result is in this case (it only says it is set
on success). Feels like a bit of an abuse for code to be relying on this
pattern.

Actually in this case it seems like this check is unnecessary, I see no reason
why HostResolver would return an empty result on success. In fact maybe that
should specifically be part of the contract do avoid these sorts of checks all
over the place.

http://codereview.chromium.org/10309002/diff/5163/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/10309002/diff/5163/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:566:
ppapi::CreateNetAddressListFromAddressList(addresses));
This sounds weird. I don't know about this code, but seems unusual. I wander if
pepper really needs these wrappers. I'm OK with it for this refactor if the
OWNER signs off on it.

http://codereview.chromium.org/10309002/diff/5163/jingle/notifier/communicato...
File jingle/notifier/communicator/xmpp_connection_generator.cc (right):

http://codereview.chromium.org/10309002/diff/5163/jingle/notifier/communicato...
jingle/notifier/communicator/xmpp_connection_generator.cc:159: for
(net::AddressList::const_iterator addr = address_list_.begin();
Same comment as before. I recommend using an integer index for readability. But
maybe I am just a STL/C++ hater...

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.cc
File net/base/address_list.cc (left):

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.cc#ol...
net/base/address_list.cc:37: memcpy(&addr->sin_addr, &address[0],
kIPv4AddressSize);
awzum how much code is getting deleted from this file... good riddance :-)

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.cc
File net/base/address_list.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.cc#ne...
net/base/address_list.cc:50: // NOTE: Ignoring non-INET* families.
Not likely an issue, but might be good to add a warning just in case we need to
debug.

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.h
File net/base/address_list.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.h#new...
net/base/address_list.h:36: static AddressList CreateFromAddrinfo(const struct
addrinfo* head);
Might be worth mentioning that |head| is copied and doesn't have to outlive the
class.

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.h#new...
net/base/address_list.h:47: void SetDefaultCanonicalName();
Please add a comment for this, explaining how it uses the IP address of front of
list.

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list_net_l...
File net/base/address_list_net_log_param.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list_net_l...
net/base/address_list_net_log_param.cc:20: for (AddressList::const_iterator iter
= address_list_.begin();
[optional] consider integer iterator.

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list_unitt...
File net/base/address_list_unittest.cc (left):

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list_unitt...
net/base/address_list_unittest.cc:13: #include "net/base/winsock_init.h"
I wander if any of these headers can be removed now?

http://codereview.chromium.org/10309002/diff/5163/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/host_port_pair.cc#...
net/base/host_port_pair.cc:13: #include "net/base/net_util.h"
Can this be deleted now?

http://codereview.chromium.org/10309002/diff/5163/net/base/host_port_pair.h
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/host_port_pair.h#n...
net/base/host_port_pair.h:13: struct addrinfo;
Delete this.

http://codereview.chromium.org/10309002/diff/5163/net/base/host_port_pair.h#n...
net/base/host_port_pair.h:29: // Creates a HostPortPair from an addrinfo struct.
Update the comment.

http://codereview.chromium.org/10309002/diff/5163/net/base/host_resolver_impl...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/host_resolver_impl...
net/base/host_resolver_impl_unittest.cc:272: return std::find(list_.begin(),
nice simplification.

http://codereview.chromium.org/10309002/diff/5163/net/base/host_resolver_proc.h
File net/base/host_resolver_proc.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/host_resolver_proc...
net/base/host_resolver_proc.h:20: void NET_EXPORT SetPortOnAddressList(uint16
port, AddressList* list);
This doesn't feel like the right place to house this code.

Maybe address_list.h would be a better fit for this first function. The second
function is a little bit too specific on how the lists were constructed, so
doesn't fit as cleanly into address_list.h

This is probably more appropriate in address_list.h, or part of
host_resolver_impl.cc which is where it is used. (who else uses this?)

I dunno; net_util.h ends up being our dumping ground for these sorts of things.

http://codereview.chromium.org/10309002/diff/5163/net/base/ip_endpoint.cc
File net/base/ip_endpoint.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/ip_endpoint.cc#new...
net/base/ip_endpoint.cc:126: return NetAddressToString(storage.addr,
storage.addr_len);
We should change this at some point to not use sockaddrs. Note that GURL has the
capability to print a string literal given an IP address, and I think that would
be a better fit. Then again, maybe I am biased since I wrote GURL's
canonicalizer for IPv6 literals .... :)

http://codereview.chromium.org/10309002/diff/5163/net/base/mapped_host_resolv...
File net/base/mapped_host_resolver_unittest.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/mapped_host_resolv...
net/base/mapped_host_resolver_unittest.cc:85: EXPECT_EQ("192.168.1.11:99",
FirstAddress(address_list));
nice cleanup.

http://codereview.chromium.org/10309002/diff/5163/net/base/net_util.h
File net/base/net_util.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/net_util.h#newcode103
net/base/net_util.h:103: struct SockaddrStorage {
nice.

http://codereview.chromium.org/10309002/diff/5163/net/base/net_util_unittest.cc
File net/base/net_util_unittest.cc (left):

http://codereview.chromium.org/10309002/diff/5163/net/base/net_util_unittest....
net/base/net_util_unittest.cc:2262: TEST(NetUtilTest, NetAddressToString_IPv4) {
IMPORTANT: Why are all these tests being removed? It doesn't seem like this is
desirable.

Rather these should be changed to test the overloaded version of
NetAddressToString (using sockaddr).

http://codereview.chromium.org/10309002/diff/5163/net/curvecp/client_packetiz...
File net/curvecp/client_packetizer.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/curvecp/client_packetiz...
net/curvecp/client_packetizer.cc:300: current_address_index_ =
(current_address_index_ + 1) % addresses_.size();
IMPORTANT: This isn't a direct translation. We are now starting at index 1
whereas before we were starting at index 0. I suggest making the index a signed
"int", and initializing it to -1. Then things should work as intended.

http://codereview.chromium.org/10309002/diff/5163/net/dns/dns_response_unitte...
File net/dns/dns_response_unittest.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:252: for (size_t i = 0; i < addrlist.size();
++i) {
[optional] nit: this is inconsistent with the other locations which use
iterators. I don't care either way, my preference is  to use size_t throughout.

http://codereview.chromium.org/10309002/diff/5163/net/ftp/ftp_network_transac...
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction.cc:657: response_.socket_address =
HostPortPair::FromIPEndPoint(address[0]);
Here for instance code assumes that on success AddressList is non-empty, which I
believe is a correct assumption and what code should be doing. It is interesting
though that other places have conditionals to check for .empty().

I guess this case is irrelevant once you convert GetPeerAddress -->
GetPeerIPEndpoint anyway, just using it as an example :)

http://codereview.chromium.org/10309002/diff/5163/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction.cc:1197: data_address =
AddressList::CreateFromIPAddress(
This is a bit awkward. How about just calling the set port directly, or setting
port on front()?

I guess doesn't really matter since GetPeerAddress() should be going away.

http://codereview.chromium.org/10309002/diff/5163/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/http/http_stream_parser...
net/http/http_stream_parser.cc:216: response_->socket_address =
HostPortPair::FromIPEndPoint(address[0]);
[optional] nit: I seem to recall some places using address.front() and others
address[0]. Might be good to use one style throughout, although really doesn't
matter so feel free to disregard.

My preference is for front()

http://codereview.chromium.org/10309002/diff/5163/net/socket/socks_client_soc...
File net/socket/socks_client_socket.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/socks_client_soc...
net/socket/socks_client_socket.cc:318: memcpy(&request.ip, &ipe.address()[0],
ipe.address().size());
I'm not sure how I feel about this... The assumption is IPAddressNumber stores
things in network order.

I would rather explicitly convert this to sockaddr, and then memcpy using that
as the source.

http://codereview.chromium.org/10309002/diff/5163/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:885: peer_id +=
std::string(reinterpret_cast<const char*>(&endpoint.address()[0]),
Same comment as earlier, these sorts of translations worry me a bit. I would
rather explicitly convert to sockaddr and only use that in places which are
expecting some sort of ordering.

http://codereview.chromium.org/10309002/diff/5163/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1026: if
(!peer_address[0].ToSockAddr(storage.addr, &storage.addr_len))
nit: [0] vs .front().

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:173: if (current_address_index_ <
addresses_.size() || bind_address_.get()) {
If you do my recommendation to change index to an "int", this will need to be
expanded to check >= 0 as well.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:260: DCHECK_GE(current_address_index_,
0u);
nit: trivially evaluates to true since index is unsigned. If you follow my
recommendation to make it signed though, this will become necessary.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:300: if
(!endpoint.ToSockAddr(storage.addr, &storage.addr_len))
nit: This is duplicated in two locations. How about extracting this to the top
and always converting endpoint to a sockaddr.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:352: if (current_address_index_ + 1 <
addresses_.size()) {
The reason I don't like current_address_index being unsigned and initialized to
-1, is code like this can pass unexpectedly (i.e. -1 + 1 = 0, which is then less
than addresses_.size()). Believe it or not I have seen bugs of this sort before
so I am always cautious about unsigned arithmetic.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:366: current_address_index_ =
addresses_.size();
This seems unnecessary... I would just delete it if it isn't clear that it is
needed. (I suspect before this was an attempt to make sure the pointer isn't
accidentally used after it may have been released?)

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:511: if
(!addresses_[current_address_index_].ToSockAddr(storage.addr,
I have seen this in a couple places now. I think it can be simplified. We should
only ever call this once.

Here is how:

Inside of DoConnect() save the sockaddr to a member variable called
current_sockaddr_. Everywhere else that needs the sockaddr for current address
would use that.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.h:158: // Where we are in above list. Set
to -1 if uninitialized.
I don't like -1 for unsigned values. Please make this an int.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_unittest.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_unittest.cc:34: AddressList(server_address),
Fits on one line now.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_unittest.cc:90: AddressList(server_address),
One line dude.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:464: connect_os_error_ =
CreateSocket(endpoint.GetFamily(), &socket_);
Side comment: When we refactor GetFamily() to return an enum rather than int we
need to be very careful about callsites like this, since they won't result in a
compile break. Might be worth changing the name while doing the refactor to
catch this.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:485: return ERR_INVALID_ARGUMENT;
I wander if these failure are possible? When building AddressList we should be
very explicit about what values are put in. I guess I need to read ToSockAddr to
see the failure cases.

Would be nice if this function didn't have any failures to check, and perhaps
just had a crash assertion if something really wonky happened.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:546: current_address_index_ =
addresses_.size();
Same comment as earlier, I wander if this is actually necessary.

The only case I can see where this matters if if someone calls GetPeerAddress()
after disconnecting the socket.

However as this code stands, it will already early return based on not being
connected.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.h:116: // Where we are in above list. Set to -1
if uninitialized.
Same comment as for the other socket file. I strongly recommend using signed
integer.

http://codereview.chromium.org/10309002/diff/5163/net/socket/transport_client...
File net/socket/transport_client_socket_pool.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/transport_client...
net/socket/transport_client_socket_pool.cc:116: std::rotate(list->begin(), i,
list->end());
Hmm I'll need to lookup rotate and think about this...

http://codereview.chromium.org/10309002/diff/5163/net/socket/transport_client...
File net/socket/transport_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/transport_client...
net/socket/transport_client_socket_pool_unittest.cc:480:
addrlist.push_back(addrlist_v6_1);
I very quickly skimmed through this file... will try to read it fully later.

http://codereview.chromium.org/10309002/diff/5163/net/socket_stream/socket_st...
File net/socket_stream/socket_stream.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket_stream/socket_st...
net/socket_stream/socket_stream.h:270: void CopyAddressList(const AddressList&
addresses);
nit: I wander if "set_addresses()" would be more appropriate name.

http://codereview.chromium.org/10309002/diff/5163/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:257: response_info_->socket_address =
HostPortPair::FromIPEndPoint(address[0]);
nit: [0] vs front(). I guess front() might make it spill onto two lines here ...

http://codereview.chromium.org/10309002/diff/5163/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:404: addr = NULL;
IMPORTANT: This translations scares me since there is drift between |addr| and
|storate.addr|, which is just an invitation for problems :) Either make |addr|
be a reference, or replace all occurrences of |addr| --> |storage.addr|.

http://codereview.chromium.org/10309002/diff/5163/net/websockets/websocket_th...
File net/websockets/websocket_throttle_unittest.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/websockets/websocket_th...
net/websockets/websocket_throttle_unittest.cc:70: addr.push_back(MakeAddr(1, 2,
3, 4));
side-comment: would be clearer if code just used an IP literal and parsed it to
an IPAddressNumber inside the helper. Don't bother changing as part of this
refactor tho.

http://codereview.chromium.org/10309002/diff/5163/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_host_resolver_shared.h (right):

http://codereview.chromium.org/10309002/diff/5163/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_host_resolver_shared.h:20: };
remove semicolon.

[szym, 2012-05-04 02:38:29]

Thanks a lot for the quick review.

A few concerns:

1) I think we should explicitly specify that IPAddressNumber is in network order
(i.e., same as sockaddr). Platform APIs are inconsistent so it'd be nice if we
could just poke at IPAddressNumber bytes.

2) I changed IPEndPoint::ToSockAddr to not crash with NOTREACHED when called on
non-INET* address but WARN_UNUSED_RESULT instead. (Even though
AddressList::CreateFromAddrinfo ignores non-INET* stuff.) I've found that most
callers do not ignore the result at this time. In a future CL, it might make
sense to change ToSockAddr to crash.

3) I need to either add freeaddrinfo after CreateFromAddrinfo (in
HostResolverProc and one test) or change the API to own it and free it. Any
preference?

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.h
File net/base/address_list.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list.h#new...
net/base/address_list.h:36: static AddressList CreateFromAddrinfo(const struct
addrinfo* head);
On 2012/05/04 01:08:41, eroman wrote:
> Might be worth mentioning that |head| is copied and doesn't have to outlive
the
> class.

Good point. This means I forgot to freeaddrinfo everywhere.

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list_unitt...
File net/base/address_list_unittest.cc (left):

http://codereview.chromium.org/10309002/diff/5163/net/base/address_list_unitt...
net/base/address_list_unittest.cc:13: #include "net/base/winsock_init.h"
On 2012/05/04 01:08:41, eroman wrote:
> I wander if any of these headers can be removed now?

I'll remove winsock_init, but now I see I need to add a test for
CreateFromAddrinfo which takes a longer list. I dislike the idea of using
SystemHostResolverProc to generate addrinfo, so I'll just assemble one manually.

http://codereview.chromium.org/10309002/diff/5163/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/host_port_pair.cc#...
net/base/host_port_pair.cc:13: #include "net/base/net_util.h"
On 2012/05/04 01:08:41, eroman wrote:
> Can this be deleted now?

Yes!

http://codereview.chromium.org/10309002/diff/5163/net/base/host_resolver_proc.h
File net/base/host_resolver_proc.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/host_resolver_proc...
net/base/host_resolver_proc.h:20: void NET_EXPORT SetPortOnAddressList(uint16
port, AddressList* list);
On 2012/05/04 01:08:41, eroman wrote:
> This is probably more appropriate in address_list.h, or part of
> host_resolver_impl.cc which is where it is used. (who else uses this?)
> 
> I dunno; net_util.h ends up being our dumping ground for these sorts of
things.

Only HostResolver-related code uses this, but this includes
mock_host_resolver.cc
On the other hand, there is little benefit from such optimization in
MockHostResolver, so I'll move EnsurePort... to host_resolver_impl.cc and
SetPort... to address_list.cc

http://codereview.chromium.org/10309002/diff/5163/net/base/ip_endpoint.cc
File net/base/ip_endpoint.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/ip_endpoint.cc#new...
net/base/ip_endpoint.cc:126: return NetAddressToString(storage.addr,
storage.addr_len);
On 2012/05/04 01:08:41, eroman wrote:
> We should change this at some point to not use sockaddrs. Note that GURL has
the
> capability to print a string literal given an IP address, and I think that
would
> be a better fit. Then again, maybe I am biased since I wrote GURL's
> canonicalizer for IPv6 literals .... :)

On the other hand, having to call getnameinfo to print an IP address sucks. I'll
file a bug.

http://codereview.chromium.org/10309002/diff/5163/net/base/net_util_unittest.cc
File net/base/net_util_unittest.cc (left):

http://codereview.chromium.org/10309002/diff/5163/net/base/net_util_unittest....
net/base/net_util_unittest.cc:2262: TEST(NetUtilTest, NetAddressToString_IPv4) {
On 2012/05/04 01:08:41, eroman wrote:
> IMPORTANT: Why are all these tests being removed? It doesn't seem like this is
> desirable.
> 
> Rather these should be changed to test the overloaded version of
> NetAddressToString (using sockaddr).

Agreed. I didn't notice that NetAddressToString(struct sockaddr*) is untested.
I'll convert it.

http://codereview.chromium.org/10309002/diff/5163/net/curvecp/client_packetiz...
File net/curvecp/client_packetizer.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/curvecp/client_packetiz...
net/curvecp/client_packetizer.cc:300: current_address_index_ =
(current_address_index_ + 1) % addresses_.size();
On 2012/05/04 01:08:41, eroman wrote:
> IMPORTANT: This isn't a direct translation. We are now starting at index 1
> whereas before we were starting at index 0. I suggest making the index a
signed
> "int", and initializing it to -1. Then things should work as intended.

Good catch!

http://codereview.chromium.org/10309002/diff/5163/net/dns/dns_response_unitte...
File net/dns/dns_response_unittest.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:252: for (size_t i = 0; i < addrlist.size();
++i) {
On 2012/05/04 01:08:41, eroman wrote:
> [optional] nit: this is inconsistent with the other locations which use
> iterators. I don't care either way, my preference is  to use size_t
throughout.

I defaulted to const_iterator where a while loop was used. I'll probably convert
those other places to size_t i for shortness.

http://codereview.chromium.org/10309002/diff/5163/net/ftp/ftp_network_transac...
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction.cc:657: response_.socket_address =
HostPortPair::FromIPEndPoint(address[0]);
On 2012/05/04 01:08:41, eroman wrote:
> Here for instance code assumes that on success AddressList is non-empty, which
I
> believe is a correct assumption and what code should be doing. It is
interesting
> though that other places have conditionals to check for .empty().

Both HostResolverImpl and its tests assume that OK implies non empty result.
I'll leave the "if (empty())" cleanup for the next CL.

> I guess this case is irrelevant once you convert GetPeerAddress -->
> GetPeerIPEndpoint anyway, just using it as an example :)

Yes, although I think GetPeerAddress will keep its name. The updated API would
match GetLocalAddress(IPEndPoint*).

http://codereview.chromium.org/10309002/diff/5163/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction.cc:1197: data_address =
AddressList::CreateFromIPAddress(
On 2012/05/04 01:08:41, eroman wrote:
> This is a bit awkward. How about just calling the set port directly, or
setting
> port on front()?

The awkwardness comes from the immutability of IPEndPoint. I considered adding
IPEndPoint::set_port, but there are only 2 places that would use it
(HostResolverImpl and here).

> I guess doesn't really matter since GetPeerAddress() should be going away.

It will still be:
IPEndPoint(endpoint.address(), data_connection_port_);

http://codereview.chromium.org/10309002/diff/5163/net/socket/socks_client_soc...
File net/socket/socks_client_socket.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/socks_client_soc...
net/socket/socks_client_socket.cc:318: memcpy(&request.ip, &ipe.address()[0],
ipe.address().size());
On 2012/05/04 01:08:41, eroman wrote:
> I'm not sure how I feel about this... The assumption is IPAddressNumber stores
> things in network order.
> 
> I would rather explicitly convert this to sockaddr, and then memcpy using that
> as the source.

Currently, IPAddressNumber stores data in same order as sockaddr. Do you suggest
we should treat IPAddressNumber as opaque and always convert to sockaddr and
using platform APIs for answering questions like "is this 127.0.0.1"?

I think we'd be better off specifying that order in IPAddressNumber is same as
in sockaddr (i.e., network order). IPEndPoint::{To,From}SockAddr already assume
that. I'm not even sure how the order could be different.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:260: DCHECK_GE(current_address_index_,
0u);
On 2012/05/04 01:08:41, eroman wrote:
> nit: trivially evaluates to true since index is unsigned. If you follow my
> recommendation to make it signed though, this will become necessary.

True. This is a leftover of my first attempt with signed int.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:366: current_address_index_ =
addresses_.size();
On 2012/05/04 01:08:41, eroman wrote:
> This seems unnecessary... I would just delete it if it isn't clear that it is
> needed. (I suspect before this was an attempt to make sure the pointer isn't
> accidentally used after it may have been released?)

I was concerned that it was used to distinguish sockets that were disconnected,
but it seems there are other checks for that.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:511: if
(!addresses_[current_address_index_].ToSockAddr(storage.addr,
On 2012/05/04 01:08:41, eroman wrote:
> I have seen this in a couple places now. I think it can be simplified. We
should
> only ever call this once.
> 
> Here is how:
> 
> Inside of DoConnect() save the sockaddr to a member variable called
> current_sockaddr_. Everywhere else that needs the sockaddr for current address
> would use that.

Not sure. |current_sockaddr_| would need to be a SockaddrStorage and it'd only
get used in two places.

On the other hand, not sure if really need to check ToSockAddr result here,
since non-INET* should already be filtered out in AddressList.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.h:158: // Where we are in above list. Set
to -1 if uninitialized.
On 2012/05/04 01:08:41, eroman wrote:
> I don't like -1 for unsigned values. Please make this an int.

Will do. That was my initial attempt, but then go lazy with static_cast<int>()
for all DCHECKs and comparisons.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:464: connect_os_error_ =
CreateSocket(endpoint.GetFamily(), &socket_);
On 2012/05/04 01:08:41, eroman wrote:
> Side comment: When we refactor GetFamily() to return an enum rather than int
we
> need to be very careful about callsites like this, since they won't result in
a
> compile break. Might be worth changing the name while doing the refactor to
> catch this.

Yup. To avoid this pitfall when converting RequestPriority order I changed that
enum to a class to catch all call sites.

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:485: return ERR_INVALID_ARGUMENT;
On 2012/05/04 01:08:41, eroman wrote:
> I wander if these failure are possible? When building AddressList we should be
> very explicit about what values are put in. I guess I need to read ToSockAddr
to
> see the failure cases.
> 
> Would be nice if this function didn't have any failures to check, and perhaps
> just had a crash assertion if something really wonky happened.

Agreed that ToSockAddr should ideally not return false. But even if we ignore
non-INET* junk, IPEndPoint could be in one of three states: empty, INET, INET6.
IPEndPoint().ToSockAddr(...) ?

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:546: current_address_index_ =
addresses_.size();
On 2012/05/04 01:08:41, eroman wrote:
> Same comment as earlier, I wander if this is actually necessary.
> 
> The only case I can see where this matters if if someone calls
GetPeerAddress()
> after disconnecting the socket.
> 
> However as this code stands, it will already early return based on not being
> connected.

Agreed. Will fix.

http://codereview.chromium.org/10309002/diff/5163/net/socket_stream/socket_st...
File net/socket_stream/socket_stream.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket_stream/socket_st...
net/socket_stream/socket_stream.h:270: void CopyAddressList(const AddressList&
addresses);
On 2012/05/04 01:08:41, eroman wrote:
> nit: I wander if "set_addresses()" would be more appropriate name.

It definitely would. Will fix.

http://codereview.chromium.org/10309002/diff/5163/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:404: addr = NULL;
On 2012/05/04 01:08:41, eroman wrote:
> IMPORTANT: This translations scares me since there is drift between |addr| and
> |storate.addr|, which is just an invitation for problems :) Either make |addr|
> be a reference, or replace all occurrences of |addr| --> |storage.addr|.

This is the only exception to using storage.addr. I need to check if passing
non-NULL sockaddr with 0-length to WSASendTo will behave as passing NULL would.
If it does, I'll go back to storage.addr.

[eroman, 2012-05-04 04:20:21]

http://codereview.chromium.org/10309002/diff/5163/net/base/host_resolver_proc.h
File net/base/host_resolver_proc.h (right):

http://codereview.chromium.org/10309002/diff/5163/net/base/host_resolver_proc...
net/base/host_resolver_proc.h:20: void NET_EXPORT SetPortOnAddressList(uint16
port, AddressList* list);
On 2012/05/04 02:38:29, szym wrote:
> On 2012/05/04 01:08:41, eroman wrote:
> > This is probably more appropriate in address_list.h, or part of
> > host_resolver_impl.cc which is where it is used. (who else uses this?)
> > 
> > I dunno; net_util.h ends up being our dumping ground for these sorts of
> things.
> 
> Only HostResolver-related code uses this, but this includes
> mock_host_resolver.cc
> On the other hand, there is little benefit from such optimization in
> MockHostResolver, so I'll move EnsurePort... to host_resolver_impl.cc and
> SetPort... to address_list.cc

Sounds like a good plan.

http://codereview.chromium.org/10309002/diff/5163/net/ftp/ftp_network_transac...
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction.cc:1197: data_address =
AddressList::CreateFromIPAddress(
On 2012/05/04 02:38:29, szym wrote:
> On 2012/05/04 01:08:41, eroman wrote:
> > This is a bit awkward. How about just calling the set port directly, or
> setting
> > port on front()?
> 
> The awkwardness comes from the immutability of IPEndPoint. I considered adding
> IPEndPoint::set_port, but there are only 2 places that would use it
> (HostResolverImpl and here).
> 
> > I guess doesn't really matter since GetPeerAddress() should be going away.
> 
> It will still be:
> IPEndPoint(endpoint.address(), data_connection_port_);

Ah. In that case I agree this is fine; no point changing the immutability at
this time. (Although if it complicates other stuff, seems like adding a simple
set_port() method wouldn't be a big deal).

http://codereview.chromium.org/10309002/diff/5163/net/socket/socks_client_soc...
File net/socket/socks_client_socket.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/socks_client_soc...
net/socket/socks_client_socket.cc:318: memcpy(&request.ip, &ipe.address()[0],
ipe.address().size());
On 2012/05/04 02:38:29, szym wrote:
> On 2012/05/04 01:08:41, eroman wrote:
> > I'm not sure how I feel about this... The assumption is IPAddressNumber
stores
> > things in network order.
> > 
> > I would rather explicitly convert this to sockaddr, and then memcpy using
that
> > as the source.
> 
> Currently, IPAddressNumber stores data in same order as sockaddr. Do you
suggest
> we should treat IPAddressNumber as opaque and always convert to sockaddr and
> using platform APIs for answering questions like "is this 127.0.0.1"?
> 
> I think we'd be better off specifying that order in IPAddressNumber is same as
> in sockaddr (i.e., network order). IPEndPoint::{To,From}SockAddr already
assume
> that. I'm not even sure how the order could be different.

My paranoia stems from doing a memcpy() from a source of unknown size.
I guess I would be placated if you could add something like this:

CHECK_LE(ip.address().size(), sizeof(request.ip));

the "CHECK_EQ(AF_INET, ipe.GetFamily())" above should presumably cover this as
well, but I think a more direct test is worthwhile.

[szym, 2012-05-04 05:27:55]

Fixed all but the TCP connect address duplicate. Filed bugs for future CLs.
Ready for re-review.

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/10309002/diff/5163/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:152: const net::AddressList& GetAddressList()
const { return addresses_; }
On 2012/05/04 01:08:41, eroman wrote:
> might want to add a TODO to rename this to addresses(), since it is a simple
> getter.

Actually, renamed to get_address_list (only used in network_stats.cc)

[szym, 2012-05-04 18:18:28]

Reasons for changes in PatchSet 8:

1) address_list.h: changed to AddressList : _private_ std::vector... This
prevents cast of AddressList* to vector*, and limits the inherited public
members to those explicitly declared with "using".

2) sock_test_util.h: changed MockTCPClientSocket::addresses() to return
const-ref to AddressList. Before, FtpNetworkTransactionTest failed with
"iterators incompatible" on win_rel.

3) dns_config_service.h: Added #include address_list.h to prevent "LNK1169: one
or more multiply defined symbols" on MSVS2010 shared build. I'm not sure why
this is happening and I'm very troubled by this, but this added #include seems
to work.

[eroman, 2012-05-04 19:37:35]

lgtm

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:366: current_address_index_ =
addresses_.size();
On 2012/05/04 02:38:29, szym wrote:
> On 2012/05/04 01:08:41, eroman wrote:
> > This seems unnecessary... I would just delete it if it isn't clear that it
is
> > needed. (I suspect before this was an attempt to make sure the pointer isn't
> > accidentally used after it may have been released?)
> 
> I was concerned that it was used to distinguish sockets that were
disconnected,
> but it seems there are other checks for that.

Actually sorry, I changed my mind!

I think it would be best to leave this as a reset to -1 (same as initial value)
to be safe.

It would be more appropriate to change that in a follow up CL, since it would be
a shame to have unnecessary fallout from this refactor.

Sorry for the extra work! (You can disregard this if you like).

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/10309002/diff/5163/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:546: current_address_index_ =
addresses_.size();
On 2012/05/04 02:38:29, szym wrote:
> On 2012/05/04 01:08:41, eroman wrote:
> > Same comment as earlier, I wander if this is actually necessary.
> > 
> > The only case I can see where this matters if if someone calls
> GetPeerAddress()
> > after disconnecting the socket.
> > 
> > However as this code stands, it will already early return based on not being
> > connected.
> 
> Agreed. Will fix.

Sorry, I suggest leaving this in place, as a reset to -1.

http://codereview.chromium.org/10309002/diff/71002/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10309002/diff/71002/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.cc:258: HostPortPair::FromIPEndPoint(address.front());
nit: indent continued lines by 4.

http://codereview.chromium.org/10309002/diff/70008/chrome/browser/net/network...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/10309002/diff/70008/chrome/browser/net/network...
chrome/browser/net/network_stats.h:154: const net::AddressList&
get_address_list() const { return addresses_; }
Why not call this addresses()?

our hacker style for accessors is to name it the same as the private variable
(without the trailing underscore). For setters you prefix with "set_". See for
instance the other accessors in this header: "load_size(), "start_time".

http://codereview.chromium.org/10309002/diff/70008/net/base/address_list.h
File net/base/address_list.h (right):

http://codereview.chromium.org/10309002/diff/70008/net/base/address_list.h#ne...
net/base/address_list.h:61: using std::vector<IPEndPoint>::end;
What selection criteria did you use for these?
For instance some other methods that could safely be exposed:

erase
insert
back
rbegin
rend
reserve
capacity

The problematic methods that I can imagine are:

assign
swap
operator=

Assignment is confusing since this is an *augmented* std::vector, so there are
more fields that describe our identity.

I am curious what happens with operator= right now. Does it just do the right
thing (i.e. copy canonical name + the std::vector)? Or does it result in a
compile failure?

http://codereview.chromium.org/10309002/diff/70008/net/dns/dns_config_service.h
File net/dns/dns_config_service.h (right):

http://codereview.chromium.org/10309002/diff/70008/net/dns/dns_config_service...
net/dns/dns_config_service.h:19: // std::vector<IPEndPoint>.
dunno what that is about...

[szym, 2012-05-04 20:20:21]

Thanks for review.

http://codereview.chromium.org/10309002/diff/70008/chrome/browser/net/network...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/10309002/diff/70008/chrome/browser/net/network...
chrome/browser/net/network_stats.h:154: const net::AddressList&
get_address_list() const { return addresses_; }
On 2012/05/04 19:37:36, eroman wrote:
> Why not call this addresses()?

Of course! Wasn't thinking.

http://codereview.chromium.org/10309002/diff/70008/net/base/address_list.h
File net/base/address_list.h (right):

http://codereview.chromium.org/10309002/diff/70008/net/base/address_list.h#ne...
net/base/address_list.h:61: using std::vector<IPEndPoint>::end;
On 2012/05/04 19:37:36, eroman wrote:
> What selection criteria did you use for these?

Whatever was already in use (needed to compile).

> For instance some other methods that could safely be exposed:
> 
> erase
> insert
> back
> rbegin
> rend
> reserve
> capacity

Agreed.
 
> I am curious what happens with operator= right now. Does it just do the right
> thing (i.e. copy canonical name + the std::vector)? Or does it result in a
> compile failure?

I was expecting a compile failure, but it seems that all our compilers generate
a sane default AddressList::operator= which properly assigns the private base
and all members. I added a test for operator=.

[szym, 2012-05-04 20:20:30]

Thanks for review.

[szym, 2012-05-04 20:25:23]

eroman: Thanks a lot for review!

sergeyu: PTAL changes in remoting/, content/browser/renderer_host/p2p/, and
optionally jingle/.

sky: PTAL changes in chrome/browser/ but not chrome/browser/net/ (already
covered by eroman)

viettrungluu: PTAL changes in ppapi/

[szym, 2012-05-04 20:27:02]

On 2012/05/04 20:25:23, szym wrote:
> sky: PTAL changes in chrome/browser/ but not chrome/browser/net/ 

sky: I meant, content/browser/ but not content/browser/renderer_host/p2p/

[sky, 2012-05-04 20:54:13]

LGTM

[Sergey Ulanov, 2012-05-04 23:47:17]

remoting/ - LGTM

[viettrungluu, 2012-05-07 19:48:04]

ppapi & pepper_message_filter.cc LGTM.

[commit-bot: I haz the power, 2012-05-07 19:53:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/10309002/18090

[commit-bot: I haz the power, 2012-05-07 21:45:06]

Change committed as 135731