SPDY - replaced SpdySettings (list) with SettingsMap

SPDY - replaced SpdySettings (list) with SettingsMap

CreateSettings accepts SettingsMap which enforces
that there are no duplicate settings.

Deleted unused class SpdySettingsStorage.

In net-internals, log setting's id, flags and value for
SpdySettings.

BUG=119752
R=rch@chromium.org,hkhalil@google.com
TEST=network unit tests

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

[ramant (doing other things), 2012-04-12 04:15:36]

Hi Ryan and Hasan,
  Took a first cut at replacing SpdySettings with SettingsMap.   Hasan if these
changes look ok, will submit a separate CL for the server.

thanks,
raman

[Ryan Hamilton, 2012-04-12 17:11:21]

nice!

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (left):

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_framer.cc#ol...
net/spdy/spdy_framer.cc:1016: DCHECK(settings);
Did you mean to remove this DCHECK?

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

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_framer_test....
net/spdy/spdy_framer_test.cc:2404: EXPECT_EQ(0, visitor.error_count_);
Why did the error_count change here?

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_protocol_tes...
File net/spdy/spdy_protocol_test.cc (right):

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_protocol_tes...
net/spdy/spdy_protocol_test.cc:252: };
How come the spdy2 and spdy3 settings are separated into two different chunks
here?

[ramant (doing other things), 2012-04-12 18:35:59]

Thanks very much Ryan for your comments. Would appreciate if you could take
another look at these changes.

-raman

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (left):

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_framer.cc#ol...
net/spdy/spdy_framer.cc:1016: DCHECK(settings);
On 2012/04/12 17:11:21, Ryan Hamilton wrote:
> Did you mean to remove this DCHECK?

(my fault. deleted it by mistake)
Done.

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

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_framer_test....
net/spdy/spdy_framer_test.cc:2404: EXPECT_EQ(0, visitor.error_count_);
On 2012/04/12 17:11:21, Ryan Hamilton wrote:
> Why did the error_count change here?

In the older version (in the list), if we had supplied out of order settings, it
gave an error.

It is hard to create out of order settings with map and this test may be
invalid. What do you think?

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_protocol_tes...
File net/spdy/spdy_protocol_test.cc (right):

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_protocol_tes...
net/spdy/spdy_protocol_test.cc:252: };
On 2012/04/12 17:11:21, Ryan Hamilton wrote:
> How come the spdy2 and spdy3 settings are separated into two different chunks
> here?

In the map, settings are stored in sorted order based on id. The id's are
different from spdy2/spdy3 (because FromWireFormat is different between spdy/2
and spdy/3)

For example: for "0x01000002" is spdy/2, id is 2 and flags is 1 and in spdy/3 id
is 1 and flags is 2.

Implementing two different arrays, made it easier to keep hasan's exiting test
code/cases. 

Will talk to him if he wants to rewrite this test.

[Ryan Hamilton, 2012-04-17 02:32:49]

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

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_framer_test....
net/spdy/spdy_framer_test.cc:2404: EXPECT_EQ(0, visitor.error_count_);
On 2012/04/12 18:35:59, ramant wrote:
> On 2012/04/12 17:11:21, Ryan Hamilton wrote:
> > Why did the error_count change here?
> 
> In the older version (in the list), if we had supplied out of order settings,
it
> gave an error.
> 
> It is hard to create out of order settings with map and this test may be
> invalid. What do you think?

Sounds like it's invalid now.  Nuke it?

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_protocol_tes...
File net/spdy/spdy_protocol_test.cc (right):

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_protocol_tes...
net/spdy/spdy_protocol_test.cc:252: };
On 2012/04/12 18:35:59, ramant wrote:
> On 2012/04/12 17:11:21, Ryan Hamilton wrote:
> > How come the spdy2 and spdy3 settings are separated into two different
chunks
> > here?
> 
> In the map, settings are stored in sorted order based on id. The id's are
> different from spdy2/spdy3 (because FromWireFormat is different between spdy/2
> and spdy/3)
> 
> For example: for "0x01000002" is spdy/2, id is 2 and flags is 1 and in spdy/3
id
> is 1 and flags is 2.

Perhaps I'm missing something, but it looks like both arrays are constructed
with the same SettingsFlagsAndId::FromWireFormat() invocations, albeit in a
different order.  Is the order significant?

> Implementing two different arrays, made it easier to keep hasan's exiting test
> code/cases. 
> 
> Will talk to him if he wants to rewrite this test.

*nod*  I'm content to defer to whatever you guys agree to.

[ramant (doing other things), 2012-04-17 18:48:19]

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

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_framer_test....
net/spdy/spdy_framer_test.cc:2404: EXPECT_EQ(0, visitor.error_count_);
On 2012/04/17 02:32:49, Ryan Hamilton wrote:
> On 2012/04/12 18:35:59, ramant wrote:
> > On 2012/04/12 17:11:21, Ryan Hamilton wrote:
> > > Why did the error_count change here?
> > 
> > In the older version (in the list), if we had supplied out of order
settings,
> it
> > gave an error.
> > 
> > It is hard to create out of order settings with map and this test may be
> > invalid. What do you think?
> 
> Sounds like it's invalid now.  Nuke it?

Done.

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_protocol_tes...
File net/spdy/spdy_protocol_test.cc (right):

http://codereview.chromium.org/10054034/diff/11005/net/spdy/spdy_protocol_tes...
net/spdy/spdy_protocol_test.cc:252: };
On 2012/04/17 02:32:49, Ryan Hamilton wrote:
> On 2012/04/12 18:35:59, ramant wrote:
> > On 2012/04/12 17:11:21, Ryan Hamilton wrote:
> > > How come the spdy2 and spdy3 settings are separated into two different
> chunks
> > > here?
> > 
> > In the map, settings are stored in sorted order based on id. The id's are
> > different from spdy2/spdy3 (because FromWireFormat is different between
spdy/2
> > and spdy/3)
> > 
> > For example: for "0x01000002" is spdy/2, id is 2 and flags is 1 and in
spdy/3
> id
> > is 1 and flags is 2.
> 
> Perhaps I'm missing something, but it looks like both arrays are constructed
> with the same SettingsFlagsAndId::FromWireFormat() invocations, albeit in a
> different order.  Is the order significant?
> 
> > Implementing two different arrays, made it easier to keep hasan's exiting
test
> > code/cases. 
> > 
> > Will talk to him if he wants to rewrite this test.
> 
> *nod*  I'm content to defer to whatever you guys agree to.

(sorry if already known)

We get data out of "ids" array and store it in a Map(where it is stored in
sorted by "id").

CreateSettings used to take a list and now it is taking a Map. Because it is
taking a Map, the settings are created in a sorted order (by "id").

Then we call ParseSettings, which returns the settings  in a Map (sorted by
"id"). It would be in the same order as "settings" Map that is passed to
CreateSettings.

Prior to this change, ParseSettings, used to return the settings in a list and
thus it would return in the same order as they were created.  

Then we compare the data returned by ParsedSettings with what is in the "ids".

So we compare the returned data in the following order by id.

id:
0
0,1
0,1,9
0,1,9,0xff
0,1,9,0xff,0x00ffffff

Because of the above, stored the data in the "ids" array sorted by "id".

As we had talked, changed the code to compare the std::maps.

Deleted duplicate entries in "ids".

[Ryan Hamilton, 2012-04-17 22:05:47]

LGTM...  sorry for the delay

[commit-bot: I haz the power, 2012-04-17 22:07:15]

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

[commit-bot: I haz the power, 2012-04-18 02:14:45]

Change committed as 132722