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

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

The scripts-require-action feature should affect all extensions that don't have
the "can run on all urls" preference. When an extension is installed, it should
have the "all urls" flag by default if the switch is off.

This is necessary in order to allow syncing the all urls flag.

BUG=454832
Committed: https://crrev.com/b8dffe56ae37e7429ca21be254480f01034cac21
Cr-Commit-Position: refs/heads/master@{#315153}

[Devlin, 2015-02-04 23:04:11]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-02-04 23:04:16]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-02-04 23:04:21]

Patchset #1 (id:40001) has been deleted

[Devlin, 2015-02-05 19:57:29]

Patchset #1 (id:60001) has been deleted

[Devlin, 2015-02-05 19:57:39]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

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

Ok, lgtm with some nits.

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

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:287:
UMA_HISTOGRAM_COUNTS_100(
As discussed, this is rather pointless at this stage.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:293: if
(extensions::FeatureSwitch::scripts_require_action()->IsEnabled()) {
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.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:83: new
base::FundamentalValue(allowed));
So that the prefs delete the value if false, use (allowed ? new
base::FundamentalValue(true) : NULL)

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:212: bool default_value =
DefaultAllowedScriptingOnAllUrls();
sorry I'm in a nit mood. Just assign to |allowed|.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:226:
SetAllowedScriptingOnAllUrlsHelper(context, extension_id, allowed, true);
now that you have this helper, it would be nice to write this method like

if (allowed != Allowed..)
  SetAllowed...

though I don't know if that check is worth it anyway. It's not like this
function is going to be called all that much, and it's a bit hard to reason
about now that AllowedScriptingOnAllUrls has sideeffects.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_util.h (right):

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_util.h:90: const scoped_refptr<const
PermissionSet>& permissions);
A little odd to be passing refcounted |extension| as a raw pointer but
|permissions| as a scoped_refptr. I'd just pass them both as a raw pointer.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:341: //   on
those specified in the permissions.
The comment and/or this variable name need updating. "Wants" isn't quite the
meaning here.

[Devlin, 2015-02-06 18:43:45]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

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

Couple questions on UMA.

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

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:287:
UMA_HISTOGRAM_COUNTS_100(
On 2015/02/06 00:26:55, kalman wrote:
> As discussed, this is rather pointless at this stage.

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?

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:293: if
(extensions::FeatureSwitch::scripts_require_action()->IsEnabled()) {
On 2015/02/06 00:26:55, kalman wrote:
> 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.

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?

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:83: new
base::FundamentalValue(allowed));
On 2015/02/06 00:26:55, kalman wrote:
> So that the prefs delete the value if false, use (allowed ? new
> base::FundamentalValue(true) : NULL)

As discussed offline, we actually need this pref to have a value (to indicate
whether the switch was on/off at installation).
Added a TODO to change it to null once the switch is on by default.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:212: bool default_value =
DefaultAllowedScriptingOnAllUrls();
On 2015/02/06 00:26:55, kalman wrote:
> sorry I'm in a nit mood. Just assign to |allowed|.

Done.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:226:
SetAllowedScriptingOnAllUrlsHelper(context, extension_id, allowed, true);
On 2015/02/06 00:26:55, kalman wrote:
> now that you have this helper, it would be nice to write this method like
> 
> if (allowed != Allowed..)
>   SetAllowed...
> 
> though I don't know if that check is worth it anyway. It's not like this
> function is going to be called all that much, and it's a bit hard to reason
> about now that AllowedScriptingOnAllUrls has sideeffects.

Thanks for the catch - I usually cringe at those useless returns, but somehow
missed this one.

Re the check itself, it's definitely worthwhile.  Two big reasons:
- hypothetical: someone is observing the pref for changes/modifications. No
reason to alert them if there is none.
- practical: very shortly, we need to sync these changes. No reason to trigger a
sync if there's no change.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_util.h (right):

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_util.h:90: const scoped_refptr<const
PermissionSet>& permissions);
On 2015/02/06 00:26:56, kalman wrote:
> A little odd to be passing refcounted |extension| as a raw pointer but
> |permissions| as a scoped_refptr. I'd just pass them both as a raw pointer.

Done.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:341: //   on
those specified in the permissions.
On 2015/02/06 00:26:56, kalman wrote:
> The comment and/or this variable name need updating. "Wants" isn't quite the
> meaning here.

Done.

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

still lgtm

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

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:287:
UMA_HISTOGRAM_COUNTS_100(
On 2015/02/06 18:58:43, Devlin wrote:
> On 2015/02/06 00:26:55, kalman wrote:
> > As discussed, this is rather pointless at this stage.
> 
> 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?

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.

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:293: if
(extensions::FeatureSwitch::scripts_require_action()->IsEnabled()) {
On 2015/02/06 18:58:43, Devlin wrote:
> On 2015/02/06 00:26:55, kalman wrote:
> > 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.
> 
> 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.

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.

> 
> 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.

Heh, yeah I realised this on my way to the BART after work.

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

It could be a bool. something_interesting_happened_ :-)

[Devlin, 2015-02-06 23:16:33]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

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

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

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:287:
UMA_HISTOGRAM_COUNTS_100(
On 2015/02/06 19:38:43, kalman wrote:
> On 2015/02/06 18:58:43, Devlin wrote:
> > On 2015/02/06 00:26:55, kalman wrote:
> > > As discussed, this is rather pointless at this stage.
> > 
> > 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?
> 
> 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.

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

https://codereview.chromium.org/874683005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:293: if
(extensions::FeatureSwitch::scripts_require_action()->IsEnabled()) {
On 2015/02/06 19:38:43, kalman wrote:
> On 2015/02/06 18:58:43, Devlin wrote:
> > On 2015/02/06 00:26:55, kalman wrote:
> > > 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.
> > 
> > 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.
> 
> 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.
> 
> > 
> > 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.
> 
> Heh, yeah I realised this on my way to the BART after work.
> 
> > 
> > Of course, if you feel strongly, we can make a new set of "tracked
extensions"
> > which is separate from "permitted extensions".  Preferences?
> 
> It could be a bool. something_interesting_happened_ :-)

Added the "something_interesting_happened" ;)

[Devlin, 2015-02-06 23:18:57]

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

[commit-bot: I haz the power, 2015-02-06 23:19:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874683005/120001

[commit-bot: I haz the power, 2015-02-07 00:58:10]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2015-02-07 00:58:35]

Patchset 3 (id:??) landed as
https://crrev.com/b8dffe56ae37e7429ca21be254480f01034cac21
Cr-Commit-Position: refs/heads/master@{#315153}