Removed static factories for data, ftp, file, and about jobs.

Removed static factories for data, ftp, file, and about jobs.
Instead add corresponding ProtocolHandlers as needed.
Remove URLRequestContext members used by these static 
factories.  Bake FtpAuthCache into FtpProtocolHandler as it
was already unique per FtpProtocolHandler.
This is a revived version of http://crrev.com/10836206

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

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

[pauljensen, 2013-01-21 03:59:18]

PTAL.

[erikwright (departed), 2013-01-21 16:20:12]

LGTM.

LVGTM!

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

https://codereview.chromium.org/11931024/diff/31004/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:255: #if
!defined(DISABLE_FTP_SUPPORT)
I think the remainder of this #if block can be canned, as it seems to have
existed only to call set_ftp_transaction_factory.

https://codereview.chromium.org/11931024/diff/31004/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:301: #if
!defined(DISABLE_FTP_SUPPORT)
Don't think this is needed, for the same reasons mentioned above.

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

https://codereview.chromium.org/11931024/diff/31004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:467: #if
!defined(DISABLE_FTP_SUPPORT)
As mentioned.

https://codereview.chromium.org/11931024/diff/31004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:555: #if
!defined(DISABLE_FTP_SUPPORT)
As mentioned.

https://codereview.chromium.org/11931024/diff/31004/content/shell/shell_conte...
File content/shell/shell_content_browser_client.cc (right):

https://codereview.chromium.org/11931024/diff/31004/content/shell/shell_conte...
content/shell/shell_content_browser_client.cc:83: bool
ShellContentBrowserClient::IsHandledURL(const GURL& url) {
At some point I'd like to understand why we can't just ask the
URLRequestJobFactory instance this question.

I don't suggest changing it in this CL, but I think this is ripe for
simplification.

https://codereview.chromium.org/11931024/diff/31004/net/url_request/ftp_proto...
File net/url_request/ftp_protocol_handler.h (right):

https://codereview.chromium.org/11931024/diff/31004/net/url_request/ftp_proto...
net/url_request/ftp_protocol_handler.h:25: URLRequest* request, NetworkDelegate*
network_delegate) const OVERRIDE;
I think an argument could be made that MaybeCreateJob shouldn't be const.

I wouldn't suggest changing it now, but this CL highlights the fact that a job
factory can change by creating jobs, as the job that is created now can
potentially change the behaviour of a job that is created later.

[pauljensen, 2013-01-22 14:25:03]

I addresed Erik's comments.

https://codereview.chromium.org/11931024/diff/31004/content/shell/shell_conte...
File content/shell/shell_content_browser_client.cc (right):

https://codereview.chromium.org/11931024/diff/31004/content/shell/shell_conte...
content/shell/shell_content_browser_client.cc:83: bool
ShellContentBrowserClient::IsHandledURL(const GURL& url) {
On 2013/01/21 16:20:13, erikwright wrote:
> At some point I'd like to understand why we can't just ask the
> URLRequestJobFactory instance this question.
> 
> I don't suggest changing it in this CL, but I think this is ripe for
> simplification.
I agree it's ripe for simplification but in another CL.  I wonder if it's called
on the IO thread.  We may also need to add a BrowserContext* argument to
ContentBrowserClient::IsHandledURL().

https://codereview.chromium.org/11931024/diff/31004/net/url_request/ftp_proto...
File net/url_request/ftp_protocol_handler.h (right):

https://codereview.chromium.org/11931024/diff/31004/net/url_request/ftp_proto...
net/url_request/ftp_protocol_handler.h:25: URLRequest* request, NetworkDelegate*
network_delegate) const OVERRIDE;
On 2013/01/21 16:20:13, erikwright wrote:
> I think an argument could be made that MaybeCreateJob shouldn't be const.
> 
> I wouldn't suggest changing it now, but this CL highlights the fact that a job
> factory can change by creating jobs, as the job that is created now can
> potentially change the behaviour of a job that is created later.
Ya, it was annoying here too:
https://codereview.chromium.org/11308362/diff/70001/content/browser/storage_p...

[mmenke, 2013-01-22 16:50:37]

https://codereview.chromium.org/11931024/diff/36003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (left):

https://codereview.chromium.org/11931024/diff/36003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:340: chrome::kMetadataScheme,
Why are you removing this?

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

https://codereview.chromium.org/11931024/diff/36003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:337: chrome::kChromeDevToolsScheme,
Why change the order of this and kExtensionResourceScheme?

https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
File chrome/service/net/service_url_request_context.cc (left):

https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
chrome/service/net/service_url_request_context.cc:118: new
net::FtpNetworkLayer(host_resolver()));
Not supporting FTP in the service process isn't an issue?

https://codereview.chromium.org/11931024/diff/36003/content/browser/child_pro...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/11931024/diff/36003/content/browser/child_pro...
content/browser/child_process_security_policy_unittest.cc:67: // protocols as
this is the responsibility of the browser (who is
nit:  who -> which

https://codereview.chromium.org/11931024/diff/36003/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/11931024/diff/36003/content/browser/renderer_...
content/browser/renderer_host/render_view_host_unittest.cc:29: virtual bool
IsHandledURL(const GURL& url) {
OVERRIDE

https://codereview.chromium.org/11931024/diff/36003/content/browser/renderer_...
content/browser/renderer_host/render_view_host_unittest.cc:42: virtual void
SetUp() {
OVERRIDE

https://codereview.chromium.org/11931024/diff/36003/content/browser/renderer_...
content/browser/renderer_host/render_view_host_unittest.cc:48: virtual void
TearDown() {
OVERRIDE

https://codereview.chromium.org/11931024/diff/36003/net/proxy/proxy_script_fe...
File net/proxy/proxy_script_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11931024/diff/36003/net/proxy/proxy_script_fe...
net/proxy/proxy_script_fetcher_impl_unittest.cc:48: // A non-mock URL request
which can access http:// and file:// urls.
This comment is no longer correct.  Should either fix the comment, or move
protocol handler initialization into it.

https://codereview.chromium.org/11931024/diff/36003/net/url_request/ftp_proto...
File net/url_request/ftp_protocol_handler.h (right):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/ftp_proto...
net/url_request/ftp_protocol_handler.h:29: mutable FtpAuthCache ftp_auth_cache_;
Can FtpAuthCache now be NET_EXPORT_PRIVATE?

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
File net/url_request/url_request_context_builder.h (left):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
net/url_request/url_request_context_builder.h:89: void set_ftp_enabled(bool
enable) {
Should we actually keep this, and create a job factory, with all the needed
protocol handlers (ftp, file, and data)?

Believe this class was created for external consumers, and this CL may break
them, if they depend on this class.

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:1022:
!request_->context()->job_factory()->IsHandledURL(location)) {
I believe this may change behavior.  In particular, I believe we allow redirects
to chrome-app:// URLs (Or something like that.  Can't remember the exact
scheme).  If we have ProtocolHandlers for them, we'll now return false here.

[mmenke, 2013-01-22 16:52:47]

https://codereview.chromium.org/11931024/diff/31004/content/shell/shell_conte...
File content/shell/shell_content_browser_client.cc (right):

https://codereview.chromium.org/11931024/diff/31004/content/shell/shell_conte...
content/shell/shell_content_browser_client.cc:83: bool
ShellContentBrowserClient::IsHandledURL(const GURL& url) {
On 2013/01/22 14:25:03, pauljensen wrote:
> On 2013/01/21 16:20:13, erikwright wrote:
> > At some point I'd like to understand why we can't just ask the
> > URLRequestJobFactory instance this question.
> > 
> > I don't suggest changing it in this CL, but I think this is ripe for
> > simplification.
> I agree it's ripe for simplification but in another CL.  I wonder if it's
called
> on the IO thread.  We may also need to add a BrowserContext* argument to
> ContentBrowserClient::IsHandledURL().

I'm pretty sure the ContentBrowserClient lives on the UI thread.

[willchan no longer on Chromium, 2013-01-23 17:14:54]

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
File net/url_request/url_request_context_builder.h (left):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
net/url_request/url_request_context_builder.h:89: void set_ftp_enabled(bool
enable) {
On 2013/01/22 16:50:37, Matt Menke wrote:
> Should we actually keep this, and create a job factory, with all the needed
> protocol handlers (ftp, file, and data)?
> 
> Believe this class was created for external consumers, and this CL may break
> them, if they depend on this class.

Minor obvious compile-time breakage is OK. They'll be able to figure it out.

I'd really like to avoid non-chromium devs from having to know how to
instantiate a URLRequestJobFactoryImpl. I'd like for them know to have to know
about it at all. They should be able to just use a setter in
URLRequestContextBuilder to have it create the appropriate URLRequestContext. I
think adding setters for file:// and data:// would be good too. It's not obvious
to me what the default should be, but I'd err on the side of leaving it off,
since I think most consumers just want a HTTP stack.

[pauljensen, 2013-01-23 21:43:33]

I addressed all Matt's comments with either code changes or in a couple cases
with follow-up questions.

https://codereview.chromium.org/11931024/diff/31004/content/shell/shell_conte...
File content/shell/shell_content_browser_client.cc (right):

https://codereview.chromium.org/11931024/diff/31004/content/shell/shell_conte...
content/shell/shell_content_browser_client.cc:83: bool
ShellContentBrowserClient::IsHandledURL(const GURL& url) {
On 2013/01/22 16:52:47, Matt Menke wrote:
> On 2013/01/22 14:25:03, pauljensen wrote:
> > On 2013/01/21 16:20:13, erikwright wrote:
> > > At some point I'd like to understand why we can't just ask the
> > > URLRequestJobFactory instance this question.
> > > 
> > > I don't suggest changing it in this CL, but I think this is ripe for
> > > simplification.
> > I agree it's ripe for simplification but in another CL.  I wonder if it's
> called
> > on the IO thread.  We may also need to add a BrowserContext* argument to
> > ContentBrowserClient::IsHandledURL().
> 
> I'm pretty sure the ContentBrowserClient lives on the UI thread.
I debugged a little and ContentBrowserClient::IsHandledURL() seems to be called
on both the IO and UI threads which is one reason we can't make this simply call
URLRequestJobFactory.

https://codereview.chromium.org/11931024/diff/36003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (left):

https://codereview.chromium.org/11931024/diff/36003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:340: chrome::kMetadataScheme,
On 2013/01/22 16:50:37, Matt Menke wrote:
> Why are you removing this?
It was added in r85556 along with the ProtocolHandler, but the ProtocolHandler
was later removed in r90962 but the entry here was never removed.  I was
removing it so as to stay in sync.

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

https://codereview.chromium.org/11931024/diff/36003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:337: chrome::kChromeDevToolsScheme,
On 2013/01/22 16:50:37, Matt Menke wrote:
> Why change the order of this and kExtensionResourceScheme?
To better match ProfileIOData::SetUpJobFactoryDefaults (which has the comment
that this function should stay in sync).  This makes it easier to keep this list
in sync.

https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
File chrome/service/net/service_url_request_context.cc (left):

https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
chrome/service/net/service_url_request_context.cc:118: new
net::FtpNetworkLayer(host_resolver()));
On 2013/01/22 16:50:37, Matt Menke wrote:
> Not supporting FTP in the service process isn't an issue?

I don't really know.  The only service I know of is cloud_print and a quick grep
doesn't show any ftp there ("git grep -i ftp -- cloud_print" returns nothing). 
Can you recommend a good person to ask? I'd hate to add a
URLRequestJobFactoryImpl with all the default ProtocolHandlers if they aren't
used.

https://codereview.chromium.org/11931024/diff/36003/net/url_request/ftp_proto...
File net/url_request/ftp_protocol_handler.h (right):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/ftp_proto...
net/url_request/ftp_protocol_handler.h:29: mutable FtpAuthCache ftp_auth_cache_;
On 2013/01/22 16:50:37, Matt Menke wrote:
> Can FtpAuthCache now be NET_EXPORT_PRIVATE?
I think it has been since r85818.

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
File net/url_request/url_request_context_builder.h (left):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
net/url_request/url_request_context_builder.h:89: void set_ftp_enabled(bool
enable) {
On 2013/01/23 17:14:55, willchan wrote:
> On 2013/01/22 16:50:37, Matt Menke wrote:
> > Should we actually keep this, and create a job factory, with all the needed
> > protocol handlers (ftp, file, and data)?
> > 
> > Believe this class was created for external consumers, and this CL may break
> > them, if they depend on this class.
> 
> Minor obvious compile-time breakage is OK. They'll be able to figure it out.
> 
> I'd really like to avoid non-chromium devs from having to know how to
> instantiate a URLRequestJobFactoryImpl. I'd like for them know to have to know
> about it at all. They should be able to just use a setter in
> URLRequestContextBuilder to have it create the appropriate URLRequestContext.
I
> think adding setters for file:// and data:// would be good too. It's not
obvious
> to me what the default should be, but I'd err on the side of leaving it off,
> since I think most consumers just want a HTTP stack.

I added a URLRequestJobFactoryImpl with file, data and ftp ProtocolHandlers all
controlled via setters.

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:1022:
!request_->context()->job_factory()->IsHandledURL(location)) {
On 2013/01/22 16:50:37, Matt Menke wrote:
> I believe this may change behavior.  In particular, I believe we allow
redirects
> to chrome-app:// URLs (Or something like that.  Can't remember the exact
> scheme).  If we have ProtocolHandlers for them, we'll now return false here.
What makes a redirect "unsafe"?  The only description I see is at the definition
of ERR_UNSAFE_REDIRECT where we mention file:// being unsafe.

The comment 2 lines above this says we should allow redirects to "externally
handled protocols".  Does this mean things registered with
navigator.registerProtocolHandler()?  In which case I think my change would also
change/break this behavior.

The chrome-app:// CL never landed: http://crrev.com/11519027
chrome-app:// was added to
ChildProcessSecurityPolicyImpl::ChildProcessSecurityPolicyImpl()'s list of
explicitly safe schemes, so maybe it should be added here to kSafeSchemes as
part of http://crrev.com/11519027

[mmenke, 2013-01-23 22:09:16]

https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
File chrome/service/net/service_url_request_context.cc (left):

https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
chrome/service/net/service_url_request_context.cc:118: new
net::FtpNetworkLayer(host_resolver()));
On 2013/01/23 21:43:33, pauljensen wrote:
> On 2013/01/22 16:50:37, Matt Menke wrote:
> > Not supporting FTP in the service process isn't an issue?
> 
> I don't really know.  The only service I know of is cloud_print and a quick
grep
> doesn't show any ftp there ("git grep -i ftp -- cloud_print" returns nothing).

> Can you recommend a good person to ask? I'd hate to add a
> URLRequestJobFactoryImpl with all the default ProtocolHandlers if they aren't
> used.

I'd suggest just sending out an email to all 6 of the chrome/service/OWNERS, and
making sure they don't need any of the 3 protocols we're removing default
support for.

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:1022:
!request_->context()->job_factory()->IsHandledURL(location)) {
On 2013/01/23 21:43:33, pauljensen wrote:
> On 2013/01/22 16:50:37, Matt Menke wrote:
> > I believe this may change behavior.  In particular, I believe we allow
> redirects
> > to chrome-app:// URLs (Or something like that.  Can't remember the exact
> > scheme).  If we have ProtocolHandlers for them, we'll now return false here.
> What makes a redirect "unsafe"?  The only description I see is at the
definition
> of ERR_UNSAFE_REDIRECT where we mention file:// being unsafe.
> 
> The comment 2 lines above this says we should allow redirects to "externally
> handled protocols".  Does this mean things registered with
> navigator.registerProtocolHandler()?  In which case I think my change would
also
> change/break this behavior.
> 
> The chrome-app:// CL never landed: http://crrev.com/11519027
> chrome-app:// was added to
> ChildProcessSecurityPolicyImpl::ChildProcessSecurityPolicyImpl()'s list of
> explicitly safe schemes, so maybe it should be added here to kSafeSchemes as
> part of http://crrev.com/11519027
> 

I believe it encompasses everything we don't want websites to be able to
redirect to that's also handled by the network stack.  Other protocols are
supposed to be handled elsewhere.

One option would be to add an IsSafeRedirect function to ProtocolHandlers, and
call the function of the protocol handler, if there is one.  If there isn't,
return true.

Default for a ProtocolHandler would be to return true (Again, with the
assumption that things outside of net are handled by logic elsewhere).  Long
term, think it would be better for them return false by default.

Not sure that this is ideal behavior, just a way to maintain current behavior,
which seems to be kind of a mess.  I suspect it's primarily intended for non-RDH
requests and external consumers of our network stack, but I'm honestly not sure.


Adding chrome-app as a safe scheme here would be a laying violation.

[erikwright (departed), 2013-01-23 22:13:42]

Alternatively, giving the HTTP protocol handler its own
URLRequestJobFactory that it uses to fulfill redirects, and ensuring that
this was a subset of the main factory.


On Wed, Jan 23, 2013 at 5:09 PM, <mmenke@chromium.org> wrote:

>
> https://codereview.chromium.**org/11931024/diff/36003/**
>
chrome/service/net/service_**url_request_context.cc<https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/service_url_request_context.cc>
> File chrome/service/net/service_**url_request_context.cc (left):
>
> https://codereview.chromium.**org/11931024/diff/36003/**
>
chrome/service/net/service_**url_request_context.cc#**oldcode118<https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/service_url_request_context.cc#oldcode118>
> chrome/service/net/service_**url_request_context.cc:118: new
> net::FtpNetworkLayer(host_**resolver()));
> On 2013/01/23 21:43:33, pauljensen wrote:
>
>> On 2013/01/22 16:50:37, Matt Menke wrote:
>> > Not supporting FTP in the service process isn't an issue?
>>
>
>  I don't really know.  The only service I know of is cloud_print and a
>>
> quick grep
>
>> doesn't show any ftp there ("git grep -i ftp -- cloud_print" returns
>>
> nothing).
>
>> Can you recommend a good person to ask? I'd hate to add a
>> URLRequestJobFactoryImpl with all the default ProtocolHandlers if they
>>
> aren't
>
>> used.
>>
>
> I'd suggest just sending out an email to all 6 of the
> chrome/service/OWNERS, and making sure they don't need any of the 3
> protocols we're removing default support for.
>
>
> https://codereview.chromium.**org/11931024/diff/36003/net/**
>
url_request/url_request_http_**job.cc<https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_request_http_job.cc>
> File net/url_request/url_request_**http_job.cc (right):
>
> https://codereview.chromium.**org/11931024/diff/36003/net/**
>
url_request/url_request_http_**job.cc#newcode1022<https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_request_http_job.cc#newcode1022>
> net/url_request/url_request_**http_job.cc:1022:
> !request_->context()->job_**factory()->IsHandledURL(**location)) {
> On 2013/01/23 21:43:33, pauljensen wrote:
>
>> On 2013/01/22 16:50:37, Matt Menke wrote:
>> > I believe this may change behavior.  In particular, I believe we
>>
> allow
>
>> redirects
>> > to chrome-app:// URLs (Or something like that.  Can't remember the
>>
> exact
>
>> > scheme).  If we have ProtocolHandlers for them, we'll now return
>>
> false here.
>
>> What makes a redirect "unsafe"?  The only description I see is at the
>>
> definition
>
>> of ERR_UNSAFE_REDIRECT where we mention file:// being unsafe.
>>
>
>  The comment 2 lines above this says we should allow redirects to
>>
> "externally
>
>> handled protocols".  Does this mean things registered with
>> navigator.**registerProtocolHandler()?  In which case I think my change
>>
> would also
>
>> change/break this behavior.
>>
>
>  The chrome-app:// CL never landed: http://crrev.com/11519027
>> chrome-app:// was added to
>> ChildProcessSecurityPolicyImpl**::**ChildProcessSecurityPolicyImpl**()'s
>>
> list of
>
>> explicitly safe schemes, so maybe it should be added here to
>>
> kSafeSchemes as
>
>> part of http://crrev.com/11519027
>>
>
>
> I believe it encompasses everything we don't want websites to be able to
> redirect to that's also handled by the network stack.  Other protocols
> are supposed to be handled elsewhere.
>
> One option would be to add an IsSafeRedirect function to
> ProtocolHandlers, and call the function of the protocol handler, if
> there is one.  If there isn't, return true.
>
> Default for a ProtocolHandler would be to return true (Again, with the
> assumption that things outside of net are handled by logic elsewhere).
> Long term, think it would be better for them return false by default.
>
> Not sure that this is ideal behavior, just a way to maintain current
> behavior, which seems to be kind of a mess.  I suspect it's primarily
> intended for non-RDH requests and external consumers of our network
> stack, but I'm honestly not sure.
>
>
> Adding chrome-app as a safe scheme here would be a laying violation.
>
>
https://codereview.chromium.**org/11931024/<https://codereview.chromium.org/1...
>

[pauljensen, 2013-01-28 19:51:49]

https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
File chrome/service/net/service_url_request_context.cc (left):

https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
chrome/service/net/service_url_request_context.cc:118: new
net::FtpNetworkLayer(host_resolver()));
On 2013/01/23 22:09:17, Matt Menke wrote:
> On 2013/01/23 21:43:33, pauljensen wrote:
> > On 2013/01/22 16:50:37, Matt Menke wrote:
> > > Not supporting FTP in the service process isn't an issue?
> > 
> > I don't really know.  The only service I know of is cloud_print and a quick
> grep
> > doesn't show any ftp there ("git grep -i ftp -- cloud_print" returns
nothing).
> 
> > Can you recommend a good person to ask? I'd hate to add a
> > URLRequestJobFactoryImpl with all the default ProtocolHandlers if they
aren't
> > used.
> 
> I'd suggest just sending out an email to all 6 of the chrome/service/OWNERS,
and
> making sure they don't need any of the 3 protocols we're removing default
> support for.

Done.  Everybody said they're just using HTTP/HTTPS so it looks like we can
leave out ftp, data and file support from ServiceURLRequestContext.

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:1022:
!request_->context()->job_factory()->IsHandledURL(location)) {
On 2013/01/23 22:09:17, Matt Menke wrote:
> On 2013/01/23 21:43:33, pauljensen wrote:
> > On 2013/01/22 16:50:37, Matt Menke wrote:
> > > I believe this may change behavior.  In particular, I believe we allow
> > redirects
> > > to chrome-app:// URLs (Or something like that.  Can't remember the exact
> > > scheme).  If we have ProtocolHandlers for them, we'll now return false
here.
> > What makes a redirect "unsafe"?  The only description I see is at the
> definition
> > of ERR_UNSAFE_REDIRECT where we mention file:// being unsafe.
> > 
> > The comment 2 lines above this says we should allow redirects to "externally
> > handled protocols".  Does this mean things registered with
> > navigator.registerProtocolHandler()?  In which case I think my change would
> also
> > change/break this behavior.
> > 
> > The chrome-app:// CL never landed: http://crrev.com/11519027
> > chrome-app:// was added to
> > ChildProcessSecurityPolicyImpl::ChildProcessSecurityPolicyImpl()'s list of
> > explicitly safe schemes, so maybe it should be added here to kSafeSchemes as
> > part of http://crrev.com/11519027
> > 
> 
> I believe it encompasses everything we don't want websites to be able to
> redirect to that's also handled by the network stack.  Other protocols are
> supposed to be handled elsewhere.
> 
> One option would be to add an IsSafeRedirect function to ProtocolHandlers, and
> call the function of the protocol handler, if there is one.  If there isn't,
> return true.
> 
> Default for a ProtocolHandler would be to return true (Again, with the
> assumption that things outside of net are handled by logic elsewhere).  Long
> term, think it would be better for them return false by default.
> 
> Not sure that this is ideal behavior, just a way to maintain current behavior,
> which seems to be kind of a mess.  I suspect it's primarily intended for
non-RDH
> requests and external consumers of our network stack, but I'm honestly not
sure.
> 
> 
> Adding chrome-app as a safe scheme here would be a laying violation.
Why wouldn't we want websites to redirect to something?

[mmenke, 2013-01-28 19:52:58]

On 2013/01/28 19:51:49, pauljensen wrote:
>
https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
> File chrome/service/net/service_url_request_context.cc (left):
> 
>
https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
> chrome/service/net/service_url_request_context.cc:118: new
> net::FtpNetworkLayer(host_resolver()));
> On 2013/01/23 22:09:17, Matt Menke wrote:
> > On 2013/01/23 21:43:33, pauljensen wrote:
> > > On 2013/01/22 16:50:37, Matt Menke wrote:
> > > > Not supporting FTP in the service process isn't an issue?
> > > 
> > > I don't really know.  The only service I know of is cloud_print and a
quick
> > grep
> > > doesn't show any ftp there ("git grep -i ftp -- cloud_print" returns
> nothing).
> > 
> > > Can you recommend a good person to ask? I'd hate to add a
> > > URLRequestJobFactoryImpl with all the default ProtocolHandlers if they
> aren't
> > > used.
> > 
> > I'd suggest just sending out an email to all 6 of the chrome/service/OWNERS,
> and
> > making sure they don't need any of the 3 protocols we're removing default
> > support for.
> 
> Done.  Everybody said they're just using HTTP/HTTPS so it looks like we can
> leave out ftp, data and file support from ServiceURLRequestContext.
> 
>
https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
> File net/url_request/url_request_http_job.cc (right):
> 
>
https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
> net/url_request/url_request_http_job.cc:1022:
> !request_->context()->job_factory()->IsHandledURL(location)) {
> On 2013/01/23 22:09:17, Matt Menke wrote:
> > On 2013/01/23 21:43:33, pauljensen wrote:
> > > On 2013/01/22 16:50:37, Matt Menke wrote:
> > > > I believe this may change behavior.  In particular, I believe we allow
> > > redirects
> > > > to chrome-app:// URLs (Or something like that.  Can't remember the exact
> > > > scheme).  If we have ProtocolHandlers for them, we'll now return false
> here.
> > > What makes a redirect "unsafe"?  The only description I see is at the
> > definition
> > > of ERR_UNSAFE_REDIRECT where we mention file:// being unsafe.
> > > 
> > > The comment 2 lines above this says we should allow redirects to
"externally
> > > handled protocols".  Does this mean things registered with
> > > navigator.registerProtocolHandler()?  In which case I think my change
would
> > also
> > > change/break this behavior.
> > > 
> > > The chrome-app:// CL never landed: http://crrev.com/11519027
> > > chrome-app:// was added to
> > > ChildProcessSecurityPolicyImpl::ChildProcessSecurityPolicyImpl()'s list of
> > > explicitly safe schemes, so maybe it should be added here to kSafeSchemes
as
> > > part of http://crrev.com/11519027
> > > 
> > 
> > I believe it encompasses everything we don't want websites to be able to
> > redirect to that's also handled by the network stack.  Other protocols are
> > supposed to be handled elsewhere.
> > 
> > One option would be to add an IsSafeRedirect function to ProtocolHandlers,
and
> > call the function of the protocol handler, if there is one.  If there isn't,
> > return true.
> > 
> > Default for a ProtocolHandler would be to return true (Again, with the
> > assumption that things outside of net are handled by logic elsewhere).  Long
> > term, think it would be better for them return false by default.
> > 
> > Not sure that this is ideal behavior, just a way to maintain current
behavior,
> > which seems to be kind of a mess.  I suspect it's primarily intended for
> non-RDH
> > requests and external consumers of our network stack, but I'm honestly not
> sure.
> > 
> > 
> > Adding chrome-app as a safe scheme here would be a laying violation.
> Why wouldn't we want websites to redirect to something?

Erm...Huh?

[mmenke, 2013-01-28 19:54:53]

On 2013/01/28 19:52:58, Matt Menke wrote:
> On 2013/01/28 19:51:49, pauljensen wrote:
> >
>
https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
> > File chrome/service/net/service_url_request_context.cc (left):
> > 
> >
>
https://codereview.chromium.org/11931024/diff/36003/chrome/service/net/servic...
> > chrome/service/net/service_url_request_context.cc:118: new
> > net::FtpNetworkLayer(host_resolver()));
> > On 2013/01/23 22:09:17, Matt Menke wrote:
> > > On 2013/01/23 21:43:33, pauljensen wrote:
> > > > On 2013/01/22 16:50:37, Matt Menke wrote:
> > > > > Not supporting FTP in the service process isn't an issue?
> > > > 
> > > > I don't really know.  The only service I know of is cloud_print and a
> quick
> > > grep
> > > > doesn't show any ftp there ("git grep -i ftp -- cloud_print" returns
> > nothing).
> > > 
> > > > Can you recommend a good person to ask? I'd hate to add a
> > > > URLRequestJobFactoryImpl with all the default ProtocolHandlers if they
> > aren't
> > > > used.
> > > 
> > > I'd suggest just sending out an email to all 6 of the
chrome/service/OWNERS,
> > and
> > > making sure they don't need any of the 3 protocols we're removing default
> > > support for.
> > 
> > Done.  Everybody said they're just using HTTP/HTTPS so it looks like we can
> > leave out ftp, data and file support from ServiceURLRequestContext.
> > 
> >
>
https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
> > File net/url_request/url_request_http_job.cc (right):
> > 
> >
>
https://codereview.chromium.org/11931024/diff/36003/net/url_request/url_reque...
> > net/url_request/url_request_http_job.cc:1022:
> > !request_->context()->job_factory()->IsHandledURL(location)) {
> > On 2013/01/23 22:09:17, Matt Menke wrote:
> > > On 2013/01/23 21:43:33, pauljensen wrote:
> > > > On 2013/01/22 16:50:37, Matt Menke wrote:
> > > > > I believe this may change behavior.  In particular, I believe we allow
> > > > redirects
> > > > > to chrome-app:// URLs (Or something like that.  Can't remember the
exact
> > > > > scheme).  If we have ProtocolHandlers for them, we'll now return false
> > here.
> > > > What makes a redirect "unsafe"?  The only description I see is at the
> > > definition
> > > > of ERR_UNSAFE_REDIRECT where we mention file:// being unsafe.
> > > > 
> > > > The comment 2 lines above this says we should allow redirects to
> "externally
> > > > handled protocols".  Does this mean things registered with
> > > > navigator.registerProtocolHandler()?  In which case I think my change
> would
> > > also
> > > > change/break this behavior.
> > > > 
> > > > The chrome-app:// CL never landed: http://crrev.com/11519027
> > > > chrome-app:// was added to
> > > > ChildProcessSecurityPolicyImpl::ChildProcessSecurityPolicyImpl()'s list
of
> > > > explicitly safe schemes, so maybe it should be added here to
kSafeSchemes
> as
> > > > part of http://crrev.com/11519027
> > > > 
> > > 
> > > I believe it encompasses everything we don't want websites to be able to
> > > redirect to that's also handled by the network stack.  Other protocols are
> > > supposed to be handled elsewhere.
> > > 
> > > One option would be to add an IsSafeRedirect function to ProtocolHandlers,
> and
> > > call the function of the protocol handler, if there is one.  If there
isn't,
> > > return true.
> > > 
> > > Default for a ProtocolHandler would be to return true (Again, with the
> > > assumption that things outside of net are handled by logic elsewhere). 
Long
> > > term, think it would be better for them return false by default.
> > > 
> > > Not sure that this is ideal behavior, just a way to maintain current
> behavior,
> > > which seems to be kind of a mess.  I suspect it's primarily intended for
> > non-RDH
> > > requests and external consumers of our network stack, but I'm honestly not
> > sure.
> > > 
> > > 
> > > Adding chrome-app as a safe scheme here would be a laying violation.
> > Why wouldn't we want websites to redirect to something?
> 
> Erm...Huh?

Sorry, misread that as "why would we...".

We wouldn't want them to redirect to local files or to chrome pages, for
security reasons.  In the network stack, we just want to prevent redirection to
unsafe protocols that the network stack handles, and return true to everything
else...Or at least that's what the old code did.

[pauljensen, 2013-02-15 15:28:15]

I've addressed all the remaining issues.  Matt, PTAL.

[mmenke, 2013-02-19 17:26:15]

https://codereview.chromium.org/11931024/diff/58005/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/11931024/diff/58005/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:211: return
job_factory_->IsSafeRedirectTarget(location);
This seems weird to me.  If io_thread_delegate_ has a ProtocolHandler for a
scheme, seems weird that we treat it much like URLRequestJobFactory does, except
we never call its IsSafeRedirectTarget function.

The problem is that we're using ProtocolHandlers for a lot of disparate
purposes, and don't really care about all methods for all purposes.  As-is,
think this behavior is rather counter-intuitive.

Could create different classes, as we don't actually use any of them
interchangeably.  Or could replace IsSafeRedirectTarget with
IsUnsafeRedirectTarget, and call them both.  If one returns false, can then just
return false.

I'm open to other ideas.

https://codereview.chromium.org/11931024/diff/58005/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11931024/diff/58005/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:846:
policy->RegisterWebSafeScheme(test_url.scheme());
Why wasn't this needed before?

https://codereview.chromium.org/11931024/diff/58005/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/11931024/diff/58005/content/browser/renderer_...
content/browser/renderer_host/render_view_host_unittest.cc:50:
GetContentClient()->set_browser_for_testing(old_browser_client_);
This should be first, so cleanup order is the reverse of setup order.

https://codereview.chromium.org/11931024/diff/58005/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.h (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.h:24: // NULL) the URLRequest is
forwarded to the |job_factory_| to be handled there.
I think we should mention that only the ProtocolHandler's MaybeCreateJob
function is called - None of its Is* functions is called.

May even want a separate class for this (In another CL), to make this clear.  I
don't think there's any ProtocolHandler used both as an interceptor and as a
full ProtocolHandler, though I could be wrong.

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
net/url_request/url_request_context_builder.h:102: }
There's a compile flag to disable ftp support.  Should we be respecting it here
and in the cc file?

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:1027: return true;
nit:  Use braces when the conditional is more than two lines.

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
File net/url_request/url_request_job_factory.h (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
net/url_request/url_request_job_factory.h:35: // protocols handled by
MaybeCreateJob().
This comment is not correct.  As-is, it sounds like if we have a
ProtocolInterceptJobFactory, then its IsSafeRedirectTarget function is checked
for the jobs it creates.

Instead, we only call it for the single protocol it's the official handler for.

[pauljensen, 2013-02-20 15:13:26]

https://codereview.chromium.org/11931024/diff/58005/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/11931024/diff/58005/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:211: return
job_factory_->IsSafeRedirectTarget(location);
On 2013/02/19 17:26:15, mmenke wrote:
> This seems weird to me.  If io_thread_delegate_ has a ProtocolHandler for a
> scheme, seems weird that we treat it much like URLRequestJobFactory does,
except
> we never call its IsSafeRedirectTarget function.
So there are two ProtocolHandler classes and three different purposes.  There's
::ProtocolHandler which gets registered with ProtocolHandlerRegistry.  This is
to handle things from navigator.registerProtocolHandler().  This does not
contain an IsSafeRedirectTarget function.
> 
> The problem is that we're using ProtocolHandlers for a lot of disparate
> purposes, and don't really care about all methods for all purposes.  As-is,
> think this behavior is rather counter-intuitive.
> 
> Could create different classes, as we don't actually use any of them
> interchangeably.  Or could replace IsSafeRedirectTarget with
> IsUnsafeRedirectTarget, and call them both.  If one returns false, can then
just
> return false.
> 
> I'm open to other ideas.
As I mention in response to a later comment, I think we should add a third
class, called ProtocolInterceptJobFactory::InterceptHandler, so that each class
has only one dedicated purpose.  I think we may want to also change the name of
one of the ProtocolHandler classes so they are not confused.  Maybe change
::ProtocolHandler to either ProtocolHandlerRegistry::ProtocolHandler or maybe
::NavigatorProtocolHandler or maybe ::ExternalProtocolHandler or maybe
::ProtocolRedirecter (as they are basically just URL redirects).  Another
alternative is to change all uses of "Protocol" to "Scheme" in
URLRequestJobFactory; I'm always confused by the fact that IsHandledProtocol()'s
only argument is called "scheme".

I think my vote is to leave things as they are in this CL but have another CL to
add ProtocolInterceptJobFactory::InterceptHandler.

https://codereview.chromium.org/11931024/diff/58005/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11931024/diff/58005/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:846:
policy->RegisterWebSafeScheme(test_url.scheme());
On 2013/02/19 17:26:15, mmenke wrote:
> Why wasn't this needed before?
The two argument version of
ChildProcessSecurityPolicyImpl::CanRequestURL() contains this code:

if (!GetContentClient()->browser()->IsHandledURL(url) &&
    !net::URLRequest::IsHandledURL(url)) {
  return true;  // This URL request is destined for ShellExecute.
}

Since I added ShellContentBrowserClient::IsHandledURL() (so as to pass other
tests) this if-statement is more accurate so to get this test to actuate the
intended functionality I temporarily add chrome:// as a safe scheme so it's
accepted by CanRequestURL().  I've been told in the past to not include history
in comments so I didn't want to mention this history of IsHandledURL().

https://codereview.chromium.org/11931024/diff/58005/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/11931024/diff/58005/content/browser/renderer_...
content/browser/renderer_host/render_view_host_unittest.cc:50:
GetContentClient()->set_browser_for_testing(old_browser_client_);
On 2013/02/19 17:26:15, mmenke wrote:
> This should be first, so cleanup order is the reverse of setup order.

Done.

https://codereview.chromium.org/11931024/diff/58005/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.h (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.h:24: // NULL) the URLRequest is
forwarded to the |job_factory_| to be handled there.
On 2013/02/19 17:26:15, mmenke wrote:
> I think we should mention that only the ProtocolHandler's MaybeCreateJob
> function is called - None of its Is* functions is called.
Done.
> 
> May even want a separate class for this (In another CL), to make this clear. 
I
> don't think there's any ProtocolHandler used both as an interceptor and as a
> full ProtocolHandler, though I could be wrong.
You are correct.  I agree; I think we should create
ProtocolInterceptorJobFactory::InterceptHandler which has only a
MaybeCreateJob() pure virtual method.

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
net/url_request/url_request_context_builder.h:102: }
On 2013/02/19 17:26:15, mmenke wrote:
> There's a compile flag to disable ftp support.  Should we be respecting it
here
> and in the cc file?

Done.

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:1027: return true;
On 2013/02/19 17:26:15, mmenke wrote:
> nit:  Use braces when the conditional is more than two lines.

Done.

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
File net/url_request/url_request_job_factory.h (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/url_reque...
net/url_request/url_request_job_factory.h:35: // protocols handled by
MaybeCreateJob().
On 2013/02/19 17:26:15, mmenke wrote:
> This comment is not correct.  As-is, it sounds like if we have a
> ProtocolInterceptJobFactory, then its IsSafeRedirectTarget function is checked
> for the jobs it creates.
> 
> Instead, we only call it for the single protocol it's the official handler
for.
I adjusted the comment to be more correct.  We can remove the extra part of the
comment when we have ProtocolInterceptJobFactory::InterceptHandler.

[erikwright (departed), 2013-02-20 15:25:48]

https://codereview.chromium.org/11931024/diff/58005/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/11931024/diff/58005/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:211: return
job_factory_->IsSafeRedirectTarget(location);
On 2013/02/20 15:13:27, pauljensen wrote:
> On 2013/02/19 17:26:15, mmenke wrote:
> > This seems weird to me.  If io_thread_delegate_ has a ProtocolHandler for a
> > scheme, seems weird that we treat it much like URLRequestJobFactory does,
> except
> > we never call its IsSafeRedirectTarget function.
> So there are two ProtocolHandler classes and three different purposes. 
There's
> ::ProtocolHandler which gets registered with ProtocolHandlerRegistry.  This is
> to handle things from navigator.registerProtocolHandler().  This does not
> contain an IsSafeRedirectTarget function.
> > 
> > The problem is that we're using ProtocolHandlers for a lot of disparate
> > purposes, and don't really care about all methods for all purposes.  As-is,
> > think this behavior is rather counter-intuitive.
> > 
> > Could create different classes, as we don't actually use any of them
> > interchangeably.  Or could replace IsSafeRedirectTarget with
> > IsUnsafeRedirectTarget, and call them both.  If one returns false, can then
> just
> > return false.
> > 
> > I'm open to other ideas.
> As I mention in response to a later comment, I think we should add a third
> class, called ProtocolInterceptJobFactory::InterceptHandler, so that each
class
> has only one dedicated purpose.  I think we may want to also change the name
of
> one of the ProtocolHandler classes so they are not confused.  Maybe change
> ::ProtocolHandler to either ProtocolHandlerRegistry::ProtocolHandler or maybe
> ::NavigatorProtocolHandler or maybe ::ExternalProtocolHandler or maybe
> ::ProtocolRedirecter (as they are basically just URL redirects).  Another
> alternative is to change all uses of "Protocol" to "Scheme" in
> URLRequestJobFactory; I'm always confused by the fact that
IsHandledProtocol()'s
> only argument is called "scheme".
> 
> I think my vote is to leave things as they are in this CL but have another CL
to
> add ProtocolInterceptJobFactory::InterceptHandler.

It is entirely reasonable to consider the ProtocolHandler interface to be a
detail of the URLRequestJobFactoryImpl.

There is no reason to assume that an instance of the URLRequestJobFactory
interface would be implemented in terms of ProtocolHandlers.

In other words, I support whatever alternative OO design makes sense.

[mmenke, 2013-02-20 15:48:58]

LGTM

https://codereview.chromium.org/11931024/diff/58005/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/11931024/diff/58005/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:211: return
job_factory_->IsSafeRedirectTarget(location);
On 2013/02/20 15:25:48, erikwright wrote:
> On 2013/02/20 15:13:27, pauljensen wrote:
> > On 2013/02/19 17:26:15, mmenke wrote:
> > > This seems weird to me.  If io_thread_delegate_ has a ProtocolHandler for
a
> > > scheme, seems weird that we treat it much like URLRequestJobFactory does,
> > except
> > > we never call its IsSafeRedirectTarget function.
> > So there are two ProtocolHandler classes and three different purposes. 
> There's
> > ::ProtocolHandler which gets registered with ProtocolHandlerRegistry.  This
is
> > to handle things from navigator.registerProtocolHandler().  This does not
> > contain an IsSafeRedirectTarget function.
> > > 
> > > The problem is that we're using ProtocolHandlers for a lot of disparate
> > > purposes, and don't really care about all methods for all purposes. 
As-is,
> > > think this behavior is rather counter-intuitive.
> > > 
> > > Could create different classes, as we don't actually use any of them
> > > interchangeably.  Or could replace IsSafeRedirectTarget with
> > > IsUnsafeRedirectTarget, and call them both.  If one returns false, can
then
> > just
> > > return false.
> > > 
> > > I'm open to other ideas.
> > As I mention in response to a later comment, I think we should add a third
> > class, called ProtocolInterceptJobFactory::InterceptHandler, so that each
> class
> > has only one dedicated purpose.  I think we may want to also change the name
> of
> > one of the ProtocolHandler classes so they are not confused.  Maybe change
> > ::ProtocolHandler to either ProtocolHandlerRegistry::ProtocolHandler or
maybe
> > ::NavigatorProtocolHandler or maybe ::ExternalProtocolHandler or maybe
> > ::ProtocolRedirecter (as they are basically just URL redirects).  Another
> > alternative is to change all uses of "Protocol" to "Scheme" in
> > URLRequestJobFactory; I'm always confused by the fact that
> IsHandledProtocol()'s
> > only argument is called "scheme".
> > 
> > I think my vote is to leave things as they are in this CL but have another
CL
> to
> > add ProtocolInterceptJobFactory::InterceptHandler.
> 
> It is entirely reasonable to consider the ProtocolHandler interface to be a
> detail of the URLRequestJobFactoryImpl.
> 
> There is no reason to assume that an instance of the URLRequestJobFactory
> interface would be implemented in terms of ProtocolHandlers.
> 
> In other words, I support whatever alternative OO design makes sense.

This all SGTM (Leaving it for another CL, separating out ProtocolHandler and
InterceptHandler, making ProtocolHandler a subclass of
URLRequestJobFactoryImpl).

https://codereview.chromium.org/11931024/diff/58005/net/url_request/protocol_...
File net/url_request/protocol_intercept_job_factory.h (right):

https://codereview.chromium.org/11931024/diff/58005/net/url_request/protocol_...
net/url_request/protocol_intercept_job_factory.h:24: // NULL) the URLRequest is
forwarded to the |job_factory_| to be handled there.
On 2013/02/20 15:13:27, pauljensen wrote:
> On 2013/02/19 17:26:15, mmenke wrote:
> > I think we should mention that only the ProtocolHandler's MaybeCreateJob
> > function is called - None of its Is* functions is called.
> Done.
> > 
> > May even want a separate class for this (In another CL), to make this clear.

> I
> > don't think there's any ProtocolHandler used both as an interceptor and as a
> > full ProtocolHandler, though I could be wrong.
> You are correct.  I agree; I think we should create
> ProtocolInterceptorJobFactory::InterceptHandler which has only a
> MaybeCreateJob() pure virtual method.

SGTM

https://codereview.chromium.org/11931024/diff/76001/net/url_request/url_reque...
File net/url_request/url_request_job_factory.h (right):

https://codereview.chromium.org/11931024/diff/76001/net/url_request/url_reque...
net/url_request/url_request_job_factory.h:36: //
URLRequestJobFactoryImpl::SetProtocolHandler(); not called when used with
nit:  Don't think we need the semi-colon.

[pauljensen, 2013-02-22 14:16:58]

Matt, I added an IsSafeRedirectTarget() implementation that always returns false
to the AboutProtocolHandler.  I think this is a good idea as I believe this
could result in access to chrome:// and it matches the prior behavior.  Care to
give it a tiny glance?

[mmenke, 2013-02-22 16:36:27]

On 2013/02/22 14:16:58, pauljensen wrote:
> Matt, I added an IsSafeRedirectTarget() implementation that always returns
false
> to the AboutProtocolHandler.  I think this is a good idea as I believe this
> could result in access to chrome:// and it matches the prior behavior.  Care
to
> give it a tiny glance?

Good idea, LGTM.

We really should have browser tests for these, if we don't already.

[pauljensen, 2013-02-27 14:52:59]

On 2013/02/22 16:36:27, mmenke wrote:
> We really should have browser tests for these, if we don't already.
I don't think there are browser tests.  I tried concocting some using
ui_test_utils::NavigateToURL() but it looks like there isn't visual changes that
occur that I could use content::TitleWatcher to watch for (presumably because
the navigation fails with ERR_UNSAFE_REDIRECT).  Do you know of something I
could watch for?  or should I just manually attempt a request with
browser()->profile()->GetRequestContext()?

[mmenke, 2013-02-27 14:58:09]

On 2013/02/27 14:52:59, pauljensen wrote:
> On 2013/02/22 16:36:27, mmenke wrote:
> > We really should have browser tests for these, if we don't already.
> I don't think there are browser tests.  I tried concocting some using
> ui_test_utils::NavigateToURL() but it looks like there isn't visual changes
that
> occur that I could use content::TitleWatcher to watch for (presumably because
> the navigation fails with ERR_UNSAFE_REDIRECT).  Do you know of something I
> could watch for?  or should I just manually attempt a request with
> browser()->profile()->GetRequestContext()?

If a redirect fails with with ERR_UNSAFE_REDIRECT, we don't actually get an
error page, but instead just cancel the navigation?

[pauljensen, 2013-02-27 15:56:20]

On 2013/02/27 14:58:09, mmenke wrote:
> On 2013/02/27 14:52:59, pauljensen wrote:
> > On 2013/02/22 16:36:27, mmenke wrote:
> > > We really should have browser tests for these, if we don't already.
> > I don't think there are browser tests.  I tried concocting some using
> > ui_test_utils::NavigateToURL() but it looks like there isn't visual changes
> that
> > occur that I could use content::TitleWatcher to watch for (presumably
because
> > the navigation fails with ERR_UNSAFE_REDIRECT).  Do you know of something I
> > could watch for?  or should I just manually attempt a request with
> > browser()->profile()->GetRequestContext()?
> 
> If a redirect fails with with ERR_UNSAFE_REDIRECT, we don't actually get an
> error page, but instead just cancel the navigation?
I think so.  If you "run_testserver --doc-root=." then go to say
http://127.0.0.1:12345, then type
"http://127.0.0.1:12345/server-redirect?chrome://flags" into the navigation box
and hit enter nothing appears to happen.

[mmenke, 2013-02-27 16:01:55]

On 2013/02/27 15:56:20, pauljensen wrote:
> On 2013/02/27 14:58:09, mmenke wrote:
> > On 2013/02/27 14:52:59, pauljensen wrote:
> > > On 2013/02/22 16:36:27, mmenke wrote:
> > > > We really should have browser tests for these, if we don't already.
> > > I don't think there are browser tests.  I tried concocting some using
> > > ui_test_utils::NavigateToURL() but it looks like there isn't visual
changes
> > that
> > > occur that I could use content::TitleWatcher to watch for (presumably
> because
> > > the navigation fails with ERR_UNSAFE_REDIRECT).  Do you know of something
I
> > > could watch for?  or should I just manually attempt a request with
> > > browser()->profile()->GetRequestContext()?
> > 
> > If a redirect fails with with ERR_UNSAFE_REDIRECT, we don't actually get an
> > error page, but instead just cancel the navigation?
> I think so.  If you "run_testserver --doc-root=." then go to say
> http://127.0.0.1:12345, then type
> "http://127.0.0.1:12345/server-redirect?chrome://flags" into the navigation
box
> and hit enter nothing appears to happen.

I had thought we checked if the redirects were allowed before passing them on to
the resource dispatcher host, but that may be incorrect.

You are testing with an official chrome build, and not your own debug build,
right?  (Just to be absolutely sure we didn't break anything).

[pauljensen, 2013-02-27 16:03:09]

On 2013/02/27 16:01:55, mmenke wrote:
> On 2013/02/27 15:56:20, pauljensen wrote:
> > On 2013/02/27 14:58:09, mmenke wrote:
> > > On 2013/02/27 14:52:59, pauljensen wrote:
> > > > On 2013/02/22 16:36:27, mmenke wrote:
> > > > > We really should have browser tests for these, if we don't already.
> > > > I don't think there are browser tests.  I tried concocting some using
> > > > ui_test_utils::NavigateToURL() but it looks like there isn't visual
> changes
> > > that
> > > > occur that I could use content::TitleWatcher to watch for (presumably
> > because
> > > > the navigation fails with ERR_UNSAFE_REDIRECT).  Do you know of
something
> I
> > > > could watch for?  or should I just manually attempt a request with
> > > > browser()->profile()->GetRequestContext()?
> > > 
> > > If a redirect fails with with ERR_UNSAFE_REDIRECT, we don't actually get
an
> > > error page, but instead just cancel the navigation?
> > I think so.  If you "run_testserver --doc-root=." then go to say
> > http://127.0.0.1:12345, then type
> > "http://127.0.0.1:12345/server-redirect?chrome://flags" into the navigation
> box
> > and hit enter nothing appears to happen.
> 
> I had thought we checked if the redirects were allowed before passing them on
to
> the resource dispatcher host, but that may be incorrect.
> 
> You are testing with an official chrome build, and not your own debug build,
> right?  (Just to be absolutely sure we didn't break anything).
Ya; same thing happens with my build or official build.

[mmenke, 2013-02-27 16:16:46]

On 2013/02/27 16:01:55, mmenke wrote:
> On 2013/02/27 15:56:20, pauljensen wrote:
> > On 2013/02/27 14:58:09, mmenke wrote:
> > > On 2013/02/27 14:52:59, pauljensen wrote:
> > > > On 2013/02/22 16:36:27, mmenke wrote:
> > > > > We really should have browser tests for these, if we don't already.
> > > > I don't think there are browser tests.  I tried concocting some using
> > > > ui_test_utils::NavigateToURL() but it looks like there isn't visual
> changes
> > > that
> > > > occur that I could use content::TitleWatcher to watch for (presumably
> > because
> > > > the navigation fails with ERR_UNSAFE_REDIRECT).  Do you know of
something
> I
> > > > could watch for?  or should I just manually attempt a request with
> > > > browser()->profile()->GetRequestContext()?
> > > 
> > > If a redirect fails with with ERR_UNSAFE_REDIRECT, we don't actually get
an
> > > error page, but instead just cancel the navigation?
> > I think so.  If you "run_testserver --doc-root=." then go to say
> > http://127.0.0.1:12345, then type
> > "http://127.0.0.1:12345/server-redirect?chrome://flags" into the navigation
> box
> > and hit enter nothing appears to happen.
> 
> I had thought we checked if the redirects were allowed before passing them on
to
> the resource dispatcher host, but that may be incorrect.
> 
> You are testing with an official chrome build, and not your own debug build,
> right?  (Just to be absolutely sure we didn't break anything).

Ok, setting breakpoints, looks like we call NotifyReceivedRedirect in
URLRequestJob::OnReceivedHeadersComplete, and only call IsSafeRedirect in
URLRequest::Redirect(), which is the next step in the redirect process.

So this code is indeed only really intended for embedders, or possibly as a
failsafe.

Think we're fine landing it as-is.

[pauljensen, 2013-02-27 22:12:06]

Brett, could you review chrome/ and content/ and webkit/ files?
Martin, could you review android_webview/?
This CL has been thoroughly reviewed as a whole by Matt and Erik.  This change
is part of larger refactor aimed at eliminating URLRequestJobManager by avoiding
the dependency on static URLRequestJob::Factories.

[mkosiba (inactive), 2013-02-28 10:57:37]

lgtm for android_webview

[brettw, 2013-02-28 21:53:22]

content/webkit lgtm

[pauljensen, 2013-02-28 22:58:38]

Brett, could you also take a look at the chrome/ files?  You can skip the
chrome/browser/net/ and chrome/browser/profiles/ files as mmenke@ has reviewed
them.

[pauljensen, 2013-03-05 17:36:41]

Brett, ping.

On 2013/02/28 22:58:38, pauljensen wrote:
> Brett, could you also take a look at the chrome/ files?  You can skip the
> chrome/browser/net/ and chrome/browser/profiles/ files as mmenke@ has reviewed
> them.

[brettw, 2013-03-14 04:42:41]

LGTM

[commit-bot: I haz the power, 2013-03-15 16:20:58]

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

[commit-bot: I haz the power, 2013-03-15 16:21:12]

Failed to apply patch for content/shell/shell_url_request_context_getter.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/shell/shell_url_request_context_getter.cc
  Hunk #1 FAILED at 27.
  Hunk #2 succeeded at 176 (offset -1 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/shell/shell_url_request_context_getter.cc.rej

Patch:       content/shell/shell_url_request_context_getter.cc
Index: content/shell/shell_url_request_context_getter.cc
diff --git a/content/shell/shell_url_request_context_getter.cc
b/content/shell/shell_url_request_context_getter.cc
index
eaef4ae0b30b96492db191bcabebb683655dd226..a269fac9489910d3902d8d30ff492f846b5e5df0
100644
--- a/content/shell/shell_url_request_context_getter.cc
+++ b/content/shell/shell_url_request_context_getter.cc
@@ -27,6 +27,8 @@
 #include "net/http/http_network_session.h"
 #include "net/http/http_server_properties_impl.h"
 #include "net/proxy/proxy_service.h"
+#include "net/url_request/data_protocol_handler.h"
+#include "net/url_request/file_protocol_handler.h"
 #include "net/url_request/protocol_intercept_job_factory.h"
 #include "net/url_request/static_http_user_agent_settings.h"
 #include "net/url_request/url_request_context.h"
@@ -177,7 +179,17 @@ net::URLRequestContext*
ShellURLRequestContextGetter::GetURLRequestContext() {
 
     scoped_ptr<net::URLRequestJobFactoryImpl> job_factory(
         new net::URLRequestJobFactoryImpl());
+    // Keep ProtocolHandlers added in sync with
+    // ShellContentBrowserClient::IsHandledURL().
     InstallProtocolHandlers(job_factory.get(), &protocol_handlers_);
+    bool set_protocol = job_factory->SetProtocolHandler(
+        chrome::kDataScheme,
+        new net::DataProtocolHandler);
+    DCHECK(set_protocol);
+    set_protocol = job_factory->SetProtocolHandler(
+        chrome::kFileScheme,
+        new net::FileProtocolHandler);
+    DCHECK(set_protocol);
     storage_->set_job_factory(job_factory.release());
   }

[commit-bot: I haz the power, 2013-03-15 16:38:41]

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

[commit-bot: I haz the power, 2013-03-15 17:59:26]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-15 21:39:20]

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

[commit-bot: I haz the power, 2013-03-16 08:40:38]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-03-17 11:46:54]

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

[commit-bot: I haz the power, 2013-03-17 20:16:36]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-03-19 01:48:19]

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

[commit-bot: I haz the power, 2013-03-19 01:56:47]

Change committed as 188912

[pauljensen, 2013-04-25 19:06:19]

Matt, I think I've revived this CL since it was reverted.  Can you take a look
over these files that have changed slightly since it was originally landed:

net/url_request/ftp_protocol_handler.h
  I just friended a Test class so it can access the FtpAuthCache.
net/url_request/url_request_ftp_job_unittest.cc
  Modified FtpTestURLRequestContext to provide access to FtpAuthCache.
  TestURLRequestFtpJob passes FtpAuthCache and FtpTransactionFactory through to
URLRequestFtpJob.
net/url_request/url_request_unittest.cc
  AddTestInterceptor() clears out old TestJobInterceptor before inserting new
one.
content/browser/renderer_host/render_view_host_unittest.cc
  Switch to new SetBrowserClientForTesting function.
content/shell/shell_url_request_context_getter.cc
  Remove FtpTransactionFactory code.
webkit/tools/test_shell/test_shell_request_context.cc
  Add DataProtocolHandler.  This is what was causing the layout test failures
that caused the revert.

I don't think my change causes any layout test failures anymore.  I've never had
a failure free run of layout tests...but I'm confident my change now does not
cause any persistent failures.

[mmenke, 2013-04-25 20:06:43]

On 2013/04/25 19:06:19, pauljensen wrote:
> Matt, I think I've revived this CL since it was reverted.  Can you take a look
> over these files that have changed slightly since it was originally landed:
> 
> net/url_request/ftp_protocol_handler.h
>   I just friended a Test class so it can access the FtpAuthCache.
> net/url_request/url_request_ftp_job_unittest.cc
>   Modified FtpTestURLRequestContext to provide access to FtpAuthCache.
>   TestURLRequestFtpJob passes FtpAuthCache and FtpTransactionFactory through
to
> URLRequestFtpJob.
> net/url_request/url_request_unittest.cc
>   AddTestInterceptor() clears out old TestJobInterceptor before inserting new
> one.
> content/browser/renderer_host/render_view_host_unittest.cc
>   Switch to new SetBrowserClientForTesting function.
> content/shell/shell_url_request_context_getter.cc
>   Remove FtpTransactionFactory code.
> webkit/tools/test_shell/test_shell_request_context.cc
>   Add DataProtocolHandler.  This is what was causing the layout test failures
> that caused the revert.
> 
> I don't think my change causes any layout test failures anymore.  I've never
had
> a failure free run of layout tests...but I'm confident my change now does not
> cause any persistent failures.

LGTM.  Just looked at the new changes (Skimmed over the others, just in case
anything was left out of the list.  Didn't see anything else significant).

[pauljensen, 2013-04-25 21:36:13]

Brett, could you look over these three files that have changed slightly since
this change was reverted?  Matt just reviewed them.

content/browser/renderer_host/render_view_host_unittest.cc
  Switch to new SetBrowserClientForTesting function.
content/shell/shell_url_request_context_getter.cc
  Remove FtpTransactionFactory code.
webkit/tools/test_shell/test_shell_request_context.cc
  Add DataProtocolHandler.  This is what was causing the layout test failures
that caused the revert.

[Paweł Hajdan Jr., 2013-04-29 18:45:25]

Some comments from the net/ftp OWNER.

I'd like to remind you to get my final approval before you commit this. Thanks.

https://codereview.chromium.org/11931024/diff/199001/net/url_request/ftp_prot...
File net/url_request/ftp_protocol_handler.h (right):

https://codereview.chromium.org/11931024/diff/199001/net/url_request/ftp_prot...
net/url_request/ftp_protocol_handler.h:31: mutable FtpAuthCache ftp_auth_cache_;
This seems wrong. If MaybeCreateJob is supposed to be const, this looks like
cheating.

Note that it's fine to use mutable for things _unrelated_ to the abstraction
like some bookkeeping/debugging/logging etc. However, FtpAuthCache is definitely
related to the core of what is going on here.

https://codereview.chromium.org/11931024/diff/199001/net/url_request/url_requ...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/11931024/diff/199001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:103: class
MockFtpTransactionFactory : public FtpTransactionFactory {
Please don't use gmock for FTP unit tests.

[pauljensen, 2013-04-29 20:30:52]

Pawel, I think I addressed your comments, PTAL.  I couldn't agree more with the
mutable comment, I can't figure out my original motivation for using mutable,
probably my subconscious aversion to forcing things to be allocated from the
heap.

[Paweł Hajdan Jr., 2013-04-29 21:09:10]

FTP code LGTM, thanks!

[pauljensen, 2013-04-29 21:15:38]

Matt, could you take a look at the diffs between patch-set 30 and 31 that I just
put in?

[mmenke1, 2013-04-29 21:17:22]

https://codereview.chromium.org/11931024/diff/213001/net/url_request/url_requ...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/11931024/diff/213001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:105: FtpTransaction*
CreateTransaction() {
virtual OVERRIDE

https://codereview.chromium.org/11931024/diff/213001/net/url_request/url_requ...
net/url_request/url_request_ftp_job_unittest.cc:109: void Suspend(bool suspend)
{}
virtual OVERRIDE

[pauljensen, 2013-04-29 21:24:40]

Matt, PTAL.

[mmenke, 2013-04-29 21:27:56]

On 2013/04/29 21:24:40, pauljensen wrote:
> Matt, PTAL.

LGTM

[pauljensen, 2013-04-29 21:29:26]

Brett, could you look over these three files that have changed slightly since
this change was reverted?  Matt reviewed them.

content/browser/renderer_host/render_view_host_unittest.cc
  Switch to new SetBrowserClientForTesting function.
content/shell/shell_url_request_context_getter.cc
  Remove FtpTransactionFactory code.
webkit/tools/test_shell/test_shell_request_context.cc
  Add DataProtocolHandler.  This is what was causing the layout test failures
that caused the revert.

[brettw, 2013-04-30 21:39:34]

I think the existing high-level owners LGTM should be fine since you only
changed a few details, so I'll just go with Matt's review.

[commit-bot: I haz the power, 2013-05-01 14:45:37]

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

[commit-bot: I haz the power, 2013-05-01 14:53:31]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[pauljensen, 2013-05-01 15:01:06]

James, could you review this file:
  webkit/tools/test_shell/test_shell_request_context.cc
Brett reviewed it in the past but is no longer an OWNER of it.

[jamesr, 2013-05-01 17:30:22]

webkit/ lgtm

[commit-bot: I haz the power, 2013-05-01 18:05:56]

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

[commit-bot: I haz the power, 2013-05-01 18:24:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pauljensen, 2013-05-01 19:33:59]

Matt, could you review the empty destructor I added to FtpProtocolHandler?

[mmenke, 2013-05-01 19:35:12]

On 2013/05/01 19:33:59, pauljensen wrote:
> Matt, could you review the empty destructor I added to FtpProtocolHandler?

LGTM

[commit-bot: I haz the power, 2013-05-01 19:36:08]

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

[commit-bot: I haz the power, 2013-05-01 20:11:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pauljensen, 2013-05-01 21:42:42]

Matt, could you please review the IsSafeForRedirect() I added to
chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc

[mmenke, 2013-05-01 21:49:35]

On 2013/05/01 21:42:42, pauljensen wrote:
> Matt, could you please review the IsSafeForRedirect() I added to
> chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc

I'm going to have to remind myself of the old behavior if IsSafe redirect, out
of paranoia.  I'll get back to you tomorrow.

[pauljensen, 2013-05-02 14:12:40]

Matt, I should mention the implementation of IsSafeForRedirect() I just added is
in a unittest.  I just made it act like the old
URLRequestHttpJob::IsSafeRedirect() (see diff for url_request_http_job.cc).

On 2013/05/01 21:49:35, mmenke wrote:
> On 2013/05/01 21:42:42, pauljensen wrote:
> > Matt, could you please review the IsSafeForRedirect() I added to
> > chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc
> 
> I'm going to have to remind myself of the old behavior if IsSafe redirect, out
> of paranoia.  I'll get back to you tomorrow.

[mmenke, 2013-05-02 14:52:57]

On 2013/05/02 14:12:40, pauljensen wrote:
> Matt, I should mention the implementation of IsSafeForRedirect() I just added
is
> in a unittest.  I just made it act like the old
> URLRequestHttpJob::IsSafeRedirect() (see diff for url_request_http_job.cc).

Right, just wanted to make sure we're being consistent.  Since we're mocking out
HTTP requests, looks like it's irrelevant, though true is the accurate response.

LGTM

[commit-bot: I haz the power, 2013-05-02 18:53:48]

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

[commit-bot: I haz the power, 2013-05-02 21:25:26]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-03 00:37:28]

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

[commit-bot: I haz the power, 2013-05-03 08:05:13]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-03 22:36:52]

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

[commit-bot: I haz the power, 2013-05-03 22:37:08]

Failed to apply patch for net/proxy/proxy_script_fetcher_impl_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file net/proxy/proxy_script_fetcher_impl_unittest.cc
  Hunk #1 FAILED at 21.
  1 out of 2 hunks FAILED -- saving rejects to file
net/proxy/proxy_script_fetcher_impl_unittest.cc.rej

Patch:       net/proxy/proxy_script_fetcher_impl_unittest.cc
Index: net/proxy/proxy_script_fetcher_impl_unittest.cc
diff --git a/net/proxy/proxy_script_fetcher_impl_unittest.cc
b/net/proxy/proxy_script_fetcher_impl_unittest.cc
index
87afb27c36b38220cc7d67e2ac24e2d3ef64e6ef..991f0000b582e3e4a82cfb5983c065b4e4874fa5
100644
--- a/net/proxy/proxy_script_fetcher_impl_unittest.cc
+++ b/net/proxy/proxy_script_fetcher_impl_unittest.cc
@@ -21,6 +21,7 @@
 #include "net/http/http_server_properties_impl.h"
 #include "net/ssl/ssl_config_service_defaults.h"
 #include "net/test/test_server.h"
+#include "net/url_request/file_protocol_handler.h"
 #include "net/url_request/url_request_context_storage.h"
 #include "net/url_request/url_request_file_job.h"
 #include "net/url_request/url_request_job_factory_impl.h"
@@ -66,7 +67,9 @@ class RequestContext : public URLRequestContext {
     storage_.set_http_transaction_factory(new HttpCache(
         network_session,
         HttpCache::DefaultBackend::InMemory(0)));
-    storage_.set_job_factory(new URLRequestJobFactoryImpl());
+    URLRequestJobFactoryImpl* job_factory = new URLRequestJobFactoryImpl();
+    job_factory->SetProtocolHandler("file", new FileProtocolHandler());
+    storage_.set_job_factory(job_factory);
   }
 
   virtual ~RequestContext() {

[pauljensen, 2013-05-04 03:05:47]

Scott, Matt has thoroughly reviewed this CL as a whole and Brett reviewed the
chrome/ code in this CL but just removed himself from the OWNERS file.  Could
you review the chrome/ code?

[sky, 2013-05-06 15:48:09]

If Brett already reviewed this I'm rubber stamping it: LGTM

[commit-bot: I haz the power, 2013-05-07 19:06:46]

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

[commit-bot: I haz the power, 2013-05-07 19:08:30]

Failed to apply patch for
chrome/browser/profiles/off_the_record_profile_io_data.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/profiles/off_the_record_profile_io_data.cc
  Hunk #3 FAILED at 260.
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/profiles/off_the_record_profile_io_data.cc.rej

Patch:       chrome/browser/profiles/off_the_record_profile_io_data.cc
Index: chrome/browser/profiles/off_the_record_profile_io_data.cc
diff --git a/chrome/browser/profiles/off_the_record_profile_io_data.cc
b/chrome/browser/profiles/off_the_record_profile_io_data.cc
index
4029bc33c7ee9f0b0099d904a8c8d1c6a2055d8c..034f7bb20fa4f3768d6cb447f4e6871506c15ce1
100644
--- a/chrome/browser/profiles/off_the_record_profile_io_data.cc
+++ b/chrome/browser/profiles/off_the_record_profile_io_data.cc
@@ -216,7 +216,6 @@ void OffTheRecordProfileIOData::InitializeInternal(
 #if !defined(DISABLE_FTP_SUPPORT)
   ftp_factory_.reset(
       new net::FtpNetworkLayer(main_context->host_resolver()));
-  main_context->set_ftp_transaction_factory(ftp_factory_.get());
 #endif  // !defined(DISABLE_FTP_SUPPORT)
 
   scoped_ptr<net::URLRequestJobFactoryImpl> main_job_factory(
@@ -227,8 +226,7 @@ void OffTheRecordProfileIOData::InitializeInternal(
       main_job_factory.Pass(),
       profile_params->protocol_handler_interceptor.Pass(),
       network_delegate(),
-      main_context->ftp_transaction_factory(),
-      main_context->ftp_auth_cache());
+      ftp_factory_.get());
   main_context->set_job_factory(main_job_factory_.get());
 
 #if defined(ENABLE_EXTENSIONS)
@@ -262,11 +260,6 @@ void OffTheRecordProfileIOData::
   extensions_cookie_store->SetCookieableSchemes(schemes, 2);
   extensions_context->set_cookie_store(extensions_cookie_store);
 
-#if !defined(DISABLE_FTP_SUPPORT)
-  DCHECK(ftp_factory_.get());
-  extensions_context->set_ftp_transaction_factory(ftp_factory_.get());
-#endif  // !defined(DISABLE_FTP_SUPPORT)
-
   scoped_ptr<net::URLRequestJobFactoryImpl> extensions_job_factory(
       new net::URLRequestJobFactoryImpl());
   // TODO(shalev): The extensions_job_factory has a NULL NetworkDelegate.
@@ -279,8 +272,7 @@ void OffTheRecordProfileIOData::
       extensions_job_factory.Pass(),
       scoped_ptr<ProtocolHandlerRegistry::JobInterceptorFactory>(NULL),
       NULL,
-      extensions_context->ftp_transaction_factory(),
-      extensions_context->ftp_auth_cache());
+      ftp_factory_.get());
   extensions_context->set_job_factory(extensions_job_factory_.get());
 }
 
@@ -318,8 +310,7 @@ OffTheRecordProfileIOData::InitializeAppRequestContext(
   top_job_factory = SetUpJobFactoryDefaults(job_factory.Pass(),
                                            
protocol_handler_interceptor.Pass(),
                                             network_delegate(),
-                                            context->ftp_transaction_factory(),
-                                            context->ftp_auth_cache());
+                                            ftp_factory_.get());
   context->SetJobFactory(top_job_factory.Pass());
   return context;
 }

[commit-bot: I haz the power, 2013-05-07 19:56:26]

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

[commit-bot: I haz the power, 2013-05-07 20:24:27]

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

[commit-bot: I haz the power, 2013-05-08 15:06:44]

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

[commit-bot: I haz the power, 2013-05-08 16:04:37]

Change committed as 198915