Refactor main URLRequestContext creation to use URLRequestContextBuilder

Refactor main URLRequestContext creation to use URLRequestContextBuilder

BUG=none

[wjmaclean, 2015-07-22 17:13:46]

wjmaclean@chromium.org changed reviewers:
+ mmenke@chromium.org

[wjmaclean, 2015-07-22 17:17:22]

mmenke@ - This is still pretty rough, but it represents a "first draft" of
trying to encapsulate main/otr-URLRequestContext creation into a function in
URLRequestContextBuilder.

Can you take a quick peek, and let me know if this looks like it's going in a
reasonable direction? Please watch out for TODO's, which act somewhat like
questions in the draft thus far.

Note: this draft does not attempt to break the dependency of shared elements in
the contexts ... presumably that will be done in follow-on CLs (in order to
better break the work down into managable chunks).

[mmenke, 2015-07-22 17:39:43]

On 2015/07/22 17:17:22, wjmaclean wrote:
> mmenke@ - This is still pretty rough, but it represents a "first draft" of
> trying to encapsulate main/otr-URLRequestContext creation into a function in
> URLRequestContextBuilder.
> 
> Can you take a quick peek, and let me know if this looks like it's going in a
> reasonable direction? Please watch out for TODO's, which act somewhat like
> questions in the draft thus far.
> 
> Note: this draft does not attempt to break the dependency of shared elements
in
> the contexts ... presumably that will be done in follow-on CLs (in order to
> better break the work down into manageable chunks).

Thanks for roping me in now!  This really does not seem to be going in the right
direction.  We definitely don't want to have two paths through the Builder, and
it shouldn't know about "main" contexts as opposed to other contexts.  It may be
simplest to just forego using the builder for now.

I can carefully dig through the code and give you a much more concrete proposal,
but I won't have time today, and I'm on bug triage the next two days, so it
would most likely be next week sometime before I get back to you.  Apologies for
slowness, but I expect this to take a fair bit of time.

[wjmaclean, 2015-07-22 17:46:52]

On 2015/07/22 17:39:43, mmenke wrote:
> On 2015/07/22 17:17:22, wjmaclean wrote:
> > mmenke@ - This is still pretty rough, but it represents a "first draft" of
> > trying to encapsulate main/otr-URLRequestContext creation into a function in
> > URLRequestContextBuilder.
> > 
> > Can you take a quick peek, and let me know if this looks like it's going in
a
> > reasonable direction? Please watch out for TODO's, which act somewhat like
> > questions in the draft thus far.
> > 
> > Note: this draft does not attempt to break the dependency of shared elements
> in
> > the contexts ... presumably that will be done in follow-on CLs (in order to
> > better break the work down into manageable chunks).
> 
> Thanks for roping me in now!  This really does not seem to be going in the
right
> direction.  We definitely don't want to have two paths through the Builder,
and
> it shouldn't know about "main" contexts as opposed to other contexts.  It may
be
> simplest to just forego using the builder for now.
> 
> I can carefully dig through the code and give you a much more concrete
proposal,
> but I won't have time today, and I'm on bug triage the next two days, so it
> would most likely be next week sometime before I get back to you.  Apologies
for
> slowness, but I expect this to take a fair bit of time.

Thanks for the feedback.

I should point out that I see the existence two paths through the builder as
*temporary* (we will ultimately merge back into one "Build()" function), and
ultimately the builder won't know about "main" vs. other types of contexts. I
guess I was hoping this intermediate step would help identify (1) what needs to
be added to the builder so it can handle all context creation tasks, and (2)
identify problematic code (e.g. circular dependencies).

It's possible I misunderstood what you thought we should be trying to accomplish
with the initial refactoring though, and I'd be interested in your feedback on
what alternate path might be better. No huge rush, I have loads on the go, and
can keep myself busy in the meantime.

[mmenke, 2015-07-22 18:22:36]

On 2015/07/22 17:46:52, wjmaclean wrote:
> On 2015/07/22 17:39:43, mmenke wrote:
> > On 2015/07/22 17:17:22, wjmaclean wrote:
> > > mmenke@ - This is still pretty rough, but it represents a "first draft" of
> > > trying to encapsulate main/otr-URLRequestContext creation into a function
in
> > > URLRequestContextBuilder.
> > > 
> > > Can you take a quick peek, and let me know if this looks like it's going
in
> a
> > > reasonable direction? Please watch out for TODO's, which act somewhat like
> > > questions in the draft thus far.
> > > 
> > > Note: this draft does not attempt to break the dependency of shared
elements
> > in
> > > the contexts ... presumably that will be done in follow-on CLs (in order
to
> > > better break the work down into manageable chunks).
> > 
> > Thanks for roping me in now!  This really does not seem to be going in the
> right
> > direction.  We definitely don't want to have two paths through the Builder,
> and
> > it shouldn't know about "main" contexts as opposed to other contexts.  It
may
> be
> > simplest to just forego using the builder for now.
> > 
> > I can carefully dig through the code and give you a much more concrete
> proposal,
> > but I won't have time today, and I'm on bug triage the next two days, so it
> > would most likely be next week sometime before I get back to you.  Apologies
> for
> > slowness, but I expect this to take a fair bit of time.
> 
> Thanks for the feedback.
> 
> I should point out that I see the existence two paths through the builder as
> *temporary* (we will ultimately merge back into one "Build()" function), and
> ultimately the builder won't know about "main" vs. other types of contexts. I
> guess I was hoping this intermediate step would help identify (1) what needs
to
> be added to the builder so it can handle all context creation tasks, and (2)
> identify problematic code (e.g. circular dependencies).
> 
> It's possible I misunderstood what you thought we should be trying to
accomplish
> with the initial refactoring though, and I'd be interested in your feedback on
> what alternate path might be better. No huge rush, I have loads on the go, and
> can keep myself busy in the meantime.

I don't think we want to have an intermediate (And actually landed) step where
we have two separate construction functions.  If we need intermediate steps, I
think it would make more sense to use the same build method, but add extra ways
to specify things.  Can get rid of the easy ones and figure out a way forward on
the harder things before landing.

Just skimming through through your list of pointers:

1)  NetLog needs to be shared.  This has come up before, and I think it's
reasonable taking a non-owning pointer.
2)  HttpNetworkTransactionFactory:  DevToolsNetworkTransactionFactory wants to
be between the cache and the network layer, which makes construction by the
builder problematic.  We'll also have to think about a lot of the
HttpNetworkSession::Params.
3)  ChromeHttpUserAgentSettings:  Reads from a pref and updates as it changes,
and the Builder currently just takes in a constant.  Could pass ownership of one
pretty easily, though that gives us two way to set the user agent a context
created by a builder uses.  Longer term, may want to make just one, but I think
that's fine for now.
4)  TransportSecurityStatePersister:  The builder only currently support an
in-memory TransportSecurityState, but it should probably have support for
persisting it to disk (Maybe when the cache is enable, or take a separate
parameter?).  First pass, though, we can just have the profile attach the
persister after context creation.
5)  HostResolver:  It may mean more DNS requests, but I think splitting up
HostResolvers is probably the way to go here.  May want histograms to measure
the effect.
6)  HttpServerProperties:  I think this can safely be passed with a scoped_ptr. 
(https://codereview.chromium.org/1175733002/ adds support for this to the
builder)
...  (Don't have time to go through them all now).

Anyhow, I'll spend more time digging through this as I have time.

[mmenke, 2015-09-11 17:05:43]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org