Convert ProtocolHandlerRegistry::Interceptor to a net::URLRequestJobFactory.

Convert ProtocolHandlerRegistry::Interceptor to a net::URLRequestJobFactory.

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

[pauljensen, 2012-12-27 15:46:25]

Erik, PTAL.

[erikwright (departed), 2013-01-02 16:29:36]

LGTM with an idea for consideration.

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

https://codereview.chromium.org/11669012/diff/15001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:163: // requests via
|ProtocolHandlerRegistry::CreateURLInterceptor|.
Update name of CreateURLInterceptor

https://codereview.chromium.org/11669012/diff/15001/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/11669012/diff/15001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:137: // is
initialized on the UI thread. Callers assume responsibility for deleting
The switch to returning a scoped_ptr makes the "Callers assume..." redundant.

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

https://codereview.chromium.org/11669012/diff/15001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:661:
protocol_handler_interceptor->Chain(
Passing a custom type around, and configuring the wrapped factory as a deferred
step is possibly the right thing to do here, but if this pattern recurs you
could end up with a lot of boiler-plate code.

If that were the case, this pattern could be simplified as follows:

1) Make the protocol handler interceptor not delegate to another factory.
2) Pass the protocol handler interceptor as a generic URLRequestJobFactory
3) Define a simple URLRequestJobFactory implementation that delegates to two
child factories, in order.
4) Here, create the delegating factory and pass it the interceptor as its first
child and job_factory as its second child.

Again, I wouldn't necessarily go to the trouble just for this case, but if this
pattern recurs it would seem to be worthwhile to centralize it. The other
advantages are:

a) One less custom type exposed.
b) Simpler life-time for the protocol handler interceptor (it doesn't have to
check in every method for a chained factory).

[willchan no longer on Chromium, 2013-01-02 17:32:01]

I'm nervous about this one. I'm on vacation, but I'm a bit tempted to say
you should wait until I get back before landing this. IIRC, there are some
subtle threading and security issues that tests won't catch. The security
issues pertain to IsHandledProtocol() or whatever it's called.
Threading...there are a lot of threading nuances there. Tread carefully and
make sure you run the TSan try bots.

If the reviewers are comfortable with this and you have other changes
blocked on this one, then proceed. I'll take a look later.
On Jan 2, 2013 1:29 PM, <erikwright@chromium.org> wrote:

> LGTM with an idea for consideration.
>
>
> https://codereview.chromium.**org/11669012/diff/15001/**
>
chrome/browser/custom_**handlers/protocol_handler_**registry.cc<https://codereview.chromium.org/11669012/diff/15001/chrome/browser/custom_handlers/protocol_handler_registry.cc>
> File chrome/browser/custom_**handlers/protocol_handler_**registry.cc
> (right):
>
> https://codereview.chromium.**org/11669012/diff/15001/**
>
chrome/browser/custom_**handlers/protocol_handler_**registry.cc#newcode163<https://codereview.chromium.org/11669012/diff/15001/chrome/browser/custom_handlers/protocol_handler_registry.cc#newcode163>
> chrome/browser/custom_**handlers/protocol_handler_**registry.cc:163: //
> requests via |ProtocolHandlerRegistry::**CreateURLInterceptor|.
> Update name of CreateURLInterceptor
>
> https://codereview.chromium.**org/11669012/diff/15001/**
>
chrome/browser/custom_**handlers/protocol_handler_**registry.h<https://codereview.chromium.org/11669012/diff/15001/chrome/browser/custom_handlers/protocol_handler_registry.h>
> File chrome/browser/custom_**handlers/protocol_handler_**registry.h
> (right):
>
> https://codereview.chromium.**org/11669012/diff/15001/**
>
chrome/browser/custom_**handlers/protocol_handler_**registry.h#newcode137<https://codereview.chromium.org/11669012/diff/15001/chrome/browser/custom_handlers/protocol_handler_registry.h#newcode137>
> chrome/browser/custom_**handlers/protocol_handler_**registry.h:137: // is
> initialized on the UI thread. Callers assume responsibility for deleting
> The switch to returning a scoped_ptr makes the "Callers assume..."
> redundant.
>
> https://codereview.chromium.**org/11669012/diff/15001/**
>
chrome/browser/profiles/**profile_io_data.cc<https://codereview.chromium.org/11669012/diff/15001/chrome/browser/profiles/profile_io_data.cc>
> File chrome/browser/profiles/**profile_io_data.cc (right):
>
> https://codereview.chromium.**org/11669012/diff/15001/**
>
chrome/browser/profiles/**profile_io_data.cc#newcode661<https://codereview.chromium.org/11669012/diff/15001/chrome/browser/profiles/profile_io_data.cc#newcode661>
> chrome/browser/profiles/**profile_io_data.cc:661:
> protocol_handler_interceptor->**Chain(
> Passing a custom type around, and configuring the wrapped factory as a
> deferred step is possibly the right thing to do here, but if this
> pattern recurs you could end up with a lot of boiler-plate code.
>
> If that were the case, this pattern could be simplified as follows:
>
> 1) Make the protocol handler interceptor not delegate to another
> factory.
> 2) Pass the protocol handler interceptor as a generic
> URLRequestJobFactory
> 3) Define a simple URLRequestJobFactory implementation that delegates to
> two child factories, in order.
> 4) Here, create the delegating factory and pass it the interceptor as
> its first child and job_factory as its second child.
>
> Again, I wouldn't necessarily go to the trouble just for this case, but
> if this pattern recurs it would seem to be worthwhile to centralize it.
> The other advantages are:
>
> a) One less custom type exposed.
> b) Simpler life-time for the protocol handler interceptor (it doesn't
> have to check in every method for a chained factory).
>
>
https://codereview.chromium.**org/11669012/<https://codereview.chromium.org/1...
>

[pauljensen, 2013-01-02 22:13:35]

I think I addressed Erik's comments.  I'm fine waiting for Will to get back
before going further; though I don't know when he gets back exactly.  I don't
immediately see threading or security issues.  As for threading, the lifetime on
all threads of the URLRequestJobFactory is pretty similar to that of the
Interceptor, so I don't think I'm changing much.  As for the security, I think
it should behave much the same as the Interceptor also.

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

https://codereview.chromium.org/11669012/diff/15001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:661:
protocol_handler_interceptor->Chain(
On 2013/01/02 16:29:36, erikwright wrote:
> Passing a custom type around, and configuring the wrapped factory as a
deferred
> step is possibly the right thing to do here, but if this pattern recurs you
> could end up with a lot of boiler-plate code.
> 
> If that were the case, this pattern could be simplified as follows:
> 
> 1) Make the protocol handler interceptor not delegate to another factory.
> 2) Pass the protocol handler interceptor as a generic URLRequestJobFactory
> 3) Define a simple URLRequestJobFactory implementation that delegates to two
> child factories, in order.
> 4) Here, create the delegating factory and pass it the interceptor as its
first
> child and job_factory as its second child.
> 
> Again, I wouldn't necessarily go to the trouble just for this case, but if
this
> pattern recurs it would seem to be worthwhile to centralize it. The other
> advantages are:
> 
> a) One less custom type exposed.
> b) Simpler life-time for the protocol handler interceptor (it doesn't have to
> check in every method for a chained factory).
I agree, I think if this code occurs again we should centralize it, but I think
this one occurrence doesn't warrant it.

[willchan no longer on Chromium, 2013-01-03 11:15:10]

Thanks for the explanation. I thought you were doing deeper surgery on
ProtocolHandlerRegistry. If you´re only changing the interceptor part, it´s
safe. Please proceed :)

[pauljensen, 2013-01-03 14:53:44]

Matt, could you review this CL?  I know you're busy but this CL is smallish and
half of the changed files are  ProfileIOData-related which you and Will are the
specialists for.

[mmenke, 2013-01-03 15:45:11]

This CL LGTM, though I wonder if we should include this capability in the
ProtocolInterceptJobFactory.

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

https://codereview.chromium.org/11669012/diff/24001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:220: bool
ProtocolHandlerRegistry::JobFactory::IsHandledProtocol(
Hmm...Wonder if this capability should be added to ProtocolInterceptJobFactory. 
We'd need to subclass ProtocolHander (Or use a similar class instead, to protect
against using the wrong class in the wrong spot), but may be a useful capability
to have elsewhere.

WDYT?

https://codereview.chromium.org/11669012/diff/24001/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/11669012/diff/24001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:13: #include
"base/memory/ref_counted.h"
Should include the scoped_ptr header, too.  And can remove it from the cc file.

https://codereview.chromium.org/11669012/diff/24001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:90: class JobFactory
: public net::URLRequestJobFactory {
I think we could do a little better on the class name / variable name agreement.
 This is passed as "protocol_handler_interceptor" everywhere.  Maybe
JobInterceptorFactory, and rename CreateURLRequestJobFactory() as well?

Could alternatively/additionally change "protocol_handler_interceptor" to
protocol_handler_interceptor_job_factory.

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

https://codereview.chromium.org/11669012/diff/24001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:660: if
(protocol_handler_interceptor.get()) {
nit:  Can remove the get() while you're here.

[pauljensen, 2013-01-03 18:43:51]

I addressed Matt's comments.  Thanks Matt!  I'll seek OWNERS approval.

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

https://codereview.chromium.org/11669012/diff/24001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:220: bool
ProtocolHandlerRegistry::JobFactory::IsHandledProtocol(
On 2013/01/03 15:45:11, Matt Menke wrote:
> Hmm...Wonder if this capability should be added to
ProtocolInterceptJobFactory. 
> We'd need to subclass ProtocolHander (Or use a similar class instead, to
protect
> against using the wrong class in the wrong spot), but may be a useful
capability
> to have elsewhere.
> 
> WDYT?
ProtocolHandlerRegistry::Interceptor and DeveloperProtocolHandler are the only
Interceptors to implement WillHandleProtocol().  When I convert
DeveloperProtocolHandler to a ProtocolHandler it won't need to implement
IsHandledProtocol() for reasons I just listed in http://crbug.com/161529.  Since
ProtocolHandlerRegistry::JobFactory will be the only user of IsHandledProtocol()
I don't think we need to add it to ProtocolInterceptJobFactory; I'm open to
adding this in the future if more users exist.  I think the general idea is that
interceptors aren't adding default scheme support, this is generally done via
URLRequestJobFactoryImpl::SetProtocolHandler().  ProtocolHandlerRegistry is a
little different because it can dynamically add and subtract any scheme support.

[mmenke, 2013-01-03 18:49:37]

And still LGTM.

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

https://codereview.chromium.org/11669012/diff/24001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:220: bool
ProtocolHandlerRegistry::JobFactory::IsHandledProtocol(
On 2013/01/03 18:43:51, pauljensen wrote:
> On 2013/01/03 15:45:11, Matt Menke wrote:
> > Hmm...Wonder if this capability should be added to
> ProtocolInterceptJobFactory. 
> > We'd need to subclass ProtocolHander (Or use a similar class instead, to
> protect
> > against using the wrong class in the wrong spot), but may be a useful
> capability
> > to have elsewhere.
> > 
> > WDYT?
> ProtocolHandlerRegistry::Interceptor and DeveloperProtocolHandler are the only
> Interceptors to implement WillHandleProtocol().  When I convert
> DeveloperProtocolHandler to a ProtocolHandler it won't need to implement
> IsHandledProtocol() for reasons I just listed in http://crbug.com/161529. 
Since
> ProtocolHandlerRegistry::JobFactory will be the only user of
IsHandledProtocol()
> I don't think we need to add it to ProtocolInterceptJobFactory; I'm open to
> adding this in the future if more users exist.  I think the general idea is
that
> interceptors aren't adding default scheme support, this is generally done via
> URLRequestJobFactoryImpl::SetProtocolHandler().  ProtocolHandlerRegistry is a
> little different because it can dynamically add and subtract any scheme
support.

SGTM

https://codereview.chromium.org/11669012/diff/20004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/11669012/diff/20004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:91: class
JobInterceptorFactory : public net::URLRequestJobFactory {
One other thing...We should have a description of this class.

Should at least mention it wraps another URLRequestJobFactory, and intercepts
certain requests.

[pauljensen, 2013-01-03 19:10:39]

Scott, could you approve the 	 chrome/browser/custom_handlers/ and 	
chrome/browser/ui/sync/ changes?
erikwright and mmenke have thoroughly reviewed the change as a whole. This
change is part of a refactor to avoid net::URLRequestJobFactory::Interceptor.

[sky, 2013-01-07 17:21:19]

I'm not familiar with the custom_handlers code at all. Is there a better
reviewer you can get?

[pauljensen, 2013-01-07 18:00:18]

James, Could you approve the chrome/browser/custom_handlers/ and
chrome/browser/ui/sync/ changes?
erikwright and mmenke have thoroughly reviewed the change as a whole. This
change is part of a refactor to avoid net::URLRequestJobFactory::Interceptor.

[James Hawkins, 2013-01-07 18:57:48]

LGTM with nits.

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:89: class Core;
nit: Make this class name a bit more descriptive.  It's easy to have a 'Core'
disconnected from its parent object.

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:93: // registered the
URLRequest is passed along to the chained
nit: 'registered, the'

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:99: explicit
JobInterceptorFactory(Core* core);
nit: Document |core|.

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:104: void
Chain(scoped_ptr<net::URLRequestJobFactory> job_factory);
nit: Document |job_factory|.

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:106: //
URLRequestJobFactory implementation
nit: Add missing period at end of comment.

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:110: virtual
net::URLRequestJob* MaybeCreateJobWithInterceptor(
ugh. Methods that have the word 'maybe' in it are awful; however, you didn't
write the interface in this CL, so I'm not asking you to change it.

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:128:
scoped_ptr<URLRequestJobFactory> job_factory_;
nit: Document member variables.

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:55: class
FakeURLRequestJobFactory : public net::URLRequestJobFactory {
nit: Document the class.

[pauljensen, 2013-01-07 20:48:58]

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:89: class Core;
On 2013/01/07 18:57:49, James Hawkins wrote:
> nit: Make this class name a bit more descriptive.  
I could rename it IOThreadDelegate?  I wanted to check with you first as I
didn't write or modify the Core class in this CL.

> It's easy to have a 'Core' disconnected from its parent object.
I'm not sure I understand.  The ProtocolHandlerRegistry and
JobInterceptorFactory each always hold a scoped_refptr to Core.  So other than
at construction and destruction time, Core should always be connected to its
parent.

[James Hawkins, 2013-01-07 20:59:42]

On 2013/01/07 20:48:58, pauljensen wrote:
>
https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
> File chrome/browser/custom_handlers/protocol_handler_registry.h (right):
> 
>
https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
> chrome/browser/custom_handlers/protocol_handler_registry.h:89: class Core;
> On 2013/01/07 18:57:49, James Hawkins wrote:
> > nit: Make this class name a bit more descriptive.  
> I could rename it IOThreadDelegate?  I wanted to check with you first as I
> didn't write or modify the Core class in this CL.
> 
> > It's easy to have a 'Core' disconnected from its parent object.
> I'm not sure I understand.  The ProtocolHandlerRegistry and
> JobInterceptorFactory each always hold a scoped_refptr to Core.  So other than
> at construction and destruction time, Core should always be connected to its
> parent.

I meant conceptually, wrt readability.

[pauljensen, 2013-01-07 21:15:52]

On 2013/01/07 20:59:42, James Hawkins wrote:
> On 2013/01/07 20:48:58, pauljensen wrote:
> >
>
https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
> > File chrome/browser/custom_handlers/protocol_handler_registry.h (right):
> > 
> >
>
https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
> > chrome/browser/custom_handlers/protocol_handler_registry.h:89: class Core;
> > On 2013/01/07 18:57:49, James Hawkins wrote:
> > > nit: Make this class name a bit more descriptive.  
> > I could rename it IOThreadDelegate?  I wanted to check with you first as I
> > didn't write or modify the Core class in this CL.
> > 
> > > It's easy to have a 'Core' disconnected from its parent object.
> > I'm not sure I understand.  The ProtocolHandlerRegistry and
> > JobInterceptorFactory each always hold a scoped_refptr to Core.  So other
than
> > at construction and destruction time, Core should always be connected to its
> > parent.
> 
> I meant conceptually, wrt readability.
I could rename it IOThreadDelegate?  I wanted to check with you first as I
didn't write or modify the Core class in this CL.

[James Hawkins, 2013-01-07 21:19:30]

On 2013/01/07 21:15:52, pauljensen wrote:
> On 2013/01/07 20:59:42, James Hawkins wrote:
> > On 2013/01/07 20:48:58, pauljensen wrote:
> > >
> >
>
https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
> > > File chrome/browser/custom_handlers/protocol_handler_registry.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11669012/diff/21008/chrome/browser/custom_han...
> > > chrome/browser/custom_handlers/protocol_handler_registry.h:89: class Core;
> > > On 2013/01/07 18:57:49, James Hawkins wrote:
> > > > nit: Make this class name a bit more descriptive.  
> > > I could rename it IOThreadDelegate?  I wanted to check with you first as I
> > > didn't write or modify the Core class in this CL.
> > > 
> > > > It's easy to have a 'Core' disconnected from its parent object.
> > > I'm not sure I understand.  The ProtocolHandlerRegistry and
> > > JobInterceptorFactory each always hold a scoped_refptr to Core.  So other
> than
> > > at construction and destruction time, Core should always be connected to
its
> > > parent.
> > 
> > I meant conceptually, wrt readability.
> I could rename it IOThreadDelegate?  I wanted to check with you first as I
> didn't write or modify the Core class in this CL.

That's fine.  I don't particularly care what you end up with, as long as it's
more descriptive than Core.

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

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

[commit-bot: I haz the power, 2013-01-08 12:58:19]

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

[commit-bot: I haz the power, 2013-01-08 14:04:23]

Change committed as 175524