Can you give some examples of 2?

Some examples of 2:

PermissionInfobarDelegate (https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_infobar_delegate.h) calls ignored metrics in its destructor for single permissions on Android.

But the GroupedPermissionInfoBarDelegate does not. Instead, the metrics are computed in the media stream device controller (not infobar delegate): https://cs.chromium.org/chromium/src/chrome/browser/media/media_stream_devices_controller.cc

On desktop, permission ignored metrics are run through PermissionRequestImpl: https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_request_impl.cc.

Granted/Denied/Dismissed metrics are called separated, with a switch/case in both PermissionQueueController and PermissionContextBase. Ideally, the switch should just be inside permission metrics code - the calling code shouldn't have to switch just for the purposes of calling the right metrics method.

OK, that makes sense.

At some point, the new permissions prompt metrics I added will be working on Android as well as desktop. Once they are there is there any reason to keep the old metrics (except for continuity)?

I think doing it at the UI layer is cleaner and avoids some of the problems you've described.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0e576e7a078735d310ff0f66ccca2fbec6373de

commit e0e576e7a078735d310ff0f66ccca2fbec6373de
Author: dominickn <dominickn@chromium.org>
Date: Thu Aug 18 06:04:03 2016

Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop and grouped infobar
   implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG= 638076 

Review-Url: https://codereview.chromium.org/2250053002
Cr-Commit-Position: refs/heads/master@{#412754}

[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/geolocation/geolocation_infobar_delegate_android.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/geolocation/geolocation_infobar_delegate_android.h
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/media/midi_permission_infobar_delegate_android.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/media/midi_permission_infobar_delegate_android.h
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/media/protected_media_identifier_infobar_delegate_android.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/media/protected_media_identifier_infobar_delegate_android.h
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/notifications/notification_permission_infobar_delegate.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/notifications/notification_permission_infobar_delegate.h
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/permissions/permission_infobar_delegate.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/permissions/permission_infobar_delegate.h
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/permissions/permission_queue_controller.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/permissions/permission_util.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/permissions/permission_util.h
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/ui/android/infobars/permission_infobar.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/chrome/browser/ui/android/infobars/permission_infobar.h
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/components/infobars/core/infobar_delegate.cc
[modify] https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de/components/infobars/core/infobar_delegate.h

Dom, do you have an outline of a plan of attack for this?

#c3: I'd definitely like to remove some metrics, but we should have a discussion about what is and isn't useful. :)

#c5: Ideally, we'll first make PermissionUmaUtil provide PermissionGranted/Denied/Dismissed/Ignored/etc. calls that receive all of the necessary information to record every single metric. We shouldn't have to call multiple metrics methods, though we may need to retain the variety of callsites due to special-cases like media permissions.

Then, I want to do a metrics audit. For instance, geolocation is now restricted to HTTPS, so perhaps that is a case for removing the insecure geolocation metrics. Some permissions metrics are empty and never reported: we should put DCHECKs in the code to enforce that those types aren't sent through to metrics, and remove them.

SGTM re #c7.

There are also some metrics that could be marked as temporary (e.g. the prior dismissals can be used to answer the particular question of what n should be for the deny-after-n-dismissals experiment, and then possibly unneeded once that has been determined).

#7: The plan sounds great, but I think it's worth having the discussion about what metrics we want in the long term before cleaning them up. That is, we might not need to clean them up if we can just delete them.

Do you have any ideas about what metrics we could / should remove? I'm not attached to the new ones I added FWIW.

Raymes and I were thinking we should track experiment-specific metrics in the experiment bug and make sure to delete them when the experiment is over.

Should we have a meeting when everyone's back in the country to decide what we want to keep?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6da2b3835157a2178a5951ff42f2e1a7d3404608

commit 6da2b3835157a2178a5951ff42f2e1a7d3404608
Author: dominickn <dominickn@chromium.org>
Date: Tue Aug 23 20:21:30 2016

Add prior dismissal and ignore count metrics for all permission actions.

This CL removes the recently added Permission.Prompt.DismissCount and
Permission.Prompt.IgnoreCount metrics, and replaces them with
equivalents over all permission actions (Accept, Deny, Dismiss, and
Ignore). The metrics are now defined strictly as the number of prompt
dismissals and ignores prior to (rather than inclusive of) the current
action for consistency with permission action reporting. Additional
histogram tests are added to verify that the metrics are recorded.

BUG= 632269 , 638076 

Review-Url: https://codereview.chromium.org/2250993002
Cr-Commit-Position: refs/heads/master@{#413827}

[modify] https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608/chrome/browser/permissions/permission_context_base_unittest.cc
[modify] https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608/chrome/browser/permissions/permission_decision_auto_blocker.cc
[modify] https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608/chrome/browser/permissions/permission_decision_auto_blocker.h
[modify] https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608/chrome/browser/permissions/permission_uma_util.cc
[modify] https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608/chrome/browser/permissions/permission_uma_util.h
[modify] https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b5e0e8356e4d3b095cbae427d8eee7d10db401c

commit 7b5e0e8356e4d3b095cbae427d8eee7d10db401c
Author: kcarattini <kcarattini@chromium.org>
Date: Wed Aug 24 08:04:12 2016

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 

Review-Url: https://codereview.chromium.org/2267353002
Cr-Commit-Position: refs/heads/master@{#414014}

[modify] https://crrev.com/7b5e0e8356e4d3b095cbae427d8eee7d10db401c/chrome/browser/permissions/permission_infobar_delegate.cc
[modify] https://crrev.com/7b5e0e8356e4d3b095cbae427d8eee7d10db401c/chrome/browser/permissions/permission_request_impl.cc
[modify] https://crrev.com/7b5e0e8356e4d3b095cbae427d8eee7d10db401c/chrome/browser/permissions/permission_uma_util.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2fcb04a7eabd49c59a9b25edd7303973d01523b1

commit 2fcb04a7eabd49c59a9b25edd7303973d01523b1
Author: kcarattini <kcarattini@chromium.org>
Date: Fri Sep 02 09:37:23 2016

Permission Action Reporting: Test num_prior* metrics are recorded correctly.

This test ensures that the change in cl/2267353002 is maintained in the correct order.

BUG= 613883 , 638076 

Review-Url: https://codereview.chromium.org/2276253002
Cr-Commit-Position: refs/heads/master@{#416223}

[modify] https://crrev.com/2fcb04a7eabd49c59a9b25edd7303973d01523b1/chrome/browser/safe_browsing/permission_reporter_browsertest.cc

+jww

Hey Joel, there are some metrics you're the owner of that aren't great - Permissions.Requested.SameOrigin and Permissions.Requested.CrossOrigin. I've just expanded the summary for these metrics as what they are measuring isn't very useful / clear. The new summary is:

"   The permission type (geolocation, and such) of a same-origin permission
    request.

    A request is when a website makes a permission request and the user has the
    permission set to prompt (i.e. not blocked or allowed).

    Note this is probably not the metric you want - it does not correspond to
    the total number of times websites request a permission. Also, because
    specific permissions have code that can automatically block or grant
    permissions based on things like incognito, installed extensions etc., this
    does also not correspond to the number of times users are prompted to allow
    permissions.

    See  https://crbug.com/638076  for more details."

Maybe you want to reimplement this metric if you're using it for anything important.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1f026f4dc5b30ceb5061a12057f32c56cc12a1e

commit f1f026f4dc5b30ceb5061a12057f32c56cc12a1e
Author: benwells <benwells@chromium.org>
Date: Thu Sep 29 07:35:35 2016

Update descriptions and owners for permissions metrics.

Some of the descriptions have been updated to be more accurate and
reflect the fact that they are probably not metrics you want to use.

BUG= 625920 ,  638076 

Review-Url: https://codereview.chromium.org/2369273002
Cr-Commit-Position: refs/heads/master@{#421770}

[modify] https://crrev.com/f1f026f4dc5b30ceb5061a12057f32c56cc12a1e/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/423c3044abad2ac900c31cda5b2f368417b6d6e0

commit 423c3044abad2ac900c31cda5b2f368417b6d6e0
Author: dominickn <dominickn@chromium.org>
Date: Wed Oct 05 05:59:34 2016

Make all PermissionDecisionAutoBlocker methods static.

This avoids having to instantiate an object every time RecordIgnore is
called.

PermissionDecisionAutoBlocker::RecordDismiss is also split out from
PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock.

BUG= 638076 

Review-Url: https://codereview.chromium.org/2386193004
Cr-Commit-Position: refs/heads/master@{#423084}

[modify] https://crrev.com/423c3044abad2ac900c31cda5b2f368417b6d6e0/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
[modify] https://crrev.com/423c3044abad2ac900c31cda5b2f368417b6d6e0/chrome/browser/permissions/permission_context_base.cc
[modify] https://crrev.com/423c3044abad2ac900c31cda5b2f368417b6d6e0/chrome/browser/permissions/permission_context_base.h
[modify] https://crrev.com/423c3044abad2ac900c31cda5b2f368417b6d6e0/chrome/browser/permissions/permission_decision_auto_blocker.cc
[modify] https://crrev.com/423c3044abad2ac900c31cda5b2f368417b6d6e0/chrome/browser/permissions/permission_decision_auto_blocker.h
[modify] https://crrev.com/423c3044abad2ac900c31cda5b2f368417b6d6e0/chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc
[modify] https://crrev.com/423c3044abad2ac900c31cda5b2f368417b6d6e0/chrome/browser/permissions/permission_request_impl.cc
[modify] https://crrev.com/423c3044abad2ac900c31cda5b2f368417b6d6e0/chrome/browser/permissions/permission_uma_util.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5a506a917f583f5aa6e32231f8a7df563dfe170

commit f5a506a917f583f5aa6e32231f8a7df563dfe170
Author: dominickn <dominickn@chromium.org>
Date: Fri Feb 24 03:36:49 2017

Remove deprecated and out of date permissions metrics.

This CL removes:

 - deprecated permissions RAPPOR metrics
 - secure/insecure origin metrics for geolocation, push, durable storage,
   and MidiSysEx (as they are restricted to secure origins)

BUG= 605836 , 638076 

Review-Url: https://codereview.chromium.org/2711513005
Cr-Commit-Position: refs/heads/master@{#452741}

[modify] https://crrev.com/f5a506a917f583f5aa6e32231f8a7df563dfe170/chrome/browser/permissions/permission_uma_util.cc
[modify] https://crrev.com/f5a506a917f583f5aa6e32231f8a7df563dfe170/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/669662b980b9fc6e91c8c525e6a0f4e14ab002c5

commit 669662b980b9fc6e91c8c525e6a0f4e14ab002c5
Author: dominickn <dominickn@chromium.org>
Date: Mon Apr 03 04:04:48 2017

Remove static_casts from PermissionUmaUtil histogram calls.

Histograms that use enum classes no longer require recorded values to be
static_cast'd to int. This CL removes static_cast<int> calls in
permissions code.

Histogram tests still require explicit cast out of enum classes, so
these are not removed by this CL.

BUG= 638076 

Review-Url: https://codereview.chromium.org/2791123002
Cr-Commit-Position: refs/heads/master@{#461369}

[modify] https://crrev.com/669662b980b9fc6e91c8c525e6a0f4e14ab002c5/chrome/browser/permissions/permission_uma_util.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0e67aadd0a50676f65e8cca7443df1d9245db3ab

commit 0e67aadd0a50676f65e8cca7443df1d9245db3ab
Author: Timothy Loh <timloh@chromium.org>
Date: Wed Nov 08 00:45:10 2017

Remove histograms for grouped permission bubbles

In M61 we restricted permissions to only group Mic+Camera, which made
the histograms for grouped permission bubbles (which permissions were
grouped together, how many were grouped) redundant. The same data can
be readily obtained from Permissions.Prompt.(Accepted/Denied/Shown).

Bug:  728483 ,  638076 
Change-Id: Iab49387d0b0d86e3e2a269cfa94f9072f1db95bb
Reviewed-on: https://chromium-review.googlesource.com/756633
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514680}
[modify] https://crrev.com/0e67aadd0a50676f65e8cca7443df1d9245db3ab/chrome/browser/permissions/permission_request_manager_unittest.cc
[modify] https://crrev.com/0e67aadd0a50676f65e8cca7443df1d9245db3ab/chrome/browser/permissions/permission_uma_util.cc
[modify] https://crrev.com/0e67aadd0a50676f65e8cca7443df1d9245db3ab/chrome/browser/permissions/permission_uma_util.h
[modify] https://crrev.com/0e67aadd0a50676f65e8cca7443df1d9245db3ab/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d921e43a05163b74853f81c581756b58a98e167b

commit d921e43a05163b74853f81c581756b58a98e167b
Author: Timothy Loh <timloh@chromium.org>
Date: Fri Nov 10 06:19:02 2017

Remove unused Permissions.*.DurableStorage histograms

The durable storage permission is automatically granted or denied based
on various heuristics and we never prompt the user for permission. This
patch removes this permission from histograms which are only logged for
permissions that prompt.

Bug:  638076 
Change-Id: I7b421f87b5eb2f6fcb0527a90efe1b7908842e4d
Reviewed-on: https://chromium-review.googlesource.com/756476
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515468}
[modify] https://crrev.com/d921e43a05163b74853f81c581756b58a98e167b/chrome/browser/permissions/permission_uma_util.cc
[modify] https://crrev.com/d921e43a05163b74853f81c581756b58a98e167b/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92a6b8c86eacd2808d23a761f62f4ba309ce900a

commit 92a6b8c86eacd2808d23a761f62f4ba309ce900a
Author: Timothy Loh <timloh@chromium.org>
Date: Fri Jan 12 06:49:54 2018

Tidy up UMA for permission prompt metrics

This patch tidies up logging for permission prompt actions, previously
split across the PermissionRequestManager and PermissionContextBase, to
now only happen in the PRM. This also removes 6 methods from the public
interface to PermissionUmaUtil, with the prompt resolution methods
consolidated into PermissionPromptResolved. We also simplify the logic
by building histogram names for prior dismissals and ignores in code
instead of hard-coding them.

Bug:  638076 
Change-Id: I0222dec2aa588b1791e3ca410934ad242952533d
Reviewed-on: https://chromium-review.googlesource.com/858984
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528898}
[modify] https://crrev.com/92a6b8c86eacd2808d23a761f62f4ba309ce900a/chrome/browser/permissions/permission_context_base.cc
[modify] https://crrev.com/92a6b8c86eacd2808d23a761f62f4ba309ce900a/chrome/browser/permissions/permission_context_base.h
[modify] https://crrev.com/92a6b8c86eacd2808d23a761f62f4ba309ce900a/chrome/browser/permissions/permission_request_manager.cc
[modify] https://crrev.com/92a6b8c86eacd2808d23a761f62f4ba309ce900a/chrome/browser/permissions/permission_uma_util.cc
[modify] https://crrev.com/92a6b8c86eacd2808d23a761f62f4ba309ce900a/chrome/browser/permissions/permission_uma_util.h

Over to timloh@; I think this is mostly done now, but maybe there are some more cleanups that will happen as a part of Crowd Consent. :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/353e0f6e4f22e4d363c9347cb1451c46c6e28fa0

commit 353e0f6e4f22e4d363c9347cb1451c46c6e28fa0
Author: Timothy Loh <timloh@chromium.org>
Date: Thu Jan 25 05:52:48 2018

Remove unused PermissionAction::(REENABLED/REQUESTED)

The enums PermissionAction::REENABLED and PermissionAction::REQUESTED
don't seem to have ever been used, so this patch removes them.

TBR=nparker

Bug:  638076 
Change-Id: Iadcbc6f44a8c33233b702287167baef56c807699
Reviewed-on: https://chromium-review.googlesource.com/867794
Commit-Queue: Timothy Loh <timloh@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531822}
[modify] https://crrev.com/353e0f6e4f22e4d363c9347cb1451c46c6e28fa0/chrome/browser/permissions/permission_util.h
[modify] https://crrev.com/353e0f6e4f22e4d363c9347cb1451c46c6e28fa0/chrome/browser/safe_browsing/permission_reporter.cc

I'm sure we could find more things to clean up, but we probably don't need to keep this open.