Add ExtensionManagement based ExternalLoader

chrome/browser/extensions/external_policy_loader.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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
 void ExternalPolicyLoader::AddExtension(base::DictionaryValue* dict,
                                         const std::string& extension_id,
                                         const std::string& update_url) {
   dict->SetString(base::StringPrintf("%s.%s", extension_id.c_str(),
                                      ExternalProviderImpl::kExternalUpdateUrl),
                   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();
 }
 
 }  // namespace extensions