Allow content layer to pass ProtocolInterceptors when we create URLRequestContext

Allow content layer to pass ProtocolInterceptors when we create URLRequestContext

Currently we pass protocol_handlers that belong to content layer
in StoragePartitionImplMap when we create URLRequestContexts for
storage partitions.

This patch also allows content layer to pass additional protocol
interceptors, so that content modules / web platform modules
(namely, ServiceWorker) can insert its own protocol interceptors.

*** Most changes are just for plumbing to pass through one more arg ***

* The additional protocol interceptors are created in:
 content/browser/storage_partition_impl_map.cc
 (where we also create protocol_handlers for content modules,
 e.g. blob, filesystem etc)

* which is passed to: ChromeContentBrowserClient::CreateRequestContext
 -> Profile -> ProfileIOData, and then

* chained to the URLJobFactory for a newly created
 URLRequestContext in ProfileIOData::SetUpJobFactoryDefaults().

BUG=349319
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256055

[kinuko, 2014-03-05 05:58:34]

(This patch was an outcome of discussion for request interception handling in
https://codereview.chromium.org/177733003/)

The patch may look big, but mostly just for plumbing
(except for changes in profile_io_data.cc). PTL

mmenke@: could you review ProfileIOData related changes in
chrome/browser/profiles?

avi@: could you review content/ changes, mainly in: content/public/browser
public API change (most of other changes come from there)

Thanks,

[Avi (use Gerrit), 2014-03-05 16:40:31]

I have no idea if passing around a bunch of interceptors is the right thing to
do. Assuming that it is, then the plumbing LGTM.

[kinuko, 2014-03-06 02:30:00]

Per TODOs (and crbug.com/159193) it looks URLRequestContext getters are planned
to be moved into StoragePartition, I'm cc-ing some related folks (ajwong@,
miket@) as they may have opinion. (If the migrations happens it's great for us
too as we wouldn't need this type of plumbing, but we want to have this change
now)

mmenke@: For ProfileIOData related changes could you review? Or should I ask
someone else?

Thanks,

[mkosiba (inactive), 2014-03-06 16:13:48]

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

https://codereview.chromium.org/187223003/diff/90001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:215:
content::ProtocolHandlerScopedVector protocol_interceptors) {
are we supposed to hook these up ourselves? if so please file a bug.

[mmenke, 2014-03-06 19:28:21]

I'm the right guy for the profile changes.

profile/ and browser/net LGTM.  Also looked over the rest.  This will require
some hookups in AnrdoidWebView, but I don't think we should worry about that in
this CL.

Do think it's a good idea to wait for ajwong's opinion.

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

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:273: protocol_handlers,
protocol_interceptors.Pass()).get();
nit:  I don't think I've ever seen this indentation style before (Yes, it was
used in this file before).  I suggest just using a 4-space indent relative to
the "return" instead, here and below.

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_io_data.h:110: OVERRIDE;
nit (x3):  I think const and OVERRIDE should be on the same line (Like
AcquireIsolatedMediaRequestContext).  AcquireIsolatedMediaRequestContext indents
them 4 more as well.  Should be consistent here, but I'm fine with either style.

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

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:873:
g_browser_process->io_thread()).get();
See earlier indentation comment (x2)

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.h:158: OVERRIDE;
See earlier const OVERRIDE linewrap comment (x3)

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1068: // Setup iterators in the
reverse order.
nit:  "Set up" (Setup is a noun).

https://codereview.chromium.org/187223003/diff/90001/content/shell/browser/sh...
File content/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/187223003/diff/90001/content/shell/browser/sh...
content/shell/browser/shell_url_request_context_getter.cc:220: // Setup
iterators in the reverse order.
nit: "Set up"

[michaeln, 2014-03-06 20:27:37]

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:615: // TODO TODOTODO_kinuko
odd todo?

[kinuko, 2014-03-07 08:25:01]

Thanks, addressed comments.

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

https://codereview.chromium.org/187223003/diff/90001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:215:
content::ProtocolHandlerScopedVector protocol_interceptors) {
On 2014/03/06 16:13:49, mkosiba wrote:
> are we supposed to hook these up ourselves? if so please file a bug.

Sorry... I wanted this part to be separated from this CL. Filed a bug and added
TODO.

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

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:273: protocol_handlers,
protocol_interceptors.Pass()).get();
On 2014/03/06 19:28:22, mmenke wrote:
> nit:  I don't think I've ever seen this indentation style before (Yes, it was
> used in this file before).  I suggest just using a 4-space indent relative to
> the "return" instead, here and below.

Done.

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_io_data.h:110: OVERRIDE;
On 2014/03/06 19:28:22, mmenke wrote:
> nit (x3):  I think const and OVERRIDE should be on the same line (Like
> AcquireIsolatedMediaRequestContext).  AcquireIsolatedMediaRequestContext
indents
> them 4 more as well.  Should be consistent here, but I'm fine with either
style.

This style didn't look very familiar to me either but this was done by git cl
format... to be consistent I changed the style to have const + OVERRIDE with 4
space indent.

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

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:873:
g_browser_process->io_thread()).get();
On 2014/03/06 19:28:22, mmenke wrote:
> See earlier indentation comment (x2)

Done. (Here and below)

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:615: // TODO TODOTODO_kinuko
On 2014/03/06 20:27:38, michaeln wrote:
> odd todo?

Oops... removed.

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.h:158: OVERRIDE;
On 2014/03/06 19:28:22, mmenke wrote:
> See earlier const OVERRIDE linewrap comment (x3)

Done. (x3)

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/187223003/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:1068: // Setup iterators in the
reverse order.
On 2014/03/06 19:28:22, mmenke wrote:
> nit:  "Set up" (Setup is a noun).

Done.

https://codereview.chromium.org/187223003/diff/90001/content/shell/browser/sh...
File content/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/187223003/diff/90001/content/shell/browser/sh...
content/shell/browser/shell_url_request_context_getter.cc:220: // Setup
iterators in the reverse order.
On 2014/03/06 19:28:22, mmenke wrote:
> nit: "Set up"

Done.

[kinuko, 2014-03-07 08:38:36]

(moving ajwong@ from cc to reviewers to hear his opinion on this.)

ajwong@:

We want to pass protocol interceptors from content layer when we create
URLRequestContext, and this patch's adding one more 'interceptors' param to
ContentBrowserClient::CreateRequestContext.

It looks there's a plan to move URLRequestContext getters to StoragePartition
(crbug.com/159193), which is great for our purpose too, but it doesn't seem to
be happening very soon so I'm making this change.  I want to make sure if it
doesn't step in the overall plan though--- Wdyt? Does this change look ok to
you?

Thanks

[awong, 2014-03-07 16:32:43]

I'm okay with the ContentBrowserClient change. As you said, nothing's happening
with URLRequestContext and StoragePartition anytime soon.

On 2014/03/07 08:38:36, kinuko wrote:
> (moving ajwong@ from cc to reviewers to hear his opinion on this.)
> 
> ajwong@:
> 
> We want to pass protocol interceptors from content layer when we create
> URLRequestContext, and this patch's adding one more 'interceptors' param to
> ContentBrowserClient::CreateRequestContext.
> 
> It looks there's a plan to move URLRequestContext getters to StoragePartition
> (crbug.com/159193), which is great for our purpose too, but it doesn't seem to
> be happening very soon so I'm making this change.  I want to make sure if it
> doesn't step in the overall plan though--- Wdyt? Does this change look ok to
> you?
> 
> Thanks

[kinuko, 2014-03-07 17:02:51]

On 2014/03/07 16:32:43, awong wrote:
> I'm okay with the ContentBrowserClient change. As you said, nothing's
happening
> with URLRequestContext and StoragePartition anytime soon.

Thanks!

[kinuko, 2014-03-07 17:06:18]

mkosiba@: could you do owner review for android_webview/ changes?

kalman@: could you do owner review for apps/ changes?

sky@: could you do owner review for chrome/browser/ui, chrome/test/, ui/
changes?

[awong, 2014-03-07 17:15:13]

Guess I should say LGTM on the URLRequestContext creation stuff.


On Fri, Mar 7, 2014 at 8:32 AM, <ajwong@chromium.org> wrote:

> I'm okay with the ContentBrowserClient change. As you said, nothing's
> happening
> with URLRequestContext and StoragePartition anytime soon.
>
>
> On 2014/03/07 08:38:36, kinuko wrote:
>
>> (moving ajwong@ from cc to reviewers to hear his opinion on this.)
>>
>
>  ajwong@:
>>
>
>  We want to pass protocol interceptors from content layer when we create
>> URLRequestContext, and this patch's adding one more 'interceptors' param
>> to
>> ContentBrowserClient::CreateRequestContext.
>>
>
>  It looks there's a plan to move URLRequestContext getters to
>> StoragePartition
>> (crbug.com/159193), which is great for our purpose too, but it doesn't
>> seem to
>> be happening very soon so I'm making this change.  I want to make sure if
>> it
>> doesn't step in the overall plan though--- Wdyt? Does this change look ok
>> to
>> you?
>>
>
>  Thanks
>>
>
>
>
> https://codereview.chromium.org/187223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-03-07 17:35:40]

https://codereview.chromium.org/187223003/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/test/fake_profile.h (right):

https://codereview.chromium.org/187223003/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/test/fake_profile.h:92:
content::ProtocolHandlerScopedVector protocol_interceptors) OVERRIDE;
Why aren't you passing a const& here? If receivers need to mutate then it should
be a pointer to a ProtocolHandlerScopedVector

[not at google - send to devlin, 2014-03-07 17:36:43]

apps plumbing lgtm

[sky, 2014-03-07 17:37:11]

https://codereview.chromium.org/187223003/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/test/fake_profile.h (right):

https://codereview.chromium.org/187223003/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/test/fake_profile.h:92:
content::ProtocolHandlerScopedVector protocol_interceptors) OVERRIDE;
On 2014/03/07 17:35:41, sky wrote:
> Why aren't you passing a const& here? If receivers need to mutate then it
should
> be a pointer to a ProtocolHandlerScopedVector 

Actually, if callers don't need to mutate than a const std::vector<XXX>& would
be better since the Scoped part is really an implementation detail.

[mkosiba (inactive), 2014-03-07 17:48:20]

android_webview/ LGTM

[mmenke, 2014-03-07 17:53:50]

https://codereview.chromium.org/187223003/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/test/fake_profile.h (right):

https://codereview.chromium.org/187223003/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/test/fake_profile.h:92:
content::ProtocolHandlerScopedVector protocol_interceptors) OVERRIDE;
On 2014/03/07 17:37:12, sky wrote:
> On 2014/03/07 17:35:41, sky wrote:
> > Why aren't you passing a const& here? If receivers need to mutate then it
> should
> > be a pointer to a ProtocolHandlerScopedVector 
> 
> Actually, if callers don't need to mutate than a const std::vector<XXX>& would
> be better since the Scoped part is really an implementation detail.

This is just like passing a scoped_ptr, only to a vector.  It indicates (And
enforces) that the function takes ownership of the vector, and its contents.

[kinuko, 2014-03-08 03:37:43]

https://codereview.chromium.org/187223003/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/test/fake_profile.h (right):

https://codereview.chromium.org/187223003/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/test/fake_profile.h:92:
content::ProtocolHandlerScopedVector protocol_interceptors) OVERRIDE;
On 2014/03/07 17:53:51, mmenke wrote:
> On 2014/03/07 17:37:12, sky wrote:
> > On 2014/03/07 17:35:41, sky wrote:
> > > Why aren't you passing a const& here? If receivers need to mutate then it
> > should
> > > be a pointer to a ProtocolHandlerScopedVector 
> > 
> > Actually, if callers don't need to mutate than a const std::vector<XXX>&
would
> > be better since the Scoped part is really an implementation detail.
> 
> This is just like passing a scoped_ptr, only to a vector.  It indicates (And
> enforces) that the function takes ownership of the vector, and its contents.

sky@: As Matt wrote I used scoped vector here to indicate the ownership
transfer, i.e. the vector's ownership is to be transferred from caller to
callee.  Neither const& or pointer can indicate it clearer than scoped vector
and I want to keep this signature (for clarity).  Does it make sense to you?

[sky, 2014-03-10 13:34:05]

Ok, LGTM

[kinuko, 2014-03-10 13:39:22]

On 2014/03/10 13:34:05, sky wrote:
> Ok, LGTM

Thanks!

[kinuko, 2014-03-10 13:39:29]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-10 13:39:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/187223003/110001

[commit-bot: I haz the power, 2014-03-10 14:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/187223003/110001

[commit-bot: I haz the power, 2014-03-10 15:42:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/187223003/110001

[commit-bot: I haz the power, 2014-03-10 17:38:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/187223003/110001

[commit-bot: I haz the power, 2014-03-10 21:27:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/187223003/110001

[commit-bot: I haz the power, 2014-03-10 22:00:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/187223003/110001

[commit-bot: I haz the power, 2014-03-10 22:57:13]

Change committed as 256055