[6] Moves CreateVersionFromString to plugin_utils and updates the callers.

chrome/browser/plugin_installer.cc

Index: chrome/browser/plugin_installer.cc
diff --git a/chrome/browser/plugin_installer.cc b/chrome/browser/plugin_installer.cc
index bcfbecc28a1c8ec5f3d6f4e161b6d66bd7c8f229..17379a2bea802eb20acb0ce92b33caffa394ac56 100644
--- a/chrome/browser/plugin_installer.cc
+++ b/chrome/browser/plugin_installer.cc
@@ -8,10 +8,12 @@
 #include "base/bind_helpers.h"
 #include "base/file_util.h"
 #include "base/process.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/platform_util.h"
+#include "chrome/browser/plugin_finder.h"
 #include "chrome/browser/plugin_installer_observer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
@@ -72,9 +74,11 @@ PluginInstaller::PluginInstaller(const std::string& identifier,
                                  const string16& name,
                                  bool url_for_display,
                                  const GURL& plugin_url,
-                                 const GURL& help_url)
+                                 const GURL& help_url,
+                                 const string16& group_name_matcher)
     : identifier_(identifier),
       name_(name),
+      group_name_matcher_(group_name_matcher),
       url_for_display_(url_for_display),
       plugin_url_(plugin_url),
       help_url_(help_url),
@@ -97,7 +101,7 @@ PluginInstaller::SecurityStatus PluginInstaller::GetSecurityStatus(
     return SECURITY_STATUS_REQUIRES_AUTHORIZATION;
 
   Version version;
-  webkit::npapi::PluginGroup::CreateVersionFromString(plugin.version, &version);
+  webkit::WebPluginInfo::CreateVersionFromString(plugin.version, &version);
   if (!version.IsValid())
     version = Version("0");
 
@@ -250,3 +254,54 @@ void PluginInstaller::DownloadCancelled() {
   state_ = INSTALLER_STATE_IDLE;
   FOR_EACH_OBSERVER(PluginInstallerObserver, observers_, DownloadCancelled());
 }
+
+/*static*/

[Bernhard Bauer, 2012/08/21 12:30:18]

Nit: The rest of the file uses // static.

[ibraaaa, 2012/08/21 14:26:04]

Done.

+string16 PluginInstaller::GetGroupName(const webkit::WebPluginInfo& plugin) {
+  PluginInstaller* plugin_installer = MatchWithConfiguredPlugins(plugin);
+  if (plugin_installer != NULL) {

[Bernhard Bauer, 2012/08/21 12:30:18]

Nit: Braces aren't required for single-line statements.

[ibraaaa, 2012/08/21 14:26:04]

Done.

+    return plugin_installer->name();
+  }
+
+  if (!plugin.name.empty()) {
+    return plugin.name;
+  }
+
+  FilePath::StringType path = plugin.path.BaseName().RemoveExtension().value();
+#if defined(OS_POSIX)
+  return UTF8ToUTF16(path);
+#elif defined(OS_WIN)
+  return WideToUTF16(path);
+#endif
+}
+
+/*static*/
+std::string PluginInstaller::GetIdentifier(
+    const webkit::WebPluginInfo& plugin) {
+  PluginInstaller* plugin_installer = MatchWithConfiguredPlugins(plugin);
+  if (plugin_installer != NULL) {
+    return plugin_installer->identifier();
+  }
+
+  FilePath path = plugin.path;
+#if defined(OS_POSIX)
+  return path.BaseName().value();
+#elif defined(OS_WIN)
+  return base::SysWideToUTF8(path.BaseName().value());
+#endif
+}
+
+/*static*/
+PluginInstaller* PluginInstaller::MatchWithConfiguredPlugins(
+    const webkit::WebPluginInfo& plugin) {
+  const std::map<std::string, PluginInstaller*>& plugins =
+      PluginFinder::GetInstance()->GetAllPluginInstallers();

[Bernhard Bauer, 2012/08/21 12:30:18]

Ok, this is where stuff gets complicated.

Right now, we bundle the plug-in metadata files as resources which are mapped
into memory, so they can be accessed from any thread. Eventually though, we want
to move them to physical files on disk though that can be updated independently.
That means that we can't load them from disk directly, because that would mean
having to wait on disk I/O, which we are only allowed to do on specific threads.
That is why I started out with an asynchronous interface to PluginFinder (there
is no public method on PluginFinder that synchronously returns the instance).
So, we should probably move this to PluginFinder and make callers use the
asynchronous interface.

[ibraaaa, 2012/08/21 14:26:04]

I want to confirm something, places where I need a list of all plugins are in
PluginInstaller::GetIdentifier and PluginInstaller::GetGroupName, they basically
need to search in a list of all plugins and do the name matching thing before
they return. I may be missing something here, but the asynchronous interface
won't guarantee that all plugins are loaded before GetIdentifier/GetGroupName
return so the result may not be correct. In other words,
GetIdentifier/GetGroupName should not return a result until the asynchronous
call is 100% finished.

I hope we are on the same page, if not, please correct me.

On 2012/08/21 12:30:18, Bernhard Bauer wrote:

[Bernhard Bauer, 2012/08/21 19:14:52]

If by that you mean that we're going to need to make callers use an asynchronous
interface as well, then we're on the same page ;-) Blocking until the call to
GetIdentifier/GetGroupName has finished would defeat the purpose of making that
asynchronous in the first place.

The easiest way to achieve that would be to make these methods synchronous but
move them to PluginFinder (non-static!), so clients would have one asynchronous
call to get the current PluginFinder, and could afterward use a synchronous
interface.

***

Another thing we could do to keep the interface simple would be to have
PluginFinder always return a PluginInstaller. We add one method to PluginFinder
that finds the corresponding PluginInstaller for a given WebPluginInfo
(returning one from the metadata if available, or creating a dummy one
otherwise), then clients could ask that one for its name or identifier. WDYT?

+  for (std::map<std::string, PluginInstaller*>::const_iterator it =
+      plugins.begin(); it != plugins.end(); ++it) {
+    if (plugin.name.find(it->second->group_name_matcher()) == std::string::npos)
+      continue;
+    return it->second;
+  }
+
+  // Not found.
+  return NULL;
+}