Make ActiveScriptController use Active Tab-style permissions

Make ActiveScriptController use Active Tab-style permissions
Also make ActiveScriptController used for all extensions with pseudo-all-hosts (e.g., *://*.com/*).
Add unittests for ActiveScriptController (to complement the browsertests).

BUG=362353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272307

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

most nit-style things.

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/active_script_controller_unittest.cc (right):

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:93: .Build())
don't need the Build()

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:101: // Make sure
the extension is correctly added. If not, return NULL.
how can this possibly fail..? ExtensionRegistry has tests :)

(taking out these lines will simplify a couple of other places)

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:140:
extension_executions_[extension_id] = 1u;
numbers are initialised to 0, so you just need
"++extension_executions_[extension_id]" for the entire function

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:167:
ASSERT_FALSE(action);
holding onto |action| here looks odd. just
ASSERT_FALSE(controller()->GetActionForExtension(extension)) etc.

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:223:
NavigateAndCommit(GURL("https://www.google.com"));
you could also Reload() if that's any easier.

would feel a little more comfortable if you [also] tested a real (cross-domain)
navigation

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:264:
EXPECT_EQ(3u, GetExecutionCountForExtension(extension->id()));
kNumInjections?

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
File extensions/common/permissions/permission_set.h (right):

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permission_set.h:99: // Returns true if this
permission set has access to so many hosts, that we
Can we call this ShouldWarnForAllHosts then? "most" hosts is pretty vague, and
probably not even correct.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permission_set.h:162: mutable scoped_ptr<bool>
has_access_to_most_hosts_;
a pointer for this seems like overkill. could you make it a tri-state enum?

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:514: if
(HasTabSpecificPermission(extension, tab_id, document_url))
and "HasTabSpecificPermission" doesn't imply to me what that permission actually
is. more accurate would be CanExecuteScriptOnPageForTab or similar.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:581: const GURL& document_url)
{
"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".

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:588:
Manifest::IsComponentLocation(extension->location()) ||
the ComponentLocation thing is actually unnecessary,
SholudDisplayInExtensionSettings does it.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.h:97: static bool
HasTabSpecificPermission(const Extension* extension,
this is not something I particularly want to expose on the public interface of
this class, but it looks like it doesn't need to be.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.h:148: static bool
HasAccessToMostHosts(const Extension* extension);
same comment as the other HasAccessToMostHosts

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

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/active_script_controller_unittest.cc (right):

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:93: .Build())
On 2014/05/21 20:10:20, kalman wrote:
> don't need the Build()

Done.

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:101: // Make sure
the extension is correctly added. If not, return NULL.
On 2014/05/21 20:10:20, kalman wrote:
> how can this possibly fail..? ExtensionRegistry has tests :)
> 
> (taking out these lines will simplify a couple of other places)

Fair. :)

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:140:
extension_executions_[extension_id] = 1u;
On 2014/05/21 20:10:20, kalman wrote:
> numbers are initialised to 0, so you just need
> "++extension_executions_[extension_id]" for the entire function

Done.

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:167:
ASSERT_FALSE(action);
On 2014/05/21 20:10:20, kalman wrote:
> holding onto |action| here looks odd. just
> ASSERT_FALSE(controller()->GetActionForExtension(extension)) etc.

Yeah, noticed that after I had uploaded.  Originally did stuff with it, but it
left in a refactor.

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:223:
NavigateAndCommit(GURL("https://www.google.com"));
On 2014/05/21 20:10:20, kalman wrote:
> you could also Reload() if that's any easier.
> 
> would feel a little more comfortable if you [also] tested a real
(cross-domain)
> navigation

Done.

https://codereview.chromium.org/293003008/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_unittest.cc:264:
EXPECT_EQ(3u, GetExecutionCountForExtension(extension->id()));
On 2014/05/21 20:10:20, kalman wrote:
> kNumInjections?

heh, whoops.  Done.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
File extensions/common/permissions/permission_set.h (right):

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permission_set.h:99: // Returns true if this
permission set has access to so many hosts, that we
On 2014/05/21 20:10:20, kalman wrote:
> Can we call this ShouldWarnForAllHosts then? "most" hosts is pretty vague, and
> probably not even correct.

Done.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permission_set.h:162: mutable scoped_ptr<bool>
has_access_to_most_hosts_;
On 2014/05/21 20:10:20, kalman wrote:
> a pointer for this seems like overkill. could you make it a tri-state enum?

Why not.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:514: if
(HasTabSpecificPermission(extension, tab_id, document_url))
On 2014/05/21 20:10:20, kalman wrote:
> and "HasTabSpecificPermission" doesn't imply to me what that permission
actually
> is. more accurate would be CanExecuteScriptOnPageForTab or similar.

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()?

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:581: const GURL& document_url)
{
On 2014/05/21 20:10:20, kalman wrote:
> "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".

Done.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:588:
Manifest::IsComponentLocation(extension->location()) ||
On 2014/05/21 20:10:20, kalman wrote:
> the ComponentLocation thing is actually unnecessary,
> SholudDisplayInExtensionSettings does it.

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.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.h:97: static bool
HasTabSpecificPermission(const Extension* extension,
On 2014/05/21 20:10:20, kalman wrote:
> this is not something I particularly want to expose on the public interface of
> this class, but it looks like it doesn't need to be.

Done.

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.h:148: static bool
HasAccessToMostHosts(const Extension* extension);
On 2014/05/21 20:10:20, kalman wrote:
> same comment as the other HasAccessToMostHosts

Done.

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

lgtm

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:514: if
(HasTabSpecificPermission(extension, tab_id, document_url))
On 2014/05/21 23:16:07, Devlin wrote:
> On 2014/05/21 20:10:20, kalman wrote:
> > and "HasTabSpecificPermission" doesn't imply to me what that permission
> actually
> > is. more accurate would be CanExecuteScriptOnPageForTab or similar.
> 
> 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()?

or HasTabSpecificPermissionToExecuteScript

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

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:588:
Manifest::IsComponentLocation(extension->location()) ||
On 2014/05/21 23:16:07, Devlin wrote:
> On 2014/05/21 20:10:20, kalman wrote:
> > the ComponentLocation thing is actually unnecessary,
> > SholudDisplayInExtensionSettings does it.
> 
> 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.

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.

https://codereview.chromium.org/293003008/diff/20001/extensions/common/permis...
File extensions/common/permissions/permission_set.h (right):

https://codereview.chromium.org/293003008/diff/20001/extensions/common/permis...
extensions/common/permissions/permission_set.h:164: // Lazily set upon first
retrieval, and stored for performance sake.
I think just "Lazily initialized" covers the last 2 sentences.

https://codereview.chromium.org/293003008/diff/20001/extensions/common/permis...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/293003008/diff/20001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:208: static bool
HasTabSpecificScriptPermission(const Extension* extension,
well it seems like it can be in an anonymous namespace within the .cc file.

[Devlin, 2014-05-22 15:52:13]

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/293003008/diff/1/extensions/common/permission...
extensions/common/permissions/permissions_data.cc:514: if
(HasTabSpecificPermission(extension, tab_id, document_url))
On 2014/05/21 23:33:45, kalman wrote:
> On 2014/05/21 23:16:07, Devlin wrote:
> > On 2014/05/21 20:10:20, kalman wrote:
> > > and "HasTabSpecificPermission" doesn't imply to me what that permission
> > actually
> > > is. more accurate would be CanExecuteScriptOnPageForTab or similar.
> > 
> > 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()?
> 
> or HasTabSpecificPermissionToExecuteScript
> 
> times like these you wonder if it's the wrong abstraction.

Done.

https://codereview.chromium.org/293003008/diff/20001/extensions/common/permis...
File extensions/common/permissions/permission_set.h (right):

https://codereview.chromium.org/293003008/diff/20001/extensions/common/permis...
extensions/common/permissions/permission_set.h:164: // Lazily set upon first
retrieval, and stored for performance sake.
On 2014/05/21 23:33:45, kalman wrote:
> I think just "Lazily initialized" covers the last 2 sentences.

Done.

https://codereview.chromium.org/293003008/diff/20001/extensions/common/permis...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/293003008/diff/20001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:208: static bool
HasTabSpecificScriptPermission(const Extension* extension,
On 2014/05/21 23:33:45, kalman wrote:
> well it seems like it can be in an anonymous namespace within the .cc file.

Done.

[Devlin, 2014-05-22 15:52:46]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-05-22 15:54:05]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/293003008/...

[Devlin, 2014-05-22 16:09:43]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2014-05-22 16:33:33]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-05-22 16:35:40]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/293003008/...

[commit-bot: I haz the power, 2014-05-22 20:33:57]

Change committed as 272307