SPDY - persist SPDY settings.

SPDY - persist SPDY settings.

HttpServerProperties stores SPDY settings in SettingsMap
whose key is settings ID and value is a (flags, value)
pair (SettingsFlagsAndValue).

The format of the persisted data is same as before (restored
the old code that persisted the data). Verified it with an older
version of the Preferences file that is very large. Verified
that we have sent the right data  (sorted and unique settings)
to the server and that duplicates were removed from the file.

Changed all the code above SpdyFramer to use the above
spdy::SettingsMap and spdy::SettingsFlagsAndValue
data structures.

When chrome sends Spdy Settings to the server, it uses
the current SpdyFramer's API calls to create Settings
frame.

TODO: 
1) Delete spdy::SpdySettings and spdy::SpdySetting
   and replace them with SettingsMap, SpdySettingsIds, 
   SpdySettingsFlags and value.
2) Add version to the persisted data.


BUG=117760
R=rch, willchan
TEST=network unit tests and browser unit tests.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128737

[ramant (doing other things), 2012-03-21 05:09:03]

Hi Ryan,
  Would appreciate if you could take a high level look at the changes to persist
the spdy settings (persisting code in http_server_properties_manager.cc is same
as before except we iterate over a std::map instead of a std::list).

  Will make a separate CL to change SpdyFramer and other code to the server team
first and then will merge those changed back into chrome.

  Would appreciate your comments.

thanks very much,
raman

[Ryan Hamilton, 2012-03-21 16:24:41]

https://chromiumcodereview.appspot.com/9802003/diff/3009/chrome/browser/net/h...
File chrome/browser/net/http_server_properties_manager.cc (right):

https://chromiumcodereview.appspot.com/9802003/diff/3009/chrome/browser/net/h...
chrome/browser/net/http_server_properties_manager.cc:280: if
((*list_it)->GetType() != Value::TYPE_DICTIONARY) {
Do I understand correctly that we're persisting this as a list of dicts?  How
come?

https://chromiumcodereview.appspot.com/9802003/diff/3009/chrome/browser/net/h...
chrome/browser/net/http_server_properties_manager.cc:569: base::ListValue*
spdy_settings_list = new ListValue();
Do you need to worry about reading old values?  Did we persist settings
differently in previous versions?

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/http/http_server...
File net/http/http_server_properties.h (right):

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/http/http_server...
net/http/http_server_properties.h:97: const spdy::SettingsFlagsAndValue&
flags_and_value) = 0;
Please expand this out to take flags and value independently.  Currently, each
call site needs to construct this flags_and_value pair simply to make this call.

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/http/http_server...
File net/http/http_server_properties_impl.cc (right):

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/http/http_server...
net/http/http_server_properties_impl.cc:226:
spdy_settings_map_.find(host_port_pair);
can this all fit on one line?  (Looks like it did in the old version)

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/http/http_server...
net/http/http_server_properties_impl.cc:244: settings_map[id] =
new_flags_and_value;
So much cleaner!  Obviously correct, too :>

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/http/http_server...
File net/http/http_server_properties_impl_unittest.cc (left):

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/http/http_server...
net/http/http_server_properties_impl_unittest.cc:307:
TEST_F(SpdySettingsServerPropertiesTest, SetSpdySettings) {
What happened to this test?  Is it now redundant?

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_framer.h
File net/spdy/spdy_framer.h (right):

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_framer...
net/spdy/spdy_framer.h:78: typedef std::list<SpdySetting> SpdySettings;
Does this type still need to exist?  If so, what is it used for?

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_sessio...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_sessio...
net/spdy/spdy_session.cc:1365: host_port_pair(), id, flags_and_value);
This API seems a bit suboptimal.  instead of taking flags_and_value which
required to caller to construct them, could we change SetSpdySetting to tkae all
three?

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_sessio...
net/spdy/spdy_session.cc:1370: net_log_.AddEvent(
Hm.  This means that we'll end up with 1 log entry for each setting in a
settings frame?  That seems a bit annoying, though not the end of the world.  It
does seem like live would be easier if buffered spdy framer buffered up the
whole settings frame.  perhaps in another cl.

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_sessio...
net/spdy/spdy_session.cc:1706: // Note: we can potentially modify the settings
for the field trial.
I think I would remove this comment.  It doesn't seem to add much value, since
the field trial code is 4 lines below.

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_sessio...
net/spdy/spdy_session.cc:1713: for (spdy::SettingsMap::const_iterator i =
settings_map.begin(),
Since this is a map, I don't think you need to loop over all the elements. 
Instead, you could simply look to see if the map has an entry for
spdy::SETTINGS_CURRENT_CWND.  If it does, then check to see if the value is the
expected value.  If not, set it to the new value and re-persist.

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_sessio...
net/spdy/spdy_session.cc:1715: const uint32 id = i->first;
nit: should the type of id be spdy::SpdySettingsIds?

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_sessio...
net/spdy/spdy_session.cc:1736: spdy::SpdySettings settings;
Oh, here we're using SpdySettings as a list.  How come?

https://chromiumcodereview.appspot.com/9802003/diff/3009/net/spdy/spdy_sessio...
net/spdy/spdy_session.cc:1918: const uint32 val = it->second.second;
Nice cleanup.  Much more readable.

nit: should id's type be spdy::SpdySettingsIds instead?

[ramant (doing other things), 2012-03-23 04:11:25]

Hi Ryan,
  Integrated your latest namespace changes. Would appreciate if you could take
another look at it.

thanks much,
raman

http://codereview.chromium.org/9802003/diff/3009/chrome/browser/net/http_serv...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/9802003/diff/3009/chrome/browser/net/http_serv...
chrome/browser/net/http_server_properties_manager.cc:280: if
((*list_it)->GetType() != Value::TYPE_DICTIONARY) {
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> Do I understand correctly that we're persisting this as a list of dicts?  How
> come?

Correct. 

Didn't change the format in this CL. The old version persisted Settings as a
List (server used to send duplicates). 

The following is the discussion willchan and I had earlier (remember 
http://codereview.chromium.org/8423028/#msg15

Will add a TODO to add version and change the format of the persisted settings
(remove the extra List).

http://codereview.chromium.org/9802003/diff/3009/chrome/browser/net/http_serv...
chrome/browser/net/http_server_properties_manager.cc:569: base::ListValue*
spdy_settings_list = new ListValue();
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> Do you need to worry about reading old values?  Did we persist settings
> differently in previous versions?

The format for spdy_settings is same as before. Format hasn't changed.

http://codereview.chromium.org/9802003/diff/3009/net/http/http_server_propert...
File net/http/http_server_properties.h (right):

http://codereview.chromium.org/9802003/diff/3009/net/http/http_server_propert...
net/http/http_server_properties.h:97: const spdy::SettingsFlagsAndValue&
flags_and_value) = 0;
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> Please expand this out to take flags and value independently.  Currently, each
> call site needs to construct this flags_and_value pair simply to make this
call.

Done.

http://codereview.chromium.org/9802003/diff/3009/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/9802003/diff/3009/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:226:
spdy_settings_map_.find(host_port_pair);
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> can this all fit on one line?  (Looks like it did in the old version)

Done.

http://codereview.chromium.org/9802003/diff/3009/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:244: settings_map[id] =
new_flags_and_value;
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> So much cleaner!  Obviously correct, too :>

Thanks.

http://codereview.chromium.org/9802003/diff/3009/net/http/http_server_propert...
File net/http/http_server_properties_impl_unittest.cc (left):

http://codereview.chromium.org/9802003/diff/3009/net/http/http_server_propert...
net/http/http_server_properties_impl_unittest.cc:307:
TEST_F(SpdySettingsServerPropertiesTest, SetSpdySettings) {
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> What happened to this test?  Is it now redundant?

It is redundant. tx.

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

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_framer.h#newco...
net/spdy/spdy_framer.h:78: typedef std::list<SpdySetting> SpdySettings;
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> Does this type still need to exist?  If so, what is it used for?

Would like to submit a separate CL to server/chrome to delete SpdySettings (need
to change SpdyFramer.cc, SpdyFramerTest.cc etc).

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

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1365: host_port_pair(), id, flags_and_value);
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> This API seems a bit suboptimal.  instead of taking flags_and_value which
> required to caller to construct them, could we change SetSpdySetting to tkae
all
> three?

Done.

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1370: net_log_.AddEvent(
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> Hm.  This means that we'll end up with 1 log entry for each setting in a
> settings frame?  That seems a bit annoying, though not the end of the world. 
It
> does seem like live would be easier if buffered spdy framer buffered up the
> whole settings frame.  perhaps in another cl.

+1. Will create a bug for buffering.

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1706: // Note: we can potentially modify the settings
for the field trial.
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> I think I would remove this comment.  It doesn't seem to add much value, since
> the field trial code is 4 lines below.

Done.

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1713: for (spdy::SettingsMap::const_iterator i =
settings_map.begin(),
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> Since this is a map, I don't think you need to loop over all the elements. 
> Instead, you could simply look to see if the map has an entry for
> spdy::SETTINGS_CURRENT_CWND.  If it does, then check to see if the value is
the
> expected value.  If not, set it to the new value and re-persist.

Thanks for the above.

Done.

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1715: const uint32 id = i->first;
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> nit: should the type of id be spdy::SpdySettingsIds?

Done.

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1736: spdy::SpdySettings settings;
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> Oh, here we're using SpdySettings as a list.  How come?

Would like to a submit a separate CL to delete all uses of spdy::SpdySettings
from our code and the common code with server. 

SpdyFramer's CreateSettings takes SpdySettings. Will file a bug (TODO) to delete
spdy::SpdySettings and use spdy::SettingsMap everywhere.

Hope that is ok.

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1918: const uint32 val = it->second.second;
On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> Nice cleanup.  Much more readable.
> 
> nit: should id's type be spdy::SpdySettingsIds instead?

Done.

[Ryan Hamilton, 2012-03-23 15:25:38]

lgtm

http://codereview.chromium.org/9802003/diff/3009/chrome/browser/net/http_serv...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/9802003/diff/3009/chrome/browser/net/http_serv...
chrome/browser/net/http_server_properties_manager.cc:280: if
((*list_it)->GetType() != Value::TYPE_DICTIONARY) {
On 2012/03/23 04:11:25, ramant wrote:
> On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> > Do I understand correctly that we're persisting this as a list of dicts? 
How
> > come?
> 
> Correct. 
> 
> Didn't change the format in this CL. The old version persisted Settings as a
> List (server used to send duplicates). 
> 
> The following is the discussion willchan and I had earlier (remember 
> http://codereview.chromium.org/8423028/#msg15
> 
> Will add a TODO to add version and change the format of the persisted settings
> (remove the extra List).

Sounds good.

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

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_framer.h#newco...
net/spdy/spdy_framer.h:78: typedef std::list<SpdySetting> SpdySettings;
On 2012/03/23 04:11:25, ramant wrote:
> On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> > Does this type still need to exist?  If so, what is it used for?
> 
> Would like to submit a separate CL to server/chrome to delete SpdySettings
(need
> to change SpdyFramer.cc, SpdyFramerTest.cc etc).

*nod*

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

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1370: net_log_.AddEvent(
On 2012/03/23 04:11:25, ramant wrote:
> On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> > Hm.  This means that we'll end up with 1 log entry for each setting in a
> > settings frame?  That seems a bit annoying, though not the end of the world.

> It
> > does seem like live would be easier if buffered spdy framer buffered up the
> > whole settings frame.  perhaps in another cl.
> 
> +1. Will create a bug for buffering.

Thanks.

http://codereview.chromium.org/9802003/diff/3009/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:1736: spdy::SpdySettings settings;
On 2012/03/23 04:11:25, ramant wrote:
> On 2012/03/21 16:24:41, Ryan Hamilton wrote:
> > Oh, here we're using SpdySettings as a list.  How come?
> 
> Would like to a submit a separate CL to delete all uses of spdy::SpdySettings
> from our code and the common code with server. 
> 
> SpdyFramer's CreateSettings takes SpdySettings. Will file a bug (TODO) to
delete
> spdy::SpdySettings and use spdy::SettingsMap everywhere.
> 
> Hope that is ok.

Sounds good.

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

http://codereview.chromium.org/9802003/diff/20093/net/http/http_server_proper...
net/http/http_server_properties.h:102: // Returns all SPDY settings.
I notice that you removed "persisted" from the comment.  Does
HttpServerProperties now return non-persisted SPDY settings?

[ramant (doing other things), 2012-03-23 16:23:47]

Fixed the comment and created bugs for the TODO.

Many many thanks Ryan for the code review (sincerely appreciating it).

mmenke/cbentzel: can I get a rubber stamp for chrome/browser/net changes? (I
think willchan is travelling).

thanks very much,
raman

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

http://codereview.chromium.org/9802003/diff/20093/net/http/http_server_proper...
net/http/http_server_properties.h:102: // Returns all SPDY settings.
On 2012/03/23 15:25:38, Ryan Hamilton wrote:
> I notice that you removed "persisted" from the comment.  Does
> HttpServerProperties now return non-persisted SPDY settings?

Thanks for catching this. Fixed it.
Done.

[commit-bot: I haz the power, 2012-03-23 16:48:17]

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

[commit-bot: I haz the power, 2012-03-23 17:26:12]

Try job failure for 9802003-20096 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-03-24 11:38:52]

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

[commit-bot: I haz the power, 2012-03-24 14:12:50]

Change committed as 128737