Permission Metrics: Switch the order of RecordIgnore and PermissionIgnored

chrome/browser/permissions/permission_request_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/permissions/permission_request_impl.h"
 
 #include "build/build_config.h"
 #include "chrome/browser/permissions/permission_decision_auto_blocker.h"
 #include "chrome/browser/permissions/permission_uma_util.h"
 #include "chrome/grit/generated_resources.h"
@@ skipping 15 matching lines @@
       profile_(profile),
       has_gesture_(has_gesture),
       permission_decided_callback_(permission_decided_callback),
       delete_callback_(delete_callback),
       is_finished_(false),
       action_taken_(false) {}
 
 PermissionRequestImpl::~PermissionRequestImpl() {
   DCHECK(is_finished_);
   if (!action_taken_) {
+    // PermissionIgnored needs to be called before RecordIgnore in the blocker
+    // because it gets the number of prior ignore and dismiss values from the
+    // blocker and we don't want to include the current ignore.
+    PermissionUmaUtil::PermissionIgnored(permission_type_, GetGestureType(),
+                                         request_origin_, profile_);
+
     PermissionDecisionAutoBlocker(profile_).RecordIgnore(request_origin_,
                                                          permission_type_);

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

Could we instead move this into PermissionUmaUtil::PermissionIgnored?

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

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]

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:58]

Yes, that's what I meant :).

-
-    PermissionUmaUtil::PermissionIgnored(permission_type_, GetGestureType(),
-                                         request_origin_, profile_);
   }
 }
 
 gfx::VectorIconId PermissionRequestImpl::GetVectorIconId() const {
 #if !defined(OS_MACOSX) && !defined(OS_ANDROID)
   switch (permission_type_) {
     case content::PermissionType::GEOLOCATION:
       return gfx::VectorIconId::LOCATION_ON;
 #if defined(ENABLE_NOTIFICATIONS)
     case content::PermissionType::NOTIFICATIONS:
@@ skipping 111 matching lines @@
       NOTREACHED();
       return PermissionRequestType::UNKNOWN;
   }
 }
 
 PermissionRequestGestureType PermissionRequestImpl::GetGestureType()
     const {
   return has_gesture_ ? PermissionRequestGestureType::GESTURE
                       : PermissionRequestGestureType::NO_GESTURE;
 }