Policy: Re-enabled plugin still disabled

webkit/glue/plugins/plugin_group.h

Index: webkit/glue/plugins/plugin_group.h
diff --git a/webkit/glue/plugins/plugin_group.h b/webkit/glue/plugins/plugin_group.h
index 5098787635b1f7d6d26b5f06b146964b0707995c..b8e49273a1e1df53488c8f5038b2db4ad2b3848b 100644
--- a/webkit/glue/plugins/plugin_group.h
+++ b/webkit/glue/plugins/plugin_group.h
@@ -23,7 +23,9 @@ struct WebPluginInfo;
 namespace NPAPI {
 class PluginList;
 };
-
+namespace plugin_test_internal {
+class PluginListWithoutFileIO;
+}
 // Hard-coded version ranges for plugin groups.
 struct VersionRangeDefinition {
   // Matcher for lowest version matched by this range (inclusive). May be empty
@@ -90,20 +92,27 @@ class PluginGroup {
   // the lookup key.
   static bool IsPluginNameDisabledByPolicy(const string16& plugin_name);
 
-  // Tests to see if a plugin is on the blacklist using its path as
-  // the lookup key.
-  static bool IsPluginPathDisabledByPolicy(const FilePath& plugin_path);
-
   // Returns true if the given plugin matches this group.
   bool Match(const WebPluginInfo& plugin) const;
 
-  // Adds the given plugin to this group. Provide the position of the
-  // plugin as given by PluginList so we can display its priority.
-  void AddPlugin(const WebPluginInfo& plugin, int position);
+  // Adds the given plugin to this group.
+  // Retuns true if plugin has been added and false if it was already contained
+  // inn the group before.

[Bernhard Bauer, 2010/12/20 22:30:28]

Nit: "in".

[pastarmovj, 2010/12/21 16:53:02]

Done.

+  bool AddPlugin(const WebPluginInfo& plugin, int priority);
+
+  // Enables a plugin given its filename. The function returns true if the
+  // plugin could be enabled. Plugins might not get enabled if they are disabled
+  // by policy or are already in the disabled state.
+  bool EnablePlugin(const FilePath& filename);
+
+  // Disables a plugin given its filename. The function returns true if the
+  // plugin could be disabled. Plugins might not get enabled if they are enabled
+  // by policy or are already in the enabled state.

[Bernhard Bauer, 2010/12/20 22:30:28]

You could group these comments ("the two following functions enable or disable
...").

[pastarmovj, 2010/12/21 16:53:02]

Done.

+  bool DisablePlugin(const FilePath& filename);
 
   // Enables/disables this group. This enables/disables all plugins in the
   // group.
-  void Enable(bool enable);
+  bool EnableGroup(bool enable);
 
   // Returns whether the plugin group is enabled or not.
   bool Enabled() const { return enabled_; }
@@ -116,6 +125,12 @@ class PluginGroup {
   // is empty.
   string16 GetGroupName() const;
 
+  const std::vector<WebPluginInfo>& GetPlugins() const;
+
+  std::vector<WebPluginInfo>& GetPlugins() { return web_plugin_infos_; }
+
+  bool ContainsPlugin(const FilePath& path) const;
+
   // Returns the description of the highest-priority plug-in in the group.
   const string16& description() const { return description_; }
 
@@ -132,6 +147,10 @@ class PluginGroup {
   // security problems.
   bool IsVulnerable() const;
 
+  // Check if the group has no plugins or only non-existing pluigns

[Bernhard Bauer, 2010/12/20 22:30:28]

Nit: plugins

[pastarmovj, 2010/12/21 16:53:02]

Done.

+  // (with priority 0).

[Bernhard Bauer, 2010/12/20 22:30:28]

This priority 0 thing is definitely a step in the wrong direction. Like John
said, if we want to have placeholder groups, we can store that state in the
group itself.

[pastarmovj, 2010/12/21 16:53:02]

I moved the priority back into the group class and introduced a placeholder flag
separately from it so that we don't mix two meanings in the same variable.

[jam, 2010/12/21 19:57:42]

I still don't think we need a priority at all.  Can you please explain why it's
needed?  The only example I've heard so far is because both internal and
external flash are in the same group.  There are two solutions to that: either
put them in separate groups, and have the internal one first, or add the
internal one before the external one in a group.  that would reduce the need to
have priority, which seems redundant since the same data can (and used to be)
taken from the ordering of the plugin list.

+  bool IsEmpty() const;
+
   // Disables all plugins in this group that are older than the
   // minimum version.
   void DisableOutdatedPlugins();
@@ -144,6 +163,7 @@ class PluginGroup {
   typedef std::map<std::string, PluginGroup*> PluginMap;
 
   friend class NPAPI::PluginList;
+  friend class plugin_test_internal::PluginListWithoutFileIO;
   friend class PluginGroupTest;
   friend class TableModelArrayControllerTest;
   friend class PluginExceptionsTableModelTest;
@@ -164,6 +184,10 @@ class PluginGroup {
   // the created PluginGroup.
   static PluginGroup* FromWebPluginInfo(const WebPluginInfo& wpi);
 
+  // Creates an empty PluginGroup from a given name. The caller takes
+  // ownership of the created PluginGroup.
+  static PluginGroup* CreateEmptyGroup(const string16& name);
+
   // Returns |true| if |version| is contained in [low, high) of |range|.
   static bool IsVersionInRange(const Version& version,
                                const VersionRange& range);
@@ -188,6 +212,9 @@ class PluginGroup {
   // enabled one, or if all plugins are disabled, simply the first one.
   void UpdateActivePlugin(const WebPluginInfo& plugin);
 
+  // Refreshes the enabled flag based on the state of its plugins.
+  void RefreshEnabledState();
+
   static std::set<string16>* policy_disabled_plugin_patterns_;
 
   std::string identifier_;
@@ -199,7 +226,6 @@ class PluginGroup {
   std::vector<VersionRange> version_ranges_;
   scoped_ptr<Version> version_;
   std::vector<WebPluginInfo> web_plugin_infos_;
-  std::vector<int> web_plugin_positions_;
 };
 
 #endif  // WEBKIT_GLUE_PLUGINS_PLUGIN_GROUP_H_