Move a number of static variables SPDY to HttpNetworkSession::Params.

Move a number of static variables SPDY to HttpNetworkSession::Params.  Simplifies the creation of HttpNetworkSession::Params in IOThread by creating a method to do just that.  Fixes a bug in chrome/browser/profiles/profile_io_data.cc where some of the Globals were not being consulted when the HttpNetworkSession was constructed.

BUG=162571
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170967

[Ryan Hamilton, 2012-12-01 00:17:56]

Howdy Will,

So...  I hear you don't like static.  So I removed all the static in SPDY code
and replaced them with HttpNetworkSession::Params.  :>  (Since Jim was sick and
you were on vacation I mostly ignored Chrome QUIC and got to focus on SPDY
again).

Sorry this CL is so big.  It's the 4th or 5th in the series that finally led to
this one.

Cheers,

Ryan

[willchan no longer on Chromium, 2012-12-01 20:49:13]

I like it! Thanks for the cleanup. LGTM, mod nits.

https://codereview.chromium.org/11415219/diff/5001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/11415219/diff/5001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:56: #include "net/http/http_network_session.h"
?

https://codereview.chromium.org/11415219/diff/5001/net/spdy/buffered_spdy_fra...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/buffered_spdy_fra...
net/spdy/buffered_spdy_framer.cc:18: //DCHECK(!enable_compression);
?

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:31: }  // namespace
?

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:88:
scoped_ptr<MockECSignatureCreatorFactory> ec_signature_creator_factory_;
Why bother with a scoped_ptr? Do you need to reset it sometime? Otherwise, just
embed it directly...

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.cc:76: //DCHECK(!enable_compression_);
Delete?

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.h:212: 
I try to avoid nitting on stuff like this, but all the vertical whitespace
bothers me. If you don't feel strongly, can you eliminate them? If you do, then
keep them :)

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.h:231: SpdySessionPool::TimeFunc time_func_;
No need for SpdySessionPool::

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
File net/spdy/spdy_session_spdy3_unittest.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
net/spdy/spdy_session_spdy3_unittest.cc:367: session_deps.time_func =
the_near_future;
Should probably still be CamelCased. Mind changing that here too?

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
net/spdy/spdy_session_spdy3_unittest.cc:410: g_delta_seconds = 301;
Strange that we're setting a global here in a single TEST_F. It's not used
anywhere else, but if it were, then we'd have test ordering dependencies, which
are bad. Any way to resolve? Perhaps use base::Bind() instead? Or just hardcode
301 in the_near_future?

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_test_util_sp...
File net/spdy/spdy_test_util_spdy3.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_test_util_sp...
net/spdy/spdy_test_util_spdy3.cc:45: : key_(key) {
indent 4 spaces, as per style guide

[Ryan Hamilton, 2012-12-01 23:02:24]

Thanks!

https://codereview.chromium.org/11415219/diff/5001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/11415219/diff/5001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:56: #include "net/http/http_network_session.h"
On 2012/12/01 20:49:13, willchan wrote:
> ?
Add back.  (Odd)

https://codereview.chromium.org/11415219/diff/5001/net/spdy/buffered_spdy_fra...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/buffered_spdy_fra...
net/spdy/buffered_spdy_framer.cc:18: //DCHECK(!enable_compression);
On 2012/12/01 20:49:13, willchan wrote:
> ?

Removed.  (Sorry, left over from debugging)

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:31: }  // namespace
On 2012/12/01 20:49:13, willchan wrote:
> ?

Removed.

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:88:
scoped_ptr<MockECSignatureCreatorFactory> ec_signature_creator_factory_;
On 2012/12/01 20:49:13, willchan wrote:
> Why bother with a scoped_ptr? Do you need to reset it sometime? Otherwise,
just
> embed it directly...

Done.

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.cc:76: //DCHECK(!enable_compression_);
On 2012/12/01 20:49:13, willchan wrote:
> Delete?

Done.

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.h:212: 
On 2012/12/01 20:49:13, willchan wrote:
> I try to avoid nitting on stuff like this, but all the vertical whitespace
> bothers me. If you don't feel strongly, can you eliminate them? If you do,
then
> keep them :)

Done.

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_pool...
net/spdy/spdy_session_pool.h:231: SpdySessionPool::TimeFunc time_func_;
On 2012/12/01 20:49:13, willchan wrote:
> No need for SpdySessionPool::

Done.

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
File net/spdy/spdy_session_spdy3_unittest.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
net/spdy/spdy_session_spdy3_unittest.cc:367: session_deps.time_func =
the_near_future;
On 2012/12/01 20:49:13, willchan wrote:
> Should probably still be CamelCased. Mind changing that here too?

Done.

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
net/spdy/spdy_session_spdy3_unittest.cc:410: g_delta_seconds = 301;
On 2012/12/01 20:49:13, willchan wrote:
> Strange that we're setting a global here in a single TEST_F. It's not used
> anywhere else, but if it were, then we'd have test ordering dependencies,
which
> are bad. Any way to resolve? Perhaps use base::Bind() instead? Or just
hardcode
> 301 in the_near_future?

I agree that it's weird :<  Happy to refactor it, but not sure how.  TimeFunc is
a simple function pointer, so I can't use a callback.  I've change the test
fixture to set g_delta_seconds to 0 in the setup.  In this way, the ordering
problems should be eliminated, at least.  What do you think?

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_test_util_sp...
File net/spdy/spdy_test_util_spdy3.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_test_util_sp...
net/spdy/spdy_test_util_spdy3.cc:45: : key_(key) {
On 2012/12/01 20:49:13, willchan wrote:
> indent 4 spaces, as per style guide

Whoops, done.

[Ryan Hamilton, 2012-12-01 23:03:23]

thakis: can you approve the Chrome changes?

[willchan no longer on Chromium, 2012-12-01 23:05:53]

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
File net/spdy/spdy_session_spdy3_unittest.cc (right):

https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
net/spdy/spdy_session_spdy3_unittest.cc:410: g_delta_seconds = 301;
On 2012/12/01 23:02:25, Ryan Hamilton wrote:
> On 2012/12/01 20:49:13, willchan wrote:
> > Strange that we're setting a global here in a single TEST_F. It's not used
> > anywhere else, but if it were, then we'd have test ordering dependencies,
> which
> > are bad. Any way to resolve? Perhaps use base::Bind() instead? Or just
> hardcode
> > 301 in the_near_future?
> 
> I agree that it's weird :<  Happy to refactor it, but not sure how.  TimeFunc
is
> a simple function pointer, so I can't use a callback.  I've change the test
> fixture to set g_delta_seconds to 0 in the setup.  In this way, the ordering
> problems should be eliminated, at least.  What do you think?

Can we just hardcode it to 301? IIUC, we only ever set g_delta_seconds when we
set the time func, so there's no need for this extra level of control. Can we
just say that setting the time func to the_near_future() means 301ms delays?

[Ryan Hamilton, 2012-12-01 23:08:30]

On 2012/12/01 23:05:53, willchan wrote:
>
https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
> File net/spdy/spdy_session_spdy3_unittest.cc (right):
> 
>
https://codereview.chromium.org/11415219/diff/5001/net/spdy/spdy_session_spdy...
> net/spdy/spdy_session_spdy3_unittest.cc:410: g_delta_seconds = 301;
> On 2012/12/01 23:02:25, Ryan Hamilton wrote:
> > On 2012/12/01 20:49:13, willchan wrote:
> > > Strange that we're setting a global here in a single TEST_F. It's not used
> > > anywhere else, but if it were, then we'd have test ordering dependencies,
> > which
> > > are bad. Any way to resolve? Perhaps use base::Bind() instead? Or just
> > hardcode
> > > 301 in the_near_future?
> > 
> > I agree that it's weird :<  Happy to refactor it, but not sure how. 
TimeFunc
> is
> > a simple function pointer, so I can't use a callback.  I've change the test
> > fixture to set g_delta_seconds to 0 in the setup.  In this way, the ordering
> > problems should be eliminated, at least.  What do you think?
> 
> Can we just hardcode it to 301? IIUC, we only ever set g_delta_seconds when we
> set the time func, so there's no need for this extra level of control. Can we
> just say that setting the time func to the_near_future() means 301ms delays?

No, I don't think so.  The way the test works is that at one point, it uses the
time func to get the current time, then the test advances the clock.  So we need
it to return two different values.

[Nico, 2012-12-02 03:54:23]

https://codereview.chromium.org/11415219/diff/7002/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11415219/diff/7002/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:251: params->enable_spdy_ip_pooling =
globals.enable_spdy_ip_pooling;
Does this even work? params->enabled_spdy_ip_pooling is a bool,
globals.enable_spdy_ip_pooling is an enum that is either 1 or 2 (since DEFAULT
is 0), so this will always set enable_spdy_ip_pooling to true. Is this what you
want?

(If not: How did you test this?)

https://codereview.chromium.org/11415219/diff/7002/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/11415219/diff/7002/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:77: };
Ugh. Do we have an optional<> template somewhere? If not, can you add a small
nexted optional<> class here?

[Ryan Hamilton, 2012-12-03 23:04:12]

willchan: can you take a look at the change to
chrome/browser/profiles/profile_io_data.cc?  I believe this is correct, but I am
not terribly familiar with the code, so it's possible that I'm missing
something.
thakis: PTAL

https://codereview.chromium.org/11415219/diff/7002/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11415219/diff/7002/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:251: params->enable_spdy_ip_pooling =
globals.enable_spdy_ip_pooling;
On 2012/12/02 03:54:23, Nico wrote:
> Does this even work? params->enabled_spdy_ip_pooling is a bool,
> globals.enable_spdy_ip_pooling is an enum that is either 1 or 2 (since DEFAULT
> is 0), so this will always set enable_spdy_ip_pooling to true. Is this what
you
> want?
> 
> (If not: How did you test this?)

Ugh.  Yes, that was a bug.  At one point I had DEFAULT last.  I did my testing
and then changed the order.  Buggy. :< 

In any case, I've switched to your suggestion of using an Optional<> class for
these optional values.  Good suggestion.  I've also verified that they flow down
into the network stack correctly.

https://codereview.chromium.org/11415219/diff/7002/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/11415219/diff/7002/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:77: };
On 2012/12/02 03:54:23, Nico wrote:
> Ugh. Do we have an optional<> template somewhere? If not, can you add a small
> nexted optional<> class here?

Done.

[Nico, 2012-12-03 23:18:19]

chrome lgtm

[willchan no longer on Chromium, 2012-12-03 23:43:43]

I need to examine the Optional stuff as we discussed offline. The other thing
you need to is beef up your changelist description. Mention the bugfixes, this
is no longer a strict refactor.

https://codereview.chromium.org/11415219/diff/12001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11415219/diff/12001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:765: void IOThread::InitializeNetworkSessionParams(
Please move this to be a member of IOThread::Globals or a class static function.
The reason is the IOThread object lives on the UI thread, so we shouldn't be
calling a member function of a UI thread object from the IO thread. Confusing,
eh? :)

[Ryan Hamilton, 2012-12-04 00:06:04]

https://codereview.chromium.org/11415219/diff/12001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11415219/diff/12001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:765: void IOThread::InitializeNetworkSessionParams(
On 2012/12/03 23:43:43, willchan wrote:
> Please move this to be a member of IOThread::Globals 

I had it as a method on Globals initially until sleevi made me move it :>
(Personally, I liked it better there).

> or a class static function.
> The reason is the IOThread object lives on the UI thread, so we shouldn't be
> calling a member function of a UI thread object from the IO thread. Confusing,
> eh? :)

Hm.  I don't understand.  Can you elaborate?  Which invocation of this method is
problematic?

[willchan no longer on Chromium, 2012-12-04 00:23:08]

the optional stuff lgtm too, so lgtm overall

[willchan no longer on Chromium, 2012-12-04 00:37:00]

https://codereview.chromium.org/11415219/diff/12001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11415219/diff/12001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:765: void IOThread::InitializeNetworkSessionParams(
On 2012/12/04 00:06:04, Ryan Hamilton wrote:
> On 2012/12/03 23:43:43, willchan wrote:
> > Please move this to be a member of IOThread::Globals 
> 
> I had it as a method on Globals initially until sleevi made me move it :>
> (Personally, I liked it better there).
> 
> > or a class static function.
> > The reason is the IOThread object lives on the UI thread, so we shouldn't be
> > calling a member function of a UI thread object from the IO thread.
Confusing,
> > eh? :)
> 
> Hm.  I don't understand.  Can you elaborate?  Which invocation of this method
is
> problematic?  

I was too lazy to re-read, but IOThread used to inherit from base::Thread, so it
could be deleted while this happens. If there's a problem, you're not making it
any worse. But there might be a race condition here. It's fine to land, and we
may want to investigate in the future.

[commit-bot: I haz the power, 2012-12-04 00:40:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/11415219/12001

[commit-bot: I haz the power, 2012-12-04 01:02:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 03:08:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/11415219/8045

[commit-bot: I haz the power, 2012-12-04 04:08:13]

Retried try job too often on win_rel for step(s) content_browsertests

[commit-bot: I haz the power, 2012-12-04 04:35:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/11415219/8045

[commit-bot: I haz the power, 2012-12-04 07:02:48]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 15:09:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/11415219/8045

[commit-bot: I haz the power, 2012-12-04 15:52:14]

Change committed as 170967