Fixes Permission Bubbles never responding to duplicate requests

chrome/browser/ui/website_settings/permission_bubble_manager.cc

Index: chrome/browser/ui/website_settings/permission_bubble_manager.cc
diff --git a/chrome/browser/ui/website_settings/permission_bubble_manager.cc b/chrome/browser/ui/website_settings/permission_bubble_manager.cc
index 50f2a4fb1800385fa79f29eb3455ce025ff25afa..6654cab024147794c4aac25c09954ace3bf87b3e 100644
--- a/chrome/browser/ui/website_settings/permission_bubble_manager.cc
+++ b/chrome/browser/ui/website_settings/permission_bubble_manager.cc
@@ -83,17 +83,14 @@ PermissionBubbleManager::~PermissionBubbleManager() {
   if (view_ != NULL)
     view_->SetDelegate(NULL);
 
-  std::vector<PermissionBubbleRequest*>::iterator requests_iter;
-  for (requests_iter = requests_.begin();
-       requests_iter != requests_.end();
-       requests_iter++) {
-    (*requests_iter)->RequestFinished();
-  }
-  for (requests_iter = queued_requests_.begin();
-       requests_iter != queued_requests_.end();
-       requests_iter++) {
-    (*requests_iter)->RequestFinished();
-  }
+  for (PermissionBubbleRequest* request : requests_)
+    request->RequestFinished();
+  for (PermissionBubbleRequest* request : queued_requests_)
+    request->RequestFinished();
+  for (PermissionBubbleRequest* request : queued_frame_requests_)

[johnme, 2016/01/20 19:29:04]

n.b. queued_frame_requests_ was missing here before. Adding it is orthogonal to
this patch, but seemed useful.

+    request->RequestFinished();
+  for (const auto& entry : duplicate_requests_)
+    entry.second->RequestFinished();
 }
 
 void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
@@ -110,15 +107,18 @@ void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
           .IsSameOriginWith(url::Origin(request->GetRequestingHostname()));
 
   // Don't re-add an existing request or one with a duplicate text request.
-  // TODO(johnme): Instead of dropping duplicate requests, we should queue them
-  // and eventually run their PermissionGranted/PermissionDenied/Cancelled
-  // callback (crbug.com/577313).
-  bool same_object = false;
-  if (ExistingRequest(request, requests_, &same_object) ||
-      ExistingRequest(request, queued_requests_, &same_object) ||
-      ExistingRequest(request, queued_frame_requests_, &same_object)) {
-    if (!same_object)
-      request->RequestFinished();
+  PermissionBubbleRequest* existing_request = GetExistingRequest(request);
+  if (existing_request) {
+    // |request| is a duplicate. Add it to |duplicate_requests_| unless it's the
+    // same object as |existing_request| or an existing duplicate.
+    if (request == existing_request)
+      return;
+    auto range = duplicate_requests_.equal_range(existing_request);
+    for (auto it = range.first; it != range.second; ++it) {
+      if (request == it->second)
+        return;
+    }
+    duplicate_requests_.insert(std::make_pair(existing_request, request));
     return;
   }
 
@@ -149,18 +149,26 @@ void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
 }
 
 void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
-  // First look in the queued requests, where we can simply delete the request
+  // First look in the queued requests, where we can simply finish the request
   // and go on.
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = queued_requests_.begin();
        requests_iter != queued_requests_.end();
        requests_iter++) {
     if (*requests_iter == request) {
-      (*requests_iter)->RequestFinished();
+      RequestFinishedIncludingDuplicates(*requests_iter);
       queued_requests_.erase(requests_iter);
       return;
     }
   }
+  for (requests_iter = queued_frame_requests_.begin();

[johnme, 2016/01/20 19:29:04]

n.b. queued_frame_requests_ was missing here before. Adding it is orthogonal to
this patch, but seemed useful.

+       requests_iter != queued_frame_requests_.end(); requests_iter++) {
+    if (*requests_iter == request) {
+      RequestFinishedIncludingDuplicates(*requests_iter);
+      queued_frame_requests_.erase(requests_iter);
+      return;
+    }
+  }
 
   std::vector<bool>::iterator accepts_iter = accept_states_.begin();
   for (requests_iter = requests_.begin(), accepts_iter = accept_states_.begin();
@@ -173,7 +181,7 @@ void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
     // showing the dialog, or if we are showing it and it can accept the update.
     bool can_erase = !IsBubbleVisible() || view_->CanAcceptRequestUpdate();
     if (can_erase) {
-      (*requests_iter)->RequestFinished();
+      RequestFinishedIncludingDuplicates(*requests_iter);
       requests_.erase(requests_iter);
       accept_states_.erase(accepts_iter);
 
@@ -188,11 +196,22 @@ void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
     // Cancel the existing request and replace it with a dummy.
     PermissionBubbleRequest* cancelled_request =
         new CancelledRequest(*requests_iter);
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
     *requests_iter = cancelled_request;
     return;
   }
 
+  // Finally look through the duplicate requests, which can simply be finished.
+  PermissionBubbleRequest* existing_request = GetExistingRequest(request);
+  auto range = duplicate_requests_.equal_range(existing_request);
+  for (auto it = range.first; it != range.second; ++it) {
+    if (request == it->second) {
+      it->second->RequestFinished();
+      duplicate_requests_.erase(it);
+      return;
+    }
+  }
+
   NOTREACHED();  // Callers should not cancel requests that are not pending.
 }
 
@@ -286,10 +305,11 @@ void PermissionBubbleManager::Accept() {
   for (requests_iter = requests_.begin(), accepts_iter = accept_states_.begin();
        requests_iter != requests_.end();
        requests_iter++, accepts_iter++) {
-    if (*accepts_iter)
-      (*requests_iter)->PermissionGranted();
-    else
-      (*requests_iter)->PermissionDenied();
+    if (*accepts_iter) {
+      PermissionGrantedIncludingDuplicates(*requests_iter);
+    } else {
+      PermissionDeniedIncludingDuplicates(*requests_iter);
+    }
   }
   FinalizeBubble();
 }
@@ -299,7 +319,7 @@ void PermissionBubbleManager::Deny() {
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->PermissionDenied();
+    PermissionDeniedIncludingDuplicates(*requests_iter);
   }
   FinalizeBubble();
 }
@@ -309,7 +329,7 @@ void PermissionBubbleManager::Closing() {
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->Cancelled();
+    CancelledIncludingDuplicates(*requests_iter);
   }
   FinalizeBubble();
 }
@@ -373,7 +393,7 @@ void PermissionBubbleManager::FinalizeBubble() {
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
   }
   requests_.clear();
   accept_states_.clear();
@@ -388,38 +408,42 @@ void PermissionBubbleManager::CancelPendingQueues() {
   for (requests_iter = queued_requests_.begin();
        requests_iter != queued_requests_.end();
        requests_iter++) {
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
   }
   for (requests_iter = queued_frame_requests_.begin();
        requests_iter != queued_frame_requests_.end();
        requests_iter++) {
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
   }
   queued_requests_.clear();
   queued_frame_requests_.clear();
 }
 
-bool PermissionBubbleManager::ExistingRequest(
-    PermissionBubbleRequest* request,
-    const std::vector<PermissionBubbleRequest*>& queue,
-    bool* same_object) {
-  CHECK(same_object);
-  *same_object = false;
-  std::vector<PermissionBubbleRequest*>::const_iterator iter;
-  for (iter = queue.begin(); iter != queue.end(); iter++) {
-    if (*iter == request) {
-      *same_object = true;
-      return true;
-    }
-    if ((*iter)->GetMessageTextFragment() ==
-            request->GetMessageTextFragment() &&
-        (*iter)->GetRequestingHostname() == request->GetRequestingHostname()) {
-      return true;
-    }
+bool MessageTextIsEqual(PermissionBubbleRequest* a,
+                        PermissionBubbleRequest* b) {
+  if (a == b)
+    return true;
+  if (a->GetMessageTextFragment() == b->GetMessageTextFragment() &&
+      a->GetRequestingHostname() == b->GetRequestingHostname()) {
+    return true;
   }
   return false;
 }
 
+PermissionBubbleRequest* PermissionBubbleManager::GetExistingRequest(
+    PermissionBubbleRequest* request) {
+  for (PermissionBubbleRequest* existing_request : requests_)
+    if (MessageTextIsEqual(existing_request, request))
+      return existing_request;
+  for (PermissionBubbleRequest* existing_request : queued_requests_)
+    if (MessageTextIsEqual(existing_request, request))
+      return existing_request;
+  for (PermissionBubbleRequest* existing_request : queued_frame_requests_)
+    if (MessageTextIsEqual(existing_request, request))
+      return existing_request;
+  return nullptr;
+}
+
 bool PermissionBubbleManager::HasUserGestureRequest(
     const std::vector<PermissionBubbleRequest*>& queue) {
   std::vector<PermissionBubbleRequest*>::const_iterator iter;
@@ -430,6 +454,45 @@ bool PermissionBubbleManager::HasUserGestureRequest(
   return false;
 }
 
+void PermissionBubbleManager::PermissionGrantedIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  DCHECK_EQ(request, GetExistingRequest(request))
+      << "Request must not be a duplicate";
+  request->PermissionGranted();
+  auto range = duplicate_requests_.equal_range(request);
+  for (auto it = range.first; it != range.second; ++it)
+    it->second->PermissionGranted();
+}
+void PermissionBubbleManager::PermissionDeniedIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  DCHECK_EQ(request, GetExistingRequest(request))
+      << "Request must not be a duplicate";
+  request->PermissionDenied();
+  auto range = duplicate_requests_.equal_range(request);
+  for (auto it = range.first; it != range.second; ++it)
+    it->second->PermissionDenied();
+}
+void PermissionBubbleManager::CancelledIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  DCHECK_EQ(request, GetExistingRequest(request))
+      << "Request must not be a duplicate";
+  request->Cancelled();
+  auto range = duplicate_requests_.equal_range(request);
+  for (auto it = range.first; it != range.second; ++it)
+    it->second->Cancelled();
+}
+void PermissionBubbleManager::RequestFinishedIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  DCHECK_EQ(request, GetExistingRequest(request))
+      << "Request must not be a duplicate";
+  request->RequestFinished();
+  auto range = duplicate_requests_.equal_range(request);
+  for (auto it = range.first; it != range.second; ++it)
+    it->second->RequestFinished();
+  // Additionally, we can now remove the duplicates.
+  duplicate_requests_.erase(request);
+}
+
 void PermissionBubbleManager::AddObserver(Observer* observer) {
   observer_list_.AddObserver(observer);
 }