Show queued geolocation InfoBars when InfoBar is hidden.

chrome/browser/geolocation/chrome_geolocation_permission_context.cc

 // Copyright (c) 2011 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/geolocation/chrome_geolocation_permission_context.h"
 
 #include <functional>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #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"
 #include "chrome/browser/tab_contents/confirm_infobar_delegate.h"
 #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"
 #include "content/public/browser/web_contents.h"
 #include "grit/generated_resources.h"
 #include "grit/locale_settings.h"
 #include "grit/theme_resources.h"
 #include "grit/theme_resources_standard.h"
 #include "net/base/net_util.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
@@ skipping 31 matching lines @@
                             int bridge_id,
                             const GURL& requesting_frame,
                             const GURL& emebedder,
                             base::Callback<void(bool)> callback);
 
   // Cancels a specific infobar request.
   void CancelInfoBarRequest(int render_process_id,
                             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.
   void OnPermissionSet(int render_process_id,
                        int render_view_id,
                        int bridge_id,
                        const GURL& requesting_frame,
                        const GURL& embedder,
                        bool allowed);
 
   // content::NotificationObserver
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details);
 
  private:
   struct PendingInfoBarRequest;
   class RequestEquals;
 
   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);
 
   // Cancels an InfoBar request and returns the next iterator position.
   PendingInfoBarRequests::iterator CancelInfoBarRequestInternal(
       PendingInfoBarRequests::iterator i);
 
   content::NotificationRegistrar registrar_;
 
   ChromeGeolocationPermissionContext* const geolocation_permission_context_;
@@ skipping 10 matching lines @@
  public:
   GeolocationConfirmInfoBarDelegate(
       InfoBarTabHelper* infobar_helper,
       GeolocationInfoBarQueueController* controller,
       int render_process_id,
       int render_view_id,
       int bridge_id,
       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(
       const content::LoadCommittedDetails& details) const OVERRIDE;
   virtual gfx::Image* GetIcon() const OVERRIDE;
   virtual Type GetInfoBarType() const OVERRIDE;
   virtual string16 GetMessageText() const OVERRIDE;
   virtual string16 GetButtonLabel(InfoBarButton button) const OVERRIDE;
   virtual bool Accept() OVERRIDE;
   virtual bool Cancel() OVERRIDE;
@@ skipping 28 matching lines @@
       render_view_id_(render_view_id),
       bridge_id_(bridge_id),
       requesting_frame_url_(requesting_frame_url),
       display_languages_(display_languages) {
   const NavigationEntry* committed_entry =
       infobar_helper->web_contents()->GetController().GetLastCommittedEntry();
   committed_contents_unique_id_ = committed_entry ?
       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())
     return false;
   return committed_contents_unique_id_ != details.entry->GetUniqueID() ||
       content::PageTransitionStripQualifier(
           details.entry->GetTransitionType()) ==
               content::PAGE_TRANSITION_RELOAD;
 }
 
@@ skipping 253 matching lines @@
           copied_request.render_process_id, copied_request.render_view_id,
           copied_request.bridge_id, copied_request.requesting_frame,
           copied_request.callback, allowed);
     } else {
       ++i;
     }
   }
 }
 
 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,
                                                           int render_view_id) {
   WebContents* tab_contents =
       tab_util::GetWebContentsByID(render_process_id, render_view_id);
   TabContentsWrapper* wrapper = NULL;
   if (tab_contents)
     wrapper = TabContentsWrapper::GetCurrentWrapperForContents(tab_contents);
   for (PendingInfoBarRequests::iterator i = pending_infobar_requests_.begin();
        i != pending_infobar_requests_.end(); ) {
     if (i->IsForTab(render_process_id, render_view_id)) {
       if (!wrapper) {
         i = pending_infobar_requests_.erase(i);
         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);
       }
       break;
     }
     ++i;
   }
 }
 
@@ skipping 146 matching lines @@
         BrowserThread::UI, FROM_HERE,
         base::Bind(
             &ChromeGeolocationPermissionContext::CancelPendingInfoBarRequest,
             this, render_process_id, render_view_id, bridge_id));
      return;
   }
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   geolocation_infobar_queue_controller_->CancelInfoBarRequest(render_process_id,
       render_view_id, bridge_id);
 }