SPDY - don't persist SpdySettings until we convert it to a map.

SPDY - don't persist SpdySettings until we convert it to a map.

- Deleted the code that read and wrote the SpdySettings data
  from preferences.
- Disabled the code that persisted the SpdySettings data.
- Disabled the unit tests that tested persisting of SpdySettings.

BUG=118148
R=willchan
TEST=browser unit tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126838

[rtenneti, 2012-03-15 01:36:22]

Hi Jim and willchan,
  Commented out (or deleted) the code that read/saved and that set the
SpdySettings.

thanks,
raman

[jar (doing other things), 2012-03-15 01:46:48]

The code LGTM.... I'd like to get Will's agreement.

[ramant (doing other things), 2012-03-15 02:16:27]

On 2012/03/15 01:46:48, jar wrote:
> The code LGTM.... I'd like to get Will's agreement.

Will submit a separate CL that persists the SpdySettings and saves the
SpdySettings as a dictionary (not as a list). Prior to SPDY/3, SpdySettings
could have duplicate entries (in SPDY/3 they will be a unique and sorted).

[willchan no longer on Chromium, 2012-03-15 03:13:53]

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

http://codereview.chromium.org/9703050/diff/1/net/spdy/spdy_session.cc#newcod...
net/spdy/spdy_session.cc:1303: // TODO(rtenneti): persist SpdySetting.
Wait, why do we have to change this? We can still set the SpdySetting, right? It
just can't be persisted?

http://codereview.chromium.org/9703050/diff/1/net/spdy/spdy_session.cc#newcod...
net/spdy/spdy_session.cc:1677: // TODO(rtenneti): Persist SpdySetting.
Do we need to change this too?

[ramant (doing other things), 2012-03-15 03:46:29]

Hi willchan,
  Was being conservative with this CL. Will implement the right fix tomorrow.

thanks,
raman

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

http://codereview.chromium.org/9703050/diff/1/net/spdy/spdy_session.cc#newcod...
net/spdy/spdy_session.cc:1303: // TODO(rtenneti): persist SpdySetting.
On 2012/03/15 03:13:54, willchan wrote:
> Wait, why do we have to change this? We can still set the SpdySetting, right?
It
> just can't be persisted?

SetSpdySetting is doing the copy of memory. It is possible over a period of time
(for plus.google.com) we get lot of settings update and we keep appending and
thus the list can grow. Will check in a fix that doesn't grow the memory
tomorrow. Hope this approach is ok.

http://codereview.chromium.org/9703050/diff/1/net/spdy/spdy_session.cc#newcod...
net/spdy/spdy_session.cc:1677: // TODO(rtenneti): Persist SpdySetting.
On 2012/03/15 03:13:54, willchan wrote:
> Do we need to change this too?

This is where the bug was. In the previous version, we used to break and we used
to set only one setting update. For sorting purposes, I used the same loop and
it had the side effect of calling SetSpdySetting for every element (and most of
the elements were CWD setting).

None of this code will get executed because GetSpdySettings will be empty.
Wanted to be 100% safe. Will implement persisting (and saving of the settings
tonight/tomorrow).

What do you think of this approach (being very conservative).

[willchan no longer on Chromium, 2012-03-15 04:12:26]

I'm a bit surprised, this seems overly conservative, but it's not worth
debating. LGTM.

On Wed, Mar 14, 2012 at 8:46 PM, <rtenneti@chromium.org> wrote:

> Hi willchan,
>  Was being conservative with this CL. Will implement the right fix
> tomorrow.
>
> thanks,
> raman
>
>
>
>
http://codereview.chromium.**org/9703050/diff/1/net/spdy/**spdy_session.cc<ht...
> File net/spdy/spdy_session.cc (right):
>
> http://codereview.chromium.**org/9703050/diff/1/net/spdy/**
>
spdy_session.cc#newcode1303<http://codereview.chromium.org/9703050/diff/1/net/spdy/spdy_session.cc#newcode1303>
> net/spdy/spdy_session.cc:1303: // TODO(rtenneti): persist SpdySetting.
> On 2012/03/15 03:13:54, willchan wrote:
>
>> Wait, why do we have to change this? We can still set the SpdySetting,
>>
> right? It
>
>> just can't be persisted?
>>
>
> SetSpdySetting is doing the copy of memory. It is possible over a period
> of time (for plus.google.com) we get lot of settings update and we keep
> appending and thus the list can grow. Will check in a fix that doesn't
> grow the memory tomorrow. Hope this approach is ok.
>
>
> http://codereview.chromium.**org/9703050/diff/1/net/spdy/**
>
spdy_session.cc#newcode1677<http://codereview.chromium.org/9703050/diff/1/net/spdy/spdy_session.cc#newcode1677>
> net/spdy/spdy_session.cc:1677: // TODO(rtenneti): Persist SpdySetting.
> On 2012/03/15 03:13:54, willchan wrote:
>
>> Do we need to change this too?
>>
>
> This is where the bug was. In the previous version, we used to break and
> we used to set only one setting update. For sorting purposes, I used the
> same loop and it had the side effect of calling SetSpdySetting for every
> element (and most of the elements were CWD setting).
>
> None of this code will get executed because GetSpdySettings will be
> empty. Wanted to be 100% safe. Will implement persisting (and saving of
> the settings tonight/tomorrow).
>
> What do you think of this approach (being very conservative).
>
>
http://codereview.chromium.**org/9703050/<http://codereview.chromium.org/9703...
>