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

chrome/browser/custom_handlers/protocol_handler_registry.cc

Index: chrome/browser/custom_handlers/protocol_handler_registry.cc
diff --git a/chrome/browser/custom_handlers/protocol_handler_registry.cc b/chrome/browser/custom_handlers/protocol_handler_registry.cc
index 3be1184aca68907c0e73f7439a13f5a28fc0837e..42436452a39fb6c7d6eb73064f3bbfcd3d102be7 100644
--- a/chrome/browser/custom_handlers/protocol_handler_registry.cc
+++ b/chrome/browser/custom_handlers/protocol_handler_registry.cc
@@ -205,6 +205,12 @@ bool ProtocolHandlerRegistry::JobInterceptorFactory::IsHandledURL(
       job_factory_->IsHandledURL(url);
 }
 
+bool ProtocolHandlerRegistry::JobInterceptorFactory::IsSafeRedirectTarget(
+    const GURL& location) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  return job_factory_->IsSafeRedirectTarget(location);

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

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.

[pauljensen, 2013/02/20 15:13:27]

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.
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.

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

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]

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

+}
+
 // DefaultClientObserver ------------------------------------------------------
 
 ProtocolHandlerRegistry::DefaultClientObserver::DefaultClientObserver(