Remove reference counting from HttpNetworkSession.

Remove reference counting from HttpNetworkSession.

This makes lifetime cleaner, and helps us avoid the case
where an HttpNetworkSession outlives the components it points
to.

Also remove some weird uses a null NetLog in GCM.

TBR=sgurun@chromium.org,davidben@chromium.org,droger@chromium.org
BUG=515947
Committed: https://crrev.com/468be2ff331c13b1a081d10a3c17e5366d26e577
Cr-Commit-Position: refs/heads/master@{#348483}

[mmenke, 2015-08-20 16:18:03]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-20 16:18:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298253002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298253002/40001

[commit-bot: I haz the power, 2015-08-20 16:21:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-20 16:21:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2015-08-27 16:20:53]

Patchset #5 (id:80001) has been deleted

[mmenke, 2015-08-31 15:49:30]

Patchset #10 (id:200001) has been deleted

[mmenke, 2015-08-31 15:49:42]

Patchset #10 (id:200001) has been deleted

[mmenke, 2015-08-31 15:49:50]

Patchset #4 (id:60001) has been deleted

[mmenke, 2015-08-31 15:49:59]

Patchset #5 (id:120001) has been deleted

[mmenke, 2015-08-31 15:50:06]

Patchset #4 (id:100001) has been deleted

[mmenke, 2015-08-31 15:50:14]

Patchset #4 (id:140001) has been deleted

[mmenke, 2015-08-31 15:50:23]

Patchset #5 (id:180001) has been deleted

[mmenke, 2015-08-31 15:50:29]

Patchset #4 (id:160001) has been deleted

[mmenke, 2015-08-31 15:50:35]

Patchset #3 (id:40001) has been deleted

[mmenke, 2015-08-31 15:50:47]

Patchset #2 (id:20001) has been deleted

[mmenke, 2015-08-31 15:50:57]

Patchset #2 (id:220001) has been deleted

[mmenke, 2015-08-31 15:55:56]

mmenke@chromium.org changed reviewers:
+ pauljensen@chromium.org, zea@chromium.org

[mmenke, 2015-08-31 15:55:57]

Paul:  Please review everything:
Zea:  Please review google_apis/gcm and components/gcm_driver

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
components/gcm_driver/gcm_client.h:221: //     deleted before the Getter's
underlying URLRequestContext.
This may seem like a scary new requirement, but we're actually already depending
on it.  The HttpNetworkSession may nominally have been refcounted, but it wasn't
safe to use after ProfileIOData was deleting, since HttpNetworkSession::Params
contains a bunch of raw pointers.

[Nicolas Zea, 2015-08-31 19:43:28]

GCM mostly LG

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
components/gcm_driver/gcm_client.h:221: //     deleted before the Getter's
underlying URLRequestContext.
On 2015/08/31 15:55:56, mmenke wrote:
> This may seem like a scary new requirement, but we're actually already
depending
> on it.  The HttpNetworkSession may nominally have been refcounted, but it
wasn't
> safe to use after ProfileIOData was deleting, since HttpNetworkSession::Params
> contains a bunch of raw pointers.

Acknowledged.

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:490: nullptr,
Did you mean to change this?

[mmenke, 2015-08-31 19:48:51]

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:490: nullptr,
On 2015/08/31 19:43:28, Nicolas Zea wrote:
> Did you mean to change this?

Yes.  The current code is creating a BoundNetLog with no arguments (Which has a
NULL net_log()), and then only using it to pass its NULL net_log() pointer to
this method.  Just a case of someone not familiar with net/ not doing stuff
correctly.

It's a bit removed from the CL, but I'm too lazy to make another CL that fixes
the code to be less silly.

If we want to hook up a non-NULL NetLog, we should grab the one from
HttpURLRequestContext - which even more removed from this CL, and not something
I'm all that sure we want to do.

[mmenke, 2015-08-31 20:13:10]

On 2015/08/31 19:48:51, mmenke wrote:
>
https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
> File components/gcm_driver/gcm_client_impl.cc (right):
> 
>
https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
> components/gcm_driver/gcm_client_impl.cc:490: nullptr,
> On 2015/08/31 19:43:28, Nicolas Zea wrote:
> > Did you mean to change this?
> 
> Yes.  The current code is creating a BoundNetLog with no arguments (Which has
a
> NULL net_log()), and then only using it to pass its NULL net_log() pointer to
> this method.  Just a case of someone not familiar with net/ not doing stuff
> correctly.
> 
> It's a bit removed from the CL, but I'm too lazy to make another CL that fixes
> the code to be less silly.
> 
> If we want to hook up a non-NULL NetLog, we should grab the one from
> HttpURLRequestContext - which even more removed from this CL, and not
something
> I'm all that sure we want to do.

Oh, and if you prefer I not make that change in this CL, I'm fine leaving it
as-is, just noticed it while touching the code, and thought I could do a modest
cleanup there (Which in no way affects behavior), but if you prefer a more
focused CL, I think that's reasonable, particularly given the size of this CL.

[Nicolas Zea, 2015-08-31 22:26:50]

GCM LGTM

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:490: nullptr,
On 2015/08/31 19:48:51, mmenke wrote:
> On 2015/08/31 19:43:28, Nicolas Zea wrote:
> > Did you mean to change this?
> 
> Yes.  The current code is creating a BoundNetLog with no arguments (Which has
a
> NULL net_log()), and then only using it to pass its NULL net_log() pointer to
> this method.  Just a case of someone not familiar with net/ not doing stuff
> correctly.
> 
> It's a bit removed from the CL, but I'm too lazy to make another CL that fixes
> the code to be less silly.
> 
> If we want to hook up a non-NULL NetLog, we should grab the one from
> HttpURLRequestContext - which even more removed from this CL, and not
something
> I'm all that sure we want to do.

Got it. May as well just remove the net_log_ member variable then as it's never
actually used anymore.

[mmenke, 2015-09-01 17:20:52]

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1298253002/diff/260001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:490: nullptr,
On 2015/08/31 22:26:49, Nicolas Zea wrote:
> On 2015/08/31 19:48:51, mmenke wrote:
> > On 2015/08/31 19:43:28, Nicolas Zea wrote:
> > > Did you mean to change this?
> > 
> > Yes.  The current code is creating a BoundNetLog with no arguments (Which
has
> a
> > NULL net_log()), and then only using it to pass its NULL net_log() pointer
to
> > this method.  Just a case of someone not familiar with net/ not doing stuff
> > correctly.
> > 
> > It's a bit removed from the CL, but I'm too lazy to make another CL that
fixes
> > the code to be less silly.
> > 
> > If we want to hook up a non-NULL NetLog, we should grab the one from
> > HttpURLRequestContext - which even more removed from this CL, and not
> something
> > I'm all that sure we want to do.
> 
> Got it. May as well just remove the net_log_ member variable then as it's
never
> actually used anymore.

Done.

[pauljensen, 2015-09-02 14:32:30]

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:186:
cookie_store_(cookie_store) {
while we're reordering initialization here, can we move proxy_config_service_'s
initialization up to between net_log_ and cookie_store_ to match declaration
order?

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:189: new
AwHttpUserAgentSettings());
nit: It's odd that some of these fields are initialized in the initializer list
and some are initialized in the function body.  Seems like
http_user_agent_settings_ initialization could be moved up to the initializer
list for more consistency.

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:244: true));
/* set_up_quic_server_info */

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:256:
http_user_agent_settings_.get());
one idea to simplify all this code a lot more would be to use a
URLRequestContextStorage, rather than defining scoped_ptr's for each thing that
goes into the Context.  maybe not for this CL.

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.h:69:
scoped_ptr<net::NetLog> net_log_;
I like how you've reordered this here to better match required destruction order
and construction/dependence order.
nit: WDYT about changing net_log_ to simply be a net::NetLog instead of a
scoped_ptr<net::NetLog>?  Ditto for http_user_agent_settings_? Maybe not for
this CL.

https://codereview.chromium.org/1298253002/diff/330001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:546:
net::HttpCache::BackendFactory* main_backend(
This should be a scoped_ptr and CreateMainHttpFactory should take a scoped_ptr. 
Maybe not for this CL.

https://codereview.chromium.org/1298253002/diff/330001/content/shell/browser/...
File content/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/content/shell/browser/...
content/shell/browser/shell_url_request_context_getter.cc:205:
storage_->http_network_session(), main_backend, true);
/* set_up_quic_server_info */

https://codereview.chromium.org/1298253002/diff/330001/ios/crnet/crnet_enviro...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1298253002/diff/330001/ios/crnet/crnet_enviro...
ios/crnet/crnet_environment.mm:449: // TODO(mmenke):  These really shouldn't be
leaked.
Perhaps expanding and mentioning http://crbug.com/523858 might be a good idea.

https://codereview.chromium.org/1298253002/diff/330001/ios/web/shell/shell_ur...
File ios/web/shell/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/ios/web/shell/shell_ur...
ios/web/shell/shell_url_request_context_getter.cc:139: make_scoped_ptr(new
net::HttpNetworkSession(network_session_params)));
FYI this will conflict with
https://codereview.chromium.org/1290243007/diff/140001/ios/web/shell/shell_ur...

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h#...
net/http/http_cache.h:137: // information about which servers support SPDY.
SPDY->QUIC

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h#...
net/http/http_cache.h:139: BackendFactory* backend_factory,
should be a scoped_ptr...for another CL.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h#...
net/http/http_cache.h:145: HttpCache(HttpTransactionFactory* network_layer,
ditto

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h#...
net/http/http_cache.h:147: BackendFactory* backend_factory,
ditto

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
File net/http/http_network_layer.h (right):

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
net/http/http_network_layer.h:30: // the HttpNetworkSession.
You're missing a word here, perhaps "before".

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
net/http/http_network_layer.h:45: HttpNetworkSession* session_;
nit: *->*const

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
File net/http/http_network_session_peer.h (right):

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
net/http/http_network_session_peer.h:34: HttpNetworkSession* session_;
nit: *->*const

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:7985: const
scoped_ptr<HttpNetworkSession>& session) {
This should be a raw pointer.  Passing scoped_ptr's by reference is strongly
discouraged: https://www.chromium.org/developers/smart-pointer-guidelines see
"What about passing or returning a smart pointer by reference?"

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_network_...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_network_...
net/spdy/spdy_network_transaction_unittest.cc:4585:
helper.SetSession(session.Pass());
Combining these three lines into the following code is shorter but also easier
to read (whenever I see a defined scoped_ptr variable I assume it's
intentionally scoped right there, rather than immediately Pass()'ed):
helper.SetSession(
    SpdySessionDependencies::SpdyCreateSession(helper.session_deps().get()));

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_network_...
net/spdy/spdy_network_transaction_unittest.cc:4651:
helper.SetSession(session.Pass());
ditto

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.cc:523: const scoped_ptr<HttpNetworkSession>&
http_session,
should be a raw pointer

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.h (right):

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:246: const scoped_ptr<HttpNetworkSession>&
http_session,
Should be a raw pointer.

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:255: const scoped_ptr<HttpNetworkSession>&
http_session,
ditto

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:262: const scoped_ptr<HttpNetworkSession>&
http_session,
ditto

https://codereview.chromium.org/1298253002/diff/330001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:7041:
HttpCache::DefaultBackend::InMemory(0), true);
Can you explain your logic for setting set_up_quic_server_info?

https://codereview.chromium.org/1298253002/diff/330001/net/url_request/view_c...
File net/url_request/view_cache_helper_unittest.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/url_request/view_c...
net/url_request/view_cache_helper_unittest.cc:35: NULL,
nullptr?

[mmenke, 2015-09-02 16:18:31]

Patchset #9 (id:370001) has been deleted

[mmenke, 2015-09-02 16:29:34]

Paul:  Thanks for taking the time to carefully review the CL!  Know these huge
refactors are a pain.

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:186:
cookie_store_(cookie_store) {
On 2015/09/02 14:32:29, pauljensen wrote:
> while we're reordering initialization here, can we move
proxy_config_service_'s
> initialization up to between net_log_ and cookie_store_ to match declaration
> order?

Done.

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:189: new
AwHttpUserAgentSettings());
On 2015/09/02 14:32:29, pauljensen wrote:
> nit: It's odd that some of these fields are initialized in the initializer
list
> and some are initialized in the function body.  Seems like
> http_user_agent_settings_ initialization could be moved up to the initializer
> list for more consistency.

Done.

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:244: true));
On 2015/09/02 14:32:29, pauljensen wrote:
> /* set_up_quic_server_info */

Done.

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:256:
http_user_agent_settings_.get());
On 2015/09/02 14:32:29, pauljensen wrote:
> one idea to simplify all this code a lot more would be to use a
> URLRequestContextStorage, rather than defining scoped_ptr's for each thing
that
> goes into the Context.  maybe not for this CL.

I'd really like to just switch to using URLRequestContextBuilder, actually.

Agree that it's beyond the scope of this CL, either way.

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/1298253002/diff/330001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.h:69:
scoped_ptr<net::NetLog> net_log_;
On 2015/09/02 14:32:29, pauljensen wrote:
> I like how you've reordered this here to better match required destruction
order
> and construction/dependence order.
> nit: WDYT about changing net_log_ to simply be a net::NetLog instead of a
> scoped_ptr<net::NetLog>?  Ditto for http_user_agent_settings_? Maybe not for
> this CL.

I think it makes sense, but is a bit beyond the scope of this CL.

https://codereview.chromium.org/1298253002/diff/330001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:546:
net::HttpCache::BackendFactory* main_backend(
On 2015/09/02 14:32:29, pauljensen wrote:
> This should be a scoped_ptr and CreateMainHttpFactory should take a
scoped_ptr. 
> Maybe not for this CL.

I agree (But again, not in this CL - it already does a lot).

https://codereview.chromium.org/1298253002/diff/330001/content/shell/browser/...
File content/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/content/shell/browser/...
content/shell/browser/shell_url_request_context_getter.cc:205:
storage_->http_network_session(), main_backend, true);
On 2015/09/02 14:32:29, pauljensen wrote:
> /* set_up_quic_server_info */

Done.

https://codereview.chromium.org/1298253002/diff/330001/ios/crnet/crnet_enviro...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1298253002/diff/330001/ios/crnet/crnet_enviro...
ios/crnet/crnet_environment.mm:449: // TODO(mmenke):  These really shouldn't be
leaked.
On 2015/09/02 14:32:29, pauljensen wrote:
> Perhaps expanding and mentioning http://crbug.com/523858 might be a good idea.

Done.

https://codereview.chromium.org/1298253002/diff/330001/ios/web/shell/shell_ur...
File ios/web/shell/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/ios/web/shell/shell_ur...
ios/web/shell/shell_url_request_context_getter.cc:139: make_scoped_ptr(new
net::HttpNetworkSession(network_session_params)));
On 2015/09/02 14:32:29, pauljensen wrote:
> FYI this will conflict with
>
https://codereview.chromium.org/1290243007/diff/140001/ios/web/shell/shell_ur...

Acknowledged.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h#...
net/http/http_cache.h:137: // information about which servers support SPDY.
On 2015/09/02 14:32:29, pauljensen wrote:
> SPDY->QUIC

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h#...
net/http/http_cache.h:139: BackendFactory* backend_factory,
On 2015/09/02 14:32:29, pauljensen wrote:
> should be a scoped_ptr...for another CL.

Almost did it in this CL, but just got too massive.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h#...
net/http/http_cache.h:145: HttpCache(HttpTransactionFactory* network_layer,
On 2015/09/02 14:32:29, pauljensen wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_cache.h#...
net/http/http_cache.h:147: BackendFactory* backend_factory,
On 2015/09/02 14:32:29, pauljensen wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
File net/http/http_network_layer.h (right):

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
net/http/http_network_layer.h:30: // the HttpNetworkSession.
On 2015/09/02 14:32:29, pauljensen wrote:
> You're missing a word here, perhaps "before".

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
net/http/http_network_layer.h:45: HttpNetworkSession* session_;
On 2015/09/02 14:32:29, pauljensen wrote:
> nit: *->*const

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
File net/http/http_network_session_peer.h (right):

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
net/http/http_network_session_peer.h:34: HttpNetworkSession* session_;
On 2015/09/02 14:32:29, pauljensen wrote:
> nit: *->*const

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:7985: const
scoped_ptr<HttpNetworkSession>& session) {
On 2015/09/02 14:32:29, pauljensen wrote:
> This should be a raw pointer.  Passing scoped_ptr's by reference is strongly
> discouraged: https://www.chromium.org/developers/smart-pointer-guidelines see
> "What about passing or returning a smart pointer by reference?"

Yikes!  That was a careless mistake when doing mass replacement.  Thanks for
catching it!  Fixed.

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_network_...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_network_...
net/spdy/spdy_network_transaction_unittest.cc:4585:
helper.SetSession(session.Pass());
On 2015/09/02 14:32:29, pauljensen wrote:
> Combining these three lines into the following code is shorter but also easier
> to read (whenever I see a defined scoped_ptr variable I assume it's
> intentionally scoped right there, rather than immediately Pass()'ed):
> helper.SetSession(
>     SpdySessionDependencies::SpdyCreateSession(helper.session_deps().get()));

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_network_...
net/spdy/spdy_network_transaction_unittest.cc:4651:
helper.SetSession(session.Pass());
On 2015/09/02 14:32:29, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.cc:523: const scoped_ptr<HttpNetworkSession>&
http_session,
On 2015/09/02 14:32:30, pauljensen wrote:
> should be a raw pointer

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.h (right):

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:246: const scoped_ptr<HttpNetworkSession>&
http_session,
On 2015/09/02 14:32:30, pauljensen wrote:
> Should be a raw pointer.

Done.  Also searched the entire CL for this mistake, just in case you missed one
(You didn't).

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:255: const scoped_ptr<HttpNetworkSession>&
http_session,
On 2015/09/02 14:32:30, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:262: const scoped_ptr<HttpNetworkSession>&
http_session,
On 2015/09/02 14:32:30, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/1298253002/diff/330001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:7041:
HttpCache::DefaultBackend::InMemory(0), true);
On 2015/09/02 14:32:30, pauljensen wrote:
> Can you explain your logic for setting set_up_quic_server_info?

I tried to keep the existing logic.

If the old constructor took in an HttpNetworkSession, it was equivalent to false
before.  If it took HttpNetworkSession::Params or an HttpTransactionFactory, old
behavior is equivalent to true.

There may be places where it makes sense to change behaviors, but this CL
already does a lot.

https://codereview.chromium.org/1298253002/diff/330001/net/url_request/view_c...
File net/url_request/view_cache_helper_unittest.cc (right):

https://codereview.chromium.org/1298253002/diff/330001/net/url_request/view_c...
net/url_request/view_cache_helper_unittest.cc:35: NULL,
On 2015/09/02 14:32:30, pauljensen wrote:
> nullptr?

Done.

[mmenke, 2015-09-09 18:14:32]

Paul:  Ping!

[pauljensen, 2015-09-09 18:23:36]

Net bug triage Thurs, Fri.  Vacation Mon.  Build sheriff Tues, Wed.  So I may
get to this later this week, though I haven't started that thing that's due
tomorrow night.

[mmenke, 2015-09-09 18:25:28]

On 2015/09/09 18:23:36, pauljensen wrote:
> Net bug triage Thurs, Fri.  Vacation Mon.  Build sheriff Tues, Wed.  So I may
> get to this later this week, though I haven't started that thing that's due
> tomorrow night.

Knew about triage last week, not about build sheriff - take all the time out
need.  No rush, just wanted to make sure it's still on your radar.  Fine if you
can't get to it until next week.

[pauljensen, 2015-09-11 13:05:25]

lgtm with what amount to nits

https://codereview.chromium.org/1298253002/diff/390001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/1298253002/diff/390001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:545: http_network_session_ =
CreateHttpNetworkSession(*profile_params);
nit: doesn't matter, but you're changing the construction order here by swapping
HttpNetworkSession and HttpCache backend.

https://codereview.chromium.org/1298253002/diff/390001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1298253002/diff/390001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1292: // Creates the HTTP network
session
Google C++ style guide says "you should not just repeat the comments given with
the function declaration".
I think the name of the function is pretty descriptive and the declaration and
definition comments could be removed as they're somewhat redundant with the
name.

https://codereview.chromium.org/1298253002/diff/390001/ios/crnet/crnet_enviro...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1298253002/diff/390001/ios/crnet/crnet_enviro...
ios/crnet/crnet_environment.mm:450: //                See
http://crbug.com/523858.
nit: http->https or personally I'd shorten it to just "crbug.com/523858" and put
it on the previous line.

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
File net/http/http_network_layer.h (right):

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
net/http/http_network_layer.h:29: // contains a valid ProxyService. The
HttpTransactionFactory must be destroyed
"HttpTransactionFactory" is a bit confusing in this context; could we instead
say "constructed HttpNetworkLayer"?

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
net/http/http_network_layer.h:30: // before the HttpNetworkSession.
maybe replace "the HttpNetworkSession" with "|session|" to be more specific.

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
File net/http/http_network_session_peer.h (right):

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
net/http/http_network_session_peer.h:21: explicit
HttpNetworkSessionPeer(HttpNetworkSession* session);
should we add a comment like "|session| must outlive constructed
HttpNetworkSessionPeer"?

https://codereview.chromium.org/1298253002/diff/390001/net/url_request/url_re...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1298253002/diff/390001/net/url_request/url_re...
net/url_request/url_request_context_builder.cc:393: HttpTransactionFactory*
http_transaction_factory = NULL;
nit: remove the " = NULL" like you did in profile_impl_io_data.cc

[mmenke, 2015-09-11 14:46:50]

Thanks for the review!  I'll plan to TBR the other owners.

https://codereview.chromium.org/1298253002/diff/390001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/1298253002/diff/390001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:545: http_network_session_ =
CreateHttpNetworkSession(*profile_params);
On 2015/09/11 13:05:25, pauljensen wrote:
> nit: doesn't matter, but you're changing the construction order here by
swapping
> HttpNetworkSession and HttpCache backend.

Done.

https://codereview.chromium.org/1298253002/diff/390001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1298253002/diff/390001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1292: // Creates the HTTP network
session
On 2015/09/11 13:05:25, pauljensen wrote:
> Google C++ style guide says "you should not just repeat the comments given
with
> the function declaration".
> I think the name of the function is pretty descriptive and the declaration and
> definition comments could be removed as they're somewhat redundant with the
> name.

Done.  Accidentally copied from the header.  Removed that one, too.

https://codereview.chromium.org/1298253002/diff/390001/ios/crnet/crnet_enviro...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1298253002/diff/390001/ios/crnet/crnet_enviro...
ios/crnet/crnet_environment.mm:450: //                See
http://crbug.com/523858.
On 2015/09/11 13:05:25, pauljensen wrote:
> nit: http->https or personally I'd shorten it to just "crbug.com/523858" and
put
> it on the previous line.

Done (Went with https - some editors have build-in URL support, seems like it's
nice to support them).

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
File net/http/http_network_layer.h (right):

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
net/http/http_network_layer.h:29: // contains a valid ProxyService. The
HttpTransactionFactory must be destroyed
On 2015/09/11 13:05:25, pauljensen wrote:
> "HttpTransactionFactory" is a bit confusing in this context; could we instead
> say "constructed HttpNetworkLayer"?

Done.

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
net/http/http_network_layer.h:30: // before the HttpNetworkSession.
On 2015/09/11 13:05:25, pauljensen wrote:
> maybe replace "the HttpNetworkSession" with "|session|" to be more specific.

Done.

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
File net/http/http_network_session_peer.h (right):

https://codereview.chromium.org/1298253002/diff/390001/net/http/http_network_...
net/http/http_network_session_peer.h:21: explicit
HttpNetworkSessionPeer(HttpNetworkSession* session);
On 2015/09/11 13:05:25, pauljensen wrote:
> should we add a comment like "|session| must outlive constructed
> HttpNetworkSessionPeer"?

Done

https://codereview.chromium.org/1298253002/diff/390001/net/url_request/url_re...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1298253002/diff/390001/net/url_request/url_re...
net/url_request/url_request_context_builder.cc:393: HttpTransactionFactory*
http_transaction_factory = NULL;
On 2015/09/11 13:05:25, pauljensen wrote:
> nit: remove the " = NULL" like you did in profile_impl_io_data.cc

Done.

[pauljensen, 2015-09-11 14:47:56]

lgtm

[mmenke, 2015-09-11 14:55:04]

mmenke@chromium.org changed reviewers:
+ davidben@chromium.org, droger@chromium.org, sgurun@chromium.org

[mmenke, 2015-09-11 14:55:05]

TBRing other OWNERs (This is an API change, refcounted to non-refcounted -
remaining changes are fairly trivial.  I'll plan to land tonight or tomorrow,
barring comments).

zea for src/jingle (Oops...zea already signed off on GCM changes)
sgurun for android_webview.
davidben for content/shell/browser
droger for src/ios

[droger, 2015-09-11 15:13:07]

//ios LGTM

[davidben, 2015-09-11 15:34:36]

content/shell/browser lgtm

[mmenke, 2015-09-11 20:09:52]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2015-09-11 20:09:53]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
pauljensen@chromium.org, davidben@chromium.org, droger@chromium.org
Link to the patchset: https://codereview.chromium.org/1298253002/#ps430001
(title: "Merge")

[mmenke, 2015-09-11 20:09:57]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2015-09-11 20:34:12]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-09-11 20:34:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298253002/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298253002/430001

[commit-bot: I haz the power, 2015-09-11 20:42:37]

Committed patchset #11 (id:430001)

[commit-bot: I haz the power, 2015-09-11 20:43:24]

Patchset 11 (id:??) landed as
https://crrev.com/468be2ff331c13b1a081d10a3c17e5366d26e577
Cr-Commit-Position: refs/heads/master@{#348483}

[mmenke, 2015-09-12 02:00:03]

On 2015/09/11 20:43:24, commit-bot: I haz the power wrote:
> Patchset 11 (id:??) landed as
> https://crrev.com/468be2ff331c13b1a081d10a3c17e5366d26e577
> Cr-Commit-Position: refs/heads/master@{#348483}

Hrm...Looks like this is probably causing memory bots to fail with a stack that
makes no sense.  Going to revert for now.  :(

[mmenke, 2015-09-12 02:03:06]

A revert of this CL (patchset #11 id:430001) has been created in
https://codereview.chromium.org/1342613002/ by mmenke@chromium.org.

The reason for reverting is: Causing net tests to fail on a couple memory FYI
bots with a stack that makes no sense, as there should be SpdySessionPool
created until after SetUp has completed:

[----------] 488 tests from NextProto/HttpNetworkTransactionTest
[ RUN      ] NextProto/HttpNetworkTransactionTest.Basic/0
[4832:3079:0911/175605:3116231085741:FATAL:weak_ptr.h(211)] Check failed: get()
!= NULL. 
0   net_unittests                       0x0000000100e4e4a3
base::debug::StackTrace::StackTrace() + 19
1   net_unittests                       0x0000000100e4e4c9
base::debug::StackTrace::StackTrace() + 9
2   net_unittests                       0x0000000100e66ad3
logging::LogMessage::~LogMessage() + 67
3   net_unittests                       0x0000000100e662d9
logging::LogMessage::~LogMessage() + 9
4   net_unittests                       0x00000001004a8d3a
base::WeakPtr<net::HttpServerProperties>::operator->() const + 90
5   net_unittests                       0x0000000101257267
net::SpdySessionPool::OnIPAddressChanged() + 439
6   net_unittests                       0x00000001010422a7
_ZN4base20DispatchToMethodImplIN3net21NetworkChangeNotifier17IPAddressObserverEMS3_FvvEJEJEEEvPT_T0_RKNS_5TupleIJDpT1_EEENS_13IndexSequenceIJXspT2_EEEE
+ 23
7   net_unittests                       0x0000000101042289
_ZN4base16DispatchToMethodIN3net21NetworkChangeNotifier17IPAddressObserverEMS3_FvvEJEEEvPT_T0_RKNS_5TupleIJDpT1_EEE
+ 9
8   net_unittests                       0x00000001010420ea
_ZNK4base8internal13UnboundMethodIN3net21NetworkChangeNotifier17IPAddressObserverEMS4_FvvENS_5TupleIJEEEE3RunEPS4_
+ 26
9   net_unittests                       0x0000000101041b3b
_ZN4base22ObserverListThreadSafeIN3net21NetworkChangeNotifier17IPAddressObserverEE13NotifyWrapperIMS3_FvvENS_5TupleIJEEEEEvPNS4_19ObserverListContextERKNS_8internal13UnboundMethodIS3_T_T0_EE
+ 203
10  net_unittests                       0x0000000101041fb5
_ZN4base8internal15RunnableAdapterIMNS_22ObserverListThreadSafeIN3net21NetworkChangeNotifier17IPAddressObserverEEEFvPNS6_19ObserverListContextERKNS0_13UnboundMethodIS5_MS5_FvvENS_5TupleIJEEEEEEE3RunEPS6_RKS8_SG_
+ 69
11  net_unittests                       0x0000000101041efb
_ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMNS_22ObserverListThreadSafeIN3net21NetworkChangeNotifier17IPAddressObserverEEEFvPNS7_19ObserverListContextERKNS0_13UnboundMethodIS6_MS6_FvvENS_5TupleIJEEEEEEEENS0_8TypeListIJRKPS7_RKS9_SH_EEEE8MakeItSoESK_SO_SQ_SH_
+ 75
12  net_unittests                       0x0000000101041e99
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1ELm2EEEENS0_9BindStateINS0_15RunnableAdapterIMNS_22ObserverListThreadSafeIN3net21NetworkChangeNotifier17IPAddressObserverEEEFvPNSA_19ObserverListContextERKNS0_13UnboundMethodIS9_MS9_FvvENS_5TupleIJEEEEEEEEFvPSA_SC_SK_ENS0_8TypeListIJSO_SC_SI_EEEEENSQ_IJNS0_12UnwrapTraitsISO_EENST_ISC_EENST_ISI_EEEEENS0_12InvokeHelperILb0EvSN_NSQ_IJRKSO_RKSC_SK_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
+ 105
13  net_unittests                       0x00000001000d31b4 base::Callback<void
()()>::Run() const + 20
14  net_unittests                       0x0000000100e4eea5
base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) + 165
15  net_unittests                       0x0000000100e751fa
base::MessageLoop::RunTask(base::PendingTask const&) + 282
16  net_unittests                       0x0000000100e7538c
base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) + 28
17  net_unittests                       0x0000000100e75462
base::MessageLoop::DoWork() + 146
18  net_unittests                       0x0000000100e3f42a
base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) + 106
19  net_unittests                       0x0000000100e74f5e
base::MessageLoop::RunHandler() + 158
20  net_unittests                       0x0000000100e953b3 base::RunLoop::Run()
+ 51
21  net_unittests                       0x0000000100e954bd
base::RunLoop::RunUntilIdle() + 13
22  net_unittests                       0x0000000100e74711
base::MessageLoop::RunUntilIdle() + 113
23  net_unittests                       0x00000001004ad776
net::HttpNetworkTransactionTest::SetUp() + 22
24  net_unittests                       0x0000000100f2d837 void
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*) + 23
25  net_unittests                       0x0000000100f278e9 void
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*) + 57
26  net_unittests                       0x0000000100f20566 testing::Test::Run()
+ 70
27  net_unittests                       0x0000000100f20ab3
testing::TestInfo::Run() + 163
28  net_unittests                       0x0000000100f20e52
testing::TestCase::Run() + 178
29  net_unittests                       0x0000000100f24733
testing::internal::UnitTestImpl::RunAllTests() + 499
30  net_unittests                       0x0000000100f2e897 bool
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool>(testing::internal::UnitTestImpl*, bool
(testing::internal::UnitTestImpl::*)(), char const*) + 23
31  net_unittests                       0x0000000100f28619 bool
testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool>(testing::internal::UnitTestImpl*, bool
(testing::internal::UnitTestImpl::*)(), char const*) + 57
32  net_unittests                       0x0000000100f24523
testing::UnitTest::Run() + 115
33  net_unittests                       0x0000000101649281 RUN_ALL_TESTS() + 17
34  net_unittests                       0x0000000101648852
base::TestSuite::Run() + 98
35  net_unittests                       0x0000000100c9509c
base::internal::RunnableAdapter<int
(base::TestSuite::*)()>::Run(base::TestSuite*) + 28
36  net_unittests                       0x0000000100c95049
_ZN4base8internal12InvokeHelperILb0EiNS0_15RunnableAdapterIMNS_9TestSuiteEFivEEENS0_8TypeListIJP12NetTestSuiteEEEE8MakeItSoES6_S9_
+ 41
37  net_unittests                       0x0000000100c9500e
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMNS_9TestSuiteEFivEEEFiPS6_ENS0_8TypeListIJNS0_17UnretainedWrapperI12NetTestSuiteEEEEEEENSC_IJNS0_12UnwrapTraitsISF_EEEEENS0_12InvokeHelperILb0EiS9_NSC_IJPSE_EEEEEFivEE3RunEPNS0_13BindStateBaseE
+ 46
38  net_unittests                       0x00000001010e4fb4 base::Callback<int
()()>::Run() const + 20
39  net_unittests                       0x0000000101640c78 base::(anonymous
namespace)::LaunchUnitTestsInternal(base::Callback<int ()()> const&, int, bool,
base::Callback<void ()()> const&) + 264
40  net_unittests                       0x0000000101640b53
base::LaunchUnitTests(int, char**, base::Callback<int ()()> const&) + 83
41  net_unittests                       0x0000000100c94d6b main + 123
42  net_unittests                       0x0000000100001674 start + 52
43  ???                                 0x0000000000000005 0x0 + 5
.

[commit-bot: I haz the power, 2015-09-23 12:26:13]

Patchset 11 (id:??) landed as
https://crrev.com/468be2ff331c13b1a081d10a3c17e5366d26e577
Cr-Commit-Position: refs/heads/master@{#348483}