Policy: Re-enabled plugin still disabled

webkit/glue/plugins/plugin_group.cc

Index: webkit/glue/plugins/plugin_group.cc
diff --git a/webkit/glue/plugins/plugin_group.cc b/webkit/glue/plugins/plugin_group.cc
index 548e624a81fd239a9406a24dd134e5cf0503f374..3837f7d5909a07d4dc09b7997236d89250d3187f 100644
--- a/webkit/glue/plugins/plugin_group.cc
+++ b/webkit/glue/plugins/plugin_group.cc
@@ -43,21 +43,6 @@ bool PluginGroup::IsPluginNameDisabledByPolicy(const string16& plugin_name) {
   return false;
 }
 
-/*static*/
-bool PluginGroup::IsPluginPathDisabledByPolicy(const FilePath& plugin_path) {
-  std::vector<WebPluginInfo> plugins;
-  NPAPI::PluginList::Singleton()->GetPlugins(false, &plugins);
-  for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin();
-       it != plugins.end();
-       ++it) {
-    if (FilePath::CompareEqualIgnoreCase(it->path.value(),
-        plugin_path.value()) && IsPluginNameDisabledByPolicy(it->name)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 VersionRange::VersionRange(VersionRangeDefinition definition)
     : low_str(definition.version_matcher_low),
       high_str(definition.version_matcher_high),
@@ -103,19 +88,20 @@ PluginGroup::PluginGroup(const string16& group_name,
 }
 
 void PluginGroup::InitFrom(const PluginGroup& other) {
+  enabled_ = false;

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

curious why this doesn't get copied from other?

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

This whole function will be changed in CL 5783005 from Jakob anyhow.

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

It's fine that this function will change in the future, but our tree should
always be shippable and anyone can sync to a specific revision.  If I sync after
you commit, this code will be confusing to me that it only copies some values
but not others.

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

Actually it is better to say that this function will change in a commit that
will precede mine:

http://codereview.chromium.org/5918003/diff/1/webkit/glue/plugins/plugin_grou...

[Bernhard Bauer, 2010/12/20 22:30:28]

So... rebase it on top of the other CL? Asking someone to review a change which
is not going to land is really confusing.

You can ping me if you need help with the rebasing.

   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());
-  for (size_t i = 0; i < other.web_plugin_infos_.size(); ++i)
-    AddPlugin(other.web_plugin_infos_[i], other.web_plugin_positions_[i]);
+  std::vector<WebPluginInfo>::const_iterator it =

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

please use size_t to iterate as before, it's much more readable

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

Same as above.

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

ditto, I see no reason to change the existing code from a size_t to an iterator
and just say it's changing in the future.

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

Same as above no loop in the code at all : 

http://codereview.chromium.org/5918003/diff/1/webkit/glue/plugins/plugin_grou...

+      other.web_plugin_infos_.begin();
+  for (; it != other.web_plugin_infos_.end(); ++it)
+    AddPlugin(*it, it->priority);
   if (!version_.get())
     version_.reset(Version::GetVersionFromString("0"));
+  enabled_ = other.enabled_;
 }
 
 PluginGroup::PluginGroup(const PluginGroup& other) {
@@ -167,6 +153,12 @@ PluginGroup* PluginGroup::FromWebPluginInfo(const WebPluginInfo& wpi) {
                          GetIdentifier(wpi));
 }
 
+/*static*/
+PluginGroup* PluginGroup::CreateEmptyGroup(const string16& name) {
+  // Create a matcher from the name of this plugin.
+  return new PluginGroup(name, name, std::string(), std::string());
+}
+
 bool PluginGroup::Match(const WebPluginInfo& plugin) const {
   if (name_matcher_.empty()) {
     return false;
@@ -214,7 +206,7 @@ Version* PluginGroup::CreateVersionFromString(const string16& version_string) {
 
 void PluginGroup::UpdateActivePlugin(const WebPluginInfo& plugin) {
   // A group is enabled if any of the files are enabled.
-  if (plugin.enabled) {
+  if (plugin.IsEnabled()) {
     if (!enabled_) {
       // If this is the first enabled plugin, use its description.
       enabled_ = true;
@@ -236,20 +228,60 @@ void PluginGroup::UpdateDescriptionAndVersion(const WebPluginInfo& plugin) {
     version_.reset(Version::GetVersionFromString("0"));
 }
 
-void PluginGroup::AddPlugin(const WebPluginInfo& plugin, int position) {
+bool PluginGroup::AddPlugin(const WebPluginInfo& plugin, int priority) {
   // Check if this group already contains this plugin.
-  for (size_t i = 0; i < web_plugin_infos_.size(); ++i) {
-    if (web_plugin_infos_[i].name == plugin.name &&
-        web_plugin_infos_[i].version == plugin.version &&
-        FilePath::CompareEqualIgnoreCase(web_plugin_infos_[i].path.value(),
+  for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin();

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

nit: please don't switch this to an iterator

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

Done.

+       it != web_plugin_infos_.end(); ++it) {
+    if (it->name == plugin.name &&
+        (!it->HasVersion() || it->version == plugin.version) &&
+        FilePath::CompareEqualIgnoreCase(it->path.value(),
                                          plugin.path.value())) {
-      return;
+      // If no version info present then just update this one. This was a
+      // placeholder put here by a disabled plugin.
+      if (!it->HasVersion()) {
+        // Preserve enabled flag and reason.
+        bool enabled = it->enabled;
+        int reason = it->reason;
+        *it = plugin;
+        it->priority = priority;
+        it->reason = reason;
+        it->enabled = enabled;
+        return true;
+      }
+      return false;
     }
   }
   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);
+  web_plugin_infos_.back().priority = priority;
+  UpdateActivePlugin(web_plugin_infos_.back());
+  return true;
+}
+
+bool PluginGroup::EnablePlugin(const FilePath& filename) {
+  for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin();
+       it != web_plugin_infos_.end(); ++it) {
+    if (it->path == filename) {
+      bool did_enable = it->Enable(WebPluginInfo::USER);
+      RefreshEnabledState();
+      return did_enable;
+    }
+  }
+  return false;
+}
+
+bool PluginGroup::DisablePlugin(const FilePath& filename) {
+  for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin();
+       it != web_plugin_infos_.end(); ++it) {
+    if (it->path == filename) {
+      bool did_disable = it->Disable(
+          IsPluginNameDisabledByPolicy(it->name) ?
+              WebPluginInfo::MANAGED : WebPluginInfo::USER);
+      RefreshEnabledState();
+      return did_disable;
+    }
+  }
+  return false;
 }
 
 string16 PluginGroup::GetGroupName() const {
@@ -257,7 +289,7 @@ string16 PluginGroup::GetGroupName() const {
     return group_name_;
   DCHECK_EQ(1u, web_plugin_infos_.size());
   FilePath::StringType path =
-      web_plugin_infos_[0].path.BaseName().RemoveExtension().value();
+      web_plugin_infos_.front().path.BaseName().RemoveExtension().value();
 #if defined(OS_POSIX)
   return UTF8ToUTF16(path);
 #elif defined(OS_WIN)
@@ -265,6 +297,21 @@ string16 PluginGroup::GetGroupName() const {
 #endif
 }
 
+const std::vector<WebPluginInfo>& PluginGroup::GetPlugins() const {
+  return web_plugin_infos_;
+}
+
+bool PluginGroup::HasPlugin(const FilePath& path) const {
+  for (std::vector<WebPluginInfo>::const_iterator it =

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

nit: size_t please

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

Done.

+           web_plugin_infos_.begin();
+       it != web_plugin_infos_.end(); ++it) {
+    if (it->path == path)
+      return true;
+  }
+  return false;
+}
+
+
 DictionaryValue* PluginGroup::GetSummary() const {
   DictionaryValue* result = new DictionaryValue();
   result->SetString("name", GetGroupName());
@@ -284,29 +331,30 @@ DictionaryValue* PluginGroup::GetDataForUI() const {
   bool group_disabled_by_policy = IsPluginNameDisabledByPolicy(name);
   ListValue* plugin_files = new ListValue();
   bool all_plugins_disabled_by_policy = true;
-  for (size_t i = 0; i < web_plugin_infos_.size(); ++i) {
-    const WebPluginInfo& web_plugin = web_plugin_infos_[i];
-    int priority = web_plugin_positions_[i];
+  for (std::vector<WebPluginInfo>::const_iterator it =

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

nit: size_t

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

Done.

+           web_plugin_infos_.begin();
+       it != web_plugin_infos_.end(); ++it) {
+    int priority = it->priority;
     DictionaryValue* plugin_file = new DictionaryValue();
-    plugin_file->SetString("name", web_plugin.name);
-    plugin_file->SetString("description", web_plugin.desc);
-    plugin_file->SetString("path", web_plugin.path.value());
-    plugin_file->SetString("version", web_plugin.version);
-    bool plugin_disabled_by_policy = group_disabled_by_policy ||
-        IsPluginNameDisabledByPolicy(web_plugin.name);
+    plugin_file->SetString("name", it->name);
+    plugin_file->SetString("description", it->desc);
+    plugin_file->SetString("path", it->path.value());
+    plugin_file->SetString("version", it->version);
+    bool plugin_disabled_by_policy =
+        group_disabled_by_policy || it->IsManaged(it->reason);
     if (plugin_disabled_by_policy) {
       plugin_file->SetString("enabledMode", "disabledByPolicy");
     } else {
       all_plugins_disabled_by_policy = false;
       plugin_file->SetString("enabledMode",
-                             web_plugin.enabled ? "enabled" : "disabledByUser");
+                             it->IsEnabled() ? "enabled" : "disabledByUser");
     }
     plugin_file->SetInteger("priority", priority);
 
     ListValue* mime_types = new ListValue();
     for (std::vector<WebPluginMimeType>::const_iterator type_it =

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

nit: size_t

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

I would prefer to keep this one an iterator as well as the inner for or else
we'll end up having web_plugin_infos_[i].mime_types[j].file_extensions[k] which
is anything but understandable.

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

you can always create a local const reference, i.e.
const WebPluginMimeType&  mime_type = web_plugin_infos_[i].mime_types[j]

it's nice to be consistent in a file for iterating over the same container

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

Done.

-         web_plugin.mime_types.begin();
-         type_it != web_plugin.mime_types.end();
+         it->mime_types.begin();
+         type_it != it->mime_types.end();
          ++type_it) {
       DictionaryValue* mime_type = new DictionaryValue();
       mime_type->SetString("mimeType", type_it->mime_type);
@@ -370,37 +418,57 @@ bool PluginGroup::IsVulnerable() const {
 }
 
 void PluginGroup::DisableOutdatedPlugins() {
-  description_ = string16();
-  enabled_ = false;
+  bool first_enabled = true;
 
-  for (std::vector<WebPluginInfo>::iterator it =
-           web_plugin_infos_.begin();
-       it != web_plugin_infos_.end(); ++it) {
+  for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin();

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

nit: size_t

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

Done.

+      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])) {
-          it->enabled = false;
-          NPAPI::PluginList::Singleton()->DisablePlugin(it->path);
+          it->Disable(WebPluginInfo::USER);
+          plugin_is_outdated = true;
+          break;
         }
       }
+      if (!plugin_is_outdated && first_enabled) {
+        first_enabled = false;
+        UpdateDescriptionAndVersion(*it);
+      }
     }
-    UpdateActivePlugin(*it);
   }
 }
 
-void PluginGroup::Enable(bool enable) {
+bool PluginGroup::Enable(bool enable) {
   bool enabled_plugin_exists = false;
-  for (std::vector<WebPluginInfo>::iterator it =
-       web_plugin_infos_.begin();
+  bool group_disabled_by_policy = IsPluginNameDisabledByPolicy(group_name_);
+  // We can't enable groups disabled by policy
+  if (group_disabled_by_policy && enable)
+    return false;
+
+  for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin();

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

nit: size_t

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

Done.

        it != web_plugin_infos_.end(); ++it) {
-    if (enable && !IsPluginNameDisabledByPolicy(it->name)) {
-      NPAPI::PluginList::Singleton()->EnablePlugin(it->path);
-      it->enabled = true;
+    bool policy_disabled = IsPluginNameDisabledByPolicy(it->name);
+    if (enable && !policy_disabled) {
+      it->Enable(WebPluginInfo::USER);
       enabled_plugin_exists = true;
     } else {
-      it->enabled = false;
-      NPAPI::PluginList::Singleton()->DisablePlugin(it->path);
+      it->Disable(policy_disabled || group_disabled_by_policy ?
+                      WebPluginInfo::MANAGED : WebPluginInfo::USER);
+    }
+  }
+  enabled_ = enabled_plugin_exists;
+  return enabled_ == enable;
+}
+
+void PluginGroup::RefreshEnabledState() {
+  bool enabled_plugin_exists = false;
+  for (std::vector<WebPluginInfo>::iterator it = web_plugin_infos_.begin();

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

nit: size_t

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

Done.

+       it != web_plugin_infos_.end(); ++it) {
+    if (it->IsEnabled()) {
+      enabled_plugin_exists = true;
+      break;
     }
   }
   enabled_ = enabled_plugin_exists;