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

chrome/browser/extensions/active_script_controller.cc

 // Copyright 2014 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/active_script_controller.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
@@ skipping 19 matching lines @@
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "ipc/ipc_message_macros.h"
 
 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_));
 }
 
 ActiveScriptController::~ActiveScriptController() {
   LogUMA();
 }
 
 // static
@@ skipping 61 matching lines @@
   // Allow current tab to run injection.
   OnClicked(extension);
 }
 
 void ActiveScriptController::OnClicked(const Extension* extension) {
   DCHECK(ContainsKey(pending_requests_, extension->id()));
   RunPendingForExtension(extension);
 }
 
 bool ActiveScriptController::WantsToRun(const Extension* extension) {
-  return enabled_ && pending_requests_.count(extension->id()) > 0;
+  return pending_requests_.count(extension->id()) > 0;
 }
 
 PermissionsData::AccessType
 ActiveScriptController::RequiresUserConsentForScriptInjection(
     const Extension* extension,
     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;
 
   GURL url = web_contents()->GetVisibleURL();
   int tab_id = SessionTabHelper::IdForTab(web_contents());
   switch (type) {
     case UserScript::CONTENT_SCRIPT:
       return extension->permissions_data()->GetContentScriptAccess(
           extension, url, url, tab_id, -1, NULL);
@@ skipping 76 matching lines @@
           ->enabled_extensions().GetByID(extension_id);
   // We shouldn't allow extensions which are no longer enabled to run any
   // scripts. Ignore the request.
   if (!extension)
     return;
 
   // If the request id is -1, that signals that the content script has already
   // 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;
   }
 
   switch (RequiresUserConsentForScriptInjection(extension, script_type)) {
     case PermissionsData::ACCESS_ALLOWED:
       PermitScriptInjection(request_id);
       break;
     case PermissionsData::ACCESS_WITHHELD:
@@ skipping 34 matching lines @@
   if (extension_action) {
     extension_action_api->NotifyChange(
         extension_action, web_contents(), browser_context_);
   }
 
   // We also notify that page actions may have changed.
   extension_action_api->NotifyPageActionsChanged(web_contents());
 }
 
 void ActiveScriptController::LogUMA() const {
   UMA_HISTOGRAM_COUNTS_100(

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

As discussed, this is rather pointless at this stage.

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

Yeah.  Part of me wonders if it's worth keeping in, since it will be worth
having once more people are dogfooding... WDYT?  Is it worth taking out, and
then putting back in a bit?  Or just leave it in, and be able to see the shift
over time?

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

Mostly the usefulness is covered by the metric below. If you're wanting to track
feature enablement over time we can add a separate non-speculative metric for
that. The extra non-speculative data this gives you is, in the universe, what %
of extensions (?) were shown an embossed icon. Or something. Not sure what you
can determine from that.

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

Fair enough - we'd probably wanna tweak this.  I'll remove it in a separate CL
(just for OWNERS reasons).

       "Extensions.ActiveScriptController.ShownActiveScriptsOnPage",
       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());
     UMA_HISTOGRAM_COUNTS_100(
         "Extensions.ActiveScriptController.DeniedExtensions",
         pending_requests_.size());
   }
 }
 
 bool ActiveScriptController::OnMessageReceived(const IPC::Message& message) {
@@ skipping 23 matching lines @@
     UnloadedExtensionInfo::Reason reason) {
   PendingRequestMap::iterator iter = pending_requests_.find(extension->id());
   if (iter != pending_requests_.end()) {
     pending_requests_.erase(iter);
     ExtensionActionAPI::Get(browser_context_)->
         NotifyPageActionsChanged(web_contents());
   }
 }
 
 }  // namespace extensions