Turned job_factory into a pure virtual class

net/url_request/url_request_job_factory.h

 // Copyright (c) 2011 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.
 
 #ifndef NET_URL_REQUEST_URL_REQUEST_JOB_FACTORY_H_
 #define NET_URL_REQUEST_URL_REQUEST_JOB_FACTORY_H_
 
-#include <map>
 #include <string>
-#include <vector>
 #include "base/basictypes.h"
+#include "base/compiler_specific.h"
 #include "base/threading/non_thread_safe.h"
 #include "net/base/net_export.h"
 
 class GURL;
 
 namespace net {
 
 class URLRequest;
 class URLRequestJob;
 
 class NET_EXPORT URLRequestJobFactory
     : NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
   class NET_EXPORT ProtocolHandler {

[willchan no longer on Chromium, 2012/08/16 20:35:37]

Mark these (ProtocolHandler & Interceptor) with TODOs to move to the
URLRequestJobFactoryImpl?

[erikwright (departed), 2012/08/21 18:02:13]

Agreed.

[shalev, 2012/08/22 19:10:11]

Done.

    public:
     virtual ~ProtocolHandler();
 
     virtual URLRequestJob* MaybeCreateJob(URLRequest* request) const = 0;
   };
 
   class NET_EXPORT Interceptor {
    public:
     virtual ~Interceptor();
 
@@ skipping 23 matching lines @@
     virtual URLRequestJob* MaybeInterceptResponse(
         URLRequest* request) const = 0;
 
     // Returns true if this interceptor handles requests for URLs with the
     // given protocol. Returning false does not imply that this interceptor
     // can't or won't handle requests with the given protocol.
     virtual bool WillHandleProtocol(const std::string& protocol) const;
   };
 
   URLRequestJobFactory();
-  ~URLRequestJobFactory();
+  virtual ~URLRequestJobFactory();
 
+  // TODO(shalev): Remove this from the interface.
   // Sets the ProtocolHandler for a scheme. Returns true on success, false on
   // failure (a ProtocolHandler already exists for |scheme|). On success,
   // URLRequestJobFactory takes ownership of |protocol_handler|.
-  bool SetProtocolHandler(const std::string& scheme,
-                          ProtocolHandler* protocol_handler);
+  virtual bool SetProtocolHandler(const std::string& scheme,
+                                  ProtocolHandler* protocol_handler) = 0;
 
+  // TODO(shalev): Remove this from the interface.
   // Takes ownership of |interceptor|. Adds it to the end of the Interceptor
   // list.
-  void AddInterceptor(Interceptor* interceptor);
+  virtual void AddInterceptor(Interceptor* interceptor) = 0;
 
-  URLRequestJob* MaybeCreateJobWithInterceptor(URLRequest* request) const;
+  virtual URLRequestJob* MaybeCreateJobWithInterceptor(

[willchan no longer on Chromium, 2012/08/16 20:35:37]

Shouldn't there be a TODO here to kill this too?

[erikwright (departed), 2012/08/21 17:43:32]

The two TODOs added are in reference to the mutators. It doesn't make sense for
the factory API to dictate how a specific implementation of the API is
configured. The only universal function of a factory is to create Jobs. Shalev:
please update the TODOs to specifically say "Remove mutators from the
interface.".

Will: If I understand your comments correctly, you are suggesting TODOs for:

1) killing the interceptor related behaviour. This makes sense because an
interceptor is just a special form of a protocol handler and there is no need
for a distinction as to which is implementing a given Job. Shalev: please add a
TODO to this effect.

2) killing the ProtocolHandler related behaviour. I don't know if you are
implying just changing the name, or whether you are implying changing this API
to be the CreateRequest API (returning a URLRequest instead of a Job) that we
have discussed.

For #1, I agree.

For #2, if it's changing the name, I agree. If it's about changing to create
URLRequest instead of Job I disagree, because I'd rather wait until we're a bit
closer to that final step before littering the code with detailed intentions.

[willchan no longer on Chromium, 2012/08/21 17:58:01]

Not sure if I understand your comments. What I was thinking was the
URLRequestJobFactory interface should not know anything about Interceptors or
ProtocolHandlers. It just maybe creates jobs at specific points.

URLRequestJobFactoryImpl on the other hand can segregate stuff into
ProtocolHandlers and Interceptors. But that's specific to
URLRequestJobFactoryImpl, and not something that needs to be part of the base
interface.

On 2012/08/21 17:43:32, erikwright wrote:

[erikwright (departed), 2012/08/21 18:02:13]

Great. We are in agreement.

A single TODO should suffice:

TODO(shalev): Consolidate MaybeCreateJobWithInterceptor and
MaybeCreateJobWithProtocolHandler into a single method.

[shalev, 2012/08/22 19:10:11]

Done.

+      URLRequest* request) const = 0;
 
-  URLRequestJob* MaybeCreateJobWithProtocolHandler(const std::string& scheme,
-                                                   URLRequest* request) const;
+  virtual URLRequestJob* MaybeCreateJobWithProtocolHandler(

[willchan no longer on Chromium, 2012/08/16 20:35:37]

Ditto

+      const std::string& scheme, URLRequest* request) const = 0;
 
-  URLRequestJob* MaybeInterceptRedirect(const GURL& location,
-                                        URLRequest* request) const;
+  virtual URLRequestJob* MaybeInterceptRedirect(
+      const GURL& location, URLRequest* request) const = 0;
 
-  URLRequestJob* MaybeInterceptResponse(URLRequest* request) const;
+  virtual URLRequestJob* MaybeInterceptResponse(URLRequest* request) const = 0;
 
-  bool IsHandledProtocol(const std::string& scheme) const;
+  virtual bool IsHandledProtocol(const std::string& scheme) const = 0;
 
-  bool IsHandledURL(const GURL& url) const;
+  virtual bool IsHandledURL(const GURL& url) const = 0;
 
  private:
-  typedef std::map<std::string, ProtocolHandler*> ProtocolHandlerMap;
-  typedef std::vector<Interceptor*> InterceptorList;
-
-  ProtocolHandlerMap protocol_handler_map_;
-  InterceptorList interceptors_;
-
   DISALLOW_COPY_AND_ASSIGN(URLRequestJobFactory);
 };
 
 }  // namespace net
 
 #endif  // NET_URL_REQUEST_URL_REQUEST_JOB_FACTORY_H_