Implement permissions embargo for prompts which are repeatedly ignored.

Implement permissions embargo for prompts which are repeatedly ignored.

This CL implements embargoing when permission prompts for an (origin,
permission) pair are ignored 4 times in a row by users.

BUG=706726
Review-Url: https://codereview.chromium.org/2790493002
Cr-Commit-Position: refs/heads/master@{#461667}
Committed: https://chromium.googlesource.com/chromium/src/+/728c5027f96edd1203c61c7683a419288830f7a6

[dominickn, 2017-03-30 07:58:51]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 07:59:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 09:04:45]

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

[commit-bot: I haz the power, 2017-03-30 09:04:46]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-03-30 23:25:24]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org

[dominickn, 2017-03-30 23:25:24]

PTAL, thanks!

[benwells, 2017-03-31 04:47:14]

Meta question: why not block based on (ignores + dimisses) > n, instead of
(ignores > n) || (dismisses > m) ??

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_context_base.cc:151: case
PermissionStatusSource::SAFE_BROWSING_BLACKLIST:
How come a safe browsing change is in this CL?

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_decision_auto_blocker.cc (right):

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_decision_auto_blocker.cc:235: if
(base::StringToInt(ignore_embargo_value, &ignore_embargo_days) &&
These variables are all confusingly named. For g_foo, you should use local foo
and foo_val or foo_str or foo_value.

So, g_ignore_embargo_days, ignore_embargo_days, and ignore_embargo_days_value.

You can probs remove the prompt_ from go_prompt_ignores_before_block as well.

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_decision_auto_blocker.cc:281: if
(IsUnderEmbargo(permission_dict, features::kPermissionsBlacklist,
Nice factoring :)

[dominickn, 2017-04-02 23:43:07]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-02 23:43:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-04-02 23:45:57]

Thanks!

The answer to the meta question is that we have metrics for (dismisses before
accept/block) and (ignores before accept/block). We don't have them for (dismiss
+ ignore before). The selected constants are based on the behaviour we see from
metrics.

We could certainly add new metrics to look at (dismiss + ignore) before actions.
I'm concerned at the number though - we'd need to add ~10 more metrics (per each
prompting permission, per accept/block). It would be 10 more if we wanted
complete coverage of dismiss + ignore before dismiss / ignore as well.

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_context_base.cc:151: case
PermissionStatusSource::SAFE_BROWSING_BLACKLIST:
On 2017/03/31 04:47:14, benwells wrote:
> How come a safe browsing change is in this CL?

I refactored this from a conditional to a switch, and realised that there was no
console message printed for when a blacklisted permission is blocked. I wanted
to explicitly enumerate all of the PermissionStatusSource values to ensure a
compiler error if a new one was added. That meant I could have the blacklist one
as falling through the break, but since adding the console log was 4 lines, I
included it.

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_decision_auto_blocker.cc (right):

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_decision_auto_blocker.cc:235: if
(base::StringToInt(ignore_embargo_value, &ignore_embargo_days) &&
On 2017/03/31 04:47:14, benwells wrote:
> These variables are all confusingly named. For g_foo, you should use local foo
> and foo_val or foo_str or foo_value.
> 
> So, g_ignore_embargo_days, ignore_embargo_days, and ignore_embargo_days_value.
> 
> You can probs remove the prompt_ from go_prompt_ignores_before_block as well.

Done.

https://codereview.chromium.org/2790493002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_decision_auto_blocker.cc:281: if
(IsUnderEmbargo(permission_dict, features::kPermissionsBlacklist,
On 2017/03/31 04:47:14, benwells wrote:
> Nice factoring :)

:)

[commit-bot: I haz the power, 2017-04-03 01:21:36]

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

[commit-bot: I haz the power, 2017-04-03 01:21:36]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-04-03 01:54:17]

lgtm

[dominickn, 2017-04-03 02:30:32]

dominickn@chromium.org changed reviewers:
+ isherman@chromium.org, msramek@chromium.org

[dominickn, 2017-04-03 02:30:32]

+isherman for histograms.xml
+msramek for browsing data remover (trivial API change)

Thanks!

[raymes, 2017-04-03 05:53:16]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2017-04-03 05:53:16]

https://codereview.chromium.org/2790493002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_decision_auto_blocker.cc (right):

https://codereview.chromium.org/2790493002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_decision_auto_blocker.cc:348: bool
PermissionDecisionAutoBlocker::RecordIgnoreAndEmbargo(
Hmm will this apply to plugins too?

[dominickn, 2017-04-03 06:06:20]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2017-04-03 06:06:28]

Thanks!

https://codereview.chromium.org/2790493002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_decision_auto_blocker.cc (right):

https://codereview.chromium.org/2790493002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_decision_auto_blocker.cc:348: bool
PermissionDecisionAutoBlocker::RecordIgnoreAndEmbargo(
On 2017/04/03 05:53:16, raymes (OOO until April 10) wrote:
> Hmm will this apply to plugins too?

Good catch! Casing that one out (and adding a test).

benwells and I discussed how having these sorts of choices apply in
PermissionContextBase would be ideal. Unfortunately, the embargo stuff is
currently called in a lot of places which don't have access to a ContextBase,
which makes that tricky.

[commit-bot: I haz the power, 2017-04-03 06:06:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 07:07:33]

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

[commit-bot: I haz the power, 2017-04-03 07:07:34]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-04-03 12:18:24]

browsing_data/ LGTM

[Ilya Sherman, 2017-04-04 00:10:38]

histograms.xml lgtm

[dominickn, 2017-04-04 06:53:28]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-04-04 06:53:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2790493002/#ps40001
(title: "Not for plugins")

[commit-bot: I haz the power, 2017-04-04 06:53:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 09:14:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491288808572600,
"parent_rev": "e272b699ad6aeb171ed6d4433d8caf1212fb424d", "commit_rev":
"728c5027f96edd1203c61c7683a419288830f7a6"}

[commit-bot: I haz the power, 2017-04-04 09:15:11]

Description was changed from

==========
Implement permissions embargo for prompts which are repeatedly ignored.

This CL implements embargoing when permission prompts for an (origin,
permission) pair are ignored 4 times in a row by users.

BUG=706726
==========

to

==========
Implement permissions embargo for prompts which are repeatedly ignored.

This CL implements embargoing when permission prompts for an (origin,
permission) pair are ignored 4 times in a row by users.

BUG=706726

Review-Url: https://codereview.chromium.org/2790493002
Cr-Commit-Position: refs/heads/master@{#461667}
Committed:
https://chromium.googlesource.com/chromium/src/+/728c5027f96edd1203c61c7683a4...
==========

[commit-bot: I haz the power, 2017-04-04 09:15:13]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/728c5027f96edd1203c61c7683a4...