Implement extension:// protocol.

Implement extension:// protocol.

[Erik does not do reviews, 2008-12-18 00:54:51]

LGTM

All of my comments are minor style / comment nits.

http://codereview.chromium.org/15010/diff/201/26
File chrome/browser/extensions/url_request_job_extension.cc (right):

http://codereview.chromium.org/15010/diff/201/26#newcode1
Line 1: #include "chrome/browser/extensions/url_request_job_extension.h"
Missing the file header comment block.

http://codereview.chromium.org/15010/diff/201/26#newcode39
Line 39: // It is hard to concatenate std::string to file paths cross-platform
because
The way this comment is worded makes me think that you'd prefer to just append
it to the FilePath directly, when in reality I think your approach is totally
the right way to go.  I don't think we'd want to put the logic of how to handle
URL-encoded paths into FilePath.  I'd just change this to "Build up a file://
URL and convert that back to a FilePath.  This avoids URL encoding and path
separator issues."  Maybe the method should even be changed to just take the
whole URL rather than the url_path for clarity.

http://codereview.chromium.org/15010/diff/201/26#newcode49
Line 49: file_url = GURL(file_url.spec() + url_path);
This makes me think that GURL should have an AppendToPath method.  While in
practice what you're doing here is safe, in feels weird because a real URL spec
might not end with a trailing / and might have a query param and/or a ref at the
end.

http://codereview.chromium.org/15010/diff/201/26#newcode60
Line 60: // TODO(aa): It would be nice if we have a PathIsContainedBy() function
Could you file a bug for this and reference it in the TODO?

http://codereview.chromium.org/15010/diff/201/26#newcode63
Line 63: if (!file_util::AbsolutePath(&ret_val) ||
Yuck.  I noticed while looking at this that IsAbsolute has different criteria
than AbsolutePath.  I filed bug 5650 for this.

http://codereview.chromium.org/15010/diff/201/26#newcode75
Line 75: }
missing EOF at end of file

http://codereview.chromium.org/15010/diff/201/27
File chrome/browser/extensions/url_request_job_extension.h (right):

http://codereview.chromium.org/15010/diff/201/27#newcode18
Line 18: static URLRequestJob* Factory(URLRequest* request,
Can't this be kept private since the registering is happening in
RegisterExtensionProtocol?

http://codereview.chromium.org/15010/diff/201/27#newcode36
Line 36: };
missing line at EOF

http://codereview.chromium.org/15010/diff/201/28
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/15010/diff/201/28#newcode175
Line 175: virtual const FilePath GetPathForExtension(const std::wstring& id) {
Did you mean for the return type to be const FilePath& here? or just FilePath?

http://codereview.chromium.org/15010/diff/201/29
File chrome/common/notification_types.h (right):

http://codereview.chromium.org/15010/diff/201/29#newcode507
Line 507: NOTIFY_NEW_EXTENSIONS,
This isn't official style, just my pet peeve.  It would be nice if this and the
greasemonkey notification were named in a way that category came first, similar
to java package naming, such as NOTIFY_EXTENSION_NEW.  That way all related
notifications are sortable together.  We're not 100% like that above, but most
of them are.

[darin (slow to review), 2008-12-18 08:39:47]

http://codereview.chromium.org/15010/diff/201/23
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/15010/diff/201/23#newcode405
Line 405: // Register our global network handler for chrome:// and extension://
URLs.
hmm.... why isn't the URL scheme prefixed by chrome- ?

http://codereview.chromium.org/15010/diff/201/27
File chrome/browser/extensions/url_request_job_extension.h (right):

http://codereview.chromium.org/15010/diff/201/27#newcode10
Line 10: class URLRequestExtensionJob : public URLRequestFileJob {
why do you need this class?  why not just use URLRequestFileJob directly?

i ask because there are no new member variables and no virtual functions being
overriden, so this is basically just a duplicate of URLRequestFileJob, which
doesn't seem to serve any purpose.

http://codereview.chromium.org/15010/diff/201/28
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/15010/diff/201/28#newcode248
Line 248: extension_paths_.insert(new_paths->begin(), new_paths->end());
this is probably a great use case for the std::map's swap method.

http://codereview.chromium.org/15010/diff/201/30
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/15010/diff/201/30#newcode70
Line 70: // Returns the path the specified extension is installed into, or an
empty
why are we adding information about extensions in the network stack?  this is
the wrong place for things related to something chrome specific like the
extension system.  everything else on the URLRequestContext is stuff known to
the network layer... exposed to the network layer by the user.

http://codereview.chromium.org/15010/diff/201/30#newcode72
Line 72: virtual const FilePath GetPathForExtension(const std::wstring& id) = 0;
nit: the use of 'const' in front of FilePath here is extraneous and should be
removed.  it doesn't add any value when used this way.  (if FilePath were a
reference type, then const could be useful.)

[Aaron Boodman, 2008-12-22 11:47:09]

Ready for another look. Note that this relies on
http://codereview.chromium.org/16408, but they should be separately reviewable.

http://codereview.chromium.org/15010/diff/201/23
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/15010/diff/201/23#newcode405
Line 405: // Register our global network handler for chrome:// and extension://
URLs.
On 2008/12/18 08:39:47, darin wrote:
> hmm.... why isn't the URL scheme prefixed by chrome- ?

Why would it be? Is there a convention or something? chrome-extension:// seems
unnecessarily verbose. I also noticed that we recently renamed "chrome-resource"
to "chrome" so I thought this was in keeping with that.

http://codereview.chromium.org/15010/diff/201/26
File chrome/browser/extensions/url_request_job_extension.cc (right):

http://codereview.chromium.org/15010/diff/201/26#newcode1
Line 1: #include "chrome/browser/extensions/url_request_job_extension.h"
On 2008/12/18 00:54:51, Erik Kay wrote:
> Missing the file header comment block.

Done.

http://codereview.chromium.org/15010/diff/201/26#newcode39
Line 39: // It is hard to concatenate std::string to file paths cross-platform
because
On 2008/12/18 00:54:51, Erik Kay wrote:
> The way this comment is worded makes me think that you'd prefer to just append
> it to the FilePath directly, when in reality I think your approach is totally
> the right way to go.  I don't think we'd want to put the logic of how to
handle
> URL-encoded paths into FilePath.  I'd just change this to "Build up a file://
> URL and convert that back to a FilePath.  This avoids URL encoding and path
> separator issues."  Maybe the method should even be changed to just take the
> whole URL rather than the url_path for clarity.

Good point, I changed the comment.

I like the method taking the arguments it does because it makes the important
bit trivially unit testable without having to mock out the interaction with
profile.

http://codereview.chromium.org/15010/diff/201/26#newcode49
Line 49: file_url = GURL(file_url.spec() + url_path);
On 2008/12/18 00:54:51, Erik Kay wrote:
> This makes me think that GURL should have an AppendToPath method.  While in
> practice what you're doing here is safe, in feels weird because a real URL
spec
> might not end with a trailing / and might have a query param and/or a ref at
the
> end.

It has a ReplaceComponents() method, I changed to use that instead. It still
doesn't handle the matter of the slash, so I put a DCHECK in there to ensure the
incoming path starts with slash.

http://codereview.chromium.org/15010/diff/201/26#newcode60
Line 60: // TODO(aa): It would be nice if we have a PathIsContainedBy() function
On 2008/12/18 00:54:51, Erik Kay wrote:
> Could you file a bug for this and reference it in the TODO?

I added a FilePath::Contains() method instead.

http://codereview.chromium.org/15010/diff/201/26#newcode75
Line 75: }
On 2008/12/18 00:54:51, Erik Kay wrote:
> missing EOF at end of file

Done.

http://codereview.chromium.org/15010/diff/201/27
File chrome/browser/extensions/url_request_job_extension.h (right):

http://codereview.chromium.org/15010/diff/201/27#newcode10
Line 10: class URLRequestExtensionJob : public URLRequestFileJob {
On 2008/12/18 08:39:47, darin wrote:
> why do you need this class?  why not just use URLRequestFileJob directly?
> 
> i ask because there are no new member variables and no virtual functions being
> overriden, so this is basically just a duplicate of URLRequestFileJob, which
> doesn't seem to serve any purpose.

Good point. I was blindly following URLRequestChromeFileJob, which also does
this. I removed the separate class and made all this code static. This required
changing the signature of URLRequestFileJob's constructor slightly and all the
call sites.

Note that the filename no longer makes much sense, but I'd rather wait until I
get a lgtm to fix that so I don't lose history on the review comments.

http://codereview.chromium.org/15010/diff/201/27#newcode18
Line 18: static URLRequestJob* Factory(URLRequest* request,
On 2008/12/18 00:54:51, Erik Kay wrote:
> Can't this be kept private since the registering is happening in
> RegisterExtensionProtocol?

Done.

http://codereview.chromium.org/15010/diff/201/27#newcode36
Line 36: };
On 2008/12/18 00:54:51, Erik Kay wrote:
> missing line at EOF
> 

Done.

http://codereview.chromium.org/15010/diff/201/28
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/15010/diff/201/28#newcode175
Line 175: virtual const FilePath GetPathForExtension(const std::wstring& id) {
On 2008/12/18 00:54:51, Erik Kay wrote:
> Did you mean for the return type to be const FilePath& here? or just FilePath?
 

Good point, fixed.

http://codereview.chromium.org/15010/diff/201/28#newcode248
Line 248: extension_paths_.insert(new_paths->begin(), new_paths->end());
On 2008/12/18 08:39:47, darin wrote:
> this is probably a great use case for the std::map's swap method.

I actually meant for this to be an append not a replacement. That way if groups
of extensions can be loaded separately later, they can reuse this.

http://codereview.chromium.org/15010/diff/201/29
File chrome/common/notification_types.h (right):

http://codereview.chromium.org/15010/diff/201/29#newcode507
Line 507: NOTIFY_NEW_EXTENSIONS,
On 2008/12/18 00:54:51, Erik Kay wrote:
> This isn't official style, just my pet peeve.  It would be nice if this and
the
> greasemonkey notification were named in a way that category came first,
similar
> to java package naming, such as NOTIFY_EXTENSION_NEW.  That way all related
> notifications are sortable together.  We're not 100% like that above, but most
> of them are.

Done.

http://codereview.chromium.org/15010/diff/201/30
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/15010/diff/201/30#newcode70
Line 70: // Returns the path the specified extension is installed into, or an
empty
On 2008/12/18 08:39:47, darin wrote:
> why are we adding information about extensions in the network stack?  this is
> the wrong place for things related to something chrome specific like the
> extension system.  everything else on the URLRequestContext is stuff known to
> the network layer... exposed to the network layer by the user.

Fixed by adding a new ChromeRequestContext and moving the chrome-specific
details there.

http://codereview.chromium.org/15010/diff/201/30#newcode72
Line 72: virtual const FilePath GetPathForExtension(const std::wstring& id) = 0;
On 2008/12/18 08:39:47, darin wrote:
> nit: the use of 'const' in front of FilePath here is extraneous and should be
> removed.  it doesn't add any value when used this way.  (if FilePath were a
> reference type, then const could be useful.)

Whoops, fixed.

[Erik does not do reviews, 2008-12-22 17:11:05]

LGTM

http://codereview.chromium.org/15010/diff/451/630
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/15010/diff/451/630#newcode246
Line 246: if (ChromeURLDataManager::URLToFilePath(request->url(), &path))
Could you add a variation for URLToFilePath to have a FilePath arg?

http://codereview.chromium.org/15010/diff/451/634
File chrome/browser/extensions/url_request_job_extension.h (right):

http://codereview.chromium.org/15010/diff/451/634#newcode11
Line 11: const std::string& resource_path);
Rename resource_path to be url_path and adjust the comment so that it's clear
why it's not a FilePath itself.

http://codereview.chromium.org/15010/diff/451/637
File chrome/browser/net/chrome_request_context.cc (right):

http://codereview.chromium.org/15010/diff/451/637#newcode118
Line 118: // NotificationObserver implementation.
remove leading whitespace