Set receive buffer size, and comment about possible setting of send buffer.

Set receive buffer size, and comment about possible setting of send buffer.

BUG=252051
r=rch@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207753

[Ryan Hamilton, 2013-06-20 03:42:22]

Thanks for doing this.

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:382: int32 const kSocketBufferSize(1200 * 100); 
// Support 100 packets.
nit: in quic_protocol.h, we have kMaxPacketSize = 1200.  so how 'bout:

const int32 kSocketReceiveBufferSize = kMaxPacketSize * 100;

Also we tend to define such constants at the top of the file, so I recommend

namespace {

const int32 kSocketReceiveBufferSize = kMaxPacketSize * 100;

}  // namespace

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:383: DLOG(INFO) << "QUIC socket receive buffer
set to " << kSocketBufferSize;
super minor nit:  Do we really need this log line?  It's going to log the exact
same value each time.  Seems a bit unnecessary.  But clearly, this is not a big
deal :>

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:384:
socket->SetReceiveBufferSize(kSocketBufferSize);
I don't think this is worth doing, but for the sake of completeness...  If you
were super excited about testing, you could add a bit of logic to
MockUDPClientSocket which would allow you to verify that the QuicStreamFactory
sets the correct value for the receive buffer size.  This doesn't seem terribly
important, though in some sense this CL is a bug fix so perhaps a test would be
a good idea?

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:385: // What should the send buffer be set to? 
If the send buffer is too large,
nit: Add TODO(jar):  (or TODO(rch)?) to this comment.

[jar (doing other things), 2013-06-20 21:45:50]

Ryan: PTAL

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:382: int32 const kSocketBufferSize(1200 * 100); 
// Support 100 packets.
On 2013/06/20 03:42:23, Ryan Hamilton wrote:
> nit: in quic_protocol.h, we have kMaxPacketSize = 1200.  so how 'bout:
> 
> const int32 kSocketReceiveBufferSize = kMaxPacketSize * 100;
> 
> Also we tend to define such constants at the top of the file, so I recommend
> 
> namespace {
> 
> const int32 kSocketReceiveBufferSize = kMaxPacketSize * 100;
> 
> }  // namespace

Done.

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:383: DLOG(INFO) << "QUIC socket receive buffer
set to " << kSocketBufferSize;
On 2013/06/20 03:42:23, Ryan Hamilton wrote:
> super minor nit:  Do we really need this log line?  It's going to log the
exact
> same value each time.  Seems a bit unnecessary.  But clearly, this is not a
big
> deal :>

Mixed feeelings.... but since it bothered you, it is out-a-here ;-).

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:384:
socket->SetReceiveBufferSize(kSocketBufferSize);
On 2013/06/20 03:42:23, Ryan Hamilton wrote:
> I don't think this is worth doing, but for the sake of completeness...  If you
> were super excited about testing, you could add a bit of logic to
> MockUDPClientSocket which would allow you to verify that the QuicStreamFactory
> sets the correct value for the receive buffer size.  This doesn't seem
terribly
> important, though in some sense this CL is a bug fix so perhaps a test would
be
> a good idea?

Yeah... probably a good idea... I'd rather get this in simply, and not have to
extend the mock socket etc. now.

https://chromiumcodereview.appspot.com/16999008/diff/1/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:385: // What should the send buffer be set to? 
If the send buffer is too large,
On 2013/06/20 03:42:23, Ryan Hamilton wrote:
> nit: Add TODO(jar):  (or TODO(rch)?) to this comment.

Done.

[Ryan Hamilton, 2013-06-20 21:53:39]

lgtm

[commit-bot: I haz the power, 2013-06-20 23:18:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/16999008/13001

[commit-bot: I haz the power, 2013-06-21 07:50:59]

Change committed as 207753