Policy: Re-enabled plugin still disabled

webkit/plugins/npapi/plugin_list.h

Index: webkit/plugins/npapi/plugin_list.h
diff --git a/webkit/plugins/npapi/plugin_list.h b/webkit/plugins/npapi/plugin_list.h
index 84026eb0125f2ff712b0adb33025725d5cefc26b..f79936f06cbe28cd515d6681da24555a578a98ec 100644
--- a/webkit/plugins/npapi/plugin_list.h
+++ b/webkit/plugins/npapi/plugin_list.h
@@ -5,8 +5,10 @@
 #ifndef WEBKIT_PLUGINS_NPAPI_PLUGIN_LIST_H_
 #define WEBKIT_PLUGINS_NPAPI_PLUGIN_LIST_H_
 
+#include <map>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
@@ -103,7 +105,6 @@ class PluginList {
   // Register an internal plugin with the specified plugin information and
   // function pointers.  An internal plugin must be registered before it can
   // be loaded using PluginList::LoadPlugin().
-  void RegisterInternalPlugin(const FilePath& path);

[jam, 2011/01/19 20:22:09]

why take this out?  it was convenient for internal plugins.

[pastarmovj, 2011/01/19 23:39:17]

As already mentioned in my comment to
http://codereview.chromium.org/5699005/diff/142001/chrome/browser/automation/...
this function was only used in one unit test. (And after Evan's CL even there
not anymore). It doesn't contribute to the interface it only can be source of
confusion. All plugins use the full fledged 5 param version (and have to because
they need to supply the entry points array).

   void RegisterInternalPlugin(const FilePath& filename,
                               const std::string& name,
                               const std::string& description,
@@ -201,8 +202,7 @@ class PluginList {
   // Enable a specific plugin, specified by path. Returns |true| iff a plugin
   // currently in the plugin list was actually enabled as a result; regardless
   // 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.
+  // will be enabled.
   bool EnablePlugin(const FilePath& filename);
 
   // Disable a specific plugin, specified by path. Returns |true| iff a plugin
@@ -214,8 +214,7 @@ class PluginList {
   // 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
   // result; regardless of return value, if a plugin is found in the future with
-  // the given name, it will be enabled/disabled. Note that plugins are enabled
-  // by default as far as |PluginList| is concerned.
+  // the given name, it will be enabled/disabled.
   bool EnableGroup(bool enable, const string16& name);
 
   // Disable all plugins groups that are known to be outdated, according to
@@ -224,25 +223,35 @@ class PluginList {
   // version.
   void DisableOutdatedPluginGroups();
 
-  ~PluginList();
-
- private:
-  FRIEND_TEST_ALL_PREFIXES(PluginGroupTest, PluginGroupDefinition);
+  virtual ~PluginList();
 
+ protected:
   // Constructors are private for singletons
   PluginList();
 
-  // Creates PluginGroups for the static group definitions, and adds them to
-  // the PluginGroup cache of this PluginList.
-  void AddHardcodedPluginGroups();
+  // Load all plugins from the default plugins directory
+  virtual void LoadPlugins(bool refresh);
+
+  // Allows tests to perform custom post-initialization tasks on newly created
+  // PluginGroups.
+  virtual void ProcessGroupAfterInitialize(PluginGroup* group) { }
 
   // Adds the given WebPluginInfo 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 WebPluginInfo& web_plugin_info);
 
-  // Load all plugins from the default plugins directory
-  void LoadPlugins(bool refresh);
+  bool plugins_loaded_;
+
+  // Holds the currently available plugin groups.
+  PluginGroup::PluginMap plugin_groups_;

[jam, 2011/01/19 20:22:09]

does this really need to be a map?  we used to use a vector before because then
ordering is a way of signifying who gets first dibs.  this way we can put the
plugin installer plugin (i.e. the one that catches all mime-types) at the end. 
we would also put internal plugins first, i.e. the pdf.

I've looked at the usage of this, and almost all places just iterate over it. 
for the other (2) places, they could iterate manually. so i think given that
performance is not an issue, we should use a vector as before.

[pastarmovj, 2011/01/19 23:39:17]

Copied from plugin_group.cc question about the typedef:
The problem is that we want to differentiate different groups based on their
identifiers (which mostly coincide with names but in cases of collisions don't
see. PluginGroup::AddToPluginGroups). Therefore we have this maps there.

[jam, 2011/01/20 00:24:25]

Sorry I don't follow here.  if AddToPluginGroups finds that it wants to insert a
group with an identifier that already exists (I'm actually not sure I understand
when this situation can happen?), why shouldn't we just set the new group's
identifier() to its GetLongIdentifier()?

+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(PluginGroupTest, PluginGroupDefinition);
+
+  // Creates PluginGroups for the static group definitions, and adds them to
+  // the PluginGroup cache of this PluginList.
+  void AddHardcodedPluginGroups();
 
   // Load all plugins from a specific directory.
   // |plugins| is updated with loaded plugin information.
@@ -264,21 +273,19 @@ 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);
-
+  // Returns true if the plugin supports |mime_type|. |mime_type| should be all
+  // lower case.
+  bool SupportsType(const WebPluginInfo& plugin,
+                    const std::string& mime_type,
+                    bool allow_wildcard);
+
+  // Returns true if the given plugin 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.
+  bool SupportsExtension(const WebPluginInfo& plugin,
+                         const std::string& extension,
+                         std::string* actual_mime_type);
   //
   // Platform functions
   //
@@ -304,14 +311,9 @@ class PluginList {
   // Internals
   //
 
-  bool plugins_loaded_;
-
   // 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_;
 
@@ -321,16 +323,10 @@ 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.
-  PluginGroup::PluginMap plugin_groups_;
+  std::vector<FilePath> prematurely_disabled_plugins_;

[jam, 2011/01/19 20:22:09]

comment please

[pastarmovj, 2011/01/19 23:39:17]

Done.

+  std::vector<string16> prematurely_disabled_groups_;
 
   int next_priority_;