Refactor ExtensionManagement

chrome/browser/extensions/extension_management.h

 // Copyright 2014 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_MANAGEMENT_H_
 #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.
 class ExtensionManagement : public KeyedService {
  public:
   // Observer class for extension management settings changes.
   class Observer {
    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;
   };
 
   // Installation mode for extensions, default is INSTALLATION_ALLOWED.
   // * INSTALLATION_ALLOWED: Extension can be installed.
   // * INSTALLATION_BLOCKED: Extension cannot be installed.
   // * INSTALLATION_FORCED: Extension will be installed automatically
   //                        and cannot be disabled.
   // * INSTALLATION_RECOMMENDED: Extension will be installed automatically but
   //                             can be disabled.
   enum InstallationMode {
     INSTALLATION_ALLOWED = 0,
     INSTALLATION_BLOCKED,
     INSTALLATION_FORCED,
     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();
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
   // 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().
   scoped_ptr<base::DictionaryValue> GetForceInstallList() const;
 
   // Returns if an extension with id |id| is allowed to install or not.
   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();
 
   // Load preference with name |pref_name| and expected type |expected_type|.
   // If |force_managed| is true, only loading from the managed preference store
   // is allowed. Returns NULL if the preference is not present, not allowed to
   // be loaded from or has the wrong type.
   const base::Value* LoadPreference(const char* pref_name,
                                     bool force_managed,
                                     base::Value::Type expected_type);

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

#include "base/values.h"

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

Done.

 
   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
   // map.
   // TODO(binjin): Add |settings_by_update_url_|, and implement mechanism for
   // it.
   SettingsIdMap settings_by_id_;
 
   // The default IndividualSettings.
   // For extension settings applied to an individual extension (identified by
   // extension ID) or a group of extension (with specified extension update
   // 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_;
 
   ObserverList<Observer, true> observer_list_;
   PrefChangeRegistrar pref_change_registrar_;
   scoped_ptr<ManagementPolicy::Provider> provider_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionManagement);
 };
 
@@ skipping 16 matching lines @@
       content::BrowserContext* context) const OVERRIDE;
   virtual void RegisterProfilePrefs(
       user_prefs::PrefRegistrySyncable* registry) OVERRIDE;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionManagementFactory);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_MANAGEMENT_H_