Policy: Re-enabled plugin still disabled

webkit/glue/plugins/plugin_list.h

Index: webkit/glue/plugins/plugin_list.h
diff --git a/webkit/glue/plugins/plugin_list.h b/webkit/glue/plugins/plugin_list.h
index 734cc6d7286693456b0704c2b5711190cba81e1c..07cadb081e36d194507b61164831a0f2169630c2 100644
--- a/webkit/glue/plugins/plugin_list.h
+++ b/webkit/glue/plugins/plugin_list.h
@@ -5,8 +5,10 @@
 #ifndef WEBKIT_GLUE_PLUGINS_PLUGIN_LIST_H_
 #define WEBKIT_GLUE_PLUGINS_PLUGIN_LIST_H_
 
+#include <map>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
@@ -190,13 +192,14 @@ class PluginList {
   // of return value, if a plugin is found in the future with the given name, it
   // will be enabled. Note that plugins are enabled by default as far as
   // |PluginList| is concerned.
-  bool EnablePlugin(const FilePath& filename);
+  bool EnablePlugin(const FilePath& filename, const string16& name);

[Bernhard Bauer, 2010/12/17 18:50:59]

Can you explain what |name| is for? Are both required? What if they don't match?

[pastarmovj, 2010/12/20 19:57:37]

Done.

 
   // Disable a specific plugin, specified by path. Returns |true| iff a plugin
   // currently in the plugin list was actually disabled as a result; regardless
   // of return value, if a plugin is found in the future with the given name, it
-  // will be disabled.
-  bool DisablePlugin(const FilePath& filename);
+  // will be disabled. The boolean flag should be set to true if the plugin is
+  // disabled by a policy and not a user.

[Bernhard Bauer, 2010/12/17 18:50:59]

That comment doesn't seem right.

[jam, 2010/12/17 19:14:45]

nit: using boolean to specify whether it's user/policy makes it hard to read at
the caller's site, and also limiting in the future in case we want to disable
through other means (say because the plugin is out-of-date).  please use an enum
instead

[pastarmovj, 2010/12/20 19:57:37]

I fixed the comment to make clear what the return flag means. It doesn't mean
weather the plugin is policy enabled or disabled but only whether the change
requested could be performed.

[jam, 2010/12/20 20:56:59]

oops, i meant to add this comment on line 209.  so is this function only used to
disable plugins based on user action, and not on group policy?

[pastarmovj, 2010/12/20 21:47:22]

In any case these functions doesn't differentiate between policy or user call
externally. If there is a policy set it has precedence (the groups store
information about what policies exist) over the user action. Normally the UI
should prevent the user from changing the status of a plugin or group if it is
policy controlled. But in case the user tries to manipulate that it will still
be overridden inside these functions. 

A policy change can trigger calling any of these function or a user interaction
can do as well. 

+  bool DisablePlugin(const FilePath& filename, const string16& name);
 
   // Enable/disable a plugin group, specified by group_name.  Returns |true| iff
   // a plugin currently in the plugin list was actually enabled/disabled as a
@@ -211,7 +214,7 @@ class PluginList {
   // version.
   void DisableOutdatedPluginGroups();
 
-  ~PluginList();
+  virtual ~PluginList();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(PluginGroupTest, PluginGroupDefinition);
@@ -228,6 +231,12 @@ class PluginList {
   // Callers need to protect calls to this method by a lock themselves.
   PluginGroup* AddToPluginGroups(const WebPluginInfo& web_plugin_info);
 
+  // Adds the Plugin given by its path and name to its corresponding group,
+  // creating it if necessary, and returns the group.
+  // Callers need to protect calls to this method by a lock themselves.
+  PluginGroup* AddToPluginGroups(const FilePath& filename,
+                                 const string16& name);
+
   // Load all plugins from the default plugins directory
   void LoadPlugins(bool refresh);
 
@@ -251,21 +260,6 @@ class PluginList {
   // list of disabled groups as well.
   bool ShouldDisableGroup(const string16& group_name);
 
-  // Returns true if the given WebPluginInfo supports "mime-type".
-  // mime_type should be all lower case.
-  static bool SupportsType(const WebPluginInfo& info,
-                           const std::string &mime_type,
-                           bool allow_wildcard);
-
-  // Returns true if the given WebPluginInfo supports a given file extension.
-  // extension should be all lower case.
-  // If mime_type is not NULL, it will be set to the mime type if found.
-  // The mime type which corresponds to the extension is optionally returned
-  // back.
-  static bool SupportsExtension(const WebPluginInfo& info,
-                                const std::string &extension,
-                                std::string* actual_mime_type);
-
   //
   // Platform functions
   //
@@ -286,8 +280,8 @@ class PluginList {
 
   // Loads plugins registered under HKCU\Software\MozillaPlugins and
   // HKLM\Software\MozillaPlugins.
-  void LoadPluginsFromRegistry(std::vector<WebPluginInfo>* plugins,
-                               std::set<FilePath>* visited_plugins);
+  void LoadPluginsFromRegistry(std::set<FilePath>* visited_plugins,

[jam, 2010/12/17 19:14:45]

why switch these parameters' order?

[pastarmovj, 2010/12/20 19:57:37]

Not by intention. Fixed.

+                               std::vector<WebPluginInfo>* plugins);
 #endif
 
   //
@@ -299,9 +293,6 @@ class PluginList {
   // If true, we reload plugins even if they've been loaded already.
   bool plugins_need_refresh_;
 
-  // Contains information about the available plugins.
-  std::vector<WebPluginInfo> plugins_;
-
   // Extra plugin paths that we want to search when loading.
   std::vector<FilePath> extra_plugin_paths_;
 
@@ -311,12 +302,6 @@ class PluginList {
   // Holds information about internal plugins.
   std::vector<PluginVersionInfo> internal_plugins_;
 
-  // Path names of plugins to disable (the default is to enable them all).
-  std::set<FilePath> disabled_plugins_;
-
-  // Group names to disable (the default is to enable them all).
-  std::set<string16> disabled_groups_;
-
   bool disable_outdated_plugins_;
 
   // Holds the currently available plugin groups.
@@ -326,7 +311,7 @@ class PluginList {
 
   // Need synchronization for the above members since this object can be
   // accessed on multiple threads.
-  Lock lock_;
+  mutable Lock lock_;

[jam, 2010/12/17 19:14:45]

why make this mutable?  if locking is required, seems the function don't need to
be const?  that would be a hint to the caller that some work can be required..

[pastarmovj, 2010/12/20 19:57:37]

I lock some getters to prevent writing happening while copying the output. The
getters don't change anything and could well function in a const situation. In
general I try to be as const friendly as possible but if you thing it's better
to do it otherwise I will change it.

 
   friend struct base::DefaultLazyInstanceTraits<PluginList>;