Make PermissionRequestManager::requests_ correspond to the active prompt

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 2a7cf3ac5729cd357ac67051284a3872ce8cc374..91d1a66031e5459499cd9e25a569cd3c72fe6eff 100644
--- a/chrome/browser/permissions/permission_request_manager.cc
+++ b/chrome/browser/permissions/permission_request_manager.cc
@@ -133,29 +133,20 @@ void PermissionRequestManager::AddRequest(PermissionRequest* request) {
     return;
   }
 
-  if (IsBubbleVisible()) {
-    if (is_main_frame) {
+  if (is_main_frame) {
+    if (IsBubbleVisible()) {
       base::RecordAction(
           base::UserMetricsAction("PermissionBubbleRequestQueued"));
-      queued_requests_.push_back(request);
-    } else {
-      base::RecordAction(
-          base::UserMetricsAction("PermissionBubbleIFrameRequestQueued"));
-      queued_frame_requests_.push_back(request);
     }
-    return;
-  }
-
-  if (is_main_frame) {
-    requests_.push_back(request);
-    accept_states_.push_back(true);
+    queued_requests_.push_back(request);
   } else {
     base::RecordAction(
         base::UserMetricsAction("PermissionBubbleIFrameRequestQueued"));

[raymes, 2017/05/04 05:00:04]

this used to only happen if IsBubbleVisible. Should that still be the case now?

[Timothy Loh, 2017/05/04 08:22:30]

Logic shouldn't be changed here, it was being called both L142 and L153. It's
inconsistent with the non-frame case but at least it's more obvious now.

     queued_frame_requests_.push_back(request);
   }
 
-  ScheduleShowBubble();
+  if (!IsBubbleVisible())
+    ScheduleShowBubble();
 }
 
 void PermissionRequestManager::CancelRequest(PermissionRequest* request) {
@@ -189,13 +180,13 @@ void PermissionRequestManager::CancelRequest(PermissionRequest* request) {
 
     // 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();
+    bool can_erase = !view_ || view_->CanAcceptRequestUpdate();
     if (can_erase) {
       RequestFinishedIncludingDuplicates(*requests_iter);
       requests_.erase(requests_iter);
       accept_states_.erase(accepts_iter);
 
-      if (IsBubbleVisible()) {
+      if (view_) {
         view_->Hide();
         // Will redraw the bubble if it is being shown.
         TriggerShowBubble();
@@ -246,6 +237,16 @@ void PermissionRequestManager::DisplayPendingRequests() {
   view_ = view_factory_.Run(web_contents());
   view_->SetDelegate(this);
 
+  if (!main_frame_has_fully_loaded_)
+    return;
+  if (!requests_.empty()) {
+    // The previous prompt was hidden due to tab switching.
+    view_->Show(requests_, accept_states_);
+    PermissionUmaUtil::PermissionPromptShown(requests_);
+    NotifyBubbleAdded();
+    return;
+  }

[raymes, 2017/05/04 05:00:05]

I think it could be better to move this into a separate function. Maybe have
ShowBubble()
TriggerShowBubble->PopRequestsAndShowBubble

[Timothy Loh, 2017/05/04 08:22:30]

Done.

+
   TriggerShowBubble();
 }
 
@@ -255,7 +256,7 @@ void PermissionRequestManager::UpdateAnchorPosition() {
 }
 
 bool PermissionRequestManager::IsBubbleVisible() {
-  return view_ && view_->IsVisible();
+  return view_ && !requests_.empty();
 }
 
 // static
@@ -377,24 +378,20 @@ void PermissionRequestManager::ScheduleShowBubble() {
 void PermissionRequestManager::TriggerShowBubble() {
   if (!view_)
     return;
-  if (IsBubbleVisible())
+  if (!requests_.empty())
     return;

[raymes, 2017/05/04 05:00:04]

Why not get rid of the above 2 checks and just call IsBubbleVisible here?

[Timothy Loh, 2017/05/04 08:22:30]

Just checking IsBubbleVisible won't exit the !view case. I can leave it as a
IsBubbleVisible() call but I think it's more obvious like this.

   if (!main_frame_has_fully_loaded_)
     return;
-  if (requests_.empty() && queued_requests_.empty() &&
-      queued_frame_requests_.empty()) {
+  if (queued_requests_.empty() && queued_frame_requests_.empty())
     return;
-  }
 
-  if (requests_.empty()) {
-    if (queued_requests_.size())
-      requests_.swap(queued_requests_);
-    else
-      requests_.swap(queued_frame_requests_);
+  if (queued_requests_.size())
+    requests_.swap(queued_requests_);
+  else
+    requests_.swap(queued_frame_requests_);
 
-    // Sets the default value for each request to be 'accept'.
-    accept_states_.resize(requests_.size(), true);
-  }
+  // Sets the default value for each request to be 'accept'.
+  accept_states_.resize(requests_.size(), true);
 
   view_->Show(requests_, accept_states_);
   PermissionUmaUtil::PermissionPromptShown(requests_);