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..9eecfec709b7ffe7f8936aefed4b45995356fdf0 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"
@@ -47,13 +48,13 @@ base::LazyInstance<std::map<FilePath, bool> > g_default_plugin_state =
 
 class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> {
  public:
-  explicit CallbackBarrier(const base::Closure& callback)
+  explicit CallbackBarrier(const base::Callback<void(bool)>& callback)
       : callback_(callback),
         outstanding_callbacks_(0) {
     DCHECK(!callback_.is_null());
   }
 
-  base::Closure CreateCallback() {
+  base::Callback<void(bool)> CreateCallback() {
     outstanding_callbacks_++;
     return base::Bind(&CallbackBarrier::MaybeRunCallback, this);
   }
@@ -65,15 +66,15 @@ class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> {
     DCHECK(callback_.is_null());
   }
 
-  void MaybeRunCallback() {
+  void MaybeRunCallback(bool dummy) {

[Bernhard Bauer, 2012/08/29 16:53:21]

Naming this parameter "dummy" is a bit confusing, because it's not actually a
dummy parameter.

It's also a bit weird to always return the last value. Maybe we should modify
the behavior so that it returns true if all (or any?) of the plug-ins could be
enabled?

[ibraaaa, 2012/08/29 17:37:53]

If I didn't miss something:

I checked the callers for EnablePluginGlobally, it is only called when Pepper
flash plugin is freshly installed. It is also called without checking if the
plugin state can be changed or not. So this gets us back a little bit, I assumed
when you asked to merge CanEnablePlugin and EnablePlugin that checking if the
plugin state can change (i.e. calling CanEnablePlugin) before calling
EnablePlugin will not affect the behavior of the code in this case (I mean in
EnablePluginGlobally).

Now, I intentionally named the parameter dummy and didn't apply any logic
because according to the old behavior of the  code there is no check need to be
performed on the result of 'CanEnablePlugin' simply because it was not called
before :)

I just don't see the point in Converting "dummy" to not "dummy"?

Done.

On 2012/08/29 16:53:21, Bernhard Bauer wrote:

[Bernhard Bauer, 2012/08/29 18:10:17]

What I meant was that it's weird to return an arbitrary value, even if we know
that it's going to be ignored. We could either not return any value at all, or
have a defined behavior (which is what you implemented). I'm fine with any of
those.

[ibraaaa, 2012/08/29 18:28:19]

Ah, I see. My intention was not returning any value at all so I thought that
fulfilling this can just be done by naming the value returned dummy, apparently
not :)

On 2012/08/29 18:10:17, Bernhard Bauer wrote:

     DCHECK_GT(outstanding_callbacks_, 0);
     if (--outstanding_callbacks_ == 0) {
-      callback_.Run();
+      callback_.Run(dummy);
       callback_.Reset();
     }
   }
 
-  base::Closure callback_;
+  base::Callback<void(bool)> callback_;
   int outstanding_callbacks_;
 };
 
@@ -102,106 +103,134 @@ 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(
+    bool enabled, const FilePath& path,
+    const base::Callback<void(bool)>& callback,
+    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(
+  if (!can_enable) {
+    callback.Run(false);
+    return;
+  }
+
+  PluginService::GetInstance()->GetPlugins(
       base::Bind(&PluginPrefs::EnablePluginInternal, this,
-                 enabled, path, callback));
+                 enabled, path, finder, callback));
+}
+
+void PluginPrefs::EnablePlugin(
+    bool enabled, const FilePath& path,
+    const base::Callback<void(bool)>& callback) {
+  PluginFinder::Get(base::Bind(&PluginPrefs::EnablePluginIfPossibleCallback,
+                               this, enabled, path, callback));
 }
 
 void PluginPrefs::EnablePluginInternal(
     bool enabled,
     const FilePath& path,
-    const base::Closure& callback,
-    const std::vector<webkit::npapi::PluginGroup>& groups) {
+    PluginFinder* plugin_finder,
+    const base::Callback<void(bool)>& callback,
+    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]);
+    DCHECK(!installer->name().empty());
+    if (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();
+  callback.Run(true);
 }
 
 // static
-void PluginPrefs::EnablePluginGlobally(bool enable, const FilePath& file_path,
-                                       const base::Closure& callback) {
+void PluginPrefs::EnablePluginGlobally(
+    bool enable,
+    const FilePath& file_path,
+    const base::Callback<void(bool)>& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   g_default_plugin_state.Get()[file_path] = enable;
   std::vector<Profile*> profiles =
@@ -511,17 +540,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 +571,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();
+      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);
   }
 }