Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

This needs to be in this order so that PermissionIgnored records the
correct num_prior_* numbers for dismissals and ignores.

BUG=613883, 638076
Committed: https://crrev.com/7b5e0e8356e4d3b095cbae427d8eee7d10db401c
Cr-Commit-Position: refs/heads/master@{#414014}

[kcarattini, 2016-08-23 04:50:36]

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

[commit-bot: I haz the power, 2016-08-23 04:50:53]

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

[kcarattini, 2016-08-23 04:50:59]

kcarattini@chromium.org changed reviewers:
+ dominickn@chromium.org, raymes@chromium.org

[raymes, 2016-08-23 06:01:48]

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

https://codereview.chromium.org/2267353002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_request_impl.cc:43: permission_type_);
Could we instead move this into PermissionUmaUtil::PermissionIgnored?

[commit-bot: I haz the power, 2016-08-23 06:32:59]

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

[commit-bot: I haz the power, 2016-08-23 06:33:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kcarattini, 2016-08-23 07:19:50]

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

https://codereview.chromium.org/2267353002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_request_impl.cc:43: permission_type_);
On 2016/08/23 06:01:48, raymes wrote:
> Could we instead move this into PermissionUmaUtil::PermissionIgnored?

Done. I like this, but I don't like that of symmetry in the recording of prior
dismisses. Maybe we could think about separating that from the test of whether
or not to block after n dismissals? I'd like to hear what dominickn@ thinks.

[raymes, 2016-08-23 08:14:58]

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

https://codereview.chromium.org/2267353002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_request_impl.cc:43: permission_type_);
On 2016/08/23 07:19:50, kcarattini wrote:
> On 2016/08/23 06:01:48, raymes wrote:
> > Could we instead move this into PermissionUmaUtil::PermissionIgnored?
> 
> Done. I like this, but I don't like that of symmetry in the recording of prior
> dismisses. Maybe we could think about separating that from the test of whether
> or not to block after n dismissals? I'd like to hear what dominickn@ thinks.

Were you thinking we should record the dismiss in the
PermissionUmaUtil::PermissionPromptDismissed and check the number of dismisses
separately? I like that idea too, I'd be interested in what Dom thinks too.

[kcarattini, 2016-08-23 10:00:59]

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

https://codereview.chromium.org/2267353002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_request_impl.cc:43: permission_type_);
On 2016/08/23 08:14:58, raymes wrote:
> On 2016/08/23 07:19:50, kcarattini wrote:
> > On 2016/08/23 06:01:48, raymes wrote:
> > > Could we instead move this into PermissionUmaUtil::PermissionIgnored?
> > 
> > Done. I like this, but I don't like that of symmetry in the recording of
prior
> > dismisses. Maybe we could think about separating that from the test of
whether
> > or not to block after n dismissals? I'd like to hear what dominickn@ thinks.
> 
> Were you thinking we should record the dismiss in the
> PermissionUmaUtil::PermissionPromptDismissed and check the number of dismisses
> separately? I like that idea too, I'd be interested in what Dom thinks too.

Yes, that's what I meant :).

[dominickn, 2016-08-23 17:09:05]

lgtm

You can add 638076 to the bug list for this.

I'll be looking at splitting up the recording from deciding to block on that bug
too once this milestone passes.

[kcarattini, 2016-08-24 03:18:06]

Description was changed from

==========
Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

This needs to be in this order so that PermissionIgnored records the
correct num_prior_* numbers for dismissals and ignores.

BUG=613883
==========

to

==========
Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

This needs to be in this order so that PermissionIgnored records the
correct num_prior_* numbers for dismissals and ignores.

BUG=613883, 638076
==========

[kcarattini, 2016-08-24 03:18:20]

On 2016/08/23 17:09:05, dominickn wrote:
> lgtm
> 
> You can add 638076 to the bug list for this.

Done.

> 
> I'll be looking at splitting up the recording from deciding to block on that
bug
> too once this milestone passes.

[raymes, 2016-08-24 05:54:27]

lgtm

[kcarattini, 2016-08-24 06:27:03]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2016-08-24 06:27:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2267353002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-24 06:27:36]

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, 2016-08-24 08:04:29]

Description was changed from

==========
Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

This needs to be in this order so that PermissionIgnored records the
correct num_prior_* numbers for dismissals and ignores.

BUG=613883, 638076
==========

to

==========
Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

This needs to be in this order so that PermissionIgnored records the
correct num_prior_* numbers for dismissals and ignores.

BUG=613883, 638076
==========

[commit-bot: I haz the power, 2016-08-24 08:04:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-24 08:08:42]

Description was changed from

==========
Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

This needs to be in this order so that PermissionIgnored records the
correct num_prior_* numbers for dismissals and ignores.

BUG=613883, 638076
==========

to

==========
Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

This needs to be in this order so that PermissionIgnored records the
correct num_prior_* numbers for dismissals and ignores.

BUG=613883, 638076

Committed: https://crrev.com/7b5e0e8356e4d3b095cbae427d8eee7d10db401c
Cr-Commit-Position: refs/heads/master@{#414014}
==========

[commit-bot: I haz the power, 2016-08-24 08:08:43]

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