Add policies to specify an enterprise web store.

Add policies to specify an enterprise web store.

Admins can specify the URL, name, and icon to be used for the enterprise
web store. The app itself is implemented as a component extension, with
some of the manifest values being specified by policy.

BUG=88464
TEST=New ComponentLoaderTest class added to unit_tests. Additional
manual testing: set "EnterpriseWebStoreURL" policy to a valid URL and
start up Chrome. Verify that there is an app on the new tab page that
links to the specified URL. If possible, try installing an extension from
somewhere on that URL, and ensure that no warning is shown before
showing the permission dialog.

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

[Patrick Dubroy, 2011-11-04 18:11:22]

Here's another crack at the enterprise web store thing, taking the approach
suggested by Aaron. The CL still needs some work, but let me know what you think
about the general approach.

[Aaron Boodman, 2011-11-06 23:05:40]

+yoz

This is a step closer. Note that ComponentLoader does not currently have a
Profile* from which to get preferences in order to test whether this component
extension should be added/modified.

Seems OK to add one though. Yoz?

I go back and forth about whether this code should be here, or someplace
external to the extension system. It seems like ideally somebody external
somewhere would just call Add() on ExtensionService for each component
extension. But ideally that needs to be done before normal extensions are
loaded, so that leads to a single initialization file, which is essentially what
ComponentLoader is.

http://codereview.chromium.org/8477005/diff/1/chrome/browser/extensions/compo...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/8477005/diff/1/chrome/browser/extensions/compo...
chrome/browser/extensions/component_loader.cc:242: Add(manifest, path,
manifest_overrides);
Instead of introducing this overrides thing, just add an overload of Add that
takes a DictionaryValue*, parse the string into a value here, and modify it
before passing to Add().

[Yoyo Zhou, 2011-11-07 17:06:38]

On 2011/11/06 23:05:40, Aaron Boodman wrote:
> +yoz
> 
> This is a step closer. Note that ComponentLoader does not currently have a
> Profile* from which to get preferences in order to test whether this component
> extension should be added/modified.
> 
> Seems OK to add one though. Yoz?

Should be okay, although we could get at the Profile via
extension_service_->profile().

[Patrick Dubroy, 2011-11-07 19:21:07]

Ok, this latest patch actually implements the policies properly. I also did some
refactoring in ComponentLoader to (hopefully) clean things up a little.

I'm pretty happy with this approach -- it solves the problem without being
scattered around in too many places.

[Aaron Boodman, 2011-11-08 20:11:19]

I like this approach too.

Instead of digging prefs out of ExtensionService, can you make the dependency
explicit by passing it into ComponentLoader's constructor?

Also, can you please add some tests? I believe with ComponentLoader's current
dependencies it should be relatively easy to unit test. ES is a bit of a beast.
I can't remember how hard it is to create in unit tests. You might also look
into changing ComponentLoader's dependency to ExtensionServiceInterface (in
extension_service.h) if necessary.

[Patrick Dubroy, 2011-11-08 20:33:13]

On 2011/11/08 20:11:19, Aaron Boodman wrote:
> I like this approach too.
>
> Instead of digging prefs out of ExtensionService, can you make the dependency
> explicit by passing it into ComponentLoader's constructor?

Sure, will do.
 
> Also, can you please add some tests? I believe with ComponentLoader's current
> dependencies it should be relatively easy to unit test. ES is a bit of a
beast.
> I can't remember how hard it is to create in unit tests. You might also look
> into changing ComponentLoader's dependency to ExtensionServiceInterface (in
> extension_service.h) if necessary.

Sure. I was going to write some but wanted to make sure that you agreed with
this approach first.

[Patrick Dubroy, 2011-11-10 17:30:07]

I've added a new ComponentLoaderTest class to unit_tests. aa + yoz, can you
please review?

[Patrick Dubroy, 2011-11-14 22:13:07]

Ping.

[Aaron Boodman, 2011-11-15 17:34:47]

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:66:
FRIEND_TEST(ComponentLoaderTest, EnterpriseWebStore);
Hate friend. Can you modify the public API to make it good for testing?

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader_unittest.cc (right):

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:18: class
StubExtensionService : public TestExtensionService {
Name this "MockExtensionService?" Also, it should be in an anonymous namespace.

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:158: ASSERT_EQ(0U,
extension_service_.extensions()->size());
Nit: we use the lowercase "u" elsewhere in the codebase.

[Patrick Dubroy, 2011-11-15 19:19:16]

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:66:
FRIEND_TEST(ComponentLoaderTest, EnterpriseWebStore);
On 2011/11/15 17:34:47, Aaron Boodman wrote:
> Hate friend. Can you modify the public API to make it good for testing?

Sure, if you'd prefer. What's the problem with using FRIEND_TEST though? I agree
that it's preferable to void 'friend' in most cases, but for testing it seems
appropriate.

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader_unittest.cc (right):

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:18: class
StubExtensionService : public TestExtensionService {
On 2011/11/15 17:34:47, Aaron Boodman wrote:
> Name this "MockExtensionService?" Also, it should be in an anonymous
namespace.

Done and done.

I named it that way because some people distinguish between a stub and a mock
(http://en.wikipedia.org/wiki/Mock_object#Mocks.2C_fakes_and_stubs). I don't
really think it matters though. I can use "Mock" if that's the usual term used
in Chromium.

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:158: ASSERT_EQ(0U,
extension_service_.extensions()->size());
On 2011/11/15 17:34:47, Aaron Boodman wrote:
> Nit: we use the lowercase "u" elsewhere in the codebase.

Seems like we use both pretty heavily:

/usr/local/google/dubroy/chromium/src$ git-gs 0U | wc -l
1765
/usr/local/google/dubroy/chromium/src$ git-gs 0u | wc -l
1195

I don't care though, I can change it if you'd like.

[Aaron Boodman, 2011-11-15 22:22:56]

On 2011/11/15 19:19:16, dubroy wrote:
>
http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
> File chrome/browser/extensions/component_loader.h (right):
> 
>
http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
> chrome/browser/extensions/component_loader.h:66:
> FRIEND_TEST(ComponentLoaderTest, EnterpriseWebStore);
> On 2011/11/15 17:34:47, Aaron Boodman wrote:
> > Hate friend. Can you modify the public API to make it good for testing?
> 
> Sure, if you'd prefer. What's the problem with using FRIEND_TEST though? I
agree
> that it's preferable to void 'friend' in most cases, but for testing it seems
> appropriate.

Explained offline.

http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
> File chrome/browser/extensions/component_loader_unittest.cc (right):
> 
>
http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
> chrome/browser/extensions/component_loader_unittest.cc:18: class
> StubExtensionService : public TestExtensionService {
> On 2011/11/15 17:34:47, Aaron Boodman wrote:
> > Name this "MockExtensionService?" Also, it should be in an anonymous
> namespace.
> 
> Done and done.

Thanks.

> I named it that way because some people distinguish between a stub and a mock
> (http://en.wikipedia.org/wiki/Mock_object#Mocks.2C_fakes_and_stubs). I don't
> really think it matters though. I can use "Mock" if that's the usual term used
> in Chromium.
> 
>
http://codereview.chromium.org/8477005/diff/10001/chrome/browser/extensions/c...
> chrome/browser/extensions/component_loader_unittest.cc:158: ASSERT_EQ(0U,
> extension_service_.extensions()->size());
> On 2011/11/15 17:34:47, Aaron Boodman wrote:
> > Nit: we use the lowercase "u" elsewhere in the codebase.
> 
> Seems like we use both pretty heavily:
> 
> /usr/local/google/dubroy/chromium/src$ git-gs 0U | wc -l
> 1765
> /usr/local/google/dubroy/chromium/src$ git-gs 0u | wc -l
> 1195
> 
> I don't care though, I can change it if you'd like.

Data wins!

[Aaron Boodman, 2011-11-15 22:23:35]

LGTM, but please fix the "friends"s before landing

[commit-bot: I haz the power, 2011-11-18 08:42:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/8477005/23001

[commit-bot: I haz the power, 2011-11-18 08:43:40]

Presubmit check for 8477005-23001 failed and returned exit status 1.

Running presubmit commit checks ...
Finished checking
/mnt/data/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json.
0 errors, 0 warnings.

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/policy/configuration_policy_handler_list.cc

Presubmit checks took 2.8s to calculate.

[Mattias Nissler (ping if slow), 2011-11-18 09:38:43]

LGTM if you improve the policy documentation strings :)

http://codereview.chromium.org/8477005/diff/23001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/8477005/diff/23001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2080: 'desc': '''Enables an enterprise
web store, where users can install Chrome apps and extensions.''',
Can you put some more detail on what happens when the policy gets set? Also,
Julian recently made a pass over this file and adding in descriptions about what
happens if the policy is not configured (which is kind of pointless in most
cases, but admins want it), so can you do the same for the policies you added?

[Patrick Dubroy, 2011-11-18 09:59:11]

http://codereview.chromium.org/8477005/diff/23001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/8477005/diff/23001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2080: 'desc': '''Enables an enterprise
web store, where users can install Chrome apps and extensions.''',
On 2011/11/18 09:38:43, Mattias Nissler wrote:
> Can you put some more detail on what happens when the policy gets set? Also,
> Julian recently made a pass over this file and adding in descriptions about
what
> happens if the policy is not configured (which is kind of pointless in most
> cases, but admins want it), so can you do the same for the policies you added?

Done.

[commit-bot: I haz the power, 2011-11-18 09:59:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/8477005/24024

[commit-bot: I haz the power, 2011-11-18 11:01:03]

Can't apply patch for file chrome/app/policy/policy_templates.json.
While running patch -p1 --forward --force;
patching file chrome/app/policy/policy_templates.json
Hunk #1 FAILED at 100.
Hunk #2 succeeded at 2103 (offset 35 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

[commit-bot: I haz the power, 2011-11-18 13:00:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/8477005/33001

[commit-bot: I haz the power, 2011-11-18 14:20:56]

Try job failure for 8477005-33001 (retry) on win_rel for step "browser_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Jeffrey Yasskin, 2013-05-14 02:26:32]

FYI

https://codereview.chromium.org/8477005/diff/33001/chrome/browser/extensions/...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/8477005/diff/33001/chrome/browser/extensions/...
chrome/browser/extensions/component_loader.cc:178: Remove(path);
This Remove() call isn't working anymore, at least in tests. The
component_extensions_ vector gets paths as absolute paths, so they're never
equal to the relative path "enterprise_web_store".