Attn: Mike Belshe

Attn: Mike Belshe
From: Jonathan Leighton (leighton@cis.udel.edu)
Date: 2011-02-10
Desc: This changelist includes all files modified or added to support running
SPDY over SCTP, including the option to use a single header compression
dictionary accross all SCTP (and SPDY) streams (the default behavior), and the
option to use a separate header compression dictionary for each SCTP stream.
Changes in the code are currently marked with a comment containing JTL (my
initials).  Note that the changes currently include an embarrassment of printf()
calls...

Date: 2011-03-04, (leighton@cis.udel.edu)
- changes are based on much more recent source.
- changes were added to allow existing net_unittests to build and run.
- changes were added to allow use of direct SCTP proxy, but are incomplete.
- files exclusively related to the flip server have been excluded.
- files with no changes other than modified <EOL> characters have been excluded.

Date: 2011-03-16, (leighton@cis.udel.edu)
- changes are described in the comments for the files in the patch set, and
  address the problems previously mentioned in comments there.

Date: 2011-03-22, (leighton@cis.udel.edu)
- the majority of changes are desribed in the comments for patch 2.
- eliminated the overloaded Write() method in socket.h and in the many other
  places affected by this.
- added sctp_stream_id_ member to IOBuffer class, to avoid need for overloaded
  Write() call.  Added methods to set and get sctp_stream_id.
- fixed problems with some of the using_sctp() accessors that were interfering
  with some net_unittests.

Date: 2011-04-06, (leighton@cis.udel.edu)
- updated code with gclient sync.

[Mike Belshe, 2011-04-06 18:32:53]

http://codereview.chromium.org/6800009/diff/1/chrome/browser/debugger/devtool...
File chrome/browser/debugger/devtools_remote.h (right):

http://codereview.chromium.org/6800009/diff/1/chrome/browser/debugger/devtool...
chrome/browser/debugger/devtools_remote.h:28: virtual int protocol() {return
IPPROTO_TCP; }
nit:  in pure virtual interface classes, we try to avoid defined funcs.  (e.g.
move to .cc)

but - do we need this?

http://codereview.chromium.org/6800009/diff/1/chrome/browser/debugger/devtool...
File chrome/browser/debugger/devtools_remote_listen_socket.cc (right):

http://codereview.chromium.org/6800009/diff/1/chrome/browser/debugger/devtool...
chrome/browser/debugger/devtools_remote_listen_socket.cc:90: SOCKET s =
ListenSocket::Listen(ip, port, listener->protocol());
see if we can avoid this...

http://codereview.chromium.org/6800009/diff/1/chrome/browser/upgrade_detector.cc
File chrome/browser/upgrade_detector.cc (right):

http://codereview.chromium.org/6800009/diff/1/chrome/browser/upgrade_detector...
chrome/browser/upgrade_detector.cc:154: return;
This is great - lets put this in a separate CL.

http://codereview.chromium.org/6800009/diff/1/jingle/notifier/base/xmpp_clien...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/6800009/diff/1/jingle/notifier/base/xmpp_clien...
jingle/notifier/base/xmpp_client_socket_factory.h:33: virtual net::ClientSocket*
CreateSCTPClientSocket(
artifact of changing the ClientSocketFactory interface...

http://codereview.chromium.org/6800009/diff/1/net/base/io_buffer.h
File net/base/io_buffer.h (right):

http://codereview.chromium.org/6800009/diff/1/net/base/io_buffer.h#newcode28
net/base/io_buffer.h:28: sctp_stream_id_ = sctp_stream_id;
lets try to make a derived IOBuffer work instead.

http://codereview.chromium.org/6800009/diff/1/net/base/listen_socket.h
File net/base/listen_socket.h (right):

http://codereview.chromium.org/6800009/diff/1/net/base/listen_socket.h#newcode61
net/base/listen_socket.h:61: virtual int protocol() { return IPPROTO_TCP; }
nit: const function

http://codereview.chromium.org/6800009/diff/1/net/http/http_network_transacti...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/http/http_network_transacti...
net/http/http_network_transaction.cc:50: #include
"net/socket/sctp_client_socket_pool.h"
nit: I don't think we need this.

http://codereview.chromium.org/6800009/diff/1/net/http/http_proxy_client_sock...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/http/http_proxy_client_sock...
net/http/http_proxy_client_socket_pool.cc:364: ConnectionTimeout(), tcp_pool_,
ssl_pool_,
nit:  I don't think we need this.

http://codereview.chromium.org/6800009/diff/1/net/http/http_stream_factory_im...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/http/http_stream_factory_im...
net/http/http_stream_factory_impl_job.cc:68: CHECK(session);
nit:  accidental?

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket.h
File net/socket/client_socket.h (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket.h#newc...
net/socket/client_socket.h:79: virtual int protocol() { return IPPROTO_TCP; }
The protocol is good, lets put that into another CL.

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket.h#newc...
net/socket/client_socket.h:84: 
will revisit....  don't want to expose these in ClientSocket.

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_factor...
File net/socket/client_socket_factory.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_factor...
net/socket/client_socket_factory.cc:58: NetLog* net_log,
Hopefully we get agreement to rename this and then we nix a whole bunch of
copy-and-paste code that you did.

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_factory.h
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_factor...
net/socket/client_socket_factory.h:40: const AddressList& addresses,
nit:  figure out if we can remove this?

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_pool_b...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_pool_b...
net/socket/client_socket_pool_base.cc:33: bool g_connect_backup_jobs_enabled =
true;
this comment change is part of the TCP -> Transport renaming

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_pool_b...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_pool_b...
net/socket/client_socket_pool_base_unittest.cc:63: CompletionCallback* /*
callback */) {
This looks obsolete.

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_pool_m...
File net/socket/client_socket_pool_manager.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_pool_m...
net/socket/client_socket_pool_manager.cc:82: printf("InitSocketPoolHelper:
forcing using_ssl = false\n");
nix this

http://codereview.chromium.org/6800009/diff/1/net/socket/client_socket_pool_m...
net/socket/client_socket_pool_manager.cc:574: g_max_sockets_per_group = 1;
can this be 

  if (sctp_enabled())
    g_max_sockets_per_group = 1;

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
File net/socket/sctp_client_socket_libevent.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:92: void SetSCTPInitMsg(int fd) {
I would rename to "SetSCTPInitialStreams"

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:104: 
nit: remove lines 104/105

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:181: read_watcher_(this),
nit: missed 
  read_buf_len_(0)
  write_buf_len_(0)

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:336: if (rv < 0 )
This must be a bug - missing curly braces, causes this function to always set rv
= ERR_UNEXPECTED?

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:350: rv = ERR_UNEXPECTED;
the return value is |result| not |rv|, so these aren't working...

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:432: if (!(stream_id & 0x00000001))
What is this doing?

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:449: int nread =
HANDLE_EINTR(read(socket_, buf->data(), buf_len));
I was surprised to see we didn't need to know the stream-id for this.

OK - this a bug for the multi-packet spdy frame reading...  we should at least
add a TODO() that it is known to be broken...

In this current model, it probably needs to call buf->set_sctp_stream_id(xx)?

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:560: //inet_aton("127.0.0.1",
&(sin.sin_addr));
Wow - what a mess!

Not sure what to do to make this code usable?  Maybe working around the bug
should be put into a special function with an ifdef or something...

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_l...
net/socket/sctp_client_socket_libevent.cc:694: bytes_transferred =
HANDLE_EINTR(write(socket_, write_buf_->data(),
i think this should be InternalWrite()?

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_p...
File net/socket/sctp_client_socket_pool.h (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_p...
net/socket/sctp_client_socket_pool.h:17: #include "net/base/host_resolver.h"
Nix this clone.

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_u...
File net/socket/sctp_client_socket_unittest.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/sctp_client_socket_u...
net/socket/sctp_client_socket_unittest.cc:29: : public PlatformTest, public
ListenSocket::ListenSocketDelegate {
Since this is a clone of TCPClientSocketTest, lets find a way to not have to
clone the code.

http://codereview.chromium.org/6800009/diff/1/net/socket/ssl_server_socket_un...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/socket/ssl_server_socket_un...
net/socket/ssl_server_socket_unittest.cc:124: CompletionCallback* callback) {
This is old...

http://codereview.chromium.org/6800009/diff/1/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

http://codereview.chromium.org/6800009/diff/1/net/spdy/spdy_framer.cc#newcode24
net/spdy/spdy_framer.cc:24: 
delete lines 22-25.

http://codereview.chromium.org/6800009/diff/1/net/spdy/spdy_framer.h
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/6800009/diff/1/net/spdy/spdy_framer.h#newcode79
net/spdy/spdy_framer.h:79: virtual bool using_sctp() { return false; }
const functions

http://codereview.chromium.org/6800009/diff/1/net/spdy/spdy_framer.h#newcode83
net/spdy/spdy_framer.h:83: virtual bool using_sctp_control_stream() { return
false; }
const

http://codereview.chromium.org/6800009/diff/1/net/spdy/spdy_framer.h#newcode86
net/spdy/spdy_framer.h:86: virtual uint16 MapSpdyToSctp(uint32 stream_id) {
return 0; }
const