Add StoragePartition's ProtocolHandlers at URLRequestContext construction time.

Add StoragePartition's ProtocolHandlers at URLRequestContext construction time. 
Previously they were added later which doesn't mesh with pending 
URLRequestJobFactory API changes. 

BUG=146602, 161529
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181519

[pauljensen, 2012-12-05 19:55:23]

Hi Albert, I implemented the plan we came up with yesterday.  I had hoped to
pass the ProtocolHandlers directly into the URLRequestContextGetter constructor
in ProfileImplIOData::Handle::CreateMainRequestContextGetter() but...I didn't
realize main_request_context_getter_ gets constructed earlier in
ProfileImplIOData::Handle::Init() :(
This required adding the less than pretty
ProfileIOData::FinishInitializationOnUIThread...  Let me know if you think this
way is worth pursuing or if I should go back to my previous CL.

[pauljensen, 2012-12-07 18:48:03]

Albert, ping.

[awong, 2012-12-07 19:12:53]

sorry..looking now.

On Fri, Dec 7, 2012 at 10:48 AM, <pauljensen@chromium.org> wrote:

> Albert, ping.
>
>
https://codereview.chromium.**org/11308362/<https://codereview.chromium.org/1...
>

[awong, 2012-12-07 19:37:35]

eek...that sucks.  Okay...crazy idea...what happens i fyou
move io_data_->predictor_->InitNetworkPredictor() later in the
initialization chain?  If there's no MainRequestContext, I can't see how
anyone can make network requests...

BTW, AFAICT, bool HasMainRequestContext() const {} in ProfileImplIOData.h
is dead code.  So no one should even be able to access the main request
context without going through the normal accessor.

-Albert


On Fri, Dec 7, 2012 at 11:12 AM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> sorry..looking now.
>
> On Fri, Dec 7, 2012 at 10:48 AM, <pauljensen@chromium.org> wrote:
>
>> Albert, ping.
>>
>>
https://codereview.chromium.**org/11308362/<https://codereview.chromium.org/1...
>>
>
>

[pauljensen, 2012-12-10 21:56:31]

Albert, I moved io_data_->predictor_->InitNetworkPredictor() later in the
initialization chain.  Things are looking a lot cleaner now.  PTAL.

[awong, 2012-12-11 02:01:39]

Yay for cleaner!  Going to need to look at this in more depth tomorrow to
validate the ProfileImplIOData logic changes.

https://codereview.chromium.org/11308362/diff/23003/content/shell/shell_brows...
File content/shell/shell_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/23003/content/shell/shell_brows...
content/shell/shell_browser_context.h:66: class ShellResourceContext : public
ResourceContext {
WHy did we make ShellRequestContext into a private nested class?

[pauljensen, 2012-12-11 02:30:35]

https://codereview.chromium.org/11308362/diff/23003/content/shell/shell_brows...
File content/shell/shell_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/23003/content/shell/shell_brows...
content/shell/shell_browser_context.h:66: class ShellResourceContext : public
ResourceContext {
On 2012/12/11 02:01:39, awong wrote:
> WHy did we make ShellRequestContext into a private nested class?
I needed to make ShellResourceContext construction into a two-step process where
we set |getter_| after construction.  I only wanted ShellBrowserContext to be
able to set it so I wanted to friend ShellBrowserContext.  The style guide
recommends keeping friends close together so I moved it here.  Really I was just
copying exactly how ProfileIOData::ResouceContext is done (it already performed
this two-step construction).

[pauljensen, 2012-12-11 04:03:41]

https://codereview.chromium.org/11308362/diff/23003/content/shell/shell_brows...
File content/shell/shell_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/23003/content/shell/shell_brows...
content/shell/shell_browser_context.h:66: class ShellResourceContext : public
ResourceContext {
On 2012/12/11 02:30:35, pauljensen wrote:
> On 2012/12/11 02:01:39, awong wrote:
> > WHy did we make ShellRequestContext into a private nested class?
> I needed to make ShellResourceContext construction into a two-step process
where
> we set |getter_| after construction.  I only wanted ShellBrowserContext to be
> able to set it so I wanted to friend ShellBrowserContext.  The style guide
> recommends keeping friends close together so I moved it here.  Really I was
just
> copying exactly how ProfileIOData::ResouceContext is done (it already
performed
> this two-step construction).
Hmm, actually ShellResourceContext doesn't need to be in the header file since
its private anyhow.  I just moved it to the .cc file.

[awong, 2012-12-12 03:44:37]

Added some comments.  The method looks pretty solid to me. We'll want someone
like willchan@ to double-check this I think.

So if I'm understanding you correclty, the contract changes have become:

BrowserContext:
  - Gains CreateMainRequestContext() and
CreateRequestContextForStoragePartition() functions.
  - CreateXXX functions must be called before the GetXXX
    functions.

ProfileIOData::Handle:
  - Gains CreateMainRequestContextGetter() and
CreateIsolatedAppRequestContextGetter() functions
  - Create functions must be called before Get functions.

ProfileIOData:
  - GetMainRequestContext() has become an "Init" function?

The main thing concerning me is on the ProfileIOData "layer", I can't tell when
Get is performing a "create" function, and when it is just an accessor.

Is there a way to make that more consistent?

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/net/chrome...
File chrome/browser/net/chrome_url_request_context.cc (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/net/chrome...
chrome/browser/net/chrome_url_request_context.cc:43: explicit FactoryForMain(
remove explicit. Not needed anymore.

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:269: 
spurious new line.

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:138: DCHECK(iter !=
app_request_context_getter_map_.end());
CHECK or DCHECK?  If this happens in production, what will returning
iter->second do?

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:160: if (iter !=
app_request_context_getter_map_.end())
If we already have a separate GetXXXContextGetter(), Is there a way to ensure
this never happens so this function always creates?  Or does it need to support
the "create if not found" semantic?

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.h:129: PrefService* local_state_;
What's our guarantee on the lifetime of local_state_ and io_thread_? Can we
document?

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:89: ChromeURLRequestContext*
GetMainRequestContext(
Should this be Init()?

GetMainRequestContext() implies to me that this API could be called. However the
scoped_ptr<> arguments make the operation look like it's non-idempotent.

Or am I misunderstanding the new contract of this class?

https://codereview.chromium.org/11308362/diff/30001/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/30001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:52:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
I wonder if having a static Create() method or something might be cleaner. 
Posting a task in a constructor crossed the boundary into "doing work in the
constructor" which is discouraged by style guide.

This particular use actually looks safe because you don't seem to reference
anything that is only partially initialized, or leak the "this" pointer out to
anything but it still scares me slightly.

https://codereview.chromium.org/11308362/diff/30001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:90: // IO thread and must be
called prior to webkit_blob::BlobProtocolHandler().
Wow this is complicated. I think the naming is part of the problem. Delegate and
Core are really generic.  Delegate seems to be the IO Thread projection of this
Handler.  Core seems to be another sort of Proxy.  Any chance the names could
reflect their roles and intended function a bit more?

https://codereview.chromium.org/11308362/diff/30001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:96: blob_storage_controller,
file_system_context,
indentation...should be 4 in from the least indent level (the 'w' in
webkit_blob)

[pauljensen, 2012-12-12 18:39:36]

On 2012/12/12 03:44:37, awong wrote:
> Added some comments.  The method looks pretty solid to me. We'll want someone
> like willchan@ to double-check this I think.
> 
> So if I'm understanding you correclty, the contract changes have become:
> 
> BrowserContext:
>   - Gains CreateMainRequestContext() and
> CreateRequestContextForStoragePartition() functions.
>   - CreateXXX functions must be called before the GetXXX
>     functions.
GetRequestContextForStoragePartition() is also eliminated (now dead).

> 
> ProfileIOData::Handle:
>   - Gains CreateMainRequestContextGetter() and
> CreateIsolatedAppRequestContextGetter() functions
>   - Create functions must be called before Get functions.
> 
> ProfileIOData:
>   - GetMainRequestContext() has become an "Init" function?
I've changed it to Init() and added back the original GetMainRequestContext().

> The main thing concerning me is on the ProfileIOData "layer", I can't tell
when
> Get is performing a "create" function, and when it is just an accessor.
> 
> Is there a way to make that more consistent?
Now that I've added Init() I think all the Get*RequestContext() functions are
idempotent so a caller need not worry if they are creating or simply giving back
a pointer.

I think a major problem is the comment above the Get*RequestContext() functions:
  // These should only be called at most once each. Ownership is reversed when
  // they get called, from ProfileIOData owning ChromeURLRequestContext to vice
  // versa.
I think every sentence is the opposite of the truth.  I've removed it.

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/net/chrome...
File chrome/browser/net/chrome_url_request_context.cc (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/net/chrome...
chrome/browser/net/chrome_url_request_context.cc:43: explicit FactoryForMain(
On 2012/12/12 03:44:37, awong wrote:
> remove explicit. Not needed anymore.

Done.

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:269: 
On 2012/12/12 03:44:37, awong wrote:
> spurious new line.

Done.

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:138: DCHECK(iter !=
app_request_context_getter_map_.end());
On 2012/12/12 03:44:37, awong wrote:
> CHECK or DCHECK?  If this happens in production, what will returning
> iter->second do?
Changed to CHECK.

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:160: if (iter !=
app_request_context_getter_map_.end())
On 2012/12/12 03:44:37, awong wrote:
> If we already have a separate GetXXXContextGetter(), Is there a way to ensure
> this never happens so this function always creates?  Or does it need to
support
> the "create if not found" semantic?
Changed to DCHECK its not already present.

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.h:129: PrefService* local_state_;
On 2012/12/12 03:44:37, awong wrote:
> What's our guarantee on the lifetime of local_state_ and io_thread_? Can we
> document?
I removed these and just made them arguments of CreateMainRequestContext()
rather than Init().

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11308362/diff/30001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:89: ChromeURLRequestContext*
GetMainRequestContext(
On 2012/12/12 03:44:37, awong wrote:
> Should this be Init()?
> 
> GetMainRequestContext() implies to me that this API could be called. However
the
> scoped_ptr<> arguments make the operation look like it's non-idempotent.
> 
> Or am I misunderstanding the new contract of this class?
I agree it should be Init().  Done.

https://codereview.chromium.org/11308362/diff/30001/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/30001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:52:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
On 2012/12/12 03:44:37, awong wrote:
> I wonder if having a static Create() method or something might be cleaner. 
> Posting a task in a constructor crossed the boundary into "doing work in the
> constructor" which is discouraged by style guide.
> 
> This particular use actually looks safe because you don't seem to reference
> anything that is only partially initialized, or leak the "this" pointer out to
> anything but it still scares me slightly.

Done.

https://codereview.chromium.org/11308362/diff/30001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:90: // IO thread and must be
called prior to webkit_blob::BlobProtocolHandler().
On 2012/12/12 03:44:37, awong wrote:
> Wow this is complicated. I think the naming is part of the problem. Delegate
and
> Core are really generic.  Delegate seems to be the IO Thread projection of
this
> Handler.  Core seems to be another sort of Proxy.  Any chance the names could
> reflect their roles and intended function a bit more?

Done.

https://codereview.chromium.org/11308362/diff/30001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:96: blob_storage_controller,
file_system_context,
On 2012/12/12 03:44:37, awong wrote:
> indentation...should be 4 in from the least indent level (the 'w' in
> webkit_blob)

Done.

[pauljensen, 2012-12-12 18:52:36]

Will, Albert has been helping review this CL but thought you might be a good
person to double-check the major interface changes.  Would you be willing or is
there someone you'd like to delegate to?

[awong, 2012-12-12 19:01:15]

Will: I'm doing another pass right now, but the main thing that could use an
early smell test is the init logic change:

In particular, we're making Init() non-lazy.  Rationale to verify: getting the
main request context occurs early enough that it'd never be "lazy" anyways.


On 2012/12/12 18:52:36, pauljensen wrote:
> Will, Albert has been helping review this CL but thought you might be a good
> person to double-check the major interface changes.  Would you be willing or
is
> there someone you'd like to delegate to?

[awong, 2012-12-12 19:01:38]

Oh, and feel free to punt to whomever you think is appropriate.

On 2012/12/12 19:01:15, awong wrote:
> Will: I'm doing another pass right now, but the main thing that could use an
> early smell test is the init logic change:
> 
> In particular, we're making Init() non-lazy.  Rationale to verify: getting the
> main request context occurs early enough that it'd never be "lazy" anyways.
> 
> 
> On 2012/12/12 18:52:36, pauljensen wrote:
> > Will, Albert has been helping review this CL but thought you might be a good
> > person to double-check the major interface changes.  Would you be willing or
> is
> > there someone you'd like to delegate to?

[willchan no longer on Chromium, 2012-12-12 19:08:19]

I plan to take a look but let me add Matt first. I'll doublecheck just to make
sure nothing crazy is happening, but I'll let y'all handle the details.

[willchan no longer on Chromium, 2012-12-12 19:25:22]

Making init non-lazy should go into the changelist description. That's a big
change.

You should note that the reason it's done lazily is for startup performance.
Initializing many of the items in ProfileIOData will cause disk I/O, and doing
that at the wrong time heavily impacts startup performance.

I don't mind making it explicit, as I like explicit orderings as I find them
easier to understand. But I fear that you may land this and find it regresses
startup performance and be stuck. Good luck sirs.

[pauljensen, 2012-12-12 19:47:28]

We're not changing the initialization order so there shouldn't be any
performance ramifications.  We're simply making the initialization explicit
rather than implicit.  Init() will be explicitly called exactly where
LazyInitialize() is now called.

[willchan no longer on Chromium, 2012-12-12 21:29:31]

OIC, it's *still* lazily initialized on the first access to
FactoryForMain::Create(). But the laziness does not percolate into
ProfileIOData. We're making the assumption that it's always the "main"
URLRequestContext that is requested from Profile, rather than the extension one
or the storage related ones. Is this guaranteed to be true? Or do we need a lazy
initialization in the Profile level?

[pauljensen, 2012-12-12 21:58:47]

On 2012/12/12 21:29:31, willchan wrote:
> OIC, it's *still* lazily initialized on the first access to
> FactoryForMain::Create(). But the laziness does not percolate into
> ProfileIOData. We're making the assumption that it's always the "main"
> URLRequestContext that is requested from Profile, rather than the extension
one
> or the storage related ones. Is this guaranteed to be true? Or do we need a
lazy
> initialization in the Profile level?

We verify via CHECK() that Init() is called prior to any URLRequestContext being
returned from ProfileIOData.  Init() is called by
ProfileIOData::InitializeOnUIThread() at the earliest possible time:
The earliest time a URLRequestContext can be created (by the ProfileIOData) is
right after profile_params_ is setup in ProfileIOData::InitializeOnUIThread(). 
A few lines after profile_params_ is setup,
ProfileIOData::InitializeOnUIThread() calls 
BrowserContext::EnsureResourceContextInitialized() which creates the default
StoragePartition.  In doing so, it posts
ChromeAppCacheService::InitializeOnIOThread to the IO thread which right off the
bat requests the URLRequestContext (which calls Init() ).
Its a somewhat obfuscated call-stack...but by happening at the earliest possible
time it guarantees nobody else has a chance to request a URLRequestContext
without calling Init().

[willchan no longer on Chromium, 2012-12-12 22:06:43]

On Wed, Dec 12, 2012 at 1:58 PM, <pauljensen@chromium.org> wrote:

> On 2012/12/12 21:29:31, willchan wrote:
>
>> OIC, it's *still* lazily initialized on the first access to
>> FactoryForMain::Create(). But the laziness does not percolate into
>> ProfileIOData. We're making the assumption that it's always the "main"
>> URLRequestContext that is requested from Profile, rather than the
>> extension
>>
> one
>
>> or the storage related ones. Is this guaranteed to be true? Or do we need
>> a
>>
> lazy
>
>> initialization in the Profile level?
>>
>
> We verify via CHECK() that Init() is called prior to any URLRequestContext
> being
> returned from ProfileIOData.  Init() is called by
> ProfileIOData::**InitializeOnUIThread() at the earliest possible time:
> The earliest time a URLRequestContext can be created (by the
> ProfileIOData) is
> right after profile_params_ is setup in ProfileIOData::**
> InitializeOnUIThread().
> A few lines after profile_params_ is setup,
> ProfileIOData::**InitializeOnUIThread() calls
> BrowserContext::**EnsureResourceContextInitializ**ed() which creates the
> default
> StoragePartition.  In doing so, it posts
> ChromeAppCacheService::**InitializeOnIOThread to the IO thread which
> right off the
> bat requests the URLRequestContext (which calls Init() ).
> Its a somewhat obfuscated call-stack...but by happening at the earliest
> possible
> time it guarantees nobody else has a chance to request a URLRequestContext
> without calling Init().
>

Unless something else changed, this doesn't make sense.
ProfileIOData::InitializeOnUIThread() is, as it's named, called on the UI
thread. URLRequestContexts are only created and initialized on the IO
thread. I believe there are a number of DCHECKs to make sure that network
objects are created and destroyed on the same thread, so I'd be somewhat
surprised if this changed. What am I missing?


>
>
https://codereview.chromium.**org/11308362/<https://codereview.chromium.org/1...
>

[pauljensen, 2012-12-12 22:30:04]

On 2012/12/12 22:06:43, willchan wrote:
> On Wed, Dec 12, 2012 at 1:58 PM, <mailto:pauljensen@chromium.org> wrote:
> 
> > On 2012/12/12 21:29:31, willchan wrote:
> >
> >> OIC, it's *still* lazily initialized on the first access to
> >> FactoryForMain::Create(). But the laziness does not percolate into
> >> ProfileIOData. We're making the assumption that it's always the "main"
> >> URLRequestContext that is requested from Profile, rather than the
> >> extension
> >>
> > one
> >
> >> or the storage related ones. Is this guaranteed to be true? Or do we need
> >> a
> >>
> > lazy
> >
> >> initialization in the Profile level?
> >>
> >
> > We verify via CHECK() that Init() is called prior to any URLRequestContext
> > being
> > returned from ProfileIOData.  Init() is called by
> > ProfileIOData::**InitializeOnUIThread() at the earliest possible time:
> > The earliest time a URLRequestContext can be created (by the
> > ProfileIOData) is
> > right after profile_params_ is setup in ProfileIOData::**
> > InitializeOnUIThread().
> > A few lines after profile_params_ is setup,
> > ProfileIOData::**InitializeOnUIThread() calls
> > BrowserContext::**EnsureResourceContextInitializ**ed() which creates the
> > default
> > StoragePartition.  In doing so, it posts
> > ChromeAppCacheService::**InitializeOnIOThread to the IO thread which
> > right off the
> > bat requests the URLRequestContext (which calls Init() ).
> > Its a somewhat obfuscated call-stack...but by happening at the earliest
> > possible
> > time it guarantees nobody else has a chance to request a URLRequestContext
> > without calling Init().
> >
> 
> Unless something else changed, this doesn't make sense.
> ProfileIOData::InitializeOnUIThread() is, as it's named, called on the UI
> thread. URLRequestContexts are only created and initialized on the IO
> thread. I believe there are a number of DCHECKs to make sure that network
> objects are created and destroyed on the same thread, so I'd be somewhat
> surprised if this changed. What am I missing?
> 
> 
> >
> >
>
https://codereview.chromium.**org/11308362/%3Chttps://codereview.chromium.org...>
> >

Well Init() is not directly called from ProfileIOData::InitializeOnUIThread().
As I said, ChromeAppCacheService::InitializeOnIOThread is posted to the IO
thread which right off the bat requests the URLRequestContext (which calls
Init() ).

[willchan no longer on Chromium, 2012-12-12 22:38:15]

On Wed, Dec 12, 2012 at 2:30 PM, <pauljensen@chromium.org> wrote:

> On 2012/12/12 22:06:43, willchan wrote:
>
>  On Wed, Dec 12, 2012 at 1:58 PM,
<mailto:pauljensen@chromium.**org<pauljensen@chromium.org>>
>> wrote:
>>
>
>  > On 2012/12/12 21:29:31, willchan wrote:
>> >
>> >> OIC, it's *still* lazily initialized on the first access to
>> >> FactoryForMain::Create(). But the laziness does not percolate into
>> >> ProfileIOData. We're making the assumption that it's always the "main"
>> >> URLRequestContext that is requested from Profile, rather than the
>> >> extension
>> >>
>> > one
>> >
>> >> or the storage related ones. Is this guaranteed to be true? Or do we
>> need
>> >> a
>> >>
>> > lazy
>> >
>> >> initialization in the Profile level?
>> >>
>> >
>> > We verify via CHECK() that Init() is called prior to any
>> URLRequestContext
>> > being
>> > returned from ProfileIOData.  Init() is called by
>> > ProfileIOData::****InitializeOnUIThread() at the earliest possible
>> time:
>>
>> > The earliest time a URLRequestContext can be created (by the
>> > ProfileIOData) is
>> > right after profile_params_ is setup in ProfileIOData::**
>>
>> > InitializeOnUIThread().
>> > A few lines after profile_params_ is setup,
>> > ProfileIOData::****InitializeOnUIThread() calls
>> > BrowserContext::****EnsureResourceContextInitializ****ed() which
>> creates the
>>
>> > default
>> > StoragePartition.  In doing so, it posts
>> > ChromeAppCacheService::****InitializeOnIOThread to the IO thread which
>>
>> > right off the
>> > bat requests the URLRequestContext (which calls Init() ).
>> > Its a somewhat obfuscated call-stack...but by happening at the earliest
>> > possible
>> > time it guarantees nobody else has a chance to request a
>> URLRequestContext
>> > without calling Init().
>> >
>>
>
>  Unless something else changed, this doesn't make sense.
>> ProfileIOData::**InitializeOnUIThread() is, as it's named, called on the
>> UI
>> thread. URLRequestContexts are only created and initialized on the IO
>> thread. I believe there are a number of DCHECKs to make sure that network
>> objects are created and destroyed on the same thread, so I'd be somewhat
>> surprised if this changed. What am I missing?
>>
>
>
>  >
>> >
>>
>
> https://codereview.chromium.****org/11308362/%3Chttps://codere**
> view.chromium.org/11308362/ <http://codereview.chromium.org/11308362/>>
>
>> >
>>
>
> Well Init() is not directly called from ProfileIOData::**
> InitializeOnUIThread().
> As I said, ChromeAppCacheService::**InitializeOnIOThread is posted to the
> IO
>
> thread which right off the bat requests the URLRequestContext (which calls
> Init() ).
>

My bad! I misunderstood your statement:
"""
The earliest time a URLRequestContext can be created (by the ProfileIOData)
is
right after profile_params_ is setup in ProfileIOData::**InitializeOnUIT
hread().
"""

I took it to meaning that the URLRequestContext was created in
ProfileIOData::InitializeOnUIThread().

I'm still concerned about this statement here:
"""
Init() is called by
ProfileIOData::**InitializeOnUIThread() at the earliest possible time
"""

That sounds to me like we are eagerly initializing ProfileIOData instead of
lazily initializing ProfileIOData. That does not change the order of calls
with which other stuff accesses ProfileIOData via getters, but it does
sound like it moves ProfileIOData initialization earlier, and thus may
introduce contention with other Chromium subsystems. Can you clarify why
this doesn't happen?


>
>
https://codereview.chromium.**org/11308362/<https://codereview.chromium.org/1...
>

[pauljensen, 2012-12-12 22:55:16]

I'm sorry if I'm not being clear.  Let me try and rephrase.  

With or without my CL, during ProfileIOData::InitializeOnUIThread() we post to
the IO thread a task that immediately requests the main URLRequestContext
thereby triggering initialization of ProfileIOData. I can't say if this is cause
for concern, I can say this is how it currently works.

Without my CL the first call to FactoryForMain::Create() calls
ProfileIOData::GetMainRequestContext() which calls
ProfileIOData::LazyInitialize().  With my CL I'm changing
FactoryForMain::Create() to call ProfileIOData::Init() and then
ProfileIOData::GetMainRequestContext().

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

Aha, so if there was a regression, it must have already been introduced and
you're not affecting any of that. Fascinating. Well, we haven't detected
it, so hopefully it isn't a big deal. Carry on. When I get time I'll go
crawl through out perf graphs to see if I can find any issue here, but
let's operate as if there isn't. Thanks for the explanation.


On Wed, Dec 12, 2012 at 2:55 PM, <pauljensen@chromium.org> wrote:

> I'm sorry if I'm not being clear.  Let me try and rephrase.
>
> With or without my CL, during ProfileIOData::**InitializeOnUIThread() we
> post to
> the IO thread a task that immediately requests the main URLRequestContext
> thereby triggering initialization of ProfileIOData. I can't say if this is
> cause
> for concern, I can say this is how it currently works.
>
> Without my CL the first call to FactoryForMain::Create() calls
> ProfileIOData::**GetMainRequestContext() which calls
> ProfileIOData::LazyInitialize(**).  With my CL I'm changing
> FactoryForMain::Create() to call ProfileIOData::Init() and then
> ProfileIOData::**GetMainRequestContext().
>
>
https://codereview.chromium.**org/11308362/<https://codereview.chromium.org/1...
>

[awong, 2012-12-13 01:06:15]

Hi Paul,

Sorry for the delay.  Had a hard time detangling the ordering. Good job working
all that out!

To summarize, I think I've verified these invariants:
  (1) ProfileIOData::Handle is still lazily initialized. No effect on
      ordering.
  (2) All ProfileIOData::Handle accessors (other than CreateXXX functions) 
      are still guarded by ProfileIOData::Handle::LazyInitialize().
      This means OnPrefsLoaded() being delayed doesn't matter.
  (3) ProfileIOData::LazyInitialize was useless because the Handle's
LazyInitialize() already served as the guard. The previous path was 
      ProfileIOData::Handle::LazyInitialize -> 
      ProfileIOData::InitializeOnUIThread() -> 
      BrowserContext::EnsureResourceContext() ->
      BrowserContext::GetDefaultStoragePartition() ->
      StoragePartitionImplMap::Get() {
        getter = [via BrowserContext] Profile::GetRequestContext() ->      
           ProfileIOData::Handle::GetMainRequestContextGetter() ->
           ChromeURLRequestContextGetter::CreateOriginal()
        PostToIOThread(getter)
      }
      [IO Thread] FactoryForMain::Create()
      [IO Thread] ProfileIOData::GetMainRequestContext()
      [IO Thread] ProfileIOData::LazyInitialize()
  (4) Making ProfileIOData non-lazy init is safe because 
      ordering on on IO thread is preserved. We know because 
      ProfileIOData::Handle::LazyInitialize() triggers the sequence
      in (3) up to FactoryForMain::Create() before anyone on the
      io_thread can get access to io_data_.

In that case, I think this code is "correct."  Thus, the comments here are
mostly style.

The main thing I'd like to consider is moving the CreateXXX calls out of
BrowserContext* into ContentBrowserClient. It feels like a better fit because
they don't quite make sense in the BrowserContext API. Not sure that works
completely though since my head is kinda fried.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:508: predictor_,
Double-check: We are safe moving the
ProfileIOData::Handle::main_request_context_getter_ creation outside of
ProfileIOData::Handle::Init() because all of the Handle functions (other than
CreateXXX) are guarded by LazyInit.

Thus, even in the case where this DoFinalInit()'s call is delayed because
OnPrefsLoaded() is being executed asynchronously (see
ProfileImpl::ProfileImpl()) we are safe?

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:114: LazyInitialize();
So the handle still initializes lazily...but the real ProfileIODate is
explicitly initialized now? Is that correct?

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:253:
ChromeURLRequestContextGetterMap::iterator app_iter =
const_iterator

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.h:35: bool HasMainRequestContext()
const {
I think this function isn't used.  Can we remove?

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.h:63:
CreateMainRequestContextGetter(
Can we group these Create functions separately from the GetXXX functions, and
then stuff a block comment above saying they are only exposed because the
circular relation between Profile, ProfileIOData, and the
ChromeURLRequestContextGetter factories requires Profile be able to call this
function.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:176: // later delivery to the job
factory in LazyInitialize.
Is LazyInitialize() still the right name here?

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:368: CHECK(initialized_);
These should all either DCHECK(initialized_) or CHECK(initialized_). Not sure
why we have both pattersn here.

For a guard like this, I think DCHECK is good enough.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:83: // The basic logic is implemented
here. The specific initialization
Comments on lines 83 to 85 should be moved into the implementation.

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:81: class IOThreadDelegate {
Names are much nicer. Thanks!

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:84: virtual ~IOThreadDelegate() {}
Why does this need to be virtual?

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:110: //
webkit_blob::BlobProtocolHandler implementation
End comment with a period please.

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:131: scoped_ptr<IOThreadDelegate>
io_thread_delegate_;
I wouldn't use a scoped_ptr<> here because we don't rely on it to do the actual
deletion.

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:144: ChromeBlobStorageContext*
blob_storage_context)
Why did we change this to take the context rather than the controler?

https://codereview.chromium.org/11308362/diff/32004/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/11308362/diff/32004/content/public/browser/br...
content/public/browser/browser_context.h:100: virtual
net::URLRequestContextGetter* CreateRequestContext(
Can you add a commenting to the CreateXXX functions saying this should only be
called once per BrowserContext object and then add a TODO(ajwong) to remove
these once we've resolved http://159193?

Also, I wonder if these would be better put in ContentBrowserClient.

If you think about it, this is really a hole that allows the embedder to specify
how to create a particular type of URLRequestContext...

https://codereview.chromium.org/11308362/diff/32004/content/public/browser/br...
content/public/browser/browser_context.h:115: virtual
net::URLRequestContextGetter* CreateRequestContextForStoragePartition(
Same comment as previoiusly.

https://codereview.chromium.org/11308362/diff/32004/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/32004/content/shell/shell_brows...
content/shell/shell_browser_context.cc:30: class ShellResourceContext : public
ResourceContext {
Dunk this in an anonymous namespace to be safe.

[pauljensen, 2012-12-13 17:58:43]

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:508: predictor_,
On 2012/12/13 01:06:15, awong wrote:
> Double-check: We are safe moving the
> ProfileIOData::Handle::main_request_context_getter_ creation outside of
> ProfileIOData::Handle::Init() because all of the Handle functions (other than
> CreateXXX) are guarded by LazyInit.
Yes, and LazyInitialize() calls io_data_->InitializeOnUIThread() which creates
the default StoragePartition which triggers creation of the
main_request_context_getter_.

> 
> Thus, even in the case where this DoFinalInit()'s call is delayed because
> OnPrefsLoaded() is being executed asynchronously (see
> ProfileImpl::ProfileImpl()) we are safe?
There shouldn't be any way for callers of ProfileImpl functions that call
io_data_ to notice any difference because LazyInitialize() guarantees everything
is initialized.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:114: LazyInitialize();
On 2012/12/13 01:06:15, awong wrote:
> So the handle still initializes lazily...but the real ProfileIODate is
> explicitly initialized now? Is that correct?
Correct.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:253:
ChromeURLRequestContextGetterMap::iterator app_iter =
On 2012/12/13 01:06:15, awong wrote:
> const_iterator

Done.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.h:35: bool HasMainRequestContext()
const {
On 2012/12/13 01:06:15, awong wrote:
> I think this function isn't used.  Can we remove?

Done.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.h:63:
CreateMainRequestContextGetter(
On 2012/12/13 01:06:15, awong wrote:
> Can we group these Create functions separately from the GetXXX functions, and
> then stuff a block comment above saying they are only exposed because the
> circular relation between Profile, ProfileIOData, and the
> ChromeURLRequestContextGetter factories requires Profile be able to call this
> function.

Done.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:176: // later delivery to the job
factory in LazyInitialize.
On 2012/12/13 01:06:15, awong wrote:
> Is LazyInitialize() still the right name here?
I changed it to Init().

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:368: CHECK(initialized_);
On 2012/12/13 01:06:15, awong wrote:
> These should all either DCHECK(initialized_) or CHECK(initialized_). Not sure
> why we have both pattersn here.
> 
> For a guard like this, I think DCHECK is good enough.

Done.

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11308362/diff/32004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:83: // The basic logic is implemented
here. The specific initialization
On 2012/12/13 01:06:15, awong wrote:
> Comments on lines 83 to 85 should be moved into the implementation.

Done.

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:84: virtual ~IOThreadDelegate() {}
On 2012/12/13 01:06:15, awong wrote:
> Why does this need to be virtual?
I see style says it doesn't need to be virtual if there are no virtual methods. 
I don't think I need one at all then.  Deleted.

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:110: //
webkit_blob::BlobProtocolHandler implementation
On 2012/12/13 01:06:15, awong wrote:
> End comment with a period please.

Done.

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:131: scoped_ptr<IOThreadDelegate>
io_thread_delegate_;
On 2012/12/13 01:06:15, awong wrote:
> I wouldn't use a scoped_ptr<> here because we don't rely on it to do the
actual
> deletion.

Done.

https://codereview.chromium.org/11308362/diff/32004/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:144: ChromeBlobStorageContext*
blob_storage_context)
On 2012/12/13 01:06:15, awong wrote:
> Why did we change this to take the context rather than the controler?
I changed this to take a context because this is now constructed on the UI
thread but ChromeBlobStorageContext::controller() must be called on the IO
thread.

https://codereview.chromium.org/11308362/diff/32004/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/11308362/diff/32004/content/public/browser/br...
content/public/browser/browser_context.h:100: virtual
net::URLRequestContextGetter* CreateRequestContext(
On 2012/12/13 01:06:15, awong wrote:
> Can you add a commenting to the CreateXXX functions saying this should only be
> called once per BrowserContext object and then add a TODO(ajwong) to remove
> these once we've resolved http://159193?
> 
> Also, I wonder if these would be better put in ContentBrowserClient.
> 
> If you think about it, this is really a hole that allows the embedder to
specify
> how to create a particular type of URLRequestContext...

Done.

https://codereview.chromium.org/11308362/diff/32004/content/public/browser/br...
content/public/browser/browser_context.h:115: virtual
net::URLRequestContextGetter* CreateRequestContextForStoragePartition(
On 2012/12/13 01:06:15, awong wrote:
> Same comment as previoiusly.

Done.

https://codereview.chromium.org/11308362/diff/32004/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/32004/content/shell/shell_brows...
content/shell/shell_browser_context.cc:30: class ShellResourceContext : public
ResourceContext {
On 2012/12/13 01:06:15, awong wrote:
> Dunk this in an anonymous namespace to be safe.
I don't think I can do that because it's declared and referenced in
shell_browser_context.h.

[mmenke, 2012-12-13 18:59:13]

Sorry, don't think I'm going to get to this today, and I have an interview
tomorrow - I hope to get back to you late tomorrow afternoon.

[pauljensen, 2012-12-13 21:41:57]

Albert, PTAL.

[awong, 2012-12-13 23:53:08]

Few more style nits, but almost there.

Matt, Will, I think this is ready for a serious scrubdown.

Paul, you will also need a content owner.

https://codereview.chromium.org/11308362/diff/47002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/11308362/diff/47002/chrome/browser/profiles/p...
chrome/browser/profiles/profile.h:229: virtual net::URLRequestContextGetter*
CreateRequestContext(
Group this and the CreateRequestContextForStoragePartition() function below
together and move them out of the content::BrowserContext section.

Also add comments similar to those in ContentBrowserContext with the
TODO(ajwong): and with the information when/how they should be called.

https://codereview.chromium.org/11308362/diff/47002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11308362/diff/47002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:82: // generation. Must be called
prior to any Get*RequestContext() methods.
Really it must be called before any of the other functions correct?

If so, update the comment and change all the function guards to be
DCHECK(initialized_) rather than checking for specific variables (eg
extension_info_map_).

https://codereview.chromium.org/11308362/diff/47002/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11308362/diff/47002/content/public/browser/co...
content/public/browser/content_browser_client.h:150: // only be called once per
ContentBrowserClient object.
This isn't strictly true...it should only be called once per partition_path per
ContentBrowserClient object right?

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
content/shell/shell_browser_context.cc:31: class ShellResourceContext : public
ResourceContext {
I see what you mean by it being referenced.  In that case, it might actually be
better to just put it back into its own file.

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
File content/shell/shell_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
content/shell/shell_browser_context.h:49: net::URLRequestContextGetter*
CreateRequestContext(
virtual?

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
content/shell/shell_browser_context.h:56: net::URLRequestContextGetter*
CreateRequestContextForStoragePartition(
virtual?

[pauljensen, 2012-12-14 16:16:20]

Albert, PTAL.
Do you think jam@ would be a good content owner?  How thorough do you think I
need to ask him to review it all?  Maybe I should wait until will/mmenke has a
chance to thoroughly review.

https://codereview.chromium.org/11308362/diff/47002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/11308362/diff/47002/chrome/browser/profiles/p...
chrome/browser/profiles/profile.h:229: virtual net::URLRequestContextGetter*
CreateRequestContext(
On 2012/12/13 23:53:08, awong wrote:
> Group this and the CreateRequestContextForStoragePartition() function below
> together and move them out of the content::BrowserContext section.
> 
> Also add comments similar to those in ContentBrowserContext with the
> TODO(ajwong): and with the information when/how they should be called.

Done.

https://codereview.chromium.org/11308362/diff/47002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11308362/diff/47002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:82: // generation. Must be called
prior to any Get*RequestContext() methods.
On 2012/12/13 23:53:08, awong wrote:
> Really it must be called before any of the other functions correct?
> 
> If so, update the comment and change all the function guards to be
> DCHECK(initialized_) rather than checking for specific variables (eg
> extension_info_map_).
Well any of the Get*() methods other than GetResourceContext() or
GetMetricsEnabledStateOnIOThread().  I've updated the comment and DCHECKs.

https://codereview.chromium.org/11308362/diff/47002/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11308362/diff/47002/content/public/browser/co...
content/public/browser/content_browser_client.h:150: // only be called once per
ContentBrowserClient object.
On 2012/12/13 23:53:08, awong wrote:
> This isn't strictly true...it should only be called once per partition_path
per
> ContentBrowserClient object right?

Done.

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
content/shell/shell_browser_context.cc:31: class ShellResourceContext : public
ResourceContext {
On 2012/12/13 23:53:08, awong wrote:
> I see what you mean by it being referenced.  In that case, it might actually
be
> better to just put it back into its own file.
Well if I move it into its own file then I think I'm violating the style guide's
recommendation that friends be defined in the same file.  I've moved it back to
being a private sub-class of ShellBrowserContext but incompletely defined in the
header.  This way its out of the global namespace and its private so others
aren't tempted to use it and its not cluttering up the interface/header file.

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
File content/shell/shell_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
content/shell/shell_browser_context.h:49: net::URLRequestContextGetter*
CreateRequestContext(
On 2012/12/13 23:53:08, awong wrote:
> virtual?
Why?  Remember this is no longer an implementation of BrowserContext, it's just
something the ShellBrwoserContext implements that the ShellContentBrowserClient
implementation can call.

[awong, 2012-12-14 16:44:24]

LGTM

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
content/shell/shell_browser_context.cc:31: class ShellResourceContext : public
ResourceContext {
On 2012/12/14 16:16:20, pauljensen wrote:
> On 2012/12/13 23:53:08, awong wrote:
> > I see what you mean by it being referenced.  In that case, it might actually
> be
> > better to just put it back into its own file.
> Well if I move it into its own file then I think I'm violating the style
guide's
> recommendation that friends be defined in the same file.  I've moved it back
to
> being a private sub-class of ShellBrowserContext but incompletely defined in
the
> header.  This way its out of the global namespace and its private so others
> aren't tempted to use it and its not cluttering up the interface/header file.

I don't feel too strongly about having keeping friend classes in the same file,
but having it be a private class in this situation is indeed nicer.  Thanks for
thinking it through.

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
File content/shell/shell_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
content/shell/shell_browser_context.h:49: net::URLRequestContextGetter*
CreateRequestContext(
On 2012/12/14 16:16:20, pauljensen wrote:
> On 2012/12/13 23:53:08, awong wrote:
> > virtual?
> Why?  Remember this is no longer an implementation of BrowserContext, it's
just
> something the ShellBrwoserContext implements that the
ShellContentBrowserClient
> implementation can call.

Ah right. Sorry.  Forgot that

[awong, 2012-12-14 16:46:20]

And yes, I think jam@ would be a good content reviewer.  As for waiting for
mmenke@'s review first...I'm think it's your call.

On 2012/12/14 16:44:24, awong wrote:
> LGTM
> 
>
https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
> File content/shell/shell_browser_context.cc (right):
> 
>
https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
> content/shell/shell_browser_context.cc:31: class ShellResourceContext : public
> ResourceContext {
> On 2012/12/14 16:16:20, pauljensen wrote:
> > On 2012/12/13 23:53:08, awong wrote:
> > > I see what you mean by it being referenced.  In that case, it might
actually
> > be
> > > better to just put it back into its own file.
> > Well if I move it into its own file then I think I'm violating the style
> guide's
> > recommendation that friends be defined in the same file.  I've moved it back
> to
> > being a private sub-class of ShellBrowserContext but incompletely defined in
> the
> > header.  This way its out of the global namespace and its private so others
> > aren't tempted to use it and its not cluttering up the interface/header
file.
> 
> I don't feel too strongly about having keeping friend classes in the same
file,
> but having it be a private class in this situation is indeed nicer.  Thanks
for
> thinking it through.
> 
>
https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
> File content/shell/shell_browser_context.h (right):
> 
>
https://codereview.chromium.org/11308362/diff/47002/content/shell/shell_brows...
> content/shell/shell_browser_context.h:49: net::URLRequestContextGetter*
> CreateRequestContext(
> On 2012/12/14 16:16:20, pauljensen wrote:
> > On 2012/12/13 23:53:08, awong wrote:
> > > virtual?
> > Why?  Remember this is no longer an implementation of BrowserContext, it's
> just
> > something the ShellBrwoserContext implements that the
> ShellContentBrowserClient
> > implementation can call.
> 
> Ah right. Sorry.  Forgot that

[pauljensen, 2012-12-14 17:37:17]

GetExtensionInfoMap() is called during Init() which would hit
DCHECK(initialized_).  I changed it to simply read extension_info_map_ directly
so we can keep the public interface sane.

[willchan no longer on Chromium, 2012-12-14 18:55:21]

I'm planning on letting Matt do the review :)

On Fri, Dec 14, 2012 at 9:37 AM,  <pauljensen@chromium.org> wrote:
> GetExtensionInfoMap() is called during Init() which would hit
> DCHECK(initialized_).  I changed it to simply read extension_info_map_
> directly
> so we can keep the public interface sane.
>
> https://codereview.chromium.org/11308362/

[mmenke, 2012-12-14 20:20:24]

This messes with enough files in a dependent way that I'm not going to be able
to do a full review this afternoon.  Sorry about that, hate to be so slow.

On 2012/12/14 18:55:21, willchan wrote:
> I'm planning on letting Matt do the review :)
> 
> On Fri, Dec 14, 2012 at 9:37 AM,  <mailto:pauljensen@chromium.org> wrote:
> > GetExtensionInfoMap() is called during Init() which would hit
> > DCHECK(initialized_).  I changed it to simply read extension_info_map_
> > directly
> > so we can keep the public interface sane.
> >
> > https://codereview.chromium.org/11308362/

[pauljensen, 2012-12-19 15:01:41]

Matt, just checking that you haven't forgot about this CL.

On 2012/12/14 20:20:24, Matt Menke wrote:
> This messes with enough files in a dependent way that I'm not going to be able
> to do a full review this afternoon.  Sorry about that, hate to be so slow.
> 
> On 2012/12/14 18:55:21, willchan wrote:
> > I'm planning on letting Matt do the review :)
> > 
> > On Fri, Dec 14, 2012 at 9:37 AM,  <mailto:pauljensen@chromium.org> wrote:
> > > GetExtensionInfoMap() is called during Init() which would hit
> > > DCHECK(initialized_).  I changed it to simply read extension_info_map_
> > > directly
> > > so we can keep the public interface sane.
> > >
> > > https://codereview.chromium.org/11308362/

[mmenke, 2012-12-19 15:20:27]

On 2012/12/19 15:01:41, pauljensen wrote:
> Matt, just checking that you haven't forgot about this CL.

I haven't - just got a bunch of other really big CLs submitted for review over
the weekend that I've been giving higher priority to, since I believe none of
your CLs in the immediate future (CLs that you may want to have reviewed this
week) depends on this CL.

[pauljensen, 2012-12-19 16:22:08]

No worries, just checking it hadn't fallen off your radar.

On 2012/12/19 15:20:27, Matt Menke wrote:
> On 2012/12/19 15:01:41, pauljensen wrote:
> > Matt, just checking that you haven't forgot about this CL.
> 
> I haven't - just got a bunch of other really big CLs submitted for review over
> the weekend that I've been giving higher priority to, since I believe none of
> your CLs in the immediate future (CLs that you may want to have reviewed this
> week) depends on this CL.

[mmenke, 2012-12-20 16:55:06]

Still need to spend some time digging through this, but I think the whole
LazyInitialization doesn't initialize everything property of
OffTheRecordProfileIOData makes it difficult to reason about when
GetMainRequestContextGetter can be safely called.

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:258:
net::URLRequestContextGetter* OffTheRecordProfileImpl::CreateRequestContext(
Definition order should match the header (Same goes for
CreateRequestContextForStoragePartition)

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.h:61:
developer_protocol_handler) OVERRIDE;
Declaration order should match profile.h

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.h:106:
developer_protocol_handler) OVERRIDE;
This is not part of the BrowserContext.  Should go up in the Profile section,
same order as in profile.h.

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:86: LazyInitialize();
This call no longer makes any sense.

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:154: LazyInitialize();
This LazyInitialize doesn't work - looks like the
ChromeURLRequestContextGetter::CreateOffTheRecordForIsolatedApp gets the main
request context from the profile, so main_request_context_getter_ must already
be initialized, which implies LazyInitialize must already have been called.

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:853:
ProfileImpl::CreateRequestContextForStoragePartition(
Definition order should match the header (Same goes for
CreateRequestContextForStoragePartition)

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.h:86: developer_protocol_handler) OVERRIDE;
Both these functions should be in the Profile implementation section, in the
same order they are in profile.h.

https://codereview.chromium.org/11308362/diff/53005/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/53005/content/shell/shell_brows...
content/shell/shell_browser_context.cc:45: CHECK(getter_);
Since we'd crash on the next line anyways, don't think we need these CHECKs.

[pauljensen, 2012-12-20 20:24:07]

I may be horribly confused :)  Let me know.

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:86: LazyInitialize();
On 2012/12/20 16:55:06, Matt Menke wrote:
> This call no longer makes any sense.
I think it still does.  If this function is the first Handle function to be
called it's necessary.  See my description of the intended initialization below.

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:154: LazyInitialize();
On 2012/12/20 16:55:06, Matt Menke wrote:
> This LazyInitialize doesn't work - looks like the
> ChromeURLRequestContextGetter::CreateOffTheRecordForIsolatedApp gets the main
> request context from the profile, so main_request_context_getter_ must already
> be initialized, which implies LazyInitialize must already have been called.
It does work.  Here's the basic idea:
1. Someone calls one of these Handle::Create/Get functions which calls
LazyInitialize()
2. which calls io_data_->InitializeOnUIThread()
3. which calls BrowserContext::EnsureResourceContextInitialized()
4. which creates the main storage partition
5. which calls GetRequestContext() (in ToT) or CreateRequestContext() (in my CL)
6. which calls LazyInitialize() again, but it just returns because initialized_
is true
5. and then the main request context is created

Five functions end up reentrant.
Here's the relevant ToT call stack:
	ProfileImplIOData::Handle::GetMainRequestContextGetter() Line 151	C++
 	ProfileImpl::GetRequestContext() Line 801	C++
 	content::StoragePartitionImplMap::Get() Line 487	C++
 	content::`anonymous namespace'::GetStoragePartitionFromConfig() Line 64	C++
 	content::BrowserContext::GetStoragePartition() Line 149	C++
 	content::BrowserContext::GetDefaultStoragePartition() Line 181	C++
 	content::BrowserContext::EnsureResourceContextInitialized() Line 193	C++
 	ProfileIOData::InitializeOnUIThread() Line 242	C++
 	ProfileImplIOData::Handle::LazyInitialize() Line 285	C++
 	ProfileImplIOData::Handle::GetResourceContext() Line 135	C++
 	ProfileImpl::GetResourceContext() Line 837	C++
 	content::InitializeResourceContext() Line 64	C++
 	content::StoragePartitionImplMap::Get() Line 463	C++
 	content::`anonymous namespace'::GetStoragePartitionFromConfig() Line 64	C++
 	content::BrowserContext::GetStoragePartition() Line 149	C++
 	content::BrowserContext::GetDefaultStoragePartition() Line 181	C++
 	ProfileImpl::DoFinalInit() Line 527	C++
 	ProfileImpl::OnPrefsLoaded() Line 732	C++
 	ProfileImpl::ProfileImpl() Line 399	C++

[mmenke, 2012-12-20 21:12:53]

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/53005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:154: LazyInitialize();
On 2012/12/20 20:24:07, pauljensen wrote:
> On 2012/12/20 16:55:06, Matt Menke wrote:
> > This LazyInitialize doesn't work - looks like the
> > ChromeURLRequestContextGetter::CreateOffTheRecordForIsolatedApp gets the
main
> > request context from the profile, so main_request_context_getter_ must
already
> > be initialized, which implies LazyInitialize must already have been called.
> It does work.  Here's the basic idea:
> 1. Someone calls one of these Handle::Create/Get functions which calls
> LazyInitialize()
> 2. which calls io_data_->InitializeOnUIThread()
> 3. which calls BrowserContext::EnsureResourceContextInitialized()
> 4. which creates the main storage partition
> 5. which calls GetRequestContext() (in ToT) or CreateRequestContext() (in my
CL)
> 6. which calls LazyInitialize() again, but it just returns because
initialized_
> is true
> 5. and then the main request context is created
> 
> Five functions end up reentrant.
> Here's the relevant ToT call stack:
> 	ProfileImplIOData::Handle::GetMainRequestContextGetter() Line 151	C++
>  	ProfileImpl::GetRequestContext() Line 801	C++
>  	content::StoragePartitionImplMap::Get() Line 487	C++
>  	content::`anonymous namespace'::GetStoragePartitionFromConfig() Line 64	C++
>  	content::BrowserContext::GetStoragePartition() Line 149	C++
>  	content::BrowserContext::GetDefaultStoragePartition() Line 181	C++
>  	content::BrowserContext::EnsureResourceContextInitialized() Line 193	C++
>  	ProfileIOData::InitializeOnUIThread() Line 242	C++
>  	ProfileImplIOData::Handle::LazyInitialize() Line 285	C++
>  	ProfileImplIOData::Handle::GetResourceContext() Line 135	C++
>  	ProfileImpl::GetResourceContext() Line 837	C++
>  	content::InitializeResourceContext() Line 64	C++
>  	content::StoragePartitionImplMap::Get() Line 463	C++
>  	content::`anonymous namespace'::GetStoragePartitionFromConfig() Line 64	C++
>  	content::BrowserContext::GetStoragePartition() Line 149	C++
>  	content::BrowserContext::GetDefaultStoragePartition() Line 181	C++
>  	ProfileImpl::DoFinalInit() Line 527	C++
>  	ProfileImpl::OnPrefsLoaded() Line 732	C++
>  	ProfileImpl::ProfileImpl() Line 399	C++

Looks like you're right, but I think this is about 5 times more complicated than
it needs to be.  I think something like:

static BrowserContext::AddProtocolHandlers(context, URLRequestFactory) (Or
BrowserContext::SetUpRequestFactory)

Would make a lot more sense.  Right now, this code repeatedly jumps between the
content/chrome boundary, which seems like a really bad idea, and has a rather
non-obvious call path.  The downside is that the new method would have to be
called explicitly, rather than being automatically called by creation of the
storage partition, but we already basically have that behavior, which is why
EnsureResourceContextInitialized has to be called in the first place.

Thoughts?

[willchan no longer on Chromium, 2012-12-20 21:24:15]

https://chromiumcodereview.appspot.com/9425026/ is where the
regression got introduced. John, do you know if this set off any of
the startup tests? I'm concerned about this since now we have more
changes building on top of this. It looks like we had no
chrome/browser/profiles/OWNER review this code =/

On Wed, Dec 12, 2012 at 3:13 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> Aha, so if there was a regression, it must have already been introduced and
> you're not affecting any of that. Fascinating. Well, we haven't detected it,
> so hopefully it isn't a big deal. Carry on. When I get time I'll go crawl
> through out perf graphs to see if I can find any issue here, but let's
> operate as if there isn't. Thanks for the explanation.
>
>
> On Wed, Dec 12, 2012 at 2:55 PM, <pauljensen@chromium.org> wrote:
>>
>> I'm sorry if I'm not being clear.  Let me try and rephrase.
>>
>> With or without my CL, during ProfileIOData::InitializeOnUIThread() we
>> post to
>> the IO thread a task that immediately requests the main URLRequestContext
>> thereby triggering initialization of ProfileIOData. I can't say if this is
>> cause
>> for concern, I can say this is how it currently works.
>>
>> Without my CL the first call to FactoryForMain::Create() calls
>> ProfileIOData::GetMainRequestContext() which calls
>> ProfileIOData::LazyInitialize().  With my CL I'm changing
>> FactoryForMain::Create() to call ProfileIOData::Init() and then
>> ProfileIOData::GetMainRequestContext().
>>
>> https://codereview.chromium.org/11308362/
>
>

[pauljensen, 2012-12-21 03:17:08]

What about this:
1. Get rid of BrowserContext::EnsureResourceContextInitialized() call from
ProfileIOData::InitializeOnUIThread()
2. (optional) Get rid of LazyInitialize() call from
ProfileImplIOData::Handle::GetResourceContext()

I *think* this may work.  A smoke test of chromium confirmed it appears to work.
This eliminates all the reentrant functions and cuts the call stack down by
about 70% to just this:

ProfileImplIOData::Handle::CreateMainRequestContextGetter
ProfileImpl::CreateRequestContext
chrome::ChromeContentBrowserClient::CreateRequestContext
content::StoragePartitionImplMap::Get
content::GetStoragePartitionFromConfig
content::BrowserContext::GetStoragePartition
content::BrowserContext::GetDefaultStoragePartition
ProfileImpl::DoFinalInit
ProfileImpl::OnPrefsLoaded
ProfileImpl::ProfileImpl

I should note this is my second attempt at this.
Here's my first: http://crrev.com/11434006
It involved creation of the ProtocolHandlers on the Profile side and then
passing them to the StoragePartition via LinkToProtocolHandlers().
I think this second attempt is a lot cleaner.

[mmenke, 2013-01-08 17:19:26]

My main concern is the potential for the get functions to be called before the
create functions.  I think we can resolve this by switching all the get
functions to go through the corresponding StoragePartition.

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:806: return
io_data_.GetMainRequestContextGetter();
Now that we're separating out creation from initialization from the profile's
perspective, shouldn't we get rid of all the ProfileIOData::GetRequestContext*
functions, and have the ProfileImpl functions that currently wrap them go
through the StoragePartition?  Then we don't have to worry about the get before
create cases - get will do the creating, too.

This, for instance, would become: BrowserContext::GetStoragePartition(this,
NULL)->GetURLRequestContext();

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (left):

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:605:
ChromeBlobStorageContext::GetFor(browser_context_))));
Does this really need to be removed?  We are still creating the context in
StoragePartitionImplMap::Get()...

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:86: fileapi::FileSystemContext*
file_system_context) {
I think this is more complicated than necessary.

Can't we get rid of IOThreadDelegate, and just lazy init on the first call to
MaybeCreateJob?  If MaybeCreateJob is called, then we're now owned by the
JabFactoryImpl, which is running on the IO thread.

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:123:
webkit_blob_protocol_handler_impl_;
Think just calling this "webkit_blob_protocol_handler_" so it fits on one line
is preferable.

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:122: CHECK(url_request_getter_.get());
Is there a reason this is a CHECK, but there's a DCHECK below?

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:122: CHECK(url_request_getter_.get());
The get() and the one in the DHCECK can be removed.

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:142: resource_context_->getter_ =
url_request_getter_.get();
I don't like digging into the guts of a class when we don't have to, and since
ShellResourceContext is only defined within the scope of this file, we don't get
anything here.  I'd suggest just adding set_url_request_getter()

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:183: ResourceContext*
ShellBrowserContext::GetResourceContext()  {
Suggest we go back to lazy init here, getting the request context through the
storage partition.

[pauljensen, 2013-01-08 17:34:30]

On 2013/01/08 17:19:26, Matt Menke wrote:
> My main concern is the potential for the get functions to be called before the
> create functions.
I thought you were going to review crrev.com/11785010 which solves this by only
having GetRequestContext() and CreateRequestContextForStoragePartition() (i.e.
no CreateRequestContext() and no GetRequestContextForStoragePartition() )?  I
leave the ShellBrowserContext as lazy init'd too.

[mmenke, 2013-01-08 18:14:37]

On 2013/01/08 17:34:30, pauljensen wrote:
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > My main concern is the potential for the get functions to be called before
the
> > create functions.
> I thought you were going to review crrev.com/11785010 which solves this by
only
> having GetRequestContext() and CreateRequestContextForStoragePartition() (i.e.
> no CreateRequestContext() and no GetRequestContextForStoragePartition() )?  I
> leave the ShellBrowserContext as lazy init'd too.

Gah...Too many CLs with the same name.

[awong, 2013-01-16 21:31:50]

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:806: return
io_data_.GetMainRequestContextGetter();
On 2013/01/08 17:19:26, Matt Menke wrote:
> Now that we're separating out creation from initialization from the profile's
> perspective, shouldn't we get rid of all the ProfileIOData::GetRequestContext*
> functions, and have the ProfileImpl functions that currently wrap them go
> through the StoragePartition?  Then we don't have to worry about the get
before
> create cases - get will do the creating, too.
> 
> This, for instance, would become: BrowserContext::GetStoragePartition(this,
> NULL)->GetURLRequestContext();

At a surface level, this sounds fine to me. I think there's some circular
creation still that makes it hard, but Paul has his head more into it than I do
right now as I still haven't quite paged this CL back in yet.

[mmenke, 2013-01-16 23:40:36]

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:806: return
io_data_.GetMainRequestContextGetter();
On 2013/01/16 21:31:50, awong wrote:
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > Now that we're separating out creation from initialization from the
profile's
> > perspective, shouldn't we get rid of all the
ProfileIOData::GetRequestContext*
> > functions, and have the ProfileImpl functions that currently wrap them go
> > through the StoragePartition?  Then we don't have to worry about the get
> before
> > create cases - get will do the creating, too.
> > 
> > This, for instance, would become: BrowserContext::GetStoragePartition(this,
> > NULL)->GetURLRequestContext();
> 
> At a surface level, this sounds fine to me. I think there's some circular
> creation still that makes it hard, but Paul has his head more into it than I
do
> right now as I still haven't quite paged this CL back in yet.

My understanding from what you said earlier is that this doesn't work in the
case of GetRequestContextForStoragePartition, since we can't map a
partition_path to the pair of IDs that are needed to get a storage partition.

[mmenke, 2013-01-16 23:45:28]

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:806: return
io_data_.GetMainRequestContextGetter();
On 2013/01/16 23:40:36, Matt Menke wrote:
> On 2013/01/16 21:31:50, awong wrote:
> > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > Now that we're separating out creation from initialization from the
> profile's
> > > perspective, shouldn't we get rid of all the
> ProfileIOData::GetRequestContext*
> > > functions, and have the ProfileImpl functions that currently wrap them go
> > > through the StoragePartition?  Then we don't have to worry about the get
> > before
> > > create cases - get will do the creating, too.
> > > 
> > > This, for instance, would become:
BrowserContext::GetStoragePartition(this,
> > > NULL)->GetURLRequestContext();
> > 
> > At a surface level, this sounds fine to me. I think there's some circular
> > creation still that makes it hard, but Paul has his head more into it than I
> do
> > right now as I still haven't quite paged this CL back in yet.
> 
> My understanding from what you said earlier is that this doesn't work in the
> case of GetRequestContextForStoragePartition, since we can't map a
> partition_path to the pair of IDs that are needed to get a storage partition.

Oh, and while I agree it's a little circular, I think it's a reasonable
intermediate step in making the StoragePartition handle context creation.  The
functions won't need to be changed when the profile no longer creates/owns the
contexts.

[awong, 2013-01-19 00:35:41]

I re-looked over this CL. I think the main place there can be confusion is in
OffTheRecordProfileImpl and ProfileImpl with the CreateXXX functions.

I've added a suggestion for how I think we could make it more clear into
ProfileImpl. If it looks good enough, the same pattern should be replicated into
OffTheRecordProfileImpl.


On 2013/01/16 23:45:28, Matt Menke wrote:
>
https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_impl.cc:806: return
> io_data_.GetMainRequestContextGetter();
> On 2013/01/16 23:40:36, Matt Menke wrote:
> > On 2013/01/16 21:31:50, awong wrote:
> > > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > > Now that we're separating out creation from initialization from the
> > profile's
> > > > perspective, shouldn't we get rid of all the
> > ProfileIOData::GetRequestContext*
> > > > functions, and have the ProfileImpl functions that currently wrap them
go
> > > > through the StoragePartition?  Then we don't have to worry about the get
> > > before
> > > > create cases - get will do the creating, too.
> > > > 
> > > > This, for instance, would become:
> BrowserContext::GetStoragePartition(this,
> > > > NULL)->GetURLRequestContext();
> > > 
> > > At a surface level, this sounds fine to me. I think there's some circular
> > > creation still that makes it hard, but Paul has his head more into it than
I
> > do
> > > right now as I still haven't quite paged this CL back in yet.
> > 
> > My understanding from what you said earlier is that this doesn't work in the
> > case of GetRequestContextForStoragePartition, since we can't map a
> > partition_path to the pair of IDs that are needed to get a storage
partition.
> 
> Oh, and while I agree it's a little circular, I think it's a reasonable
> intermediate step in making the StoragePartition handle context creation.  The
> functions won't need to be changed when the profile no longer creates/owns the
> contexts.

[awong, 2013-01-19 00:43:42]

this time with the actual comment.

https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:807:
BrowserContext::GetStoragePartition(this, NULL);
Use BrowserContext::GetDefaultStoragePartition() instead of the explicit NULL.

https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.h:68: virtual net::URLRequestContextGetter*
CreateRequestContext(
Both CreateRequestContext() and CreateRequestContextForStoragePartition() are no
longer part of the BorwserContext API.  Move it lower?

Also, this is where it would be good to add the comment explaining the circular
initialization logic + API contract. Namely:
  (1) They create the context returned by the corresponding GetXXX() functions
  (2) They are exposed because of a circular dependency where
GetStoragePartition() is used to retrieve the request context, but creation
still has to happen here so the StoragePartition calls ContextBrowserClient to
call this function.  Also add a link to the bug I have.

[pauljensen, 2013-01-21 06:24:56]

I think I've addressed all of Matt and Albert's comments through changes or
replies.

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:806: return
io_data_.GetMainRequestContextGetter();
On 2013/01/16 23:45:28, Matt Menke wrote:
> On 2013/01/16 23:40:36, Matt Menke wrote:
> > On 2013/01/16 21:31:50, awong wrote:
> > > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > > Now that we're separating out creation from initialization from the
> > profile's
> > > > perspective, shouldn't we get rid of all the
> > ProfileIOData::GetRequestContext*
> > > > functions, and have the ProfileImpl functions that currently wrap them
go
> > > > through the StoragePartition?  Then we don't have to worry about the get
> > > before
> > > > create cases - get will do the creating, too.
> > > > 
> > > > This, for instance, would become:
> BrowserContext::GetStoragePartition(this,
> > > > NULL)->GetURLRequestContext();
> > > 
> > > At a surface level, this sounds fine to me. I think there's some circular
> > > creation still that makes it hard, but Paul has his head more into it than
I
> > do
> > > right now as I still haven't quite paged this CL back in yet.
> > 
> > My understanding from what you said earlier is that this doesn't work in the
> > case of GetRequestContextForStoragePartition, since we can't map a
> > partition_path to the pair of IDs that are needed to get a storage
partition.
> 
> Oh, and while I agree it's a little circular, I think it's a reasonable
> intermediate step in making the StoragePartition handle context creation.  The
> functions won't need to be changed when the profile no longer creates/owns the
> contexts.
I adjusted GetRequestContext() as described.  I could do something similar for
GetMediaRequestContext() but since creating that doesn't require extra
parameters (as the main request context getter does) it would just be adding an
extra function to ContentBrowserClient and all of its implementations for Albert
to remove during his refactor.  Let me know if this is still desired.  After
this refactor there is no GetRequestContextForStoragePartition() to make call
through the StoragePartition.

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (left):

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:605:
ChromeBlobStorageContext::GetFor(browser_context_))));
On 2013/01/08 17:19:26, Matt Menke wrote:
> Does this really need to be removed?  We are still creating the context in
> StoragePartitionImplMap::Get()...
The reason I started all this was to eliminate InitializeURLRequestContext()
because it uses URLRequestJobFactory::SetProtocol() which we're trying to remove
(crbug.com/146602).  The const_cast also seems dangerous as it's indicative of
changing the URLRequestJobFactory's behavior after its been constructed.  When
URLRequestContext/URLRequestJobFactory creation is moved to the StoragePartition
this can all be done here, but safely.

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:86: fileapi::FileSystemContext*
file_system_context) {
On 2013/01/08 17:19:26, Matt Menke wrote:
> I think this is more complicated than necessary.
> 
> Can't we get rid of IOThreadDelegate, and just lazy init on the first call to
> MaybeCreateJob?  If MaybeCreateJob is called, then we're now owned by the
> JabFactoryImpl, which is running on the IO thread.

Done.  The lazy-init does require making webkit_blob_protocol_handler_impl_
mutable (because MaybeCreateJob() is const) but I think this is okay as
MaybeCreateJob() is only called on one thread.  I had used all the nested
classes to avoid the mutable keyword previously but it's a lot cleaner without
the intervening class.

https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:123:
webkit_blob_protocol_handler_impl_;
On 2013/01/08 17:19:26, Matt Menke wrote:
> Think just calling this "webkit_blob_protocol_handler_" so it fits on one line
> is preferable.
With the mutable keyword it won't fit either way so I left it alone.

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:122: CHECK(url_request_getter_.get());
On 2013/01/08 17:19:26, Matt Menke wrote:
> The get() and the one in the DHCECK can be removed.

Done.

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:122: CHECK(url_request_getter_.get());
On 2013/01/08 17:19:26, Matt Menke wrote:
> Is there a reason this is a CHECK, but there's a DCHECK below?
I made this just go through the StoragePartition now. Much cleaner.

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:142: resource_context_->getter_ =
url_request_getter_.get();
On 2013/01/08 17:19:26, Matt Menke wrote:
> I don't like digging into the guts of a class when we don't have to, and since
> ShellResourceContext is only defined within the scope of this file, we don't
get
> anything here.  I'd suggest just adding set_url_request_getter()
This line of code is gone now that I made GetResouceContext() lazy again.

https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
content/shell/shell_browser_context.cc:183: ResourceContext*
ShellBrowserContext::GetResourceContext()  {
On 2013/01/08 17:19:26, Matt Menke wrote:
> Suggest we go back to lazy init here, getting the request context through the
> storage partition.

Done. I had taken away the lazy init previously because it was failing. I was
able to bring back the lazy init by adding a re-entrancy check.

https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:807:
BrowserContext::GetStoragePartition(this, NULL);
On 2013/01/19 00:43:42, awong wrote:
> Use BrowserContext::GetDefaultStoragePartition() instead of the explicit NULL.
Your comment about that function is pretty inhibiting:
"DON'T USE THIS. GetDefaultStoragePartition() is going away."
Are you sure you want me to use it?

https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.h:68: virtual net::URLRequestContextGetter*
CreateRequestContext(
On 2013/01/19 00:43:42, awong wrote:
> Both CreateRequestContext() and CreateRequestContextForStoragePartition() are
no
> longer part of the BorwserContext API.  Move it lower?
Done. 

> 
> Also, this is where it would be good to add the comment explaining the
circular
> initialization logic + API contract. Namely:
>   (1) They create the context returned by the corresponding GetXXX() functions
>   (2) They are exposed because of a circular dependency where
> GetStoragePartition() is used to retrieve the request context, but creation
> still has to happen here so the StoragePartition calls ContextBrowserClient to
> call this function.  Also add a link to the bug I have.
I feel like the comment should go in profile.h as this is just an implementation
of that.  Do you agree?  There is already a comment there; I've updated it.

[mmenke, 2013-01-22 15:32:55]

Have a number of reviews pending.  Probably won't get to this until Wednesday. 
I'll do your smaller one today.

On 2013/01/21 06:24:56, pauljensen wrote:
> I think I've addressed all of Matt and Albert's comments through changes or
> replies.
> 
>
https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_impl.cc:806: return
> io_data_.GetMainRequestContextGetter();
> On 2013/01/16 23:45:28, Matt Menke wrote:
> > On 2013/01/16 23:40:36, Matt Menke wrote:
> > > On 2013/01/16 21:31:50, awong wrote:
> > > > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > > > Now that we're separating out creation from initialization from the
> > > profile's
> > > > > perspective, shouldn't we get rid of all the
> > > ProfileIOData::GetRequestContext*
> > > > > functions, and have the ProfileImpl functions that currently wrap them
> go
> > > > > through the StoragePartition?  Then we don't have to worry about the
get
> > > > before
> > > > > create cases - get will do the creating, too.
> > > > > 
> > > > > This, for instance, would become:
> > BrowserContext::GetStoragePartition(this,
> > > > > NULL)->GetURLRequestContext();
> > > > 
> > > > At a surface level, this sounds fine to me. I think there's some
circular
> > > > creation still that makes it hard, but Paul has his head more into it
than
> I
> > > do
> > > > right now as I still haven't quite paged this CL back in yet.
> > > 
> > > My understanding from what you said earlier is that this doesn't work in
the
> > > case of GetRequestContextForStoragePartition, since we can't map a
> > > partition_path to the pair of IDs that are needed to get a storage
> partition.
> > 
> > Oh, and while I agree it's a little circular, I think it's a reasonable
> > intermediate step in making the StoragePartition handle context creation. 
The
> > functions won't need to be changed when the profile no longer creates/owns
the
> > contexts.
> I adjusted GetRequestContext() as described.  I could do something similar for
> GetMediaRequestContext() but since creating that doesn't require extra
> parameters (as the main request context getter does) it would just be adding
an
> extra function to ContentBrowserClient and all of its implementations for
Albert
> to remove during his refactor.  Let me know if this is still desired.  After
> this refactor there is no GetRequestContextForStoragePartition() to make call
> through the StoragePartition.
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> File content/browser/storage_partition_impl_map.cc (left):
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> content/browser/storage_partition_impl_map.cc:605:
> ChromeBlobStorageContext::GetFor(browser_context_))));
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > Does this really need to be removed?  We are still creating the context in
> > StoragePartitionImplMap::Get()...
> The reason I started all this was to eliminate InitializeURLRequestContext()
> because it uses URLRequestJobFactory::SetProtocol() which we're trying to
remove
> (crbug.com/146602).  The const_cast also seems dangerous as it's indicative of
> changing the URLRequestJobFactory's behavior after its been constructed.  When
> URLRequestContext/URLRequestJobFactory creation is moved to the
StoragePartition
> this can all be done here, but safely.
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> File content/browser/storage_partition_impl_map.cc (right):
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> content/browser/storage_partition_impl_map.cc:86: fileapi::FileSystemContext*
> file_system_context) {
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > I think this is more complicated than necessary.
> > 
> > Can't we get rid of IOThreadDelegate, and just lazy init on the first call
to
> > MaybeCreateJob?  If MaybeCreateJob is called, then we're now owned by the
> > JabFactoryImpl, which is running on the IO thread.
> 
> Done.  The lazy-init does require making webkit_blob_protocol_handler_impl_
> mutable (because MaybeCreateJob() is const) but I think this is okay as
> MaybeCreateJob() is only called on one thread.  I had used all the nested
> classes to avoid the mutable keyword previously but it's a lot cleaner without
> the intervening class.
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> content/browser/storage_partition_impl_map.cc:123:
> webkit_blob_protocol_handler_impl_;
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > Think just calling this "webkit_blob_protocol_handler_" so it fits on one
line
> > is preferable.
> With the mutable keyword it won't fit either way so I left it alone.
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> File content/shell/shell_browser_context.cc (right):
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> content/shell/shell_browser_context.cc:122: CHECK(url_request_getter_.get());
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > The get() and the one in the DHCECK can be removed.
> 
> Done.
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> content/shell/shell_browser_context.cc:122: CHECK(url_request_getter_.get());
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > Is there a reason this is a CHECK, but there's a DCHECK below?
> I made this just go through the StoragePartition now. Much cleaner.
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> content/shell/shell_browser_context.cc:142: resource_context_->getter_ =
> url_request_getter_.get();
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > I don't like digging into the guts of a class when we don't have to, and
since
> > ShellResourceContext is only defined within the scope of this file, we don't
> get
> > anything here.  I'd suggest just adding set_url_request_getter()
> This line of code is gone now that I made GetResouceContext() lazy again.
> 
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> content/shell/shell_browser_context.cc:183: ResourceContext*
> ShellBrowserContext::GetResourceContext()  {
> On 2013/01/08 17:19:26, Matt Menke wrote:
> > Suggest we go back to lazy init here, getting the request context through
the
> > storage partition.
> 
> Done. I had taken away the lazy init previously because it was failing. I was
> able to bring back the lazy init by adding a re-entrancy check.
> 
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_impl.cc:807:
> BrowserContext::GetStoragePartition(this, NULL);
> On 2013/01/19 00:43:42, awong wrote:
> > Use BrowserContext::GetDefaultStoragePartition() instead of the explicit
NULL.
> Your comment about that function is pretty inhibiting:
> "DON'T USE THIS. GetDefaultStoragePartition() is going away."
> Are you sure you want me to use it?
> 
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_impl.h (right):
> 
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_impl.h:68: virtual
net::URLRequestContextGetter*
> CreateRequestContext(
> On 2013/01/19 00:43:42, awong wrote:
> > Both CreateRequestContext() and CreateRequestContextForStoragePartition()
are
> no
> > longer part of the BorwserContext API.  Move it lower?
> Done. 
> 
> > 
> > Also, this is where it would be good to add the comment explaining the
> circular
> > initialization logic + API contract. Namely:
> >   (1) They create the context returned by the corresponding GetXXX()
functions
> >   (2) They are exposed because of a circular dependency where
> > GetStoragePartition() is used to retrieve the request context, but creation
> > still has to happen here so the StoragePartition calls ContextBrowserClient
to
> > call this function.  Also add a link to the bug I have.
> I feel like the comment should go in profile.h as this is just an
implementation
> of that.  Do you agree?  There is already a comment there; I've updated it.

[mmenke, 2013-01-24 04:30:27]

I'm going to have to apply this locally and go through some stack traces... 
Just too spaghetti-ish to get enough of a handle on it to sign off on it
otherwise (This was the old state of the code as well, not the fault of this
CL).  Still don't understand fundamental things like why we can still keep lazy
initialization of the OTR data.

I'll try to get back to you tomorrow, but no promises, unfortunately.

On 2013/01/22 15:32:55, Matt Menke wrote:
> Have a number of reviews pending.  Probably won't get to this until Wednesday.

> I'll do your smaller one today.
> 
> On 2013/01/21 06:24:56, pauljensen wrote:
> > I think I've addressed all of Matt and Albert's comments through changes or
> > replies.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
> > File chrome/browser/profiles/profile_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/chrome/browser/profiles/p...
> > chrome/browser/profiles/profile_impl.cc:806: return
> > io_data_.GetMainRequestContextGetter();
> > On 2013/01/16 23:45:28, Matt Menke wrote:
> > > On 2013/01/16 23:40:36, Matt Menke wrote:
> > > > On 2013/01/16 21:31:50, awong wrote:
> > > > > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > > > > Now that we're separating out creation from initialization from the
> > > > profile's
> > > > > > perspective, shouldn't we get rid of all the
> > > > ProfileIOData::GetRequestContext*
> > > > > > functions, and have the ProfileImpl functions that currently wrap
them
> > go
> > > > > > through the StoragePartition?  Then we don't have to worry about the
> get
> > > > > before
> > > > > > create cases - get will do the creating, too.
> > > > > > 
> > > > > > This, for instance, would become:
> > > BrowserContext::GetStoragePartition(this,
> > > > > > NULL)->GetURLRequestContext();
> > > > > 
> > > > > At a surface level, this sounds fine to me. I think there's some
> circular
> > > > > creation still that makes it hard, but Paul has his head more into it
> than
> > I
> > > > do
> > > > > right now as I still haven't quite paged this CL back in yet.
> > > > 
> > > > My understanding from what you said earlier is that this doesn't work in
> the
> > > > case of GetRequestContextForStoragePartition, since we can't map a
> > > > partition_path to the pair of IDs that are needed to get a storage
> > partition.
> > > 
> > > Oh, and while I agree it's a little circular, I think it's a reasonable
> > > intermediate step in making the StoragePartition handle context creation. 
> The
> > > functions won't need to be changed when the profile no longer creates/owns
> the
> > > contexts.
> > I adjusted GetRequestContext() as described.  I could do something similar
for
> > GetMediaRequestContext() but since creating that doesn't require extra
> > parameters (as the main request context getter does) it would just be adding
> an
> > extra function to ContentBrowserClient and all of its implementations for
> Albert
> > to remove during his refactor.  Let me know if this is still desired.  After
> > this refactor there is no GetRequestContextForStoragePartition() to make
call
> > through the StoragePartition.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> > File content/browser/storage_partition_impl_map.cc (left):
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> > content/browser/storage_partition_impl_map.cc:605:
> > ChromeBlobStorageContext::GetFor(browser_context_))));
> > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > Does this really need to be removed?  We are still creating the context in
> > > StoragePartitionImplMap::Get()...
> > The reason I started all this was to eliminate InitializeURLRequestContext()
> > because it uses URLRequestJobFactory::SetProtocol() which we're trying to
> remove
> > (crbug.com/146602).  The const_cast also seems dangerous as it's indicative
of
> > changing the URLRequestJobFactory's behavior after its been constructed. 
When
> > URLRequestContext/URLRequestJobFactory creation is moved to the
> StoragePartition
> > this can all be done here, but safely.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> > File content/browser/storage_partition_impl_map.cc (right):
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> > content/browser/storage_partition_impl_map.cc:86:
fileapi::FileSystemContext*
> > file_system_context) {
> > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > I think this is more complicated than necessary.
> > > 
> > > Can't we get rid of IOThreadDelegate, and just lazy init on the first call
> to
> > > MaybeCreateJob?  If MaybeCreateJob is called, then we're now owned by the
> > > JabFactoryImpl, which is running on the IO thread.
> > 
> > Done.  The lazy-init does require making webkit_blob_protocol_handler_impl_
> > mutable (because MaybeCreateJob() is const) but I think this is okay as
> > MaybeCreateJob() is only called on one thread.  I had used all the nested
> > classes to avoid the mutable keyword previously but it's a lot cleaner
without
> > the intervening class.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...
> > content/browser/storage_partition_impl_map.cc:123:
> > webkit_blob_protocol_handler_impl_;
> > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > Think just calling this "webkit_blob_protocol_handler_" so it fits on one
> line
> > > is preferable.
> > With the mutable keyword it won't fit either way so I left it alone.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> > File content/shell/shell_browser_context.cc (right):
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> > content/shell/shell_browser_context.cc:122:
CHECK(url_request_getter_.get());
> > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > The get() and the one in the DHCECK can be removed.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> > content/shell/shell_browser_context.cc:122:
CHECK(url_request_getter_.get());
> > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > Is there a reason this is a CHECK, but there's a DCHECK below?
> > I made this just go through the StoragePartition now. Much cleaner.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> > content/shell/shell_browser_context.cc:142: resource_context_->getter_ =
> > url_request_getter_.get();
> > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > I don't like digging into the guts of a class when we don't have to, and
> since
> > > ShellResourceContext is only defined within the scope of this file, we
don't
> > get
> > > anything here.  I'd suggest just adding set_url_request_getter()
> > This line of code is gone now that I made GetResouceContext() lazy again.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/70001/content/shell/shell_brows...
> > content/shell/shell_browser_context.cc:183: ResourceContext*
> > ShellBrowserContext::GetResourceContext()  {
> > On 2013/01/08 17:19:26, Matt Menke wrote:
> > > Suggest we go back to lazy init here, getting the request context through
> the
> > > storage partition.
> > 
> > Done. I had taken away the lazy init previously because it was failing. I
was
> > able to bring back the lazy init by adding a re-entrancy check.
> > 
> >
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
> > File chrome/browser/profiles/profile_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
> > chrome/browser/profiles/profile_impl.cc:807:
> > BrowserContext::GetStoragePartition(this, NULL);
> > On 2013/01/19 00:43:42, awong wrote:
> > > Use BrowserContext::GetDefaultStoragePartition() instead of the explicit
> NULL.
> > Your comment about that function is pretty inhibiting:
> > "DON'T USE THIS. GetDefaultStoragePartition() is going away."
> > Are you sure you want me to use it?
> > 
> >
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
> > File chrome/browser/profiles/profile_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...
> > chrome/browser/profiles/profile_impl.h:68: virtual
> net::URLRequestContextGetter*
> > CreateRequestContext(
> > On 2013/01/19 00:43:42, awong wrote:
> > > Both CreateRequestContext() and CreateRequestContextForStoragePartition()
> are
> > no
> > > longer part of the BorwserContext API.  Move it lower?
> > Done. 
> > 
> > > 
> > > Also, this is where it would be good to add the comment explaining the
> > circular
> > > initialization logic + API contract. Namely:
> > >   (1) They create the context returned by the corresponding GetXXX()
> functions
> > >   (2) They are exposed because of a circular dependency where
> > > GetStoragePartition() is used to retrieve the request context, but
creation
> > > still has to happen here so the StoragePartition calls
ContextBrowserClient
> to
> > > call this function.  Also add a link to the bug I have.
> > I feel like the comment should go in profile.h as this is just an
> implementation
> > of that.  Do you agree?  There is already a comment there; I've updated it.

[mmenke, 2013-01-25 18:38:58]

LGTM.  Sorry again for slowness.

https://codereview.chromium.org/11308362/diff/109001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/11308362/diff/109001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_io_data.cc:87:
developer_protocol_handler) const {
nit:  Fix indent.

https://codereview.chromium.org/11308362/diff/109001/content/browser/storage_...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/109001/content/browser/storage_...
content/browser/storage_partition_impl_map.cc:478: file_system_protocol_handler(
Hmm...Is this indentation right?  Know that's what we do for function
declarations, but they're somewhat unique in that they don't have stuff on the
preceding line, normally.

Not sure this is wrong, but not sure it isn't, either.

https://codereview.chromium.org/11308362/diff/109001/content/shell/shell_brow...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/109001/content/shell/shell_brow...
content/shell/shell_browser_context.cc:104: content::StoragePartition*
storage_partition =
nit:  content not needed.

https://codereview.chromium.org/11308362/diff/109001/content/shell/shell_cont...
File content/shell/shell_content_browser_client.cc (right):

https://codereview.chromium.org/11308362/diff/109001/content/shell/shell_cont...
content/shell/shell_content_browser_client.cc:83: content::BrowserContext*
content_browser_context,
nit:  content not needed.

https://codereview.chromium.org/11308362/diff/109001/content/shell/shell_cont...
content/shell/shell_content_browser_client.cc:99: content::BrowserContext*
content_browser_context,
nit:  content not needed.

[awong, 2013-01-25 19:52:57]

LGTM w/ nit

Thanks for bearing through all the churn on the CL.  Let's get this in!

https://codereview.chromium.org/11308362/diff/109001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/11308362/diff/109001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.h:98: virtual
net::URLRequestContextGetter* CreateRequestContext(
Move out of the "content::BrowserContext implementation:"

https://codereview.chromium.org/11308362/diff/109001/content/browser/storage_...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/109001/content/browser/storage_...
content/browser/storage_partition_impl_map.cc:478: file_system_protocol_handler(
On 2013/01/25 18:38:58, Matt Menke wrote:
> Hmm...Is this indentation right?  Know that's what we do for function
> declarations, but they're somewhat unique in that they don't have stuff on the
> preceding line, normally.
> 
> Not sure this is wrong, but not sure it isn't, either.

Yah...filesystem_protocol_handlers should be in 4 more spaces.  Basically each
line continuation should gain a 4-space indent.  Exceptions are return types in
function signatures, templates parameters, and if there's something on the
proceeding line that makes it go longer.

On the otherhand, it might just be good to insert a using
net::URLRequestJobFactory::ProtocolHandler; in this function right about these
declarations?  Up to you.

[pauljensen, 2013-01-25 22:09:02]

Now that everyone has approved this CL...
jam@ just landed http://crrev.com/12049052 which adds a couple more
ProtocolHandlers to InitializeURLRequestContext() (which this CL seeks to
remove).  This leaves me two options:

1. Abandon this CL.  I'd concede that InitializeURLRequestContext() will
eventually morph into something that actually creates the
URLRequestJobFactoryImpl anyhow, once Albert is done with crbug.com/159193.
2. Modify this CL to pass 5 interceptor/ProtocolHandlers instead of 3
intercetor/ProtocolHandlers from StoragePartition to the Profiles and content
Shell.  This will add maybe 50 lines of code that Albert will end up removing
for crbug.com/159193.  I'd also have to make a couple small changes to the two
ProtocolHandlers so they can be initialized on the UI thread.

Option #1 is easy for me but blocks crbug.com/146602 on crbug.com/159193.
Option #2 is fairly easy also (could be done by Monday) but is adding some cruft
for Albert to remove.  When I say cruft I should mention that it's basically
duplicating a bunch of code that should all be (in the long run)  in the
StoragePartition instead of copied out to each Profile and content Shell.

I'm tempted to go with option #2 so Erik's refactoring can keep moving.
Anyhow have a strong vote for #1?

I think I'll implement #2 and upload it; then reviewers can give it the yay or
nay.

[awong, 2013-01-25 22:21:19]

Go for 2. Blah...I actually looked at jam@'s CL remembering thinking "does
this conflict?"

Let's get Erik unblocked. I think that's the highest priority as it paves
the road to cleans up so much other stuff.


On Fri, Jan 25, 2013 at 2:09 PM, <pauljensen@chromium.org> wrote:

> Now that everyone has approved this CL...
> jam@ just landed http://crrev.com/12049052 which adds a couple more
> ProtocolHandlers to InitializeURLRequestContext() (which this CL seeks to
> remove).  This leaves me two options:
>
> 1. Abandon this CL.  I'd concede that InitializeURLRequestContext() will
> eventually morph into something that actually creates the
> URLRequestJobFactoryImpl anyhow, once Albert is done with crbug.com/159193
> .
> 2. Modify this CL to pass 5 interceptor/ProtocolHandlers instead of 3
> intercetor/ProtocolHandlers from StoragePartition to the Profiles and
> content
> Shell.  This will add maybe 50 lines of code that Albert will end up
> removing
> for crbug.com/159193.  I'd also have to make a couple small changes to
> the two
> ProtocolHandlers so they can be initialized on the UI thread.
>
> Option #1 is easy for me but blocks crbug.com/146602 on crbug.com/159193.
> Option #2 is fairly easy also (could be done by Monday) but is adding some
> cruft
> for Albert to remove.  When I say cruft I should mention that it's
> basically
> duplicating a bunch of code that should all be (in the long run)  in the
> StoragePartition instead of copied out to each Profile and content Shell.
>
> I'm tempted to go with option #2 so Erik's refactoring can keep moving.
> Anyhow have a strong vote for #1?
>
> I think I'll implement #2 and upload it; then reviewers can give it the
> yay or
> nay.
>
>
https://codereview.chromium.**org/11308362/<https://codereview.chromium.org/1...
>

[mmenke, 2013-01-25 22:28:21]

On 2013/01/25 22:09:02, pauljensen wrote:
> Now that everyone has approved this CL...
> jam@ just landed http://crrev.com/12049052 which adds a couple more
> ProtocolHandlers to InitializeURLRequestContext() (which this CL seeks to
> remove).  This leaves me two options:
> 
> 1. Abandon this CL.  I'd concede that InitializeURLRequestContext() will
> eventually morph into something that actually creates the
> URLRequestJobFactoryImpl anyhow, once Albert is done with crbug.com/159193.
> 2. Modify this CL to pass 5 interceptor/ProtocolHandlers instead of 3
> intercetor/ProtocolHandlers from StoragePartition to the Profiles and content
> Shell.  This will add maybe 50 lines of code that Albert will end up removing
> for crbug.com/159193.  I'd also have to make a couple small changes to the two
> ProtocolHandlers so they can be initialized on the UI thread.
> 
> Option #1 is easy for me but blocks crbug.com/146602 on crbug.com/159193.
> Option #2 is fairly easy also (could be done by Monday) but is adding some
cruft
> for Albert to remove.  When I say cruft I should mention that it's basically
> duplicating a bunch of code that should all be (in the long run)  in the
> StoragePartition instead of copied out to each Profile and content Shell.
> 
> I'm tempted to go with option #2 so Erik's refactoring can keep moving.
> Anyhow have a strong vote for #1?
> 
> I think I'll implement #2 and upload it; then reviewers can give it the yay or
> nay.

Any possibility more stuff will be added, requiring yet another modifiction of
all 37 gazillion files?  You could also pass them all together in one of two
ways.

1)  Create a content::ProtocolHandlerWidgetThingy(TM) and pass it around
instead, and then move the protocol handlers over manually.
2)  Create a ProtocolInterceptJobFactory, and set the URLRequestJobFactory that
it wraps late in the game.  You'd need a meta ProtocolHandlerInterceptor thing
to delegate to all of content's handlers as appropriate, unfortunately, and this
wouldn't complain about two protocol handlers that for the same protocol.

Only real problem with both approaches is it becomes part of content's
interface, but think that's better than periodically adding more protocol
handlers that go through all these functions.

[mmenke, 2013-01-25 22:29:23]

On 2013/01/25 22:28:21, Matt Menke wrote:
> On 2013/01/25 22:09:02, pauljensen wrote:
> > Now that everyone has approved this CL...
> > jam@ just landed http://crrev.com/12049052 which adds a couple more
> > ProtocolHandlers to InitializeURLRequestContext() (which this CL seeks to
> > remove).  This leaves me two options:
> > 
> > 1. Abandon this CL.  I'd concede that InitializeURLRequestContext() will
> > eventually morph into something that actually creates the
> > URLRequestJobFactoryImpl anyhow, once Albert is done with crbug.com/159193.
> > 2. Modify this CL to pass 5 interceptor/ProtocolHandlers instead of 3
> > intercetor/ProtocolHandlers from StoragePartition to the Profiles and
content
> > Shell.  This will add maybe 50 lines of code that Albert will end up
removing
> > for crbug.com/159193.  I'd also have to make a couple small changes to the
two
> > ProtocolHandlers so they can be initialized on the UI thread.
> > 
> > Option #1 is easy for me but blocks crbug.com/146602 on crbug.com/159193.
> > Option #2 is fairly easy also (could be done by Monday) but is adding some
> cruft
> > for Albert to remove.  When I say cruft I should mention that it's basically
> > duplicating a bunch of code that should all be (in the long run)  in the
> > StoragePartition instead of copied out to each Profile and content Shell.
> > 
> > I'm tempted to go with option #2 so Erik's refactoring can keep moving.
> > Anyhow have a strong vote for #1?
> > 
> > I think I'll implement #2 and upload it; then reviewers can give it the yay
or
> > nay.
> 
> Any possibility more stuff will be added, requiring yet another modifiction of
> all 37 gazillion files?  You could also pass them all together in one of two
> ways.
> 
> 1)  Create a content::ProtocolHandlerWidgetThingy(TM) and pass it around
> instead, and then move the protocol handlers over manually.
> 2)  Create a ProtocolInterceptJobFactory, and set the URLRequestJobFactory
that
> it wraps late in the game.  You'd need a meta ProtocolHandlerInterceptor thing
> to delegate to all of content's handlers as appropriate, unfortunately, and
this
> wouldn't complain about two protocol handlers that for the same protocol.
> 
> Only real problem with both approaches is it becomes part of content's
> interface, but think that's better than periodically adding more protocol
> handlers that go through all these functions.

2)  Could also be done by just passing the super protocol handler around, come
to think of it.

[pauljensen, 2013-01-28 02:40:23]

Guessing from jam@'s bug I think he won't be adding any more ProtocolHandlers. 
I think Albert may have to move lots of them over to the StorageParition however
as he works on crbug.com/159193.

This leaves several solutions for this CL. I'd like a consensus on what everyone
wants for an interface before I go an change all the gazillion files.  As I see
it, here are the possible API's for passing the ProtocolHandlers from the
StoragePartition to the ContentBrowserClient/Profile:
1. Just add two more ProtocolHandler arguments (bringing the total to five).
  Pro: Simple. Easy for Albert to eliminate later.
  Con: Bulky, esp if Albert has to add to it.
2. Pass one ProtocolHandler that gets wrapped in a ProtocolInterceptorJobFactory
later by Profile.
  Pro: Tiny API.  Easy for Albert to add to and eventually eliminate.
  Con: URLRequestJobFactory::IsHandledProtocol() is not correct.
3. Pass a std::vector<std::pair<std::string, URLRequestProtocolHandler*> > where
the string is the scheme (or EmptyString if its an interceptor) that the
ProtocolHandler should be registered to handle.
  Pro: Simple-ish.  Easy for Albert to add to and eventually eliminate.
  Con: A little wonky and not terribly straightforward.
4. Pass a chainable URLRequestJobFactory, just like
ProtocolHandlerRegistry::JobInterceptorFactory
  Pro: Very clean interface.  Easy for Albert to add to and eventually
eliminate.
  Con: Slightly more code.
5. Pass a couple BindState's.  One that takes a URLRequestJobFactoryImpl and
adds ProtocolHandlers.  Another that takes and returns a
scoped_ptr<URLRequestJobFactory> that potentially inserts other factories into
the chain (i.e. for the interceptor).
  Pro: Easiest for Albert to eventually eliminate.
  Con: Ghastly abomination to look at.

I guess my vote is for #4.  Though 1, 2, or 3 are easier for me.  Keep in mind
that whatever choice we pick is ephemeral, as Albert will be removing it.

On 2013/01/25 22:29:23, Matt Menke wrote:
> On 2013/01/25 22:28:21, Matt Menke wrote:
> > On 2013/01/25 22:09:02, pauljensen wrote:
> > > Now that everyone has approved this CL...
> > > jam@ just landed http://crrev.com/12049052 which adds a couple more
> > > ProtocolHandlers to InitializeURLRequestContext() (which this CL seeks to
> > > remove).  This leaves me two options:
> > > 
> > > 1. Abandon this CL.  I'd concede that InitializeURLRequestContext() will
> > > eventually morph into something that actually creates the
> > > URLRequestJobFactoryImpl anyhow, once Albert is done with
crbug.com/159193.
> > > 2. Modify this CL to pass 5 interceptor/ProtocolHandlers instead of 3
> > > intercetor/ProtocolHandlers from StoragePartition to the Profiles and
> content
> > > Shell.  This will add maybe 50 lines of code that Albert will end up
> removing
> > > for crbug.com/159193.  I'd also have to make a couple small changes to the
> two
> > > ProtocolHandlers so they can be initialized on the UI thread.
> > > 
> > > Option #1 is easy for me but blocks crbug.com/146602 on crbug.com/159193.
> > > Option #2 is fairly easy also (could be done by Monday) but is adding some
> > cruft
> > > for Albert to remove.  When I say cruft I should mention that it's
basically
> > > duplicating a bunch of code that should all be (in the long run)  in the
> > > StoragePartition instead of copied out to each Profile and content Shell.
> > > 
> > > I'm tempted to go with option #2 so Erik's refactoring can keep moving.
> > > Anyhow have a strong vote for #1?
> > > 
> > > I think I'll implement #2 and upload it; then reviewers can give it the
yay
> or
> > > nay.
> > 
> > Any possibility more stuff will be added, requiring yet another modifiction
of
> > all 37 gazillion files?  You could also pass them all together in one of two
> > ways.
> > 
> > 1)  Create a content::ProtocolHandlerWidgetThingy(TM) and pass it around
> > instead, and then move the protocol handlers over manually.
> > 2)  Create a ProtocolInterceptJobFactory, and set the URLRequestJobFactory
> that
> > it wraps late in the game.  You'd need a meta ProtocolHandlerInterceptor
thing
> > to delegate to all of content's handlers as appropriate, unfortunately, and
> this
> > wouldn't complain about two protocol handlers that for the same protocol.
> > 
> > Only real problem with both approaches is it becomes part of content's
> > interface, but think that's better than periodically adding more protocol
> > handlers that go through all these functions.
> 
> 2)  Could also be done by just passing the super protocol handler around, come
> to think of it.

[awong, 2013-01-28 05:09:00]

I'm good with 1, 2, or 4.  No real preference. If you're willing to do (4),
and you think it's the cleanest, go for it.

Thanks again for dealing with the shifting sands on this code.

-Albert


On Sun, Jan 27, 2013 at 6:40 PM, <pauljensen@chromium.org> wrote:

> Guessing from jam@'s bug I think he won't be adding any more
> ProtocolHandlers.
> I think Albert may have to move lots of them over to the StorageParition
> however
> as he works on crbug.com/159193.
>
> This leaves several solutions for this CL. I'd like a consensus on what
> everyone
> wants for an interface before I go an change all the gazillion files.  As
> I see
> it, here are the possible API's for passing the ProtocolHandlers from the
> StoragePartition to the ContentBrowserClient/Profile:
> 1. Just add two more ProtocolHandler arguments (bringing the total to
> five).
>   Pro: Simple. Easy for Albert to eliminate later.
>   Con: Bulky, esp if Albert has to add to it.
> 2. Pass one ProtocolHandler that gets wrapped in a
> ProtocolInterceptorJobFactory
> later by Profile.
>   Pro: Tiny API.  Easy for Albert to add to and eventually eliminate.
>   Con: URLRequestJobFactory::**IsHandledProtocol() is not correct.
> 3. Pass a std::vector<std::pair<std::**string,
> URLRequestProtocolHandler*> > where
> the string is the scheme (or EmptyString if its an interceptor) that the
> ProtocolHandler should be registered to handle.
>   Pro: Simple-ish.  Easy for Albert to add to and eventually eliminate.
>   Con: A little wonky and not terribly straightforward.
> 4. Pass a chainable URLRequestJobFactory, just like
> ProtocolHandlerRegistry::**JobInterceptorFactory
>   Pro: Very clean interface.  Easy for Albert to add to and eventually
> eliminate.
>   Con: Slightly more code.
> 5. Pass a couple BindState's.  One that takes a URLRequestJobFactoryImpl
> and
> adds ProtocolHandlers.  Another that takes and returns a
> scoped_ptr<**URLRequestJobFactory> that potentially inserts other
> factories into
> the chain (i.e. for the interceptor).
>   Pro: Easiest for Albert to eventually eliminate.
>   Con: Ghastly abomination to look at.
>
> I guess my vote is for #4.  Though 1, 2, or 3 are easier for me.  Keep in
> mind
> that whatever choice we pick is ephemeral, as Albert will be removing it.
>
>
> On 2013/01/25 22:29:23, Matt Menke wrote:
>
>> On 2013/01/25 22:28:21, Matt Menke wrote:
>> > On 2013/01/25 22:09:02, pauljensen wrote:
>> > > Now that everyone has approved this CL...
>> > > jam@ just landed http://crrev.com/12049052 which adds a couple more
>> > > ProtocolHandlers to InitializeURLRequestContext() (which this CL
>> seeks to
>> > > remove).  This leaves me two options:
>> > >
>> > > 1. Abandon this CL.  I'd concede that InitializeURLRequestContext()
>> will
>> > > eventually morph into something that actually creates the
>> > > URLRequestJobFactoryImpl anyhow, once Albert is done with
>>
> crbug.com/159193.
>
>> > > 2. Modify this CL to pass 5 interceptor/ProtocolHandlers instead of 3
>> > > intercetor/ProtocolHandlers from StoragePartition to the Profiles and
>> content
>> > > Shell.  This will add maybe 50 lines of code that Albert will end up
>> removing
>> > > for crbug.com/159193.  I'd also have to make a couple small changes
>> to the
>> two
>> > > ProtocolHandlers so they can be initialized on the UI thread.
>> > >
>> > > Option #1 is easy for me but blocks crbug.com/146602 on
>> crbug.com/159193.
>> > > Option #2 is fairly easy also (could be done by Monday) but is adding
>> some
>> > cruft
>> > > for Albert to remove.  When I say cruft I should mention that it's
>>
> basically
>
>> > > duplicating a bunch of code that should all be (in the long run)  in
>> the
>> > > StoragePartition instead of copied out to each Profile and content
>> Shell.
>> > >
>> > > I'm tempted to go with option #2 so Erik's refactoring can keep
>> moving.
>> > > Anyhow have a strong vote for #1?
>> > >
>> > > I think I'll implement #2 and upload it; then reviewers can give it
>> the
>>
> yay
>
>> or
>> > > nay.
>> >
>> > Any possibility more stuff will be added, requiring yet another
>> modifiction
>>
> of
>
>> > all 37 gazillion files?  You could also pass them all together in one
>> of two
>> > ways.
>> >
>> > 1)  Create a content::**ProtocolHandlerWidgetThingy(**TM) and pass it
>> around
>> > instead, and then move the protocol handlers over manually.
>> > 2)  Create a ProtocolInterceptJobFactory, and set the
>> URLRequestJobFactory
>> that
>> > it wraps late in the game.  You'd need a meta ProtocolHandlerInterceptor
>>
> thing
>
>> > to delegate to all of content's handlers as appropriate, unfortunately,
>> and
>> this
>> > wouldn't complain about two protocol handlers that for the same
>> protocol.
>> >
>> > Only real problem with both approaches is it becomes part of content's
>> > interface, but think that's better than periodically adding more
>> protocol
>> > handlers that go through all these functions.
>>
>
>  2)  Could also be done by just passing the super protocol handler around,
>> come
>> to think of it.
>>
>
>
>
https://codereview.chromium.**org/11308362/<https://codereview.chromium.org/1...
>

[mmenke, 2013-01-28 06:04:45]

I agree with Albert, though I think either 1 or 4 is better than 2.

On 2013/01/28 05:09:00, awong wrote:
> I'm good with 1, 2, or 4.  No real preference. If you're willing to do (4),
> and you think it's the cleanest, go for it.
> 
> Thanks again for dealing with the shifting sands on this code.
> 
> -Albert
> 
> 
> On Sun, Jan 27, 2013 at 6:40 PM, <mailto:pauljensen@chromium.org> wrote:
> 
> > Guessing from jam@'s bug I think he won't be adding any more
> > ProtocolHandlers.
> > I think Albert may have to move lots of them over to the StorageParition
> > however
> > as he works on crbug.com/159193.
> >
> > This leaves several solutions for this CL. I'd like a consensus on what
> > everyone
> > wants for an interface before I go an change all the gazillion files.  As
> > I see
> > it, here are the possible API's for passing the ProtocolHandlers from the
> > StoragePartition to the ContentBrowserClient/Profile:
> > 1. Just add two more ProtocolHandler arguments (bringing the total to
> > five).
> >   Pro: Simple. Easy for Albert to eliminate later.
> >   Con: Bulky, esp if Albert has to add to it.
> > 2. Pass one ProtocolHandler that gets wrapped in a
> > ProtocolInterceptorJobFactory
> > later by Profile.
> >   Pro: Tiny API.  Easy for Albert to add to and eventually eliminate.
> >   Con: URLRequestJobFactory::**IsHandledProtocol() is not correct.
> > 3. Pass a std::vector<std::pair<std::**string,
> > URLRequestProtocolHandler*> > where
> > the string is the scheme (or EmptyString if its an interceptor) that the
> > ProtocolHandler should be registered to handle.
> >   Pro: Simple-ish.  Easy for Albert to add to and eventually eliminate.
> >   Con: A little wonky and not terribly straightforward.
> > 4. Pass a chainable URLRequestJobFactory, just like
> > ProtocolHandlerRegistry::**JobInterceptorFactory
> >   Pro: Very clean interface.  Easy for Albert to add to and eventually
> > eliminate.
> >   Con: Slightly more code.
> > 5. Pass a couple BindState's.  One that takes a URLRequestJobFactoryImpl
> > and
> > adds ProtocolHandlers.  Another that takes and returns a
> > scoped_ptr<**URLRequestJobFactory> that potentially inserts other
> > factories into
> > the chain (i.e. for the interceptor).
> >   Pro: Easiest for Albert to eventually eliminate.
> >   Con: Ghastly abomination to look at.
> >
> > I guess my vote is for #4.  Though 1, 2, or 3 are easier for me.  Keep in
> > mind
> > that whatever choice we pick is ephemeral, as Albert will be removing it.
> >
> >
> > On 2013/01/25 22:29:23, Matt Menke wrote:
> >
> >> On 2013/01/25 22:28:21, Matt Menke wrote:
> >> > On 2013/01/25 22:09:02, pauljensen wrote:
> >> > > Now that everyone has approved this CL...
> >> > > jam@ just landed http://crrev.com/12049052 which adds a couple more
> >> > > ProtocolHandlers to InitializeURLRequestContext() (which this CL
> >> seeks to
> >> > > remove).  This leaves me two options:
> >> > >
> >> > > 1. Abandon this CL.  I'd concede that InitializeURLRequestContext()
> >> will
> >> > > eventually morph into something that actually creates the
> >> > > URLRequestJobFactoryImpl anyhow, once Albert is done with
> >>
> > crbug.com/159193.
> >
> >> > > 2. Modify this CL to pass 5 interceptor/ProtocolHandlers instead of 3
> >> > > intercetor/ProtocolHandlers from StoragePartition to the Profiles and
> >> content
> >> > > Shell.  This will add maybe 50 lines of code that Albert will end up
> >> removing
> >> > > for crbug.com/159193.  I'd also have to make a couple small changes
> >> to the
> >> two
> >> > > ProtocolHandlers so they can be initialized on the UI thread.
> >> > >
> >> > > Option #1 is easy for me but blocks crbug.com/146602 on
> >> crbug.com/159193.
> >> > > Option #2 is fairly easy also (could be done by Monday) but is adding
> >> some
> >> > cruft
> >> > > for Albert to remove.  When I say cruft I should mention that it's
> >>
> > basically
> >
> >> > > duplicating a bunch of code that should all be (in the long run)  in
> >> the
> >> > > StoragePartition instead of copied out to each Profile and content
> >> Shell.
> >> > >
> >> > > I'm tempted to go with option #2 so Erik's refactoring can keep
> >> moving.
> >> > > Anyhow have a strong vote for #1?
> >> > >
> >> > > I think I'll implement #2 and upload it; then reviewers can give it
> >> the
> >>
> > yay
> >
> >> or
> >> > > nay.
> >> >
> >> > Any possibility more stuff will be added, requiring yet another
> >> modifiction
> >>
> > of
> >
> >> > all 37 gazillion files?  You could also pass them all together in one
> >> of two
> >> > ways.
> >> >
> >> > 1)  Create a content::**ProtocolHandlerWidgetThingy(**TM) and pass it
> >> around
> >> > instead, and then move the protocol handlers over manually.
> >> > 2)  Create a ProtocolInterceptJobFactory, and set the
> >> URLRequestJobFactory
> >> that
> >> > it wraps late in the game.  You'd need a meta ProtocolHandlerInterceptor
> >>
> > thing
> >
> >> > to delegate to all of content's handlers as appropriate, unfortunately,
> >> and
> >> this
> >> > wouldn't complain about two protocol handlers that for the same
> >> protocol.
> >> >
> >> > Only real problem with both approaches is it becomes part of content's
> >> > interface, but think that's better than periodically adding more
> >> protocol
> >> > handlers that go through all these functions.
> >>
> >
> >  2)  Could also be done by just passing the super protocol handler around,
> >> come
> >> to think of it.
> >>
> >
> >
> >
>
https://codereview.chromium.**org/11308362/%3Chttps://codereview.chromium.org...>
> >

[pauljensen, 2013-01-29 04:48:58]

PTAL.  I addressed Matt and Albert's comments.  I synced it up to ToT.

[mmenke, 2013-01-29 05:21:23]

On 2013/01/29 04:48:58, pauljensen wrote:
> PTAL.  I addressed Matt and Albert's comments.  I synced it up to ToT.

Mind fixing the compile error and pinging again?  Expect it's a minor fix, but
this CL is big enough that I'd rather not have to go through it more than once
more.  :)

[pauljensen, 2013-02-01 14:06:01]

Matt & Albert, PTAL.

[awong, 2013-02-02 03:26:12]

Did a pass through.  Had a few questions and a couple of clarifications, etc.

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:17: class
AwBrowserContext::AwResourceContext : public content::ResourceContext {
Any reason this isn't just file-scoped in an anonymous namespace?

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:19:
AwResourceContext(net::URLRequestContextGetter* getter) : getter_(getter) {}
explicit

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:22: // content::ResourceContext
implementation:
: -> .

Seems to match the prevailing convention for these kinds of comments better.

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.h:31:
AwURLRequestContextGetter(AwBrowserContext* browser_context);
Should this be explicit?

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.h:44: // Prior to
GetURLRequestContext() being called, SetProtocolHandlers() is
Can we dump this in private and friend AwBrowserContext? 

Also, am I right that this is supposed to be called on the UI thread and that
this is needed because we can't just create the JobFactory on the UI thread?  If
so, please explain in comment.  The comment is currently good at explaining waht
is happening, but it doesn't answer why we need this.

https://codereview.chromium.org/11308362/diff/132023/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/132023/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:256:
BrowserContext::GetStoragePartition(this, NULL);
Prefer GetDefaultStoragePartition() to using NULL.

https://codereview.chromium.org/11308362/diff/132023/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/132023/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:855:
BrowserContext::GetStoragePartition(this, NULL);
GetDefaultStoragePartition()

https://codereview.chromium.org/11308362/diff/132023/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.h (right):

https://codereview.chromium.org/11308362/diff/132023/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.h:43: content::ResourceContext*
resource_context,
Where is this change coming from?

https://codereview.chromium.org/11308362/diff/132023/content/shell/shell_brow...
File content/shell/shell_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/132023/content/shell/shell_brow...
content/shell/shell_browser_context.cc:131:
BrowserContext::GetStoragePartition(this, NULL);
GetDefaultStoragePartition()

[pauljensen, 2013-02-04 14:18:53]

Albert, I addressed several of your comments but had a couple follow-on
questions below.

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:17: class
AwBrowserContext::AwResourceContext : public content::ResourceContext {
On 2013/02/02 03:26:12, awong wrote:
> Any reason this isn't just file-scoped in an anonymous namespace?
We need a pointer to it inside AwBrowserContext so it must be at least
incompletely exposed outside this file, in aw_browser_context.h, so it cannot be
in an anonymous namespace.

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:19:
AwResourceContext(net::URLRequestContextGetter* getter) : getter_(getter) {}
On 2013/02/02 03:26:12, awong wrote:
> explicit

Done.

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:22: // content::ResourceContext
implementation:
On 2013/02/02 03:26:12, awong wrote:
> : -> .
> 
> Seems to match the prevailing convention for these kinds of comments better.

Done.

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.h:31:
AwURLRequestContextGetter(AwBrowserContext* browser_context);
On 2013/02/02 03:26:12, awong wrote:
> Should this be explicit?

Done.

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.h:44: // Prior to
GetURLRequestContext() being called, SetProtocolHandlers() is
On 2013/02/02 03:26:12, awong wrote:
> Can we dump this in private and friend AwBrowserContext?
I could, but the style guide says friends are normally in the same file.  I
could also move this into an anonymous namespace inside aw_browser_context.cc
but that would be a lot of code thrown in one file.  Do you still want me to
privatize SetProtocolHandlers() and friend AwBrowserContext?
> 
> Also, am I right that this is supposed to be called on the UI thread and that
> this is needed because we can't just create the JobFactory on the UI thread? 
If
> so, please explain in comment.  The comment is currently good at explaining
waht
> is happening, but it doesn't answer why we need this.
Done.

https://codereview.chromium.org/11308362/diff/132023/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/132023/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:256:
BrowserContext::GetStoragePartition(this, NULL);
On 2013/02/02 03:26:12, awong wrote:
> Prefer GetDefaultStoragePartition() to using NULL.
Please respond to my prior comment on this matter here:
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...

https://codereview.chromium.org/11308362/diff/132023/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.h (right):

https://codereview.chromium.org/11308362/diff/132023/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.h:43: content::ResourceContext*
resource_context,
On 2013/02/02 03:26:12, awong wrote:
> Where is this change coming from?
Creation now takes place on the UI thread instead of the IO thread, so we must
delay calling GetURLDataManagerForResourceContext() until we're on the IO
thread.  I've added a comment to address this.

[awong, 2013-02-05 02:57:11]

Here's the responses to your questions.

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/11308362/diff/132023/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.h:44: // Prior to
GetURLRequestContext() being called, SetProtocolHandlers() is
On 2013/02/04 14:18:54, pauljensen wrote:
> On 2013/02/02 03:26:12, awong wrote:
> > Can we dump this in private and friend AwBrowserContext?
> I could, but the style guide says friends are normally in the same file.  I
> could also move this into an anonymous namespace inside aw_browser_context.cc
> but that would be a lot of code thrown in one file.  Do you still want me to
> privatize SetProtocolHandlers() and friend AwBrowserContext?
> > 
> > Also, am I right that this is supposed to be called on the UI thread and
that
> > this is needed because we can't just create the JobFactory on the UI thread?

> If
> > so, please explain in comment.  The comment is currently good at explaining
> waht
> > is happening, but it doesn't answer why we need this.
> Done.

Yah, I'd still friend it.  Style guide doesn't ban it, and leaving it public
makes it even harder to know who can touch it.

https://codereview.chromium.org/11308362/diff/132023/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11308362/diff/132023/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:256:
BrowserContext::GetStoragePartition(this, NULL);
On 2013/02/04 14:18:54, pauljensen wrote:
> On 2013/02/02 03:26:12, awong wrote:
> > Prefer GetDefaultStoragePartition() to using NULL.
> Please respond to my prior comment on this matter here:
>
https://codereview.chromium.org/11308362/diff/82003/chrome/browser/profiles/p...

Oh sorry. I missed that. Yes, use it. I should modify that comment.  Doing
GetSP(this, NULL) is equiv to GetDefaultSP() with all the badness except that
now I can't as easily grep for all the violators.

https://codereview.chromium.org/11308362/diff/147003/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.h (right):

https://codereview.chromium.org/11308362/diff/147003/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.h:41: // be set for incognito
profiles. Called on the UI thread, though the protocol
Just saying "Called on the UI Thread." is probably good enough here.

[pauljensen, 2013-02-05 15:49:24]

I addressed your comments Albert, thanks!
Albert & Matt, PTAL.

[awong, 2013-02-05 18:10:33]

LGTM w/ 2 comment nits

BTW, Matt, if you are reading through it again, I found it much easier to go
through files by releated refactors rather than just sequentially.  So

(1) browser_context.{h,cc} with {.*browser_context.{h,cc}, profile.h}

(2) content_browser_client.{h,cc} with .*content_browser_client.{h,cc}

(3) All the .*url_request_context_getter.{h,cc}

(4) storage_partition_impl_map.cc with url_data_manager_backend.{h,cc}

(5) {profile, off_the_record}_io_data.*

And then pick up the stragglers.

https://codereview.chromium.org/11308362/diff/165060/android_webview/browser/...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/11308362/diff/165060/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.h:75: //
ProtocolHandlers are stored here between SetProtocolHandlers() and
-> between the SetProtocolHandlers() and the first GetURLRequestContext() call.

https://codereview.chromium.org/11308362/diff/165060/content/browser/storage_...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11308362/diff/165060/content/browser/storage_...
content/browser/storage_partition_impl_map.cc:470:
partition->SetURLRequestContext(
nit: Can we make this an if-statement rather than a ternary operator?  It's long
enough that it's hard to read...also it made me look up the spec on the rules
about expression evaluation on ternary operators to ensure the .Pass() worked as
expected (which it does -- *whew*).

Anyways. doing an if would be easier to read.

[awong, 2013-02-05 18:10:41]

LGTM w/ 2 comment nits

BTW, Matt, if you are reading through it again, I found it much easier to go
through files by releated refactors rather than just sequentially.  So

(1) browser_context.{h,cc} with {.*browser_context.{h,cc}, profile.h}

(2) content_browser_client.{h,cc} with .*content_browser_client.{h,cc}

(3) All the .*url_request_context_getter.{h,cc}

(4) storage_partition_impl_map.cc with url_data_manager_backend.{h,cc}

(5) {profile, off_the_record}_io_data.*

And then pick up the stragglers.

[mmenke, 2013-02-05 22:22:38]

On 2013/02/05 18:10:41, awong wrote:
> LGTM w/ 2 comment nits
> 
> BTW, Matt, if you are reading through it again, I found it much easier to go
> through files by releated refactors rather than just sequentially.  So
> 
> (1) browser_context.{h,cc} with {.*browser_context.{h,cc}, profile.h}
> 
> (2) content_browser_client.{h,cc} with .*content_browser_client.{h,cc}
> 
> (3) All the .*url_request_context_getter.{h,cc}
> 
> (4) storage_partition_impl_map.cc with url_data_manager_backend.{h,cc}
> 
> (5) {profile, off_the_record}_io_data.*
> 
> And then pick up the stragglers.

I'll skim through this tonight, and if I find no issues, I'll go ahead and sign
off on it, deferring to awong to some extent (Who's now also a ProfileIOData
owner).

[pauljensen, 2013-02-06 13:39:26]

I addressed Albert's two nits.

[mmenke, 2013-02-06 16:11:26]

Just don't have time time to give this a thorough review, I defer to awong's
signoff.

https://codereview.chromium.org/11308362/diff/173007/android_webview/browser/...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/173007/android_webview/browser/...
android_webview/browser/aw_browser_context.h:111: class AwResourceContext;
This isn't needed, is it?  Can just store resource_context_ as a
ResourceContext.

[pauljensen, 2013-02-06 22:29:46]

jam@, could you please review the content/ files?
jochen@, could you please review the chrome/ files?
joth@, could you please review the android_webview/ files?
awong@ has thoroughly reviewed the change as a whole.  I should mention this is
essentially a refactoring with no user-visible behavior changes.

[joth, 2013-02-07 00:41:41]

aw/ lgtm on basis this is knock on of already reviewed content changes, but a
couple minor thoughts I had in passing while reading this...

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:186:
chrome_devtools_protocol_handler) {
comment why all the protocol handlers are ignored?

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/aw_browser_context.h:86: net::URLRequestContextGetter*
CreateRequestContext(
nit: we currently have all non-virtual (non-overriden) methods ordered before
virtuals. so move up to line 60

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/aw_browser_context.h:96:
chrome_devtools_protocol_handler);
seems a list of 5 params of same type would be easy to get mixed up. worth
making a struct for these, to allow them to be passed by name?
(but of course, that will defeat the scoped_ptr::Pass() mechanism...)

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:106: return
browser_context_->CreateRequestContext(
might as well DCHECK(browser_context_ == browser_context) here and below.

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.cc:119: job_factory_ =
CreateAndroidJobFactoryAndCookieMonster(
maybe a comment here explaining this creates a chain of job_factories, hence why
we repeatedly assign to the same member. (Took me a moment to work out).

Also, any reason to use "job_factory_ =" first time but then
job_factory_.reset() on the subsequent two calls? (ah... functions returning
scoped_ptr vs raw pointer. tricky).

[awong, 2013-02-07 00:44:38]

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/aw_browser_context.h:96:
chrome_devtools_protocol_handler);
On 2013/02/07 00:41:41, joth wrote:
> seems a list of 5 params of same type would be easy to get mixed up. worth
> making a struct for these, to allow them to be passed by name?
> (but of course, that will defeat the scoped_ptr::Pass() mechanism...)

A struct is possible, but the long term plan (read: 2-month plan) is to move
most of this into content so you don't need to see it here.  We're just
untangling one piece of the knot that is URLRequestContext.  Thus, i'm biased
towards leaving as is.

[joth, 2013-02-07 01:09:58]

On 6 February 2013 16:44, <ajwong@chromium.org> wrote:

>
> https://codereview.chromium.**org/11308362/diff/168007/**
>
android_webview/browser/aw_**browser_context.h<https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/aw_browser_context.h>
> File android_webview/browser/aw_**browser_context.h (right):
>
> https://codereview.chromium.**org/11308362/diff/168007/**
>
android_webview/browser/aw_**browser_context.h#newcode96<https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/aw_browser_context.h#newcode96>
> android_webview/browser/aw_**browser_context.h:96:
> chrome_devtools_protocol_**handler);
>
> On 2013/02/07 00:41:41, joth wrote:
>
>> seems a list of 5 params of same type would be easy to get mixed up.
>>
> worth
>
>> making a struct for these, to allow them to be passed by name?
>> (but of course, that will defeat the scoped_ptr::Pass() mechanism...)
>>
>
> A struct is possible, but the long term plan (read: 2-month plan) is to
> move most of this into content so you don't need to see it here.  We're
> just untangling one piece of the knot that is URLRequestContext.  Thus,
> i'm biased towards leaving as is.
>
>
sgtm

I felt like I was peering through a small window onto a much bigger picture
I didn't understand

>
https://codereview.chromium.**org/11308362/<https://codereview.chromium.org/1...
>

[pauljensen, 2013-02-07 14:10:54]

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:186:
chrome_devtools_protocol_handler) {
On 2013/02/07 00:41:41, joth wrote:
> comment why all the protocol handlers are ignored?
Since we're returning the same URLRequestContextGetter for StoragePartitions as
for the main one (like prior behavior), I don't think we want to be adding
StoragePartition-specific ProtocolHandlers to the main URLRequestContext.

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/aw_browser_context.h:86: net::URLRequestContextGetter*
CreateRequestContext(
On 2013/02/07 00:41:41, joth wrote:
> nit: we currently have all non-virtual (non-overriden) methods ordered before
> virtuals. so move up to line 60

Done.

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:106: return
browser_context_->CreateRequestContext(
On 2013/02/07 00:41:41, joth wrote:
> might as well DCHECK(browser_context_ == browser_context) here and below.

Done.

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/11308362/diff/168007/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.cc:119: job_factory_ =
CreateAndroidJobFactoryAndCookieMonster(
On 2013/02/07 00:41:41, joth wrote:
> maybe a comment here explaining this creates a chain of job_factories, hence
why
> we repeatedly assign to the same member. (Took me a moment to work out).
> 
> Also, any reason to use "job_factory_ =" first time but then
> job_factory_.reset() on the subsequent two calls? (ah... functions returning
> scoped_ptr vs raw pointer. tricky).

Done.

[jam, 2013-02-07 17:17:22]

content lgtm

i'm really looking forward to when the code in chrome that deals with creating
the url request stuff moves to storage partition :)

[jochen (gone - plz use gerrit), 2013-02-08 07:51:32]

chrome/ lgtm

[commit-bot: I haz the power, 2013-02-08 15:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11308362/155025

[commit-bot: I haz the power, 2013-02-08 18:17:14]

Change committed as 181519