SPDY - integration of spdy/3 code.

SPDY - integration of spdy/3 code.

TEST=network unit tests and browser unit_tests
R=willchan
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126175

[ramant (doing other things), 2012-03-09 06:44:16]

Hi Hasan, Ryan and William,
  The following CL has changes to support SPDY/3.

 Hasan and I have tested chrome with SPDY/2, SPDY/2.1 and SPDY/3 protocol
negotiation. The following are some of the high level changes.

1) OnSettings is removed. We get called for each Setting incrementally
(OnSetting). BufferedSpdyFramer forwards the OnSetting call to SpdySession which
updates the HttpServerProperties. SetSetting persists the setting (if it is be
persisted). 

   For SPDY/3 when we send Settings, the settings are unique and sorted (these
are the settings from HttpServerProperties). 

  The Setting is logged in net logs similar to the Settings.

2) SettingsFlagsAndId data structure has changed (from Hasan  to support
SPDY/3).

3) frames have either 2 or 3 to specify the version number (hasan)

4) SpdyFramer and below - the code is very close to Hasan's code 

5) Added --use-spdy=--v3 to enable SPDY/3

6) Deleted SendCredentialsEC from SPDY2 tests.

7) Deleted enabling/disabling of flow control in some of the unit tests (see
below for some more cleanup)

8) Added  ResetStaticSettingsToInit to SpdySession to reset the default
protocol. It is called after each unit test is run.

9) Deleted ParseHeaderBlock from BufferedSpdyFramer (it is not used in the
code).

10) By default we run in SPDY/2 mode. SPDY/2.1 and SPDY/3 are behind command
line options.

11) Retired --v1 (SPDY/1) option. Heard from willchan and hkhalil there are no
SPDY/1 servers.


TODO
----
Some of the things to clean up (would like to do them in another CL if there are
no objections).

1) Consolidate HttpServerProperties's SetSpdySettings (is a convenient function
used only in unit tests) and SetSpdySetting 
2) Remove the global use_flow_control (change the unit tests not to set that
global).
3) unit tests - automatically pick up which protocol we are testing (kProtoSPDY2
or kProtoSPDY3). We call set_default_protocol in SetUp.
4) We are using StringPiece. Will change to character buffer (after Hasan is
done).
5) SpdyFramerTest (unittests from Hasan) are tested for SPDY/2 (we need to run
unittests for SPDY/3 as part of chrome unit tests).

Would appreciate if you could take a quick look at the changes and give
feedback.

thanks very much,
raman

[willchan no longer on Chromium, 2012-03-09 17:45:19]

Ryan/Hasan, can you guys take a first pass at this? I'll take a look too
later to make sure things are sane, but I'd like to rely on Ryan especially
for most of this review.

On Thu, Mar 8, 2012 at 10:44 PM, <rtenneti@chromium.org> wrote:

> Reviewers: willchan, Ryan Hamilton, hkhalil,
>
> Message:
> Hi Hasan, Ryan and William,
>  The following CL has changes to support SPDY/3.
>
>  Hasan and I have tested chrome with SPDY/2, SPDY/2.1 and SPDY/3 protocol
> negotiation. The following are some of the high level changes.
>
> 1) OnSettings is removed. We get called for each Setting incrementally
> (OnSetting). BufferedSpdyFramer forwards the OnSetting call to SpdySession
> which
> updates the HttpServerProperties. SetSetting persists the setting (if it
> is be
> persisted).
>
>   For SPDY/3 when we send Settings, the settings are unique and sorted
> (these
> are the settings from HttpServerProperties).
>
>  The Setting is logged in net logs similar to the Settings.
>
> 2) SettingsFlagsAndId data structure has changed (from Hasan  to support
> SPDY/3).
>
> 3) frames have either 2 or 3 to specify the version number (hasan)
>
> 4) SpdyFramer and below - the code is very close to Hasan's code
>
> 5) Added --use-spdy=--v3 to enable SPDY/3
>
> 6) Deleted SendCredentialsEC from SPDY2 tests.
>
> 7) Deleted enabling/disabling of flow control in some of the unit tests
> (see
> below for some more cleanup)
>
> 8) Added  ResetStaticSettingsToInit to SpdySession to reset the default
> protocol. It is called after each unit test is run.
>
> 9) Deleted ParseHeaderBlock from BufferedSpdyFramer (it is not used in the
> code).
>
> 10) By default we run in SPDY/2 mode. SPDY/2.1 and SPDY/3 are behind
> command
> line options.
>
> 11) Retired --v1 (SPDY/1) option. Heard from willchan and hkhalil there
> are no
> SPDY/1 servers.
>
>
> TODO
> ----
> Some of the things to clean up (would like to do them in another CL if
> there are
> no objections).
>
> 1) Consolidate HttpServerProperties's SetSpdySettings (is a convenient
> function
> used only in unit tests) and SetSpdySetting
> 2) Remove the global use_flow_control (change the unit tests not to set
> that
> global).
> 3) unit tests - automatically pick up which protocol we are testing
> (kProtoSPDY2
> or kProtoSPDY3). We call set_default_protocol in SetUp.
> 4) We are using StringPiece. Will change to character buffer (after Hasan
> is
> done).
> 5) SpdyFramerTest (unittests from Hasan) are tested for SPDY/2 (we need to
> run
> unittests for SPDY/3 as part of chrome unit tests).
>
> Would appreciate if you could take a quick look at the changes and give
> feedback.
>
> thanks very much,
> raman
>
> Description:
> SPDY - integration of spdy/3 code.
>
> TEST=network unit tests and browser unit_tests
> R=willchan
>
> Please review this at
http://codereview.chromium.**org/9618002/<http://codereview.chromium.org/9618...
>
> SVN Base: svn://chrome-svn/chrome/trunk/**src/
>
> Affected files:
>  M     chrome/browser/net/http_**server_properties_manager.h
>  M     chrome/browser/net/http_**server_properties_manager.cc
>  M     chrome/browser/net/http_**server_properties_manager_**unittest.cc
>  M     net/base/net_test_suite.cc
>  M     net/http/http_network_layer.cc
>  M     net/http/http_network_**transaction_spdy21_unittest.cc
>  M     net/http/http_network_**transaction_spdy2_unittest.cc
>  M     net/http/http_network_**transaction_spdy3_unittest.cc
>  M     net/http/http_proxy_client_**socket_pool_spdy3_unittest.cc
>  M     net/http/http_server_**properties.h
>  M     net/http/http_server_**properties.cc
>  M     net/http/http_server_**properties_impl.h
>  M     net/http/http_server_**properties_impl.cc
>  M     net/http/http_server_**properties_impl_unittest.cc
>  M     net/http/http_stream_factory.**cc
>  M     net/net.gyp
>  M     net/socket/ssl_client_socket.**cc
>  M     net/socket/ssl_client_socket_**pool.cc
>  M     net/spdy/buffered_spdy_framer.**h
>  M     net/spdy/buffered_spdy_framer.**cc
>  M     net/spdy/buffered_spdy_framer_**spdy2_unittest.cc
>  M     net/spdy/buffered_spdy_framer_**spdy3_unittest.cc
>  M     net/spdy/spdy_bitmasks.h
>  M     net/spdy/spdy_frame_builder.h
>  M     net/spdy/spdy_frame_builder.cc
>  A     net/spdy/spdy_frame_reader.h
>  A     net/spdy/spdy_frame_reader.cc
>  A     net/spdy/spdy_frame_reader_**test.cc
>  M     net/spdy/spdy_framer.h
>  M     net/spdy/spdy_framer.cc
>  M     net/spdy/spdy_framer_test.cc
>  M     net/spdy/spdy_http_stream_**spdy2_unittest.cc
>  M     net/spdy/spdy_http_stream_**spdy3_unittest.cc
>  M     net/spdy/spdy_network_**transaction_spdy21_unittest.cc
>  M     net/spdy/spdy_network_**transaction_spdy2_unittest.cc
>  M     net/spdy/spdy_network_**transaction_spdy3_unittest.cc
>  M     net/spdy/spdy_protocol.h
>  M     net/spdy/spdy_protocol_test.cc
>  M     net/spdy/spdy_proxy_client_**socket_spdy2_unittest.cc
>  M     net/spdy/spdy_proxy_client_**socket_spdy3_unittest.cc
>  M     net/spdy/spdy_session.h
>  M     net/spdy/spdy_session.cc
>  M     net/spdy/spdy_session_spdy2_**unittest.cc
>  M     net/spdy/spdy_session_spdy3_**unittest.cc
>  M     net/spdy/spdy_settings_**storage.cc
>  M     net/spdy/spdy_stream_spdy2_**unittest.cc
>  M     net/spdy/spdy_stream_spdy3_**unittest.cc
>  M     net/spdy/spdy_websocket_**stream_spdy2_unittest.cc
>  M     net/spdy/spdy_websocket_**stream_spdy3_unittest.cc
>  M     net/tools/flip_server/spdy_**interface.h
>  M     net/tools/flip_server/spdy_**interface.cc
>  M     net/websockets/websocket_job_**spdy2_unittest.cc
>  M     net/websockets/websocket_job_**spdy3_unittest.cc
>
>
>

[rch (use chromium not google), 2012-03-09 17:47:47]

Yup, I'm working on the review as we speak!

On Fri, Mar 9, 2012 at 9:45 AM, William Chan (陈智昌) <willchan@chromium.org>wrote:

> Ryan/Hasan, can you guys take a first pass at this? I'll take a look too
> later to make sure things are sane, but I'd like to rely on Ryan especially
> for most of this review.
>
>
> On Thu, Mar 8, 2012 at 10:44 PM, <rtenneti@chromium.org> wrote:
>
>> Reviewers: willchan, Ryan Hamilton, hkhalil,
>>
>> Message:
>> Hi Hasan, Ryan and William,
>>  The following CL has changes to support SPDY/3.
>>
>>  Hasan and I have tested chrome with SPDY/2, SPDY/2.1 and SPDY/3 protocol
>> negotiation. The following are some of the high level changes.
>>
>> 1) OnSettings is removed. We get called for each Setting incrementally
>> (OnSetting). BufferedSpdyFramer forwards the OnSetting call to
>> SpdySession which
>> updates the HttpServerProperties. SetSetting persists the setting (if it
>> is be
>> persisted).
>>
>>   For SPDY/3 when we send Settings, the settings are unique and sorted
>> (these
>> are the settings from HttpServerProperties).
>>
>>  The Setting is logged in net logs similar to the Settings.
>>
>> 2) SettingsFlagsAndId data structure has changed (from Hasan  to support
>> SPDY/3).
>>
>> 3) frames have either 2 or 3 to specify the version number (hasan)
>>
>> 4) SpdyFramer and below - the code is very close to Hasan's code
>>
>> 5) Added --use-spdy=--v3 to enable SPDY/3
>>
>> 6) Deleted SendCredentialsEC from SPDY2 tests.
>>
>> 7) Deleted enabling/disabling of flow control in some of the unit tests
>> (see
>> below for some more cleanup)
>>
>> 8) Added  ResetStaticSettingsToInit to SpdySession to reset the default
>> protocol. It is called after each unit test is run.
>>
>> 9) Deleted ParseHeaderBlock from BufferedSpdyFramer (it is not used in the
>> code).
>>
>> 10) By default we run in SPDY/2 mode. SPDY/2.1 and SPDY/3 are behind
>> command
>> line options.
>>
>> 11) Retired --v1 (SPDY/1) option. Heard from willchan and hkhalil there
>> are no
>> SPDY/1 servers.
>>
>>
>> TODO
>> ----
>> Some of the things to clean up (would like to do them in another CL if
>> there are
>> no objections).
>>
>> 1) Consolidate HttpServerProperties's SetSpdySettings (is a convenient
>> function
>> used only in unit tests) and SetSpdySetting
>> 2) Remove the global use_flow_control (change the unit tests not to set
>> that
>> global).
>> 3) unit tests - automatically pick up which protocol we are testing
>> (kProtoSPDY2
>> or kProtoSPDY3). We call set_default_protocol in SetUp.
>> 4) We are using StringPiece. Will change to character buffer (after Hasan
>> is
>> done).
>> 5) SpdyFramerTest (unittests from Hasan) are tested for SPDY/2 (we need
>> to run
>> unittests for SPDY/3 as part of chrome unit tests).
>>
>> Would appreciate if you could take a quick look at the changes and give
>> feedback.
>>
>> thanks very much,
>> raman
>>
>> Description:
>> SPDY - integration of spdy/3 code.
>>
>> TEST=network unit tests and browser unit_tests
>> R=willchan
>>
>> Please review this at
http://codereview.chromium.**org/9618002/<http://codereview.chromium.org/9618...
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/**src/
>>
>> Affected files:
>>  M     chrome/browser/net/http_**server_properties_manager.h
>>  M     chrome/browser/net/http_**server_properties_manager.cc
>>  M     chrome/browser/net/http_**server_properties_manager_**unittest.cc
>>  M     net/base/net_test_suite.cc
>>  M     net/http/http_network_layer.cc
>>  M     net/http/http_network_**transaction_spdy21_unittest.cc
>>  M     net/http/http_network_**transaction_spdy2_unittest.cc
>>  M     net/http/http_network_**transaction_spdy3_unittest.cc
>>  M     net/http/http_proxy_client_**socket_pool_spdy3_unittest.cc
>>  M     net/http/http_server_**properties.h
>>  M     net/http/http_server_**properties.cc
>>  M     net/http/http_server_**properties_impl.h
>>  M     net/http/http_server_**properties_impl.cc
>>  M     net/http/http_server_**properties_impl_unittest.cc
>>  M     net/http/http_stream_factory.**cc
>>  M     net/net.gyp
>>  M     net/socket/ssl_client_socket.**cc
>>  M     net/socket/ssl_client_socket_**pool.cc
>>  M     net/spdy/buffered_spdy_framer.**h
>>  M     net/spdy/buffered_spdy_framer.**cc
>>  M     net/spdy/buffered_spdy_framer_**spdy2_unittest.cc
>>  M     net/spdy/buffered_spdy_framer_**spdy3_unittest.cc
>>  M     net/spdy/spdy_bitmasks.h
>>  M     net/spdy/spdy_frame_builder.h
>>  M     net/spdy/spdy_frame_builder.cc
>>  A     net/spdy/spdy_frame_reader.h
>>  A     net/spdy/spdy_frame_reader.cc
>>  A     net/spdy/spdy_frame_reader_**test.cc
>>  M     net/spdy/spdy_framer.h
>>  M     net/spdy/spdy_framer.cc
>>  M     net/spdy/spdy_framer_test.cc
>>  M     net/spdy/spdy_http_stream_**spdy2_unittest.cc
>>  M     net/spdy/spdy_http_stream_**spdy3_unittest.cc
>>  M     net/spdy/spdy_network_**transaction_spdy21_unittest.cc
>>  M     net/spdy/spdy_network_**transaction_spdy2_unittest.cc
>>  M     net/spdy/spdy_network_**transaction_spdy3_unittest.cc
>>  M     net/spdy/spdy_protocol.h
>>  M     net/spdy/spdy_protocol_test.cc
>>  M     net/spdy/spdy_proxy_client_**socket_spdy2_unittest.cc
>>  M     net/spdy/spdy_proxy_client_**socket_spdy3_unittest.cc
>>  M     net/spdy/spdy_session.h
>>  M     net/spdy/spdy_session.cc
>>  M     net/spdy/spdy_session_spdy2_**unittest.cc
>>  M     net/spdy/spdy_session_spdy3_**unittest.cc
>>  M     net/spdy/spdy_settings_**storage.cc
>>  M     net/spdy/spdy_stream_spdy2_**unittest.cc
>>  M     net/spdy/spdy_stream_spdy3_**unittest.cc
>>  M     net/spdy/spdy_websocket_**stream_spdy2_unittest.cc
>>  M     net/spdy/spdy_websocket_**stream_spdy3_unittest.cc
>>  M     net/tools/flip_server/spdy_**interface.h
>>  M     net/tools/flip_server/spdy_**interface.cc
>>  M     net/websockets/websocket_job_**spdy2_unittest.cc
>>  M     net/websockets/websocket_job_**spdy3_unittest.cc
>>
>>
>>
>

[ramant (doing other things), 2012-03-09 18:34:32]

The following TODO came up while discussing the code with jar.

One more TODO is to convert SpdySetting to std::map instead of std::list.

1) Remove the unique/sort code from SendSettings
2) Add version to persistent http_server properties and if there is no version,
then throw away the settings and persist sorted/unique properties and when
version is there if we come across a duplicate property when we read the data,
then ignore that property (may be).


On 2012/03/09 17:47:47, rch wrote:
> Yup, I'm working on the review as we speak!
> 
> On Fri, Mar 9, 2012 at 9:45 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
> 
> > Ryan/Hasan, can you guys take a first pass at this? I'll take a look too
> > later to make sure things are sane, but I'd like to rely on Ryan especially
> > for most of this review.
> >
> >
> > On Thu, Mar 8, 2012 at 10:44 PM, <mailto:rtenneti@chromium.org> wrote:
> >
> >> Reviewers: willchan, Ryan Hamilton, hkhalil,
> >>
> >> Message:
> >> Hi Hasan, Ryan and William,
> >>  The following CL has changes to support SPDY/3.
> >>
> >>  Hasan and I have tested chrome with SPDY/2, SPDY/2.1 and SPDY/3 protocol
> >> negotiation. The following are some of the high level changes.
> >>
> >> 1) OnSettings is removed. We get called for each Setting incrementally
> >> (OnSetting). BufferedSpdyFramer forwards the OnSetting call to
> >> SpdySession which
> >> updates the HttpServerProperties. SetSetting persists the setting (if it
> >> is be
> >> persisted).
> >>
> >>   For SPDY/3 when we send Settings, the settings are unique and sorted
> >> (these
> >> are the settings from HttpServerProperties).
> >>
> >>  The Setting is logged in net logs similar to the Settings.
> >>
> >> 2) SettingsFlagsAndId data structure has changed (from Hasan  to support
> >> SPDY/3).
> >>
> >> 3) frames have either 2 or 3 to specify the version number (hasan)
> >>
> >> 4) SpdyFramer and below - the code is very close to Hasan's code
> >>
> >> 5) Added --use-spdy=--v3 to enable SPDY/3
> >>
> >> 6) Deleted SendCredentialsEC from SPDY2 tests.
> >>
> >> 7) Deleted enabling/disabling of flow control in some of the unit tests
> >> (see
> >> below for some more cleanup)
> >>
> >> 8) Added  ResetStaticSettingsToInit to SpdySession to reset the default
> >> protocol. It is called after each unit test is run.
> >>
> >> 9) Deleted ParseHeaderBlock from BufferedSpdyFramer (it is not used in the
> >> code).
> >>
> >> 10) By default we run in SPDY/2 mode. SPDY/2.1 and SPDY/3 are behind
> >> command
> >> line options.
> >>
> >> 11) Retired --v1 (SPDY/1) option. Heard from willchan and hkhalil there
> >> are no
> >> SPDY/1 servers.
> >>
> >>
> >> TODO
> >> ----
> >> Some of the things to clean up (would like to do them in another CL if
> >> there are
> >> no objections).
> >>
> >> 1) Consolidate HttpServerProperties's SetSpdySettings (is a convenient
> >> function
> >> used only in unit tests) and SetSpdySetting
> >> 2) Remove the global use_flow_control (change the unit tests not to set
> >> that
> >> global).
> >> 3) unit tests - automatically pick up which protocol we are testing
> >> (kProtoSPDY2
> >> or kProtoSPDY3). We call set_default_protocol in SetUp.
> >> 4) We are using StringPiece. Will change to character buffer (after Hasan
> >> is
> >> done).
> >> 5) SpdyFramerTest (unittests from Hasan) are tested for SPDY/2 (we need
> >> to run
> >> unittests for SPDY/3 as part of chrome unit tests).
> >>
> >> Would appreciate if you could take a quick look at the changes and give
> >> feedback.
> >>
> >> thanks very much,
> >> raman
> >>
> >> Description:
> >> SPDY - integration of spdy/3 code.
> >>
> >> TEST=network unit tests and browser unit_tests
> >> R=willchan
> >>
> >> Please review this at
>
http://codereview.chromium.**org/9618002/%3Chttp://codereview.chromium.org/96...>
> >>
> >> SVN Base: svn://chrome-svn/chrome/trunk/**src/
> >>
> >> Affected files:
> >>  M     chrome/browser/net/http_**server_properties_manager.h
> >>  M     chrome/browser/net/http_**server_properties_manager.cc
> >>  M     chrome/browser/net/http_**server_properties_manager_**unittest.cc
> >>  M     net/base/net_test_suite.cc
> >>  M     net/http/http_network_layer.cc
> >>  M     net/http/http_network_**transaction_spdy21_unittest.cc
> >>  M     net/http/http_network_**transaction_spdy2_unittest.cc
> >>  M     net/http/http_network_**transaction_spdy3_unittest.cc
> >>  M     net/http/http_proxy_client_**socket_pool_spdy3_unittest.cc
> >>  M     net/http/http_server_**properties.h
> >>  M     net/http/http_server_**properties.cc
> >>  M     net/http/http_server_**properties_impl.h
> >>  M     net/http/http_server_**properties_impl.cc
> >>  M     net/http/http_server_**properties_impl_unittest.cc
> >>  M     net/http/http_stream_factory.**cc
> >>  M     net/net.gyp
> >>  M     net/socket/ssl_client_socket.**cc
> >>  M     net/socket/ssl_client_socket_**pool.cc
> >>  M     net/spdy/buffered_spdy_framer.**h
> >>  M     net/spdy/buffered_spdy_framer.**cc
> >>  M     net/spdy/buffered_spdy_framer_**spdy2_unittest.cc
> >>  M     net/spdy/buffered_spdy_framer_**spdy3_unittest.cc
> >>  M     net/spdy/spdy_bitmasks.h
> >>  M     net/spdy/spdy_frame_builder.h
> >>  M     net/spdy/spdy_frame_builder.cc
> >>  A     net/spdy/spdy_frame_reader.h
> >>  A     net/spdy/spdy_frame_reader.cc
> >>  A     net/spdy/spdy_frame_reader_**test.cc
> >>  M     net/spdy/spdy_framer.h
> >>  M     net/spdy/spdy_framer.cc
> >>  M     net/spdy/spdy_framer_test.cc
> >>  M     net/spdy/spdy_http_stream_**spdy2_unittest.cc
> >>  M     net/spdy/spdy_http_stream_**spdy3_unittest.cc
> >>  M     net/spdy/spdy_network_**transaction_spdy21_unittest.cc
> >>  M     net/spdy/spdy_network_**transaction_spdy2_unittest.cc
> >>  M     net/spdy/spdy_network_**transaction_spdy3_unittest.cc
> >>  M     net/spdy/spdy_protocol.h
> >>  M     net/spdy/spdy_protocol_test.cc
> >>  M     net/spdy/spdy_proxy_client_**socket_spdy2_unittest.cc
> >>  M     net/spdy/spdy_proxy_client_**socket_spdy3_unittest.cc
> >>  M     net/spdy/spdy_session.h
> >>  M     net/spdy/spdy_session.cc
> >>  M     net/spdy/spdy_session_spdy2_**unittest.cc
> >>  M     net/spdy/spdy_session_spdy3_**unittest.cc
> >>  M     net/spdy/spdy_settings_**storage.cc
> >>  M     net/spdy/spdy_stream_spdy2_**unittest.cc
> >>  M     net/spdy/spdy_stream_spdy3_**unittest.cc
> >>  M     net/spdy/spdy_websocket_**stream_spdy2_unittest.cc
> >>  M     net/spdy/spdy_websocket_**stream_spdy3_unittest.cc
> >>  M     net/tools/flip_server/spdy_**interface.h
> >>  M     net/tools/flip_server/spdy_**interface.cc
> >>  M     net/websockets/websocket_job_**spdy2_unittest.cc
> >>  M     net/websockets/websocket_job_**spdy3_unittest.cc
> >>
> >>
> >>
> >

[Ryan Hamilton, 2012-03-09 19:09:58]

Your cl is not long enough, please change more files :>

Ok, joking aside I have a couple of important changes that I think need to be
made:
1) Please make sure we're testing all spdy framer versions. 
SPDY_VERSION_FOR_TESTS does not seem to be what we want.
2) Please get rid of the use_flow_control_ static in SpdySession
3) Please use kProtoUnknown as the default value for SPDY protocol, unless
explicitly set.

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_layer.cc
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_layer...
net/http/http_network_layer.cc:108: next_protos.push_back("spdy/2");
Why is spdy/2.1 not in this list?

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
net/http/http_network_transaction_spdy21_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY2);
Shouldn't this say kProtoSPDY21?  And if so, I wonder why all the tests are
passing...

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
File net/http/http_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
net/http/http_network_transaction_spdy3_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY3);
Is this required to be set of all tests in this file?  I hope not...

http://codereview.chromium.org/9618002/diff/21007/net/http/http_server_proper...
File net/http/http_server_properties.h (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_server_proper...
net/http/http_server_properties.h:94: virtual bool SetSpdySettings(const
HostPortPair& host_port_pair,
If this is only used by unit tests, then perhaps we can add a utility method
somewhere in a _test file.  Can you add a TODO here for this?

http://codereview.chromium.org/9618002/diff/21007/net/spdy/buffered_spdy_fram...
File net/spdy/buffered_spdy_framer.h (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.h:67: virtual void OnSetting(SpdySettingsIds id,
uint8 flags, uint32 value) = 0;
Please add a comment.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.h:137: SpdyPriority GetHighestPriority() const;
Hm.  This seems fine, but I wonder why we don't have other similar methods?

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

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:68: size_t
SpdyFramer::kUncompressedControlFrameBufferInitialSize = 18 + 16;
Can you add a comment explaining why this is the right value?  What are 18 and
16?

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.cc
File net/spdy/spdy_framer_test.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.c...
net/spdy/spdy_framer_test.cc:19: unsigned char kVer = SPDY_VERSION_FOR_TESTS;
We need to test both spdy2 and spdy2, right?  I think we should fork
spdy_framer_test.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.c...
net/spdy/spdy_framer_test.cc:38: // the SpdyFramer.  The implemention is meant
for use only in tests
Since it is defined in a _test file, I don't think it needs to mention that it
is test only in the comment.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.c...
net/spdy/spdy_framer_test.cc:157: static const char kHexChars[] =
"0123456789abcdef";
Out of curiosity, why is this changing?

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.c...
net/spdy/spdy_framer_test.cc:198: const int min_len = (actual_len >
expected_len) ? expected_len : actual_len;
const int min_len = min(actual_len, expected_len);
const int max_len = max(actual_len, expected_len);

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h
File net/spdy/spdy_protocol.h (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h#ne...
net/spdy/spdy_protocol.h:148: const int SPDY_VERSION_FOR_TESTS = 2;
Oh, now I see where this is defined.  This does not seem like a good idea.  We
need to test both versions.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h#ne...
net/spdy/spdy_protocol.h:156: const int kSpdyProtocolVersion = 2;
Where is this used?

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h#ne...
net/spdy/spdy_protocol.h:384: NOOP,  // No longer valid kept for
identifiability/enum order.
This class is shared between spdy 2 and spdy 3, so this comment is not quite
accurate.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h#ne...
net/spdy/spdy_protocol.h:662: type != NOOP;  // NOOP is not a 'valid' control
frame.
NOOP is valid in SPDY/2

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol_test.cc
File net/spdy/spdy_protocol_test.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol_test...
net/spdy/spdy_protocol_test.cc:53: EXPECT_EQ(12u,
SpdySynReplyControlFrame::size());
Why is this changing?  Just want to make sure we're not regressing spdy/2
support in some way.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol_test...
net/spdy/spdy_protocol_test.cc:95: SpdyFramer framer(SPDY_VERSION_FOR_TESTS);
Where is this defined?  We want to make sure we're testing both / all spdy
versions.  Is that happening?

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol_test...
net/spdy/spdy_protocol_test.cc:353: // NOOP frames aren't 'valid'.
NOOP is valid in spdy/2 right?

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

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:271: SpdySession::FlowControl
SpdySession::use_flow_control_ =
As we discussed yesterday, please remove this variable.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:275: SSLClientSocket::kProtoSPDY2;
I think that the default value should be set to kProtoUnknown, since we should
never use this value unless it is specified by command line value, or by unit
test.

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

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:190: // Specify which protocol to use. Used by unit
tests. This only applies for
// Specify the SPDY protocol to be used for SPDY session which do not use NPN to
negotiate a particular protocol.  This is only called from unit tests.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session_spdy3...
File net/spdy/spdy_session_spdy3_unittest.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session_spdy3...
net/spdy/spdy_session_spdy3_unittest.cc:717:
settings.push_back(spdy::SpdySetting(id1, kBogusSettingValue));
Can you file a bug against me to to add a CreateSpdySettings() method that takes
3 arguments: id, flags, value and use it.  It would clean up code like this a
bit.

http://codereview.chromium.org/9618002/diff/21007/net/tools/flip_server/spdy_...
File net/tools/flip_server/spdy_interface.h (right):

http://codereview.chromium.org/9618002/diff/21007/net/tools/flip_server/spdy_...
net/tools/flip_server/spdy_interface.h:72: const spdy::SpdySettingsControlFrame&
frame) OVERRIDE {}
Is OnSettings still required?  I think BufferedSpdyFramer doesn't call it, but I
could be wrong.

[ramant (doing other things), 2012-03-10 01:14:09]

Hi Ryan,
  Many many thanks for the code review. There is no difference between versions
11 and 12 (except deleted kSpdyProtocolVersion. Will work with hkhalil to merge
them into the server).

thanks,
raman

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_layer.cc
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_layer...
net/http/http_network_layer.cc:108: next_protos.push_back("spdy/2");
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Why is spdy/2.1 not in this list?

I thought if we enable spdy/3, we will disable spdy/2.1. Added spdy/2.1 to
next_protos.

When I ran with this change when 3 is enabled, was able to negotiate 2, 2.1 and
3 protocols. Hope that is ok.

Done.

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
net/http/http_network_transaction_spdy21_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY2);
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Shouldn't this say kProtoSPDY21?  And if so, I wonder why all the tests are
> passing...

I haven't removed how we determine flow_control_ (it is still determined based
on use_flow_control_).

default_protocol = kProtoSpdy2 says use version 2 when we create SpdyFramer
objects. We do the same for kProtoSPDY21. Thus all tests pass because this
change doesn't change flow_control.

Done.

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
File net/http/http_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
net/http/http_network_transaction_spdy3_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY3);
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Is this required to be set of all tests in this file?  I hope not...

Added this because more than one test requires.

Have a TODO to avoid calling set_default_protocol in SetUp.

http://codereview.chromium.org/9618002/diff/21007/net/http/http_server_proper...
File net/http/http_server_properties.h (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_server_proper...
net/http/http_server_properties.h:94: virtual bool SetSpdySettings(const
HostPortPair& host_port_pair,
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> If this is only used by unit tests, then perhaps we can add a utility method
> somewhere in a _test file.  Can you add a TODO here for this?

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/buffered_spdy_fram...
File net/spdy/buffered_spdy_framer.h (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.h:67: virtual void OnSetting(SpdySettingsIds id,
uint8 flags, uint32 value) = 0;
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Please add a comment.

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.h:137: SpdyPriority GetHighestPriority() const;
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Hm.  This seems fine, but I wonder why we don't have other similar methods? 

Agreed.

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

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:68: size_t
SpdyFramer::kUncompressedControlFrameBufferInitialSize = 18 + 16;
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Can you add a comment explaining why this is the right value?  What are 18 and
> 16?

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.cc
File net/spdy/spdy_framer_test.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.c...
net/spdy/spdy_framer_test.cc:19: unsigned char kVer = SPDY_VERSION_FOR_TESTS;
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> We need to test both spdy2 and spdy2, right?  I think we should fork
> spdy_framer_test.

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.c...
net/spdy/spdy_framer_test.cc:38: // the SpdyFramer.  The implemention is meant
for use only in tests
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Since it is defined in a _test file, I don't think it needs to mention that it
> is test only in the comment.

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.c...
net/spdy/spdy_framer_test.cc:157: static const char kHexChars[] =
"0123456789abcdef";
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Out of curiosity, why is this changing?

Took the change as is from the server. Was trying to keep the code as close to
that code as possible.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_framer_test.c...
net/spdy/spdy_framer_test.cc:198: const int min_len = (actual_len >
expected_len) ? expected_len : actual_len;
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> const int min_len = min(actual_len, expected_len);
> const int max_len = max(actual_len, expected_len);

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h
File net/spdy/spdy_protocol.h (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h#ne...
net/spdy/spdy_protocol.h:148: const int SPDY_VERSION_FOR_TESTS = 2;
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Oh, now I see where this is defined.  This does not seem like a good idea.  We
> need to test both versions.

Moved this to forked versions for spdy_framer_test unittests.
Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h#ne...
net/spdy/spdy_protocol.h:156: const int kSpdyProtocolVersion = 2;
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Where is this used?

Deleted it (and changed all uses of it). Will upstream the change to server.
thanks

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h#ne...
net/spdy/spdy_protocol.h:384: NOOP,  // No longer valid kept for
identifiability/enum order.
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> This class is shared between spdy 2 and spdy 3, so this comment is not quite
> accurate.

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol.h#ne...
net/spdy/spdy_protocol.h:662: type != NOOP;  // NOOP is not a 'valid' control
frame.
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> NOOP is valid in SPDY/2

Will do the "type != NOOP" test if version !=2. Thanks for catching this.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol_test.cc
File net/spdy/spdy_protocol_test.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol_test...
net/spdy/spdy_protocol_test.cc:53: EXPECT_EQ(12u,
SpdySynReplyControlFrame::size());
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Why is this changing?  Just want to make sure we're not regressing spdy/2
> support in some way.

Added separate tests for SPDY/2 and SPDY/3.

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol_test...
net/spdy/spdy_protocol_test.cc:95: SpdyFramer framer(SPDY_VERSION_FOR_TESTS);
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Where is this defined?  We want to make sure we're testing both / all spdy
> versions.  Is that happening?

Forked this test for spdy/2 and spdy/3.

Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_protocol_test...
net/spdy/spdy_protocol_test.cc:353: // NOOP frames aren't 'valid'.
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> NOOP is valid in spdy/2 right?

Checked in SPDY/2 tests and kept the as above for spdy/3.

Done.

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

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:271: SpdySession::FlowControl
SpdySession::use_flow_control_ =
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> As we discussed yesterday, please remove this variable.

As we had discussed, will do that in a separate CL which does flow_control
changes.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:275: SSLClientSocket::kProtoSPDY2;
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> I think that the default value should be set to kProtoUnknown, since we should
> never use this value unless it is specified by command line value, or by unit
> test.

Will work with you to make this change flow_control change.

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

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:190: // Specify which protocol to use. Used by unit
tests. This only applies for
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> // Specify the SPDY protocol to be used for SPDY session which do not use NPN
to
> negotiate a particular protocol.  This is only called from unit tests.

thanks much.
Done.

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session_spdy3...
File net/spdy/spdy_session_spdy3_unittest.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session_spdy3...
net/spdy/spdy_session_spdy3_unittest.cc:717:
settings.push_back(spdy::SpdySetting(id1, kBogusSettingValue));
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Can you file a bug against me to to add a CreateSpdySettings() method that
takes
> 3 arguments: id, flags, value and use it.  It would clean up code like this a
> bit.

Will do. thanks.

http://codereview.chromium.org/9618002/diff/21007/net/tools/flip_server/spdy_...
File net/tools/flip_server/spdy_interface.h (right):

http://codereview.chromium.org/9618002/diff/21007/net/tools/flip_server/spdy_...
net/tools/flip_server/spdy_interface.h:72: const spdy::SpdySettingsControlFrame&
frame) OVERRIDE {}
On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> Is OnSettings still required?  I think BufferedSpdyFramer doesn't call it, but
I
> could be wrong.

Done.

[ramant (doing other things), 2012-03-10 02:26:20]

Changed default_protocol_ to default to kProtoUnknown.

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

http://codereview.chromium.org/9618002/diff/21007/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:275: SSLClientSocket::kProtoSPDY2;
On 2012/03/10 01:14:09, ramant wrote:
> On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> > I think that the default value should be set to kProtoUnknown, since we
should
> > never use this value unless it is specified by command line value, or by
unit
> > test.
> 
> Will work with you to make this change flow_control change.

Made the change to use kProtoUnknown.
Done.

[Ryan Hamilton, 2012-03-10 03:23:02]

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
net/http/http_network_transaction_spdy21_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY2);
On 2012/03/10 01:14:09, ramant wrote:
> On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> > Shouldn't this say kProtoSPDY21?  And if so, I wonder why all the tests are
> > passing...
> 
> I haven't removed how we determine flow_control_ (it is still determined based
> on use_flow_control_).
> 
> default_protocol = kProtoSpdy2 says use version 2 when we create SpdyFramer
> objects. We do the same for kProtoSPDY21. Thus all tests pass because this
> change doesn't change flow_control.
> 
> Done.

Ah.  Once you check this in, I'll remove use_flow_control_

http://codereview.chromium.org/9618002/diff/31011/net/http/http_network_layer.cc
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/http/http_network_layer...
net/http/http_network_layer.cc:106:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY3);
I don't think this belongs here.   The default_protocol should only be used when
we are doing spdy over non-NPN.  I think we should add this option for
kDisableSSL, kSSL.  What do you think?

http://codereview.chromium.org/9618002/diff/31011/net/spdy/buffered_spdy_fram...
File net/spdy/buffered_spdy_framer.h (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.h:68: // Called when a complete setting within a
SETTINGS frame has been parsed and
Called when an individual setting within a SETTINGS frame has been parsed and
validated.

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

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:75: size_t
SpdyFramer::kUncompressedControlFrameBufferInitialSize = 18 + 16;
Hm.  Ok, I guess this explains the value, but we don't do LOAS or VTL in chrome.
 This makes me think  we should change this value.  What do you think?

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
File net/spdy/spdy_framer_spdy2_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
net/spdy/spdy_framer_spdy2_test.cc:17: const int SPDY_VERSION_FOR_TESTS = 2;
Please remove this.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
net/spdy/spdy_framer_spdy2_test.cc:22: unsigned char kVer =
SPDY_VERSION_FOR_TESTS;
unsigned char kSpdy2 = 2;

Or some such.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
net/spdy/spdy_framer_spdy2_test.cc:620: SpdyFramer framer(kVer);
It looks like you re-ordered the tests.  Unfortunately, this makes the diff all
but impossible to read.  Is it just a re-order after this point?  If so, that's
fine...

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy3_...
File net/spdy/spdy_framer_spdy3_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy3_...
net/spdy/spdy_framer_spdy3_test.cc:17: const int SPDY_VERSION_FOR_TESTS = 3;
Please remove this.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy3_...
net/spdy/spdy_framer_spdy3_test.cc:22: unsigned char kVer =
SPDY_VERSION_FOR_TESTS;
unsigned char kSpdy2 = 2;

(Or something similar)

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
File net/spdy/spdy_protocol_spdy2_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy2_test.cc:48: const int SPDY_VERSION_FOR_TESTS = 2;
Please rename to something like kSpdy2

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy2_test.cc:109: EXPECT_EQ((SPDY_VERSION_FOR_TESTS < 3)
? 2 : 4,
no reason to have the conditional here, since we know what version we're using.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy2_test.cc:122: EXPECT_EQ((SPDY_VERSION_FOR_TESTS < 3)
? 2 : 4,
no reason to have the conditional here, since we know what version we're using.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy2_test.cc:161: EXPECT_EQ((SPDY_VERSION_FOR_TESTS < 3)
? 2 : 4,
no reason to have the conditional here, since we know what version we're using.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
File net/spdy/spdy_protocol_spdy3_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy3_test.cc:48: const int SPDY_VERSION_FOR_TESTS = 3;
Please rename to kSpdy3.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy3_test.cc:109: EXPECT_EQ((SPDY_VERSION_FOR_TESTS < 3)
? 2 : 4,
no reason to have the conditional here, since we know what version we're using.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy3_test.cc:123: syn_reply->header_block_len());
no reason to have the conditional here, since we know what version we're using.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy3_test.cc:161: EXPECT_EQ((SPDY_VERSION_FOR_TESTS < 3)
? 2 : 4,
no reason to have the conditional here, since we know what version we're using.

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

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:423: break;
I'm not sure I understand why you had to change this?

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

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:191: // to negotiate a particular protocol.  This is
only called from unit tests.
Actually, looks like this is called by both command line flags and unit tests.

[ramant (doing other things), 2012-03-10 19:45:28]

Hi Ryan,
  Many many thanks for your time (sincerely appreciate it).

  Made all the changes except for variable name changes in the code that is
shared with server (it is an easy change, don't have objections. Was trying to
keep the code as similar as possible with the server code). If you and hkhalil
let me know, will make the change.

thanks very much,
raman

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

http://codereview.chromium.org/9618002/diff/21007/net/http/http_network_trans...
net/http/http_network_transaction_spdy21_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY2);
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> On 2012/03/10 01:14:09, ramant wrote:
> > On 2012/03/09 19:09:58, Ryan Hamilton wrote:
> > > Shouldn't this say kProtoSPDY21?  And if so, I wonder why all the tests
are
> > > passing...
> > 
> > I haven't removed how we determine flow_control_ (it is still determined
based
> > on use_flow_control_).
> > 
> > default_protocol = kProtoSpdy2 says use version 2 when we create SpdyFramer
> > objects. We do the same for kProtoSPDY21. Thus all tests pass because this
> > change doesn't change flow_control.
> > 
> > Done.
> 
> Ah.  Once you check this in, I'll remove use_flow_control_

Sounds great. thx

http://codereview.chromium.org/9618002/diff/31011/net/http/http_network_layer.cc
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/http/http_network_layer...
net/http/http_network_layer.cc:106:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY3);
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> I don't think this belongs here.   The default_protocol should only be used
when
> we are doing spdy over non-NPN.  I think we should add this option for
> kDisableSSL, kSSL.  What do you think?

+1. Will test chrome when kEnableNpnHttpOnly, kDisableAltProtocols options are
enabled (to verify nothing is broken).
Done.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/buffered_spdy_fram...
File net/spdy/buffered_spdy_framer.h (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/buffered_spdy_fram...
net/spdy/buffered_spdy_framer.h:68: // Called when a complete setting within a
SETTINGS frame has been parsed and
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> Called when an individual setting within a SETTINGS frame has been parsed and
> validated.

Done.

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

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:75: size_t
SpdyFramer::kUncompressedControlFrameBufferInitialSize = 18 + 16;
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> Hm.  Ok, I guess this explains the value, but we don't do LOAS or VTL in
chrome.
>  This makes me think  we should change this value.  What do you think?

I think it could get used in unittests (when in some of the tests if we disable
compression). The above sounds good to me.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
File net/spdy/spdy_framer_spdy2_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
net/spdy/spdy_framer_spdy2_test.cc:17: const int SPDY_VERSION_FOR_TESTS = 2;
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> Please remove this.

I don't have objections to name changes.

Most of this code is from server. Would be really great if we can keep
server/chrome codes as close as possible. 
 
They have only one file and use command line to define SPDY_VERSION_FOR_TESTS
version 2 or 3 and run the tests.

Hi hkhalil: hope it is ok if we use kSpdy2 (instead of kVer and
SPDY_VERSION_FOR_TESTS) in this whole file.

What do all you think? 

Will make the change after running it by hkhalil. Hope you
don't mind.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
net/spdy/spdy_framer_spdy2_test.cc:22: unsigned char kVer =
SPDY_VERSION_FOR_TESTS;
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> unsigned char kSpdy2 = 2;
> 
> Or some such.

Sounds good to me. Will make the change after running it by hkhalil. Hope you
don't mind.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
net/spdy/spdy_framer_spdy2_test.cc:620: SpdyFramer framer(kVer);
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> It looks like you re-ordered the tests.  Unfortunately, this makes the diff
all
> but impossible to read.  Is it just a re-order after this point?  If so,
that's
> fine...

Hi Ryan,
  I did a diff with base and the diff is lot better (it shows what has changed).

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
File net/spdy/spdy_protocol_spdy2_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy2_test.cc:109: EXPECT_EQ((SPDY_VERSION_FOR_TESTS < 3)
? 2 : 4,
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> no reason to have the conditional here, since we know what version we're
using.

I thought if we keep it same as server's code, it would be less error prone.
What do you think?

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
File net/spdy/spdy_protocol_spdy3_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy3_test.cc:48: const int SPDY_VERSION_FOR_TESTS = 3;
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> Please rename to kSpdy3.

hkhalil: what do you think of using something like kSpdyVer instead of
SPDY_VERSION_FOR_TESTS in this unittest. thanks.

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

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:423: break;
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> I'm not sure I understand why you had to change this?

Changed it back to NOTREACHED().

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

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:191: // to negotiate a particular protocol.  This is
only called from unit tests.
On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> Actually, looks like this is called by both command line flags and unit tests.

Done.

[Ryan Hamilton, 2012-03-11 15:52:02]

lgtm

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

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:75: size_t
SpdyFramer::kUncompressedControlFrameBufferInitialSize = 18 + 16;
On 2012/03/10 19:45:29, ramant wrote:
> On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> > Hm.  Ok, I guess this explains the value, but we don't do LOAS or VTL in
> chrome.
> >  This makes me think  we should change this value.  What do you think?
> 
> I think it could get used in unittests (when in some of the tests if we
disable
> compression). The above sounds good to me.

Let's find out from hkhalil.  The way I read the comment, it looks like the 16
bytes is for a LOAS header, which we don't ever have, but hkhalil will know for
sure.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
File net/spdy/spdy_framer_spdy2_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
net/spdy/spdy_framer_spdy2_test.cc:17: const int SPDY_VERSION_FOR_TESTS = 2;
On 2012/03/10 19:45:29, ramant wrote:
> On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> > Please remove this.
> 
> I don't have objections to name changes.
> 
> Most of this code is from server. Would be really great if we can keep
> server/chrome codes as close as possible. 
>  
> They have only one file and use command line to define SPDY_VERSION_FOR_TESTS
> version 2 or 3 and run the tests.
> 
> Hi hkhalil: hope it is ok if we use kSpdy2 (instead of kVer and
> SPDY_VERSION_FOR_TESTS) in this whole file.
> 
> What do all you think? 
> 
> Will make the change after running it by hkhalil. Hope you
> don't mind.

That's fine.  This is minor, and I don't want it to stand in the way of landing
this CL.  We can always fix later.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
net/spdy/spdy_framer_spdy2_test.cc:22: unsigned char kVer =
SPDY_VERSION_FOR_TESTS;
On 2012/03/10 19:45:29, ramant wrote:
> On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> > unsigned char kSpdy2 = 2;
> > 
> > Or some such.
> 
> Sounds good to me. Will make the change after running it by hkhalil. Hope you
> don't mind.

That's fine.  This is minor, and I don't want it to stand in the way of landing
this CL.  We can always fix later.

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
File net/spdy/spdy_protocol_spdy2_test.cc (right):

http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
net/spdy/spdy_protocol_spdy2_test.cc:109: EXPECT_EQ((SPDY_VERSION_FOR_TESTS < 3)
? 2 : 4,
On 2012/03/10 19:45:29, ramant wrote:
> On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> > no reason to have the conditional here, since we know what version we're
> using.
> 
> I thought if we keep it same as server's code, it would be less error prone.
> What do you think?

Can you add a TODO to discuss this?

I think we should pursue one of two routes:
1) fork spdy_framer_test.cc into a spdy2 and a spdy3 version w/o conditions
2) use TEST_P to paramaterze spd_framer_test.cc to use spdy2/spdy2 in which case
we could keep the conditions. 

But since we don't build two distinct binaries with -DSPDY_VERSION_FOR_TESTS, I
think the server approach works well for use.

But you can land this CL and then we'll worry about this later.  Work for you?

[ramant (doing other things), 2012-03-11 20:29:55]

Thanks very much Ryan. What you have suggested sounds good to me.

Many many thanks (and sincerely appreciating) for your time over the weekend.

-raman

On 2012/03/11 15:52:02, Ryan Hamilton wrote:
> lgtm
> 
> http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer.cc
> File net/spdy/spdy_framer.cc (right):
> 
>
http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer.cc#new...
> net/spdy/spdy_framer.cc:75: size_t
> SpdyFramer::kUncompressedControlFrameBufferInitialSize = 18 + 16;
> On 2012/03/10 19:45:29, ramant wrote:
> > On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> > > Hm.  Ok, I guess this explains the value, but we don't do LOAS or VTL in
> > chrome.
> > >  This makes me think  we should change this value.  What do you think?
> > 
> > I think it could get used in unittests (when in some of the tests if we
> disable
> > compression). The above sounds good to me.
> 
> Let's find out from hkhalil.  The way I read the comment, it looks like the 16
> bytes is for a LOAS header, which we don't ever have, but hkhalil will know
for
> sure.
> 
>
http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
> File net/spdy/spdy_framer_spdy2_test.cc (right):
> 
>
http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
> net/spdy/spdy_framer_spdy2_test.cc:17: const int SPDY_VERSION_FOR_TESTS = 2;
> On 2012/03/10 19:45:29, ramant wrote:
> > On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> > > Please remove this.
> > 
> > I don't have objections to name changes.
> > 
> > Most of this code is from server. Would be really great if we can keep
> > server/chrome codes as close as possible. 
> >  
> > They have only one file and use command line to define
SPDY_VERSION_FOR_TESTS
> > version 2 or 3 and run the tests.
> > 
> > Hi hkhalil: hope it is ok if we use kSpdy2 (instead of kVer and
> > SPDY_VERSION_FOR_TESTS) in this whole file.
> > 
> > What do all you think? 
> > 
> > Will make the change after running it by hkhalil. Hope you
> > don't mind.
> 
> That's fine.  This is minor, and I don't want it to stand in the way of
landing
> this CL.  We can always fix later.
> 
>
http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_framer_spdy2_...
> net/spdy/spdy_framer_spdy2_test.cc:22: unsigned char kVer =
> SPDY_VERSION_FOR_TESTS;
> On 2012/03/10 19:45:29, ramant wrote:
> > On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> > > unsigned char kSpdy2 = 2;
> > > 
> > > Or some such.
> > 
> > Sounds good to me. Will make the change after running it by hkhalil. Hope
you
> > don't mind.
> 
> That's fine.  This is minor, and I don't want it to stand in the way of
landing
> this CL.  We can always fix later.
> 
>
http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
> File net/spdy/spdy_protocol_spdy2_test.cc (right):
> 
>
http://codereview.chromium.org/9618002/diff/31011/net/spdy/spdy_protocol_spdy...
> net/spdy/spdy_protocol_spdy2_test.cc:109: EXPECT_EQ((SPDY_VERSION_FOR_TESTS <
3)
> ? 2 : 4,
> On 2012/03/10 19:45:29, ramant wrote:
> > On 2012/03/10 03:23:02, Ryan Hamilton wrote:
> > > no reason to have the conditional here, since we know what version we're
> > using.
> > 
> > I thought if we keep it same as server's code, it would be less error prone.
> > What do you think?
> 
> Can you add a TODO to discuss this?
> 
> I think we should pursue one of two routes:
> 1) fork spdy_framer_test.cc into a spdy2 and a spdy3 version w/o conditions
> 2) use TEST_P to paramaterze spd_framer_test.cc to use spdy2/spdy2 in which
case
> we could keep the conditions. 
> 
> But since we don't build two distinct binaries with -DSPDY_VERSION_FOR_TESTS,
I
> think the server approach works well for use.
> 
> But you can land this CL and then we'll worry about this later.  Work for you?

[commit-bot: I haz the power, 2012-03-12 00:30:19]

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

[commit-bot: I haz the power, 2012-03-12 00:30:32]

Can't apply patch for file net/spdy/spdy_framer_spdy2_test.cc.
While running patch -p0 --forward --force;
patching file net/spdy/spdy_framer_spdy2_test.cc
Hunk #1 FAILED at 11.
Hunk #2 FAILED at 58.
Hunk #3 FAILED at 89.
Hunk #4 FAILED at 166.
Hunk #5 FAILED at 173.
Hunk #6 FAILED at 182.
Hunk #7 FAILED at 197.
Hunk #8 FAILED at 272.
Hunk #9 FAILED at 297.
Hunk #10 FAILED at 315.
Hunk #11 FAILED at 335.
Hunk #12 FAILED at 382.
Hunk #13 FAILED at 405.
Hunk #14 FAILED at 424.
Hunk #15 FAILED at 435.
Hunk #16 FAILED at 449.
Hunk #17 FAILED at 467.
Hunk #18 FAILED at 481.
Hunk #19 FAILED at 489.
Hunk #20 FAILED at 500.
Hunk #21 FAILED at 513.
Hunk #22 FAILED at 522.
Hunk #23 FAILED at 531.
Hunk #24 FAILED at 544.
Hunk #25 FAILED at 566.
Hunk #26 FAILED at 597.
Hunk #27 FAILED at 608.
Hunk #28 FAILED at 620.
Hunk #29 FAILED at 635.
Hunk #30 FAILED at 656.
Hunk #31 FAILED at 666.
Hunk #32 FAILED at 683.
Hunk #33 FAILED at 699.
Hunk #34 FAILED at 709.
Hunk #35 FAILED at 717.
Hunk #36 FAILED at 732.
Hunk #37 FAILED at 751.
Hunk #38 FAILED at 788.
Hunk #39 FAILED at 816.
Hunk #40 FAILED at 830.
Hunk #41 FAILED at 838.
Hunk #42 FAILED at 867.
Hunk #43 FAILED at 883.
Hunk #44 FAILED at 969.
Hunk #45 FAILED at 980.
Hunk #46 FAILED at 1009.
Hunk #47 FAILED at 1021.
Hunk #48 FAILED at 1081.
Hunk #49 FAILED at 1099.
Hunk #50 FAILED at 1110.
Hunk #51 FAILED at 1123.
Hunk #52 FAILED at 1134.
Hunk #53 FAILED at 1147.
Hunk #54 FAILED at 1158.
Hunk #55 FAILED at 1175.
Hunk #56 FAILED at 1188.
Hunk #57 FAILED at 1227.
Hunk #58 FAILED at 1238.
Hunk #59 FAILED at 1291.
Hunk #60 FAILED at 1300.
Hunk #61 FAILED at 1364.
Hunk #62 FAILED at 1375.
Hunk #63 FAILED at 1386.
Hunk #64 FAILED at 1399.
Hunk #65 FAILED at 1410.
Hunk #66 FAILED at 1423.
Hunk #67 FAILED at 1434.
Hunk #68 FAILED at 1451.
Hunk #69 FAILED at 1464.
Hunk #70 FAILED at 1502.
Hunk #71 FAILED at 1512.
Hunk #72 FAILED at 1607.
Hunk #73 FAILED at 1618.
Hunk #74 FAILED at 1635.
Hunk #75 FAILED at 1662.
Hunk #76 FAILED at 1684.
Hunk #77 FAILED at 1706.
Hunk #78 FAILED at 1716.
Hunk #79 FAILED at 1730.
Hunk #80 FAILED at 1760.
Hunk #81 FAILED at 1790.
Hunk #82 FAILED at 1811.
Hunk #83 FAILED at 1830.
Hunk #84 FAILED at 1845.
Hunk #85 FAILED at 1861.
85 out of 85 hunks FAILED -- saving rejects to file
net/spdy/spdy_framer_spdy2_test.cc.rej

[ramant (doing other things), 2012-03-12 02:26:33]

hi willchan,
  Could you bless the changes in chrome/browser/net area (rch is not in the
OWNERS in chrome/browser/net)? Would like to submit when there is less activity.

thanks very much,
raman

[hkhalil, 2012-03-12 13:40:54]

lgtm

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

http://codereview.chromium.org/9618002/diff/24076/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:73: // In addition to the maximum control frame header
size, we account for any
You should be able to go ahead and remove this 16B addition and comment.

[ramant (doing other things), 2012-03-12 17:14:29]

Thanks much hkhalil. Fixed the comments.

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

http://codereview.chromium.org/9618002/diff/24076/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:73: // In addition to the maximum control frame header
size, we account for any
On 2012/03/12 13:40:54, hkhalil wrote:
> You should be able to go ahead and remove this 16B addition and comment.

Done.

[willchan no longer on Chromium, 2012-03-12 18:19:58]

The SetSpdySetting vs SetSpdySettings naming was confusing. LGTM though. I'd
advise updating the comment at least to make it more clear that it's an
individual setting.

[ramant (doing other things), 2012-03-12 18:28:52]

Updated the comments.

Many many thanks Ryan, Hasan and William for great team work,
raman

On 2012/03/12 18:19:58, willchan wrote:
> The SetSpdySetting vs SetSpdySettings naming was confusing. LGTM though. I'd
> advise updating the comment at least to make it more clear that it's an
> individual setting.

[rch (use chromium not google), 2012-03-12 18:33:14]

Thanks for all your hard work getting this CL together!

On Mon, Mar 12, 2012 at 11:28 AM, <rtenneti@chromium.org> wrote:

> Updated the comments.
>
> Many many thanks Ryan, Hasan and William for great team work,
> raman

[hkhalil, 2012-03-12 19:14:24]

On Mon, Mar 12, 2012 at 2:33 PM, Ryan Hamilton <rch@google.com> wrote:

> Thanks for all your hard work getting this CL together!


+1

    -Hasan