Port QuicServer to Chrome network stack

Port QuicServer to Chrome network stack

Also ported quic_server_bin and quic_server_test.

BUG=

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

[dmziegler, 2014-06-17 02:01:10]

I ported the quic_server binary as well, and it seems to work!

[wtc, 2014-06-18 02:04:48]

Review comments on patch set 3:

Good progress!

I reviewed everything except quic_server.cc. I will review quic_server.cc
tomorrow.

The CL is not yet polished, but this is to be expected.

https://codereview.chromium.org/340433002/diff/40001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/340433002/diff/40001/net/net.gyp#newcode1420
net/net.gyp:1420: 'target_name': 'quic_ported_server',

Is this a temporary target? It seems that these files should be added to the
'net' target. I guess you want to wait until you have eliminated the balsa
dependency?

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher.h
File net/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:40: class QuicPacketWriterWrapper;

Delete this unused forward declaration.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:42: class UDPServerSocket;

Delete this unused forward declaration.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:77: virtual void OnCanWrite() OVERRIDE;

Add a comment to point out that this method is in the
QuicServerPacketWriterVisitor interface.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:160: QuicPacketWriter* writer() { return
writer_.get(); }

Should this return a QuicServerPacketWriter* ?

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_in_memory_...
File net/quic/quic_in_memory_cache.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_in_memory_...
net/quic/quic_in_memory_cache.cc:22: FilePath g_quic_in_memory_cache_dir =
FilePath(FILE_PATH_LITERAL(""));

Global objects are banned in Chromium. We can probably make an exception because
this file is only used in tests, but we should try to avoid this.

Can this global variable be a FilePath::CharType string?

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_in_memory_...
net/quic/quic_in_memory_cache.cc:186: string utf8File = file.AsUTF8Unsafe();

Name this variable "utf8_file".

Actually you should be able to name this variable "base" and delete the
StringPiece "base" variable. A StringPiece can be automatically constructed from
a std::string.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h
File net/quic/quic_server.h (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:12: #include "base/file_util.h"

It doesn't seem necessary to include "base/file_util.h".

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:19: #include "net/quic/quic_framer.h"

It doesn't seem necessary to include "net/quic/quic_framer.h".

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:46: void OnReadComplete(int result);

Document this method.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:75: // Listening connection. Also used for outbound
client communication.

"Listening connection" seems wrong. I think this should be "Listening socket".

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:94: IPEndPoint server_address_;

Add a comment. Perhaps adapting the original comment for the "int fd_" member:
  // The address the server is listening on.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:104: base::WeakPtrFactory<QuicServer> weak_factory_;

Except for weak_factory_, all the new data members should be documented.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.cc:12: #include "net/quic/quic_dispatcher.h"

Remove this header?

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.cc:76: visitor_->OnCanWrite();

It is strange that we are invoking the OnCanWrite() methods on two objects, but
only one of them is considered a "visitor".

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:18: class QuicDispatcher;

Delete the unused forward declaration.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:19: struct WriteResult;

I think this forward declaration can be removed. I'd be surprised that a forward
declaration of struct WriteResult is enough. I think this header needs the
definition of this struct.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:25: virtual void OnCanWrite() {}

Document this method.

This method should be abstract (= 0).

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:54: QuicServerPacketWriterVisitor*
visitor_;

Nit: the order of these two members should match the order of the equivalent
constructor arguments.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_tes...
File net/quic/quic_server_test.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_tes...
net/quic/quic_server_test.cc:13: #include
"net/quic/test_tools/test_task_runner.h"

We should be able to remove some of these headers. The code is no longer using
MockClock, MockRandom, etc.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_tes...
net/quic/quic_server_test.cc:23: class QuicChromeServerDispatchPacketTest :
public ::testing::Test {

Why do we need to rename this test class? Is there a name conflict?

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
File net/tools/quic/quic_server_bin.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
net/tools/quic/quic_server_bin.cc:20: const uint32
kServerInitialFlowControlWindow = 100 * net::kMaxPacketSize;

This is a very non-obvious constant. Can we get this constant from some quic
header?

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
net/tools/quic/quic_server_bin.cc:47: net::g_quic_in_memory_cache_dir =
base::FilePath::FromUTF8Unsafe(

It may be better to provide a setter function. Seems bad to set a global
variable directly.

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
net/tools/quic/quic_server_bin.cc:60: base::MessageLoopForIO messageLoop;

This variable should be named "message_loop". But it doesn't seem to be used. I
guess we need the side effects of the base::MessageLoopForIO constructor and
destructor?

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
net/tools/quic/quic_server_bin.cc:73: versions.push_back(net::QUIC_VERSION_15);

There should be a function that returns all the supported QUIC versions.

[dmziegler, 2014-06-18 20:13:42]

I realized that if the writer ever got blocked, everything would fail
catastrophically (immediate segfault). To fix this, I needed to add a new
callback argument to QuicPacketWriter::WritePacket. The callback is only called
when the write completes asynchronously. Many WritePacket callers also do not
need a callback, so they pass in a null Callback object, in which the
PacketWriters do not try to call them. I tested this by artificially simulating
long-lasting asynchronous writes in QuicServerPacketWriter. I also made sure
that QuicTimeWaitListManager actually works with async I/O, which it seems to.

Note my questions around g_quic_in_memory_cache_dir and utf8_file.

https://codereview.chromium.org/340433002/diff/40001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/340433002/diff/40001/net/net.gyp#newcode1420
net/net.gyp:1420: 'target_name': 'quic_ported_server',
On 2014/06/18 02:04:46, wtc wrote:
> 
> Is this a temporary target? It seems that these files should be added to the
> 'net' target. I guess you want to wait until you have eliminated the balsa
> dependency?

Yes. I also don't want to bloat 'net' until these classes are actually used
there.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher.h
File net/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:40: class QuicPacketWriterWrapper;
On 2014/06/18 02:04:46, wtc wrote:
> 
> Delete this unused forward declaration.

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:42: class UDPServerSocket;
On 2014/06/18 02:04:46, wtc wrote:
> 
> Delete this unused forward declaration.

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:77: virtual void OnCanWrite() OVERRIDE;
On 2014/06/18 02:04:46, wtc wrote:
> 
> Add a comment to point out that this method is in the
> QuicServerPacketWriterVisitor interface.

Done (Is now QuicBlockedWriterInterface).

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:160: QuicPacketWriter* writer() { return
writer_.get(); }
On 2014/06/18 02:04:46, wtc wrote:
> 
> Should this return a QuicServerPacketWriter* ?

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_in_memory_...
File net/quic/quic_in_memory_cache.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_in_memory_...
net/quic/quic_in_memory_cache.cc:22: FilePath g_quic_in_memory_cache_dir =
FilePath(FILE_PATH_LITERAL(""));
On 2014/06/18 02:04:46, wtc wrote:
> 
> Global objects are banned in Chromium. We can probably make an exception
because
> this file is only used in tests, but we should try to avoid this.
> 
> Can this global variable be a FilePath::CharType string?

I've made it a FilePath::StringType.

In general, we should think about how QuicServer/QuicDispatcher can provide an
actual interface to let you pass in what you want to serve (whether that's a
WebRTC connection or actual content), rather than always serving a
QuicSpdyServerStream and always initializing QuicInMemoryCache with this global
variable hack.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_in_memory_...
net/quic/quic_in_memory_cache.cc:186: string utf8File = file.AsUTF8Unsafe();
On 2014/06/18 02:04:46, wtc wrote:
> 
> Name this variable "utf8_file".
> 
> Actually you should be able to name this variable "base" and delete the
> StringPiece "base" variable. A StringPiece can be automatically constructed
from
> a std::string.

This would require more unnecessary copying, since "base =
response_headers.GetHeader("X-Original-Url")" would have to construct a
std::string out of a StringPiece and the remove_prefix calls on lines 194 and
196 would have to copy out the suffix. Do you think that would be better anyway?

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h
File net/quic/quic_server.h (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:12: #include "base/file_util.h"
On 2014/06/18 02:04:47, wtc wrote:
> 
> It doesn't seem necessary to include "base/file_util.h".

Removed.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:19: #include "net/quic/quic_framer.h"
On 2014/06/18 02:04:47, wtc wrote:
> 
> It doesn't seem necessary to include "net/quic/quic_framer.h".

Removed.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:46: void OnReadComplete(int result);
On 2014/06/18 02:04:47, wtc wrote:
> 
> Document this method.

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:75: // Listening connection. Also used for outbound
client communication.
On 2014/06/18 02:04:47, wtc wrote:
> 
> "Listening connection" seems wrong. I think this should be "Listening socket".

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:94: IPEndPoint server_address_;
On 2014/06/18 02:04:46, wtc wrote:
> 
> Add a comment. Perhaps adapting the original comment for the "int fd_" member:
>   // The address the server is listening on.

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server.h#n...
net/quic/quic_server.h:104: base::WeakPtrFactory<QuicServer> weak_factory_;
On 2014/06/18 02:04:47, wtc wrote:
> 
> Except for weak_factory_, all the new data members should be documented.

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.cc:12: #include "net/quic/quic_dispatcher.h"
On 2014/06/18 02:04:47, wtc wrote:
> 
> Remove this header?

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.cc:76: visitor_->OnCanWrite();
On 2014/06/18 02:04:47, wtc wrote:
> 
> It is strange that we are invoking the OnCanWrite() methods on two objects,
but
> only one of them is considered a "visitor".

Now there's one Callback and one QuicBlockedWriterInterface.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:18: class QuicDispatcher;
On 2014/06/18 02:04:47, wtc wrote:
> 
> Delete the unused forward declaration.

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:19: struct WriteResult;
On 2014/06/18 02:04:47, wtc wrote:
> 
> I think this forward declaration can be removed. I'd be surprised that a
forward
> declaration of struct WriteResult is enough. I think this header needs the
> definition of this struct.

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:25: virtual void OnCanWrite() {}
On 2014/06/18 02:04:47, wtc wrote:
> 
> Document this method.
> 
> This method should be abstract (= 0).

(This got deleted)

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:54: QuicServerPacketWriterVisitor*
visitor_;
On 2014/06/18 02:04:47, wtc wrote:
> 
> Nit: the order of these two members should match the order of the equivalent
> constructor arguments.

No longer in the constructor.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_tes...
File net/quic/quic_server_test.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_tes...
net/quic/quic_server_test.cc:13: #include
"net/quic/test_tools/test_task_runner.h"
On 2014/06/18 02:04:47, wtc wrote:
> 
> We should be able to remove some of these headers. The code is no longer using
> MockClock, MockRandom, etc.

Done.

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_server_tes...
net/quic/quic_server_test.cc:23: class QuicChromeServerDispatchPacketTest :
public ::testing::Test {
On 2014/06/18 02:04:47, wtc wrote:
> 
> Why do we need to rename this test class? Is there a name conflict?

Yes, it conflicts with the other quic_server_test.cc in tools/quic/. Once we get
rid of tools/quic/, we can name it back.

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
File net/tools/quic/quic_server_bin.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
net/tools/quic/quic_server_bin.cc:20: const uint32
kServerInitialFlowControlWindow = 100 * net::kMaxPacketSize;
It used to live in quic_server.cc with no explanatory comment. I can't find
anything better.

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
net/tools/quic/quic_server_bin.cc:47: net::g_quic_in_memory_cache_dir =
base::FilePath::FromUTF8Unsafe(
On 2014/06/18 02:04:47, wtc wrote:
> 
> It may be better to provide a setter function. Seems bad to set a global
> variable directly.

Do you want me to make this better now, or should we wait until we have a proper
solution for telling the QuicServer what to serve?

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
net/tools/quic/quic_server_bin.cc:60: base::MessageLoopForIO messageLoop;
On 2014/06/18 02:04:47, wtc wrote:
> 
> This variable should be named "message_loop". But it doesn't seem to be used.
I
> guess we need the side effects of the base::MessageLoopForIO constructor and
> destructor?

Yes, it initializes the message loop on the thread, which is used in QuicServer
and in the "base::RunLoop().Run()" below.

https://codereview.chromium.org/340433002/diff/40001/net/tools/quic/quic_serv...
net/tools/quic/quic_server_bin.cc:73: versions.push_back(net::QUIC_VERSION_15);
On 2014/06/18 02:04:48, wtc wrote:
> 
> There should be a function that returns all the supported QUIC versions.

Yes, replaced.

[Ryan Hamilton, 2014-06-18 23:59:51]

This is looking reasonable, but one gotcha is the changes to the default packet
writer taking a callback.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.h:768: base::WeakPtrFactory<QuicConnection>
weak_factory_;
This is chrome specific, and probably shouldn't go into shared code.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_packet_wri...
File net/quic/quic_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_packet_wri...
net/quic/quic_packet_writer.h:31: base::Callback<void(WriteResult wr)> callback)
= 0;
This is chrome specific, and probably shouldn't go into shared code.

After discussing this offline, perhaps we need a per-connection writer.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:29: static const int kReadBufferSize = 2 *
net::kMaxPacketSize;
This seems too small. Why 2 (vs, say 100 from the old version)?

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:85: rc =
socket->SetSendBufferSize(TcpReceiver::kReceiveWindowTCP);
Seems surprising to set the send buffer to the *receive* window.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:121: void QuicServer::OnReadComplete(int result) {
Please move this method below StartReading();

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server_tes...
File net/quic/quic_server_test.cc (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server_tes...
net/quic/quic_server_test.cc:53: QuicUtils::AsChars(valid_packet),
arraysize(valid_packet), false);
Bah! I like the old formatting better :>

[dmz, 2014-06-19 17:46:25]

PTAL

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.h:768: base::WeakPtrFactory<QuicConnection>
weak_factory_;
On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> This is chrome specific, and probably shouldn't go into shared code.

Removed.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_packet_wri...
File net/quic/quic_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_packet_wri...
net/quic/quic_packet_writer.h:31: base::Callback<void(WriteResult wr)> callback)
= 0;
On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> This is chrome specific, and probably shouldn't go into shared code.
> 
> After discussing this offline, perhaps we need a per-connection writer.

Done.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:29: static const int kReadBufferSize = 2 *
net::kMaxPacketSize;
On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> This seems too small. Why 2 (vs, say 100 from the old version)?

This is actually a different constant (the size of the buffer to read every
packet into). I pulled this one out of the code below, and deleted
kServerInitialFlowControlWindow, since an upstream change seems to have made it
obsolete.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:85: rc =
socket->SetSendBufferSize(TcpReceiver::kReceiveWindowTCP);
On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> Seems surprising to set the send buffer to the *receive* window.

I agree -- although this is unchanged from the original code. I imagine for
peer-to-peer connections, we'll want to change all these magic constants.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:121: void QuicServer::OnReadComplete(int result) {
On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> Please move this method below StartReading();

Done.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server_tes...
File net/quic/quic_server_test.cc (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server_tes...
net/quic/quic_server_test.cc:53: QuicUtils::AsChars(valid_packet),
arraysize(valid_packet), false);
On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> Bah! I like the old formatting better :>

Ah, it seems one last 'git cl format' change slipped through. I put the old
formatting back.

https://codereview.chromium.org/340433002/diff/120001/net/quic/quic_connectio...
File net/quic/quic_connection_packet_writer_wrapper.h (right):

https://codereview.chromium.org/340433002/diff/120001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:24: class
QuicConnectionPacketWriterWrapper : public QuicPacketWriter {
I wonder whether it would be better to inherit from QuicPacketWriterWrapper --
the only problem is that QuicPacketWriterWrapper expects to own the writer it
wraps.

[Ryan Hamilton, 2014-06-19 19:21:30]

Great progress. Are you able to run the quic server and point something at it to
ensure that it works (Before you get the tests ported)

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:85: rc =
socket->SetSendBufferSize(TcpReceiver::kReceiveWindowTCP);
On 2014/06/19 17:46:24, dmz wrote:
> On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> > Seems surprising to set the send buffer to the *receive* window.
> 
> I agree -- although this is unchanged from the original code. I imagine for
> peer-to-peer connections, we'll want to change all these magic constants.

Let's do what we do in the QuicStreamFactory:

net/quic/quic_stream_factory.cc:  rv = socket->SetSendBufferSize(kMaxPacketSize
* 20);

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:121: void QuicServer::OnReadComplete(int result) {
On 2014/06/19 17:46:24, dmz wrote:
> On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> > Please move this method below StartReading();
> 
> Done.

Still looks to be above StartReading()?

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:128: return;
I'd recommend closing connections and exiting.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:34: base::Callback<void(WriteResult wr)>
callback) OVERRIDE;
Consider using a typedef for this type. (And remove "wr" from the signature, I
think).

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
File net/quic/quic_connection_packet_writer_wrapper.cc (right):

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.cc:37: return
writer_->WritePacket(buffer, buf_len, self_address, peer_address);
Instead of making this 2 distinct call, you could add a new
WritePacketWithCallback method to the server writer.

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
File net/quic/quic_connection_packet_writer_wrapper.h (right):

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:24: class
QuicConnectionPacketWriterWrapper : public QuicPacketWriter {
It's worth thinking about how we could potentially merge this with Chrome's
default packet writer. Can you add a TODO and we can worry about this in a
second CL.

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:43: QuicServerPacketWriter*
writer_;
Please add a comment that this is not owned.

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:72: callback_.Run(result);
Is it expected that callback_ can be NULL here?

[dmz, 2014-06-19 20:06:28]

Yes, I've run the ported quic_server binary and downloaded stuff using
quic_client, and everything seems to work.

PTAL

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_in_memory_...
File net/quic/quic_in_memory_cache.cc (right):

https://codereview.chromium.org/340433002/diff/40001/net/quic/quic_in_memory_...
net/quic/quic_in_memory_cache.cc:186: string utf8File = file.AsUTF8Unsafe();
On 2014/06/18 20:13:41, dmziegler wrote:
> On 2014/06/18 02:04:46, wtc wrote:
> > 
> > Name this variable "utf8_file".
> > 
> > Actually you should be able to name this variable "base" and delete the
> > StringPiece "base" variable. A StringPiece can be automatically constructed
> from
> > a std::string.
> 
> This would require more unnecessary copying, since "base =
> response_headers.GetHeader("X-Original-Url")" would have to construct a
> std::string out of a StringPiece and the remove_prefix calls on lines 194 and
> 196 would have to copy out the suffix. Do you think that would be better
anyway?

Update: My next CL changes things anyway.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:85: rc =
socket->SetSendBufferSize(TcpReceiver::kReceiveWindowTCP);
On 2014/06/19 19:21:29, Ryan Hamilton wrote:
> On 2014/06/19 17:46:24, dmz wrote:
> > On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> > > Seems surprising to set the send buffer to the *receive* window.
> > 
> > I agree -- although this is unchanged from the original code. I imagine for
> > peer-to-peer connections, we'll want to change all these magic constants.
> 
> Let's do what we do in the QuicStreamFactory:
> 
> net/quic/quic_stream_factory.cc:  rv =
socket->SetSendBufferSize(kMaxPacketSize
> * 20);

Done.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:121: void QuicServer::OnReadComplete(int result) {
On 2014/06/19 19:21:29, Ryan Hamilton wrote:
> On 2014/06/19 17:46:24, dmz wrote:
> > On 2014/06/18 23:59:51, Ryan Hamilton wrote:
> > > Please move this method below StartReading();
> > 
> > Done.
> 
> Still looks to be above StartReading()?

Not in the newest patch set -- looks like you still had the old version open
somewhere :). (I think the only changes to quic_server.cc following this patch
set were moving this method and eliminating
server_initial_flow_control_receive_window_)

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server.cc#...
net/quic/quic_server.cc:128: return;
On 2014/06/19 19:21:29, Ryan Hamilton wrote:
> I'd recommend closing connections and exiting.

Done.

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/80001/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:34: base::Callback<void(WriteResult wr)>
callback) OVERRIDE;
On 2014/06/19 19:21:29, Ryan Hamilton wrote:
> Consider using a typedef for this type. (And remove "wr" from the signature, I
> think).

Done.

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
File net/quic/quic_connection_packet_writer_wrapper.cc (right):

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.cc:37: return
writer_->WritePacket(buffer, buf_len, self_address, peer_address);
On 2014/06/19 19:21:29, Ryan Hamilton wrote:
> Instead of making this 2 distinct call, you could add a new
> WritePacketWithCallback method to the server writer.

Done.

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
File net/quic/quic_connection_packet_writer_wrapper.h (right):

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:24: class
QuicConnectionPacketWriterWrapper : public QuicPacketWriter {
On 2014/06/19 19:21:29, Ryan Hamilton wrote:
> It's worth thinking about how we could potentially merge this with Chrome's
> default packet writer. Can you add a TODO and we can worry about this in a
> second CL.

Done.

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:43: QuicServerPacketWriter*
writer_;
On 2014/06/19 19:21:29, Ryan Hamilton wrote:
> Please add a comment that this is not owned. 

Done.

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/140001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:72: callback_.Run(result);
On 2014/06/19 19:21:29, Ryan Hamilton wrote:
> Is it expected that callback_ can be NULL here?

You're right, not anymore.

[Ryan Hamilton, 2014-06-20 15:26:24]

lgtm

[Ryan Hamilton, 2014-06-20 18:07:51]

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
File net/tools/quic/quic_server_bin.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
net/tools/quic/quic_server_bin.cc:6: // (default 6121) until it's killed or
ctrl-cd to death.
Should this file be copied to net/quic?

[wtc, 2014-06-20 21:40:50]

Patch set 9 LGTM. Please address my review comments before you commit this CL.
Good work!

In the CL's description, please add
  R=rch@chromium.org,wtc@chromium.org

I reviewed the whole CL very carefully.

I didn't review your response to my previous comments. So I'm sorry that I asked
some questions again. You don't need to answer those questions again if you
already answered them.

https://codereview.chromium.org/340433002/diff/160001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/340433002/diff/160001/net/net.gyp#newcode547
net/net.gyp:547: 'quic_ported_server',

List this in alphabetical order, after 'net_test_support'.

https://codereview.chromium.org/340433002/diff/160001/net/net.gyp#newcode1420
net/net.gyp:1420: 'target_name': 'quic_ported_server',

Please document why these files are a separate build target and not part of the
'net' target. (I believe it's because of the balsa dependency.)

https://codereview.chromium.org/340433002/diff/160001/net/net.gyp#newcode1445
net/net.gyp:1445: 'quic/quic_time_wait_list_manager.h',

List these files in alphabetical order.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
File net/quic/quic_connection_packet_writer_wrapper.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:15: #include
"net/udp/udp_server_socket.h"

Delete this header. This file is not using UDPServerSocket.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:19: class
QuicBlockedWriterInterface;

Delete this unused forward declaration.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:25: class
QuicConnectionPacketWriterWrapper : public QuicPacketWriter {

Why isn't this class named QuicServerPacketWriterWrapper? It seems to wrap a
QuicServerPacketWriter.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:41: void
OnWriteComplete(WriteResult wr);

Name this parameter "result". Our Style Guide recommends against uncommon
abbreviations as variable/parameter names. I didn't find any "WriteResult wr"
variable in net/quic.

Note: "rv" and "rc" are acceptable.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:44: QuicServerPacketWriter*
writer_; // Not owned.

Our Style Guide recommends at least two spaces before a comment. git cl format
should fix this. Can you try that?

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_dispatcher.h
File net/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_dispatche...
net/quic/quic_dispatcher.h:49: public QuicBlockedWriterInterface {

Nit: since you declare the OnCanWrite method before the OnConnectionClosed, it
seems more consistent to list the base classes in the same order.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_in_memory...
File net/quic/quic_in_memory_cache.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_in_memory...
net/quic/quic_in_memory_cache.h:23: extern base::FilePath::StringType
g_quic_in_memory_cache_dir;

Why does g_quic_in_memory_cache_dir need to be a string object rather than a C
string?

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc
File net/quic/quic_server.cc (left):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:68: epoll_server_.set_timeout_in_us(50 * 1000);

Do you know what this 50-second timeout is?

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:80:
config_.SetInitialCongestionWindowToSend(kServerInitialFlowControlWindow);

Just wanted to make sure you meant to delete this.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:137: }

This is a problem with the original code. Lines 125-137 are exactly what the
QuicSocketUtils::SetGetAddressInfo method does. So this is redundant with the
QuicSocketUtils::SetGetAddressInfo call on line 95. We should fix this in the
internal version.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:210: }

Please explain to me in person how the new code does the equivalent of lines
205-210 of the original code.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:29: static const int kReadBufferSize = 2 *
kMaxPacketSize;

Put these two constants inside an unnamed namespace.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:33: : helper_(new QuicConnectionHelper(

BUG: |helper_| seems to be leaked.

I think |helper_| should be declared as a non-pointer member:

  QuicConnectionHelperInterface helper_;

and initialized like this:

    : helper_(base::MessageLoop::current()->message_loop_proxy().get(),
              &clock_,
              QuicRandom::GetInstance())),

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:52:
crypto_config_.AddDefaultConfig(QuicRandom::GetInstance(),

It seems that we should get the QuicRandom from |helper_|.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:62: new UDPServerSocket(NULL, NetLog::Source()));

We may want to add a
  NetLog net_log_;
member and pass &net_log_ as the first argument to the UDPServerSocket
constructor.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:64: // TODO(dmz): socket->AllowBroadcast()?
socket->SetMulticastInterface?

The answer is No to both questions. The original code doesn't set the equivalent
of either socket option.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:65: socket->AllowAddressReuse();

The original code doesn't set this socket option (SO_REUSEADDR) either. It seems
fine to me though.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:67: int rc = socket->Listen(address);

socket->Listen() matches the bind() call in the original code. So it is better
to do this after the socket->SetSendBufferSize call to be safe.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:69: LOG(ERROR) << "Listen() failed: " <<
strerror(errno);

IMPORTANT: print ErrorToString(rc) instead of strerror(errno). Fix all the
occurrences in this file.

On Windows, |errno| is only used for Standard C Library functions, not for
Winsock functions.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:89: socket->GetLocalAddress(&server_address_);

Nit: check the return value.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:114: socket_->Close();

Add:
    socket_.reset();

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:134: synchronous_read_count_ = 0;

IMPORTANT: we should set |read_pending_| to true in this case. We are simulating
a pending read here even though the read has completed synchronously. A
|read_pending_| of true prevents the read_buffer_ and read_addr_ from being
overwritten by a new RecvFrom call.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:149: if (result == 0)

I don't think we will ever get a read result of 0 for a connectionless socket.
If you copied this from some other file, then this is fine.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.h
File net/quic/quic_server.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.h#...
net/quic/quic_server.h:33: const QuicVersionVector& supported_versions);

Why did you delete the default QuicServer constructor? Do you think it is better
to force people to consider |config| and |supported_versions| when constructing
a QuicServer?

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.h#...
net/quic/quic_server.h:106: IPEndPoint read_addr_;

Name this member client_address_ or source_address_, unless you want to
emphasize the fact that read_buffer_ and read_addr_ are strongly related.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:9: #include
"base/message_loop/message_loop.h"

Delete this header.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:13: #include
"net/quic/crypto/quic_random.h"

I believe we can delete this header.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:61: return WritePacket(buffer, buf_len,
self_address, peer_address);

IMPORTANT: I think we should clear callback_ if WritePacket returns a status
other than WRITE_STATUS_BLOCKED. We can also just test the |write_blocked_|
member.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:81: callback_.Run(result);

BUG: I think we need to clear |callback_|. Our convention is to clear callback_
before running it. You can use the ResetAndRun method to do that.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:82: blocked_writer_->OnCanWrite();

Can you explain this change? It seems that callback_.Run(result) is equivalent
to the original code (connection_->OnPacketSent(result) +
connection_->OnCanWrite()), so the net effect is the new
blocked_writer_->OnCanWrite() call. What does this accomplish?

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:15: #include "net/udp/udp_server_socket.h"

A forward declaration of class UDPServerSocket may be enough.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:28: UDPServerSocket* socket);

Nit: I would reorder these two parameters because the |socket| seems more
important to a packet writer.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:31: virtual WriteResult
WritePacketWithCallback(

Document this method.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:56: base::Callback<void(WriteResult wr)>
callback_;

Change "wr" to "result". But we should be able to use the WriteCallback typedef:

  WriteCallback callback_;

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_se...
File net/quic/quic_server_session.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_se...
net/quic/quic_server_session.h:47: // Takes ownership of
connection_packet_writer

This is a strange design. The only reason we're passing
|connection_packet_writer| to the QuicServerSession constructor is for the
QuicServerSession object to own |connection_packet_writer|. The
QuicServerSession object itself doesn't use |connection_packet_writer| at all!

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_te...
File net/quic/quic_server_test.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_te...
net/quic/quic_server_test.cc:20: class QuicChromeServerDispatchPacketTest :
public ::testing::Test {

Why do we need to rename this test class? If it is to avoid naming conflict, add
a TODO comment to remind ourselves that when the QuicServerDispatchPacketTest
class is deleted, we should rename this back to the original name.

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
File net/tools/quic/quic_server_bin.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
net/tools/quic/quic_server_bin.cc:47:
line->GetSwitchValueNative("quic_in_memory_cache_dir");

Why change GetSwitchValueASCII to GetSwitchValueNative?

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
net/tools/quic/quic_server_bin.cc:71: return -rc;

IMPORTANT: the main() function can only return a value in the range [0, 255].
Our error code (the negated value) may be greater than 255. Can we just return
1?

This seems to be the only place where we call QuicServer::Listen. If we can't
really use the error code it returns, we can change QuicServer::Listen back to
returning a bool.

[dmziegler, 2014-06-23 18:31:25]

I'll commit soon.

https://codereview.chromium.org/340433002/diff/160001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/340433002/diff/160001/net/net.gyp#newcode547
net/net.gyp:547: 'quic_ported_server',
On 2014/06/20 21:40:48, wtc wrote:
> 
> List this in alphabetical order, after 'net_test_support'.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/net.gyp#newcode1420
net/net.gyp:1420: 'target_name': 'quic_ported_server',
On 2014/06/20 21:40:48, wtc wrote:
> 
> Please document why these files are a separate build target and not part of
the
> 'net' target. (I believe it's because of the balsa dependency.)

Done.

https://codereview.chromium.org/340433002/diff/160001/net/net.gyp#newcode1445
net/net.gyp:1445: 'quic/quic_time_wait_list_manager.h',
On 2014/06/20 21:40:48, wtc wrote:
> 
> List these files in alphabetical order.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
File net/quic/quic_connection_packet_writer_wrapper.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:15: #include
"net/udp/udp_server_socket.h"
On 2014/06/20 21:40:48, wtc wrote:
> 
> Delete this header. This file is not using UDPServerSocket.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:19: class
QuicBlockedWriterInterface;
On 2014/06/20 21:40:48, wtc wrote:
> 
> Delete this unused forward declaration.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:25: class
QuicConnectionPacketWriterWrapper : public QuicPacketWriter {
On 2014/06/20 21:40:48, wtc wrote:
> 
> Why isn't this class named QuicServerPacketWriterWrapper? It seems to wrap a
> QuicServerPacketWriter.

I've renamed to QuicPerConnectionPacketWriter (and elaborated the comment) to
make its purpose clearer.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:41: void
OnWriteComplete(WriteResult wr);
On 2014/06/20 21:40:48, wtc wrote:
> 
> Name this parameter "result". Our Style Guide recommends against uncommon
> abbreviations as variable/parameter names. I didn't find any "WriteResult wr"
> variable in net/quic.
> 
> Note: "rv" and "rc" are acceptable.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_packet_writer_wrapper.h:44: QuicServerPacketWriter*
writer_; // Not owned.
On 2014/06/20 21:40:48, wtc wrote:
> 
> Our Style Guide recommends at least two spaces before a comment. git cl format
> should fix this. Can you try that?

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_dispatcher.h
File net/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_dispatche...
net/quic/quic_dispatcher.h:49: public QuicBlockedWriterInterface {
On 2014/06/20 21:40:48, wtc wrote:
> 
> Nit: since you declare the OnCanWrite method before the OnConnectionClosed, it
> seems more consistent to list the base classes in the same order.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_in_memory...
File net/quic/quic_in_memory_cache.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_in_memory...
net/quic/quic_in_memory_cache.h:23: extern base::FilePath::StringType
g_quic_in_memory_cache_dir;
On 2014/06/20 21:40:49, wtc wrote:
> 
> Why does g_quic_in_memory_cache_dir need to be a string object rather than a C
> string?

So that it can be set.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc
File net/quic/quic_server.cc (left):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:68: epoll_server_.set_timeout_in_us(50 * 1000);
On 2014/06/20 21:40:49, wtc wrote:
> 
> Do you know what this 50-second timeout is?

It controls how long epoll_server_.WaitForEventsAndExecuteCallbacks() waits;
since that was just called in an infinite loop anyway and Chrome's message loop
doesn't have this functionality, I took it out. (Also, it's actually a 50ms
timeout.)

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:80:
config_.SetInitialCongestionWindowToSend(kServerInitialFlowControlWindow);
On 2014/06/20 21:40:49, wtc wrote:
> 
> Just wanted to make sure you meant to delete this.

Hmm, no, I can't remember deleting it.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:29: static const int kReadBufferSize = 2 *
kMaxPacketSize;
On 2014/06/20 21:40:49, wtc wrote:
> 
> Put these two constants inside an unnamed namespace.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:33: : helper_(new QuicConnectionHelper(
On 2014/06/20 21:40:49, wtc wrote:
> 
> BUG: |helper_| seems to be leaked.
> 
> I think |helper_| should be declared as a non-pointer member:
> 
>   QuicConnectionHelperInterface helper_;
> 
> and initialized like this:
> 
>     : helper_(base::MessageLoop::current()->message_loop_proxy().get(),
>               &clock_,
>               QuicRandom::GetInstance())),

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:52:
crypto_config_.AddDefaultConfig(QuicRandom::GetInstance(),
On 2014/06/20 21:40:49, wtc wrote:
> 
> It seems that we should get the QuicRandom from |helper_|.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:62: new UDPServerSocket(NULL, NetLog::Source()));
On 2014/06/20 21:40:49, wtc wrote:
> 
> We may want to add a
>   NetLog net_log_;
> member and pass &net_log_ as the first argument to the UDPServerSocket
> constructor.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:64: // TODO(dmz): socket->AllowBroadcast()?
socket->SetMulticastInterface?
On 2014/06/20 21:40:49, wtc wrote:
> 
> The answer is No to both questions. The original code doesn't set the
equivalent
> of either socket option.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:65: socket->AllowAddressReuse();
On 2014/06/20 21:40:49, wtc wrote:
> 
> The original code doesn't set this socket option (SO_REUSEADDR) either. It
seems
> fine to me though.

Okay. I can't remember why I thought it should be added.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:67: int rc = socket->Listen(address);
On 2014/06/20 21:40:49, wtc wrote:
> 
> socket->Listen() matches the bind() call in the original code. So it is better
> to do this after the socket->SetSendBufferSize call to be safe.

Actually, I had that originally, but the code crashed -- it appears the
UDPServerSocket doesn't have a valid fd until you call Listen.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:69: LOG(ERROR) << "Listen() failed: " <<
strerror(errno);
On 2014/06/20 21:40:49, wtc wrote:
> 
> IMPORTANT: print ErrorToString(rc) instead of strerror(errno). Fix all the
> occurrences in this file.
> 
> On Windows, |errno| is only used for Standard C Library functions, not for
> Winsock functions.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:89: socket->GetLocalAddress(&server_address_);
On 2014/06/20 21:40:49, wtc wrote:
> 
> Nit: check the return value.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:114: socket_->Close();
On 2014/06/20 21:40:49, wtc wrote:
> 
> Add:
>     socket_.reset();

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:134: synchronous_read_count_ = 0;
On 2014/06/20 21:40:49, wtc wrote:
> 
> IMPORTANT: we should set |read_pending_| to true in this case. We are
simulating
> a pending read here even though the read has completed synchronously. A
> |read_pending_| of true prevents the read_buffer_ and read_addr_ from being
> overwritten by a new RecvFrom call.

Good point. In practice, I don't think this causes a bug, since StartReading()
should just be called once to start, and after that only from the callback.
However, it's definitely good to protect this in case that changes.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.cc...
net/quic/quic_server.cc:149: if (result == 0)
On 2014/06/20 21:40:49, wtc wrote:
> 
> I don't think we will ever get a read result of 0 for a connectionless socket.
> If you copied this from some other file, then this is fine.

Yes, I copied it from QuicClientSession.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.h
File net/quic/quic_server.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.h#...
net/quic/quic_server.h:33: const QuicVersionVector& supported_versions);
On 2014/06/20 21:40:49, wtc wrote:
> 
> Why did you delete the default QuicServer constructor? Do you think it is
better
> to force people to consider |config| and |supported_versions| when
constructing
> a QuicServer?

Oh, I believe I did that back when the constructor required a
QuicConnectionHelper to be passed in. However, I will leave it deleted because
|config| should always be different anyway in any real application.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server.h#...
net/quic/quic_server.h:106: IPEndPoint read_addr_;
On 2014/06/20 21:40:49, wtc wrote:
> 
> Name this member client_address_ or source_address_, unless you want to
> emphasize the fact that read_buffer_ and read_addr_ are strongly related.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:9: #include
"base/message_loop/message_loop.h"
On 2014/06/20 21:40:50, wtc wrote:
> 
> Delete this header.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:13: #include
"net/quic/crypto/quic_random.h"
On 2014/06/20 21:40:50, wtc wrote:
> 
> I believe we can delete this header.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:61: return WritePacket(buffer, buf_len,
self_address, peer_address);
On 2014/06/20 21:40:50, wtc wrote:
> 
> IMPORTANT: I think we should clear callback_ if WritePacket returns a status
> other than WRITE_STATUS_BLOCKED. We can also just test the |write_blocked_|
> member.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:81: callback_.Run(result);
On 2014/06/20 21:40:50, wtc wrote:
> 
> BUG: I think we need to clear |callback_|. Our convention is to clear
callback_
> before running it. You can use the ResetAndRun method to do that.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:82: blocked_writer_->OnCanWrite();
On 2014/06/20 21:40:50, wtc wrote:
> 
> Can you explain this change? It seems that callback_.Run(result) is equivalent
> to the original code (connection_->OnPacketSent(result) +
> connection_->OnCanWrite()), so the net effect is the new
> blocked_writer_->OnCanWrite() call. What does this accomplish?

As explained in person, both lines are actually different. The first calls back
to the specific connection which performed the write (rather than a single
connection associated with the writer), and the second calls
QuicDispatcher::OnCanWrite, which then takes care of calling
QuicConnection::OnCanWrite on the relevant connections.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:15: #include "net/udp/udp_server_socket.h"
On 2014/06/20 21:40:50, wtc wrote:
> 
> A forward declaration of class UDPServerSocket may be enough.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:28: UDPServerSocket* socket);
On 2014/06/20 21:40:50, wtc wrote:
> 
> Nit: I would reorder these two parameters because the |socket| seems more
> important to a packet writer.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:31: virtual WriteResult
WritePacketWithCallback(
On 2014/06/20 21:40:50, wtc wrote:
> 
> Document this method.

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:56: base::Callback<void(WriteResult wr)>
callback_;
On 2014/06/20 21:40:50, wtc wrote:
> 
> Change "wr" to "result". But we should be able to use the WriteCallback
typedef:
> 
>   WriteCallback callback_;

Done.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_se...
File net/quic/quic_server_session.h (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_se...
net/quic/quic_server_session.h:47: // Takes ownership of
connection_packet_writer
On 2014/06/20 21:40:50, wtc wrote:
> 
> This is a strange design. The only reason we're passing
> |connection_packet_writer| to the QuicServerSession constructor is for the
> QuicServerSession object to own |connection_packet_writer|. The
> QuicServerSession object itself doesn't use |connection_packet_writer| at all!

I will change this in a future CL.

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_te...
File net/quic/quic_server_test.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/quic/quic_server_te...
net/quic/quic_server_test.cc:20: class QuicChromeServerDispatchPacketTest :
public ::testing::Test {
On 2014/06/20 21:40:50, wtc wrote:
> 
> Why do we need to rename this test class? If it is to avoid naming conflict,
add
> a TODO comment to remind ourselves that when the QuicServerDispatchPacketTest
> class is deleted, we should rename this back to the original name.

Added the TODO.

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
File net/tools/quic/quic_server_bin.cc (right):

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
net/tools/quic/quic_server_bin.cc:6: // (default 6121) until it's killed or
ctrl-cd to death.
On 2014/06/20 18:07:51, Ryan Hamilton wrote:
> Should this file be copied to net/quic?

Good idea, there's no reason to wait. In fact, I think I'll just move it, since
I don't think we need the unported version for anything.

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
net/tools/quic/quic_server_bin.cc:47:
line->GetSwitchValueNative("quic_in_memory_cache_dir");
On 2014/06/20 21:40:50, wtc wrote:
> 
> Why change GetSwitchValueASCII to GetSwitchValueNative?

Windows compatibility (wstring filenames).

https://codereview.chromium.org/340433002/diff/160001/net/tools/quic/quic_ser...
net/tools/quic/quic_server_bin.cc:71: return -rc;
On 2014/06/20 21:40:50, wtc wrote:
> 
> IMPORTANT: the main() function can only return a value in the range [0, 255].
> Our error code (the negated value) may be greater than 255. Can we just return
> 1?
> 
> This seems to be the only place where we call QuicServer::Listen. If we can't
> really use the error code it returns, we can change QuicServer::Listen back to
> returning a bool.

I'm leaving the error code for now in case it's useful if we use QuicServer in a
peer-to-peer context, but I'm returning 1.

[dmziegler, 2014-06-23 18:50:28]

The CQ bit was checked by dmziegler@google.com

[commit-bot: I haz the power, 2014-06-23 18:52:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmziegler@google.com/340433002/10009

[commit-bot: I haz the power, 2014-06-23 22:12:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[dmziegler, 2014-06-23 22:17:11]

The CQ bit was unchecked by dmziegler@google.com

[wtc, 2014-06-23 23:12:16]

Patch set 12 LGTM.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_dispatcher.cc
File net/quic/quic_dispatcher.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.cc:369: QuicPerConnectionPacketWriter*
connection_packet_writer =

Nit: rename this variable "per_connection_packet_writer"?

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.cc:376:
connection_packet_writer->set_connection(connection);

1. Nit: the two-way links between |connection| and |connection_packet_writer|
are a little confusing. Why are they necessary?

2. Can we hide the creation and setup of the QuicPerConnectionPacketWriter
inside the CreateQuicConnection() method?

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_per_connec...
File net/quic/quic_per_connection_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_per_connec...
net/quic/quic_per_connection_packet_writer.cc:13: #include
"net/quic/crypto/quic_random.h"

We don't need many of these headers.

I remember you removed them before. Maybe there was a merge or rebase error?

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_per_connec...
net/quic/quic_per_connection_packet_writer.cc:52: void
QuicPerConnectionPacketWriter::OnWriteComplete(WriteResult wr) {

Rename "wr" to "result".

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server.cc#...
net/quic/quic_server.cc:35: }

Add a "namespace" comment, like this:

}  // namespace

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server.h
File net/quic/quic_server.h (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server.h#n...
net/quic/quic_server.h:34: QuicServer();

You said you will leave the default QuicServer constructor deleted, but you
added it back. Is this a mistake?

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.cc:67: }

If it is safe to do so, it is cheaper to do this:

  DCHECK(callback_.is_null());
  WriteResult result = WritePacket(buffer, buf_len, self_address, peer_address);
  if (result.status == WRITE_STATUS_BLOCKED) {
    callback_ = callback;
  }
  return result;

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:31: // Use this method to write packets
rather than WritePacket: this class

Should "this class" be changed to "this method"?

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_tes...
File net/quic/quic_server_test.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_tes...
net/quic/quic_server_test.cc:20: // TODO(dmz) Remove "Chrome" part of name once
tools/quic is deleted.

Nit: don't omit "net" in the path name. Say "net/tools/quic".

https://codereview.chromium.org/340433002/diff/230001/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/230001/net/quic/quic_server.cc...
net/quic/quic_server.cc:32: const int kServerInitialFlowControlWindow = 100 *
kMaxPacketSize;

Please use the 'uint32' type as in the original code.

[dmziegler, 2014-06-24 00:08:53]

Comments addressed.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_dispatcher.cc
File net/quic/quic_dispatcher.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.cc:369: QuicPerConnectionPacketWriter*
connection_packet_writer =
On 2014/06/23 23:12:15, wtc wrote:
> 
> Nit: rename this variable "per_connection_packet_writer"?

Done.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.cc:376:
connection_packet_writer->set_connection(connection);
On 2014/06/23 23:12:15, wtc wrote:
> 
> 1. Nit: the two-way links between |connection| and |connection_packet_writer|
> are a little confusing. Why are they necessary?
> 
> 2. Can we hide the creation and setup of the QuicPerConnectionPacketWriter
> inside the CreateQuicConnection() method?

The connection needs to write on the writer, and the writer needs to tell the
connection that its writes finished. It would be possible to create the
QuicPerConnectionPacketWriter inside CreateQuicConnection, but it would have to
be passed out as an out parameter, since it's passed to the QuicServerSession
constructor. I will change that once the QuicServerSession no longer owns it.
However, I have moved the set_connection call to CreateQuicConnection.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_per_connec...
File net/quic/quic_per_connection_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_per_connec...
net/quic/quic_per_connection_packet_writer.cc:13: #include
"net/quic/crypto/quic_random.h"
On 2014/06/23 23:12:16, wtc wrote:
> 
> We don't need many of these headers.
> 
> I remember you removed them before. Maybe there was a merge or rebase error?

Strange, seems so. Removed.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_per_connec...
net/quic/quic_per_connection_packet_writer.cc:52: void
QuicPerConnectionPacketWriter::OnWriteComplete(WriteResult wr) {
On 2014/06/23 23:12:15, wtc wrote:
> 
> Rename "wr" to "result".

Done.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server.cc#...
net/quic/quic_server.cc:35: }
On 2014/06/23 23:12:16, wtc wrote:
> 
> Add a "namespace" comment, like this:
> 
> }  // namespace

Done.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server.h
File net/quic/quic_server.h (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server.h#n...
net/quic/quic_server.h:34: QuicServer();
On 2014/06/23 23:12:16, wtc wrote:
> 
> You said you will leave the default QuicServer constructor deleted, but you
> added it back. Is this a mistake?

It's only declared -- yes, that was a mistake.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.cc:67: }
On 2014/06/23 23:12:16, wtc wrote:
> 
> If it is safe to do so, it is cheaper to do this:
> 
>   DCHECK(callback_.is_null());
>   WriteResult result = WritePacket(buffer, buf_len, self_address,
peer_address);
>   if (result.status == WRITE_STATUS_BLOCKED) {
>     callback_ = callback;
>   }
>   return result;

I wanted to use the DCHECK(!callback_.is_null()) in WritePacket to make sure
WritePacket is not called on its own.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_pac...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_pac...
net/quic/quic_server_packet_writer.h:31: // Use this method to write packets
rather than WritePacket: this class
On 2014/06/23 23:12:16, wtc wrote:
> 
> Should "this class" be changed to "this method"?

No, actually, QuicServerPacketWriter can't be used without callbacks anymore.
WritePacket on its own is unusable. I've hidden WritePacket and clarified the
comment.

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_tes...
File net/quic/quic_server_test.cc (right):

https://codereview.chromium.org/340433002/diff/10009/net/quic/quic_server_tes...
net/quic/quic_server_test.cc:20: // TODO(dmz) Remove "Chrome" part of name once
tools/quic is deleted.
On 2014/06/23 23:12:16, wtc wrote:
> 
> Nit: don't omit "net" in the path name. Say "net/tools/quic".

Done.

https://codereview.chromium.org/340433002/diff/230001/net/quic/quic_server.cc
File net/quic/quic_server.cc (right):

https://codereview.chromium.org/340433002/diff/230001/net/quic/quic_server.cc...
net/quic/quic_server.cc:32: const int kServerInitialFlowControlWindow = 100 *
kMaxPacketSize;
On 2014/06/23 23:12:16, wtc wrote:
> 
> Please use the 'uint32' type as in the original code.

Done.

[wtc, 2014-06-24 01:06:48]

Patch set 15 LGTM.

https://codereview.chromium.org/340433002/diff/270001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/340433002/diff/270001/net/BUILD.gn#newcode704
net/BUILD.gn:704: source_set("quic_tools") {

Why don't you call this "quic_ported_server" (the name you used in the GYP
file)?

https://codereview.chromium.org/340433002/diff/270001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/270001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:39: static_cast<int>(buf_len),

IMPORTANT: we should check whether buf_len <= MAX_INT and fail if that's false.

https://codereview.chromium.org/340433002/diff/270001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/270001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:41: void OnWriteComplete(int rv);

The Style Guide recommends that declaration order in the .h file matches the
definition order in the .cc file. So please make the two orders match as much as
possible. For example, OnWriteComplete should be declared after SetWritable.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order:

Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible.

[dmziegler, 2014-06-24 19:01:18]

PTAL (Can you verify that my buf_len size check is correct? See
https://codereview.chromium.org/340433002/patch/300001/310015#newcode75)

https://codereview.chromium.org/340433002/diff/270001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/340433002/diff/270001/net/BUILD.gn#newcode704
net/BUILD.gn:704: source_set("quic_tools") {
On 2014/06/24 01:06:48, wtc wrote:
> 
> Why don't you call this "quic_ported_server" (the name you used in the GYP
> file)?

I decided in a later CL to rename quic_ported_server to quic_tools (since it
will include all of net/tools/quic, including quic_client), and thought I might
as well do it already here so I don't have unnecessary changes later.

https://codereview.chromium.org/340433002/diff/270001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/270001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:39: static_cast<int>(buf_len),
On 2014/06/24 01:06:48, wtc wrote:
> 
> IMPORTANT: we should check whether buf_len <= MAX_INT and fail if that's
false.

Done.

https://codereview.chromium.org/340433002/diff/270001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/270001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:41: void OnWriteComplete(int rv);
On 2014/06/24 01:06:48, wtc wrote:
> 
> The Style Guide recommends that declaration order in the .h file matches the
> definition order in the .cc file. So please make the two orders match as much
as
> possible. For example, OnWriteComplete should be declared after SetWritable.
> 
> See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order:
> 
> Method definitions in the corresponding .cc file should be the same as the
> declaration order, as much as possible. 

Done.

[wtc, 2014-06-24 21:47:22]

Patch set 17 LGTM.

https://codereview.chromium.org/340433002/diff/300001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.cc (right):

https://codereview.chromium.org/340433002/diff/300001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.cc:75: if (buf_len <=
static_cast<size_t>(std::numeric_limits<int>::max())) {

I confirm this check is correct.

https://codereview.chromium.org/340433002/diff/300001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/300001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:41: void OnWriteComplete(int rv);

Does OnWriteComplete need to be public? It seems that it can be a private
method.

[dmziegler, 2014-06-24 22:36:48]

https://codereview.chromium.org/340433002/diff/300001/net/quic/quic_server_pa...
File net/quic/quic_server_packet_writer.h (right):

https://codereview.chromium.org/340433002/diff/300001/net/quic/quic_server_pa...
net/quic/quic_server_packet_writer.h:41: void OnWriteComplete(int rv);
On 2014/06/24 21:47:21, wtc wrote:
> 
> Does OnWriteComplete need to be public? It seems that it can be a private
> method.

Good point. I'll make it private in a future CL to avoid merge conflicts.

[dmziegler, 2014-06-24 22:37:21]

The CQ bit was checked by dmziegler@google.com

[commit-bot: I haz the power, 2014-06-24 22:38:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmziegler@google.com/340433002/300001

[commit-bot: I haz the power, 2014-06-25 02:33:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-25 05:14:45]

Change committed as 279602