Changing PluginPrefs to use PluginFinder's async interface.

chrome/browser/ui/webui/plugins_ui.cc

Index: chrome/browser/ui/webui/plugins_ui.cc
diff --git a/chrome/browser/ui/webui/plugins_ui.cc b/chrome/browser/ui/webui/plugins_ui.cc
index 1b9676f66e7579a50c5f86043bab5fce23968cf8..043c591dbd8b6c88469ee9836212de017d422768 100644
--- a/chrome/browser/ui/webui/plugins_ui.cc
+++ b/chrome/browser/ui/webui/plugins_ui.cc
@@ -19,6 +19,8 @@
 #include "base/values.h"
 #include "chrome/browser/api/prefs/pref_member.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
+#include "chrome/browser/plugin_finder.h"
+#include "chrome/browser/plugin_installer.h"
 #include "chrome/browser/plugin_prefs.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
@@ -49,15 +51,6 @@
 #include "chrome/browser/ui/webui/chromeos/ui_account_tweaks.h"
 #endif
 
-#if defined(ENABLE_PLUGIN_INSTALLATION)
-#include "chrome/browser/plugin_finder.h"
-#include "chrome/browser/plugin_installer.h"
-#else
-// Forward-declare PluginFinder. It's never actually used, but we pass a NULL
-// pointer instead.
-class PluginFinder;
-#endif
-
 using content::PluginService;
 using content::WebContents;
 using content::WebUIMessageHandler;
@@ -66,6 +59,11 @@ using webkit::WebPluginInfo;
 
 namespace {
 
+// Helper callback method to process result of CanEnablePlugin method.

[James Hawkins, 2012/09/04 14:13:17]

nit: No need to specify that this is a helper callback.

For future reference this is a function not a method, the latter being a
function on an object.

[ibraaaa, 2012/09/04 14:52:09]

Done.

+void CheckCanEnablePluginCallback(bool can_enable) {

[James Hawkins, 2012/09/04 14:13:17]

What is the purpose of this function?

[ibraaaa, 2012/09/04 14:52:09]

We merged EnablePlugin and CanEnablePlugin into one method and accordingly the
result of the state-change check done by CanEnablePlugin is now supplied as
input to the Callback function (in this case CheckCanEnablePluginCallback).

Since the old code used to DCHECK the result of CanEnablePlugin we maintained
the same behavior by DCHECK-ing the result input to the callback.

On 2012/09/04 14:13:17, James Hawkins wrote:

[James Hawkins, 2012/09/04 14:55:50]

OK.  The function name seems a bit misleading as it's not checking anything (it
is asserting, however).  It's also misleading since a 'check' callback function
(to me) implies there is a boolean return value that the caller will use.

[ibraaaa, 2012/09/04 15:09:08]

Ahh, ok. Actually, the name is a left over from multiple review iterations. Just
missed it out, also the documentation had CanEnablePlugin which does not exist
now.
I modified this and the parameter name to be consistent with other usages of
EnablePlugin and also to be consistent with the fact that the check has already
happened and this is the result of either the plugin was enabled or not. Thanks!

On 2012/09/04 14:55:50, James Hawkins wrote:

+  DCHECK(can_enable);
+}
+
 ChromeWebUIDataSource* CreatePluginsUIHTMLSource() {
   ChromeWebUIDataSource* source =
       new ChromeWebUIDataSource(chrome::kChromeUIPluginsHost);
@@ -174,12 +172,15 @@ class PluginsDOMHandler : public WebUIMessageHandler,
 
   // Called on the UI thread when the plugin information is ready.
   void PluginsLoaded(PluginFinder* plugin_finder,
-                     const std::vector<PluginGroup>& groups);
+                     const std::vector<webkit::WebPluginInfo>& plugins);
 
   content::NotificationRegistrar registrar_;
 
   base::WeakPtrFactory<PluginsDOMHandler> weak_ptr_factory_;
 
+  typedef base::hash_map<std::string, std::vector<const WebPluginInfo*> >

[James Hawkins, 2012/09/04 14:13:17]

nit: Document the map.

[ibraaaa, 2012/09/04 14:52:09]

Done.

+      PluginGroups;
+
   // This pref guards the value whether about:plugins is in the details mode or
   // not.
   BooleanPrefMember show_details_;
@@ -266,9 +267,9 @@ void PluginsDOMHandler::HandleEnablePluginMessage(const ListValue* args) {
       NOTREACHED();
       return;
     }
-    DCHECK(plugin_prefs->CanEnablePlugin(enable, FilePath(file_path)));
+
     plugin_prefs->EnablePlugin(enable, FilePath(file_path),
-                               base::Bind(&base::DoNothing));
+                               base::Bind(&CheckCanEnablePluginCallback));
   }
 }
 
@@ -334,32 +335,43 @@ void PluginsDOMHandler::GetPluginFinder() {
 }
 
 void PluginsDOMHandler::LoadPlugins(PluginFinder* plugin_finder) {
-  PluginService::GetInstance()->GetPluginGroups(
+  PluginService::GetInstance()->GetPlugins(
       base::Bind(&PluginsDOMHandler::PluginsLoaded,
           weak_ptr_factory_.GetWeakPtr(), plugin_finder));
 }
 
-void PluginsDOMHandler::PluginsLoaded(PluginFinder* plugin_finder,
-                                      const std::vector<PluginGroup>& groups) {
+void PluginsDOMHandler::PluginsLoaded(
+    PluginFinder* plugin_finder,
+    const std::vector<webkit::WebPluginInfo>& plugins) {
   Profile* profile = Profile::FromWebUI(web_ui());
   PluginPrefs* plugin_prefs = PluginPrefs::GetForProfile(profile);
 
   ContentSettingsPattern wildcard = ContentSettingsPattern::Wildcard();
 
-  // Construct DictionaryValues to return to the UI
+  // Group plug-ins by identifier. This is done to be able to display
+  // the plug-ins in UI in a grouped fashion.
+  PluginGroups groups;
+  for (size_t i = 0; i < plugins.size(); ++i) {
+    PluginInstaller* installer = plugin_finder->GetPluginInstaller(plugins[i]);
+    groups[installer->identifier()].push_back(&plugins[i]);
+  }
+
+  // Construct DictionaryValues to return to UI.
   ListValue* plugin_groups_data = new ListValue();
-  for (size_t i = 0; i < groups.size(); ++i) {
-    const PluginGroup& group = groups[i];
-    if (group.IsEmpty())
-      continue;
+  for (PluginGroups::const_iterator it = groups.begin();
+      it != groups.end(); ++it) {
+    const std::vector<const WebPluginInfo*>& group_plugins = it->second;
     ListValue* plugin_files = new ListValue();
-    string16 group_name = group.GetGroupName();
+    PluginInstaller* plugin_installer =
+        plugin_finder->GetPluginInstaller(*group_plugins[0]);
+    string16 group_name = plugin_installer->name();
+    std::string group_identifier = plugin_installer->identifier();
     bool group_enabled = false;
     bool all_plugins_enabled_by_policy = true;
     bool all_plugins_disabled_by_policy = true;
     const WebPluginInfo* active_plugin = NULL;
-    for (size_t j = 0; j < group.web_plugin_infos().size(); ++j) {
-      const WebPluginInfo& group_plugin = group.web_plugin_infos()[j];
+    for (size_t j = 0; j < group_plugins.size(); ++j) {
+      const WebPluginInfo& group_plugin = *group_plugins[j];
 
       DictionaryValue* plugin_file = new DictionaryValue();
       plugin_file->SetString("name", group_plugin.name);
@@ -404,7 +416,7 @@ void PluginsDOMHandler::PluginsLoaded(PluginFinder* plugin_finder,
       } else {
         all_plugins_enabled_by_policy = false;
         if (plugin_status == PluginPrefs::POLICY_DISABLED ||
-          group_status == PluginPrefs::POLICY_DISABLED) {
+            group_status == PluginPrefs::POLICY_DISABLED) {
           enabled_mode = "disabledByPolicy";
         } else {
           all_plugins_disabled_by_policy = false;
@@ -423,13 +435,13 @@ void PluginsDOMHandler::PluginsLoaded(PluginFinder* plugin_finder,
 
     group_data->Set("plugin_files", plugin_files);
     group_data->SetString("name", group_name);
-    group_data->SetString("id", group.identifier());
+    group_data->SetString("id", group_identifier);
     group_data->SetString("description", active_plugin->desc);
     group_data->SetString("version", active_plugin->version);
 
 #if defined(ENABLE_PLUGIN_INSTALLATION)
     PluginInstaller* installer =
-        plugin_finder->FindPluginWithIdentifier(group.identifier());
+        plugin_finder->FindPluginWithIdentifier(group_identifier);
     if (installer) {
       bool out_of_date = installer->GetSecurityStatus(*active_plugin) ==
                          PluginInstaller::SECURITY_STATUS_OUT_OF_DATE;
@@ -454,7 +466,7 @@ void PluginsDOMHandler::PluginsLoaded(PluginFinder* plugin_finder,
     if (group_enabled) {
       const DictionaryValue* whitelist = profile->GetPrefs()->GetDictionary(
           prefs::kContentSettingsPluginWhitelist);
-      whitelist->GetBoolean(group.identifier(), &always_allowed);
+      whitelist->GetBoolean(group_identifier, &always_allowed);
     }
     group_data->SetBoolean("alwaysAllowed", always_allowed);