Don't send preface-PING. Send trailing ping 200ms

Don't send preface-PING. Send trailing ping 200ms after sending the SYN_STREAM.Send PING no more than 1 ping after sendingSYN_STREAM ping. Server accepts PINGs only afterSYN_STREAM (which is a server bug). We will enablesendign PING after sending DATA after server fixes the bug.BUG=100587
R=jar,wtcTEST=network unit testsCommitted: http://src.chromium.org/viewvc/chrome?view=rev&revision=105949

[wtc, 2011-10-17 21:17:58]

Patch Set 2 LGTM.

My only complaint is that the meaning of last_sent_was_ping_
has changed, which makes the code a little confusing.

[wtc, 2011-10-17 21:37:10]

Patch Set 4 LGTM.  Thanks.

[commit-bot: I haz the power, 2011-10-17 21:37:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/8319015/6001

[jar (doing other things), 2011-10-17 22:09:27]

Two changes suggested below.

Thanks!

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:240: int SpdySession::trailing_ping_delay_time_ms_ =
200;
Will argued for keeping this at 1000, and was persuasive.  His argument is that
the common case will be a connect to a site... get top level html... connect to
get subresources... and *then* we'll send the trailing ping.  Hence he wanted to
be well over one or two round trip times... and 1 second works great.

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:531: need_to_send_ping_ = true;
Please add comment:
Some servers don't like too many pings, so we limit our current sending to no
more than one ping for any syn sent.  To do this, we avoid ever setting this to
true unless we send a syn (which we have just done).  This approach may change
over time as servers change their responses to
pings.

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:531: bool need_to_send_ping_;
Nice improvement on comments!

[jar (doing other things), 2011-10-17 22:40:05]

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1545: // TODO(rtenneti): Enable sending Preface-PING
after server fix.
Suggest:
TODO(rtenneti): Send preface pings when more servers support additional pings.

[commit-bot: I haz the power, 2011-10-17 23:00:27]

Change committed as 105949

[ramant (doing other things), 2011-10-17 23:40:57]

Will submit for the changes suggested by jar.

thanks,
raman

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:240: int SpdySession::trailing_ping_delay_time_ms_ =
200;
On 2011/10/17 22:09:28, jar wrote:
> Will argued for keeping this at 1000, and was persuasive.  His argument is
that
> the common case will be a connect to a site... get top level html... connect
to
> get subresources... and *then* we'll send the trailing ping.  Hence he wanted
to
> be well over one or two round trip times... and 1 second works great.

Done.

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:531: need_to_send_ping_ = true;
On 2011/10/17 22:09:28, jar wrote:
> Please add comment:
> Some servers don't like too many pings, so we limit our current sending to no
> more than one ping for any syn sent.  To do this, we avoid ever setting this
to
> true unless we send a syn (which we have just done).  This approach may change
> over time as servers change their responses to
> pings.

Done.

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1545: // TODO(rtenneti): Enable sending Preface-PING
after server fix.
On 2011/10/17 22:40:05, jar wrote:
> Suggest:
> TODO(rtenneti): Send preface pings when more servers support additional pings.

Done.

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/8319015/diff/6001/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:531: bool need_to_send_ping_;
On 2011/10/17 22:09:28, jar wrote:
> Nice improvement on comments!

Done.