[Extensions] Enable the scripts-require-action feature based on all-urls pref

chrome/browser/extensions/active_script_controller.cc

Index: chrome/browser/extensions/active_script_controller.cc
diff --git a/chrome/browser/extensions/active_script_controller.cc b/chrome/browser/extensions/active_script_controller.cc
index f3b73b933f6c266192b7e870578f072b3ef13e7f..58307b3408dc8c13242248560afd00acf1a329d2 100644
--- a/chrome/browser/extensions/active_script_controller.cc
+++ b/chrome/browser/extensions/active_script_controller.cc
@@ -37,31 +37,10 @@
 
 namespace extensions {
 
-namespace {
-
-// Returns true if the extension should be regarded as a "permitted" extension
-// for the case of metrics. We need this because we only actually withhold
-// permissions if the switch is enabled, but want to record metrics in all
-// cases.
-// "ExtensionWouldHaveHadHostPermissionsWithheldIfSwitchWasOn()" would be
-// more accurate, but too long.
-bool ShouldRecordExtension(const Extension* extension) {
-  return extension->ShouldDisplayInExtensionSettings() &&
-         !Manifest::IsPolicyLocation(extension->location()) &&
-         !Manifest::IsComponentLocation(extension->location()) &&
-         !PermissionsData::CanExecuteScriptEverywhere(extension) &&
-         extension->permissions_data()
-             ->active_permissions()
-             ->ShouldWarnAllHosts();
-}
-
-}  // namespace
-
 ActiveScriptController::ActiveScriptController(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       browser_context_(web_contents->GetBrowserContext()),
-      enabled_(FeatureSwitch::scripts_require_action()->IsEnabled()),
       extension_registry_observer_(this) {
   CHECK(web_contents);
   extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
@@ -143,7 +122,7 @@ void ActiveScriptController::OnClicked(const Extension* extension) {
 }
 
 bool ActiveScriptController::WantsToRun(const Extension* extension) {
-  return enabled_ && pending_requests_.count(extension->id()) > 0;
+  return pending_requests_.count(extension->id()) > 0;
 }
 
 PermissionsData::AccessType
@@ -152,11 +131,6 @@ ActiveScriptController::RequiresUserConsentForScriptInjection(
     UserScript::InjectionType type) {
   CHECK(extension);
 
-  // If the feature is not enabled, we automatically allow all extensions to
-  // run scripts.
-  if (!enabled_)
-    permitted_extensions_.insert(extension->id());
-
   // Allow the extension if it's been explicitly granted permission.
   if (permitted_extensions_.count(extension->id()) > 0)
     return PermissionsData::ACCESS_ALLOWED;
@@ -253,8 +227,9 @@ void ActiveScriptController::OnRequestScriptInjectionPermission(
   // ran (because this feature is not enabled). Add the extension to the list of
   // permitted extensions (for metrics), and return immediately.
   if (request_id == -1) {
-    if (ShouldRecordExtension(extension)) {
-      DCHECK(!enabled_);
+    if (util::ScriptsMayRequireActionForExtension(
+            extension,
+            extension->permissions_data()->active_permissions())) {
       permitted_extensions_.insert(extension->id());
     }
     return;
@@ -314,8 +289,8 @@ void ActiveScriptController::LogUMA() const {
       pending_requests_.size());
 
   // We only log the permitted extensions metric if the feature is enabled,
-  // because otherwise the data will be boring (100% allowed).
-  if (enabled_) {
+  // because otherwise the data will likely be boring (100% allowed).
+  if (extensions::FeatureSwitch::scripts_require_action()->IsEnabled()) {

[not at google - send to devlin, 2015/02/06 00:26:55]

This isn't very useful with this change, since there's no longer a correlation
between flag and whether the feature is enabled.

A useful condition here would be (permitted_extensions_.size() +
pending_request_.size() > 0) i.e. if the logic here was triggered, how often is
something run, and how often is it ignored. You might want to update the
histogram descriptions.

[Devlin, 2015/02/06 18:58:43]

To say there's no longer a correlation between the flag and the feature, I
think, is a little unreasonable.  Overall, the chance that someone will have the
feature enabled on one machine but not another is relatively slim (googlers
aside).  Also, you could read this if as "if the data _will_ be interesting" vs
"if the data might be interesting".  This way, we do log all the known
interesting data, and ignore "likely" uninteresting data.  I think for sampling
purposes, this is okay.

The reason to not just do permitted + pending_request is that if the feature is
off for an extension, we add it to permitted extensions automatically - so that
will almost always be true.

Of course, if you feel strongly, we can make a new set of "tracked extensions"
which is separate from "permitted extensions".  Preferences?

[not at google - send to devlin, 2015/02/06 19:38:43]

I think that ultimately whether or not there's useful data there's still the
question - how much of it is polluted by the flag uncertainty? Furthermore, in
MX when the feature is enabled by default, we'll need to change the metric
anyway. In that case, anybody with M(X-1) will still only be logging with the
flag on - so, never. We might as well change it now.
Heh, yeah I realised this on my way to the BART after work.
It could be a bool. something_interesting_happened_ :-)

[Devlin, 2015/02/06 23:18:49]

Added the "something_interesting_happened" ;)

     UMA_HISTOGRAM_COUNTS_100(
         "Extensions.ActiveScriptController.PermittedExtensions",
         permitted_extensions_.size());