Remove static variables from HttpStreamFactory.

Remove static variables from HttpStreamFactory.

Refresh of issue 10389162.

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

[szager1, 2012-08-20 23:55:59]

Dusting this one off... the last comment from mmenke was:

"Heard back from Will, and it seems like it would be a good idea for both
ServiceURLRequestContext and URLRequestContextBuilder::Build to support the
flags.  Shouldn't be too hard to make the flags settable with
URLRequestContextBuilder.

ServiceURLRequestContext is a little trickier.  It actually lives in another
process, and the constructor is in net/.  My suggestion is to get everything
else consistent, and then handle ServiceURLRequestContext in a followup CL,
since modifying it will actually change behavior slightly."

[mmenke, 2012-08-21 17:55:47]

Sorry, I may not get to this until tomorrow.

[szager1, 2012-08-22 00:12:34]

Upon further examination, it appears that ServiceURLRequestContext is not
currently affected by the static variables in HttpNetworkSesion in the browser
process.  I've filed a bug:

https://code.google.com/p/chromium/issues/detail?id=144060

So I'm going to leave ServiceURLRequestContext alone until that one gets figured
out.

[szager1, 2012-08-23 17:20:08]

ping

[mmenke, 2012-08-23 18:11:27]

Just nits.  The main concern is to make sure we don't break anything by not
having something support these values.

As far as I can tell, these are what don't support the moved flags: 
ConnectionTester, ServiceURLRequestContext, Remoting's URLRequestContext,
jingle's ProxyResolvingClientSocket.

There are also some unit tests and fetch_client, but those should be fine.

So, of those...

* I don't think ConnectionTester isn't really necessary, though it would be nice
to have.  It's more aimed at testing proxy settings, and only used by
net-internals.  The net-internals browser tests do exercise this code, but
browser_tests don't use kHostRules, so things should be fine.

* ServiceURLRequestContext we've already determined runs in another process,
which apparently doesn't currently respect the flags.

This leaves remoting and Jingle, neither of which I'm terribly familiar with. 
Jingle is only used by remoting and sync.

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

http://codereview.chromium.org/10834215/diff/15001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:445:
command_line.GetSwitchValueASCII(switches::kHostRules));
nit:  Use braces when the body of an if statement takes up more than one line.

http://codereview.chromium.org/10834215/diff/15001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:488: session_params.host_mapping_rules =
globals_->host_mapping_rules.get();
nit:  This should go before ignore_certificate_errors, since it appears earlier
in both structures.  No need to resort the old code, but think it's best to be
consistent with new code.

http://codereview.chromium.org/10834215/diff/15001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:670: system_params.host_mapping_rules =
globals_->host_mapping_rules.get();
nit:  Move host_mapping_rules up, as above.

http://codereview.chromium.org/10834215/diff/15001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10834215/diff/15001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:251:
io_thread_globals->host_mapping_rules.get();
nit:  Move above ignore_certificate_errors

http://codereview.chromium.org/10834215/diff/15001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

http://codereview.chromium.org/10834215/diff/15001/net/http/http_cache.cc#new...
net/http/http_cache.cc:253: new HttpNetworkSession(params))) {
nit:  You may be able to move this onto the same line as "new HttpNetworkLayer("

http://codereview.chromium.org/10834215/diff/15001/net/http/http_network_sess...
File net/http/http_network_session.h (right):

http://codereview.chromium.org/10834215/diff/15001/net/http/http_network_sess...
net/http/http_network_session.h:66: HostMappingRules* host_mapping_rules;
nit:  Please put the HostMappingRules up with the other pointers.

http://codereview.chromium.org/10834215/diff/15001/net/http/http_stream_facto...
File net/http/http_stream_factory_impl.cc (right):

http://codereview.chromium.org/10834215/diff/15001/net/http/http_stream_facto...
net/http/http_stream_factory_impl.cc:130: 
nit:  Remove extra empty line.

http://codereview.chromium.org/10834215/diff/15001/net/socket_stream/socket_s...
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/10834215/diff/15001/net/socket_stream/socket_s...
net/socket_stream/socket_stream.cc:1304: if (ssl_socket->IgnoreCertError(result,
LOAD_IGNORE_ALL_CERT_ERRORS)) {
Could you switch this to SSLClientSocket::IgnoreCertError(), so it's clear this
is calling a static function?  Makes it clearer it doesn't affect the underlying
socket.

http://codereview.chromium.org/10834215/diff/15001/net/url_request/url_reques...
File net/url_request/url_request_context_builder.cc (right):

http://codereview.chromium.org/10834215/diff/15001/net/url_request/url_reques...
net/url_request/url_request_context_builder.cc:201: 
Remove extra linebreaks.

http://codereview.chromium.org/10834215/diff/15001/webkit/tools/test_shell/te...
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/10834215/diff/15001/webkit/tools/test_shell/te...
webkit/tools/test_shell/test_shell_request_context.cc:113:
network_session_params.host_resolver = host_resolver();
I think that once should generally be enough.

[mmenke, 2012-08-23 18:12:05]

Oh, and sorry for not getting back to you earlier - a lot of reviews this week.

[szager1, 2012-08-30 22:32:05]

Sorry this is lagging so much; stuff keeps coming up.

Scratched all nits, and applied network settings to jingle (which should cover
remoting as well).

[mmenke, 2012-08-31 16:22:04]

On 2012/08/30 22:32:05, szager1 wrote:
> Sorry this is lagging so much; stuff keeps coming up.
> 
> Scratched all nits, and applied network settings to jingle (which should cover
> remoting as well).

No problem, I've been pretty busy with reviews this week, myself.

LGTM.

[mmenke, 2012-08-31 16:26:32]

Let's see...You'll need OWNERS reviews for the changes in chrome/browser (Except
profiles/), jingle, and webkit.

[szager1, 2012-08-31 16:57:45]

Added OWNERS:

akalin: jingle
jamesr: webkit
nico: chrome/*

Thanks,

Stefan

[Nico, 2012-08-31 17:12:19]

http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:450:
network_session_params.trusted_spdy_proxy = trusted_spdy_proxy;
This looks fairly similar to off_the_record_profile_io_data.cc. Should there be
a net::HttpNetworkSession::Params::FromContext(main_context) function that does
this field copying which you can use in both places?

[szager1, 2012-08-31 17:17:43]

http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:450:
network_session_params.trusted_spdy_proxy = trusted_spdy_proxy;
On 2012/08/31 17:12:19, Nico wrote:
> This looks fairly similar to off_the_record_profile_io_data.cc. Should there
be
> a net::HttpNetworkSession::Params::FromContext(main_context) function that
does
> this field copying which you can use in both places?

The type of main_context is ChromeURLRequestContext, which can't be used inside
net/.  Also, there are other similar passages of code in other places, but
unfortunately they all have minor differences in the way the Params are set.

Having said that, I could define something in the abstract ProfileIOData to do
this.

[mmenke, 2012-08-31 17:21:53]

http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:450:
network_session_params.trusted_spdy_proxy = trusted_spdy_proxy;
On 2012/08/31 17:17:43, szager1 wrote:
> On 2012/08/31 17:12:19, Nico wrote:
> > This looks fairly similar to off_the_record_profile_io_data.cc. Should there
> be
> > a net::HttpNetworkSession::Params::FromContext(main_context) function that
> does
> > this field copying which you can use in both places?
> 
> The type of main_context is ChromeURLRequestContext, which can't be used
inside
> net/.  Also, there are other similar passages of code in other places, but
> unfortunately they all have minor differences in the way the Params are set.
> 
> Having said that, I could define something in the abstract ProfileIOData to do
> this.

Aren't all these parameters also members of net::URLRequestContext?

[szager, 2012-08-31 17:39:39]

On Aug 31, 2012 10:21 AM, <mmenke@chromium.org> wrote:
>
>
>
http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
>
>
http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_impl_io_data.cc:450:
> network_session_params.trusted_spdy_proxy = trusted_spdy_proxy;
> On 2012/08/31 17:17:43, szager1 wrote:
>>
>> On 2012/08/31 17:12:19, Nico wrote:
>> > This looks fairly similar to off_the_record_profile_io_data.cc.
>
> Should there
>>
>> be
>> > a net::HttpNetworkSession::Params::FromContext(main_context)
>
> function that
>>
>> does
>> > this field copying which you can use in both places?
>
>
>> The type of main_context is ChromeURLRequestContext, which can't be
>
> used inside
>>
>> net/.  Also, there are other similar passages of code in other places,
>
> but
>>
>> unfortunately they all have minor differences in the way the Params
>
> are set.
>
>> Having said that, I could define something in the abstract
>
> ProfileIOData to do
>>
>> this.
>
>
> Aren't all these parameters also members of net::URLRequestContext?

Now that you mention it, yes, they are.  Would it be preferable to have a
best-effort method in HttpNetworkSession::Params, with overrides where
necessary?  I think there would be overrides for nearly all invocations.

>
> http://codereview.chromium.org/10834215/

[mmenke, 2012-08-31 17:51:52]

On 2012/08/31 17:39:39, szager wrote:
> On Aug 31, 2012 10:21 AM, <mailto:mmenke@chromium.org> wrote:
> >
> >
> >
>
http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
> > File chrome/browser/profiles/profile_impl_io_data.cc (right):
> >
> >
>
http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
> > chrome/browser/profiles/profile_impl_io_data.cc:450:
> > network_session_params.trusted_spdy_proxy = trusted_spdy_proxy;
> > On 2012/08/31 17:17:43, szager1 wrote:
> >>
> >> On 2012/08/31 17:12:19, Nico wrote:
> >> > This looks fairly similar to off_the_record_profile_io_data.cc.
> >
> > Should there
> >>
> >> be
> >> > a net::HttpNetworkSession::Params::FromContext(main_context)
> >
> > function that
> >>
> >> does
> >> > this field copying which you can use in both places?
> >
> >
> >> The type of main_context is ChromeURLRequestContext, which can't be
> >
> > used inside
> >>
> >> net/.  Also, there are other similar passages of code in other places,
> >
> > but
> >>
> >> unfortunately they all have minor differences in the way the Params
> >
> > are set.
> >
> >> Having said that, I could define something in the abstract
> >
> > ProfileIOData to do
> >>
> >> this.
> >
> >
> > Aren't all these parameters also members of net::URLRequestContext?
> 
> Now that you mention it, yes, they are.  Would it be preferable to have a
> best-effort method in HttpNetworkSession::Params, with overrides where
> necessary?  I think there would be overrides for nearly all invocations.

I think it's safer as-is.  If someone adds a new parameter, think it makes more
sense for them to have to figure out how it applies to each case, rather than
add it by default.  Ideally, everything would just "work", but given all the
variations, I'm more than a little worried about that assumption not being
false, and things only working most of the time.

> >
> > http://codereview.chromium.org/10834215/

[mmenke, 2012-08-31 17:53:13]

On 2012/08/31 17:51:52, Matt Menke wrote:
> On 2012/08/31 17:39:39, szager wrote:
> > On Aug 31, 2012 10:21 AM, <mailto:mmenke@chromium.org> wrote:
> > >
> > >
> > >
> >
>
http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
> > > File chrome/browser/profiles/profile_impl_io_data.cc (right):
> > >
> > >
> >
>
http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
> > > chrome/browser/profiles/profile_impl_io_data.cc:450:
> > > network_session_params.trusted_spdy_proxy = trusted_spdy_proxy;
> > > On 2012/08/31 17:17:43, szager1 wrote:
> > >>
> > >> On 2012/08/31 17:12:19, Nico wrote:
> > >> > This looks fairly similar to off_the_record_profile_io_data.cc.
> > >
> > > Should there
> > >>
> > >> be
> > >> > a net::HttpNetworkSession::Params::FromContext(main_context)
> > >
> > > function that
> > >>
> > >> does
> > >> > this field copying which you can use in both places?
> > >
> > >
> > >> The type of main_context is ChromeURLRequestContext, which can't be
> > >
> > > used inside
> > >>
> > >> net/.  Also, there are other similar passages of code in other places,
> > >
> > > but
> > >>
> > >> unfortunately they all have minor differences in the way the Params
> > >
> > > are set.
> > >
> > >> Having said that, I could define something in the abstract
> > >
> > > ProfileIOData to do
> > >>
> > >> this.
> > >
> > >
> > > Aren't all these parameters also members of net::URLRequestContext?
> > 
> > Now that you mention it, yes, they are.  Would it be preferable to have a
> > best-effort method in HttpNetworkSession::Params, with overrides where
> > necessary?  I think there would be overrides for nearly all invocations.
> 
> I think it's safer as-is.  If someone adds a new parameter, think it makes
more
> sense for them to have to figure out how it applies to each case, rather than
> add it by default.  Ideally, everything would just "work", but given all the
> variations, I'm more than a little worried about that assumption not being
> false, and things only working most of the time.

But I am fine with either adding a ChromeURLRequestContext function to be used
to create the Profile's SessionParams, or a Profile function to do so.

> > >
> > > http://codereview.chromium.org/10834215/

[willchan no longer on Chromium, 2012-08-31 17:56:47]

I'd prefer to keep stuff out of Profile. ProfileIOData is fine though. Or
ChromeURLRequestContext.


On Fri, Aug 31, 2012 at 10:53 AM, <mmenke@chromium.org> wrote:

> On 2012/08/31 17:51:52, Matt Menke wrote:
>
>> On 2012/08/31 17:39:39, szager wrote:
>> > On Aug 31, 2012 10:21 AM, <mailto:mmenke@chromium.org> wrote:
>> > >
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.**org/10834215/diff/25001/**
>
chrome/browser/profiles/**profile_impl_io_data.cc<http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/profile_impl_io_data.cc>
>
>> > > File chrome/browser/profiles/**profile_impl_io_data.cc (right):
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.**org/10834215/diff/25001/**
>
chrome/browser/profiles/**profile_impl_io_data.cc#**newcode450<http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/profile_impl_io_data.cc#newcode450>
>
>> > > chrome/browser/profiles/**profile_impl_io_data.cc:450:
>> > > network_session_params.**trusted_spdy_proxy = trusted_spdy_proxy;
>> > > On 2012/08/31 17:17:43, szager1 wrote:
>> > >>
>> > >> On 2012/08/31 17:12:19, Nico wrote:
>> > >> > This looks fairly similar to off_the_record_profile_io_**data.cc.
>> > >
>> > > Should there
>> > >>
>> > >> be
>> > >> > a net::HttpNetworkSession::**Params::FromContext(main_**context)
>> > >
>> > > function that
>> > >>
>> > >> does
>> > >> > this field copying which you can use in both places?
>> > >
>> > >
>> > >> The type of main_context is ChromeURLRequestContext, which can't be
>> > >
>> > > used inside
>> > >>
>> > >> net/.  Also, there are other similar passages of code in other
>> places,
>> > >
>> > > but
>> > >>
>> > >> unfortunately they all have minor differences in the way the Params
>> > >
>> > > are set.
>> > >
>> > >> Having said that, I could define something in the abstract
>> > >
>> > > ProfileIOData to do
>> > >>
>> > >> this.
>> > >
>> > >
>> > > Aren't all these parameters also members of net::URLRequestContext?
>> >
>> > Now that you mention it, yes, they are.  Would it be preferable to have
>> a
>> > best-effort method in HttpNetworkSession::Params, with overrides where
>> > necessary?  I think there would be overrides for nearly all invocations.
>>
>
>  I think it's safer as-is.  If someone adds a new parameter, think it makes
>>
> more
>
>> sense for them to have to figure out how it applies to each case, rather
>> than
>> add it by default.  Ideally, everything would just "work", but given all
>> the
>> variations, I'm more than a little worried about that assumption not being
>> false, and things only working most of the time.
>>
>
> But I am fine with either adding a ChromeURLRequestContext function to be
> used
> to create the Profile's SessionParams, or a Profile function to do so.
>
>  > >
>> > >
http://codereview.chromium.**org/10834215/<http://codereview.chromium.org/108...
>>
>
>
>
>
http://codereview.chromium.**org/10834215/<http://codereview.chromium.org/108...
>

[mmenke, 2012-08-31 17:57:57]

On 2012/08/31 17:56:47, willchan wrote:
> I'd prefer to keep stuff out of Profile. ProfileIOData is fine though. Or
> ChromeURLRequestContext.

Apologies, I was thinking ProfileIOData.  Name just slipped my mind.

[akalin, 2012-08-31 19:44:48]

http://codereview.chromium.org/10834215/diff/25001/jingle/glue/proxy_resolvin...
File jingle/glue/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/10834215/diff/25001/jingle/glue/proxy_resolvin...
jingle/glue/proxy_resolving_client_socket.cc:67: net::HttpTransactionFactory*
transaction_factory =
This seems to violate the principle of least knowledge, since this code has to
reach into the guts of the request context to build session params.  Would it be
possible for RequestContext to expose a GetReferenceParams() method or
something?

Ideally, this code would look like:

SessionParams session_params = context->GetReferenceParams();
session_params.socket_factory = socket_factory;
// Any other overrides here...
network_session_ = new net::HttpNetworkSession(session_params);

If that's too much to do for this CL, perhaps a TODO for the above?

[szager1, 2012-08-31 19:59:15]

http://codereview.chromium.org/10834215/diff/25001/jingle/glue/proxy_resolvin...
File jingle/glue/proxy_resolving_client_socket.cc (right):

http://codereview.chromium.org/10834215/diff/25001/jingle/glue/proxy_resolvin...
jingle/glue/proxy_resolving_client_socket.cc:67: net::HttpTransactionFactory*
transaction_factory =
On 2012/08/31 19:44:48, akalin wrote:
> This seems to violate the principle of least knowledge, since this code has to
> reach into the guts of the request context to build session params.  Would it
be
> possible for RequestContext to expose a GetReferenceParams() method or
> something?
> 
> Ideally, this code would look like:
> 
> SessionParams session_params = context->GetReferenceParams();
> session_params.socket_factory = socket_factory;
> // Any other overrides here...
> network_session_ = new net::HttpNetworkSession(session_params);
> 
> If that's too much to do for this CL, perhaps a TODO for the above?

That's kind of at odds with comment #15.  Maybe something like this:

net::HttpNetworkSession::Params reference_params;
// GetReferenceParams returns a bool, true iff it was able
// to populate the argument Params.
if (request_context->GetReferenceParams(reference_params)) {
  session_params.host_mapping_rules = reference_params.host_mapping_rules;
  ...
}

WDYT?

[akalin, 2012-08-31 20:18:05]

> That's kind of at odds with comment #15.  Maybe something like this:
> 
> net::HttpNetworkSession::Params reference_params;
> // GetReferenceParams returns a bool, true iff it was able
> // to populate the argument Params.
> if (request_context->GetReferenceParams(reference_params)) {
>   session_params.host_mapping_rules = reference_params.host_mapping_rules;
>   ...
> }
> 
> WDYT?

Ah, I see.  Yeah, the above would work for me!

[szager1, 2012-08-31 22:58:04]

On 2012/08/31 20:18:05, akalin wrote:
> > That's kind of at odds with comment #15.  Maybe something like this:
> > 
> > net::HttpNetworkSession::Params reference_params;
> > // GetReferenceParams returns a bool, true iff it was able
> > // to populate the argument Params.
> > if (request_context->GetReferenceParams(reference_params)) {
> >   session_params.host_mapping_rules = reference_params.host_mapping_rules;
> >   ...
> > }
> > 
> > WDYT?
> 
> Ah, I see.  Yeah, the above would work for me!

Done.

[Nico, 2012-08-31 23:03:16]

http://codereview.chromium.org/10834215/diff/30003/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10834215/diff/30003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:657: net::HttpNetworkSession::Params&
params,
style guide says out params should be pointers

[szager1, 2012-08-31 23:05:19]

On 2012/08/31 17:12:19, Nico wrote:
>
http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
http://codereview.chromium.org/10834215/diff/25001/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_impl_io_data.cc:450:
> network_session_params.trusted_spdy_proxy = trusted_spdy_proxy;
> This looks fairly similar to off_the_record_profile_io_data.cc. Should there
be
> a net::HttpNetworkSession::Params::FromContext(main_context) function that
does
> this field copying which you can use in both places?

Based on subsequent discussion, I moved this code to
ProfileIOData::PopulateNetworkSessionParams().

[szager1, 2012-08-31 23:07:47]

http://codereview.chromium.org/10834215/diff/30003/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10834215/diff/30003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:657: net::HttpNetworkSession::Params&
params,
On 2012/08/31 23:03:16, Nico wrote:
> style guide says out params should be pointers

Done.

[akalin, 2012-09-01 00:28:18]

jingle lgtm

[mmenke, 2012-09-04 17:43:13]

http://codereview.chromium.org/10834215/diff/26028/net/socket_stream/socket_s...
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/10834215/diff/26028/net/socket_stream/socket_s...
net/socket_stream/socket_stream.cc:1305: HttpTransactionFactory* factory =
context_->http_transaction_factory();
GetReferenceParams?

http://codereview.chromium.org/10834215/diff/26028/net/url_request/url_reques...
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/10834215/diff/26028/net/url_request/url_reques...
net/url_request/url_request_context.h:58: bool
GetReferenceParams(HttpNetworkSession::Params* params) const;
Is a copy really necessary?  Seems like we could just return a const pointer.

http://codereview.chromium.org/10834215/diff/26028/net/url_request/url_reques...
net/url_request/url_request_context.h:58: bool
GetReferenceParams(HttpNetworkSession::Params* params) const;
Suggest calling this GetNetworkSessionParams, since that's what it actually
does, and it may be useful in more cases than for just copying parts of the
SessionParams.

[szager1, 2012-09-04 19:31:07]

http://codereview.chromium.org/10834215/diff/26028/net/socket_stream/socket_s...
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/10834215/diff/26028/net/socket_stream/socket_s...
net/socket_stream/socket_stream.cc:1305: HttpTransactionFactory* factory =
context_->http_transaction_factory();
On 2012/09/04 17:43:13, Matt Menke wrote:
> GetReferenceParams?

Done.

http://codereview.chromium.org/10834215/diff/26028/net/url_request/url_reques...
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/10834215/diff/26028/net/url_request/url_reques...
net/url_request/url_request_context.h:58: bool
GetReferenceParams(HttpNetworkSession::Params* params) const;
On 2012/09/04 17:43:13, Matt Menke wrote:
> Is a copy really necessary?  Seems like we could just return a const pointer.

The problem is that the URLRequestContext may not have an associated
HttpNetworkSession at the time this method is called (I have actually seen this
happen in the wild).  In that case, it would have to do something like return a
const reference to a static Params object containing default values.  Which
feels wrong to me; not least because the caller of GetReferenceParams can't tell
whether it actually received the default values, or values that are genuinely
associated with this context.

I could add DCHECK's to this method to ensure that this context has a non-NULL
HttpNetworkSession at the time GetReferenceParams is called, and then make sure
it's never called at the wrong time.  WDYT?

http://codereview.chromium.org/10834215/diff/26028/net/url_request/url_reques...
net/url_request/url_request_context.h:58: bool
GetReferenceParams(HttpNetworkSession::Params* params) const;
On 2012/09/04 17:43:13, Matt Menke wrote:
> Suggest calling this GetNetworkSessionParams, since that's what it actually
> does, and it may be useful in more cases than for just copying parts of the
> SessionParams.

Done.

[mmenke, 2012-09-04 19:35:40]

http://codereview.chromium.org/10834215/diff/26028/net/url_request/url_reques...
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/10834215/diff/26028/net/url_request/url_reques...
net/url_request/url_request_context.h:58: bool
GetReferenceParams(HttpNetworkSession::Params* params) const;
On 2012/09/04 19:31:07, szager1 wrote:
> On 2012/09/04 17:43:13, Matt Menke wrote:
> > Is a copy really necessary?  Seems like we could just return a const
pointer.
> 
> The problem is that the URLRequestContext may not have an associated
> HttpNetworkSession at the time this method is called (I have actually seen
this
> happen in the wild).  In that case, it would have to do something like return
a
> const reference to a static Params object containing default values.  Which
> feels wrong to me; not least because the caller of GetReferenceParams can't
tell
> whether it actually received the default values, or values that are genuinely
> associated with this context.
> 
> I could add DCHECK's to this method to ensure that this context has a non-NULL
> HttpNetworkSession at the time GetReferenceParams is called, and then make
sure
> it's never called at the wrong time.  WDYT?

I was thinking something more along the lines of:

const HttpNetworkSession::Params* GetReferenceParams() const;

And have it return NULL if no such params exist.

Returning a bool is nice in that it encourages checking for false, but it's not
strictly necessary.  Suppose you could do something like:

bool GetReferenceParams(HttpNetworkSession::Params* const* params) const;

But that's just weird, ugly, and I'm not event sure I got the syntax right.

[szager1, 2012-09-04 20:53:52]

On 2012/09/04 19:35:40, Matt Menke wrote:

> I was thinking something more along the lines of:
> 
> const HttpNetworkSession::Params* GetReferenceParams() const;

OK, this is done.  Please take another look.

[mmenke, 2012-09-04 20:58:54]

On 2012/09/04 20:53:52, szager1 wrote:
> On 2012/09/04 19:35:40, Matt Menke wrote:
> 
> > I was thinking something more along the lines of:
> > 
> > const HttpNetworkSession::Params* GetReferenceParams() const;
> 
> OK, this is done.  Please take another look.

Thanks.  LGTM!

[Nico, 2012-09-04 21:08:13]

http://codereview.chromium.org/10834215/diff/30003/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10834215/diff/30003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:657: net::HttpNetworkSession::Params&
params,
On 2012/08/31 23:07:48, szager1 wrote:
> On 2012/08/31 23:03:16, Nico wrote:
> > style guide says out params should be pointers
> 
> Done.

As far as I can tell, this is not done.

[mmenke, 2012-09-04 21:11:05]

http://codereview.chromium.org/10834215/diff/27044/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10834215/diff/27044/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:658: const ProfileParams*
profile_params) const {
Nico is right that params should be a pointer.  Also, profile_params should be
the first argument - input params should go before output params.

[szager1, 2012-09-04 22:45:26]

On 2012/09/04 21:08:13, Nico wrote:
>
http://codereview.chromium.org/10834215/diff/30003/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
http://codereview.chromium.org/10834215/diff/30003/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_io_data.cc:657:
net::HttpNetworkSession::Params&
> params,
> On 2012/08/31 23:07:48, szager1 wrote:
> > On 2012/08/31 23:03:16, Nico wrote:
> > > style guide says out params should be pointers
> > 
> > Done.
> 
> As far as I can tell, this is not done.

Whoops!  Done now (I promise).

[Nico, 2012-09-04 23:01:48]

chrome/ lgtm

[mmenke, 2012-09-05 17:34:01]

On 2012/09/04 23:01:48, Nico wrote:
> chrome/ lgtm

net/ LGTM

[jamesr, 2012-09-05 18:21:58]

webkit/ lgtm

[commit-bot: I haz the power, 2012-09-05 18:55:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szager@chromium.org/10834215/39002

[commit-bot: I haz the power, 2012-09-05 18:56:11]

Presubmit check for 10834215-39002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/shell

Presubmit checks took 2.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[szager1, 2012-09-05 19:13:42]

Adding jochen for content/shell.

[jochen (gone - plz use gerrit), 2012-09-05 19:16:23]

lgtm

[commit-bot: I haz the power, 2012-09-05 20:57:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szager@chromium.org/10834215/39002

[commit-bot: I haz the power, 2012-09-05 21:25:29]

Try job failure for 10834215-39002 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-09-07 18:11:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szager@chromium.org/10834215/31007

[commit-bot: I haz the power, 2012-09-07 18:26:36]

Try job failure for 10834215-31007 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-09-07 18:28:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szager@chromium.org/10834215/37008

[commit-bot: I haz the power, 2012-09-07 18:54:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szager@chromium.org/10834215/40006

[commit-bot: I haz the power, 2012-09-07 21:27:37]

Change committed as 155485