Change infobar creation to use a public static Create() method on the infobar delegate classes. Make constructors as private as possible.

This has several purposes:
* By preventing direct instantiation, it prevents callers from leaking if they create an infobar and don't add it to an InfoBarService.
* By moving decision-making about when to show infobars into these Create() functions, there's a pattern for where such code should go, and caller code becomes simpler and easier to read.
* The two bullets above together mean that for infobars which should only be displayed in certain circumstances, code can't accidentally bypass the decision logic.
* It enables us to eliminate a common InfoBarService* temp on the caller side since the caller no longer needs to both pass the pointer to the infobar _and_ call AddInfoBar() on the pointer.  This was also a somewhat redundant-looking pattern.
* It makes it easier to change the ownership model for infobars in the future by limiting the affected callsites to only the Create() functions.

Note that right now, this still feels pretty redundant since we pass all the same args to Create() functions as constructors most times.  In the new ownership model constructors will no longer need to take InfoBarService*s, which will make this better.

Additionally, this makes AddInfoBar()/ReplaceInfoBar() take scoped_ptr<>s to indicate they're receiving ownership.  This sort of change is easy to make since we only need change the create functions.

This change also has a functional effect: it eliminates some cases where we tried to only show infobars when no other infobars were already showing (discussed and approved by Glen).

BUG=none
TEST=none
Review URL: https://codereview.chromium.org/11644059

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175467 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 45 insertions, 80 deletions

diff --git a/chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc b/chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc
index 0155641..0fe111c 100644
--- a/chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc
+++ b/chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc
@@ -294,7 +294,7 @@ TEST_F(GeolocationPermissionContextTests, SinglePermission) {
   infobar_service()->RemoveInfoBar(infobar_0);
   EXPECT_EQ(1U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_0));
-  infobar_0->InfoBarClosed();
+  delete infobar_0;
 }
 
 #if defined(OS_ANDROID)
@@ -399,7 +399,7 @@ TEST_F(GeolocationPermissionContextTests, QueuedPermission) {
   EXPECT_EQ(1U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_0));
   closed_delegate_tracker_.Clear();
-  infobar_0->InfoBarClosed();
+  delete infobar_0;
   // Now we should have a new infobar for the second frame.
   ASSERT_EQ(1U, infobar_service()->GetInfoBarCount());
 
@@ -416,7 +416,7 @@ TEST_F(GeolocationPermissionContextTests, QueuedPermission) {
   infobar_service()->RemoveInfoBar(infobar_1);
   EXPECT_EQ(1U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_1));
-  infobar_1->InfoBarClosed();
+  delete infobar_1;
   EXPECT_EQ(0U, infobar_service()->GetInfoBarCount());
   // Ensure the persisted permissions are ok.
   EXPECT_EQ(CONTENT_SETTING_ALLOW,
@@ -470,7 +470,7 @@ TEST_F(GeolocationPermissionContextTests, CancelGeolocationPermissionRequest) {
   EXPECT_EQ(1U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_0));
   closed_delegate_tracker_.Clear();
-  infobar_0->InfoBarClosed();
+  delete infobar_0;
   ASSERT_EQ(1U, infobar_service()->GetInfoBarCount());
 
   ConfirmInfoBarDelegate* infobar_1 =
@@ -486,7 +486,7 @@ TEST_F(GeolocationPermissionContextTests, CancelGeolocationPermissionRequest) {
   infobar_service()->RemoveInfoBar(infobar_1);
   EXPECT_EQ(1U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_1));
-  infobar_1->InfoBarClosed();
+  delete infobar_1;
   EXPECT_EQ(0U, infobar_service()->GetInfoBarCount());
   // Ensure the persisted permissions are ok.
   EXPECT_EQ(CONTENT_SETTING_ASK,
@@ -543,14 +543,14 @@ TEST_F(GeolocationPermissionContextTests, SameOriginMultipleTabs) {
   infobar_service()->RemoveInfoBar(infobar_0);
   EXPECT_EQ(2U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_0));
-  infobar_0->InfoBarClosed();
+  delete infobar_0;
   // Now the infobar for the tab with the same origin should have gone.
   EXPECT_EQ(0U, infobar_service_for_tab(1)->GetInfoBarCount());
   CheckPermissionMessageSentForTab(1, 0, true);
   EXPECT_TRUE(closed_delegate_tracker_.Contains(removed_infobar));
   closed_delegate_tracker_.Clear();
   // Destroy the infobar that has just been removed.
-  removed_infobar->InfoBarClosed();
+  delete removed_infobar;
 
   // But the other tab should still have the info bar...
   ASSERT_EQ(1U, infobar_service_for_tab(0)->GetInfoBarCount());
@@ -560,7 +560,7 @@ TEST_F(GeolocationPermissionContextTests, SameOriginMultipleTabs) {
   infobar_service_for_tab(0)->RemoveInfoBar(infobar_1);
   EXPECT_EQ(1U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_1));
-  infobar_1->InfoBarClosed();
+  delete infobar_1;
 }
 
 TEST_F(GeolocationPermissionContextTests, QueuedOriginMultipleTabs) {
@@ -591,14 +591,14 @@ TEST_F(GeolocationPermissionContextTests, QueuedOriginMultipleTabs) {
   infobar_service_for_tab(0)->RemoveInfoBar(infobar_0);
   EXPECT_EQ(2U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_0));
-  infobar_0->InfoBarClosed();
+  delete infobar_0;
   // Now the infobar for the tab with the same origin should have gone.
   EXPECT_EQ(0U, infobar_service()->GetInfoBarCount());
   CheckPermissionMessageSent(0, true);
   EXPECT_TRUE(closed_delegate_tracker_.Contains(removed_infobar));
   closed_delegate_tracker_.Clear();
   // Destroy the infobar that has just been removed.
-  removed_infobar->InfoBarClosed();
+  delete removed_infobar;
 
   // And we should have the queued infobar displayed now.
   ASSERT_EQ(1U, infobar_service_for_tab(0)->GetInfoBarCount());
@@ -612,7 +612,7 @@ TEST_F(GeolocationPermissionContextTests, QueuedOriginMultipleTabs) {
   infobar_service_for_tab(0)->RemoveInfoBar(infobar_1);
   EXPECT_EQ(1U, closed_delegate_tracker_.size());
   EXPECT_TRUE(closed_delegate_tracker_.Contains(infobar_1));
-  infobar_1->InfoBarClosed();
+  delete infobar_1;
 }
 
 TEST_F(GeolocationPermissionContextTests, TabDestroyed) {
@@ -647,7 +647,7 @@ TEST_F(GeolocationPermissionContextTests, TabDestroyed) {
 
   // Delete the tab contents.
   DeleteContents();
-  infobar_0->InfoBarClosed();
+  delete infobar_0;
 
   // During contents destruction, the infobar will have been closed, and the
   // pending request should have been cleared without an infobar being created.
@@ -680,5 +680,5 @@ TEST_F(GeolocationPermissionContextTests, InfoBarUsesCommittedEntry) {
 
   // Delete the tab contents.
   DeleteContents();
-  infobar_0->InfoBarClosed();
+  delete infobar_0;
 }
diff --git a/chrome/browser/geolocation/geolocation_confirm_infobar_delegate.cc b/chrome/browser/geolocation/geolocation_confirm_infobar_delegate.cc
index d768a50..377dd68 100644
--- a/chrome/browser/geolocation/geolocation_confirm_infobar_delegate.cc
+++ b/chrome/browser/geolocation/geolocation_confirm_infobar_delegate.cc
@@ -17,6 +17,25 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
+#if defined(OS_ANDROID)
+#include "chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.h"
+typedef GeolocationConfirmInfoBarDelegateAndroid DelegateType;
+#else
+typedef GeolocationConfirmInfoBarDelegate DelegateType;
+#endif
+
+
+// static
+InfoBarDelegate* GeolocationConfirmInfoBarDelegate::Create(
+    InfoBarService* infobar_service,
+    GeolocationInfoBarQueueController* controller,
+    const GeolocationPermissionRequestID& id,
+    const GURL& requesting_frame,
+    const std::string& display_languages) {
+  return infobar_service->AddInfoBar(scoped_ptr<InfoBarDelegate>(
+      new DelegateType(infobar_service, controller, id, requesting_frame,
+                       display_languages)));
+}
 
 GeolocationConfirmInfoBarDelegate::GeolocationConfirmInfoBarDelegate(
     InfoBarService* infobar_service,
diff --git a/chrome/browser/geolocation/geolocation_confirm_infobar_delegate.h b/chrome/browser/geolocation/geolocation_confirm_infobar_delegate.h
index ab9c1e1..28a35cb 100644
--- a/chrome/browser/geolocation/geolocation_confirm_infobar_delegate.h
+++ b/chrome/browser/geolocation/geolocation_confirm_infobar_delegate.h
@@ -19,6 +19,15 @@ class InfoBarService;
 // and handling of geolocation permission infobars to the user.
 class GeolocationConfirmInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
+  // Creates a geolocation delegate and adds it to |infobar_service|.  Returns
+  // the delegate if it was successfully added.
+  static InfoBarDelegate* Create(InfoBarService* infobar_service,
+                                 GeolocationInfoBarQueueController* controller,
+                                 const GeolocationPermissionRequestID& id,
+                                 const GURL& requesting_frame,
+                                 const std::string& display_languages);
+
+ protected:
   GeolocationConfirmInfoBarDelegate(
       InfoBarService* infobar_service,
       GeolocationInfoBarQueueController* controller,
@@ -28,7 +37,6 @@ class GeolocationConfirmInfoBarDelegate : public ConfirmInfoBarDelegate {
 
   const GeolocationPermissionRequestID& id() const { return id_; }
 
- protected:
   // ConfirmInfoBarDelegate:
   virtual gfx::Image* GetIcon() const OVERRIDE;
   virtual Type GetInfoBarType() const OVERRIDE;
diff --git a/chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.cc b/chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.cc
deleted file mode 100644
index 14daafc..0000000
--- a/chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.cc
+++ /dev/null
@@ -1,26 +0,0 @@
-// Copyright (c) 2012 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/geolocation_confirm_infobar_delegate_factory.h"
-
-#if defined(OS_ANDROID)
-#include "chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.h"
-#else
-#include "chrome/browser/geolocation/geolocation_confirm_infobar_delegate.h"
-#endif
-
-GeolocationConfirmInfoBarDelegate*
-    GeolocationConfirmInfoBarDelegateFactory::Create(
-    InfoBarService* infobar_service,
-    GeolocationInfoBarQueueController* controller,
-    const GeolocationPermissionRequestID& id,
-    const GURL& requesting_frame_url,
-    const std::string& display_languages) {
-#if defined(OS_ANDROID)
-  return new GeolocationConfirmInfoBarDelegateAndroid(
-#else
-  return new GeolocationConfirmInfoBarDelegate(
-#endif
-      infobar_service, controller, id, requesting_frame_url, display_languages);
-}
diff --git a/chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h b/chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h
deleted file mode 100644
index c33045b..0000000
--- a/chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h
+++ /dev/null
@@ -1,32 +0,0 @@
-// Copyright (c) 2012 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.
-
-#ifndef CHROME_BROWSER_GEOLOCATION_GEOLOCATION_CONFIRM_INFOBAR_DELEGATE_FACTORY_H_
-#define CHROME_BROWSER_GEOLOCATION_GEOLOCATION_CONFIRM_INFOBAR_DELEGATE_FACTORY_H_
-
-#include "base/values.h"
-
-class GeolocationConfirmInfoBarDelegate;
-class GeolocationInfoBarQueueController;
-class GeolocationPermissionRequestID;
-class GURL;
-class InfoBarService;
-
-class GeolocationConfirmInfoBarDelegateFactory {
- public:
-  GeolocationConfirmInfoBarDelegateFactory() {}
-  ~GeolocationConfirmInfoBarDelegateFactory() {}
-  static GeolocationConfirmInfoBarDelegate* Create(
-      InfoBarService* infobar_service,
-      GeolocationInfoBarQueueController* controller,
-      const GeolocationPermissionRequestID& id,
-      const GURL& requesting_frame_url,
-      const std::string& display_languages);
-
- private:
-
-  DISALLOW_COPY_AND_ASSIGN(GeolocationConfirmInfoBarDelegateFactory);
-};
-
-#endif  // CHROME_BROWSER_GEOLOCATION_GEOLOCATION_CONFIRM_INFOBAR_DELEGATE_FACTORY_H_
diff --git a/chrome/browser/geolocation/geolocation_infobar_queue_controller.cc b/chrome/browser/geolocation/geolocation_infobar_queue_controller.cc
index 57dc9d5..40ebde8 100644
--- a/chrome/browser/geolocation/geolocation_infobar_queue_controller.cc
+++ b/chrome/browser/geolocation/geolocation_infobar_queue_controller.cc
@@ -7,7 +7,6 @@
 #include "chrome/browser/api/infobars/infobar_service.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/geolocation/geolocation_confirm_infobar_delegate.h"
-#include "chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h"
 #include "chrome/browser/infobars/infobar.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -51,9 +50,7 @@ class GeolocationInfoBarQueueController::PendingInfoBarRequest {
   const GeolocationPermissionRequestID& id() const { return id_; }
   const GURL& requesting_frame() const { return requesting_frame_; }
   bool has_infobar_delegate() const { return !!infobar_delegate_; }
-  GeolocationConfirmInfoBarDelegate* infobar_delegate() {
-      return infobar_delegate_;
-  }
+  InfoBarDelegate* infobar_delegate() { return infobar_delegate_; }
 
   void RunCallback(bool allowed);
   void CreateInfoBarDelegate(GeolocationInfoBarQueueController* controller,
@@ -64,7 +61,7 @@ class GeolocationInfoBarQueueController::PendingInfoBarRequest {
   GURL requesting_frame_;
   GURL embedder_;
   PermissionDecidedCallback callback_;
-  GeolocationConfirmInfoBarDelegate* infobar_delegate_;
+  InfoBarDelegate* infobar_delegate_;
 
   // Purposefully do not disable copying, as this is stored in STL containers.
 };
@@ -99,7 +96,7 @@ void GeolocationInfoBarQueueController::PendingInfoBarRequest::RunCallback(
 void GeolocationInfoBarQueueController::PendingInfoBarRequest::
     CreateInfoBarDelegate(GeolocationInfoBarQueueController* controller,
                           const std::string& display_languages) {
-  infobar_delegate_ = GeolocationConfirmInfoBarDelegateFactory::Create(
+  infobar_delegate_ = GeolocationConfirmInfoBarDelegate::Create(
       GetInfoBarService(id_), controller, id_, requesting_frame_,
       display_languages);
 
@@ -227,7 +224,7 @@ void GeolocationInfoBarQueueController::Observe(
       content::Details<InfoBarRemovedDetails>(details)->first;
   for (PendingInfoBarRequests::iterator i = pending_infobar_requests_.begin();
        i != pending_infobar_requests_.end(); ++i) {
-    GeolocationConfirmInfoBarDelegate* confirm_delegate = i->infobar_delegate();
+    InfoBarDelegate* confirm_delegate = i->infobar_delegate();
     if (confirm_delegate == delegate) {
       GeolocationPermissionRequestID id(i->id());
       pending_infobar_requests_.erase(i);
@@ -270,7 +267,6 @@ void GeolocationInfoBarQueueController::ShowQueuedInfoBarForTab(
       RegisterForInfoBarNotifications(infobar_service);
       i->CreateInfoBarDelegate(
           this, profile_->GetPrefs()->GetString(prefs::kAcceptLanguages));
-      infobar_service->AddInfoBar(i->infobar_delegate());
       return;
     }
   }