Clean up PluginGroup and related code.

webkit/glue/plugins/plugin_group.cc

 // Copyright (c) 2010 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.
 
 #include "webkit/glue/plugins/plugin_group.h"
 
 #include "base/linked_ptr.h"
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
@@ skipping 139 matching lines @@
   if (!version_range_low.empty())
     version_range_low_.reset(Version::GetVersionFromString(version_range_low));
   if (!version_range_high.empty()) {
     version_range_high_.reset(
         Version::GetVersionFromString(version_range_high));
   }
   if (!min_version.empty())
     min_version_.reset(Version::GetVersionFromString(min_version));
 }
 
+PluginGroup::PluginGroup(const PluginGroup& other)
+    : identifier_(other.identifier_),
+      group_name_(other.group_name_),
+      name_matcher_(other.name_matcher_),
+      version_range_low_str_(other.version_range_low_str_),
+      version_range_high_str_(other.version_range_high_str_),
+      version_range_low_(Version::GetVersionFromString(version_range_low_str_)),
+      version_range_high_(
+          Version::GetVersionFromString(version_range_high_str_)),
+      description_(other.description_),
+      update_url_(other.update_url_),
+      enabled_(other.enabled_),
+      min_version_str_(other.min_version_str_),
+      min_version_(Version::GetVersionFromString(min_version_str_)) {
+  DCHECK(other.web_plugin_infos_.size() == other.web_plugin_positions_.size());

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

You could pull this and the version_ constructor calls above out into an Init
method.

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

Done. Good idea.

+  for (size_t i = 0; i < other.web_plugin_infos_.size(); ++i)
+    AddPlugin(other.web_plugin_infos_[i], other.web_plugin_positions_[i]);
+  if (!version_.get())
+    version_.reset(Version::GetVersionFromString("0"));
+  DCHECK(enabled_ == other.enabled_);
+}
+
+PluginGroup& PluginGroup::operator=(const PluginGroup& other) {
+  identifier_ = other.identifier_;
+  group_name_ = other.group_name_;
+  name_matcher_ = other.name_matcher_;
+  version_range_low_str_ = other.version_range_low_str_;
+  version_range_high_str_ = other.version_range_high_str_;
+  version_range_low_.reset(
+      Version::GetVersionFromString(version_range_low_str_));
+  version_range_high_.reset(
+      Version::GetVersionFromString(version_range_high_str_));
+  description_ = other.description_;
+  update_url_ = other.update_url_;
+  enabled_ = other.enabled_;
+  min_version_str_ = other.min_version_str_;
+  min_version_.reset(Version::GetVersionFromString(min_version_str_));
+  DCHECK(other.web_plugin_infos_.size() == other.web_plugin_positions_.size());
+  for (size_t i = 0; i < other.web_plugin_infos_.size(); ++i)
+    AddPlugin(other.web_plugin_infos_[i], other.web_plugin_positions_[i]);
+  DCHECK(web_plugin_infos_.size() == other.web_plugin_infos_.size());
+  if (!version_.get())
+    version_.reset(Version::GetVersionFromString("0"));
+  return *this;
+}
+
+/*static*/
 PluginGroup* PluginGroup::FromPluginGroupDefinition(
     const PluginGroupDefinition& definition) {
   return new PluginGroup(ASCIIToUTF16(definition.name),
                          ASCIIToUTF16(definition.name_matcher),
                          definition.version_matcher_low,
                          definition.version_matcher_high,
                          definition.min_version,
                          definition.update_url,
                          definition.identifier);
 }
 
 PluginGroup::~PluginGroup() { }
 
+/*static*/
+std::string PluginGroup::GetIdentifier(const WebPluginInfo& wpi) {
+#if defined(OS_POSIX)
+  return wpi.path.BaseName().value();
+#elif defined(OS_WIN)
+  return base::SysWideToUTF8(wpi.path.BaseName().value());
+#endif
+}
+
+/*static*/
+std::string PluginGroup::GetLongIdentifier(const WebPluginInfo& wpi) {
+#if defined(OS_POSIX)
+  return wpi.path.value();
+#elif defined(OS_WIN)
+  return base::SysWideToUTF8(wpi.path.value());
+#endif
+}
+
+/*static*/
 PluginGroup* PluginGroup::FromWebPluginInfo(const WebPluginInfo& wpi) {
   // Create a matcher from the name of this plugin.
-#if defined(OS_POSIX)
-  std::string identifier = wpi.path.BaseName().value();
-#elif defined(OS_WIN)
-  std::string identifier = base::SysWideToUTF8(wpi.path.BaseName().value());
-#endif
   return new PluginGroup(wpi.name, wpi.name, std::string(), std::string(),
-                         std::string(), std::string(), identifier);
+                         std::string(), std::string(),
+                         GetIdentifier(wpi));
 }
 
-PluginGroup* PluginGroup::CopyOrCreatePluginGroup(
-    const WebPluginInfo& info) {
-  static PluginMap* hardcoded_plugin_groups = NULL;
-  if (!hardcoded_plugin_groups) {
-    PluginMap* groups = new PluginMap();
-    const PluginGroupDefinition* definitions = GetPluginGroupDefinitions();
-    for (size_t i = 0; i < GetPluginGroupDefinitionsSize(); ++i) {
-      PluginGroup* definition_group = PluginGroup::FromPluginGroupDefinition(
-          definitions[i]);
-      std::string identifier = definition_group->identifier();
-      DCHECK(groups->find(identifier) == groups->end());
-      (*groups)[identifier] = linked_ptr<PluginGroup>(definition_group);
-    }
-    hardcoded_plugin_groups = groups;
-  }
-
-  // See if this plugin matches any of the hardcoded groups.
-  PluginGroup* hardcoded_group = FindGroupMatchingPlugin(
-      *hardcoded_plugin_groups, info);
-  if (hardcoded_group) {
-    // Make a copy.
-    return hardcoded_group->Copy();
-  } else {
-    // Not found in our hardcoded list, create a new one.
-    return PluginGroup::FromWebPluginInfo(info);
-  }
-}
-
+/*static*/
 PluginGroup* PluginGroup::FindGroupMatchingPlugin(
-    const PluginMap& plugin_groups,
+    const PluginMap* plugin_groups,

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

If you remove the const here, you can get rid of the const_cast below.

Is this method used anywhere else beside PluginList? If not, you could inline it
there, which could maybe simplify the code.

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

Done.

     const WebPluginInfo& plugin) {
-  for (std::map<std::string, linked_ptr<PluginGroup> >::const_iterator it =
-       plugin_groups.begin();
-       it != plugin_groups.end();
+  for (PluginMap::const_iterator it = plugin_groups->begin();
+       it != plugin_groups->end();
        ++it) {
     if (it->second->Match(plugin))
-      return it->second.get();
+      return const_cast<PluginGroup*>(it->second);
   }
   return NULL;
 }
 
 bool PluginGroup::Match(const WebPluginInfo& plugin) const {
   if (name_matcher_.empty()) {
     return false;
   }
 
   // Look for the name matcher anywhere in the plugin name.
@@ skipping 49 matching lines @@
 
 void PluginGroup::UpdateDescriptionAndVersion(const WebPluginInfo& plugin) {
   description_ = plugin.desc;
   if (Version* new_version = CreateVersionFromString(plugin.version))
     version_.reset(new_version);
   else
     version_.reset(Version::GetVersionFromString("0"));
 }
 
 void PluginGroup::AddPlugin(const WebPluginInfo& plugin, int position) {
+  // Check if this group already contains this plugin.
+  for (size_t i = 0; i < web_plugin_infos_.size(); ++i) {
+    if (FilePath::CompareEqualIgnoreCase(web_plugin_infos_[i].path.value(),
+                                         plugin.path.value())) {
+      return;
+    }
+  }
   web_plugin_infos_.push_back(plugin);
   // The position of this plugin relative to the global list of plugins.
   web_plugin_positions_.push_back(position);
   UpdateActivePlugin(plugin);
 }
 
 string16 PluginGroup::GetGroupName() const {
   if (!group_name_.empty())
     return group_name_;
   DCHECK_EQ(1u, web_plugin_infos_.size());
@@ skipping 100 matching lines @@
     scoped_ptr<Version> version(CreateVersionFromString(it->version));
     if (version.get() && version->CompareTo(*min_version_) < 0) {
       it->enabled = false;
       NPAPI::PluginList::Singleton()->DisablePlugin(it->path);
     }
     UpdateActivePlugin(*it);
   }
 }
 
 void PluginGroup::Enable(bool enable) {
-  for (std::vector<WebPluginInfo>::const_iterator it =
+  bool enabled_plugin_exists = false;
+  for (std::vector<WebPluginInfo>::iterator it =
        web_plugin_infos_.begin();
        it != web_plugin_infos_.end(); ++it) {
     if (enable && !IsPluginNameDisabledByPolicy(it->name)) {
       NPAPI::PluginList::Singleton()->EnablePlugin(it->path);
+      it->enabled = true;
+      enabled_plugin_exists = true;
     } else {
+      it->enabled = false;
       NPAPI::PluginList::Singleton()->DisablePlugin(it->path);
     }
   }
+  enabled_ = enable && enabled_plugin_exists;

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

Nit: I think you can leave out the |enable &&| here.

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

Done.
I agree that |enable &&| is superfluous; I left it there because I thought it
enhanced readability (in the sense of easily understanding what's being done
here).

+  LOG(WARNING) << "Group " << GetGroupName() << " is now enabled: " << enabled_;

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

Nit: Remove logging plz :-)

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

Done.
(Of course -- that was just a leftover I forgot to remove.)

 }