Add PolicyProvider to ExtensionManagement

Add StandardManagementPolicyProvider implementation based on ExtensionManagement Service.

Make ExtensionManagement listening to preference changes.

Move all functionality of admin_policy into StandardManagementPolicyProvider, the latter was essentially a wrapper over admin_policy before.

Merge admin_policy_unittest into extension_management_unittests with minor changes.

BUG=177351
TEST=ExtensionServiceTest,ExtensionManagementTest,ExtensionAdminPolicyTest

Committed: https://crrev.com/1569c9bdcf86c361b0b79c697d0da76d03d70a96
Cr-Commit-Position: refs/heads/master@{#293514}

[binjin, 2014-09-02 20:47:48]

binjin@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[binjin, 2014-09-02 20:47:49]

Hello Joao,

PTAL at this CL, there are several major changes and it's been a while as well
so I opened another CL issue.

-bjin

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:139: ObserverList<Observer>
observer_list_;
adding true as 2nd parameter fails the ExtensionServiceTest now, will
investigate later.

[Joao da Silva, 2014-09-03 11:52:54]

Looks good.

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:61: base::Unretained(this)));
Suggestion: all of these registrations use the same callback function. Instead
of creating 5 base::Callbacks with base::Bind, create just 1 and share it with
the registrations.

(base::Callbacks can be copied cheaply since they share the internal storage,
but calling base::Bind 5 times won't share those internal structures)

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:73: void
ExtensionManagement::RemoveObserver(Observer* obs) {
Rename "obs" to "observer"

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:44: virtual void
OnExtensionManagementPrefChanged() = 0;
Rename to "OnExtensionManagementSettingsChanged".

Prefs are an implementation detail that Observers shouldn't need to know about.

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:92: void
RemoveObserver(Observer* obs);
Rename "obs" to "observer"

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:139: ObserverList<Observer>
observer_list_;
On 2014/09/02 20:47:49, bjin wrote:
> adding true as 2nd parameter fails the ExtensionServiceTest now, will
> investigate later.

That suggests that there is some missing cleanup at shutdown.

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/test_extension_system.cc:50: process_manager_.reset();
The ObserverList failure at shutdown may be because this method isn't forwarding
to extension_service_->Shutdown

[binjin, 2014-09-03 15:31:37]

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:61: base::Unretained(this)));
On 2014/09/03 11:52:54, Joao da Silva wrote:
> Suggestion: all of these registrations use the same callback function. Instead
> of creating 5 base::Callbacks with base::Bind, create just 1 and share it with
> the registrations.
> 
> (base::Callbacks can be copied cheaply since they share the internal storage,
> but calling base::Bind 5 times won't share those internal structures)

Done.

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:73: void
ExtensionManagement::RemoveObserver(Observer* obs) {
On 2014/09/03 11:52:54, Joao da Silva wrote:
> Rename "obs" to "observer"

Done.

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:44: virtual void
OnExtensionManagementPrefChanged() = 0;
On 2014/09/03 11:52:54, Joao da Silva wrote:
> Rename to "OnExtensionManagementSettingsChanged".
> 
> Prefs are an implementation detail that Observers shouldn't need to know
about.

Done.

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:92: void
RemoveObserver(Observer* obs);
On 2014/09/03 11:52:54, Joao da Silva wrote:
> Rename "obs" to "observer"

Done.

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/500043003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/test_extension_system.cc:50: process_manager_.reset();
On 2014/09/03 11:52:54, Joao da Silva wrote:
> The ObserverList failure at shutdown may be because this method isn't
forwarding
> to extension_service_->Shutdown

Done.

[Joao da Silva, 2014-09-04 10:17:46]

lgtm

https://codereview.chromium.org/500043003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/500043003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:39: }
This is not needed

[binjin, 2014-09-04 10:43:09]

binjin@chromium.org changed reviewers:
+ darin@chromium.org, finnur@chromium.org

[binjin, 2014-09-04 10:43:10]

finnur: It's the following CL after https://codereview.chromium.org/499313002/ ,
PTAL at  chrome/browser/extensions/* and extensions/* changes

darin: Please review two minor changes:
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
chrome/browser/ui/webui/extensions/extension_settings_handler.cc

https://codereview.chromium.org/500043003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/500043003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:39: }
On 2014/09/04 10:17:46, Joao da Silva wrote:
> This is not needed

Done.

[Finnur, 2014-09-04 11:29:46]

I'm not an expert on management policies, so I'm trusting Joao da Silva for
that. I only have two comments...

https://codereview.chromium.org/500043003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/standard_management_policy_provider.cc (right):

https://codereview.chromium.org/500043003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/standard_management_policy_provider.cc:26: bool
modifiable_value) {
This function is a bit weirdly named and needs some explaining. What does the
return value mean, for example? The name doesn't reflect what it does, so it
makes the code at the bottom of this file harder to read.

https://codereview.chromium.org/500043003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/standard_management_policy_provider.cc:77: if
(extension->location() == Manifest::COMPONENT)
Should EXTERNAL_COMPONENT also be included?

[binjin, 2014-09-04 12:54:24]

Patchset #9 (id:160001) has been deleted

[binjin, 2014-09-04 13:06:57]

These code were inherited from (now deleted) admin_policy code. I made necessary
simplification though.

@Joao, could you please also have a look at these changes?

https://codereview.chromium.org/500043003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/standard_management_policy_provider.cc (right):

https://codereview.chromium.org/500043003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/standard_management_policy_provider.cc:26: bool
modifiable_value) {
On 2014/09/04 11:29:46, Finnur wrote:
> This function is a bit weirdly named and needs some explaining. What does the
> return value mean, for example? The name doesn't reflect what it does, so it
> makes the code at the bottom of this file harder to read.

Done.

https://codereview.chromium.org/500043003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/standard_management_policy_provider.cc:77: if
(extension->location() == Manifest::COMPONENT)
On 2014/09/04 11:29:46, Finnur wrote:
> Should EXTERNAL_COMPONENT also be included?

Done. For reference, it seems that starting crrev.com/ab89454 EXTERNAL_COMPONENT
was treat same as COMPONENT from management policy aspect.

[Joao da Silva, 2014-09-04 14:46:10]

Still lgtm after the recent changes.

[Finnur, 2014-09-04 16:06:14]

LGTM

[binjin, 2014-09-04 16:13:13]

binjin@chromium.org changed reviewers:
+ erg@chromium.org, noms@chromium.org

[binjin, 2014-09-04 16:13:14]

erg, noms: please take a quick look on changes to
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc	

thanks

-bjin

[Elliot Glaysher, 2014-09-04 17:10:56]

lgtm

[noms (inactive), 2014-09-04 17:12:07]

profiles lgtm

[binjin, 2014-09-04 17:35:30]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-04 17:50:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/binjin@chromium.org/500043003/200001

[commit-bot: I haz the power, 2014-09-04 20:03:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 20:04:01]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[binjin, 2014-09-04 22:50:17]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-04 22:53:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/binjin@chromium.org/500043003/200001

[commit-bot: I haz the power, 2014-09-04 23:35:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 23:35:46]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[binjin, 2014-09-05 02:26:37]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-05 02:28:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/binjin@chromium.org/500043003/200001

[commit-bot: I haz the power, 2014-09-05 02:56:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 02:56:47]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Joao da Silva, 2014-09-05 05:51:15]

admin_policy.h and .cc need to be removed from extensions/browser/BUILD.gn too

[binjin, 2014-09-05 12:03:12]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-05 12:05:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/binjin@chromium.org/500043003/220001

[commit-bot: I haz the power, 2014-09-05 12:16:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 12:16:05]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/52125)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[binjin, 2014-09-05 12:22:54]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-05 12:23:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/binjin@chromium.org/500043003/240001

[commit-bot: I haz the power, 2014-09-05 13:36:17]

Committed patchset #12 (id:240001) as b5da32eab6e40a28dc863ddc438128da126e1a09

[commit-bot: I haz the power, 2014-09-10 03:39:20]

Patchset 12 (id:??) landed as
https://crrev.com/1569c9bdcf86c361b0b79c697d0da76d03d70a96
Cr-Commit-Position: refs/heads/master@{#293514}