Add in-profile external_extensions.json file for Chrome OS OEM customization.

Add in-profile external_extensions.json file for Chrome OS OEM customization.

BUG=57289
TEST=ExtensionServiceTest.ExternalPrefProvider, manualy craete an external_extensions.json file in the new in-profile path.

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

[Sam Kerner (Chrome), 2011-01-21 15:47:01]

Erik, 
  Comments point out issues we discussed yesterday on another code review.

Sam

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_extension_loader.cc (left):

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:46:
CHECK(!base_path_.empty());
Yesterday we talked about changing this to:
  CHECK(!prefs_->empty());

That doesn't work because by the time this function is called, prefs_ has been
passed to a consumer and prefs.get() is NULL.  I added a check below that has
the same effect.

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

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:89:
CHECK(!prefs_->empty() || !base_path_.empty());
New CHECK here.

[Erik does not do reviews, 2011-01-21 17:05:25]

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

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:395: CHECK(extensions_service);
you can remove this since the next line crashes the same way

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

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:3037: // Test that
running multiple update checks simultaneously does not
just curious, was there a bug here that this would have caught?

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:3056: // Two calls
should cause two checks for external extensions.
nit: newline above comment

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:3067: // Two checks for
external updates should find the extension, an install it
an->and

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:3087: // Two calls
should cause two checks for external extensions.
nit: newline
also, add comment why loaded should be 0 here

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

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:63: //
PathServcie::Get() to return false, because the path does
PathService

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:66: // OS develoer
builds and linux install packages.
developer

http://codereview.chromium.org/6370009/diff/2001/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/6370009/diff/2001/chrome/common/chrome_paths.c...
chrome/common/chrome_paths.cc:325: if (!PathService::Get(chrome::DIR_USER_DATA,
&cur))
I haven't been following your conversations with Dmitry here, but this seems a
bit odd.  How will the files get written separately into each user's profile? 
Won't it need to be in some shared location?

[Dmitry Polukhin, 2011-01-21 17:46:19]

http://codereview.chromium.org/6370009/diff/2001/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/6370009/diff/2001/chrome/common/chrome_paths.c...
chrome/common/chrome_paths.cc:325: if (!PathService::Get(chrome::DIR_USER_DATA,
&cur))
On 2011/01/21 17:05:25, Erik Kay wrote:
> I haven't been following your conversations with Dmitry here, but this seems a
> bit odd.  How will the files get written separately into each user's profile? 
> Won't it need to be in some shared location?

It can't be some shared location because it needs to be writable by user. If it
is writable, it is security issue that one user will be able to install
extensions to all other user on this machine. There is solution with shared
location but it requires signing of the file itself so if file was unauthorized
we can detect it and don't apply. Moreover we already have such shared location
/opt/google/chrome/extensions/... Per-user location was chosen as a compromise
solution that is secure and is applied only once (i.e. if some app is installed
initially, it won't be removed with some OS update or server changes).

[Sam Kerner (Chrome), 2011-01-21 17:59:26]

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

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:395: CHECK(extensions_service);
On 2011/01/21 17:05:25, Erik Kay wrote:
> you can remove this since the next line crashes the same way

Done.

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

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:3037: // Test that
running multiple update checks simultaneously does not
On 2011/01/21 17:05:25, Erik Kay wrote:
> just curious, was there a bug here that this would have caught?

No.  I created the test while reading the code to verify that it worked, and
make it easy to step thru in the debugger.  Committing it should help catch
regressions.

Dmitry will need to call this function after updates to the OEM manifest. 
Today, the only call to CheckForExternalUpdates() is in the ExtensionService
constructor.  I wanted to make sure Dmitry's use case won't break anything.

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:3056: // Two calls
should cause two checks for external extensions.
On 2011/01/21 17:05:25, Erik Kay wrote:
> nit: newline above comment

Done.

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:3067: // Two checks for
external updates should find the extension, an install it
On 2011/01/21 17:05:25, Erik Kay wrote:
> an->and

Done.

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service_unittest.cc:3087: // Two calls
should cause two checks for external extensions.
On 2011/01/21 17:05:25, Erik Kay wrote:
> nit: newline
> also, add comment why loaded should be 0 here

Done.

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

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:63: //
PathServcie::Get() to return false, because the path does
On 2011/01/21 17:05:25, Erik Kay wrote:
> PathService

Done.

http://codereview.chromium.org/6370009/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:66: // OS develoer
builds and linux install packages.
On 2011/01/21 17:05:25, Erik Kay wrote:
> developer

Done.

[Erik does not do reviews, 2011-01-21 19:08:20]

LGTM