Changing PluginPrefs to use PluginFinder's async interface.

chrome/browser/plugin_prefs.cc

Index: chrome/browser/plugin_prefs.cc
diff --git a/chrome/browser/plugin_prefs.cc b/chrome/browser/plugin_prefs.cc
index 00472170397c438f6079862e772131a61e6e38e0..f27fec213fda6ccdbc1abae815da58341e87ea4b 100644
--- a/chrome/browser/plugin_prefs.cc
+++ b/chrome/browser/plugin_prefs.cc
@@ -18,6 +18,7 @@
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/plugin_installer.h"
 #include "chrome/browser/plugin_prefs_factory.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
@@ -77,6 +78,10 @@ class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> {
   int outstanding_callbacks_;
 };
 
+// A callback function that does nothing.

[Pam (message me for reviews), 2012/08/27 13:18:05]

This comment doesn't add any information. Please either expand it (when and why
would this callback be used?) or remove it.

[ibraaaa, 2012/08/28 09:25:52]

Done.

+void CanEnablePluginCallbackDoNothing(bool dummy) {

[Bernhard Bauer, 2012/08/29 12:56:36]

This is available as base::DoNothing() in base/bind_helpers.h.

[ibraaaa, 2012/08/29 15:35:50]

It doesn't take bool parameter, does it?
On 2012/08/29 12:56:36, Bernhard Bauer wrote:

+}
+
 }  // namespace
 
 // How long to wait to save the plugin enabled information, which might need to
@@ -102,98 +107,128 @@ void PluginPrefs::SetPluginListForTesting(
 }
 
 void PluginPrefs::EnablePluginGroup(bool enabled, const string16& group_name) {
-  PluginService::GetInstance()->GetPluginGroups(
-        base::Bind(&PluginPrefs::EnablePluginGroupInternal,
-                   this, enabled, group_name));
+  PluginFinder::Get(
+      base::Bind(&PluginPrefs::GetPluginFinderForEnablePluginGroup,
+                 this, enabled, group_name));
+}
+
+void PluginPrefs::GetPluginFinderForEnablePluginGroup(
+    bool enabled,
+    const string16& group_name,
+    PluginFinder* finder) {
+  PluginService::GetInstance()->GetPlugins(
+      base::Bind(&PluginPrefs::EnablePluginGroupInternal,
+                 this, enabled, group_name, finder));
 }
 
 void PluginPrefs::EnablePluginGroupInternal(
     bool enabled,
     const string16& group_name,
-    const std::vector<webkit::npapi::PluginGroup>& groups) {
+    PluginFinder* finder,
+    const std::vector<webkit::WebPluginInfo>& plugins) {
   base::AutoLock auto_lock(lock_);
 
   // Set the desired state for the group.
   plugin_group_state_[group_name] = enabled;
 
   // Update the state for all plug-ins in the group.
-  for (size_t i = 0; i < groups.size(); ++i) {
-    if (groups[i].GetGroupName() != group_name)
+  for (size_t i = 0; i < plugins.size(); ++i) {
+    PluginInstaller* installer = finder->GetPluginInstaller(plugins[i]);
+    if (group_name != installer->name())
       continue;
-    const std::vector<webkit::WebPluginInfo>& plugins =
-        groups[i].web_plugin_infos();
-    for (size_t j = 0; j < plugins.size(); ++j)
-      plugin_state_[plugins[j].path] = enabled;
-    break;
+    plugin_state_[plugins[i].path] = enabled;
   }
 
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-      base::Bind(&PluginPrefs::OnUpdatePreferences, this, groups));
+      base::Bind(&PluginPrefs::OnUpdatePreferences, this, plugins, finder));
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
       base::Bind(&PluginPrefs::NotifyPluginStatusChanged, this));
 }
 
-bool PluginPrefs::CanEnablePlugin(bool enabled, const FilePath& path) {
-  webkit::npapi::PluginList* plugin_list = GetPluginList();
+void PluginPrefs::EnablePluginIfPossibleCallback(

[Bernhard Bauer, 2012/08/29 12:56:36]

I still don't think we need two separate callbacks here. It should be fine to
just call |canEnableCallback| (which should be |can_enable_callback|, BTW) with
false if we can't enable the plug-in, or with true after enabling it if we can.

[ibraaaa, 2012/08/29 15:35:50]

I wanted to keep the exact same interface but I guess what you thought is
better. Done.

On 2012/08/29 12:56:36, Bernhard Bauer wrote:

+    bool enabled, const FilePath& path,
+    const base::Closure& enableCallback,
+    const base::Callback<void(bool)>& canEnableCallback,
+    PluginFinder* finder) {
   webkit::WebPluginInfo plugin;
+  bool can_enable = true;
   if (PluginService::GetInstance()->GetPluginInfoByPath(path, &plugin)) {
-    scoped_ptr<webkit::npapi::PluginGroup> group(
-        plugin_list->GetPluginGroup(plugin));
+    PluginInstaller* installer = finder->GetPluginInstaller(plugin);
     PolicyStatus plugin_status = PolicyStatusForPlugin(plugin.name);
-    PolicyStatus group_status = PolicyStatusForPlugin(group->GetGroupName());
+    PolicyStatus group_status = PolicyStatusForPlugin(installer->name());
     if (enabled) {
       if (plugin_status == POLICY_DISABLED || group_status == POLICY_DISABLED)
-        return false;
+        can_enable = false;
     } else {
       if (plugin_status == POLICY_ENABLED || group_status == POLICY_ENABLED)
-        return false;
+        can_enable = false;
     }
   } else {
     NOTREACHED();
   }
-  return true;
-}
 
-void PluginPrefs::EnablePlugin(bool enabled, const FilePath& path,
-                               const base::Closure& callback) {
-  PluginService::GetInstance()->GetPluginGroups(
+  canEnableCallback.Run(can_enable);
+
+  if (!can_enable)
+    return;
+
+  PluginService::GetInstance()->GetPlugins(
       base::Bind(&PluginPrefs::EnablePluginInternal, this,
-                 enabled, path, callback));
+                 enabled, path, finder, enableCallback));
+}
+
+void PluginPrefs::EnablePlugin(
+    bool enabled, const FilePath& path,
+    const base::Closure& enableCallback,
+    const base::Callback<void(bool)>& canEnableCallback) {
+  PluginFinder::Get(base::Bind(&PluginPrefs::EnablePluginIfPossibleCallback,
+                               this, enabled, path, enableCallback,
+                               canEnableCallback));
 }
 
 void PluginPrefs::EnablePluginInternal(
     bool enabled,
     const FilePath& path,
+    PluginFinder* plugin_finder,
     const base::Closure& callback,
-    const std::vector<webkit::npapi::PluginGroup>& groups) {
+    const std::vector<webkit::WebPluginInfo>& plugins) {
   {
     // Set the desired state for the plug-in.
     base::AutoLock auto_lock(lock_);
     plugin_state_[path] = enabled;
   }
 
-  bool found_group = false;
-  for (size_t i = 0; i < groups.size(); ++i) {
-    bool all_disabled = true;
-    const std::vector<webkit::WebPluginInfo>& plugins =
-        groups[i].web_plugin_infos();
-    for (size_t j = 0; j < plugins.size(); ++j) {
-      all_disabled = all_disabled && !IsPluginEnabled(plugins[j]);
-      if (plugins[j].path == path) {
-        found_group = true;
-        DCHECK_EQ(enabled, IsPluginEnabled(plugins[j]));
-      }
-    }
-    if (found_group) {
-      // Update the state for the corresponding plug-in group.
-      base::AutoLock auto_lock(lock_);
-      plugin_group_state_[groups[i].GetGroupName()] = !all_disabled;
+  string16 group_name;
+  for (size_t i = 0; i < plugins.size(); ++i) {
+    if (plugins[i].path == path) {
+      PluginInstaller* installer =
+          plugin_finder->GetPluginInstaller(plugins[i]);
+      // set the group name for this plug-in.
+      group_name = installer->name();
+      DCHECK_EQ(enabled, IsPluginEnabled(plugins[i]));
       break;
     }
   }
 
+  bool all_disabled = true;
+  for (size_t i = 0; i < plugins.size(); ++i) {
+    PluginInstaller* installer = plugin_finder->GetPluginInstaller(plugins[i]);
+    // The check !group_name.empty() is only a sanity check since
+    // GetPluginInstaller will always return a non-empty name.

[Bernhard Bauer, 2012/08/29 12:56:36]

If that is the case, then you should DCHECK, I guess.

[ibraaaa, 2012/08/29 15:35:50]

Added, however let me confirm if we can have a plugin with path set to an empty
string, can we?

On 2012/08/29 12:56:36, Bernhard Bauer wrote:

+    if (!group_name.empty() && group_name == installer->name()) {
+      all_disabled = all_disabled && !IsPluginEnabled(plugins[i]);
+    }
+  }
+
+  if (!group_name.empty()) {
+    // Update the state for the corresponding plug-in group.
+    base::AutoLock auto_lock(lock_);
+    plugin_group_state_[group_name] = !all_disabled;
+  }
+
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-      base::Bind(&PluginPrefs::OnUpdatePreferences, this, groups));
+      base::Bind(&PluginPrefs::OnUpdatePreferences, this,
+                 plugins, plugin_finder));
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
       base::Bind(&PluginPrefs::NotifyPluginStatusChanged, this));
   callback.Run();
@@ -211,7 +246,8 @@ void PluginPrefs::EnablePluginGlobally(bool enable, const FilePath& file_path,
        it != profiles.end(); ++it) {
     PluginPrefs* plugin_prefs = PluginPrefs::GetForProfile(*it);
     DCHECK(plugin_prefs);
-    plugin_prefs->EnablePlugin(enable, file_path, barrier->CreateCallback());
+    plugin_prefs->EnablePlugin(enable, file_path, barrier->CreateCallback(),
+                               base::Bind(&CanEnablePluginCallbackDoNothing));
   }
 }
 
@@ -511,17 +547,24 @@ webkit::npapi::PluginList* PluginPrefs::GetPluginList() const {
 }
 
 void PluginPrefs::GetPreferencesDataOnFileThread() {
-  std::vector<webkit::npapi::PluginGroup> groups;
+  PluginFinder::Get(
+      base::Bind(&PluginPrefs::GetPluginFinderForGetPreferencesDataOnFileThread,
+                 this));
+}
 
+void PluginPrefs::GetPluginFinderForGetPreferencesDataOnFileThread(
+    PluginFinder* finder) {
+  std::vector<webkit::WebPluginInfo> plugins;
   webkit::npapi::PluginList* plugin_list = GetPluginList();
-  plugin_list->GetPluginGroups(false, &groups);
+  plugin_list->GetPluginsNoRefresh(&plugins);
 
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-      base::Bind(&PluginPrefs::OnUpdatePreferences, this, groups));
+      base::Bind(&PluginPrefs::OnUpdatePreferences, this, plugins, finder));
 }
 
 void PluginPrefs::OnUpdatePreferences(
-    const std::vector<webkit::npapi::PluginGroup>& groups) {
+    const std::vector<webkit::WebPluginInfo>& plugins,
+    PluginFinder* finder) {
   if (!prefs_)
     return;
 
@@ -535,35 +578,37 @@ void PluginPrefs::OnUpdatePreferences(
 
   base::AutoLock auto_lock(lock_);
 
-  // Add the plug-in groups.
-  for (size_t i = 0; i < groups.size(); ++i) {
-    // Add the plugin files to the same list.
-    const std::vector<webkit::WebPluginInfo>& plugins =
-        groups[i].web_plugin_infos();
-    for (size_t j = 0; j < plugins.size(); ++j) {
-      DictionaryValue* summary = new DictionaryValue();
-      summary->SetString("path", plugins[j].path.value());
-      summary->SetString("name", plugins[j].name);
-      summary->SetString("version", plugins[j].version);
-      bool enabled = true;
-      std::map<FilePath, bool>::iterator it =
-          plugin_state_.find(plugins[j].path);
-      if (it != plugin_state_.end())
-        enabled = it->second;
-      summary->SetBoolean("enabled", enabled);
-      plugins_list->Append(summary);
-    }
-
+  // Add the plugin files.
+  std::set<string16> group_names;
+  for (size_t i = 0; i < plugins.size(); ++i) {
     DictionaryValue* summary = new DictionaryValue();
-    string16 name = groups[i].GetGroupName();
-    summary->SetString("name", name);
+    summary->SetString("path", plugins[i].path.value());
+    summary->SetString("name", plugins[i].name);
+    summary->SetString("version", plugins[i].version);
     bool enabled = true;
-    std::map<string16, bool>::iterator it =
-        plugin_group_state_.find(name);
-    if (it != plugin_group_state_.end())
+    std::map<FilePath, bool>::iterator it = plugin_state_.find(plugins[i].path);
+    if (it != plugin_state_.end())
       enabled = it->second;
     summary->SetBoolean("enabled", enabled);
     plugins_list->Append(summary);
+
+    PluginInstaller* installer = finder->GetPluginInstaller(plugins[i]);
+    // Insert into a set of all group names.
+    group_names.insert(installer->name());
+  }
+
+  // Add the plug-in groups.
+  for (std::set<string16>::const_iterator it = group_names.begin();

[ibraaaa, 2012/08/24 15:56:21]

I guess that here the order of having the plugins within a group inserted in the
list followed by the group is not important, if it tended out that I missed
something, then this should change to group plugins first.

+      it != group_names.end(); ++it) {
+    DictionaryValue* summary = new DictionaryValue();
+    summary->SetString("name", *it);
+    bool enabled = true;
+    std::map<string16, bool>::iterator gstate_it =
+        plugin_group_state_.find(*it);
+    if (gstate_it != plugin_group_state_.end())
+      enabled = gstate_it->second;
+    summary->SetBoolean("enabled", enabled);
+    plugins_list->Append(summary);
   }
 }