Return a specific error code for unknown URL schemes.

Return a specific error code for unknown URL schemes.

The ERR_ABORTED error code is very general and for this particular case
we already have a corresponding net:: error case.


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

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

[Avi (use Gerrit), 2011-10-03 17:13:57]

Code LGTM. Do you have a bug you can reference in the commit message, perhaps an
indication of how to test it for the QA folks?

[jam, 2011-10-03 17:15:23]

1) why send it to multiple reviewers? patches should only be sent to one
reviewer in general, otherwise multiple people are spending time reviewing a
patch unnecessarily
2) I'm not familiar enough with this code, better to send it to someone who
works on networking code. I'd suggest either willchan/eroman/wtc

[mkosiba (inactive), 2011-10-03 17:42:07]

On 2011/10/03 17:15:23, John Abd-El-Malek wrote:
> 1) why send it to multiple reviewers? patches should only be sent to one
> reviewer in general, otherwise multiple people are spending time reviewing a
> patch unnecessarily
> 2) I'm not familiar enough with this code, better to send it to someone who
> works on networking code. I'd suggest either willchan/eroman/wtc

Looks like I included you by accident, probably because of 7e88fc14. Sorry about
that.

[mkosiba (inactive), 2011-10-03 17:58:29]

On 2011/10/03 17:13:57, Avi wrote:
> Code LGTM. Do you have a bug you can reference in the commit message, perhaps
an
> indication of how to test it for the QA folks?

I don't have a bug number for this. Do you want me to file one?

I'll add a unit test but as far as testing nothing except for "check if
foo://bar redirects to search" comes to mind.

[wtc, 2011-10-03 23:27:50]

I don't know when ResourceDispatcherHost::HandleExternalProtocol
is called, so my review is not authoritative.  But the
patch looks correct to me.

[mkosiba (inactive), 2011-10-04 10:43:04]

I added a unit test to verify the change.

Thanks!
    Martin

[wtc, 2011-10-04 18:45:37]

http://codereview.chromium.org/8117003/diff/1002/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/8117003/diff/1002/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:1232:
MakeTestRequest(0, 1, GURL("foo://bar"));
mkosiba: I don't understand why the network stack does
not return the net::ERR_UNKNOWN_URL_SCHEME error for this
URL.

Can you set breakpoints on the two return statements in
URLRequestJobManager::CreateJob() in
net/url_request/url_request_job_manager.cc, and see if
the breakpoints are hit when you run this unit test?

  if (job_factory) {
    if (!job_factory->IsHandledProtocol(scheme)) {
      return new URLRequestErrorJob(request, ERR_UNKNOWN_URL_SCHEME);
    }
  } else if (!SupportsScheme(scheme)) {
    return new URLRequestErrorJob(request, ERR_UNKNOWN_URL_SCHEME);
  }

Thank you.

[wtc, 2011-10-04 21:34:55]

http://codereview.chromium.org/8117003/diff/1002/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/8117003/diff/1002/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:1232:
MakeTestRequest(0, 1, GURL("foo://bar"));
I did the experiment in gdb.  I found that
net::URLRequestJobManager::CreateJob() is not called at all.
Looking at ResourceDispatcherHost::BeginRequest(), I think
this is because we don't create a net::URLRequest object in
this case.

[mkosiba (inactive), 2011-10-05 10:13:49]

On 2011/10/04 21:34:55, wtc wrote:
>
http://codereview.chromium.org/8117003/diff/1002/content/browser/renderer_hos...
> File content/browser/renderer_host/resource_dispatcher_host_unittest.cc
(right):
> 
>
http://codereview.chromium.org/8117003/diff/1002/content/browser/renderer_hos...
> content/browser/renderer_host/resource_dispatcher_host_unittest.cc:1232:
> MakeTestRequest(0, 1, GURL("foo://bar"));
> I did the experiment in gdb.  I found that
> net::URLRequestJobManager::CreateJob() is not called at all.
> Looking at ResourceDispatcherHost::BeginRequest(), I think
> this is because we don't create a net::URLRequest object in
> this case.

This is what I've found as well. Does this mean that the correct fix is to use
net::URLRequest, or is my change OK?

[wtc, 2011-10-05 22:59:23]

willchan: please review this CL.  I also have a question for you at
the end of this comment.

On 2011/10/05 10:13:49, Martin Kosiba wrote:
>
> This is what I've found as well. Does this mean that the correct fix is to use
> net::URLRequest, or is my change OK?

mkosiba: Your change seems OK to me.  Unfortunately I don't know
the ResourceDispatcherHost code very well, even though I work on
the network stack.

The experiments done by you and me showed that
ResourceDispatcherHost and the network stack (represented by
the net::URLRequest class) each have a list of URL schemes it
can handle.  So what we discovered seems like a duplication of
effort, which is not necessarily a bug.

svn blame shows that willchan seems to know the
job_factory.IsHandledURL() related code in ResourceDispatcherHost
well.  Perhaps he can shed some light on why ResourceDispatcherHost
doesn't just rely on net::URLRequestJobManager::CreateJob() to detect
unknown URL schemes.

[willchan no longer on Chromium, 2011-10-07 18:41:42]

On 2011/10/05 22:59:23, wtc wrote:
> willchan: please review this CL.  I also have a question for you at
> the end of this comment.
> 
> On 2011/10/05 10:13:49, Martin Kosiba wrote:
> >
> > This is what I've found as well. Does this mean that the correct fix is to
use
> > net::URLRequest, or is my change OK?
> 
> mkosiba: Your change seems OK to me.  Unfortunately I don't know
> the ResourceDispatcherHost code very well, even though I work on
> the network stack.
> 
> The experiments done by you and me showed that
> ResourceDispatcherHost and the network stack (represented by
> the net::URLRequest class) each have a list of URL schemes it
> can handle.  So what we discovered seems like a duplication of
> effort, which is not necessarily a bug.
> 
> svn blame shows that willchan seems to know the
> job_factory.IsHandledURL() related code in ResourceDispatcherHost
> well.  Perhaps he can shed some light on why ResourceDispatcherHost
> doesn't just rely on net::URLRequestJobManager::CreateJob() to detect
> unknown URL schemes.

Because of features like
https://developer.mozilla.org/En/DOM/Window.navigator.registerProtocolHandler
that allow for registering new schemes on a per-profile basis.
net::URLRequestJobManager is not profile-context aware, it needs to delegate to
a URLRequestContext specific object like URLRequestJobFactory which is intended
to subsume URLRequestJobManager.

[wtc, 2011-10-07 20:19:00]

willchan: thanks for answering my question.  Could you take
a quick look at this CL and see if it makes sense?

[willchan no longer on Chromium, 2011-10-09 06:08:52]

lgtm

[commit-bot: I haz the power, 2011-10-10 10:55:04]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/8117003/1002

[commit-bot: I haz the power, 2011-10-10 12:23:22]

Change committed as 104712

[commit-bot: I haz the power, 2011-11-16 10:24:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/8117003/19001

[commit-bot: I haz the power, 2011-11-16 11:57:09]

Change committed as 110287