Using MIME types in addition to plugin name to differentiate between plugins.

chrome/browser/plugins/plugin_finder.cc

Index: chrome/browser/plugins/plugin_finder.cc
diff --git a/chrome/browser/plugins/plugin_finder.cc b/chrome/browser/plugins/plugin_finder.cc
index 2ac0b99352ce6c745c4b821415d7bf5057de0d6a..f168bbdfe37ef83b8746cdcfe4db8deff22f89a0 100644
--- a/chrome/browser/plugins/plugin_finder.cc
+++ b/chrome/browser/plugins/plugin_finder.cc
@@ -31,6 +31,9 @@ using content::PluginService;
 
 namespace {
 
+const char kMimeTypesListKey[] = "mime_types";
+const char kMatchingMimeTypesListKey[] = "matching_mime_types";
+
 // Gets the base name of the file path as the identifier.
 static std::string GetIdentifier(const webkit::WebPluginInfo& plugin) {
 #if defined(OS_POSIX)
@@ -54,6 +57,30 @@ static string16 GetGroupName(const webkit::WebPluginInfo& plugin) {
 #endif
 }
 
+void LoadMimeTypes(const std::string& mime_type_list_key,
+                   const DictionaryValue* plugin_dict,
+                   PluginMetadata* plugin) {
+  const ListValue* mime_types = NULL;
+  if (!plugin_dict->GetList(mime_type_list_key, &mime_types))
+    return;
+
+  bool success = false;
+  for (ListValue::const_iterator mime_type_it = mime_types->begin();
+       mime_type_it != mime_types->end(); ++mime_type_it) {
+    std::string mime_type_str;
+    success = (*mime_type_it)->GetAsString(&mime_type_str);
+    DCHECK(success);
+    if (mime_type_list_key == kMatchingMimeTypesListKey) {

[Bernhard Bauer, 2012/10/02 14:40:13]

This might be a bit simpler if you create a method that parses a ListValue of
StringValues and writes the strings to a std::vector<std::string>. Then you
fetch the list from the dictionary below and add the MIME types directly.

Alternatively, you could pass in a boolean that tells whether to look at the
list of MIME types for installing or for matching.

[ibraaaa, 2012/10/02 16:14:53]

Well, if I send a boolean value (actually that is what I was doing before this)
it will be doing the same thing as |mime_type_list_key| since I need this key
anyway to fetch the correct list from the dictionary, WDYT?

On 2012/10/02 14:40:13, Bernhard Bauer wrote:

[Bernhard Bauer, 2012/10/04 11:37:19]

Right, it just seems a bit more awkward to pass in a string that can only have
two values instead of a boolean.

[ibraaaa, 2012/10/04 12:24:12]

Done.

+      plugin->AddMatchingMimeType(mime_type_str);
+    } else if (mime_type_list_key == kMimeTypesListKey) {
+      plugin->AddMimeType(mime_type_str);
+    } else {
+      NOTREACHED();
+      return;
+    }
+  }
+}
+
 PluginMetadata* CreatePluginMetadata(
     const std::string& identifier,
     const DictionaryValue* plugin_dict) {
@@ -69,13 +96,16 @@ PluginMetadata* CreatePluginMetadata(
   string16 group_name_matcher;
   success = plugin_dict->GetString("group_name_matcher", &group_name_matcher);
   DCHECK(success);
+  std::string language_str;
+  plugin_dict->GetString("lang", &language_str);
 
   PluginMetadata* plugin = new PluginMetadata(identifier,
                                               name,
                                               display_url,
                                               GURL(url),
                                               GURL(help_url),
-                                              group_name_matcher);
+                                              group_name_matcher,
+                                              language_str);
   const ListValue* versions = NULL;
   if (plugin_dict->GetList("versions", &versions)) {
     for (ListValue::const_iterator it = versions->begin();
@@ -99,6 +129,8 @@ PluginMetadata* CreatePluginMetadata(
     }
   }
 
+  LoadMimeTypes(kMimeTypesListKey, plugin_dict, plugin);
+  LoadMimeTypes(kMatchingMimeTypesListKey, plugin_dict, plugin);
   return plugin;
 }
 
@@ -167,38 +199,19 @@ bool PluginFinder::FindPlugin(
   base::AutoLock lock(mutex_);
   if (g_browser_process->local_state()->GetBoolean(prefs::kDisablePluginFinder))
     return false;
-  for (DictionaryValue::Iterator plugin_it(*plugin_list_);
-       plugin_it.HasNext(); plugin_it.Advance()) {
-    const DictionaryValue* plugin = NULL;
-    if (!plugin_it.value().GetAsDictionary(&plugin)) {
-      NOTREACHED();
-      continue;
-    }
-    std::string language_str;
-    bool success = plugin->GetString("lang", &language_str);
-    if (language_str != language)
-      continue;
-    const ListValue* mime_types = NULL;
-    plugin->GetList("mime_types", &mime_types);
-    DCHECK(success);
-    for (ListValue::const_iterator mime_type_it = mime_types->begin();
-         mime_type_it != mime_types->end(); ++mime_type_it) {
-      std::string mime_type_str;
-      success = (*mime_type_it)->GetAsString(&mime_type_str);
-      DCHECK(success);
-      if (mime_type_str == mime_type) {
-        std::string identifier = plugin_it.key();
-        std::map<std::string, PluginMetadata*>::const_iterator metadata_it =
-            identifier_plugin_.find(identifier);
-        DCHECK(metadata_it != identifier_plugin_.end());
-        *plugin_metadata = metadata_it->second->Clone();
-
-        std::map<std::string, PluginInstaller*>::const_iterator installer_it =
-            installers_.find(identifier);
-        DCHECK(installer_it != installers_.end());
-        *installer = installer_it->second;
-        return true;
-      }
+
+  std::map<std::string, PluginMetadata*>::const_iterator metadata_it =

[Bernhard Bauer, 2012/10/02 14:40:13]

You use this map type in a bunch of places. You could typedef it to make it a
bit shorter. 

[ibraaaa, 2012/10/02 16:14:53]

Done.

+      identifier_plugin_.begin();
+  for (; metadata_it != identifier_plugin_.end(); ++metadata_it) {
+    if (language == metadata_it->second->language() &&
+        metadata_it->second->HasMimeType(mime_type)) {
+      *plugin_metadata = metadata_it->second->Clone();
+
+      std::map<std::string, PluginInstaller*>::const_iterator installer_it =
+          installers_.find(metadata_it->second->identifier());
+      DCHECK(installer_it != installers_.end());
+      *installer = installer_it->second;
+      return true;
     }
   }
   return false;
@@ -239,16 +252,13 @@ string16 PluginFinder::FindPluginNameWithIdentifier(
 scoped_ptr<PluginMetadata> PluginFinder::GetPluginMetadata(
     const webkit::WebPluginInfo& plugin) {
   base::AutoLock lock(mutex_);
-  if (name_plugin_.find(plugin.name) != name_plugin_.end())
-    return name_plugin_[plugin.name]->Clone();
-
-  // Use the group name matcher to find the plug-in metadata we want.
   for (std::map<std::string, PluginMetadata*>::const_iterator it =
       identifier_plugin_.begin(); it != identifier_plugin_.end(); ++it) {
-    if (!it->second->MatchesPlugin(plugin))
+    string16 matching_name;
+    if (!it->second->MatchesPlugin(plugin, &matching_name))
       continue;
 
-    name_plugin_[plugin.name] = it->second;
+    name_plugin_[matching_name] = it->second;

[Bernhard Bauer, 2012/10/02 14:40:13]

You're never reading from |name_plugin_| anymore.

It seems that now we can't find a plug-in based on the name alone, so the cache
might not make sense anymore.

[ibraaaa, 2012/10/02 16:14:53]

Done.

     return it->second->Clone();
   }
 
@@ -258,8 +268,8 @@ scoped_ptr<PluginMetadata> PluginFinder::GetPluginMetadata(
   PluginMetadata* metadata = new PluginMetadata(identifier,
                                                 GetGroupName(plugin),
                                                 false, GURL(), GURL(),
-                                                GetGroupName(plugin));
-
+                                                GetGroupName(plugin),
+                                                "");
   name_plugin_[plugin.name] = metadata;
   identifier_plugin_[identifier] = metadata;
   return metadata->Clone();