Move GeolocationPermissionContextUnittest from subclassing TabContentsWrapper to merely observing...

chrome/browser/geolocation/geolocation_permission_context_unittest.cc

Index: chrome/browser/geolocation/geolocation_permission_context_unittest.cc
===================================================================
--- chrome/browser/geolocation/geolocation_permission_context_unittest.cc	(revision 87725)
+++ chrome/browser/geolocation/geolocation_permission_context_unittest.cc	(working copy)
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <set>
+
 #include "base/memory/scoped_vector.h"
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/geolocation/geolocation_content_settings_map.h"
@@ -18,59 +20,61 @@
 #include "content/browser/renderer_host/mock_render_process_host.h"
 #include "content/browser/tab_contents/test_tab_contents.h"
 #include "content/common/geolocation_messages.h"
-#include "content/common/notification_details.h"
 #include "content/common/notification_registrar.h"
-#include "content/common/notification_source.h"
-#include "content/common/notification_type.h"
+#include "content/common/notification_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-// TestTabContentsWithPendingInfoBar ------------------------------------------
+// ClosedDelegateTracker ------------------------------------------------------
 
 namespace {
 
-// TestTabContents short-circuits TAB_CONTENTS_INFOBAR_REMOVED to call
-// InfoBarClosed() directly. We need to observe it and call InfoBarClosed()
-// later.
-class TestTabContentsWithPendingInfoBar : public TabContentsWrapper {
+// We need to track which infobars were closed.
+class ClosedDelegateTracker : public NotificationObserver {
  public:
-  TestTabContentsWithPendingInfoBar(Profile* profile, SiteInstance* instance);
-  virtual ~TestTabContentsWithPendingInfoBar();
+  ClosedDelegateTracker();
+  virtual ~ClosedDelegateTracker();
 
-  // TabContentsWrapper:
+  // NotificationObserver:
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
-  InfoBarDelegate* removed_infobar_delegate_;
+  size_t size() const {
+    return removed_infobar_delegates_.size();
+  }
+
+  bool Contains(InfoBarDelegate* delegate) const;
+  void Clear();
+
+ private:
   NotificationRegistrar registrar_;
+  std::set<InfoBarDelegate*> removed_infobar_delegates_;
 };
 
-TestTabContentsWithPendingInfoBar::TestTabContentsWithPendingInfoBar(
-    Profile* profile,
-    SiteInstance* instance)
-    : TabContentsWrapper(
-          new TabContents(profile, instance, MSG_ROUTING_NONE, NULL, NULL)),
-      removed_infobar_delegate_(NULL) {
-  Source<TabContentsWrapper> source(this);
+ClosedDelegateTracker::ClosedDelegateTracker() {
   registrar_.Add(this, NotificationType::TAB_CONTENTS_INFOBAR_REMOVED,
-                 source);
+                 NotificationService::AllSources());
 }
 
-TestTabContentsWithPendingInfoBar::~TestTabContentsWithPendingInfoBar() {
+ClosedDelegateTracker::~ClosedDelegateTracker() {
 }
 
-void TestTabContentsWithPendingInfoBar::Observe(
-    NotificationType type,
-    const NotificationSource& source,
-    const NotificationDetails& details) {
-  if (type.value == NotificationType::TAB_CONTENTS_INFOBAR_REMOVED) {
-    removed_infobar_delegate_ =
-        Details<std::pair<InfoBarDelegate*, bool> >(details)->first;
-  } else {
-    TabContentsWrapper::Observe(type, source, details);
-  }
+void ClosedDelegateTracker::Observe(NotificationType type,
+                                    const NotificationSource& source,
+                                    const NotificationDetails& details) {
+  DCHECK(type.value == NotificationType::TAB_CONTENTS_INFOBAR_REMOVED);
+  removed_infobar_delegates_.insert(
+      Details<std::pair<InfoBarDelegate*, bool> >(details)->first);
 }
 
+bool ClosedDelegateTracker::Contains(InfoBarDelegate* delegate) const {
+  return removed_infobar_delegates_.count(delegate) != 0;
+}
+
+void ClosedDelegateTracker::Clear() {
+  removed_infobar_delegates_.clear();
+}
+
 }  // namespace
 
 
@@ -86,11 +90,12 @@
 
   int process_id() { return contents()->render_view_host()->process()->id(); }
   int process_id_for_tab(int tab) {
-    return extra_tabs_[tab]->render_view_host()->process()->id();
+    return extra_tabs_[tab]->tab_contents()->render_view_host()->process()->
+        id();
   }
   int render_id() { return contents()->render_view_host()->routing_id(); }
   int render_id_for_tab(int tab) {
-    return extra_tabs_[tab]->render_view_host()->routing_id();
+    return extra_tabs_[tab]->tab_contents()->render_view_host()->routing_id();
   }
   int bridge_id() const { return 42; }  // Not relevant at this level.
 
@@ -103,9 +108,9 @@
   void CheckTabContentsState(const GURL& requesting_frame,
                              ContentSetting expected_content_setting);
 
-  TestTabContentsWithPendingInfoBar* tab_contents_with_pending_infobar_;
   scoped_refptr<GeolocationPermissionContext> geolocation_permission_context_;
-  ScopedVector<TestTabContentsWithPendingInfoBar> extra_tabs_;
+  scoped_ptr<ClosedDelegateTracker> closed_delegate_tracker_;

[bulach, 2011/06/03 19:15:05]

nit: it no longer depends on anything for lazy initialization, you may as well
just have a regular member instead of a scoped_ptr.

+  ScopedVector<TabContentsWrapper> extra_tabs_;
 
  private:
   // TabContentsWrapperTestHarness:
@@ -117,7 +122,7 @@
 };
 
 GeolocationPermissionContextTests::GeolocationPermissionContextTests()
-    : tab_contents_with_pending_infobar_(NULL),
+    : TabContentsWrapperTestHarness(),

[bulach, 2011/06/03 19:15:05]

nit: unnecessary?

[Peter Kasting, 2011/06/03 20:37:36]

The compiler will do it implicitly, but I prefer to make the construction
explicit.  This also matches the style in TabContentsWrapperTestHarness()
itself.

       ui_thread_(BrowserThread::UI, MessageLoop::current()),
       dependency_factory_(
           new GeolocationArbitratorDependencyFactoryWithLocationProvider(
@@ -138,7 +143,8 @@
     int bridge_id,
     bool allowed) {
   CheckPermissionMessageSentInternal(static_cast<MockRenderProcessHost*>(
-      extra_tabs_[tab]->render_view_host()->process()), bridge_id, allowed);
+      extra_tabs_[tab]->tab_contents()->render_view_host()->process()),
+      bridge_id, allowed);
 }
 
 void GeolocationPermissionContextTests::CheckPermissionMessageSentInternal(
@@ -158,12 +164,12 @@
 }
 
 void GeolocationPermissionContextTests::AddNewTab(const GURL& url) {
-  TestTabContentsWithPendingInfoBar* new_tab =
-      new TestTabContentsWithPendingInfoBar(profile(), NULL);
+  TabContents* new_tab =
+      new TabContents(profile(), NULL, MSG_ROUTING_NONE, NULL, NULL);
   new_tab->controller().LoadURL(url, GURL(), PageTransition::TYPED);
-  static_cast<TestRenderViewHost*>(new_tab->tab_contents()->render_manager()->
-      current_host())->SendNavigate(extra_tabs_.size() + 1, url);
-  extra_tabs_.push_back(new_tab);
+  static_cast<TestRenderViewHost*>(new_tab->render_manager()->current_host())->
+      SendNavigate(extra_tabs_.size() + 1, url);
+  extra_tabs_.push_back(new TabContentsWrapper(new_tab));
 }
 
 void GeolocationPermissionContextTests::CheckTabContentsState(
@@ -187,14 +193,13 @@
   GeolocationArbitrator::SetDependencyFactoryForTest(
       dependency_factory_.get());
   SiteInstance* site_instance = contents()->GetSiteInstance();
-  tab_contents_with_pending_infobar_ =
-      new TestTabContentsWithPendingInfoBar(profile_.get(), site_instance);
-  SetContentsWrapper(tab_contents_with_pending_infobar_);
   geolocation_permission_context_ =
       new GeolocationPermissionContext(profile());
+  closed_delegate_tracker_.reset(new ClosedDelegateTracker());
 }
 
 void GeolocationPermissionContextTests::TearDown() {
+  closed_delegate_tracker_.reset();
   GeolocationArbitrator::SetDependencyFactoryForTest(NULL);
   TabContentsWrapperTestHarness::TearDown();
 }
@@ -241,8 +246,9 @@
   CheckPermissionMessageSent(bridge_id(), true);
 
   contents_wrapper()->RemoveInfoBar(infobar_0);
-  EXPECT_EQ(infobar_0,
-            tab_contents_with_pending_infobar_->removed_infobar_delegate_);
+  EXPECT_EQ(1U, closed_delegate_tracker_->size());
+  EXPECT_TRUE(closed_delegate_tracker_->Contains(infobar_0));
+  closed_delegate_tracker_->Clear();
   infobar_0->InfoBarClosed();
   // Now we should have a new infobar for the second frame.
   EXPECT_EQ(1U, contents_wrapper()->infobar_count());
@@ -258,8 +264,8 @@
   CheckTabContentsState(requesting_frame_1, CONTENT_SETTING_BLOCK);
   CheckPermissionMessageSent(bridge_id() + 1, false);
   contents_wrapper()->RemoveInfoBar(infobar_1);
-  EXPECT_EQ(infobar_1,
-            tab_contents_with_pending_infobar_->removed_infobar_delegate_);
+  EXPECT_EQ(1U, closed_delegate_tracker_->size());
+  EXPECT_TRUE(closed_delegate_tracker_->Contains(infobar_1));
   infobar_1->InfoBarClosed();
   EXPECT_EQ(0U, contents_wrapper()->infobar_count());
   // Ensure the persisted permissions are ok.
@@ -299,8 +305,9 @@
   // is removed and the next pending infobar is created.
   geolocation_permission_context_->CancelGeolocationPermissionRequest(
       process_id(), render_id(), bridge_id(), requesting_frame_0);
-  EXPECT_EQ(infobar_0,
-            tab_contents_with_pending_infobar_->removed_infobar_delegate_);
+  EXPECT_EQ(1U, closed_delegate_tracker_->size());
+  EXPECT_TRUE(closed_delegate_tracker_->Contains(infobar_0));
+  closed_delegate_tracker_->Clear();
   infobar_0->InfoBarClosed();
   EXPECT_EQ(1U, contents_wrapper()->infobar_count());
 
@@ -315,8 +322,8 @@
   CheckTabContentsState(requesting_frame_1, CONTENT_SETTING_ALLOW);
   CheckPermissionMessageSent(bridge_id() + 1, true);
   contents_wrapper()->RemoveInfoBar(infobar_1);
-  EXPECT_EQ(infobar_1,
-            tab_contents_with_pending_infobar_->removed_infobar_delegate_);
+  EXPECT_EQ(1U, closed_delegate_tracker_->size());
+  EXPECT_TRUE(closed_delegate_tracker_->Contains(infobar_1));
   infobar_1->InfoBarClosed();
   EXPECT_EQ(0U, contents_wrapper()->infobar_count());
   // Ensure the persisted permissions are ok.
@@ -369,12 +376,13 @@
   infobar_0->Accept();
   CheckPermissionMessageSent(bridge_id(), true);
   contents_wrapper()->RemoveInfoBar(infobar_0);
-  EXPECT_EQ(infobar_0,
-            tab_contents_with_pending_infobar_->removed_infobar_delegate_);
+  EXPECT_EQ(2U, closed_delegate_tracker_->size());

[bulach, 2011/06/03 19:15:05]

this may fail: infobar_0 may just get the same address as removed_infobar, and
the set will contain a single entry.
since it's a tiny amount, we may get away with using a vector: size would be 2,
and it wouldn't matter which one we matched..

[Peter Kasting, 2011/06/03 20:37:36]

I don't see how that's possible; the two infobars are showing simultaneously, so
they can't be the same object.

[bulach, 2011/06/08 14:23:03]

sorry again for the lack of response, I was happy with your explanation :) 
I mistakenly thought that the the infobars would be created sequentially (which
could potentially break the set size) rather than simultaneously (which is safe
as the addresses are never reused).

+  EXPECT_TRUE(closed_delegate_tracker_->Contains(infobar_0));
   infobar_0->InfoBarClosed();
   // Now the infobar for the tab with the same origin should have gone.
   EXPECT_EQ(0U, extra_tabs_[1]->infobar_count());
   CheckPermissionMessageSentForTab(1, bridge_id(), true);
+  EXPECT_TRUE(closed_delegate_tracker_->Contains(removed_infobar));
   // Destroy the infobar that has just been removed.
   removed_infobar->InfoBarClosed();
 
@@ -412,12 +420,14 @@
   infobar_0->Accept();
   CheckPermissionMessageSentForTab(0, bridge_id(), true);
   extra_tabs_[0]->RemoveInfoBar(infobar_0);
-  EXPECT_EQ(infobar_0,
-            extra_tabs_[0]->removed_infobar_delegate_);
+  EXPECT_EQ(2U, closed_delegate_tracker_->size());

[bulach, 2011/06/03 19:15:05]

ditto

+  EXPECT_TRUE(closed_delegate_tracker_->Contains(infobar_0));
   infobar_0->InfoBarClosed();
   // Now the infobar for the tab with the same origin should have gone.
   EXPECT_EQ(0U, contents_wrapper()->infobar_count());
   CheckPermissionMessageSent(bridge_id(), true);
+  EXPECT_TRUE(closed_delegate_tracker_->Contains(removed_infobar));
+  closed_delegate_tracker_->Clear();
   // Destroy the infobar that has just been removed.
   removed_infobar->InfoBarClosed();
 
@@ -431,8 +441,8 @@
   infobar_1->Accept();
   CheckPermissionMessageSentForTab(0, bridge_id() + 1, true);
   extra_tabs_[0]->RemoveInfoBar(infobar_1);
-  EXPECT_EQ(infobar_1,
-            extra_tabs_[0]->removed_infobar_delegate_);
+  EXPECT_EQ(1U, closed_delegate_tracker_->size());
+  EXPECT_TRUE(closed_delegate_tracker_->Contains(infobar_1));
   infobar_1->InfoBarClosed();
 
   extra_tabs_.reset();