Fix cancelling permission requests on Android when the PermissionRequestManager is enabled

chrome/browser/permissions/permission_request_manager.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_manager.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
@@ skipping 170 matching lines @@
   }
 
   std::vector<bool>::iterator accepts_iter = accept_states_.begin();
   for (requests_iter = requests_.begin(), accepts_iter = accept_states_.begin();
        requests_iter != requests_.end();
        requests_iter++, accepts_iter++) {
     if (*requests_iter != request)
       continue;
 
     // We can simply erase the current entry in the request table if we aren't
-    // showing the dialog, or if we are showing it and it can accept the update.
-    bool can_erase = !IsBubbleVisible() || view_->CanAcceptRequestUpdate();
-    if (can_erase) {
+    // showing the dialog
+    if (!IsBubbleVisible()) {
       RequestFinishedIncludingDuplicates(*requests_iter);
       requests_.erase(requests_iter);
       accept_states_.erase(accepts_iter);
-
-      if (IsBubbleVisible()) {
-        view_->Hide();
-        // Will redraw the bubble if it is being shown.
-        TriggerShowBubble();
-      }
-      return;
+    } else if (view_->MaybeCancelRequest()) {

[raymes, 2017/05/01 04:23:35]

I think we could document each of these cases better. Firstly, maybe we should
talk about hiding the prompt here rather than cancelling the request, because
this won't actually modify the request? Secondly, the prompt will be hidden if
the user isn't interacting with the prompt. So maybe we should call the function
something like:
HideIfUserNotInteracting ?

+      RequestFinishedIncludingDuplicates(*requests_iter);
+      requests_.erase(requests_iter);
+      accept_states_.erase(accepts_iter);
+      TriggerShowBubble();
+    } else {
+      // Cancel the existing request and replace it with a dummy.

[raymes, 2017/05/01 04:23:35]

I think we can clarify this comment a bit.
// If the user is still interacting with the prompt, we don't 
// hide it but we still cancel the permission request and replace
// it with a dummy request that will do nothing when the permission
// prompt is answered.

+      PermissionRequest* cancelled_request =
+          new CancelledRequest(*requests_iter);
+      RequestFinishedIncludingDuplicates(*requests_iter);
+      *requests_iter = cancelled_request;
+      view_->Show(requests_, accept_states_);

[raymes, 2017/05/01 04:23:35]

As discussed I think it would be preferable to allow access to the vector of
requests via the delegate, which would remove the need for calling Show() again
and also ensure that we don't have the 2 lists getting out of sync,

     }
-
-    // Cancel the existing request and replace it with a dummy.
-    PermissionRequest* cancelled_request =
-        new CancelledRequest(*requests_iter);
-    RequestFinishedIncludingDuplicates(*requests_iter);
-    *requests_iter = cancelled_request;
     return;
   }
 
   // Since |request| wasn't found in queued_requests_, queued_frame_requests_ or
   // requests_ it must have been marked as a duplicate. We can't search
   // duplicate_requests_ by value, so instead use GetExistingRequest to find the
   // key (request it was duped against), and iterate through duplicates of that.
   PermissionRequest* existing_request = GetExistingRequest(request);
   auto range = duplicate_requests_.equal_range(existing_request);
   for (auto it = range.first; it != range.second; ++it) {
@@ skipping 305 matching lines @@
     case DENY_ALL:
       Deny();
       break;
     case DISMISS:
       Closing();
       break;
     case NONE:
       NOTREACHED();
   }
 }