I added a function "void SpdySettingsStorage::Clear()". I added a call to this function "spdy_set...

Added a function "void SpdySettingsStorage::Clear()". Added a call to the
function "spdy_settings_.Clear()" to the function "void SpdySessionPool::OnIPAddressChanged()".

BUG=55868
TEST="./net_unittests --gtest_filter=SpdySessionTest.ClearSettingsStorage" and "./net_unittests --gtest_filter=SpdySessionTest.ClearOnIPAddressChanged".

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

[wtc, 2011-05-19 22:54:37]

willchan: is there an easy way to test OnIPAddressChanged()
functions?

http://codereview.chromium.org/7046016/diff/2002/net/spdy/spdy_settings_stora...
File net/spdy/spdy_settings_storage.h (right):

http://codereview.chromium.org/7046016/diff/2002/net/spdy/spdy_settings_stora...
net/spdy/spdy_settings_storage.h:27: 
Nit: don't add this blank line.

willchan: since Get() returns a (const) reference to a value
in settings_map_, a caller may store the reference, which
will become invalid after a Clear() call.

We can eliminating this risk entirely by changing Get() to
return a copy of the value, but since the value (spdy::SpdySettings)
is an std::list, it seems expensive to always copy it.
So rkn and I decided to add this comment to warn the callers.
rkn verified that no existing callers store the returned
reference.

http://codereview.chromium.org/7046016/diff/2002/net/spdy/spdy_settings_stora...
net/spdy/spdy_settings_storage.h:34: // Flush structure
Nit: this comment should say a little more.

[willchan no longer on Chromium, 2011-05-19 22:59:43]

The code is right. I defer to wtc wrt nits. Comments should be better and the
changelist description is a bit awkward (no need to say "I added", just say what
the change does).

http://codereview.chromium.org/7046016/diff/2002/net/spdy/spdy_settings_stora...
File net/spdy/spdy_settings_storage.h (right):

http://codereview.chromium.org/7046016/diff/2002/net/spdy/spdy_settings_stora...
net/spdy/spdy_settings_storage.h:27: 
On 2011/05/19 22:54:37, wtc wrote:
> Nit: don't add this blank line.
> 
> willchan: since Get() returns a (const) reference to a value
> in settings_map_, a caller may store the reference, which
> will become invalid after a Clear() call.
> 
> We can eliminating this risk entirely by changing Get() to
> return a copy of the value, but since the value (spdy::SpdySettings)
> is an std::list, it seems expensive to always copy it.
> So rkn and I decided to add this comment to warn the callers.
> rkn verified that no existing callers store the returned
> reference.

Adding a comment is indeed the right solution here.

[rkn1, 2011-05-20 16:57:37]

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_session_pool.c...
net/spdy/spdy_session_pool.cc:181: }
willchan: Is there a good way to test this function?

Also, you mentioned unifying everything that needs to be flushed in a single
struct, can you elaborate on what would need to be done?

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_settings_stora...
File net/spdy/spdy_settings_storage.h (right):

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_settings_stora...
net/spdy/spdy_settings_storage.h:27: const spdy::SpdySettings& Get(const
HostPortPair& host_port_pair) const;
I'm not sure what the function "SpdySession::SendSettings()" is doing. It seems
like the return values from Get might be stored, but I am not sure. Can someone
take a look at that? Other than that, I don't think any values are being stored.

[willchan no longer on Chromium, 2011-05-20 21:29:46]

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_session_pool.c...
net/spdy/spdy_session_pool.cc:181: }
On 2011/05/20 16:57:37, rkn1 wrote:
> willchan: Is there a good way to test this function?
> 
> Also, you mentioned unifying everything that needs to be flushed in a single
> struct, can you elaborate on what would need to be done?

OnIPAddressChanged() is a public method so you can call it from tests easily.
You can get a non-const SpdySettingsStorage via
SpdySessionPool::mutable_spdy_settings(). Modify the SpdySettingsStorage, then
call SpdySessionPool::OnIPAddressChanged() and then verify that it's empty.

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_settings_stora...
File net/spdy/spdy_settings_storage.h (right):

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_settings_stora...
net/spdy/spdy_settings_storage.h:27: const spdy::SpdySettings& Get(const
HostPortPair& host_port_pair) const;
On 2011/05/20 16:57:37, rkn1 wrote:
> I'm not sure what the function "SpdySession::SendSettings()" is doing. It
seems
> like the return values from Get might be stored, but I am not sure. Can
someone
> take a look at that? Other than that, I don't think any values are being
stored.

It looks like a minor bug. It's copying rather than using a const reference.
It's not storing the settings though. It'd copying the settings to send to the
server. It doesn't hold onto a reference long-term, so don't worry.

[rkn1, 2011-05-20 22:01:36]

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:575: // in order to create a SpdySessionPool
object.
The above code in this test was copied from a test above. Some of it is probably
unnecessary. However, the two added tests pass.

[wtc, 2011-05-20 23:04:28]

rkn: I suggest some changes to the new unit test.

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_settings_stora...
File net/spdy/spdy_settings_storage.h (right):

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_settings_stora...
net/spdy/spdy_settings_storage.h:27: const spdy::SpdySettings& Get(const
HostPortPair& host_port_pair) const;
On 2011/05/20 21:29:47, willchan wrote:
>
> It looks like a minor bug. It's copying rather than using a const reference.

There is a comment that says the copying is intentional:

  // Note:  we're copying the settings here, so that we can potentially modify
  // the settings for the field trial.  When removing the field trial, make
  // this a reference to the const SpdySettings again.

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:547: TEST_F(SpdySessionTest,
ClearOnIPAddressChanged) {
This test should be named ClearSettingsStorageOnIPAddressChanged.

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:575: // in order to create a SpdySessionPool
object.
This comment is not necessary.  (It's also wrong; I think
you copied the code from TEST_F(SpdySessionTest, GoAway).)

I hope we can delete all the code that manipulates the
data members of session_deps (lines 549-563) because they
probably won't be used by this unit test.  Can you give that
a try?

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:578:
spdy_session_pool->mutable_spdy_settings();
This continuation line should be indented by 4 spaces.

[rkn1, 2011-05-21 01:05:48]

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:547: TEST_F(SpdySessionTest,
ClearOnIPAddressChanged) {
On 2011/05/20 23:04:28, wtc wrote:
> This test should be named ClearSettingsStorageOnIPAddressChanged.

Done.

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:575: // in order to create a SpdySessionPool
object.
On 2011/05/20 23:04:28, wtc wrote:
> This comment is not necessary.  (It's also wrong; I think
> you copied the code from TEST_F(SpdySessionTest, GoAway).)
> 
> I hope we can delete all the code that manipulates the
> data members of session_deps (lines 549-563) because they
> probably won't be used by this unit test.  Can you give that
> a try?
>       
>   
> 

Done.

http://codereview.chromium.org/7046016/diff/13002/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:578:
spdy_session_pool->mutable_spdy_settings();
On 2011/05/20 23:04:28, wtc wrote:
> This continuation line should be indented by 4 spaces.

Done.

[wtc, 2011-05-21 01:38:32]

Thank you for making changes quickly.  I suggest some
more changes, hopefully final.

http://codereview.chromium.org/7046016/diff/8003/net/spdy/spdy_session_unitte...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7046016/diff/8003/net/spdy/spdy_session_unitte...
net/spdy/spdy_session_unittest.cc:531: SpdySettingsStorage new_settings_storage;
Nit: please remove "new_" from the variable name.

http://codereview.chromium.org/7046016/diff/8003/net/spdy/spdy_session_unitte...
net/spdy/spdy_session_unittest.cc:537:
id.set_id(spdy::SETTINGS_FLAG_PLEASE_PERSIST);
This comment also applies to the next unit test.

I believe we also need to specify which setting this is.
See lines 279-281.

So we should do:
  spdy::SettingsFlagsAndId id(spdy::SETTINGS_MAX_CONCURRENT_STREAMS);
  id.set_id(spdy::SETTINGS_MAX_CONCURRENT_STREAMS);
  id.set_flags(spdy::SETTINGS_FLAG_PLEASE_PERSIST);

Note: I don't understand why we pass
spdy::SETTINGS_MAX_CONCURRENT_STREAMS twice.

http://codereview.chromium.org/7046016/diff/8003/net/spdy/spdy_session_unitte...
net/spdy/spdy_session_unittest.cc:551: HostPortProxyPair
pair(test_host_port_pair, ProxyServer::Direct());
Delete this line.  The 'pair' variable is not used.

[rkn1, 2011-05-21 01:54:39]

http://codereview.chromium.org/7046016/diff/8003/net/spdy/spdy_session_unitte...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7046016/diff/8003/net/spdy/spdy_session_unitte...
net/spdy/spdy_session_unittest.cc:531: SpdySettingsStorage new_settings_storage;
On 2011/05/21 01:38:33, wtc wrote:
> Nit: please remove "new_" from the variable name.

Done.

http://codereview.chromium.org/7046016/diff/8003/net/spdy/spdy_session_unitte...
net/spdy/spdy_session_unittest.cc:537:
id.set_id(spdy::SETTINGS_FLAG_PLEASE_PERSIST);
On 2011/05/21 01:38:33, wtc wrote:
> This comment also applies to the next unit test.
> 
> I believe we also need to specify which setting this is.
> See lines 279-281.
> 
> So we should do:
>   spdy::SettingsFlagsAndId id(spdy::SETTINGS_MAX_CONCURRENT_STREAMS);
>   id.set_id(spdy::SETTINGS_MAX_CONCURRENT_STREAMS);
>   id.set_flags(spdy::SETTINGS_FLAG_PLEASE_PERSIST);
> 
> Note: I don't understand why we pass
> spdy::SETTINGS_MAX_CONCURRENT_STREAMS twice.
>   

Done.

http://codereview.chromium.org/7046016/diff/8003/net/spdy/spdy_session_unitte...
net/spdy/spdy_session_unittest.cc:551: HostPortProxyPair
pair(test_host_port_pair, ProxyServer::Direct());
On 2011/05/21 01:38:33, wtc wrote:
> Delete this line.  The 'pair' variable is not used.

Done.

[willchan no longer on Chromium, 2011-05-21 08:45:28]

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_settings_stora...
File net/spdy/spdy_settings_storage.h (right):

http://codereview.chromium.org/7046016/diff/5003/net/spdy/spdy_settings_stora...
net/spdy/spdy_settings_storage.h:27: const spdy::SpdySettings& Get(const
HostPortPair& host_port_pair) const;
On 2011/05/20 23:04:28, wtc wrote:
> On 2011/05/20 21:29:47, willchan wrote:
> >
> > It looks like a minor bug. It's copying rather than using a const reference.
> 
> There is a comment that says the copying is intentional:
> 
>   // Note:  we're copying the settings here, so that we can potentially modify
>   // the settings for the field trial.  When removing the field trial, make
>   // this a reference to the const SpdySettings again.

Good catch. OK, if it's for a field trial, that makes sense. It doesn't have the
use after free danger though, so I think rkn's in the clear.

[wtc, 2011-05-23 18:33:35]

LGTM.

I studied the definition of union SettingsFlagsAndId and
finally understood it fully.  So I suggest one more change
below.

http://codereview.chromium.org/7046016/diff/12013/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7046016/diff/12013/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:536: spdy::SettingsFlagsAndId
id(spdy::SETTINGS_MAX_CONCURRENT_STREAMS);
Please pass 0 instead of spdy::SETTINGS_MAX_CONCURRENT_STREAMS
to the constructor here and on line 560 below.

[rkn1, 2011-05-23 18:42:15]

http://codereview.chromium.org/7046016/diff/12013/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7046016/diff/12013/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:536: spdy::SettingsFlagsAndId
id(spdy::SETTINGS_MAX_CONCURRENT_STREAMS);
On 2011/05/23 18:33:35, wtc wrote:
> Please pass 0 instead of spdy::SETTINGS_MAX_CONCURRENT_STREAMS
> to the constructor here and on line 560 below.

Done.

[commit-bot: I haz the power, 2011-05-23 23:48:10]

Change committed as 86362