Introduce net::HttpServerPropertiesManager to manage server-specific properties.

Introduce net::HttpServerPropertiesManager to manage server-specific properties.

Currently the only property we manage is whether or not a server supports SPDY, as indicated by NPN. Also introduce a chrome/ implementation of HttpServerPropertiesManager that persists the information to Prefererences.

When we get a SpdySession for a SPDY server, record that that server supports SPDY in HttpServerPropertiesManager. When preconnecting, if we know that the server supports SPDY, only preconnect 1 socket.

R=willchan
BUG=66472
TEST=browser ui and unit tests,network unit tests

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

[willchan no longer on Chromium, 2011-09-02 22:31:13]

http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service.h
File net/spdy/spdy_config_service.h (right):

http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
net/spdy/spdy_config_service.h:28: : public
base::RefCountedThreadSafe<SpdyConfigService> {
SpdyConfigService should not be refcounted. I'll look to see whether or not it
makes sense for the chrome/ implementation to be refcounted (I suspect not), but
the net/ interface is only accessed from a single thread, so there's no need to
make it RefCountedThreadSafe. The network stack shouldn't hold any owning
references, only weak references. Actual storage is defined outside of net/.

http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
net/spdy/spdy_config_service.h:33: virtual void GetSpdyConfig(SpdyConfig*
config) = 0;
We plan to use this class in HttpStreamFactoryImpl::Job::Preconnect(int
num_streams); There, if the host supports SPDY, then we set num_streams to 1.

Therefore, what we want is a virtual bool SupportsSpdy(const HostPortPair&
server) const = 0; function.

http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
net/spdy/spdy_config_service.h:37: virtual void AddSpdyServer(const std::string&
server) = 0;
Instead of this, we probably want a virtual void SetSupportsSpdy(const
HostPortPair& server, bool supports_spdy);

This would probably be called in
HttpStreamFactoryImpl::Job::DoInitConnectionComplete() when we determine that
the server does or does not support SPDY.

It's important in net/ code not to make direct references to the IO thread,
because net/ is agnostic of the browser level threads. It assumes that it's
running on a single thread.

[ramant (doing other things), 2011-09-14 20:15:57]

Hi willchan,
  Made the changes we had talked about. Would appreciate if you could look at
these changes when you have sometime.

thanks much,
raman

http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service.h
File net/spdy/spdy_config_service.h (right):

http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
net/spdy/spdy_config_service.h:28: : public
base::RefCountedThreadSafe<SpdyConfigService> {
On 2011/09/02 22:31:13, willchan wrote:
> SpdyConfigService should not be refcounted. I'll look to see whether or not it
> makes sense for the chrome/ implementation to be refcounted (I suspect not),
but
> the net/ interface is only accessed from a single thread, so there's no need
to
> make it RefCountedThreadSafe. The network stack shouldn't hold any owning
> references, only weak references. Actual storage is defined outside of net/.

Done.

http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
net/spdy/spdy_config_service.h:33: virtual void GetSpdyConfig(SpdyConfig*
config) = 0;
On 2011/09/02 22:31:13, willchan wrote:
> We plan to use this class in HttpStreamFactoryImpl::Job::Preconnect(int
> num_streams); There, if the host supports SPDY, then we set num_streams to 1.
> 
> Therefore, what we want is a virtual bool SupportsSpdy(const HostPortPair&
> server) const = 0; function.

Done.

http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
net/spdy/spdy_config_service.h:37: virtual void AddSpdyServer(const std::string&
server) = 0;
On 2011/09/02 22:31:13, willchan wrote:
> Instead of this, we probably want a virtual void SetSupportsSpdy(const
> HostPortPair& server, bool supports_spdy);
> 
> This would probably be called in
> HttpStreamFactoryImpl::Job::DoInitConnectionComplete() when we determine that
> the server does or does not support SPDY.
> 
> It's important in net/ code not to make direct references to the IO thread,
> because net/ is agnostic of the browser level threads. It assumes that it's
> running on a single thread.

Done.

[willchan no longer on Chromium, 2011-09-14 20:16:47]

Sorry I'm slow on this. Been busy with the code yellow stuff. I'll try
to get you comments today.

On Wed, Sep 14, 2011 at 1:15 PM,  <rtenneti@chromium.org> wrote:
> Reviewers: willchan,
>
> Message:
> Hi willchan,
>  Made the changes we had talked about. Would appreciate if you could look at
> these changes when you have sometime.
>
> thanks much,
> raman
>
>
>
http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service.h
> File net/spdy/spdy_config_service.h (right):
>
>
http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
> net/spdy/spdy_config_service.h:28: : public
> base::RefCountedThreadSafe<SpdyConfigService> {
> On 2011/09/02 22:31:13, willchan wrote:
>>
>> SpdyConfigService should not be refcounted. I'll look to see whether
>
> or not it
>>
>> makes sense for the chrome/ implementation to be refcounted (I suspect
>
> not), but
>>
>> the net/ interface is only accessed from a single thread, so there's
>
> no need to
>>
>> make it RefCountedThreadSafe. The network stack shouldn't hold any
>
> owning
>>
>> references, only weak references. Actual storage is defined outside of
>
> net/.
>
> Done.
>
>
http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
> net/spdy/spdy_config_service.h:33: virtual void
> GetSpdyConfig(SpdyConfig* config) = 0;
> On 2011/09/02 22:31:13, willchan wrote:
>>
>> We plan to use this class in
>
> HttpStreamFactoryImpl::Job::Preconnect(int
>>
>> num_streams); There, if the host supports SPDY, then we set
>
> num_streams to 1.
>
>> Therefore, what we want is a virtual bool SupportsSpdy(const
>
> HostPortPair&
>>
>> server) const = 0; function.
>
> Done.
>
>
http://codereview.chromium.org/7827033/diff/2027/net/spdy/spdy_config_service...
> net/spdy/spdy_config_service.h:37: virtual void AddSpdyServer(const
> std::string& server) = 0;
> On 2011/09/02 22:31:13, willchan wrote:
>>
>> Instead of this, we probably want a virtual void SetSupportsSpdy(const
>> HostPortPair& server, bool supports_spdy);
>
>> This would probably be called in
>> HttpStreamFactoryImpl::Job::DoInitConnectionComplete() when we
>
> determine that
>>
>> the server does or does not support SPDY.
>
>> It's important in net/ code not to make direct references to the IO
>
> thread,
>>
>> because net/ is agnostic of the browser level threads. It assumes that
>
> it's
>>
>> running on a single thread.
>
> Done.
>
> Description:
> HttpServerPropertiesManager class to persist spdy specific data.
> During PreConnect if a server supports Spdy, then only do 1 preconnect.
> When we get a spdy session for a spdy server, add that sever as a server
> that supports Spdy and persist that data so that we could use that
> information during future browser preconnects.
>
> TEST=browser preferences, network unit tests
> R=willchan
>
> Please review this at http://codereview.chromium.org/7827033/
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/app/policy/policy_templates.json
>  M     chrome/browser/extensions/extension_webrequest_api_unittest.cc
>  M     chrome/browser/io_thread.cc
>  M     chrome/browser/net/chrome_network_delegate.h
>  M     chrome/browser/net/chrome_network_delegate.cc
>  A     chrome/browser/net/http_server_properties_manager.h
>  A     chrome/browser/net/http_server_properties_manager.cc
>  A     chrome/browser/net/http_server_properties_manager_unittest.cc
>  M     chrome/browser/policy/configuration_policy_pref_store.cc
>  M     chrome/browser/policy/configuration_policy_pref_store_unittest.cc
>  M     chrome/browser/prefs/browser_prefs.cc
>  M     chrome/browser/profiles/profile_io_data.h
>  M     chrome/browser/profiles/profile_io_data.cc
>  M     chrome/chrome_browser.gypi
>  M     chrome/chrome_tests.gypi
>  M     chrome/common/pref_names.h
>  M     chrome/common/pref_names.cc
>  M     net/base/network_delegate.h
>  M     net/base/network_delegate.cc
>  M     net/http/http_stream_factory_impl_job.cc
>  M     net/proxy/network_delegate_error_observer_unittest.cc
>  M     net/url_request/url_request_test_util.h
>  M     net/url_request/url_request_test_util.cc
>
>
>

[willchan no longer on Chromium, 2011-10-02 18:33:13]

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:60:
ALLOW_THIS_IN_INITIALIZER_LIST(io_weak_ptr_factory_(this)) {
For the io_method_factory_ and io_weak_ptr_factory_, you need to make sure you
initialize them on the IO thread. This is initialized on the UI thread right?
You probably need a InitializeOnIOThread() method called in
ProfileIOData::LazyInitialize().

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:78:
io_method_factory_.RevokeAll();
ScopedRunnableMethodFactory isn't threadsafe. You can't revoke it on the UI
thread. It'll get revoked anyway when it gets destroyed on the IO thread, so
don't bother.

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:98: void
HttpServerPropertiesManager::RegisterPrefs(PrefService* prefs) {
You should assert that this is only called from the UI thread.

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:177:
base::Unretained(this), spdy_server_list));
From IO=>UI thread, this is where you can have a leak. You need to use a weak
pointer here, not a raw pointer. Use your io_weak_ptr_factory_ here.

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

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:31: virtual bool SupportsSpdy(const
HostPortPair& server);
Member function should be const.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:33: // Add |server| into the persisitent
store. Should only be called from IO
persistent

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:35: virtual void SetSupportsSpdy(const
HostPortPair& server,
Can you make these virtuals be pure virtuals? Also make sure to use the OVERRIDE
macro.

Take all the implementation details (the non-public virtuals) and move them out
into a HttpServerPropertiesImpl class. Turn the protected member functions into
public member functions. Then have HttpServerPropertiesManager include the
HttpServerPropertiesImpl class as a member variable. This is composition. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...
which says composition is often more appropriate than inheritance.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:52: void GetFlattenedSpdyServer(const
net::HostPortPair& host_port_pair,
Does this need to be in the .h file? It seems like this could be a non-member
function placed in an anonymous namespace in the .cc file. See
https://plus.google.com/109863453537346300676/posts/jBkBP18DKCD (internal only)
for an explanation ('I feel like people often forget Effective C++'s Item 23:
"Prefer non-member non-friend functions to member functions." C++ doesn't mean
everything needs to be a member function.').

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:53: std::string& spdy_server);
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere....
spdy_server should not be a non-const reference argument. The common ways to
accomplish this are to use an pointer output argument or return the value. I'd
return the value. Don't worry about the cost of a copy, because all modern
compilers will use (http://efnetcpp.org/wiki/Return_value_optimization) to
optimize it away.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:57: typedef base::hash_map<std::string, bool>
SpdyServerHostPortTable;
types go first in the section

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:59: };
DISALLOW_COPY_AND_ASSIGN

[ramant (doing other things), 2011-10-04 05:23:24]

Hi willchan,
  Made all the changes you had suggested. Could you please take another high
level look at the changes?

thanks very much,
raman

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:60:
ALLOW_THIS_IN_INITIALIZER_LIST(io_weak_ptr_factory_(this)) {
On 2011/10/02 18:33:13, willchan wrote:
> For the io_method_factory_ and io_weak_ptr_factory_, you need to make sure you
> initialize them on the IO thread. This is initialized on the UI thread right?
> You probably need a InitializeOnIOThread() method called in
> ProfileIOData::LazyInitialize().

Done.

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:78:
io_method_factory_.RevokeAll();
On 2011/10/02 18:33:13, willchan wrote:
> ScopedRunnableMethodFactory isn't threadsafe. You can't revoke it on the UI
> thread. It'll get revoked anyway when it gets destroyed on the IO thread, so
> don't bother.

Done.

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:98: void
HttpServerPropertiesManager::RegisterPrefs(PrefService* prefs) {
On 2011/10/02 18:33:13, willchan wrote:
> You should assert that this is only called from the UI thread.

Hi willchan,
  Some of the unit tests are registering prefs on non-UI thread
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...).
Removed the assert I had tried.

Done.

http://codereview.chromium.org/7827033/diff/25001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:177:
base::Unretained(this), spdy_server_list));
On 2011/10/02 18:33:13, willchan wrote:
> From IO=>UI thread, this is where you can have a leak. You need to use a weak
> pointer here, not a raw pointer. Use your io_weak_ptr_factory_ here.

Done.

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

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:31: virtual bool SupportsSpdy(const
HostPortPair& server);
On 2011/10/02 18:33:13, willchan wrote:
> Member function should be const.

Done.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:33: // Add |server| into the persisitent
store. Should only be called from IO
On 2011/10/02 18:33:13, willchan wrote:
> persistent

Done.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:35: virtual void SetSupportsSpdy(const
HostPortPair& server,
On 2011/10/02 18:33:13, willchan wrote:
> Can you make these virtuals be pure virtuals? Also make sure to use the
OVERRIDE
> macro.
> 
> Take all the implementation details (the non-public virtuals) and move them
out
> into a HttpServerPropertiesImpl class. Turn the protected member functions
into
> public member functions. Then have HttpServerPropertiesManager include the
> HttpServerPropertiesImpl class as a member variable. This is composition. See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...
> which says composition is often more appropriate than inheritance.

Done.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:52: void GetFlattenedSpdyServer(const
net::HostPortPair& host_port_pair,
On 2011/10/02 18:33:13, willchan wrote:
> Does this need to be in the .h file? It seems like this could be a non-member
> function placed in an anonymous namespace in the .cc file. See
> https://plus.google.com/109863453537346300676/posts/jBkBP18DKCD (internal
only)
> for an explanation ('I feel like people often forget Effective C++'s Item 23:
> "Prefer non-member non-friend functions to member functions." C++ doesn't mean
> everything needs to be a member function.').

Done.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:53: std::string& spdy_server);
On 2011/10/02 18:33:13, willchan wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere....
> spdy_server should not be a non-const reference argument. The common ways to
> accomplish this are to use an pointer output argument or return the value. I'd
> return the value. Don't worry about the cost of a copy, because all modern
> compilers will use (http://efnetcpp.org/wiki/Return_value_optimization) to
> optimize it away.

Done.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:57: typedef base::hash_map<std::string, bool>
SpdyServerHostPortTable;
On 2011/10/02 18:33:13, willchan wrote:
> types go first in the section

Done.

http://codereview.chromium.org/7827033/diff/25001/net/http/http_server_proper...
net/http/http_server_properties.h:59: };
On 2011/10/02 18:33:13, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[willchan no longer on Chromium, 2011-10-04 05:55:02]

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:63:
http_server_properties_impl_.reset(new net::HttpServerPropertiesImpl());
How about moving this into InitializeOnIOThread()? Then you can make
net::HttpServerPropertiesImpl inherit from base::NonThreadSafe and add
DCHECK(CalledOnValidThread()) on all the member functions.

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:79:
ui_weak_ptr_factory_.reset(
You don't appear to use this member, can you delete it?

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:185:
base::Unretained(this), spdy_server_list));
You're using base::Unretained(this), you need to use a
base::WeakPtr<HttpServerPropertiesManager> instead, otherwise this may crash on
the UI thread. You need a io_weak_ptr_factory_ for this. Put it in a scoped_ptr
that you initialize in InitializeOnIOThread().

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.h (right):

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.h:132: mutable
scoped_ptr<net::HttpServerPropertiesImpl>
Why is this mutable?

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:7: #include
<ostream>
What's this for? Do you really need it?

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
File net/http/http_server_properties.cc (right):

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
net/http/http_server_properties.cc:9:
HttpServerProperties::HttpServerProperties() {
There's no need for these to be in the .cc file. Just inline them in the .h.

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

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
net/http/http_server_properties.h:34:
DISALLOW_COPY_AND_ASSIGN(HttpServerProperties);
This should be private.

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:71: base::ListValue* spdy_server_list) {
This should probably be a const member function. Use a const_iterator instead.

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
File net/http/http_server_properties_impl.h (right):

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
net/http/http_server_properties_impl.h:23: // The implementation for
setting/retrieving the HTTP server properties.
You should add a test for this class that tests this basic functionality. The
HttpServerPropertiesManager should only test the functionality specific to the
chrome/browser/net code or have some integration tests.

[willchan no longer on Chromium, 2011-10-04 06:12:20]

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:185:
base::Unretained(this), spdy_server_list));
On 2011/10/04 05:55:02, willchan wrote:
> You're using base::Unretained(this), you need to use a
> base::WeakPtr<HttpServerPropertiesManager> instead, otherwise this may crash
on
> the UI thread. You need a io_weak_ptr_factory_ for this. Put it in a
scoped_ptr
> that you initialize in InitializeOnIOThread().

Sorry, I got this wrong! You need the ui_weak_ptr_factory_, which should be
initialized in the constructor (on the UI thread) within a scoped_ptr. You can
delete the io_weak_ptr_factory_. Then, also in the constructor, you should
initialize a base::WeakPtr<HttpServerPropertiesManager> ui_weak_ptr_ member
variable. In ShutdownOnUIThread(), you should call ui_weak_ptr_factory_.reset().
Here, instead of base::Unretained(this), you should use ui_weak_ptr_. Note that
it's OK for the ui_weak_ptr_ to be destroyed on the IO thread. That's key here.

[ramant (doing other things), 2011-10-06 06:17:52]

Hi willchan,
  Made all the changes we had talked about. Would appreciate if you could take
another look at this CL.

thanks,
raman

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:63:
http_server_properties_impl_.reset(new net::HttpServerPropertiesImpl());
On 2011/10/04 05:55:02, willchan wrote:
> How about moving this into InitializeOnIOThread()? Then you can make
> net::HttpServerPropertiesImpl inherit from base::NonThreadSafe and add
> DCHECK(CalledOnValidThread()) on all the member functions.

Done.

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:79:
ui_weak_ptr_factory_.reset(
On 2011/10/04 05:55:02, willchan wrote:
> You don't appear to use this member, can you delete it?

Done.

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:185:
base::Unretained(this), spdy_server_list));
On 2011/10/04 06:12:20, willchan wrote:
> On 2011/10/04 05:55:02, willchan wrote:
> > You're using base::Unretained(this), you need to use a
> > base::WeakPtr<HttpServerPropertiesManager> instead, otherwise this may crash
> on
> > the UI thread. You need a io_weak_ptr_factory_ for this. Put it in a
> scoped_ptr
> > that you initialize in InitializeOnIOThread().
> 
> Sorry, I got this wrong! You need the ui_weak_ptr_factory_, which should be
> initialized in the constructor (on the UI thread) within a scoped_ptr. You can
> delete the io_weak_ptr_factory_. Then, also in the constructor, you should
> initialize a base::WeakPtr<HttpServerPropertiesManager> ui_weak_ptr_ member
> variable. In ShutdownOnUIThread(), you should call
ui_weak_ptr_factory_.reset().
> Here, instead of base::Unretained(this), you should use ui_weak_ptr_. Note
that
> it's OK for the ui_weak_ptr_ to be destroyed on the IO thread. That's key
here.

Done.

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:185:
base::Unretained(this), spdy_server_list));
On 2011/10/04 05:55:02, willchan wrote:
> You're using base::Unretained(this), you need to use a
> base::WeakPtr<HttpServerPropertiesManager> instead, otherwise this may crash
on
> the UI thread. You need a io_weak_ptr_factory_ for this. Put it in a
scoped_ptr
> that you initialize in InitializeOnIOThread().

Done.

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.h (right):

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.h:132: mutable
scoped_ptr<net::HttpServerPropertiesImpl>
On 2011/10/04 05:55:02, willchan wrote:
> Why is this mutable?

Done.

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/38003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:7: #include
<ostream>
On 2011/10/04 05:55:02, willchan wrote:
> What's this for? Do you really need it?

Done.

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
File net/http/http_server_properties.cc (right):

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
net/http/http_server_properties.cc:9:
HttpServerProperties::HttpServerProperties() {
On 2011/10/04 05:55:02, willchan wrote:
> There's no need for these to be in the .cc file. Just inline them in the .h.

Done.

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

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
net/http/http_server_properties.h:34:
DISALLOW_COPY_AND_ASSIGN(HttpServerProperties);
On 2011/10/04 05:55:02, willchan wrote:
> This should be private.

Done.

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:71: base::ListValue* spdy_server_list) {
On 2011/10/04 05:55:02, willchan wrote:
> This should probably be a const member function. Use a const_iterator instead.

Done.

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
File net/http/http_server_properties_impl.h (right):

http://codereview.chromium.org/7827033/diff/38003/net/http/http_server_proper...
net/http/http_server_properties_impl.h:23: // The implementation for
setting/retrieving the HTTP server properties.
On 2011/10/04 05:55:02, willchan wrote:
> You should add a test for this class that tests this basic functionality. The
> HttpServerPropertiesManager should only test the functionality specific to the
> chrome/browser/net code or have some integration tests.

Done.

[willchan no longer on Chromium, 2011-10-06 22:13:22]

http://codereview.chromium.org/7827033/diff/71002/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:245: 'supported_on': ['chrome.*:8-',
'chrome_os:0.11-'],
Is this right? I actually don't know what 8- means, but I'm guessing that's a
version number.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api_unittest.cc:90:
context_->set_http_server_properties(NULL);
This shouldn't be necessary, right?

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:263: context->set_http_server_properties(NULL);
I don't see why this is necessary, doesn't URLRequestContext initialize it to
NULL already?

I also thought about this, we should probably use HttpServerPropertiesManager
for the system URLRequestContext too. There's no reason this should be tied to a
profile.

Leave that as a TODO. This changelist is getting large enough as is.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/net/connecti...
chrome/browser/net/connection_tester.cc:87:
session_params.http_server_properties = NULL;
This should not be necessary, it should be NULL by default.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:132:
session_params.http_server_properties = NULL;
Shouldn't be necessary.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:179:
prefs::kSpdyServers },
This shouldn't be in the policies. That's meant for things that
enterprises/admins should be able to control. There's no reason for people to
fiddle with this.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:131: return
reinterpret_cast<net::HttpServerProperties*>(
There shouldn't be any reason to reinterpret_cast this.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:294: http_server_properties_manager_;
This doesn't belong here. It should be in ProfileImplIOData. It's not used at
all by OffTheRecordProfileIOData.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
File net/http/http_server_properties_impl.h (right):

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl.h:57:
FRIEND_TEST_ALL_PREFIXES(HttpServerPropertiesImplTest, InitializeTest);
This is a stylistic argument, but please remove these. I prefer to test via the
public API unless I need access to the internals. In this case, there's not much
need here. You can simply call Initialize(), then EXPECT_TRUE(SupportsSpdy(foo))
and EXPECT_FALSE(SupportsSpdy(bar)). Same thing to test SetSupportsSpdy() and
DeleteAll().

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
File net/http/http_server_properties_impl_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:22: 
Please put everything in an anonymous namespace within net. So it'll be
net::(anonymous)::Foo.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:24: public:
Make this protected.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:25:
scoped_ptr<HttpServerPropertiesImpl> impl_;
member variables come after methods according to the style guide.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:32: impl_.reset();
No need to call scoped_ptr<T>::reset() in the destructor. The destructor will
automatically destroy it, that's why it's scoped.

Actually, why are you using a scoped_ptr in the first place? Just make the
member be:
HttpServerPropertiesImpl impl_;

instead of:
scoped_ptr<HttpServerPropertiesImpl> impl_;

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:126:
EXPECT_EQ(impl_->spdy_servers_table_.size(), 2u);
Your expectations are reversed. The expected value should come first.

[ramant (doing other things), 2011-10-07 01:40:19]

Hi willchan,
  Made all the changes you had suggested. Removed spdy_servers prefs from
policy. Moved http_server_properties into profile_impl_io_data and fixed
http_server_properties_impl unit tests not to access private data, etc. Would
appreciate if you could take another look.

Thanks very much for your comments,
raman

http://codereview.chromium.org/7827033/diff/71002/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:245: 'supported_on': ['chrome.*:8-',
'chrome_os:0.11-'],
On 2011/10/06 22:13:22, willchan wrote:
> Is this right? I actually don't know what 8- means, but I'm guessing that's a
> version number.

Done.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api_unittest.cc:90:
context_->set_http_server_properties(NULL);
On 2011/10/06 22:13:22, willchan wrote:
> This shouldn't be necessary, right?

Done.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:263: context->set_http_server_properties(NULL);
On 2011/10/06 22:13:22, willchan wrote:
> I don't see why this is necessary, doesn't URLRequestContext initialize it to
> NULL already?
> 
> I also thought about this, we should probably use HttpServerPropertiesManager
> for the system URLRequestContext too. There's no reason this should be tied to
a
> profile.
> 
> Leave that as a TODO. This changelist is getting large enough as is.

Done.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/net/connecti...
chrome/browser/net/connection_tester.cc:87:
session_params.http_server_properties = NULL;
On 2011/10/06 22:13:22, willchan wrote:
> This should not be necessary, it should be NULL by default.

Done.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:132:
session_params.http_server_properties = NULL;
On 2011/10/06 22:13:22, willchan wrote:
> Shouldn't be necessary.

Done.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:179:
prefs::kSpdyServers },
On 2011/10/06 22:13:22, willchan wrote:
> This shouldn't be in the policies. That's meant for things that
> enterprises/admins should be able to control. There's no reason for people to
> fiddle with this.

Done.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:131: return
reinterpret_cast<net::HttpServerProperties*>(
On 2011/10/06 22:13:22, willchan wrote:
> There shouldn't be any reason to reinterpret_cast this.

Moved this code to profile_impl_io_data.h.
Done.

http://codereview.chromium.org/7827033/diff/71002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:294: http_server_properties_manager_;
On 2011/10/06 22:13:22, willchan wrote:
> This doesn't belong here. It should be in ProfileImplIOData. It's not used at
> all by OffTheRecordProfileIOData.

Done.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
File net/http/http_server_properties_impl.h (right):

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl.h:57:
FRIEND_TEST_ALL_PREFIXES(HttpServerPropertiesImplTest, InitializeTest);
On 2011/10/06 22:13:22, willchan wrote:
> This is a stylistic argument, but please remove these. I prefer to test via
the
> public API unless I need access to the internals. In this case, there's not
much
> need here. You can simply call Initialize(), then
EXPECT_TRUE(SupportsSpdy(foo))
> and EXPECT_FALSE(SupportsSpdy(bar)). Same thing to test SetSupportsSpdy() and
> DeleteAll().

Done.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
File net/http/http_server_properties_impl_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:22: 
On 2011/10/06 22:13:22, willchan wrote:
> Please put everything in an anonymous namespace within net. So it'll be
> net::(anonymous)::Foo.

Done.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:24: public:
On 2011/10/06 22:13:22, willchan wrote:
> Make this protected.

Done.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:25:
scoped_ptr<HttpServerPropertiesImpl> impl_;
On 2011/10/06 22:13:22, willchan wrote:
> member variables come after methods according to the style guide.

Done.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:32: impl_.reset();
On 2011/10/06 22:13:22, willchan wrote:
> No need to call scoped_ptr<T>::reset() in the destructor. The destructor will
> automatically destroy it, that's why it's scoped.
> 
> Actually, why are you using a scoped_ptr in the first place? Just make the
> member be:
> HttpServerPropertiesImpl impl_;
> 
> instead of:
> scoped_ptr<HttpServerPropertiesImpl> impl_;

Done.

http://codereview.chromium.org/7827033/diff/71002/net/http/http_server_proper...
net/http/http_server_properties_impl_unittest.cc:126:
EXPECT_EQ(impl_->spdy_servers_table_.size(), 2u);
On 2011/10/06 22:13:22, willchan wrote:
> Your expectations are reversed. The expected value should come first.

Done.

[willchan no longer on Chromium, 2011-10-07 01:45:15]

http://codereview.chromium.org/7827033/diff/72001/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.h (right):

http://codereview.chromium.org/7827033/diff/72001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.h:90: virtual void
PostUpdateTaskOnUI(Task* task);  // Virtual for testing.
Do these virtuals need to be protected? Are they all called by the testing mock?
Or are they only overridden? Unless they're actually called, they can be
private. Please make as many of them private as you can.

[ramant (doing other things), 2011-10-07 02:01:12]

Hi willchan,
  Thanks for catching the Post* methods being protected. Made them private.

-raman

http://codereview.chromium.org/7827033/diff/72001/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.h (right):

http://codereview.chromium.org/7827033/diff/72001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.h:90: virtual void
PostUpdateTaskOnUI(Task* task);  // Virtual for testing.
On 2011/10/07 01:45:15, willchan wrote:
> Do these virtuals need to be protected? Are they all called by the testing
mock?
> Or are they only overridden? Unless they're actually called, they can be
> private. Please make as many of them private as you can.

In the tests, we overwrite PostUpdateTaskOn* methods not to have delay (made
them private) and tests call ScheduleUpdate* methods. 

Done.

[willchan no longer on Chromium, 2011-10-07 19:11:45]

LGTM, things look great! I love the gmock tests you added, well done.

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:52: void
UpdateCacheNotMocked() {
Please rename to UpdateCacheFromPrefsConcrete().

That's the style used in the GMock docs, so it's good to be consistent.

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:192:
TEST_F(HttpServerPropertiesManagerTest, ShutdownWithPendingUpdateCache0) {
Shouldn't you have an EXPECT_CALL().Times(0) here too?

[willchan no longer on Chromium, 2011-10-07 19:15:46]

Last nits. But I just see now that there's a leak on unit_tests, lemme check
that out.

http://codereview.chromium.org/7827033/diff/72007/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/7827033/diff/72007/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.h:282: virtual void
ClearNetworkingHistorySince(base::Time time);
Please use OVERRIDE here too. We should eventually switch all of these virtuals
to use OVERRIDE.

http://codereview.chromium.org/7827033/diff/72007/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/72007/net/url_request/url_request...
net/url_request/url_request_unittest.cc:256: // TODO(rtenneti): We should
probably use HttpServerPropertiesManager for
This TODO doesn't make sense, since net/ has no concept of a "system"
URLRequestContext.

[willchan no longer on Chromium, 2011-10-07 19:17:46]

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:194: // Update the
preferences on the UI thread.
Ah yes, this task is not getting run, so this is getting leaked. Please fix
this. The lame thing to do for now until we have PassOwnPtr<T> is to use
base::RefCountedData<base::ListValue> instead of base::ListValue.

[willchan no longer on Chromium, 2011-10-08 20:22:37]

I've fixed your changelist description.

Everything LGTM and valgrind seems to pass, so it seems good to go!

http://codereview.chromium.org/7827033/diff/88001/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/7827033/diff/88001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:192:
http_server_properties_impl_->GetSpdyServerList(&(spdy_server_list->data));
I think it's more common to do:
&object->member_;

rather than

&(object->member_);

[ramant (doing other things), 2011-10-08 21:51:40]

Made the changes you suggested willchan,
raman

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:194: // Update the
preferences on the UI thread.
On 2011/10/07 19:17:47, willchan wrote:
> Ah yes, this task is not getting run, so this is getting leaked. Please fix
> this. The lame thing to do for now until we have PassOwnPtr<T> is to use
> base::RefCountedData<base::ListValue> instead of base::ListValue.

Done.

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:52: void
UpdateCacheNotMocked() {
On 2011/10/07 19:11:45, willchan wrote:
> Please rename to UpdateCacheFromPrefsConcrete().
> 
> That's the style used in the GMock docs, so it's good to be consistent.

Done.

http://codereview.chromium.org/7827033/diff/72007/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:192:
TEST_F(HttpServerPropertiesManagerTest, ShutdownWithPendingUpdateCache0) {
On 2011/10/07 19:11:45, willchan wrote:
> Shouldn't you have an EXPECT_CALL().Times(0) here too?

Done.

http://codereview.chromium.org/7827033/diff/72007/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/7827033/diff/72007/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.h:282: virtual void
ClearNetworkingHistorySince(base::Time time);
On 2011/10/07 19:15:46, willchan wrote:
> Please use OVERRIDE here too. We should eventually switch all of these
virtuals
> to use OVERRIDE.

Done.

http://codereview.chromium.org/7827033/diff/72007/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/7827033/diff/72007/net/url_request/url_request...
net/url_request/url_request_unittest.cc:256: // TODO(rtenneti): We should
probably use HttpServerPropertiesManager for
On 2011/10/07 19:15:46, willchan wrote:
> This TODO doesn't make sense, since net/ has no concept of a "system"
> URLRequestContext.

Done.

[commit-bot: I haz the power, 2011-10-09 00:31:21]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/7827033/88010

[commit-bot: I haz the power, 2011-10-09 02:01:57]

Change committed as 104666