Make a proper TCPSocketParams

Make a proper TCPSocketParams

BUG=none
TEST=existing unit tests

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

[willchan no longer on Chromium, 2010-02-26 21:12:48]

LGTM.  Thanks for the cleanup!

http://codereview.chromium.org/661194/diff/1/5
File net/socket/tcp_client_socket_pool.h (right):

http://codereview.chromium.org/661194/diff/1/5#newcode37
net/socket/tcp_client_socket_pool.h:37: HostResolver::RequestInfo destination_;
I know we have many instances of this in our code (and I've gradually been
trying to fix them), but it's against the style guide to have public member
variables in classes.  You might want to make this a struct instead and rename
the member variable to get rid of the trailing underscore.  Otherwise, you can
make this private and use accessors.

http://codereview.chromium.org/661194/diff/1/6
File net/socket/tcp_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/661194/diff/1/6#newcode18
net/socket/tcp_client_socket_pool_unittest.cc:18: #include
"net/socket/tcp_client_socket_pool.h"
It's already included at the top.

http://codereview.chromium.org/661194/diff/1/9
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/661194/diff/1/9#newcode28
net/spdy/spdy_session.h:28: #include "testing/platform_test.h"
I see that you just sorted this.  I'm surprised we have a testing include in a
production code file.  Do you think we could remove this?

[vandebo (ex-Chrome), 2010-02-26 21:46:22]

Thanks for the review.

Waiting for try bots to commit.

http://codereview.chromium.org/661194/diff/1/5
File net/socket/tcp_client_socket_pool.h (right):

http://codereview.chromium.org/661194/diff/1/5#newcode37
net/socket/tcp_client_socket_pool.h:37: HostResolver::RequestInfo destination_;
On 2010/02/26 21:12:48, willchan wrote:
> I know we have many instances of this in our code (and I've gradually been
> trying to fix them), but it's against the style guide to have public member
> variables in classes.  You might want to make this a struct instead and rename
> the member variable to get rid of the trailing underscore.  Otherwise, you can
> make this private and use accessors.

Wouldn't it also be against the style guide to have this constructor with a
struct?

Changed to an accessor.

http://codereview.chromium.org/661194/diff/1/6
File net/socket/tcp_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/661194/diff/1/6#newcode18
net/socket/tcp_client_socket_pool_unittest.cc:18: #include
"net/socket/tcp_client_socket_pool.h"
On 2010/02/26 21:12:48, willchan wrote:
> It's already included at the top.

Derr, indeed. Done

http://codereview.chromium.org/661194/diff/1/9
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/661194/diff/1/9#newcode28
net/spdy/spdy_session.h:28: #include "testing/platform_test.h"
On 2010/02/26 21:12:48, willchan wrote:
> I see that you just sorted this.  I'm surprised we have a testing include in a
> production code file.  Do you think we could remove this?

Seems to work fine without it.

[willchan no longer on Chromium, 2010-02-26 22:10:20]

http://codereview.chromium.org/661194/diff/1/5
File net/socket/tcp_client_socket_pool.h (right):

http://codereview.chromium.org/661194/diff/1/5#newcode37
net/socket/tcp_client_socket_pool.h:37: HostResolver::RequestInfo destination_;
On 2010/02/26 21:46:22, vandebo wrote:
> On 2010/02/26 21:12:48, willchan wrote:
> > I know we have many instances of this in our code (and I've gradually been
> > trying to fix them), but it's against the style guide to have public member
> > variables in classes.  You might want to make this a struct instead and
rename
> > the member variable to get rid of the trailing underscore.  Otherwise, you
can
> > make this private and use accessors.
> 
> Wouldn't it also be against the style guide to have this constructor with a
> struct?

From
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...:

"Methods should not provide behavior but should only be used to set up the data
members, e.g., constructor, destructor, Initialize(), Reset(), Validate()."

This constructor seems to only be initializing the data.  There doesn't seem to
be any other behavior provided.

> 
> Changed to an accessor.
>