Allow relative paths to external extension files for some providers, error out for others.

Allow relative paths to external extension files for some providers, error out for others.

This change will enable a fix for issue 57289, which requires a different base directory for extensions.

BUG=70111
TEST=ExtensionServiceTest.ExternalPrefProvider

[Sam Kerner (Chrome), 2011-01-14 19:44:21]

Gábor,
  In the past, external extensions from the windows registry had to have
absolute paths.  External extensions from external_extensions.json could use
absolute paths, which would be interpreted as relative to
app::DIR_EXTERNAL_EXTENSIONS .  Your recent refactor changed the windows
registry case, and I failed to notice in the code review.  This change restores
the old behavior.

  How should the policy external extension provider deal with relative paths to
CRX files?  Should they be relative to a directory in which we suggest that
policy authors put files, or should we require an absolute path?

Sam

[gfeher, 2011-01-14 21:23:17]

Sam,

Sorry for my mistake. This looks overall good. Please check my comment. The
policy providers currently don't support crx files, so they are fine with the
default (no-relative) behavior.

I have an idea for this CL, but I'm also fine with the current approach. How
about storing the base path in ExternalExtensionProviderImpl, and initializing
it the same style as download_location_ and crx_location_?

Gabor

http://codereview.chromium.org/6293006/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_extension_loader.cc (right):

http://codereview.chromium.org/6293006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_extension_loader.cc:19:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
My understanding is that this will be called on the UI thread.

[Sam Kerner (Chrome), 2011-01-17 20:15:51]

Ready for another look.

Fixed a bug in the previous version:  PathService::Get() should be called on the
file thread.  It may create the fetched directory for some path keys.

On 2011/01/14 21:23:17, gfeher wrote:
> Sam,
> 
> Sorry for my mistake. This looks overall good. Please check my comment. The
> policy providers currently don't support crx files, so they are fine with the
> default (no-relative) behavior.
> 
> I have an idea for this CL, but I'm also fine with the current approach. How
> about storing the base path in ExternalExtensionProviderImpl, and initializing
> it the same style as download_location_ and crx_location_?

I started implementing this, and I slightly prefer the current setup.  The path
under which exteranl extensions are searched for as they load seems to be the
responsibility of the ExternalPrefExtensionLoader.  Getting the path with
PathService::Get() shoudl happen on the file thread (because it may create a
directory), so I would have to take the path key from
ExternalExtensionProviderImpl() and pass it to ExternalPrefExtensionLoader to
actually get the path.


> 
> Gabor
> 
>
http://codereview.chromium.org/6293006/diff/2001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/external_extension_loader.cc (right):
> 
>
http://codereview.chromium.org/6293006/diff/2001/chrome/browser/extensions/ex...
> chrome/browser/extensions/external_extension_loader.cc:19:
> CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
> My understanding is that this will be called on the UI thread.

[Sam Kerner (Chrome), 2011-01-17 20:21:35]

http://codereview.chromium.org/6293006/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_extension_loader.cc (right):

http://codereview.chromium.org/6293006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_extension_loader.cc:19:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2011/01/14 21:23:17, gfeher wrote:
> My understanding is that this will be called on the UI thread.

Yes, my mistake.

[gfeher, 2011-01-18 08:59:43]

LGTM