[4] Changing content_settings_api to use PluginFinder's async interface

chrome/browser/extensions/api/content_settings/content_settings_api.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/content_settings/content_settings_api.h"
 
+#include <set>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/content_settings/content_settings_api_constants.h"
 #include "chrome/browser/extensions/api/content_settings/content_settings_helpers.h"
 #include "chrome/browser/extensions/api/content_settings/content_settings_store.h"
 #include "chrome/browser/content_settings/cookie_settings.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/extensions/extension_preference_api_constants.h"
 #include "chrome/browser/extensions/extension_preference_helpers.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/plugin_installer.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/api/content_settings.h"
 #include "chrome/common/extensions/extension_error_utils.h"
 #include "content/public/browser/plugin_service.h"
 #include "webkit/plugins/npapi/plugin_group.h"
 
 using content::BrowserThread;
 using content::PluginService;
 
 namespace Clear = extensions::api::content_settings::ContentSetting::Clear;
 namespace Get = extensions::api::content_settings::ContentSetting::Get;
 namespace Set = extensions::api::content_settings::ContentSetting::Set;
 namespace pref_helpers = extension_preference_helpers;
 namespace pref_keys = extension_preference_api_constants;
 
 namespace {
 
-const std::vector<webkit::npapi::PluginGroup>* g_testing_plugin_groups_;
+const std::vector<webkit::WebPluginInfo>* g_testing_plugins_;
 
 bool RemoveContentType(ListValue* args, ContentSettingsType* content_type) {
   std::string content_type_str;
   if (!args->GetString(0, &content_type_str))
     return false;
   // We remove the ContentSettingsType parameter since this is added by the
   // renderer, and is not part of the JSON schema.
   args->Remove(0, NULL);
   *content_type =
       extensions::content_settings_helpers::StringToContentSettingsType(
           content_type_str);
   return *content_type != CONTENT_SETTINGS_TYPE_DEFAULT;
 }
 
+class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> {

[ibraaaa, 2012/08/30 08:51:52]

How about moving this to
chrome/browser/plugins_pluginfinder_callback_barrier.(h/cc) (in a separate CL to
be finalized first) and reusing it here as well:
http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref...
for EnablePlugin and EnablePluginGroup?

[ibraaaa, 2012/08/30 11:58:51]

we can also update code here to make use of it:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/renderer_ho...
since it used to do the same thing

On 2012/08/30 08:51:52, ibraaaa wrote:

[ibraaaa, 2012/08/31 12:48:49]

Apparently the callback required to get plugins (I mentioned above in
plugin_info_message_filter.cc) is useless because the fetched plugins are never
used after that, isn't it?  

On 2012/08/30 11:58:51, ibraaaa wrote:

[Bernhard Bauer, 2012/08/31 12:55:55]

SGTM! (Both here and in PluginInfoMessageFilter)

We could also add a convenience method somewhere that uses this and calls the
individual methods on PluginFinder and PluginService.

[Bernhard Bauer, 2012/08/31 12:55:55]

The plugins are used in PluginInfoMessageFilter::PluginsLoaded() to find the
first enabled plug-in for the MIME type.

+ public:
+  typedef std::vector<webkit::WebPluginInfo> PluginVector;
+  typedef base::Callback<void(PluginFinder*, const PluginVector&)>
+      CombinedCallback;
+
+  explicit CallbackBarrier(const CombinedCallback& callback)
+      : callback_(callback),
+        finder_(NULL),
+        plugins_(NULL) {

[Bernhard Bauer, 2012/08/31 12:55:55]

The default constructor for a scoped_ptr already initializes it to NULL.

+    DCHECK(!callback_.is_null());
+    if (g_testing_plugins_) {
+      plugins_.reset(new PluginVector(g_testing_plugins_->begin(),
+                                      g_testing_plugins_->end()));
+    }
+  }
+
+  base::Callback<void(PluginFinder*)> CreateGetPluginFinderCallback() {
+    return base::Bind(&CallbackBarrier::SavePluginFinderAndMaybeRunCallback,
+                      this);
+  }
+
+  base::Callback<void(const PluginVector&)> CreateGetPluginVectorCallback() {
+    return base::Bind(&CallbackBarrier::SavePluginVectorAndMaybeRunCallback,
+                      this);
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<CallbackBarrier>;
+
+  ~CallbackBarrier() {
+    DCHECK(callback_.is_null());
+  }
+
+  void SavePluginFinderAndMaybeRunCallback(PluginFinder* finder) {

[Bernhard Bauer, 2012/08/31 12:55:55]

This method name is a bit verbose. Maybe just GotPluginFinder() (and below
GotPlugins())?

+    finder_ = finder;
+    MaybeRunCallback();
+  }
+
+  void SavePluginVectorAndMaybeRunCallback(const PluginVector& plugins) {
+    plugins_.reset(new PluginVector(plugins.begin(), plugins.end()));

[Bernhard Bauer, 2012/08/31 12:55:55]

Could you simply use the copy constructor on vector?

+    MaybeRunCallback();
+  }
+
+  void MaybeRunCallback() {
+    if (!finder_ || !plugins_.get())
+      return;
+    callback_.Run(finder_, *plugins_);
+    callback_.Reset();
+  }
+
+  CombinedCallback callback_;
+  PluginFinder* finder_;
+  scoped_ptr<std::vector<webkit::WebPluginInfo> > plugins_;

[Bernhard Bauer, 2012/08/31 12:55:55]

You typedef'd this as PluginVector above, so you could the typedef here.

+};
+
 }  // namespace
 
 namespace extensions {
 
 namespace helpers = content_settings_helpers;
 namespace keys = content_settings_api_constants;
 
 bool ClearContentSettingsFunction::RunImpl() {
   ContentSettingsType content_type;
   EXTENSION_FUNCTION_VALIDATE(RemoveContentType(args_.get(), &content_type));
@@ skipping 180 matching lines @@
                                     secondary_pattern, content_type,
                                     resource_identifier, setting, scope);
   return true;
 }
 
 bool GetResourceIdentifiersFunction::RunImpl() {
   ContentSettingsType content_type;
   EXTENSION_FUNCTION_VALIDATE(RemoveContentType(args_.get(), &content_type));
 
   if (content_type == CONTENT_SETTINGS_TYPE_PLUGINS) {
-    if (g_testing_plugin_groups_) {
-      OnGotPluginGroups(*g_testing_plugin_groups_);
-    } else {
-      PluginService::GetInstance()->GetPluginGroups(
-          base::Bind(&GetResourceIdentifiersFunction::OnGotPluginGroups, this));
+    scoped_refptr<CallbackBarrier> barrier = new CallbackBarrier(
+        base::Bind(&GetResourceIdentifiersFunction::OnGotPlugins, this));
+
+    PluginFinder::Get(base::Bind(barrier->CreateGetPluginFinderCallback()));
+    if (!g_testing_plugins_) {
+      PluginService::GetInstance()->GetPlugins(

[Bernhard Bauer, 2012/08/31 12:55:55]

You could switch the order of the parameters in OnGotPlugin(). Then, if
|g_testing_plugins_| is not NULL, you could pass in the callback to
OnGotPlugins() with the first parameter bound to |g_testing_plugins_|. That way
you don't need the extra logic in CallbackBarrier to deal with
|g_testing_plugins_|.

+          base::Bind(barrier->CreateGetPluginVectorCallback()));
     }
   } else {
     SendResponse(true);
   }
 
   return true;
 }
 
-void GetResourceIdentifiersFunction::OnGotPluginGroups(
-    const std::vector<webkit::npapi::PluginGroup>& groups) {
+void GetResourceIdentifiersFunction::OnGotPlugins(
+    PluginFinder* finder,
+    const std::vector<webkit::WebPluginInfo>& plugins) {
+  std::set<std::string> group_identifiers;
   ListValue* list = new ListValue();
-  for (std::vector<webkit::npapi::PluginGroup>::const_iterator it =
-          groups.begin();
-       it != groups.end(); ++it) {
+  for (std::vector<webkit::WebPluginInfo>::const_iterator it = plugins.begin();
+       it != plugins.end(); ++it) {
+    PluginInstaller* installer = finder->GetPluginInstaller(*it);
+    const std::string& group_identifier = installer->identifier();
+    if (group_identifiers.find(group_identifier) != group_identifiers.end())
+      continue;
+
+    group_identifiers.insert(group_identifier);
     DictionaryValue* dict = new DictionaryValue();
-    dict->SetString(keys::kIdKey, it->identifier());
-    dict->SetString(keys::kDescriptionKey, it->GetGroupName());
+    dict->SetString(keys::kIdKey, group_identifier);
+    dict->SetString(keys::kDescriptionKey, installer->name());
     list->Append(dict);
   }
   SetResult(list);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE, base::Bind(
           &GetResourceIdentifiersFunction::SendResponse, this, true));
 }
 
 // static
-void GetResourceIdentifiersFunction::SetPluginGroupsForTesting(
-    const std::vector<webkit::npapi::PluginGroup>* plugin_groups) {
-  g_testing_plugin_groups_ = plugin_groups;
+void GetResourceIdentifiersFunction::SetPluginsForTesting(
+    const std::vector<webkit::WebPluginInfo>* plugins) {
+  g_testing_plugins_ = plugins;
 }
 
 }  // namespace extensions