PluginList: Unit tests and bugfixes

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 81 matching lines @@
 
 PluginGroup::PluginGroup(const string16& group_name,
                          const string16& name_matcher,
                          const std::string& update_url,
                          const std::string& identifier)
     : identifier_(identifier),
       group_name_(group_name),
       name_matcher_(name_matcher),
       update_url_(update_url),
       enabled_(false),
-      version_(Version::GetVersionFromString("0")) {
+      version_(Version::GetVersionFromString("0")),
+      plugin_list_(NULL) {
 }
 
 void PluginGroup::InitFrom(const PluginGroup& other) {
+  enabled_ = false;

[danno, 2010/12/15 10:42:12]

Please comment why there enabled_ flag is set twice, or only set once.

[Jakob Kummerow, 2010/12/15 18:03:27]

Done. Cleaned up this whole method, it now performs only copying operations.

   identifier_ = other.identifier_;
   group_name_ = other.group_name_;
   name_matcher_ = other.name_matcher_;
-  description_ = other.description_;
   update_url_ = other.update_url_;
-  enabled_ = other.enabled_;
   for (size_t i = 0; i < other.version_ranges_.size(); ++i)
     version_ranges_.push_back(other.version_ranges_[i]);
   DCHECK_EQ(other.web_plugin_infos_.size(), other.web_plugin_positions_.size());
   std::list<WebPluginInfo>::const_iterator it = other.web_plugin_infos_.begin();
   std::vector<int>::const_iterator itprio = other.web_plugin_positions_.begin();
   for (; it != other.web_plugin_infos_.end(); ++it, ++itprio)
     AddPlugin(*it, *itprio);
   if (!version_.get())
     version_.reset(Version::GetVersionFromString("0"));
+  DCHECK_EQ(0, version_->CompareTo(*other.version_));
+  plugin_list_ = other.plugin_list_;
+  enabled_ = other.enabled_;
 }
 
 PluginGroup::PluginGroup(const PluginGroup& other) {
   InitFrom(other);
 }
 
 PluginGroup& PluginGroup::operator=(const PluginGroup& other) {

[danno, 2010/12/15 10:42:12]

use the copy/swap idiom?

[Jakob Kummerow, 2010/12/15 18:03:27]

As discussed offline: would be nice to have, but is not a priority right now.

   version_ranges_.clear();
   InitFrom(other);
   return *this;
 }
 
 /*static*/
 PluginGroup* PluginGroup::FromPluginGroupDefinition(
     const PluginGroupDefinition& definition) {
   PluginGroup* group = new PluginGroup(ASCIIToUTF16(definition.name),
                                        ASCIIToUTF16(definition.name_matcher),
@@ skipping 119 matching lines @@
 }
 
 std::list<WebPluginInfo>& PluginGroup::GetPlugins() {
   return web_plugin_infos_;
 }
 
 std::vector<int>& PluginGroup::GetPluginPositions() {
   return web_plugin_positions_;
 }
 
+bool PluginGroup::IsEmpty() {
+  return web_plugin_infos_.empty();
+}
+
 string16 PluginGroup::GetGroupName() const {
   if (!group_name_.empty())
     return group_name_;
   DCHECK_EQ(1u, web_plugin_infos_.size());
   FilePath::StringType path =
       web_plugin_infos_.front().path.BaseName().RemoveExtension().value();
 #if defined(OS_POSIX)
   return UTF8ToUTF16(path);
 #elif defined(OS_WIN)
   return WideToUTF16(path);
@@ skipping 109 matching lines @@
 void PluginGroup::DisableOutdatedPlugins() {
  bool first_enabled = true;
 
  for (std::list<WebPluginInfo>::iterator it = web_plugin_infos_.begin();
       it != web_plugin_infos_.end(); ++it) {
    scoped_ptr<Version> version(CreateVersionFromString(it->version));
    if (version.get()) {
      bool plugin_is_outdated = false;
      for (size_t i = 0; i < version_ranges_.size(); ++i) {
        if (IsPluginOutdated(*version, version_ranges_[i])) {
-         NPAPI::PluginList::Singleton()->DisablePlugin(it->path, false);
+         GetPluginList()->DisablePlugin(it->path, false);
          plugin_is_outdated = true;
          break;
        }
      }
      if (!plugin_is_outdated && first_enabled) {
        first_enabled = false;
        UpdateDescriptionAndVersion(*it);
      }
    }
  }
 }
 
 void PluginGroup::Enable(bool enable) {
   bool enabled_plugin_exists = false;
   for (std::list<WebPluginInfo>::iterator it = web_plugin_infos_.begin();
        it != web_plugin_infos_.end(); ++it) {
     bool policy_disabled = IsPluginNameDisabledByPolicy(it->name);
     if (enable && !policy_disabled) {
-      NPAPI::PluginList::Singleton()->EnablePlugin(it->path);
+      GetPluginList()->EnablePlugin(it->path);
       enabled_plugin_exists = true;
     } else {
-      NPAPI::PluginList::Singleton()->DisablePlugin(it->path, policy_disabled);
+      GetPluginList()->DisablePlugin(it->path, policy_disabled);
     }
   }
   enabled_ = enabled_plugin_exists;
 }
 
 void PluginGroup::RefreshEnabledState() {
   bool enabled_plugin_exists = false;
   for (std::list<WebPluginInfo>::iterator it = web_plugin_infos_.begin();
        it != web_plugin_infos_.end(); ++it) {
     if (it->enabled) {
       enabled_plugin_exists = true;
       break;
     }
   }
   enabled_ = enabled_plugin_exists;
 }
+
+NPAPI::PluginList* PluginGroup::GetPluginList() {

[danno, 2010/12/15 10:42:12]

I think you might be better served by making this a protected method that
returns the Singleton and created a subclass that mocks an override for the
test.

[Jakob Kummerow, 2010/12/15 18:03:27]

As discussed offline: Good plan, but would require major restructuring of
PluginGroup/PluginList, and will therefore have to wait.

+  if (plugin_list_)
+    return plugin_list_;
+  return NPAPI::PluginList::Singleton();
+}