Introduce ActiveScriptController; track active extension scripts

Introduce ActiveScriptController, track active extension scripts.

ActiveScriptController tracks all running active scripts (content scripts and
scripts injected via tabs.executeScript), and, if the flag is enabled, ensures
an ExtensionAction is displayed in the location bar.

Next step: Require permission before executing scripts.

TBR=sky@chromium.org (mechanical, no-impact changes to c/b/ui)

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

[not at google - send to devlin, 2014-05-07 22:49:02]

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:36:
enabled_(FeatureSwitch::active_script_enforcement()->IsEnabled()) {
if it's not enabled then just don't create it. this is made possible with the
CompositeController -- if the flag is off, just don't insert an
ActiveScriptController into that list.

(you also probably wouldn't bother with the Composite version at all, since it'd
only have the single one)

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:45: if
(!extensions_executing_scripts_.count(extension_id)) {
nit: early return.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:57: bool
ActiveScriptController::HasActionForExtensionId(
eventually we'll want to separate the metrics for an icon showing but not
injected vs an icon was shown and injected.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:91: ExtensionAction*
ActiveScriptController::GetActionForExtension(
nit: GetOrCreateExtensionAction

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:104: const ActionInfo*
action_info = ActionInfo::GetPageActionInfo(extension);
see above

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:111:
badge->set_default_icon(icon_set.Pass());
I swear we already have code somewhere to do this

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/uma_policy.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.cc:48: const int
kAnyAdInjection = kAdInjected | kAdRemoved | kAdReplaced;
how about kAnyAdActivity?

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.cc:184:
ActiveScriptController* active_script_controller =
maybe ActiveScriptController should be a WebContentsUserData rather than hanging
off TabHelper.

related point, at a high level having the LocationBarController own the
ActiveScriptController doesn't seem right. It's a separate entity, or at least
that's how I'd like to think about it.

Similar comment made in the other file.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/uma_policy.h (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.h:104: content::WebContents*
web_contents);
seems like the URL is implied by the web contents i.e. only need the single
argument.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:28:
std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
To engineer this in a way that doesn't depend on ownership of the
PageActionController and/or ActionScriptController -- which as I mentioned,
doesn't seem right to me -- you'd keep LocationBarController as an interface and
have another implementation like CompositeLocationBarController. It would take a
list of LocationBarControllers as its interface.

The logic needs to take into account like
- only show a single action per extension. have the precedence be well-defined.
- fire both OnClicked events, similar point re precedence. The rationale here is
that if an extension shows a page action and expects that only clicking it a
script has already been injected, it should first inject the script and then
fire the onClicked for the page action.

nicely testable too.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller_provider.h (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller_provider.h:23: class
LocationBarControllerProvider : public content::WebContentsObserver {
This looks a lot like what LocationBarController was. It seems ok to move the
utility-like functions into there.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller_provider.h:45: void
NotifyChange();
it would be nice to make these protected: things as minimal as possible, if even
have them at all. i don't like looking up inheritance chains.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/tab_helper.cc:453: if
(Extension::IdIsValid(extension_id)) {
how could this ever be invalid?

https://codereview.chromium.org/270153004/diff/40001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/common/extens...
extensions/common/extension_messages.h:573: std::string /* extension id */)
we should include the page ID here to avoid races.

https://codereview.chromium.org/270153004/diff/40001/extensions/common/featur...
File extensions/common/feature_switch.h (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/common/featur...
extensions/common/feature_switch.h:28: static FeatureSwitch*
active_script_enforcement();
something more abstract would be appropriate, like
require_action_for_script_injection()? a little too strict (i.e. doesn't imply
that it's not for non-TLD permissions) but close enough.

https://codereview.chromium.org/270153004/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:566: // For now, the user
should be notified when an extension with all hosts
When this changes the name will no longer be appropriate. Maybe something in the
vein of RequiresActionForScriptExecution()?

https://codereview.chromium.org/270153004/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:198: content::RenderView* render_view =
top_render_view?

https://codereview.chromium.org/270153004/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:246: render_view->GetRoutingID(),
(need page ID)

[not at google - send to devlin, 2014-05-07 23:27:51]

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:88:
extensions_executing_scripts_.clear();
also need to make sure not to clear these on a hash change

[not at google - send to devlin, 2014-05-07 23:30:37]

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.h (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.h:43: virtual
~LocationBarController();
no longer virtual

[Devlin, 2014-05-08 00:02:54]

Per offline discussion:
- Keep LocationBarController[Provider] object model/ownership graph.
- Move lots of the logic for LocationBarProviders into LocationBarController
  * Loop over extensions, call LocationBarProvider::GetActionForExtension().
  * Have LocationBarController observe frame changes.
Ben, any other highlights?

[Devlin, 2014-05-08 18:15:46]

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:36:
enabled_(FeatureSwitch::active_script_enforcement()->IsEnabled()) {
On 2014/05/07 22:49:02, kalman wrote:
> if it's not enabled then just don't create it. this is made possible with the
> CompositeController -- if the flag is off, just don't insert an
> ActiveScriptController into that list.
> 
> (you also probably wouldn't bother with the Composite version at all, since
it'd
> only have the single one)

Can't just not create it, since we need it for the UMA metrics.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:45: if
(!extensions_executing_scripts_.count(extension_id)) {
On 2014/05/07 22:49:02, kalman wrote:
> nit: early return.

Done.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:57: bool
ActiveScriptController::HasActionForExtensionId(
On 2014/05/07 22:49:02, kalman wrote:
> eventually we'll want to separate the metrics for an icon showing but not
> injected vs an icon was shown and injected.

Agreed, but not at that point yet (since right now we don't disallow script
injection at all).

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:88:
extensions_executing_scripts_.clear();
On 2014/05/07 23:27:51, kalman wrote:
> also need to make sure not to clear these on a hash change

Done.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:91: ExtensionAction*
ActiveScriptController::GetActionForExtension(
On 2014/05/07 22:49:02, kalman wrote:
> nit: GetOrCreateExtensionAction

Moot.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:104: const ActionInfo*
action_info = ActionInfo::GetPageActionInfo(extension);
On 2014/05/07 22:49:02, kalman wrote:
> see above

Assuming you're referencing the "GetOrCreate" comment.  If not, lemme know.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:111:
badge->set_default_icon(icon_set.Pass());
On 2014/05/07 22:49:02, kalman wrote:
> I swear we already have code somewhere to do this

It's similar to the code in the ctor for ExtensionAction, but far enough apart
that it's not worth hacking it to work.  Otherwise, you might be thinking of the
code that used to be in ScriptBadgeController?

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/uma_policy.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.cc:48: const int
kAnyAdInjection = kAdInjected | kAdRemoved | kAdReplaced;
On 2014/05/07 22:49:02, kalman wrote:
> how about kAnyAdActivity?

Sure.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.cc:184:
ActiveScriptController* active_script_controller =
On 2014/05/07 22:49:02, kalman wrote:
> maybe ActiveScriptController should be a WebContentsUserData rather than
hanging
> off TabHelper.
> 
> related point, at a high level having the LocationBarController own the
> ActiveScriptController doesn't seem right. It's a separate entity, or at least
> that's how I'd like to think about it.
> 
> Similar comment made in the other file.

Talked about offline, and decided to keep current structure.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/uma_policy.h (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.h:104: content::WebContents*
web_contents);
On 2014/05/07 22:49:02, kalman wrote:
> seems like the URL is implied by the web contents i.e. only need the single
> argument.

The url passed here has been cleaned via CleanURL(), so it would be a waste to
duplicate that effort, I think.  Thoughts?

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:28:
std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
On 2014/05/07 22:49:02, kalman wrote:
> To engineer this in a way that doesn't depend on ownership of the
> PageActionController and/or ActionScriptController -- which as I mentioned,
> doesn't seem right to me -- you'd keep LocationBarController as an interface
and
> have another implementation like CompositeLocationBarController. It would take
a
> list of LocationBarControllers as its interface.
> 
> The logic needs to take into account like
> - only show a single action per extension. have the precedence be
well-defined.
> - fire both OnClicked events, similar point re precedence. The rationale here
is
> that if an extension shows a page action and expects that only clicking it a
> script has already been injected, it should first inject the script and then
> fire the onClicked for the page action.
> 
> nicely testable too.

Talked about offline, and decided to restructure the controller/providers, but
keep the same object structure.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.h (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.h:43: virtual
~LocationBarController();
On 2014/05/07 23:30:38, kalman wrote:
> no longer virtual

And... virtual again. :)

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller_provider.h (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller_provider.h:23: class
LocationBarControllerProvider : public content::WebContentsObserver {
On 2014/05/07 22:49:02, kalman wrote:
> This looks a lot like what LocationBarController was. It seems ok to move the
> utility-like functions into there.

Done.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller_provider.h:45: void
NotifyChange();
On 2014/05/07 22:49:02, kalman wrote:
> it would be nice to make these protected: things as minimal as possible, if
even
> have them at all. i don't like looking up inheritance chains.

Done.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/tab_helper.cc:453: if
(Extension::IdIsValid(extension_id)) {
On 2014/05/07 22:49:02, kalman wrote:
> how could this ever be invalid?

IPC defense.  Security team requests that, when passing strings (which they
already dislike), you verify that the string is something you expect as soon as
you get it.

https://codereview.chromium.org/270153004/diff/40001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/common/extens...
extensions/common/extension_messages.h:573: std::string /* extension id */)
On 2014/05/07 22:49:02, kalman wrote:
> we should include the page ID here to avoid races.

Done.

https://codereview.chromium.org/270153004/diff/40001/extensions/common/featur...
File extensions/common/feature_switch.h (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/common/featur...
extensions/common/feature_switch.h:28: static FeatureSwitch*
active_script_enforcement();
On 2014/05/07 22:49:02, kalman wrote:
> something more abstract would be appropriate, like
> require_action_for_script_injection()? a little too strict (i.e. doesn't imply
> that it's not for non-TLD permissions) but close enough.

Done. (per offline discussion, "scripts-require-action").

https://codereview.chromium.org/270153004/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:566: // For now, the user
should be notified when an extension with all hosts
On 2014/05/07 22:49:02, kalman wrote:
> When this changes the name will no longer be appropriate. Maybe something in
the
> vein of RequiresActionForScriptExecution()?

Yeah, I was just planning on changing it when the behavior changed.  But we can
do it now, I guess.

https://codereview.chromium.org/270153004/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:198: content::RenderView* render_view =
On 2014/05/07 22:49:02, kalman wrote:
> top_render_view?

Why not.

https://codereview.chromium.org/270153004/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:246: render_view->GetRoutingID(),
On 2014/05/07 22:49:02, kalman wrote:
> (need page ID)

Done.

[not at google - send to devlin, 2014-05-08 20:47:08]

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/uma_policy.h (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.h:104: content::WebContents*
web_contents);
On 2014/05/08 18:15:46, D Cronin wrote:
> On 2014/05/07 22:49:02, kalman wrote:
> > seems like the URL is implied by the web contents i.e. only need the single
> > argument.
> 
> The url passed here has been cleaned via CleanURL(), so it would be a waste to
> duplicate that effort, I think.  Thoughts?

I see what you mean. maybe call the variable clean_url then. and it would be
nice, where it matters, to DCHECK_EQ(clean_url, CleanURL(clean_url));

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/tab_helper.cc:453: if
(Extension::IdIsValid(extension_id)) {
On 2014/05/08 18:15:46, D Cronin wrote:
> On 2014/05/07 22:49:02, kalman wrote:
> > how could this ever be invalid?
> 
> IPC defense.  Security team requests that, when passing strings (which they
> already dislike), you verify that the string is something you expect as soon
as
> you get it.

hm ok, I was wondering about that, and it seems like you might want to be more
alarmed than just doing nothing in that case. like killing the renderer. but,
eh, checking is fine. maybe you can NOTREACHED() the failure case?

https://codereview.chromium.org/270153004/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/270153004/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:566: // For now, the user
should be notified when an extension with all hosts
On 2014/05/08 18:15:46, D Cronin wrote:
> On 2014/05/07 22:49:02, kalman wrote:
> > When this changes the name will no longer be appropriate. Maybe something in
> the
> > vein of RequiresActionForScriptExecution()?
> 
> Yeah, I was just planning on changing it when the behavior changed.  But we
can
> do it now, I guess.

whatever matches the flag name.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:38:
LogActiveScriptUMA(extensions_executing_scripts_.size());
you should only log UMA if it's enabled

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:61: return
extensions_executing_scripts_.count(extension_id) > 0;
If this isn't |enabled_| then you'll be giving misleading data to callers.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:65: const Extension*
extension) {
check |enabled_|

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:88:
action->set_default_icon(icon_set.Pass());
easier on the eyes:

action->set_default_icon(
    make_scoped_ptr(new ExtensionIconSet(action_info->default_icon));

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:96: const Extension*
extension) {
DCHECK(enabled_)

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:101:
LogActiveScriptUMA(extensions_executing_scripts_.size());
only log UMA if enable

Generally there is so much checking of enabled, and it will be so easy for this
to regress, and it has issues... need to step back and have a higher-level look
at this.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.h (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.h:27: class
ActiveScriptController : public LocationBarControllerProvider {
"Controller" is not an accurate term at this point since it's no longer tied to
a view. It:
- manages active scripts
- provides the LocationBarController with extension actions.
i'd go with ActiveScriptManager.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.h:35: void
NotifyScriptExecuting(const std::string& extension_id, int page_id);
it would be a more convenient interface to pass the whole Extension in here, if
possible.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.h:38: bool
HasActionForExtensionId(const std::string& extension_id);
ditto

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_browsertest.cc:88:
controller->GetActionForExtension(content_scripts_explicit_hosts));
A lot of repeated code here. how about defining a class like Tester:

class Tester {
  Tester(const char* name, bool expect_has_action) {}

  testing::AssertionResult Start() {
    extension_ = LoadExtension(...);
    if (!extension_) return testing::AssertionFailure() << _name;
    listener_ = ...;
    return testing::AssertionSuccess();
  }

  testing::AssertionResult Verify() {
    listener_.WaitUntilSatisfied();
    ...
    return expect_has_action_ == (controller->GetActionForExtension(...) !=
NULL))
        ? testing::AssertionSuccess() : testing::AssertionFailure() << name_;
  }
};

TEST(...) {
  Tester testers[] = {
    Tester("content_scripts_all_hosts", true),
    Tester("...", true),
    Tester("...", false);
  };

  for (size_t i = 0; i < arraysize(testers); ++i)
    ASSERT_TRUE(testers[i].Start());

  // web contents thing.

  for (size_t i = 0; i < arraysize(testers); ++i)
    EXPECT_TRUE(testers[i].Verify());
}

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/uma_policy.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.cc:205:
active_script_controller->HasActionForExtensionId(ext_iter->first);
for the enabled_ problem I mentioned in the controller, maybe move the UMA
logging onto the ASC interface and implement in there? Like
OnAdActivity(extension). I don't like the idea of exposing enabled() outside the
class. Hiding HasActionForExtensionId would be nice.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:402:
NOTREACHED();  // We shouldn't get these calls (yet).
even if we did I don't think we'll ever expose an API for them. so the "yet"
might be a little misleading :)

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:59: Action page_action =
page_action_controller_->OnClicked(extension);
It's a bit weird to be sending these events if the controller didn't actually
provide an action. Perhaps guard each of these with an

if (page_action_controller_->GetActionForExtension(action->extension_id())
  page_action_controller_->OnClicked();
...

Two things I worry about -- and don't especially want to go look up:
- calling OnClicked does too much (e.g. grant activeTab or send onClicked event
when it shouldn't)
- calling OnClicked does too little (e.g. if we didn't create an active script
badge due to logic on line 43, but the implementation checks the wrong thing).

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.h (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.h:25: // the location bar.
Depending on switches, these icons can have differing
hey this comment makes sense again!

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.h:49: ActiveScriptController*
active_script_controller() {
hey if the activity log exposed an observer interface for an ad being injected
then you wouldn't need this either! (don't worry about it)

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.h:55: }
if you don't need to expose this, don't. the active script controller is a
special case (maybe the page action controller is too?)

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller_provider.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller_provider.cc:31: void
LocationBarControllerProvider::DidNavigateMainFrame(
delete this method?

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller_provider.h (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller_provider.h:16: class
LocationBarControllerProvider {
the name of this class implies that it provides location bar controllers, which
is not right. LocationBarControllerDelegate?

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/page_action_controller.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/page_action_controller.cc:38:
ExtensionActionManager::Get(GetProfile())->GetPageAction(*extension);
I would expect ExtensionActionManager to do its own caching of these values?
I.e. most of these methods basically just forward through to the
ExtensionActionManager.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/script_executor.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/script_executor.cc:134:
web_contents_->GetController().GetVisibleEntry()->GetPageID());
now I remember why the old code implemented ScriptExecutor. So that it could
implement this via delegation.

no big deal. Observer interface might have been better.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/tab_helper.cc:455:
location_bar_controller()->active_script_controller()
Can you make ActiveScriptController a WebContentsObserver and implement this
itself? unfortunately to be catching the IPC here and just routing it somewhere
else.

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

https://codereview.chromium.org/270153004/diff/70001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:181: // |extension| is running
a script.
shorter: Returns true if the user should be alerted that |extension| is running
a script.

[Devlin, 2014-05-08 23:00:59]

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/uma_policy.h (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.h:104: content::WebContents*
web_contents);
On 2014/05/08 20:47:09, kalman wrote:
> On 2014/05/08 18:15:46, D Cronin wrote:
> > On 2014/05/07 22:49:02, kalman wrote:
> > > seems like the URL is implied by the web contents i.e. only need the
single
> > > argument.
> > 
> > The url passed here has been cleaned via CleanURL(), so it would be a waste
to
> > duplicate that effort, I think.  Thoughts?
> 
> I see what you mean. maybe call the variable clean_url then. and it would be
> nice, where it matters, to DCHECK_EQ(clean_url, CleanURL(clean_url));

Done.

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/270153004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/tab_helper.cc:453: if
(Extension::IdIsValid(extension_id)) {
On 2014/05/08 20:47:09, kalman wrote:
> On 2014/05/08 18:15:46, D Cronin wrote:
> > On 2014/05/07 22:49:02, kalman wrote:
> > > how could this ever be invalid?
> > 
> > IPC defense.  Security team requests that, when passing strings (which they
> > already dislike), you verify that the string is something you expect as soon
> as
> > you get it.
> 
> hm ok, I was wondering about that, and it seems like you might want to be more
> alarmed than just doing nothing in that case. like killing the renderer. but,
> eh, checking is fine. maybe you can NOTREACHED() the failure case?

Done.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:38:
LogActiveScriptUMA(extensions_executing_scripts_.size());
On 2014/05/08 20:47:09, kalman wrote:
> you should only log UMA if it's enabled

Free (still good data) even if not enabled.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:65: const Extension*
extension) {
On 2014/05/08 20:47:09, kalman wrote:
> check |enabled_|

Done.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:88:
action->set_default_icon(icon_set.Pass());
On 2014/05/08 20:47:09, kalman wrote:
> easier on the eyes:
> 
> action->set_default_icon(
>     make_scoped_ptr(new ExtensionIconSet(action_info->default_icon));

Done.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:96: const Extension*
extension) {
On 2014/05/08 20:47:09, kalman wrote:
> DCHECK(enabled_)

We can get OnClicked even if not enabled, since the OnClick() goes through the
location bar controller into each of the providers (since actions may be
grouped).

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:101:
LogActiveScriptUMA(extensions_executing_scripts_.size());
On 2014/05/08 20:47:09, kalman wrote:
> only log UMA if enable
> 
> Generally there is so much checking of enabled, and it will be so easy for
this
> to regress, and it has issues... need to step back and have a higher-level
look
> at this.

UMA if not enabled is fine.  Only need to check enabled once.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.h (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.h:27: class
ActiveScriptController : public LocationBarControllerProvider {
On 2014/05/08 20:47:09, kalman wrote:
> "Controller" is not an accurate term at this point since it's no longer tied
to
> a view. It:
> - manages active scripts
> - provides the LocationBarController with extension actions.
> i'd go with ActiveScriptManager.

Added a TODO, per offline conversation.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.h:35: void
NotifyScriptExecuting(const std::string& extension_id, int page_id);
On 2014/05/08 20:47:09, kalman wrote:
> it would be a more convenient interface to pass the whole Extension in here,
if
> possible.

Since this is forwarded via IPC, it doesn't make sense to make them look up the
extension when all we need is the ID.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.h:38: bool
HasActionForExtensionId(const std::string& extension_id);
On 2014/05/08 20:47:09, kalman wrote:
> ditto
Moot.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_browsertest.cc:88:
controller->GetActionForExtension(content_scripts_explicit_hosts));
On 2014/05/08 20:47:09, kalman wrote:
> A lot of repeated code here. how about defining a class like Tester:
> 
> class Tester {
>   Tester(const char* name, bool expect_has_action) {}
> 
>   testing::AssertionResult Start() {
>     extension_ = LoadExtension(...);
>     if (!extension_) return testing::AssertionFailure() << _name;
>     listener_ = ...;
>     return testing::AssertionSuccess();
>   }
> 
>   testing::AssertionResult Verify() {
>     listener_.WaitUntilSatisfied();
>     ...
>     return expect_has_action_ == (controller->GetActionForExtension(...) !=
> NULL))
>         ? testing::AssertionSuccess() : testing::AssertionFailure() << name_;
>   }
> };
> 
> TEST(...) {
>   Tester testers[] = {
>     Tester("content_scripts_all_hosts", true),
>     Tester("...", true),
>     Tester("...", false);
>   };
> 
>   for (size_t i = 0; i < arraysize(testers); ++i)
>     ASSERT_TRUE(testers[i].Start());
> 
>   // web contents thing.
> 
>   for (size_t i = 0; i < arraysize(testers); ++i)
>     EXPECT_TRUE(testers[i].Verify());
> }

Definitely not as pretty as it was when you wrote it... but done!

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/uma_policy.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/activity_log/uma_policy.cc:205:
active_script_controller->HasActionForExtensionId(ext_iter->first);
On 2014/05/08 20:47:09, kalman wrote:
> for the enabled_ problem I mentioned in the controller, maybe move the UMA
> logging onto the ASC interface and implement in there? Like
> OnAdActivity(extension). I don't like the idea of exposing enabled() outside
the
> class. Hiding HasActionForExtensionId would be nice.

Done.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:402:
NOTREACHED();  // We shouldn't get these calls (yet).
On 2014/05/08 20:47:09, kalman wrote:
> even if we did I don't think we'll ever expose an API for them. so the "yet"
> might be a little misleading :)

Actually, since PageActions and ActiveScriptActions have to be able to be
combined now, we don't even use the new type. Removed.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:59: Action page_action =
page_action_controller_->OnClicked(extension);
On 2014/05/08 20:47:09, kalman wrote:
> It's a bit weird to be sending these events if the controller didn't actually
> provide an action. Perhaps guard each of these with an
> 
> if (page_action_controller_->GetActionForExtension(action->extension_id())
>   page_action_controller_->OnClicked();
> ...
> 
> Two things I worry about -- and don't especially want to go look up:
> - calling OnClicked does too much (e.g. grant activeTab or send onClicked
event
> when it shouldn't)
> - calling OnClicked does too little (e.g. if we didn't create an active script
> badge due to logic on line 43, but the implementation checks the wrong thing).

We're already checking in each of the classes, and this will result in slightly
more work in a edge case, and more code here.  But, per offline discussion,
done.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.h (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.h:55: }
On 2014/05/08 20:47:09, kalman wrote:
> if you don't need to expose this, don't. the active script controller is a
> special case (maybe the page action controller is too?)

Done.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller_provider.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller_provider.cc:31: void
LocationBarControllerProvider::DidNavigateMainFrame(
On 2014/05/08 20:47:09, kalman wrote:
> delete this method?

s/method/file.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller_provider.h (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller_provider.h:16: class
LocationBarControllerProvider {
On 2014/05/08 20:47:09, kalman wrote:
> the name of this class implies that it provides location bar controllers,
which
> is not right. LocationBarControllerDelegate?

Not a delegate either.  Or a helper, filter, etc.  Offline decided
LocationBarController::ActionProvider.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/page_action_controller.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/page_action_controller.cc:38:
ExtensionActionManager::Get(GetProfile())->GetPageAction(*extension);
On 2014/05/08 20:47:09, kalman wrote:
> I would expect ExtensionActionManager to do its own caching of these values?
> I.e. most of these methods basically just forward through to the
> ExtensionActionManager.

Was for clearing values below, but refactored to not need.

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/script_executor.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/script_executor.cc:134:
web_contents_->GetController().GetVisibleEntry()->GetPageID());
On 2014/05/08 20:47:09, kalman wrote:
> now I remember why the old code implemented ScriptExecutor. So that it could
> implement this via delegation.
> 
> no big deal. Observer interface might have been better.

Yeah, I was debating it.  But wanted to keep the CL "small".

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/270153004/diff/70001/chrome/browser/extension...
chrome/browser/extensions/tab_helper.cc:455:
location_bar_controller()->active_script_controller()
On 2014/05/08 20:47:09, kalman wrote:
> Can you make ActiveScriptController a WebContentsObserver and implement this
> itself? unfortunately to be catching the IPC here and just routing it
somewhere
> else.

It makes me sad that now ActiveScriptController inherits from two classes (one
of which is LocationBarController already inherits, but... such is life.  Done.

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

https://codereview.chromium.org/270153004/diff/70001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:181: // |extension| is running
a script.
On 2014/05/08 20:47:09, kalman wrote:
> shorter: Returns true if the user should be alerted that |extension| is
running
> a script.

Yay for 20 characters less!

[not at google - send to devlin, 2014-05-08 23:33:19]

lgtm

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

https://codereview.chromium.org/270153004/diff/70001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:181: // |extension| is running
a script.
On 2014/05/08 23:01:00, D Cronin wrote:
> On 2014/05/08 20:47:09, kalman wrote:
> > shorter: Returns true if the user should be alerted that |extension| is
> running
> > a script.
> 
> Yay for 20 characters less!

Think of all the people that have to read 20 fewer characters.

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:36:
web_contents_(web_contents),
WebContentsObserver has a web_contents() method, use that rather than hanging
onto web_contents_ yourself.

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:38: LOG(WARNING) <<
"Constructed";
remove this

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:65: UMA_HISTOGRAM_BOOLEAN(
a couple of problems here:
- it would be useful to know a number here, rather than just a heap of
true/false
- I'm not confident that |extensions_executing_scripts_| hasn't already been
deleted by this point.

a couple of fixes:
- stream extension IDs to this method instead of telling it at the end
- pass in the set/vector of extension IDs all at once and hold onto those IDs
until the next page loads

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:125: NOTREACHED();
<< "\"" << extension_id << "\" is not a valid ID";

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_browsertest.cc:73: return
testing::AssertionFailure() << "Could not load extension: " << name_;
what happened to your RETURN_IF_FALSE macro?

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:57: CHECK(extension);
<< action->extension_id();

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/page_action_controller.cc (right):

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/page_action_controller.cc:80: // Report the number of
page actions for this profile, if we haven't already.
this is so broken. can you TODO-ify to fix this (should be the same as the other
UMA, log on page close)

https://codereview.chromium.org/270153004/diff/90001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/270153004/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5390: +    user's permission before
injecting an extension script.
I'll let isherman (or other) review this file properly, but it'd be worth
mentioning here the ActivityLog UMA which relates to this.

[Devlin, 2014-05-09 00:15:32]

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:36:
web_contents_(web_contents),
On 2014/05/08 23:33:19, kalman wrote:
> WebContentsObserver has a web_contents() method, use that rather than hanging
> onto web_contents_ yourself.

Done.

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:38: LOG(WARNING) <<
"Constructed";
On 2014/05/08 23:33:19, kalman wrote:
> remove this

D'oh, missed that one. Thanks.

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:65: UMA_HISTOGRAM_BOOLEAN(
On 2014/05/08 23:33:19, kalman wrote:
> a couple of problems here:
> - it would be useful to know a number here, rather than just a heap of
> true/false
> - I'm not confident that |extensions_executing_scripts_| hasn't already been
> deleted by this point.
> 
> a couple of fixes:
> - stream extension IDs to this method instead of telling it at the end
> - pass in the set/vector of extension IDs all at once and hold onto those IDs
> until the next page loads

Can't stream because we can't get a web contents (without changing a bunch of
code).  But, in theory, the order is right. ActivityLog does stuff, and then
ActiveScriptController does stuff.  If we get an overwhelming amount of zeros,
we'll know something went wrong.

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:125: NOTREACHED();
On 2014/05/08 23:33:19, kalman wrote:
> << "\"" << extension_id << "\" is not a valid ID";

Done.

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_browsertest.cc:73: return
testing::AssertionFailure() << "Could not load extension: " << name_;
On 2014/05/08 23:33:19, kalman wrote:
> what happened to your RETURN_IF_FALSE macro?

I don't wanna just chuck it into a test file.  I'll look into getting it into
gtest next week, and, failing that, put it in a common extension_test_macros.

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:57: CHECK(extension);
On 2014/05/08 23:33:19, kalman wrote:
> << action->extension_id();

Done.

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/page_action_controller.cc (right):

https://codereview.chromium.org/270153004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/page_action_controller.cc:80: // Report the number of
page actions for this profile, if we haven't already.
On 2014/05/08 23:33:19, kalman wrote:
> this is so broken. can you TODO-ify to fix this (should be the same as the
other
> UMA, log on page close)

Done.

https://codereview.chromium.org/270153004/diff/90001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/270153004/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5390: +    user's permission before
injecting an extension script.
On 2014/05/08 23:33:19, kalman wrote:
> I'll let isherman (or other) review this file properly, but it'd be worth
> mentioning here the ActivityLog UMA which relates to this.

Done.

[Devlin, 2014-05-09 00:17:06]

+jschuh@ - extension_messages.h
+isherman@ - histograms.xml

[not at google - send to devlin, 2014-05-09 00:22:58]

https://codereview.chromium.org/270153004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/active_script_controller.cc:127: }
easier:

size_t num_preventable_ad_injectors = STLSetIntersection(
    ad_injectors_, extensions_executing_scripts_).size();

https://codereview.chromium.org/270153004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/active_script_controller.cc:131:
num_preventable_ad_injectors);
maybe it WOULD be nice to also log how many wouldn't have been prevented, even
if you can figure it out by looking at the ActivityLog UMA... hm. Ilya WDYT?

[Ilya Sherman, 2014-05-09 21:47:17]

histograms.xml LGTM

https://codereview.chromium.org/270153004/diff/110001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/270153004/diff/110001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5735: +    ActivityLog metrics,
ExtensionActivity.Ad*.
Please document when this is recorded -- also when the page is closed?

[jschuh, 2014-05-09 22:04:49]

ipc security lgtm (notes: extension id tracking string, validated by receiving
process)

[Ilya Sherman, 2014-05-09 22:08:43]

https://codereview.chromium.org/270153004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/active_script_controller.cc:131:
num_preventable_ad_injectors);
On 2014/05/09 00:22:59, kalman wrote:
> maybe it WOULD be nice to also log how many wouldn't have been prevented, even
> if you can figure it out by looking at the ActivityLog UMA... hm. Ilya WDYT?

Sorry, missed this question before.  Either way is fine by me -- whichever you
guys prefer.

[Devlin, 2014-05-09 22:23:28]

https://codereview.chromium.org/270153004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/270153004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/active_script_controller.cc:127: }
On 2014/05/09 00:22:59, kalman wrote:
> easier:
> 
> size_t num_preventable_ad_injectors = STLSetIntersection(
>     ad_injectors_, extensions_executing_scripts_).size();

Done.

https://codereview.chromium.org/270153004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/active_script_controller.cc:131:
num_preventable_ad_injectors);
On 2014/05/09 22:08:43, Ilya Sherman wrote:
> On 2014/05/09 00:22:59, kalman wrote:
> > maybe it WOULD be nice to also log how many wouldn't have been prevented,
even
> > if you can figure it out by looking at the ActivityLog UMA... hm. Ilya WDYT?
> 
> Sorry, missed this question before.  Either way is fine by me -- whichever you
> guys prefer.

Added it, to save us from having to add up all different ad injection types.

https://codereview.chromium.org/270153004/diff/110001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/270153004/diff/110001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5735: +    ActivityLog metrics,
ExtensionActivity.Ad*.
On 2014/05/09 21:47:17, Ilya Sherman wrote:
> Please document when this is recorded -- also when the page is closed?

Done.

[Devlin, 2014-05-09 22:23:37]

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

[commit-bot: I haz the power, 2014-05-09 22:25:09]

CQ is trying da patch. Follow status at

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

[Devlin, 2014-05-10 01:05:56]

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

[commit-bot: I haz the power, 2014-05-10 01:08:43]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-05-10 02:45:32]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-10 02:52:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 02:52:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Devlin, 2014-05-10 03:24:04]

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

[commit-bot: I haz the power, 2014-05-10 03:25:08]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-05-10 04:34:47]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-10 04:41:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 04:41:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[not at google - send to devlin, 2014-05-10 14:14:49]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-05-10 14:15:29]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-05-10 17:57:39]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Devlin, 2014-05-10 18:25:27]

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

[Devlin, 2014-05-10 18:58:00]

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

[commit-bot: I haz the power, 2014-05-10 18:58:57]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-05-11 01:40:05]

Change committed as 269681