Clean up PluginGroup and related code.

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 228143757167ba035e05902fdda253e07dfe3b7e..ab3e191a8cb02aaf56e11c392d6a3638e292b9f3 100644
--- a/webkit/glue/plugins/plugin_group.h
+++ b/webkit/glue/plugins/plugin_group.h
@@ -8,6 +8,7 @@
 
 #include <map>
 #include <set>
+#include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
@@ -23,9 +24,6 @@ namespace NPAPI {
   class PluginList;
 };
 
-template <typename T>
-class linked_ptr;
-
 // Hard-coded definitions of plugin groups.
 struct PluginGroupDefinition {
   const char* identifier;  // Unique identifier for this group.
@@ -51,23 +49,21 @@ class PluginGroup {
   static const char* kAdobeReader8GroupName;
   static const char* kAdobeReader9GroupName;
 
-  typedef std::map<std::string, linked_ptr<PluginGroup> > PluginMap;
+  typedef std::map<std::string, PluginGroup*> PluginMap;

[Bernhard Bauer, 2010/12/03 16:13:28]

Do we need this typedef outside of PluginGroup or can we make it private?

[Jakob Kummerow, 2010/12/06 18:21:12]

Done. (Made private.)

 
   // Creates a PluginGroup from a PluginGroupDefinition.
   static PluginGroup* FromPluginGroupDefinition(
       const PluginGroupDefinition& definition);
 
+  PluginGroup(const PluginGroup& other);

[Bernhard Bauer, 2010/12/03 16:13:28]

Nit: make the single-argument constructor explicit.

[Jakob Kummerow, 2010/12/06 18:21:12]

Nope. Let me quote the style guide:
"We require all single argument constructors to be explicit. [...]
The exception is copy constructors, which, in the rare cases when we allow them,
should probably not be explicit."
In fact, it doesn't compile if this copy constructor is explicit.
The only alternative would be to never store plain PluginGroups (as opposed to
pointers) in std::vectors or similar containers, and get rid of this constructor
altogether, but then we'd still have the same threading/concurrency problems
this cleanup is intended to get rid of.

+
   ~PluginGroup();
 
-  // Creates a PluginGroup from a WebPluginInfo -- when no hard-coded
-  // definition is found.
-  static PluginGroup* FromWebPluginInfo(const WebPluginInfo& wpi);
+  PluginGroup& operator=(const PluginGroup& other);
 
-  // Find a plugin group matching |info| in the list of hardcoded plugins and
-  // returns a copy of it if found, or a new group matching exactly this plugin
-  // otherwise.
-  // The caller should take ownership of the return PluginGroup.
-  static PluginGroup* CopyOrCreatePluginGroup(const WebPluginInfo& info);
+  // Creates a PluginGroup from a WebPluginInfo. Should not be called externally
+  // except by PluginList and unit tests.

[Bernhard Bauer, 2010/12/03 16:13:28]

PluginList is already a friend, so just declare this method private. 

Also, please explain that the caller takes ownership of the returned object.

[Jakob Kummerow, 2010/12/06 18:21:12]

Done.
This required adding several more friend classes (which is why I didn't make the
method private before).
Done.

+  static PluginGroup* FromWebPluginInfo(const WebPluginInfo& wpi);
 
   // Configures the set of plugin name patterns for disabling plugins via
   // enterprise configuration management.
@@ -81,19 +77,6 @@ class PluginGroup {
   // the lookup key.
   static bool IsPluginPathDisabledByPolicy(const FilePath& plugin_path);
 
-  // Find the PluginGroup matching a Plugin in a list of plugin groups. Returns
-  // NULL if no matching PluginGroup is found.
-  static PluginGroup* FindGroupMatchingPlugin(
-      const std::map<std::string, linked_ptr<PluginGroup> >& plugin_groups,
-      const WebPluginInfo& plugin);
-
-  // Creates a copy of this plugin group.
-  PluginGroup* Copy() {
-    return new PluginGroup(group_name_, name_matcher_, version_range_low_str_,
-                           version_range_high_str_, min_version_str_,
-                           update_url_, identifier_);
-  }
-
   // Returns true if the given plugin matches this group.
   bool Match(const WebPluginInfo& plugin) const;
 
@@ -137,11 +120,25 @@ class PluginGroup {
   void DisableOutdatedPlugins();
 
  private:
+  friend class NPAPI::PluginList;
   FRIEND_TEST_ALL_PREFIXES(PluginGroupTest, PluginGroupDefinition);
 
   static const PluginGroupDefinition* GetPluginGroupDefinitions();
   static size_t GetPluginGroupDefinitionsSize();
 
+  // Generates the (short) identifier string for the given plugin.
+  static std::string GetIdentifier(const WebPluginInfo& wpi);
+
+  // Generates the long identifier (based on the full file path) for the given
+  // plugin, to be called when the short identifier is not unique.
+  static std::string GetLongIdentifier(const WebPluginInfo& wpi);
+
+  // Find the PluginGroup matching a Plugin in a list of plugin groups. Returns
+  // NULL if no matching PluginGroup is found.
+  static PluginGroup* FindGroupMatchingPlugin(
+      const PluginMap* plugin_groups,
+      const WebPluginInfo& plugin);
+
   PluginGroup(const string16& group_name,
               const string16& name_matcher,
               const std::string& version_range_low,
@@ -177,8 +174,6 @@ class PluginGroup {
   scoped_ptr<Version> version_;
   std::vector<WebPluginInfo> web_plugin_infos_;
   std::vector<int> web_plugin_positions_;
-
-  DISALLOW_COPY_AND_ASSIGN(PluginGroup);
 };
 
 #endif  // WEBKIT_GLUE_PLUGINS_PLUGIN_GROUP_H_