Add ExtensionManagement based ExternalLoader

chrome/browser/extensions/external_policy_loader.cc

Index: chrome/browser/extensions/external_policy_loader.cc
diff --git a/chrome/browser/extensions/external_policy_loader.cc b/chrome/browser/extensions/external_policy_loader.cc
index a1e9e6edd82d3c1d1650c9c519209ffcb88879c0..ffd2f0d35c08e6fab16d344b46353b1b41e0aaca 100644
--- a/chrome/browser/extensions/external_policy_loader.cc
+++ b/chrome/browser/extensions/external_policy_loader.cc
@@ -4,33 +4,25 @@
 
 #include "chrome/browser/extensions/external_policy_loader.h"
 
-#include "base/bind.h"
-#include "base/logging.h"
-#include "base/prefs/pref_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/common/pref_names.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_source.h"
-#include "extensions/browser/pref_names.h"
 
 namespace extensions {
 
 ExternalPolicyLoader::ExternalPolicyLoader(Profile* profile)
-    : profile_(profile) {
-  pref_change_registrar_.Init(profile_->GetPrefs());
-  pref_change_registrar_.Add(pref_names::kInstallForceList,
-                             base::Bind(&ExternalPolicyLoader::StartLoading,
-                                        base::Unretained(this)));
-  pref_change_registrar_.Add(pref_names::kAllowedTypes,
-                             base::Bind(&ExternalPolicyLoader::StartLoading,
-                                        base::Unretained(this)));
-  notification_registrar_.Add(this,
-                              chrome::NOTIFICATION_PROFILE_DESTROYED,
-                              content::Source<Profile>(profile_));
+    : settings_(ExtensionManagementFactory::GetInstance()

[Joao da Silva, 2014/09/04 12:07:46]

GetInstance() is not needed here, just do

 ExtensionManagementFactory::GetForBrowserContext(profile)

I think this should also be moved to the owner of the object, instead of passing
the Profile* to this constructor.

[binjin, 2014/09/04 14:27:06]

Done.

+                    ->GetForBrowserContext(profile)) {
+  settings_->AddObserver(this);
+}
+
+ExternalPolicyLoader::~ExternalPolicyLoader() {
+  settings_->RemoveObserver(this);
+}
+
+void ExternalPolicyLoader::OnExtensionManagementSettingsChanged() {
+  StartLoading();
 }
 
 // static
@@ -42,24 +34,8 @@ void ExternalPolicyLoader::AddExtension(base::DictionaryValue* dict,
                   update_url);

[Joao da Silva, 2014/09/04 12:07:46]

Move this method to ExtensionManagement too.

[binjin, 2014/09/04 14:27:06]

I think it's better to move this to ExternalProviderImpl or ExternalProvider,
the format was generally used and actually defined in ExternalProviderImpl. If
not, leaving it here is still better than moving it to ExternsionManagement, the
latter one will eventually drop support of legacy forcelist policy.

 }
 
-void ExternalPolicyLoader::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  if (profile_ == NULL) return;
-  DCHECK(type == chrome::NOTIFICATION_PROFILE_DESTROYED) <<

[Joao da Silva, 2014/09/04 12:07:46]

I wonder why this was here before. Are we sure that these objects are destroyed
in the right order?

[binjin, 2014/09/04 14:27:06]

Not sure. Does the destroyed order of ExtensionManagement matters, like here?

[Joao da Silva, 2014/09/04 16:55:29]

I just worry that |settings_| may be a stray pointers at ~ExternalPolicyLoader.

I looked around in the code and it seems that won't happen, so let's keep this
change as is.

-      "Unexpected notification type.";
-  if (content::Source<Profile>(source).ptr() == profile_) {
-    notification_registrar_.RemoveAll();
-    pref_change_registrar_.RemoveAll();
-    profile_ = NULL;
-  }
-}
-
 void ExternalPolicyLoader::StartLoading() {
-  const base::DictionaryValue* forcelist =
-      profile_->GetPrefs()->GetDictionary(pref_names::kInstallForceList);
-  prefs_.reset(forcelist ? forcelist->DeepCopy() : NULL);
+  prefs_ = settings_->GetForceInstallList();
   LoadFinished();
 }