Add ExtensionManagement class

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
new file mode 100644
index 0000000000000000000000000000000000000000..8fba58ef5623d1be7550b2d3b31a9f9fff8606fe
--- /dev/null
+++ b/chrome/browser/extensions/extension_management.h
@@ -0,0 +1,106 @@
+// 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/values.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/url_pattern_set.h"
+
+class PrefService;
+
+namespace extensions {
+
+// Service class to listen to prefs changes, hold preference settings, and
+// provide interfaces for monitoring and obtaining these settings.

[Joao da Silva, 2014/09/02 19:34:02]

This comment is very generic and a bit misleading. Here's a suggestion:

"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."

[binjin, 2014/09/02 20:02:56]

Done.

+class ExtensionManagement {
+ public:
+  // 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 {
+    void Reset();
+
+    InstallationMode installation_mode;
+    std::string update_url;
+  };
+
+  // Global extension management settings, applicable to all extensions.
+  struct GlobalSettings {
+    GlobalSettings();
+    ~GlobalSettings();
+
+    void Reset();
+
+    scoped_ptr<URLPatternSet> install_sources;

[Joao da Silva, 2014/09/02 19:34:02]

Why is this in a scoped_ptr and not just a URLPatternSet?

If it's meant to indicate that there is a restricted set of install sources then
it's clearer to just add a bool for that (something like
has_restricted_install_sources)

[binjin, 2014/09/02 20:02:56]

Done.

+    scoped_ptr<std::vector<Manifest::Type> > allowed_types;

[Joao da Silva, 2014/09/02 19:34:02]

Same here: why isn't this just a vector?

If it's meant to distinguish from "empty vector because no type is allowed" vs
"empty vector because policy not set and all types are allowed" then I think a
boolean is clearer again. Something like has_restricted_allowed_types.

[binjin, 2014/09/02 20:02:56]

Done.

+  };
+
+  typedef std::map<ExtensionId, IndividualSettings> SettingsIdMap;
+
+  explicit ExtensionManagement(PrefService* pref_service);
+  virtual ~ExtensionManagement();
+
+  // Load all extension management preferences from |pref_service|, and
+  // refresh the settings.
+  void Refresh();

[Joao da Silva, 2014/09/02 19:34:02]

I don't think this should be public. The ExtensionManagement class should
observe the pref_service_ and update automatically. Or is that part of another
CL?

[binjin, 2014/09/02 20:02:56]

Yes, it's part of the following CL, I will remove this to private part then.

+
+  // 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;

[Joao da Silva, 2014/09/02 19:34:02]

I think it would be simpler to have accessors for |install_sources| and
|allowed_types| directly here, and have those fields as members of the
ExtensionManagement class directly. The GlobalSettings structure won't be needed
after that.

But I haven't seen other CLs after this one, so maybe there's a reason for this.
WDYT?

[binjin, 2014/09/02 20:02:56]

I prefer to keep this structure, there are plenty many other global settings
from design doc.

[Joao da Silva, 2014/09/03 09:20:40]

Right, and they are all managed by this class. So why don't have just have
direct acessors in ExtensionManagement? It seems like the GlobalSettings struct
is adding one more indirection that is not necessary. Let's discuss this
offline.

+
+ private:
+  // 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,

[Joao da Silva, 2014/09/02 19:34:02]

Why do we force managed for some prefs only?

[binjin, 2014/09/02 20:02:56]

I think it might be useful for user (not admin) to manually set blocked
extensions.

+                                    base::Value::Type expected_type);
+
+  // 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);
+
+  // TODO(binjin): Add |settings_by_update_url_|, and implement mechanism for
+  // it.
+  SettingsIdMap settings_by_id_;
+  IndividualSettings default_settings_;
+  GlobalSettings global_settings_;
+
+  PrefService* pref_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionManagement);
+};
+
+}  // namespace extensions
+
+

[Joao da Silva, 2014/09/02 19:34:02]

nit: single newline here

[binjin, 2014/09/02 20:02:56]

Done.

+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_MANAGEMENT_H_