Refactor ExtensionManagement

chrome/browser/extensions/extension_management.h

Index: chrome/browser/extensions/extension_management.h
diff --git a/chrome/browser/extensions/extension_management.h b/chrome/browser/extensions/extension_management.h
index d7d1466409cb672c0b5f1f44f1f22a0e3853651e..33d60c67eba5c3af93a71020f4d9d2bda2641170 100644
--- a/chrome/browser/extensions/extension_management.h
+++ b/chrome/browser/extensions/extension_management.h
@@ -6,31 +6,37 @@
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_MANAGEMENT_H_
 
 #include <map>
-#include <string>
-#include <vector>
 
 #include "base/macros.h"
+#include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
 #include "base/observer_list.h"
 #include "base/prefs/pref_change_registrar.h"
-#include "base/values.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "extensions/browser/management_policy.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
-#include "extensions/common/url_pattern_set.h"
 
 class GURL;
 class PrefService;
 
+namespace base {
+class DictionaryValue;
+}  // namespace base
+
 namespace content {
 class BrowserContext;
 }  // namespace content
 
 namespace extensions {
 
+namespace internal {
+struct IndividualSettings;
+struct GlobalSettings;
+}  // namespace internal
+
 // Tracks the management policies that affect extensions and provides interfaces
 // for observing and obtaining the global settings for all extensions, as well
 // as per-extension settings.
@@ -41,7 +47,7 @@ class ExtensionManagement : public KeyedService {
    public:
     virtual ~Observer() {}
 
-    // Will be called when an extension management preference changes.
+    // Will be called when then extension management setting changed.

[Joao da Silva, 2014/09/25 08:53:51]

Called when the extension management settings change.

[binjin, 2014/09/25 13:19:19]

Done.

     virtual void OnExtensionManagementSettingsChanged() = 0;
   };
 
@@ -59,48 +65,6 @@ class ExtensionManagement : public KeyedService {
     INSTALLATION_RECOMMENDED,
   };
 
-  // Class to hold extension management settings for one or a group of
-  // extensions. Settings can be applied to an individual extension identified
-  // by an ID, a group of extensions with specific |update_url| or all
-  // extensions at once.
-  struct IndividualSettings {
-    IndividualSettings();
-    ~IndividualSettings();
-
-    void Reset();
-
-    // Extension installation mode. Setting this to INSTALLATION_FORCED or
-    // INSTALLATION_RECOMMENDED will enable extension auto-loading (only
-    // applicable to single extension), and in this case the |update_url| must
-    // be specified, containing the update URL for this extension.
-    // Note that |update_url| will be ignored for INSTALLATION_ALLOWED and
-    // INSTALLATION_BLOCKED installation mode.
-    // These settings will override the default settings, and unspecified
-    // settings will take value from default settings.
-    InstallationMode installation_mode;
-    std::string update_url;
-  };
-
-  // Global extension management settings, applicable to all extensions.
-  struct GlobalSettings {
-    GlobalSettings();
-    ~GlobalSettings();
-
-    void Reset();
-
-    // Settings specifying which URLs are allowed to install extensions, will be
-    // enforced only if |has_restricted_install_sources| is set to true.
-    URLPatternSet install_sources;
-    bool has_restricted_install_sources;
-
-    // Settings specifying all allowed app/extension types, will be enforced
-    // only of |has_restricted_allowed_types| is set to true.
-    std::vector<Manifest::Type> allowed_types;
-    bool has_restricted_allowed_types;
-  };
-
-  typedef std::map<ExtensionId, IndividualSettings> SettingsIdMap;
-
   explicit ExtensionManagement(PrefService* pref_service);
   virtual ~ExtensionManagement();
 
@@ -109,12 +73,15 @@ class ExtensionManagement : public KeyedService {
 
   // Get the ManagementPolicy::Provider controlled by extension management
   // policy settings.
-  ManagementPolicy::Provider* GetProvider();
+  ManagementPolicy::Provider* GetProvider() const;
 
   // Checks if extensions are blacklisted by default, by policy. When true,
   // this means that even extensions without an ID should be blacklisted (e.g.
   // from the command line, or when loaded as an unpacked extension).
-  bool BlacklistedByDefault();
+  bool BlacklistedByDefault() const;
+
+  // Returns installation mode for an extension.
+  InstallationMode GetInstallationMode(const ExtensionId &id) const;
 
   // Returns the force install list, in format specified by
   // ExternalPolicyLoader::AddExtension().
@@ -124,16 +91,18 @@ class ExtensionManagement : public KeyedService {
   bool IsInstallationAllowed(const ExtensionId& id) const;
 
   // Returns true if an extension download should be allowed to proceed.
-  bool IsOffstoreInstallAllowed(const GURL& url, const GURL& referrer_url);
+  bool IsOffstoreInstallAllowed(const GURL& url,
+                                const GURL& referrer_url) const;
 
-  // Helper function to read |settings_by_id_| with |id| as key. Returns a
-  // constant reference to default settings if |id| does not exist.
-  const IndividualSettings& ReadById(const ExtensionId& id) const;
-
-  // Returns a constant reference to |global_settings_|.
-  const GlobalSettings& ReadGlobalSettings() const;
+  // Returns true if an extension with manifest type |manifest_type| is
+  // allowed to be installed.
+  bool IsAllowedManifestType(Manifest::Type manifest_type) const;
 
  private:
+  typedef std::map<ExtensionId, linked_ptr<internal::IndividualSettings> >
+      SettingsIdMap;

[Joao da Silva, 2014/09/25 08:53:51]

I'm guessing that the linked_ptrs are used so that you can put the ownership in
this map, and so that you can point to forward declared types.

We've tried to refrain from using linked_ptrs in the past, because they are just
ref counted ptrs and there are better ways to do the same thing.

For this map, I think you can use base/containers/scoped_ptr_hash_map.h. That is
just a hash_map and should work for ExtensionId (which is just a typedef for
std::string). It will automatically own the IndividualSettings, so no need for
linked_ptrs there.

You'll probably need to include extension_management_internal.h in this header
too, because the ScopedPtrHashMap needs to "know" how to delete
IndividualSettings structs.

[binjin, 2014/09/25 13:19:18]

Done. But without extension_management_internal.h included in header file. It
compiles fine now and including the internal header file actually part ways from
my original goal for splitting it.

+  friend class ExtensionManagementServiceTest;
+
   // Load all extension management preferences from |pref_service|, and
   // refresh the settings.
   void Refresh();
@@ -149,10 +118,18 @@ class ExtensionManagement : public KeyedService {
   void OnExtensionPrefChanged();
   void NotifyExtensionManagementPrefChanged();
 
+  // Helper function to read |settings_by_id_| with |id| as key. Returns a
+  // constant reference to default settings if |id| does not exist.
+  linked_ptr<const internal::IndividualSettings> ReadById(

[Joao da Silva, 2014/09/25 08:53:51]

Return an IndividualSettings* here

[binjin, 2014/09/25 13:19:18]

Done.

+      const ExtensionId& id) const;
+
+  // Returns a constant reference to |global_settings_|.
+  linked_ptr<const internal::GlobalSettings> ReadGlobalSettings() const;

[Joao da Silva, 2014/09/25 08:53:51]

Same here

[binjin, 2014/09/25 13:19:18]

Done.

+
   // Helper function to access |settings_by_id_| with |id| as key.
   // Adds a new IndividualSettings entry to |settings_by_id_| if none exists for
   // |id| yet.
-  IndividualSettings* AccessById(const ExtensionId& id);
+  linked_ptr<internal::IndividualSettings> AccessById(const ExtensionId& id);

[Joao da Silva, 2014/09/25 08:53:51]

And here

[binjin, 2014/09/25 13:19:18]

Done.

 
   // A map containing all IndividualSettings applied to an individual extension
   // identified by extension ID. The extension ID is used as index key of the
@@ -167,10 +144,10 @@ class ExtensionManagement : public KeyedService {
   // URL), all unspecified part will take value from |default_settings_|.
   // For all other extensions, all settings from |default_settings_| will be
   // enforced.
-  IndividualSettings default_settings_;
+  linked_ptr<internal::IndividualSettings> default_settings_;

[Joao da Silva, 2014/09/25 08:53:51]

scoped_ptr works here. Actually, there is no need for a smart ptr if you include
extension_management_internal.h, which is fine.

[binjin, 2014/09/25 13:19:18]

Done.

 
   // Extension settings applicable to all extensions.
-  GlobalSettings global_settings_;
+  linked_ptr<internal::GlobalSettings> global_settings_;

[Joao da Silva, 2014/09/25 08:53:51]

Same here.

[binjin, 2014/09/25 13:19:18]

Done.

 
   PrefService* pref_service_;