chrome://plugins Mojo-ification part 2/2, populating the page.

chrome/browser/ui/webui/plugins/plugins_handler.cc

Index: chrome/browser/ui/webui/plugins/plugins_handler.cc
diff --git a/chrome/browser/ui/webui/plugins/plugins_handler.cc b/chrome/browser/ui/webui/plugins/plugins_handler.cc
index a3f216532545675903fe8e508992294f4585e036..6eeba225bbb29ab3e220ac83ea6cf52f95b503c2 100644
--- a/chrome/browser/ui/webui/plugins/plugins_handler.cc
+++ b/chrome/browser/ui/webui/plugins/plugins_handler.cc
@@ -29,6 +29,7 @@
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/common/content_constants.h"
+#include "mojo/common/common_type_converters.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::WebPluginInfo;
@@ -88,148 +89,85 @@ base::string16 GetPluginDescription(const WebPluginInfo& plugin) {
   return desc;
 }
 
-scoped_ptr<base::ListValue> GetPluginMimeTypes(const WebPluginInfo& plugin) {
-  scoped_ptr<base::ListValue> mime_types(new base::ListValue());
+mojo::Array<MimeTypePtr> GeneratePluginMimeTypes(const WebPluginInfo& plugin) {
+  mojo::Array<MimeTypePtr> mime_types(0u);

[Dan Beam, 2016/01/26 20:10:43]

shouldn't this be initialized with plugin.mime_types.size()?

[dpapad, 2016/01/26 22:34:48]

Does not have to. There are two ways to populate a mojo::Array
1) Initialize size to 0u and use push_back(), which grows the array as
necessary.
2) Initialize size to a non-zero value and use my_array[index] = 'foo' (instead
of push_back).

I chose approach number 1, because it allows me to not manually keep track of
'index' within the modern-style 'for' loop.

   for (const auto& plugin_mime_type: plugin.mime_types) {
-    base::DictionaryValue* mime_type = new base::DictionaryValue();
-    mime_type->SetString("mimeType", plugin_mime_type.mime_type);
-    mime_type->SetString("description", plugin_mime_type.description);
+    MimeTypePtr mime_type(MimeType::New());
+    mime_type->description = mojo::String::From(plugin_mime_type.description);
 
-    base::ListValue* file_extensions = new base::ListValue();
-    for (const auto& mime_file_extension : plugin_mime_type.file_extensions) {
-      file_extensions->Append(new base::StringValue(mime_file_extension));
-    }
-    mime_type->Set("fileExtensions", file_extensions);
-    mime_types->Append(mime_type);
+    mime_type->mime_type = mojo::String::From(plugin_mime_type.mime_type);
+    mime_type->file_extensions = mojo::Array<mojo::String>::From(
+        plugin_mime_type.file_extensions);
+    mime_types.push_back(std::move(mime_type));
   }
+
   return mime_types;
 }
 
 }  // namespace
 
 
-PluginsHandler::PluginsHandler() : weak_ptr_factory_(this) {
-}
-
-PluginsHandler::~PluginsHandler() {
-}
-
-void PluginsHandler::RegisterMessages() {
-  Profile* profile = Profile::FromWebUI(web_ui());
-
+PluginsHandler::PluginsHandler(
+    content::WebUI* web_ui,
+    mojo::InterfaceRequest<PluginsHandlerMojo> request)
+    : web_ui_(web_ui), binding_(this, std::move(request)),
+      weak_ptr_factory_(this) {

[Dan Beam, 2016/01/26 20:10:43]

i think the style-guide (or previous reviewers of mine?) have preferred
multi-line initializers be 1-per-line, tl;dr - 

  web_ui_(web_ui),
  binding_(this, std::move(request)),
  weak_ptr_factory_(this) {

but i honestly don't remember that this is a hard rule

[dpapad, 2016/01/26 22:34:48]

Changed (along with other style fixes, just run clang-format on this file).

+  Profile* profile = Profile::FromWebUI(web_ui_);
   PrefService* prefs = profile->GetPrefs();
   show_details_.Init(prefs::kPluginsShowDetails, prefs);
 
   registrar_.Add(this,
                  chrome::NOTIFICATION_PLUGIN_ENABLE_STATUS_CHANGED,
                  content::Source<Profile>(profile));
-
-  web_ui()->RegisterMessageCallback("requestPluginsData",
-      base::Bind(&PluginsHandler::HandleRequestPluginsData,
-                 base::Unretained(this)));
-  web_ui()->RegisterMessageCallback("enablePlugin",
-      base::Bind(&PluginsHandler::HandleEnablePluginMessage,
-                 base::Unretained(this)));
-  web_ui()->RegisterMessageCallback("setPluginAlwaysAllowed",
-      base::Bind(&PluginsHandler::HandleSetPluginAlwaysAllowed,
-                 base::Unretained(this)));
-  web_ui()->RegisterMessageCallback("saveShowDetailsToPrefs",
-      base::Bind(&PluginsHandler::HandleSaveShowDetailsToPrefs,
-                 base::Unretained(this)));
-  web_ui()->RegisterMessageCallback("getShowDetails",
-      base::Bind(&PluginsHandler::HandleGetShowDetails,
-                 base::Unretained(this)));
 }
 
-void PluginsHandler::HandleRequestPluginsData(const base::ListValue* args) {
-  LoadPlugins();
+PluginsHandler::~PluginsHandler() {
 }
 
-void PluginsHandler::HandleEnablePluginMessage(const base::ListValue* args) {
-  Profile* profile = Profile::FromWebUI(web_ui());
-
-  // Be robust in accepting badness since plugins display HTML (hence
-  // JavaScript).
-  if (args->GetSize() != 3) {
-    NOTREACHED();
-    return;
-  }
-
-  std::string enable_str;
-  std::string is_group_str;
-  if (!args->GetString(1, &enable_str) || !args->GetString(2, &is_group_str)) {
-    NOTREACHED();
-    return;
-  }
-  bool enable = enable_str == "true";
-
+void PluginsHandler::SetPluginEnabled(
+    const mojo::String& plugin_path, bool enable) {
+  Profile* profile = Profile::FromWebUI(web_ui_);
   PluginPrefs* plugin_prefs = PluginPrefs::GetForProfile(profile).get();
-  if (is_group_str == "true") {
-    base::string16 group_name;
-    if (!args->GetString(0, &group_name)) {
-      NOTREACHED();
-      return;
-    }
-
-    plugin_prefs->EnablePluginGroup(enable, group_name);
-    if (enable) {
-      // See http://crbug.com/50105 for background.
-      base::string16 adobereader = base::ASCIIToUTF16(
-          PluginMetadata::kAdobeReaderGroupName);
-      base::string16 internalpdf =
-          base::ASCIIToUTF16(ChromeContentClient::kPDFPluginName);
-      if (group_name == adobereader)
-        plugin_prefs->EnablePluginGroup(false, internalpdf);
-      else if (group_name == internalpdf)
-        plugin_prefs->EnablePluginGroup(false, adobereader);
-    }
-  } else {
-    base::FilePath::StringType file_path;
-    if (!args->GetString(0, &file_path)) {
-      NOTREACHED();
-      return;
-    }
-
-    plugin_prefs->EnablePlugin(enable, base::FilePath(file_path),
-                               base::Bind(&AssertPluginEnabled));
-  }
+  plugin_prefs->EnablePlugin(
+      enable, base::FilePath(plugin_path.To<base::FilePath::StringType>()),
+      base::Bind(&AssertPluginEnabled));
 }
 
-void PluginsHandler::HandleSaveShowDetailsToPrefs(
-    const base::ListValue* args) {
-  std::string details_mode;
-  if (!args->GetString(0, &details_mode)) {
-    NOTREACHED();
-    return;
+void PluginsHandler::SetPluginGroupEnabled(
+    const mojo::String& group_name, bool enable) {
+  Profile* profile = Profile::FromWebUI(web_ui_);
+  PluginPrefs* plugin_prefs = PluginPrefs::GetForProfile(profile).get();
+  base::string16 group_name_as_string16 = group_name.To<base::string16>();
+  plugin_prefs->EnablePluginGroup(enable, group_name_as_string16);
+  if (enable) {
+    // See http://crbug.com/50105 for background.
+    base::string16 adobereader = base::ASCIIToUTF16(
+        PluginMetadata::kAdobeReaderGroupName);
+    base::string16 internalpdf =
+        base::ASCIIToUTF16(ChromeContentClient::kPDFPluginName);
+    if (group_name_as_string16 == adobereader)
+      plugin_prefs->EnablePluginGroup(false, internalpdf);
+    else if (group_name_as_string16 == internalpdf)
+      plugin_prefs->EnablePluginGroup(false, adobereader);
   }
-  show_details_.SetValue(details_mode == "true");
 }
 
-void PluginsHandler::HandleGetShowDetails(const base::ListValue* args) {
-  base::FundamentalValue show_details(show_details_.GetValue());
-  web_ui()->CallJavascriptFunction("loadShowDetailsFromPrefs", show_details);
+void PluginsHandler::GetShowDetails(const GetShowDetailsCallback& callback) {
+  callback.Run(show_details_.GetValue());
 }
 
-void PluginsHandler::HandleSetPluginAlwaysAllowed(
-    const base::ListValue* args) {
-  // Be robust in the input parameters, but crash in a Debug build.
-  if (args->GetSize() != 2) {
-    NOTREACHED();
-    return;
-  }
+void PluginsHandler::SaveShowDetailsToPrefs(bool details_mode) {
+  show_details_.SetValue(details_mode);
+}
 
-  std::string plugin;
-  bool allowed = false;
-  if (!args->GetString(0, &plugin) || !args->GetBoolean(1, &allowed)) {
-    NOTREACHED();
-    return;
-  }
-  Profile* profile = Profile::FromWebUI(web_ui());
+void PluginsHandler::SetPluginAlwaysAllowed(
+    const mojo::String& plugin, bool allowed) {
+  Profile* profile = Profile::FromWebUI(web_ui_);
   HostContentSettingsMapFactory::GetForProfile(profile)->SetContentSetting(
       ContentSettingsPattern::Wildcard(),
       ContentSettingsPattern::Wildcard(),
       CONTENT_SETTINGS_TYPE_PLUGINS,
-      plugin,
+      plugin.get(),
       allowed ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_DEFAULT);
 
   // Keep track of the whitelist separately, so that we can distinguish plugins
@@ -239,28 +177,48 @@ void PluginsHandler::HandleSetPluginAlwaysAllowed(
   update->SetBoolean(plugin, allowed);
 }
 
+void PluginsHandler::GetPluginsData(const GetPluginsDataCallback& callback) {
+  if (weak_ptr_factory_.HasWeakPtrs())

[Dan Beam, 2016/01/26 20:10:43]

what is this supposed to be doing?  seeing if there are any other things in
flight?

+    return;
+
+  content::PluginService::GetInstance()->GetPlugins(
+      base::Bind(&PluginsHandler::RespondWithPluginsData,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback));
+}
+
+void PluginsHandler::SetClientPage(PluginsPageMojoPtr page) {
+  page_ = std::move(page);
+}
+
 void PluginsHandler::Observe(int type,
                              const content::NotificationSource& source,
                              const content::NotificationDetails& details) {
   DCHECK_EQ(chrome::NOTIFICATION_PLUGIN_ENABLE_STATUS_CHANGED, type);
-  LoadPlugins();
-}
 
-void PluginsHandler::LoadPlugins() {
   if (weak_ptr_factory_.HasWeakPtrs())
     return;
 
   content::PluginService::GetInstance()->GetPlugins(
-      base::Bind(&PluginsHandler::PluginsLoaded,
+      base::Bind(&PluginsHandler::NotifyWithPluginsData,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PluginsHandler::PluginsLoaded(
+void PluginsHandler::RespondWithPluginsData(
+    const GetPluginsDataCallback& callback,
     const std::vector<WebPluginInfo>& plugins) {
-  Profile* profile = Profile::FromWebUI(web_ui());
-  PluginPrefs* plugin_prefs = PluginPrefs::GetForProfile(profile).get();
+  callback.Run(GeneratePluginsData(plugins));
+}
 
-  ContentSettingsPattern wildcard = ContentSettingsPattern::Wildcard();
+void PluginsHandler::NotifyWithPluginsData(
+    const std::vector<WebPluginInfo>& plugins) {
+  page_->OnPluginsUpdated(GeneratePluginsData(plugins));
+}
+
+mojo::Array<PluginDataPtr> PluginsHandler::GeneratePluginsData(
+    const std::vector<WebPluginInfo>& plugins) {
+  Profile* profile = Profile::FromWebUI(web_ui_);
+  PluginPrefs* plugin_prefs = PluginPrefs::GetForProfile(profile).get();
 
   PluginFinder* plugin_finder = PluginFinder::GetInstance();
   // Group plugins by identifier. This is done to be able to display
@@ -272,78 +230,88 @@ void PluginsHandler::PluginsLoaded(
     groups[plugin->identifier()].push_back(&plugins[i]);
   }
 
-  // Construct DictionaryValues to return to UI.
-  base::ListValue* plugin_groups_data = new base::ListValue();
+  mojo::Array<PluginDataPtr> plugins_data(0u);

[Dan Beam, 2016/01/26 20:10:43]

init with groups.size()?

[dpapad, 2016/01/26 22:34:48]

See previous response to similar question.

+
   for (PluginGroups::const_iterator it = groups.begin();
       it != groups.end(); ++it) {
+    PluginDataPtr plugin_data(PluginData::New());
     const std::vector<const WebPluginInfo*>& group_plugins = it->second;
-    base::ListValue* plugin_files = new base::ListValue();
+
     scoped_ptr<PluginMetadata> plugin_metadata(
         plugin_finder->GetPluginMetadata(*group_plugins[0]));
-    base::string16 group_name = plugin_metadata->name();
     std::string group_identifier = plugin_metadata->identifier();
+    plugin_data->id = mojo::String::From(group_identifier);
+
+    const WebPluginInfo* active_plugin = nullptr;
     bool group_enabled = false;
-    const WebPluginInfo* active_plugin = NULL;
-    for (size_t j = 0; j < group_plugins.size(); ++j) {
-      const WebPluginInfo& group_plugin = *group_plugins[j];
-
-      base::DictionaryValue* plugin_file = new base::DictionaryValue();
-      plugin_file->SetString("name", group_plugin.name);
-      plugin_file->SetString("description", GetPluginDescription(group_plugin));
-      plugin_file->SetString("path", group_plugin.path.value());
-      plugin_file->SetString("version", group_plugin.version);
-      plugin_file->SetString("type", PluginTypeToString(group_plugin.type));
-      plugin_file->Set("mimeTypes", GetPluginMimeTypes(group_plugin));
-
-      bool plugin_enabled = plugin_prefs->IsPluginEnabled(group_plugin);
-      plugin_file->SetString(
-          "enabledMode",
-          GetPluginEnabledMode(group_plugin.name, group_name, plugin_enabled));
-      plugin_files->Append(plugin_file);
 
+    mojo::Array<PluginFilePtr> plugin_files(0u);

[Dan Beam, 2016/01/26 20:10:43]

shouldn't this be initialized with group_plugins.size()?

[dpapad, 2016/01/26 22:34:48]

See previous response to similar question.

+    for (const auto& group_plugin: group_plugins) {
+      bool plugin_enabled = plugin_prefs->IsPluginEnabled(*group_plugin);
+
+      plugin_files.push_back(GeneratePluginFile(
+          *group_plugin, plugin_metadata->name(), plugin_enabled));
+
+      // Update |active_plugin| and |group_enabled|.
       if (!active_plugin || (plugin_enabled && !group_enabled))
-        active_plugin = &group_plugin;
+        active_plugin = group_plugin;
       group_enabled = plugin_enabled || group_enabled;
     }
 
-    base::DictionaryValue* group_data = new base::DictionaryValue();
-    group_data->Set("plugin_files", plugin_files);
-    group_data->SetString("name", group_name);
-    group_data->SetString("id", group_identifier);
-    group_data->SetString("description", active_plugin->desc);
-    group_data->SetString("version", active_plugin->version);
+    plugin_data->enabled_mode = mojo::String::From(
+        GetPluginGroupEnabledMode(plugin_files, group_enabled));
 
-#if defined(ENABLE_PLUGIN_INSTALLATION)
-    bool out_of_date = plugin_metadata->GetSecurityStatus(*active_plugin) ==
-        PluginMetadata::SECURITY_STATUS_OUT_OF_DATE;
-    group_data->SetBoolean("critical", out_of_date);
-    group_data->SetString("update_url", plugin_metadata->plugin_url().spec());
-#endif
-
-    group_data->SetString(
-        "enabledMode", GetPluginGroupEnabledMode(*plugin_files, group_enabled));
-
-    bool always_allowed = false;
+    plugin_data->always_allowed = false;
     if (group_enabled) {
       const base::DictionaryValue* whitelist =
           profile->GetPrefs()->GetDictionary(
               prefs::kContentSettingsPluginWhitelist);
-      whitelist->GetBoolean(group_identifier, &always_allowed);
+      whitelist->GetBoolean(group_identifier, &plugin_data->always_allowed);
     }
-    group_data->SetBoolean("alwaysAllowed", always_allowed);
 
-    plugin_groups_data->Append(group_data);
+    plugin_data->critical = false;
+    plugin_data->update_url = "";
+#if defined(ENABLE_PLUGIN_INSTALLATION)
+    bool out_of_date = plugin_metadata->GetSecurityStatus(*active_plugin) ==
+        PluginMetadata::SECURITY_STATUS_OUT_OF_DATE;
+    plugin_data->critical = out_of_date;
+    plugin_data->update_url = plugin_metadata->plugin_url().spec();
+#endif
+
+    plugin_data->description = mojo::String::From(active_plugin->desc);
+    plugin_data->name = plugin_files[0]->name;
+    plugin_data->plugin_files = std::move(plugin_files);
+    plugin_data->version = mojo::String::From(active_plugin->version);
+    plugins_data.push_back(std::move(plugin_data));
   }
-  base::DictionaryValue results;
-  results.Set("plugins", plugin_groups_data);
-  web_ui()->CallJavascriptFunction("returnPluginsData", results);
+
+  return plugins_data;
+}
+
+PluginFilePtr PluginsHandler::GeneratePluginFile(
+    const WebPluginInfo& plugin,
+    const base::string16& group_name,
+    bool plugin_enabled) const {
+  PluginFilePtr plugin_file(PluginFile::New());
+  plugin_file->description = mojo::String::From(
+      GetPluginDescription(plugin));
+  plugin_file->enabled_mode = mojo::String::From(
+      GetPluginEnabledMode(plugin.name, group_name, plugin_enabled));
+  plugin_file->name = mojo::String::From(plugin.name);
+  plugin_file->path = mojo::String::From(plugin.path.value());
+  plugin_file->type = mojo::String::From(
+      PluginTypeToString(plugin.type));
+  plugin_file->version = mojo::String::From(plugin.version);
+  plugin_file->mime_types = GeneratePluginMimeTypes(plugin);
+
+  return plugin_file;
 }
 
 std::string PluginsHandler::GetPluginEnabledMode(
     const base::string16& plugin_name,
     const base::string16& group_name,
     bool plugin_enabled) const {
-  Profile* profile = Profile::FromWebUI(web_ui());
+  Profile* profile = Profile::FromWebUI(web_ui_);
   PluginPrefs* plugin_prefs = PluginPrefs::GetForProfile(profile).get();
   PluginPrefs::PolicyStatus plugin_status =
       plugin_prefs->PolicyStatusForPlugin(plugin_name);
@@ -362,17 +330,14 @@ std::string PluginsHandler::GetPluginEnabledMode(
 }
 
 std::string PluginsHandler::GetPluginGroupEnabledMode(
-    const base::ListValue& plugin_files, bool group_enabled) const {
+    const mojo::Array<PluginFilePtr>& plugin_files,
+    bool group_enabled) const {
   bool plugins_enabled_by_policy = true;
   bool plugins_disabled_by_policy = true;
   bool plugins_managed_by_policy = true;
 
-  for (base::ListValue::const_iterator it = plugin_files.begin();
-       it != plugin_files.end(); ++it) {
-    base::DictionaryValue* plugin_dict;
-    CHECK((*it)->GetAsDictionary(&plugin_dict));
-    std::string plugin_enabled_mode;
-    CHECK(plugin_dict->GetString("enabledMode", &plugin_enabled_mode));
+  for (size_t i = 0; i < plugin_files.size(); i++) {
+    std::string plugin_enabled_mode = plugin_files[i]->enabled_mode;
 
     plugins_enabled_by_policy = plugins_enabled_by_policy &&
         plugin_enabled_mode == "enabledByPolicy";