Make ActiveScriptController use Active Tab-style permissions

extensions/common/permissions/permissions_data.cc

Index: extensions/common/permissions/permissions_data.cc
diff --git a/extensions/common/permissions/permissions_data.cc b/extensions/common/permissions/permissions_data.cc
index 65359d9950954bd8496fa38b5d6b1dfff2da1c5f..5a39036ad1c8994e6c901ff11acd8e8aacbedd14 100644
--- a/extensions/common/permissions/permissions_data.cc
+++ b/extensions/common/permissions/permissions_data.cc
@@ -342,6 +342,21 @@ void PermissionsData::ClearTabSpecificPermissions(
 }
 
 // static
+bool PermissionsData::HasTabSpecificPermission(const Extension* extension,
+                                               int tab_id,
+                                               const GURL& url) {
+  if (tab_id >= 0) {
+    scoped_refptr<const PermissionSet> tab_permissions =
+        GetTabSpecificPermissions(extension, tab_id);
+    if (tab_permissions.get() &&
+        tab_permissions->explicit_hosts().MatchesSecurityOrigin(url)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+// static
 bool PermissionsData::HasAPIPermission(const Extension* extension,
                                        APIPermission::ID permission) {
   base::AutoLock auto_lock(extension->permissions_data()->runtime_lock_);
@@ -414,6 +429,12 @@ bool PermissionsData::HasEffectiveAccessToAllHosts(const Extension* extension) {
 }
 
 // static
+bool PermissionsData::HasAccessToMostHosts(const Extension* extension) {
+  base::AutoLock auto_lock(extension->permissions_data()->runtime_lock_);
+  return GetActivePermissions(extension)->HasAccessToMostHosts();
+}
+
+// static
 PermissionMessages PermissionsData::GetPermissionMessages(
     const Extension* extension) {
   base::AutoLock auto_lock(extension->permissions_data()->runtime_lock_);
@@ -490,15 +511,8 @@ bool PermissionsData::CanExecuteScriptOnPage(const Extension* extension,
     return false;
   }
 
-  // If a tab ID is specified, try the tab-specific permissions.
-  if (tab_id >= 0) {
-    scoped_refptr<const PermissionSet> tab_permissions =
-        GetTabSpecificPermissions(extension, tab_id);
-    if (tab_permissions.get() &&
-        tab_permissions->explicit_hosts().MatchesSecurityOrigin(document_url)) {
-      return true;
-    }
-  }
+  if (HasTabSpecificPermission(extension, tab_id, document_url))

[not at google - send to devlin, 2014/05/21 20:10:20]

and "HasTabSpecificPermission" doesn't imply to me what that permission actually
is. more accurate would be CanExecuteScriptOnPageForTab or similar.

[Devlin, 2014/05/21 23:16:07]

My worry is making it clear that this is only asking if there are active tab
permissions for that page/tab, not whether or not it could generically execute
(there are times when this would return false, but it can execute script).  How
about... HasTabSpecificScriptPermission()?  Or, if it needs the extra word,
HasTabSpecificScriptExecutionPermission()?

[not at google - send to devlin, 2014/05/21 23:33:45]

or HasTabSpecificPermissionToExecuteScript

times like these you wonder if it's the wrong abstraction.

[Devlin, 2014/05/22 15:52:14]

Done.

+    return true;
 
   bool can_access = false;
 
@@ -562,14 +576,26 @@ bool PermissionsData::CanCaptureVisiblePage(const Extension* extension,
 
 // static
 bool PermissionsData::RequiresActionForScriptExecution(
-    const Extension* extension) {
+    const Extension* extension,
+    int tab_id,
+    const GURL& document_url) {

[not at google - send to devlin, 2014/05/21 20:10:20]

"document URL" isn't right, it's actually top level URL that this function cares
about.

but I'm not sure it should really care either way, so perhaps "url".

[Devlin, 2014/05/21 23:16:07]

Done.

   // For now, the user should be notified when an extension with all hosts
-  // permission tries to execute a script on a page. Exceptions for policy-
-  // enabled and component extensions.
-  return extension->ShouldDisplayInExtensionSettings() &&
-         !Manifest::IsPolicyLocation(extension->location()) &&
-         !Manifest::IsComponentLocation(extension->location()) &&
-         HasEffectiveAccessToAllHosts(extension);
+  // permission tries to execute a script on a page, with exceptions for policy-
+  // enabled and component extensions. If this doesn't meet those criteria,
+  // return immediately.
+  if (!extension->ShouldDisplayInExtensionSettings() ||
+      Manifest::IsPolicyLocation(extension->location()) ||
+      Manifest::IsComponentLocation(extension->location()) ||

[not at google - send to devlin, 2014/05/21 20:10:20]

the ComponentLocation thing is actually unnecessary,
SholudDisplayInExtensionSettings does it.

[Devlin, 2014/05/21 23:16:07]

Yes, but that's using internal logic from ShouldDisplayInExtensionSettings(),
and I think they're two different reasons to allow.  What if we someday show
component extensions in chrome:extensions?  (I think there's already discussion
for this as-is).  I think I'd rather keep the condition, given it's all of a one
byte comparison.  But if you feel strongly, I can take it out.

[not at google - send to devlin, 2014/05/21 23:33:45]

Leaving in is fine.

I think we're going to want 2 separate functions here eventually, one which
takes the tab ID / document URL and one which is just the old version, to know
when to show the [*] all URLs checkbox.

+      !HasAccessToMostHosts(extension)) {
+    return false;
+  }
+
+  // If the extension has explicit permission to run on the given tab, then
+  // we don't need to alert the user.
+  if (HasTabSpecificPermission(extension, tab_id, document_url))
+    return false;
+
+  return true;
 }
 
 bool PermissionsData::ParsePermissions(Extension* extension,