Replaced static URLRequestFileJob factory with non-static protocol handler for File jobs

Replaced static URLRequestFileJob factory with non-static protocol handler for File jobs

This is a refactor and shouldn't have any visible behavioral change.

BUG=crbug.com/142945
TEST=browser_tests --single_process --gtest_filter=FullscreenControllerTest.FullscreenFileURL

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

[erikwright (departed), 2012-07-13 14:50:36]

http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:319:
chrome::kFtpScheme,
kFileScheme?

http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:555: chrome::kFtpScheme,
kFileScheme?

http://codereview.chromium.org/10700117/diff/2001/net/url_request/url_request...
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/10700117/diff/2001/net/url_request/url_request...
net/url_request/url_request_file_job.cc:265: return
delegate->CanAccessFile(request, path);
I would just inline this at this point. Replace calls to
IsFileAccessAllowed(request, path, delegate) with (!delegate ||
delegate->CanAccessFile(request, path)).

[shalev, 2012-07-17 19:40:16]

http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:319:
chrome::kFtpScheme,
On 2012/07/13 14:50:36, erikwright wrote:
> kFileScheme?

Done.

http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:555: chrome::kFtpScheme,
On 2012/07/13 14:50:36, erikwright wrote:
> kFileScheme?

Done.

http://codereview.chromium.org/10700117/diff/2001/net/url_request/url_request...
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/10700117/diff/2001/net/url_request/url_request...
net/url_request/url_request_file_job.cc:265: return
delegate->CanAccessFile(request, path);
On 2012/07/13 14:50:36, erikwright wrote:
> I would just inline this at this point. Replace calls to
> IsFileAccessAllowed(request, path, delegate) with (!delegate ||
> delegate->CanAccessFile(request, path)).

Done.

[shalev, 2012-08-14 19:50:05]

Hi Matt,

This cl introduces a protocol handler for file jobs, analogously to the cls for
ftp, data, about, and http. Please take a look.

Thanks,

Shalev

[willchan no longer on Chromium, 2012-08-14 20:45:47]

Is there a metabug tracking all this work? Please add a BUG link if so. And if
there isn't, it'd be nice to create a metabug for it :)

A better CL description would be appreciated too. In particular, explicitly
calling out that it's a refactor that shouldn't have any visible behavioral
change.

[mmenke, 2012-08-15 14:57:15]

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:259:
chrome::kFileScheme, new net::FileProtocolHandler(NULL));
Suggest a comment that a FileProtocolHandler without a network delegate will
deny all file requests.  Same goes for the the other file.

We do still want a file handler here because otherwise it would presumably be
treated as an unknown scheme, once we get rid of the factory.

http://codereview.chromium.org/10700117/diff/31001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

http://codereview.chromium.org/10700117/diff/31001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:90:
EXPECT_EQ(false, params.resize_pending);
Why this change?  Doesn't seem to belong in this CL.  Also, the old code is the
preferred form for testing bools.

http://codereview.chromium.org/10700117/diff/31001/net/url_request/file_proto...
File net/url_request/file_protocol_handler.h (right):

http://codereview.chromium.org/10700117/diff/31001/net/url_request/file_proto...
net/url_request/file_protocol_handler.h:17: // Implements a ProtocolHandler for
File jobs.
Suggest a comment that all file requests will fail with ERR_ACCESS_DENIED if
network_delegate is NULL.

http://codereview.chromium.org/10700117/diff/31001/net/url_request/url_reques...
File net/url_request/url_request_file_job.h (right):

http://codereview.chromium.org/10700117/diff/31001/net/url_request/url_reques...
net/url_request/url_request_file_job.h:62: NetworkDelegate* network_delegate);
Since you manually inlined this function, you need to delete its declaration.

[erikwright (departed), 2012-08-15 19:02:02]

URLRequestJobManager returns an Error job when a scheme is unknown, with
ERR_FAILED. The file protocol handler without a network delegate returns
ERR_ACCESS_DENIED.

That being said, I don't see that there is any meaningful difference between the
two, especially since this is an error being returned from the extensions
context, and the difference between the two errors is probably not actually
detectable by an extension.

So I propose making the network_delegate a required parameter to the
URLRequestFileJob protocol handler and not adding it in the cases where the
network_delegate was NULL.

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:259:
chrome::kFileScheme, new net::FileProtocolHandler(NULL));
On 2012/08/15 14:57:16, Matt Menke wrote:
> Suggest a comment that a FileProtocolHandler without a network delegate will
> deny all file requests.  Same goes for the the other file.
> 
> We do still want a file handler here because otherwise it would presumably be
> treated as an unknown scheme, once we get rid of the factory.

Wouldn't an unknown scheme be a reasonable treatment?

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.cc (right):

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:597: request, path,
request->context()->network_delegate());
Perhaps just go ahead and add the network_delegate to the parameters to
CreateDevToolsProtocolHandler and DevToolsJobFactory::DevToolsJobFactory, and
make it a member of DevToolsJobFactory?

[mmenke1, 2012-08-15 19:13:15]

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:259:
chrome::kFileScheme, new net::FileProtocolHandler(NULL));
On 2012/08/15 19:02:03, erikwright wrote:
> On 2012/08/15 14:57:16, Matt Menke wrote:
> > Suggest a comment that a FileProtocolHandler without a network delegate will
> > deny all file requests.  Same goes for the the other file.
> > 
> > We do still want a file handler here because otherwise it would presumably
be
> > treated as an unknown scheme, once we get rid of the factory.
> 
> Wouldn't an unknown scheme be a reasonable treatment?

We currently blindly try to pass unknown schemes to the OS.  This just seems a
like a really bad idea with file:// URLs.  I'm assuming extensions deliberately
don't have access to file URLs, though I'm not familiar enough with them to be
sure.

[erikwright (departed), 2012-08-15 19:20:25]

On 2012/08/15 19:13:15, mmenke (incorrect) wrote:
>
http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
> File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):
> 
>
http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
> chrome/browser/profiles/off_the_record_profile_io_data.cc:259:
> chrome::kFileScheme, new net::FileProtocolHandler(NULL));
> On 2012/08/15 19:02:03, erikwright wrote:
> > On 2012/08/15 14:57:16, Matt Menke wrote:
> > > Suggest a comment that a FileProtocolHandler without a network delegate
will
> > > deny all file requests.  Same goes for the the other file.
> > > 
> > > We do still want a file handler here because otherwise it would presumably
> be
> > > treated as an unknown scheme, once we get rid of the factory.
> > 
> > Wouldn't an unknown scheme be a reasonable treatment?
> 
> We currently blindly try to pass unknown schemes to the OS.  This just seems a
> like a really bad idea with file:// URLs.  I'm assuming extensions
deliberately
> don't have access to file URLs, though I'm not familiar enough with them to be
> sure.

Really? I haven't seen the code for that. Is it at the URLRequest level or
higher up? In the former case, it should probably be something that
URLRequestJobManager and the job_factory are responsible for. In the latter
case, the change we are discussing wouldn't affect it.

[mmenke1, 2012-08-15 19:34:22]

On 2012/08/15 19:20:25, erikwright wrote:
> On 2012/08/15 19:13:15, mmenke (incorrect) wrote:
> >
>
http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
> > File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):
> > 
> >
>
http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
> > chrome/browser/profiles/off_the_record_profile_io_data.cc:259:
> > chrome::kFileScheme, new net::FileProtocolHandler(NULL));
> > On 2012/08/15 19:02:03, erikwright wrote:
> > > On 2012/08/15 14:57:16, Matt Menke wrote:
> > > > Suggest a comment that a FileProtocolHandler without a network delegate
> will
> > > > deny all file requests.  Same goes for the the other file.
> > > > 
> > > > We do still want a file handler here because otherwise it would
presumably
> > be
> > > > treated as an unknown scheme, once we get rid of the factory.
> > > 
> > > Wouldn't an unknown scheme be a reasonable treatment?
> > 
> > We currently blindly try to pass unknown schemes to the OS.  This just seems
a
> > like a really bad idea with file:// URLs.  I'm assuming extensions
> deliberately
> > don't have access to file URLs, though I'm not familiar enough with them to
be
> > sure.
> 
> Really? I haven't seen the code for that. Is it at the URLRequest level or
> higher up? In the former case, it should probably be something that
> URLRequestJobManager and the job_factory are responsible for. In the latter
> case, the change we are discussing wouldn't affect it.

ResourceDispatcherHostImpl::HandleExternalProtocol.  It checks if URLs are
handled by the job_factory before trying to pass them on to its delegate for
external handling.

[erikwright (departed), 2012-08-15 19:52:05]

OK. Add a TODO: Without a network_delegate this protocol handler will never
handle file: requests, but as a side effect it makes
job_factory::IsHandledProtocol return true, which prevents attempts to
handle the protocol externally.

On Wed, Aug 15, 2012 at 3:34 PM, <mmenke@google.com> wrote:

> On 2012/08/15 19:20:25, erikwright wrote:
>
>> On 2012/08/15 19:13:15, mmenke (incorrect) wrote:
>> >
>>
>
> http://codereview.chromium.**org/10700117/diff/31001/**
>
chrome/browser/profiles/off_**the_record_profile_io_data.cc<http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc>
>
>> > File chrome/browser/profiles/off_**the_record_profile_io_data.cc
>> (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/10700117/diff/31001/**
>
chrome/browser/profiles/off_**the_record_profile_io_data.cc#**newcode259<http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc#newcode259>
>
>> > chrome/browser/profiles/off_**the_record_profile_io_data.cc:**259:
>> > chrome::kFileScheme, new net::FileProtocolHandler(NULL)**);
>> > On 2012/08/15 19:02:03, erikwright wrote:
>> > > On 2012/08/15 14:57:16, Matt Menke wrote:
>> > > > Suggest a comment that a FileProtocolHandler without a network
>> delegate
>> will
>> > > > deny all file requests.  Same goes for the the other file.
>> > > >
>> > > > We do still want a file handler here because otherwise it would
>>
> presumably
>
>> > be
>> > > > treated as an unknown scheme, once we get rid of the factory.
>> > >
>> > > Wouldn't an unknown scheme be a reasonable treatment?
>> >
>> > We currently blindly try to pass unknown schemes to the OS.  This just
>> seems
>>
> a
>
>> > like a really bad idea with file:// URLs.  I'm assuming extensions
>> deliberately
>> > don't have access to file URLs, though I'm not familiar enough with
>> them to
>>
> be
>
>> > sure.
>>
>
>  Really? I haven't seen the code for that. Is it at the URLRequest level or
>> higher up? In the former case, it should probably be something that
>> URLRequestJobManager and the job_factory are responsible for. In the
>> latter
>> case, the change we are discussing wouldn't affect it.
>>
>
> ResourceDispatcherHostImpl::**HandleExternalProtocol.  It checks if URLs
> are
> handled by the job_factory before trying to pass them on to its delegate
> for
> external handling.
>
>
http://codereview.chromium.**org/10700117/<http://codereview.chromium.org/107...
>

[shalev, 2012-08-15 21:23:41]

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:259:
chrome::kFileScheme, new net::FileProtocolHandler(NULL));
On 2012/08/15 19:13:16, mmenke (incorrect) wrote:
> On 2012/08/15 19:02:03, erikwright wrote:
> > On 2012/08/15 14:57:16, Matt Menke wrote:
> > > Suggest a comment that a FileProtocolHandler without a network delegate
will
> > > deny all file requests.  Same goes for the the other file.
> > > 
> > > We do still want a file handler here because otherwise it would presumably
> be
> > > treated as an unknown scheme, once we get rid of the factory.
> > 
> > Wouldn't an unknown scheme be a reasonable treatment?
> 
> We currently blindly try to pass unknown schemes to the OS.  This just seems a
> like a really bad idea with file:// URLs.  I'm assuming extensions
deliberately
> don't have access to file URLs, though I'm not familiar enough with them to be
> sure.

Done.

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.cc (right):

http://codereview.chromium.org/10700117/diff/31001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:597: request, path,
request->context()->network_delegate());
On 2012/08/15 19:02:03, erikwright wrote:
> Perhaps just go ahead and add the network_delegate to the parameters to
> CreateDevToolsProtocolHandler and DevToolsJobFactory::DevToolsJobFactory, and
> make it a member of DevToolsJobFactory?

Done.

http://codereview.chromium.org/10700117/diff/31001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

http://codereview.chromium.org/10700117/diff/31001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:90:
EXPECT_EQ(false, params.resize_pending);
On 2012/08/15 14:57:16, Matt Menke wrote:
> Why this change?  Doesn't seem to belong in this CL.  Also, the old code is
the
> preferred form for testing bools.

Oops, this change is not supposed to be here (it was only there temporarily
because I was investigating why
EXPECT_EQ(false, __)
seems to not build on my machine).

http://codereview.chromium.org/10700117/diff/31001/net/url_request/file_proto...
File net/url_request/file_protocol_handler.h (right):

http://codereview.chromium.org/10700117/diff/31001/net/url_request/file_proto...
net/url_request/file_protocol_handler.h:17: // Implements a ProtocolHandler for
File jobs.
On 2012/08/15 14:57:16, Matt Menke wrote:
> Suggest a comment that all file requests will fail with ERR_ACCESS_DENIED if
> network_delegate is NULL.

Done.

http://codereview.chromium.org/10700117/diff/31001/net/url_request/url_reques...
File net/url_request/url_request_file_job.h (right):

http://codereview.chromium.org/10700117/diff/31001/net/url_request/url_reques...
net/url_request/url_request_file_job.h:62: NetworkDelegate* network_delegate);
On 2012/08/15 14:57:16, Matt Menke wrote:
> Since you manually inlined this function, you need to delete its declaration.

Done.

[mmenke, 2012-08-16 14:54:31]

LGTM with nits addressed

https://chromiumcodereview.appspot.com/10700117/diff/23019/chrome/browser/ext...
File chrome/browser/extensions/extension_resource_protocols.cc (right):

https://chromiumcodereview.appspot.com/10700117/diff/23019/chrome/browser/ext...
chrome/browser/extensions/extension_resource_protocols.cc:22:
request->context()->network_delegate()),
While you're here, could you fix this indent?  Should start at 4 spaces, not 2.

https://chromiumcodereview.appspot.com/10700117/diff/23019/chrome/browser/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://chromiumcodereview.appspot.com/10700117/diff/23019/chrome/browser/pro...
chrome/browser/profiles/profile_impl_io_data.cc:481: DCHECK(set_protocol);
Know that I argued the other way for the file:// scheme, but I wonder if we
really need this for the media and extensions schemes.  Should leave well enough
alone in this CL, to preserve behavior, but something that may be worth looking
into.

https://chromiumcodereview.appspot.com/10700117/diff/23019/net/url_request/fi...
File net/url_request/file_protocol_handler.h (right):

https://chromiumcodereview.appspot.com/10700117/diff/23019/net/url_request/fi...
net/url_request/file_protocol_handler.h:22: FileProtocolHandler(NetworkDelegate*
network_delegate);
explicit

[shalev, 2012-08-16 15:07:25]

http://codereview.chromium.org/10700117/diff/23019/chrome/browser/extensions/...
File chrome/browser/extensions/extension_resource_protocols.cc (right):

http://codereview.chromium.org/10700117/diff/23019/chrome/browser/extensions/...
chrome/browser/extensions/extension_resource_protocols.cc:22:
request->context()->network_delegate()),
On 2012/08/16 14:54:31, Matt Menke wrote:
> While you're here, could you fix this indent?  Should start at 4 spaces, not
2.

Done.

http://codereview.chromium.org/10700117/diff/23019/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10700117/diff/23019/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:481: DCHECK(set_protocol);
On 2012/08/16 14:54:31, Matt Menke wrote:
> Know that I argued the other way for the file:// scheme, but I wonder if we
> really need this for the media and extensions schemes.  Should leave well
enough
> alone in this CL, to preserve behavior, but something that may be worth
looking
> into.

You're right. I will try to look at this soon and remove some of these protocol
handlers in a future CL.

http://codereview.chromium.org/10700117/diff/23019/net/url_request/file_proto...
File net/url_request/file_protocol_handler.h (right):

http://codereview.chromium.org/10700117/diff/23019/net/url_request/file_proto...
net/url_request/file_protocol_handler.h:22: FileProtocolHandler(NetworkDelegate*
network_delegate);
On 2012/08/16 14:54:31, Matt Menke wrote:
> explicit

Done.

[erikwright (departed), 2012-08-16 15:18:32]

LGTM.

[mmenke, 2012-08-16 15:24:34]

Oh, and I'm a profile/ owner now, so you can land without Will's signoff, though
you'll probably need owners for extensions/, webui/, and webkit/.

[Evan Stade, 2012-08-16 16:45:56]

webui lgtm

http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.cc (right):

http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:582:
net::NetworkDelegate* network_delegate_;
document ownership

http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager_backend.h:28: class
NetworkDelegate;
alphabetize

[not at google - send to devlin, 2012-08-16 22:38:08]

extensions lgtm

[shalev, 2012-08-17 15:55:13]

http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.cc (right):

http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:582:
net::NetworkDelegate* network_delegate_;
On 2012/08/16 16:45:56, Evan Stade wrote:
> document ownership

Done.

http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_url_data_manager_backend.h:28: class
NetworkDelegate;
On 2012/08/16 16:45:56, Evan Stade wrote:
> alphabetize

Done.

[brettw, 2012-08-17 17:22:05]

webkit lgtm, i only looked at test_shell.cc

[tony, 2012-08-20 17:13:01]

webkit LGTM

[commit-bot: I haz the power, 2012-08-20 17:48:20]

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

[commit-bot: I haz the power, 2012-08-20 20:30:48]

Change committed as 152381