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

chrome/browser/custom_handlers/protocol_handler_registry.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 187 matching lines @@
 }
 
 bool ProtocolHandlerRegistry::JobInterceptorFactory::IsHandledURL(
     const GURL& url) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   return (url.is_valid() &&
       io_thread_delegate_->IsHandledProtocol(url.scheme())) ||
       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(
     ProtocolHandlerRegistry* registry)
     : worker_(NULL),
       registry_(registry) {
   DCHECK(registry_);
 }
 
 ProtocolHandlerRegistry::DefaultClientObserver::~DefaultClientObserver() {
@@ skipping 652 matching lines @@
 
 scoped_ptr<ProtocolHandlerRegistry::JobInterceptorFactory>
 ProtocolHandlerRegistry::CreateJobInterceptorFactory() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // this is always created on the UI thread (in profile_io's
   // InitializeOnUIThread. Any method calls must be done
   // on the IO thread (this is checked).
   return scoped_ptr<JobInterceptorFactory>(new JobInterceptorFactory(
       io_thread_delegate_));
 }