Show queued geolocation InfoBars when InfoBar is hidden.

chrome/browser/geolocation/chrome_geolocation_permission_context.cc

Index: chrome/browser/geolocation/chrome_geolocation_permission_context.cc
diff --git a/chrome/browser/geolocation/chrome_geolocation_permission_context.cc b/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
index 1943ed53ec4e70b1856321589ad4841f5385d78b..258d0d0c5211828780e3b6e6a986777aa2fba7dd 100644
--- a/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
+++ b/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
@@ -14,6 +14,7 @@
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/google/google_util.h"
+#include "chrome/browser/infobars/infobar.h"
 #include "chrome/browser/infobars/infobar_tab_helper.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -21,11 +22,13 @@
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/chrome_notification_types.h"

[tfarina, 2012/02/28 23:20:58]

nit: sort, should be before extension.h

[John Knottenbelt, 2012/03/01 10:10:49]

Done.

 #include "chrome/common/pref_names.h"
 #include "content/browser/renderer_host/render_view_host.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
@@ -77,12 +80,6 @@ class GeolocationInfoBarQueueController : content::NotificationObserver {
                             int render_view_id,
                             int bridge_id);
 
-  // Called by the InfoBarDelegate to notify it's closed. It'll display a new
-  // InfoBar if there's any request pending for this tab.
-  void OnInfoBarClosed(int render_process_id,
-                       int render_view_id,
-                       int bridge_id);
-
   // Called by the InfoBarDelegate to notify permission has been set.
   // It'll notify and dismiss any other pending InfoBar request for the same
   // |requesting_frame| and embedder.
@@ -104,6 +101,12 @@ class GeolocationInfoBarQueueController : content::NotificationObserver {
 
   typedef std::vector<PendingInfoBarRequest> PendingInfoBarRequests;
 
+  // Called by Observe in response to NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED.
+  // It'll display a new InfoBar if there's any request pending for this tab.
+  void OnInfoBarClosed(int render_process_id,
+                       int render_view_id,
+                       int bridge_id);
+
   // Shows the first pending infobar for this tab.
   void ShowQueuedInfoBar(int render_process_id, int render_view_id);
 
@@ -134,8 +137,10 @@ class GeolocationConfirmInfoBarDelegate : public ConfirmInfoBarDelegate {
       const GURL& requesting_frame_url,
       const std::string& display_languages);
 
+  int render_process_id() const { return render_process_id_; }
+  int render_view_id() const { return render_view_id_; }
+
  private:
-  virtual ~GeolocationConfirmInfoBarDelegate();
 
   // ConfirmInfoBarDelegate:
   virtual bool ShouldExpire(
@@ -184,11 +189,6 @@ GeolocationConfirmInfoBarDelegate::GeolocationConfirmInfoBarDelegate(
       committed_entry->GetUniqueID() : 0;
 }
 
-GeolocationConfirmInfoBarDelegate::~GeolocationConfirmInfoBarDelegate() {
-  controller_->OnInfoBarClosed(render_process_id_, render_view_id_,
-                               bridge_id_);
-}
-
 bool GeolocationConfirmInfoBarDelegate::ShouldExpire(
     const content::LoadCommittedDetails& details) const {
   if (details.did_replace_entry || !details.is_navigation_to_different_page())
@@ -464,19 +464,13 @@ void GeolocationInfoBarQueueController::OnPermissionSet(
 void GeolocationInfoBarQueueController::Observe(
     int type, const content::NotificationSource& source,

[Peter Kasting, 2012/02/28 19:34:59]

Nit: While here, one arg per line

[John Knottenbelt, 2012/03/01 10:10:49]

Done.

     const content::NotificationDetails& details) {
-  registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,

[bulach, 2012/02/28 17:20:34]

as we chatted, I think we also need this one..
this is owned by GeolocationPermissionContext, which is owned by the Profile
which outlives the web contents... should it be destroyed by any reason, we
probably want to clean up any pending infobars from it too..

just to be clear, I'm advocating just for keeping this side, in addition to the
right hand side of this patch.

if you do agree with this, please also update the patch description :)

[Peter Kasting, 2012/02/28 19:34:59]

No, we don't want to keep this.

If the web contents is destroyed the infobars on it should all be removed
beforehand.  That's what the InfoBarTabHelper does in its destructor.  Keeping
this would imply that somehow a WebContents can reach the shutdown-notification
stage with open infobars, which is wrong.

[bulach, 2012/02/29 11:29:29]

thanks for the details, and sorry I wasn't clear. 
let me try to explain my thoughts, let me know if they make any sense:
this container here may have pending requests that haven't materialized as
infobars yet: suppose a page with N frames requesting access to geolocation.
initially, it'll have 1 actual infobar, and N-1 requests.. should the
WebContents go away, the infobar will be dismissed as you mentioned, but this
pending requests will not.
I guess the previous code here would take care of this case, no?

[Peter Kasting, 2012/02/29 20:47:54]

The pending requests should be cleared as well, because before the WebContents
goes away, it will first tell the InfoBarTabHelper to shut down.  This will in
turn tell the visible infobars to dismiss.  Dismissing the visible geolocation
infobar will pump the queue immediately via the INFOBAR_REMOVED notification
calling ShowQueuedInfoBar.  That will add a new infobar.  Then that infobar will
be removed by the InfoBarTabHelper, causing the cycle to repeat.

The user will never actually see any of these newly-added infobars because none
of them will be animated above 0 height before they're removed.

[bulach, 2012/03/01 11:19:32]

got it, thanks for the clarification.

+  registrar_.Remove(this, chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,

[Peter Kasting, 2012/02/28 19:34:59]

This code will be reached when any infobar is removed, not just when the one you
want is removed!  You need to ensure that the infobar being removed is actually
the one you expect.

                     source);
-  WebContents* web_contents = content::Source<WebContents>(source).ptr();
-  for (PendingInfoBarRequests::iterator i = pending_infobar_requests_.begin();
-       i != pending_infobar_requests_.end();) {
-    if (i->infobar_delegate == NULL &&
-        web_contents == tab_util::GetWebContentsByID(i->render_process_id,
-                                                     i->render_view_id)) {
-      i = pending_infobar_requests_.erase(i);
-    } else {
-      ++i;
-    }
-  }
+  InfoBarRemovedDetails* removed_details =
+      content::Details<InfoBarRemovedDetails>(details).ptr();
+  GeolocationConfirmInfoBarDelegate* delegate =
+      static_cast<GeolocationConfirmInfoBarDelegate*>(removed_details->first);
+  ShowQueuedInfoBar(delegate->render_process_id(), delegate->render_view_id());
 }
 
 void GeolocationInfoBarQueueController::ShowQueuedInfoBar(int render_process_id,
@@ -494,15 +488,18 @@ void GeolocationInfoBarQueueController::ShowQueuedInfoBar(int render_process_id,
         continue;
       }
 
+      InfoBarTabHelper *helper = wrapper->infobar_tab_helper();

[bulach, 2012/02/28 17:20:34]

nit: s/r *helper/r* helper/

[John Knottenbelt, 2012/03/01 10:10:49]

Done.

+
       if (!i->infobar_delegate) {
         if (!registrar_.IsRegistered(

[Peter Kasting, 2012/02/28 19:34:59]

If infobars are disabled, you'll add a listener for a notification that will not
fire, because the infobar will never have been added.  To solve this, I suggest
checking that after AddInfobar(), the helper has added an infobar and it's the
one you created; then only register if this is the case.  You'll need this
infobar pointer anyway to address my comment in Observe() above.

I also don't see why IsRegistered() should ever return true here.  If it does
that means you're adding a second infobar to the tab, which is wrong.  So I
would instead DCHECK() that IsRegistered() is false.

-                this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-                content::Source<WebContents>(tab_contents))) {
-          registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-                         content::Source<WebContents>(tab_contents));

[bulach, 2012/02/28 17:20:34]

as above, we may need to keep this too..

+                this, chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,
+                content::Source<InfoBarTabHelper>(helper))) {
+          registrar_.Add(
+              this, chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,
+              content::Source<InfoBarTabHelper>(helper));
         }
         i->infobar_delegate = new GeolocationConfirmInfoBarDelegate(
-            wrapper->infobar_tab_helper(), this, render_process_id,
+            helper, this, render_process_id,
             render_view_id, i->bridge_id, i->requesting_frame,
             profile_->GetPrefs()->GetString(prefs::kAcceptLanguages));
         wrapper->infobar_tab_helper()->AddInfoBar(i->infobar_delegate);