Turned job_factory into a pure virtual class

Turned job_factory into a pure virtual class

This makes it possible to create wrappers for job_factory that handle a particular job type, and forward the rest to the old job_factory.

TBR=michaeln@chromium.org, brettw@chromium.org, davemoore@chromium.org, kalman@chromium.org, benjhayden@chromium.org, jochen@chromium.org

BUG=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153241

[shalev, 2012-08-14 20:21:17]

Hi Erik,

Could you look at this to let me know if this is what you had in mind for the
virtual job_factory class? If it looks, okay, I will send it to Matt for a code
review.

Thanks,

Shalev

[erikwright (departed), 2012-08-16 15:49:01]

Looks about right. Suggest responding to my comments before implicating other
reviewers.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
File net/url_request/http_intercept_job_factory.cc (right):

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.cc:32: DCHECK(false);
NOTREACHED()

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.cc:43: if (scheme == "http" || scheme
== "https")
constants for http/https?

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.cc:61: if (scheme == "http" || scheme
== "https")
constants

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.cc:67: if (url.scheme() == "http" ||
url.scheme() == "https")
constants

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
File net/url_request/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.h:9: #include
"base/threading/non_thread_safe.h"
why is this required?

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.h:20: class NET_EXPORT
HttpInterceptJobFactory : public URLRequestJobFactory {
class comments required.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.h:26: bool SetProtocolHandler(const
std::string& scheme,
These are presumably overrides from URLRequestJobFactory. Add OVERRIDE keyword
and group them under a comment:

// URLRequestJobFactory implementation.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:8: #include <map>
don't need map or vector

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:23: : NON_EXPORTED_BASE(public
base::NonThreadSafe) {
#include "base/compiler_specific.h" for NON_EXPORTED_BASE

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:69: ~URLRequestJobFactory();
virtual

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:71: // Sets the ProtocolHandler for a
scheme. Returns true on success, false on
TODO remove from the interface.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:78: // list.
TODO remove from the interface.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
File net/url_request/url_request_job_factory_impl.h (right):

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:9: #include <string>
string is part of the interface you are implementing, thus you can assume it is
included.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:12: #include
"base/threading/non_thread_safe.h"
Don't need this any more.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:16: class GURL;
part of the interface you are implementing, you can assume it is already
declared.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:21: class URLRequestJob;
part of the interface you are implementing, you can assume they are already
declared.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:26: ~URLRequestJobFactoryImpl();
virtual

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:29: // failure (a ProtocolHandler
already exists for |scheme|). On success,
Group under a comment:

// URLRequestJobFactory implementation

Remove comments (they moved to the interface, presumably).

Add virtual and OVERRIDE.

[shalev, 2012-08-16 18:40:38]

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
File net/url_request/http_intercept_job_factory.cc (right):

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.cc:32: DCHECK(false);
On 2012/08/16 15:49:01, erikwright wrote:
> NOTREACHED()

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.cc:43: if (scheme == "http" || scheme
== "https")
On 2012/08/16 15:49:01, erikwright wrote:
> constants for http/https?

I can't use the constant chrome::kHttpScheme because that violates checkdeps
rules. Instead, I redefined the constants for use in this file.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.cc:61: if (scheme == "http" || scheme
== "https")
On 2012/08/16 15:49:01, erikwright wrote:
> constants

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.cc:67: if (url.scheme() == "http" ||
url.scheme() == "https")
On 2012/08/16 15:49:01, erikwright wrote:
> constants

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
File net/url_request/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.h:9: #include
"base/threading/non_thread_safe.h"
On 2012/08/16 15:49:01, erikwright wrote:
> why is this required?

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.h:20: class NET_EXPORT
HttpInterceptJobFactory : public URLRequestJobFactory {
On 2012/08/16 15:49:01, erikwright wrote:
> class comments required.

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/http_interc...
net/url_request/http_intercept_job_factory.h:26: bool SetProtocolHandler(const
std::string& scheme,
On 2012/08/16 15:49:01, erikwright wrote:
> These are presumably overrides from URLRequestJobFactory. Add OVERRIDE keyword
> and group them under a comment:
> 
> // URLRequestJobFactory implementation.

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:8: #include <map>
On 2012/08/16 15:49:01, erikwright wrote:
> don't need map or vector

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:23: : NON_EXPORTED_BASE(public
base::NonThreadSafe) {
On 2012/08/16 15:49:01, erikwright wrote:
> #include "base/compiler_specific.h" for NON_EXPORTED_BASE

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:69: ~URLRequestJobFactory();
On 2012/08/16 15:49:01, erikwright wrote:
> virtual

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:71: // Sets the ProtocolHandler for a
scheme. Returns true on success, false on
On 2012/08/16 15:49:01, erikwright wrote:
> TODO remove from the interface.

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory.h:78: // list.
On 2012/08/16 15:49:01, erikwright wrote:
> TODO remove from the interface.

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
File net/url_request/url_request_job_factory_impl.h (right):

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:9: #include <string>
On 2012/08/16 15:49:01, erikwright wrote:
> string is part of the interface you are implementing, thus you can assume it
is
> included.

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:12: #include
"base/threading/non_thread_safe.h"
On 2012/08/16 15:49:01, erikwright wrote:
> Don't need this any more.

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:16: class GURL;
On 2012/08/16 15:49:01, erikwright wrote:
> part of the interface you are implementing, you can assume it is already
> declared.

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:21: class URLRequestJob;
On 2012/08/16 15:49:01, erikwright wrote:
> part of the interface you are implementing, you can assume they are already
> declared.

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:26: ~URLRequestJobFactoryImpl();
On 2012/08/16 15:49:01, erikwright wrote:
> virtual

Done.

http://codereview.chromium.org/10836248/diff/2001/net/url_request/url_request...
net/url_request/url_request_job_factory_impl.h:29: // failure (a ProtocolHandler
already exists for |scheme|). On success,
On 2012/08/16 15:49:01, erikwright wrote:
> Group under a comment:
> 
> // URLRequestJobFactory implementation
> 
> Remove comments (they moved to the interface, presumably).
> 
> Add virtual and OVERRIDE.

Done.

[shalev, 2012-08-16 18:50:32]

Hi Will and Matt,

This CL turns URLRequestJobFactory into a pure virtual class, and provides and
implementation class and a wrapper for http protocols. Please take a look.

Shalev

[mmenke, 2012-08-16 19:12:11]

This code seems pretty special case to me.  Wonder if it really belongs in net/
How many consumers will need it?  Do they all only need to override just HTTP
and HTTPS?  If not, could make the intercept job factory a subclass of the job
factory, and just call into it when it can't handle something itself.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
File net/url_request/http_intercept_job_factory.cc (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.cc:29: const std::string& scheme,
ProtocolHandler* protocol_handler) {
NOTREACHED()?

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
File net/url_request/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.h:8: #include "base/basictypes.h"
#include "base/compiler_specific.h" for OVERRIDE, I believe.  Or is it
DISALLOW_COPY_AND_ASSIGN?  One or the other.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.h:26: ~HttpInterceptJobFactory();
Destructor should be virtual

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.h:48: bool IsHandledURL(const GURL&
url) const OVERRIDE;
These should all be virtual

[erikwright (departed), 2012-08-16 19:17:03]

On 2012/08/16 19:12:11, Matt Menke wrote:
> This code seems pretty special case to me.  Wonder if it really belongs in
net/
> How many consumers will need it?  Do they all only need to override just HTTP
> and HTTPS?  If not, could make the intercept job factory a subclass of the job
> factory, and just call into it when it can't handle something itself.

I don't think the behaviour should be in the main job factory implementation,
but I do think that, at least for now, it should probably live in
chrome/browser/net (assuming that the only users are in browser-land).

> 
>
http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
> File net/url_request/http_intercept_job_factory.cc (right):
> 
>
http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
> net/url_request/http_intercept_job_factory.cc:29: const std::string& scheme,
> ProtocolHandler* protocol_handler) {
> NOTREACHED()?
> 
>
http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
> File net/url_request/http_intercept_job_factory.h (right):
> 
>
http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
> net/url_request/http_intercept_job_factory.h:8: #include "base/basictypes.h"
> #include "base/compiler_specific.h" for OVERRIDE, I believe.  Or is it
> DISALLOW_COPY_AND_ASSIGN?  One or the other.
> 
>
http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
> net/url_request/http_intercept_job_factory.h:26: ~HttpInterceptJobFactory();
> Destructor should be virtual
> 
>
http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
> net/url_request/http_intercept_job_factory.h:48: bool IsHandledURL(const GURL&
> url) const OVERRIDE;
> These should all be virtual

[mmenke, 2012-08-16 19:25:26]

On 2012/08/16 19:17:03, erikwright wrote:
> On 2012/08/16 19:12:11, Matt Menke wrote:
> > This code seems pretty special case to me.  Wonder if it really belongs in
> net/
> > How many consumers will need it?  Do they all only need to override just
HTTP
> > and HTTPS?  If not, could make the intercept job factory a subclass of the
job
> > factory, and just call into it when it can't handle something itself.
> 
> I don't think the behaviour should be in the main job factory implementation,
> but I do think that, at least for now, it should probably live in
> chrome/browser/net (assuming that the only users are in browser-land).

I was thinking more along the lines of:

URLRequestJobInterceptorFactory::DoStuff() {
  StuffResult result = URLRequestFactoryJob::DoStuff();
  if (result)
    return result;
  return wrapped_factory_->DoStuff();
}

Then we just set the protocol handlers that override the ones in the wrapped
factory.  Anyhow, I'm happy keeping it as-is, but moving it to
chrome/browser/net.

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

Yeah, I think the HttpInterceptJobFactory should live in chrome/browser/net. And
I like to mention that I prefer composition to implementation inheritance, so
the strategy you have right now is great.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
File net/url_request/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.h:30: bool SetProtocolHandler(const
std::string& scheme,
Everything that is overridden should include "virtual", as per the Google C++
Style Guide.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
File net/url_request/url_request_job_factory.cc (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.cc:7: #include "base/stl_util.h"
I think we can kill off most of these, right?

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:24: class NET_EXPORT ProtocolHandler {
Mark these (ProtocolHandler & Interceptor) with TODOs to move to the
URLRequestJobFactoryImpl?

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:82: virtual URLRequestJob*
MaybeCreateJobWithInterceptor(
Shouldn't there be a TODO here to kill this too?

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:85: virtual URLRequestJob*
MaybeCreateJobWithProtocolHandler(
Ditto

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

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:82: virtual URLRequestJob*
MaybeCreateJobWithInterceptor(
On 2012/08/16 20:35:37, willchan wrote:
> Shouldn't there be a TODO here to kill this too?

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:00]

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:82: virtual URLRequestJob*
MaybeCreateJobWithInterceptor(
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:
> On 2012/08/16 20:35:37, willchan wrote:
> > Shouldn't there be a TODO here to kill this too?
> 
> 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.

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

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:24: class NET_EXPORT ProtocolHandler {
On 2012/08/16 20:35:37, willchan wrote:
> Mark these (ProtocolHandler & Interceptor) with TODOs to move to the
> URLRequestJobFactoryImpl?

Agreed.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:82: virtual URLRequestJob*
MaybeCreateJobWithInterceptor(
On 2012/08/21 17:58:01, willchan wrote:
> 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:
> > On 2012/08/16 20:35:37, willchan wrote:
> > > Shouldn't there be a TODO here to kill this too?
> > 
> > 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.
> 

Great. We are in agreement.

A single TODO should suffice:

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

[willchan no longer on Chromium, 2012-08-21 18:04:12]

OK, great :)


On Tue, Aug 21, 2012 at 11:02 AM, <erikwright@chromium.org> wrote:

>
> http://codereview.chromium.**org/10836248/diff/11001/net/**
>
url_request/url_request_job_**factory.h<http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_request_job_factory.h>
> File net/url_request/url_request_**job_factory.h (right):
>
> http://codereview.chromium.**org/10836248/diff/11001/net/**
>
url_request/url_request_job_**factory.h#newcode24<http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_request_job_factory.h#newcode24>
> net/url_request/url_request_**job_factory.h:24: class NET_EXPORT
> ProtocolHandler {
> On 2012/08/16 20:35:37, willchan wrote:
>
>> Mark these (ProtocolHandler & Interceptor) with TODOs to move to the
>> URLRequestJobFactoryImpl?
>>
>
> Agreed.
>
>
> http://codereview.chromium.**org/10836248/diff/11001/net/**
>
url_request/url_request_job_**factory.h#newcode82<http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_request_job_factory.h#newcode82>
> net/url_request/url_request_**job_factory.h:82: virtual URLRequestJob*
> MaybeCreateJobWithInterceptor(
> On 2012/08/21 17:58:01, willchan wrote:
>
>> 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:
>> > On 2012/08/16 20:35:37, willchan wrote:
>> > > Shouldn't there be a TODO here to kill this too?
>> >
>> > 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.
>
>
> Great. We are in agreement.
>
> A single TODO should suffice:
>
> TODO(shalev): Consolidate MaybeCreateJobWithInterceptor and
> MaybeCreateJobWithProtocolHand**ler into a single method.
>
>
http://codereview.chromium.**org/10836248/<http://codereview.chromium.org/108...
>

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

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
File net/url_request/http_intercept_job_factory.cc (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.cc:29: const std::string& scheme,
ProtocolHandler* protocol_handler) {
On 2012/08/16 19:12:11, Matt Menke wrote:
> NOTREACHED()?

Done.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
File net/url_request/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.h:8: #include "base/basictypes.h"
On 2012/08/16 19:12:11, Matt Menke wrote:
> #include "base/compiler_specific.h" for OVERRIDE, I believe.  Or is it
> DISALLOW_COPY_AND_ASSIGN?  One or the other.

Done.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.h:26: ~HttpInterceptJobFactory();
On 2012/08/16 19:12:11, Matt Menke wrote:
> Destructor should be virtual

Done.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.h:30: bool SetProtocolHandler(const
std::string& scheme,
On 2012/08/16 20:35:37, willchan wrote:
> Everything that is overridden should include "virtual", as per the Google C++
> Style Guide.

Done.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/http_inter...
net/url_request/http_intercept_job_factory.h:48: bool IsHandledURL(const GURL&
url) const OVERRIDE;
On 2012/08/16 19:12:11, Matt Menke wrote:
> These should all be virtual

Done.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
File net/url_request/url_request_job_factory.cc (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.cc:7: #include "base/stl_util.h"
On 2012/08/16 20:35:37, willchan wrote:
> I think we can kill off most of these, right?

Done.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:24: class NET_EXPORT ProtocolHandler {
On 2012/08/21 18:02:13, erikwright wrote:
> On 2012/08/16 20:35:37, willchan wrote:
> > Mark these (ProtocolHandler & Interceptor) with TODOs to move to the
> > URLRequestJobFactoryImpl?
> 
> Agreed.

Done.

http://codereview.chromium.org/10836248/diff/11001/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:82: virtual URLRequestJob*
MaybeCreateJobWithInterceptor(
On 2012/08/21 18:02:13, erikwright wrote:
> On 2012/08/21 17:58:01, willchan wrote:
> > 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:
> > > On 2012/08/16 20:35:37, willchan wrote:
> > > > Shouldn't there be a TODO here to kill this too?
> > > 
> > > 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.
> > 
> 
> Great. We are in agreement.
> 
> A single TODO should suffice:
> 
> TODO(shalev): Consolidate MaybeCreateJobWithInterceptor and
> MaybeCreateJobWithProtocolHandler into a single method.

Done.

[erikwright (departed), 2012-08-22 19:29:37]

I need to look through in a bit more detail before giving LGTM.

http://codereview.chromium.org/10836248/diff/14001/chrome/browser/net/http_in...
File chrome/browser/net/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/14001/chrome/browser/net/http_in...
chrome/browser/net/http_intercept_job_factory.h:19: // This class acts as a
wrapper for URLRequestJobFactory. It handles http and
nit: capitalize HTTP and HTTPS in this comment.

(it is correct to use camel case in names like HttpInterceptJobFactory, FYI).

http://codereview.chromium.org/10836248/diff/14001/chrome/browser/net/http_in...
chrome/browser/net/http_intercept_job_factory.h:29: 
nit: I think you can/should remove vertical whitespace between all of the
implementation declarations.

[shalev, 2012-08-22 20:04:00]

http://codereview.chromium.org/10836248/diff/14001/chrome/browser/net/http_in...
File chrome/browser/net/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/14001/chrome/browser/net/http_in...
chrome/browser/net/http_intercept_job_factory.h:19: // This class acts as a
wrapper for URLRequestJobFactory. It handles http and
On 2012/08/22 19:29:37, erikwright wrote:
> nit: capitalize HTTP and HTTPS in this comment.
> 
> (it is correct to use camel case in names like HttpInterceptJobFactory, FYI).

Done.

http://codereview.chromium.org/10836248/diff/14001/chrome/browser/net/http_in...
chrome/browser/net/http_intercept_job_factory.h:29: 
On 2012/08/22 19:29:37, erikwright wrote:
> nit: I think you can/should remove vertical whitespace between all of the
> implementation declarations.

Done.

[erikwright (departed), 2012-08-22 20:57:35]

LGTM.

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
File net/url_request/url_request_job_factory_impl.h (right):

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
net/url_request/url_request_job_factory_impl.h:22: 
nit: remove vertical whitespace.

[mmenke, 2012-08-23 15:00:20]

LGTM

http://codereview.chromium.org/10836248/diff/15021/chrome/browser/net/http_in...
File chrome/browser/net/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/15021/chrome/browser/net/http_in...
chrome/browser/net/http_intercept_job_factory.h:30: ProtocolHandler*
protocol_handler) OVERRIDE;
nit:  Fix indent.

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:8: #include <string>
While you're here, could you add a blank line after the first include?  Standard
C++ headers should be separated from other headers.

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
File net/url_request/url_request_job_factory_unittest.cc (right):

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
net/url_request/url_request_job_factory_unittest.cc:5: #include
"net/url_request/url_request_job_factory_impl.h"
Think this file should now be called url_request_job_factory_impl_unittest.cc

[shalev, 2012-08-23 15:15:46]

http://codereview.chromium.org/10836248/diff/15021/chrome/browser/net/http_in...
File chrome/browser/net/http_intercept_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/15021/chrome/browser/net/http_in...
chrome/browser/net/http_intercept_job_factory.h:30: ProtocolHandler*
protocol_handler) OVERRIDE;
On 2012/08/23 15:00:21, Matt Menke wrote:
> nit:  Fix indent.

Done.

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
File net/url_request/url_request_job_factory.h (right):

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
net/url_request/url_request_job_factory.h:8: #include <string>
On 2012/08/23 15:00:21, Matt Menke wrote:
> While you're here, could you add a blank line after the first include? 
Standard
> C++ headers should be separated from other headers.

Done.

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
File net/url_request/url_request_job_factory_impl.h (right):

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
net/url_request/url_request_job_factory_impl.h:22: 
On 2012/08/22 20:57:35, erikwright wrote:
> nit: remove vertical whitespace.

Done.

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
File net/url_request/url_request_job_factory_unittest.cc (right):

http://codereview.chromium.org/10836248/diff/15021/net/url_request/url_reques...
net/url_request/url_request_job_factory_unittest.cc:5: #include
"net/url_request/url_request_job_factory_impl.h"
On 2012/08/23 15:00:21, Matt Menke wrote:
> Think this file should now be called url_request_job_factory_impl_unittest.cc

Done.

[mmenke, 2012-08-24 14:42:08]

And still LGTM

[commit-bot: I haz the power, 2012-08-24 15:24:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shalev@chromium.org/10836248/19007

[commit-bot: I haz the power, 2012-08-24 17:42:54]

Change committed as 153241