Make PermissionRequestManager::requests_ correspond to the active prompt

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 115 matching lines @@
       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;
   }
 
-  if (IsBubbleVisible()) {
-    if (is_main_frame) {
-      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);
+    if (IsBubbleVisible()) {
+      base::RecordAction(
+          base::UserMetricsAction("PermissionBubbleRequestQueued"));
+    }
+    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) {
   // First look in the queued requests, where we can simply finish the request
   // and go on.
   std::vector<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = queued_requests_.begin();
        requests_iter != queued_requests_.end();
        requests_iter++) {
     if (*requests_iter == request) {
@@ skipping 13 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();
+    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();
       }
       return;
     }
 
     // Cancel the existing request and replace it with a dummy.
     PermissionRequest* cancelled_request =
         new CancelledRequest(*requests_iter);
@@ skipping 30 matching lines @@
   view_.reset();
 }
 
 void PermissionRequestManager::DisplayPendingRequests() {
   if (IsBubbleVisible())
     return;
 
   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();
 }
 
 void PermissionRequestManager::UpdateAnchorPosition() {
   if (view_)
     view_->UpdateAnchorPosition();
 }
 
 bool PermissionRequestManager::IsBubbleVisible() {
-  return view_ && view_->IsVisible();
+  return view_ && !requests_.empty();
 }
 
 // static
 bool PermissionRequestManager::IsEnabled() {
 #if defined(OS_ANDROID)
   return base::FeatureList::IsEnabled(features::kUseGroupedPermissionInfobars);
 #else
   return true;
 #endif
 }
@@ skipping 101 matching lines @@
 
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::BindOnce(&PermissionRequestManager::TriggerShowBubble,
                      weak_factory_.GetWeakPtr()));
 }
 
 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_);
   NotifyBubbleAdded();
 
   // If in testing mode, automatically respond to the bubble that was shown.
   if (auto_response_for_test_ != NONE)
     DoAutoResponseForTesting();
 }
 
@@ skipping 118 matching lines @@
     case DENY_ALL:
       Deny();
       break;
     case DISMISS:
       Closing();
       break;
     case NONE:
       NOTREACHED();
   }
 }