Migrate enabled state of plugins into specialized Flash and PDF prefs.

chrome/browser/plugins/plugin_prefs.cc

Index: chrome/browser/plugins/plugin_prefs.cc
diff --git a/chrome/browser/plugins/plugin_prefs.cc b/chrome/browser/plugins/plugin_prefs.cc
index 85862139b88122b5dc6937ba9bc975cfb8d69c6a..375567701c9694079714432aafea9c9252c86a9b 100644
--- a/chrome/browser/plugins/plugin_prefs.cc
+++ b/chrome/browser/plugins/plugin_prefs.cc
@@ -13,6 +13,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/pattern.h"
@@ -23,6 +24,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/plugins/plugin_installer.h"
 #include "chrome/browser/plugins/plugin_metadata.h"
 #include "chrome/browser/plugins/plugin_prefs_factory.h"
@@ -32,11 +34,15 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/content_settings/core/common/content_settings.h"
+#include "components/content_settings/core/common/pref_names.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/plugin_service.h"
+#include "content/public/common/content_constants.h"
 #include "content/public/common/webplugininfo.h"
 
 #if !defined(DISABLE_NACL)
@@ -65,6 +71,11 @@ bool IsPDFViewerPlugin(const base::string16& plugin_name) {
   return plugin_name == base::ASCIIToUTF16(ChromeContentClient::kPDFPluginName);
 }
 
+bool IsAdobeFlashPlayerPlugin(const base::string16& plugin_name) {
+  return (plugin_name == base::ASCIIToUTF16(content::kFlashPluginName) ||
+          plugin_name == base::ASCIIToUTF16("Adobe Flash Player"));

[Bernhard Bauer, 2016/11/11 12:15:24]

Would it make sense to move this to a constant in chrome/ rather than
duplicating it over and over?

[pastarmovj, 2016/11/14 13:58:00]

Done.

+}
+
 }  // namespace
 
 PluginPrefs::PluginState::PluginState() {
@@ -339,6 +350,7 @@ void PluginPrefs::ListValueToStringSet(const base::ListValue* src,
 void PluginPrefs::SetPrefs(PrefService* prefs) {
   prefs_ = prefs;
   bool update_internal_dir = false;
+  bool plugins_migrated = false;
   base::FilePath last_internal_dir =
       prefs_->GetFilePath(prefs::kPluginsLastInternalDirectory);
   base::FilePath cur_internal_dir;
@@ -360,11 +372,36 @@ void PluginPrefs::SetPrefs(PrefService* prefs) {
           continue;  // Oops, don't know what to do with this item.
         }
 
-        base::string16 group_name;
+        base::string16 name;
+        if (!plugin->GetString("name", &name)) {
+          // This should not happen in the post NPAPI world. I remember that my
+          // initial assumption was the same about NPAPI plugins back then and
+          // and then was bitterly surprised when my change hit the canary and
+          // not handling well the empty names became a top crasher. Not so bad
+          // this time but we'd rather not CHECK on that.

[Bernhard Bauer, 2016/11/11 12:15:24]

Couple of things:
1) This explanation seems a bit too detailed (and really, subjective), in
particular given that it's not clear who "I" is.
2) The code doesn't actually do anything apart from logging, so I don't see how
it would prevent a CHECK.
3) If you say "X should not happen", don't add a check for X. In this particular
case, we either make it a precondition that the name is non-empty, in which case
we DCHECK it, and accept crashes in case that turns out to be untrue (which is
fine, because that's actually the only way we'll learn about it!), or we
outright accept any name, but then we shouldn't spam the log about it either.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
has some good guidance there.

[pastarmovj, 2016/11/14 13:58:00]

 Restructued this code. It doesn't matter anymore just like it didn't before.

+          LOG(WARNING) << "Nameless plugin detected in the post-NPAPI world?!";
+        }
+
         bool enabled;
         if (!plugin->GetBoolean("enabled", &enabled))
           enabled = true;
 
+        // Migrate disabled plugins and re-enabled them all internally.
+        // TODO(pastarmovj): crbug/662006: Remove migration eventually.

[Bernhard Bauer, 2016/11/11 12:15:24]

Nit: For ideal clickability, use a full URL: https://crbug/com/662006.

[pastarmovj, 2016/11/14 13:58:00]

Was trying to save a line :) changed.

[Bernhard Bauer, 2016/11/14 14:35:38]

Gotcha. FWIW, TODO(https://crbug.com/662006) is also acceptable (even prefered
by some people) and would also be shorter :)

+        if (!enabled) {
+          if (IsPDFViewerPlugin(name))
+            prefs->SetBoolean(prefs::kPluginsAlwaysOpenPdfExternally, true);
+          if (IsAdobeFlashPlayerPlugin(name)) {
+            HostContentSettingsMapFactory::GetForProfile(profile_)
+                ->SetDefaultContentSetting(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                           CONTENT_SETTING_BLOCK);
+          }
+          // TODO(pastarmovj): crbug/662002: Remove the enabled flag completely.
+          enabled = true;
+          plugin->SetBoolean("enabled", true);
+          plugins_migrated = true;

[Bernhard Bauer, 2016/11/11 12:15:24]

So, this would set the flag even if the migrated plugin is neither the PDF
viewer nor Flash. Is that really what we want?

[pastarmovj, 2016/11/14 13:58:00]

Yes. I want to know that any plugin was user-disabled (which is going to be
undone so once per-user, provided that the about:plugins page is gone). This way
we can measure when we have reset [almost]all visible in uma instances.

+        }
+
         base::FilePath::StringType path;
         // The plugin list constains all the plugin files in addition to the
         // plugin groups.
@@ -412,9 +449,6 @@ void PluginPrefs::SetPrefs(PrefService* prefs) {
           }
 
           plugin_state_.Set(plugin_path, enabled);

[Bernhard Bauer, 2016/11/11 12:15:24]

So, if we enable all plugins anyway, why do we even need this state?

[pastarmovj, 2016/11/14 13:58:00]

Not anymore but untangling this is going to make this CL too complex. This is
the plan for http://crbug.com/662002 anyways.

-        } else if (!enabled && plugin->GetString("name", &group_name)) {
-          // Otherwise this is a list of groups.
-          plugin_group_state_[group_name] = false;
         }
       }
     } else {
@@ -429,8 +463,11 @@ void PluginPrefs::SetPrefs(PrefService* prefs) {
     }
   }  // Scoped update of prefs::kPluginsPluginsList.
 
+  UMA_HISTOGRAM_BOOLEAN("Plugin.EnabledStatusMigrationDone", plugins_migrated);
+
   // Build the set of policy enabled/disabled plugin patterns once and cache it.
   // Don't do this in the constructor, there's no profile available there.
+  // TODO(pastarmovj): crbug/662002: Remove the prefs completely.
   ListValueToStringSet(prefs_->GetList(prefs::kPluginsDisabledPlugins),
                        &policy_disabled_plugin_patterns_);
   ListValueToStringSet(