Give URLRequestJobFactory::Interceptors the ability to specify protocols they handle.

Give URLRequestJobFactory::Interceptors the ability to specify protocols they handle.

TEST=Unit tests provided.

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

[koz (OOO until 15th September), 2011-05-20 06:46:44]

Hey Will,

I noticed that RPH stopped working. In your previous change you added an
interceptor to URLRequestJobFactory that would handle requests that
ProtocolHanlderRegistry knows how to handle, but
URLRequestJobFactory::IsHandledProtocol() doesn't reflect that there's an
interceptor that will handle those protocols, and so they get skipped.

This change adds a way for Interceptors to specify protocols that they
explicitly handle, and so URJF::IsHandledProtocol() will be accurate.

[willchan no longer on Chromium, 2011-05-20 21:23:22]

Oops, my bad. Can we add a browser_test to test this integration to make sure I
don't break it in the future?

I'm trying to think about if this is the ideal way of doing this. A big
difference between URLRequestJobFactory::ProtocolHandler and
URLRequestJobFactory::Interceptor is that the former is tied to specific
protocols and the latter is not. I think I'd like to maintain this difference if
possible. Do you think it's possible to, whenever ProtocolHandlerRegistry adds a
ProtocolHandler for a new scheme, call
URLRequestJobFactory::SetProtocolHandler(). And when all ProtocolHandlers have
been removed for a scheme, call URLRequestJobFactory::SetProtocolHandler(scheme,
NULL);

I realize this is more burden for your use. It's less burden for all other
Interceptors and keeps their API simpler. I think keeping the API simpler is
more important.

What do you think? Feel free to push back if you think this request is
unreasonable.

[willchan no longer on Chromium, 2011-05-20 21:50:58]

LGTM, with the understanding that my suggested
URLRequestJobFactory::ProtocolHandler solution is cleaner and we should do that
long-term.

[oshima, 2011-05-25 22:45:23]

http://codereview.chromium.org/7056003/diff/1/net/url_request/url_request_job...
File net/url_request/url_request_job_factory_unittest.cc (right):

http://codereview.chromium.org/7056003/diff/1/net/url_request/url_request_job...
net/url_request/url_request_job_factory_unittest.cc:168: DummyInterceptor*
interceptor = new DummyInterceptor;
This seems to be leaking. I also wonder if "new DummyIntercepter" below may be
just "interceptor"?

[willchan no longer on Chromium, 2011-05-25 22:53:56]

http://codereview.chromium.org/7056003/diff/1/net/url_request/url_request_job...
File net/url_request/url_request_job_factory_unittest.cc (right):

http://codereview.chromium.org/7056003/diff/1/net/url_request/url_request_job...
net/url_request/url_request_job_factory_unittest.cc:168: DummyInterceptor*
interceptor = new DummyInterceptor;
On 2011/05/25 22:45:23, oshima wrote:
> This seems to be leaking. I also wonder if "new DummyIntercepter" below may be
> just "interceptor"?

Yes, that's correct. We should fix this.

[koz (OOO until 15th September), 2011-05-25 22:54:57]

http://codereview.chromium.org/7056003/diff/1/net/url_request/url_request_job...
File net/url_request/url_request_job_factory_unittest.cc (right):

http://codereview.chromium.org/7056003/diff/1/net/url_request/url_request_job...
net/url_request/url_request_job_factory_unittest.cc:168: DummyInterceptor*
interceptor = new DummyInterceptor;
On 2011/05/25 22:53:56, willchan wrote:
> On 2011/05/25 22:45:23, oshima wrote:
> > This seems to be leaking. I also wonder if "new DummyIntercepter" below may
be
> > just "interceptor"?
> 
> Yes, that's correct. We should fix this.

Yep, fixing now.

[oshima, 2011-05-25 23:54:22]

On 2011/05/25 22:54:57, koz wrote:
>
http://codereview.chromium.org/7056003/diff/1/net/url_request/url_request_job...
> File net/url_request/url_request_job_factory_unittest.cc (right):
> 
>
http://codereview.chromium.org/7056003/diff/1/net/url_request/url_request_job...
> net/url_request/url_request_job_factory_unittest.cc:168: DummyInterceptor*
> interceptor = new DummyInterceptor;
> On 2011/05/25 22:53:56, willchan wrote:
> > On 2011/05/25 22:45:23, oshima wrote:
> > > This seems to be leaking. I also wonder if "new DummyIntercepter" below
may
> be
> > > just "interceptor"?
> > 
> > Yes, that's correct. We should fix this.
> 
> Yep, fixing now.
If it was passing, you probably want to improve your test as well.

[koz (OOO until 15th September), 2011-05-26 00:03:24]

On 2011/05/25 23:54:22, oshima wrote:
> > Yep, fixing now.
> If it was passing, you probably want to improve your test as well.

The test is fine after the fix in r86746 - the bug was that we were asserting
that a new, private dummy interceptor was not being called, which would always
be true. Now the test is actually testing something :-)

[oshima, 2011-05-26 00:41:40]

koz, can you remove suppression as well?

On Wed, May 25, 2011 at 5:03 PM, <koz@chromium.org> wrote:

> On 2011/05/25 23:54:22, oshima wrote:
>
>> > Yep, fixing now.
>> If it was passing, you probably want to improve your test as well.
>>
>
> The test is fine after the fix in r86746 - the bug was that we were
> asserting
> that a new, private dummy interceptor was not being called, which would
> always
> be true. Now the test is actually testing something :-)
>
>
> http://codereview.chromium.org/7056003/
>

[koz (OOO until 15th September), 2011-05-26 01:37:14]

On 2011/05/26 00:41:40, oshima wrote:
> koz, can you remove suppression as well?
> 
> On Wed, May 25, 2011 at 5:03 PM, <mailto:koz@chromium.org> wrote:
> 
> > On 2011/05/25 23:54:22, oshima wrote:
> >
> >> > Yep, fixing now.
> >> If it was passing, you probably want to improve your test as well.
> >>
> >
> > The test is fine after the fix in r86746 - the bug was that we were
> > asserting
> > that a new, private dummy interceptor was not being called, which would
> > always
> > be true. Now the test is actually testing something :-)
> >
> >
> > http://codereview.chromium.org/7056003/
> >

Oh, I didn't know there was a suppression, and I can't find one either. Did you
make one?

[oshima, 2011-05-26 01:46:28]

On Wed, May 25, 2011 at 6:37 PM, <koz@chromium.org> wrote:

> On 2011/05/26 00:41:40, oshima wrote:
>
>> koz, can you remove suppression as well?
>>
>
>  On Wed, May 25, 2011 at 5:03 PM, <mailto:koz@chromium.org> wrote:
>>
>
>  > On 2011/05/25 23:54:22, oshima wrote:
>> >
>> >> > Yep, fixing now.
>> >> If it was passing, you probably want to improve your test as well.
>> >>
>> >
>> > The test is fine after the fix in r86746 - the bug was that we were
>> > asserting
>> > that a new, private dummy interceptor was not being called, which would
>> > always
>> > be true. Now the test is actually testing something :-)
>> >
>> >
>> > http://codereview.chromium.org/7056003/
>> >
>>
>
> Oh, I didn't know there was a suppression, and I can't find one either. Did
> you
> make one?
>
> Sorry, I mixed with other leaks. No suppression, so you're good.

- oshima


>
> http://codereview.chromium.org/7056003/
>