Log site engagement scores for permission actions

chrome/browser/permissions/permission_request_manager.cc

Index: chrome/browser/permissions/permission_request_manager.cc
diff --git a/chrome/browser/permissions/permission_request_manager.cc b/chrome/browser/permissions/permission_request_manager.cc
index 678c50aaa8e7b08794bf46c73d80abd639cf6379..34efa40a2b776fc165b06a025a47c2e7160891aa 100644
--- a/chrome/browser/permissions/permission_request_manager.cc
+++ b/chrome/browser/permissions/permission_request_manager.cc
@@ -31,7 +31,8 @@ class CancelledRequest : public PermissionRequest {
         message_(cancelled->GetMessageText()),
 #endif
         message_fragment_(cancelled->GetMessageTextFragment()),
-        origin_(cancelled->GetOrigin()) {
+        origin_(cancelled->GetOrigin()),
+        request_type_(cancelled->GetPermissionRequestType()) {
   }
   ~CancelledRequest() override {}
 
@@ -51,6 +52,10 @@ class CancelledRequest : public PermissionRequest {
 
   void RequestFinished() override { delete this; }
 
+  PermissionRequestType GetPermissionRequestType() const override {
+    return request_type_;
+  }
+
  private:
   IconId icon_;
 #if defined(OS_ANDROID)
@@ -58,6 +63,7 @@ class CancelledRequest : public PermissionRequest {
 #endif
   base::string16 message_fragment_;
   GURL origin_;
+  PermissionRequestType request_type_;
 };
 
 bool IsMessageTextEqual(PermissionRequest* a,
@@ -187,35 +193,23 @@ void PermissionRequestManager::CancelRequest(PermissionRequest* request) {
     }
   }
 
-  std::vector<PermissionRequest*>::iterator requests_iter;
-  for (requests_iter = requests_.begin(); requests_iter != requests_.end();
-       requests_iter++) {
-    if (*requests_iter != request)
-      continue;
+  if (!requests_.empty() && requests_[0] == request) {
+    // TODO(timloh): We should fix this at some point.
+    // Grouped (mic+camera) requests are currently never cancelled.

[raymes, 2017/07/20 05:21:17]

nit: these comments are probably in the reverse order

[Timothy Loh, 2017/07/20 06:09:33]

Done.

+    DCHECK_EQ(static_cast<size_t>(1), requests_.size());

[raymes, 2017/07/20 05:21:17]

nit: 1u?

optional: may want to move this up above the if-statement (and check that it's 0
or 1) just so it's checked regardless of whether the condition is matched

[Timothy Loh, 2017/07/20 06:09:33]

Don't think 1u works on 64 bit platforms? But we can get into a state where we
have mic+camera showing but we cancel a different request, so I can't move the
check up.

 
-    // We can simply erase the current entry in the request table if we aren't
-    // showing the dialog (because we switched tabs with an active prompt), or
-    // if we are showing it and it can accept the update.
+    // We can finalize the prompt if we aren't showing the dialog (because we
+    // switched tabs with an active prompt), or if we are showing it and it
+    // can accept the update.
     if (!view_ || view_->CanAcceptRequestUpdate()) {
-      // TODO(timloh): We should fix this at some point.
-      // Grouped (mic+camera) requests are currently never cancelled.
-      DCHECK_EQ(static_cast<size_t>(1), requests_.size());
-
-      if (view_)
-        DeleteBubble();
-
-      RequestFinishedIncludingDuplicates(*requests_iter);
-      requests_.erase(requests_iter);
-
-      DequeueRequestsAndShowBubble();
+      FinalizeBubble(PermissionAction::IGNORED);
       return;
     }
 
     // Cancel the existing request and replace it with a dummy.
-    PermissionRequest* cancelled_request =
-        new CancelledRequest(*requests_iter);
-    RequestFinishedIncludingDuplicates(*requests_iter);
-    *requests_iter = cancelled_request;
+    PermissionRequest* cancelled_request = new CancelledRequest(request);
+    RequestFinishedIncludingDuplicates(request);
+    requests_[0] = cancelled_request;
     return;
   }
 
@@ -329,26 +323,22 @@ void PermissionRequestManager::TogglePersist(bool new_value) {
 }
 
 void PermissionRequestManager::Accept() {
-  PermissionUmaUtil::PermissionPromptAccepted(requests_);
-
   std::vector<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin(); requests_iter != requests_.end();
        requests_iter++) {
     PermissionGrantedIncludingDuplicates(*requests_iter);
   }
-  FinalizeBubble();
+  FinalizeBubble(PermissionAction::GRANTED);
 }
 
 void PermissionRequestManager::Deny() {
-  PermissionUmaUtil::PermissionPromptDenied(requests_);
-
   std::vector<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
     PermissionDeniedIncludingDuplicates(*requests_iter);
   }
-  FinalizeBubble();
+  FinalizeBubble(PermissionAction::DENIED);
 }
 
 void PermissionRequestManager::Closing() {
@@ -357,13 +347,14 @@ void PermissionRequestManager::Closing() {
   if (!view_)
     return;
 #endif
+
   std::vector<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
     CancelledIncludingDuplicates(*requests_iter);
   }
-  FinalizeBubble();
+  FinalizeBubble(PermissionAction::DISMISSED);
 }
 
 void PermissionRequestManager::ScheduleShowBubble() {
@@ -419,11 +410,15 @@ void PermissionRequestManager::DeleteBubble() {
   view_.reset();
 }
 
-void PermissionRequestManager::FinalizeBubble() {
-  DCHECK(view_);

[raymes, 2017/07/20 05:21:17]

nit: I think it would be useful to hoist this DCHECK up into the callsites of
this function where it still holds.

[Timothy Loh, 2017/07/20 06:09:33]

Done.

+void PermissionRequestManager::FinalizeBubble(
+    PermissionAction permission_action) {
   DCHECK(!requests_.empty());
 
-  DeleteBubble();
+  if (view_)
+    DeleteBubble();
+
+  PermissionUmaUtil::PermissionPromptResolved(requests_, web_contents(),
+                                              permission_action);
 
   std::vector<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
@@ -446,7 +441,7 @@ void PermissionRequestManager::CleanUpRequests() {
   queued_requests_.clear();
 
   if (view_)
-    FinalizeBubble();
+    FinalizeBubble(PermissionAction::IGNORED);
 }
 
 PermissionRequest* PermissionRequestManager::GetExistingRequest(