Allow duplicate infobar requests

chrome/browser/content_settings/permission_queue_controller.cc

 // Copyright 2013 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/content_settings/permission_queue_controller.h"
 
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/geolocation/geolocation_infobar_delegate.h"
@@ skipping 135 matching lines @@
 }
 
 void PermissionQueueController::CreateInfoBarRequest(
     const PermissionRequestID& id,
     const GURL& requesting_frame,
     const GURL& embedder,
     const std::string& accept_button_label,
     PermissionDecidedCallback callback) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  // We shouldn't get duplicate requests.
-  for (PendingInfobarRequests::const_iterator i(
-           pending_infobar_requests_.begin());
-       i != pending_infobar_requests_.end(); ++i)
-    DCHECK(!i->id().Equals(id));
-
   pending_infobar_requests_.push_back(PendingInfobarRequest(
       type_, id, requesting_frame, embedder,
       accept_button_label, callback));
   if (!AlreadyShowingInfoBarForTab(id))
     ShowQueuedInfoBarForTab(id);
 }
 
 void PermissionQueueController::CancelInfoBarRequest(
     const PermissionRequestID& id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ skipping 10 matching lines @@
   }
 }
 
 void PermissionQueueController::OnPermissionSet(
     const PermissionRequestID& id,
     const GURL& requesting_frame,
     const GURL& embedder,
     bool update_content_setting,
     bool allowed) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-
   if (update_content_setting)
     UpdateContentSetting(requesting_frame, embedder, allowed);
 
   // Cancel this request first, then notify listeners.  TODO(pkasting): Why
   // is this order important?
   PendingInfobarRequests requests_to_notify;
   PendingInfobarRequests infobars_to_remove;
   for (PendingInfobarRequests::iterator i = pending_infobar_requests_.begin();
        i != pending_infobar_requests_.end(); ) {
     if (i->IsForPair(requesting_frame, embedder)) {

[xhwang, 2014/06/10 21:48:39]

nit: See below about using a normal for loop. If you do that, you can return
early here:

if (!foo)
  continue;

bar...

       requests_to_notify.push_back(*i);
+      bool infobar_created = false;
       if (i->id().Equals(id)) {
         // The infobar that called us is i->infobar(), and its delegate is
         // currently in either Accept() or Cancel(). This means that
         // RemoveInfoBar() will be called later on, and that will trigger a
         // notification we're observing.
-        ++i;
-      } else if (i->has_infobar()) {
-        // This infobar is for the same frame/embedder pair, but in a different
-        // tab. We should remove it now that we've got an answer for it.
+        infobar_created = true;
+      }
+      if (i->has_infobar()) {
+        // This infobar is for the same frame/embedder pair. We should remove
+        // it now that we've got an answer for it.
         infobars_to_remove.push_back(*i);
-        ++i;
-      } else {
+        infobar_created = true;
+      }
+      if (!infobar_created) {
         // We haven't created an infobar yet, just remove the pending request.
         i = pending_infobar_requests_.erase(i);
+      } else {
+        ++i;
       }
     } else {
       ++i;
     }

[xhwang, 2014/06/10 21:48:39]

This for loop is crazy and can be improved. Since we already have
requests_to_notify and infobars_to_remove, why can't we just have a
infobar_requests_to_remove? Then we don't need to do erase(i) in the loop and
can just use a normal for loop.

   }
 
   // Remove all infobars for the same |requesting_frame| and |embedder|.
   for (PendingInfobarRequests::iterator i = infobars_to_remove.begin();
        i != infobars_to_remove.end(); ++i)
     GetInfoBarService(i->id())->RemoveInfoBar(i->infobar());
 
   // Send out the permission notifications.
   for (PendingInfobarRequests::iterator i = requests_to_notify.begin();
        i != requests_to_notify.end(); ++i)
@@ skipping 118 matching lines @@
 
   ContentSetting content_setting =
       allowed ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
   profile_->GetHostContentSettingsMap()->SetContentSetting(
       ContentSettingsPattern::FromURLNoWildcard(requesting_frame.GetOrigin()),
       ContentSettingsPattern::FromURLNoWildcard(embedder.GetOrigin()),
       type_,
       std::string(),
       content_setting);
 }