Remove Infobars notifications from GoogleURLTrackerMapEntry

chrome/browser/google/google_url_tracker_unittest.cc

Index: chrome/browser/google/google_url_tracker_unittest.cc
diff --git a/chrome/browser/google/google_url_tracker_unittest.cc b/chrome/browser/google/google_url_tracker_unittest.cc
index 81f9e82d35bc8b1358e8e0ec72bf5f6daff3ad30..d4fd3ddeded85c3084b61d194431bd4f7845db5b 100644
--- a/chrome/browser/google/google_url_tracker_unittest.cc
+++ b/chrome/browser/google/google_url_tracker_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright 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.
 
@@ -7,9 +7,10 @@
 #include <set>
 #include <string>
 
+#include "base/containers/scoped_ptr_hash_map.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/google/google_url_tracker_factory.h"
 #include "chrome/browser/google/google_url_tracker_infobar_delegate.h"
 #include "chrome/browser/google/google_url_tracker_navigation_helper.h"
@@ -18,53 +19,13 @@
 #include "components/google/core/browser/google_url_tracker_client.h"
 #include "components/infobars/core/infobar.h"
 #include "components/infobars/core/infobar_delegate.h"
-#include "content/public/browser/notification_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "net/url_request/url_fetcher.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-class GoogleURLTrackerTest;
-
 namespace {
 
-// TestInfoBarDelegate --------------------------------------------------------
-
-class TestInfoBarDelegate : public GoogleURLTrackerInfoBarDelegate {
- public:
-   // Creates a test infobar and delegate and returns the infobar.  Unlike the
-   // parent class, this does not add the infobar to |infobar_manager|, since
-   // that "pointer" is really just a magic number.  Thus there is no
-   // InfoBarManager ownership of the returned object; and since the caller
-   // doesn't own the returned object, we rely on |test_harness| cleaning this
-   // up eventually in GoogleURLTrackerTest::OnInfoBarClosed() to avoid leaks.
-   static infobars::InfoBar* Create(
-      GoogleURLTrackerTest* test_harness,
-      infobars::InfoBarManager* infobar_manager,
-      GoogleURLTracker* google_url_tracker,
-      const GURL& search_url);
-
- private:
-  TestInfoBarDelegate(GoogleURLTrackerTest* test_harness,
-                      infobars::InfoBarManager* infobar_manager,
-                      GoogleURLTracker* google_url_tracker,
-                      const GURL& search_url);
-  virtual ~TestInfoBarDelegate();
-
-  // GoogleURLTrackerInfoBarDelegate:
-  virtual void Update(const GURL& search_url) OVERRIDE;
-  virtual void Close(bool redo_search) OVERRIDE;
-
-  GoogleURLTrackerTest* test_harness_;
-  infobars::InfoBarManager* infobar_manager_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestInfoBarDelegate);
-};
-
-// The member function definitions come after the declaration of
-// GoogleURLTrackerTest, so they can call members on it.
-
-
 // TestCallbackListener ---------------------------------------------------
 
 class TestCallbackListener {
@@ -201,33 +162,23 @@ void TestGoogleURLTrackerNavigationHelper::OpenURL(
     bool user_clicked_on_link) {
 }
 
-}  // namespace
 
+// TestInfoBarManager ---------------------------------------------------------
+
+class TestInfoBarManager : public infobars::InfoBarManager {
+ public:
+  virtual int GetActiveEntryID() OVERRIDE;
+};
+
+int TestInfoBarManager::GetActiveEntryID() {
+  return 0;
+}
+
+}  // namespace
 
 // GoogleURLTrackerTest -------------------------------------------------------
 
-// Ths class exercises GoogleURLTracker.  In order to avoid instantiating more
-// of the Chrome infrastructure than necessary, the GoogleURLTracker functions
-// are carefully written so that many of the functions which take
-// NavigationController* or infobars::InfoBarManager* do not actually
-// dereference the
-// objects, merely use them for comparisons and lookups, e.g. in |entry_map_|.
-// This then allows the test code here to not create any of these objects, and
-// instead supply "pointers" that are actually reinterpret_cast<>()ed magic
-// numbers.  Then we write the necessary stubs/hooks, here and in
-// TestInfoBarDelegate above, to make everything continue to work.
-//
-// Technically, the C++98 spec defines the result of casting
-// T* -> intptr_t -> T* to be an identity, but intptr_t -> T* -> intptr_t (what
-// we use here) is "implementation-defined".  Since I've never seen a compiler
-// break this, though, and the result would simply be a failing test rather than
-// a bug in Chrome, we'll use it anyway.
 class GoogleURLTrackerTest : public testing::Test {
- public:
-  // Called by TestInfoBarDelegate::Close().
-  void OnInfoBarClosed(scoped_ptr<infobars::InfoBar> infobar,
-                       infobars::InfoBarManager* infobar_manager);
-
  protected:
   GoogleURLTrackerTest();
   virtual ~GoogleURLTrackerTest();
@@ -250,31 +201,28 @@ class GoogleURLTrackerTest : public testing::Test {
   GURL google_url() const { return google_url_tracker_->google_url(); }
   void SetLastPromptedGoogleURL(const GURL& url);
   GURL GetLastPromptedGoogleURL();
-  void SetNavigationPending(intptr_t unique_id, bool is_search);
-  void CommitNonSearch(intptr_t unique_id);
-  void CommitSearch(intptr_t unique_id, const GURL& search_url);
-  void CloseTab(intptr_t unique_id);
-  GoogleURLTrackerMapEntry* GetMapEntry(intptr_t unique_id);
-  GoogleURLTrackerInfoBarDelegate* GetInfoBarDelegate(intptr_t unique_id);
-  GoogleURLTrackerNavigationHelper* GetNavigationHelper(intptr_t unique_id);
+  void SetNavigationPending(int unique_id, bool is_search);
+  void CommitNonSearch(int unique_id);
+  void CommitSearch(int unique_id, const GURL& search_url);
+  void CloseTab(int unique_id);
+  GoogleURLTrackerMapEntry* GetMapEntry(int unique_id);
+  GoogleURLTrackerInfoBarDelegate* GetInfoBarDelegate(int unique_id);
+  GoogleURLTrackerNavigationHelper* GetNavigationHelper(int unique_id);
   void ExpectDefaultURLs() const;
-  void ExpectListeningForCommit(intptr_t unique_id, bool listening);
+  void ExpectListeningForCommit(int unique_id, bool listening);
   bool listener_notified() const { return listener_.notified(); }
   void clear_listener_notified() { listener_.clear_notified(); }
 
  private:
-  // Since |infobar_manager| is really a magic number rather than an actual
-  // object, we don't add the created infobar to it.  Instead we will simulate
-  // any helper<->infobar interaction necessary.  The returned object will be
-  // cleaned up in OnInfoBarClosed().
-  infobars::InfoBar* CreateTestInfoBar(
-      infobars::InfoBarManager* infobar_manager,
-      GoogleURLTracker* google_url_tracker,
-      const GURL& search_url);
-
   // These are required by the TestURLFetchers GoogleURLTracker will create (see
   // test_url_fetcher_factory.h).
   content::TestBrowserThreadBundle thread_bundle_;
+
+  // Gets the infobar manager indexed by |unique_id|.
+  // Lazily creates WebContents instances, adds them in |infobar_manager_map_|

[blundell, 2014/06/02 15:02:40]

This comment is stale now, right?

[droger, 2014/06/02 15:56:29]

No.

[blundell, 2014/06/02 16:00:09]

But you're not creating WebContents instances anymore...

On 2014/06/02 15:56:29, droger wrote:

+  // and attaches InfoBarManagers to them.
+  infobars::InfoBarManager* GetInfoBarManager(int unique_id);
+
   // Creating this allows us to call
   // net::NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests().
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
@@ -283,32 +231,12 @@ class GoogleURLTrackerTest : public testing::Test {
   TestingProfile profile_;
   scoped_ptr<GoogleURLTracker> google_url_tracker_;
   TestCallbackListener listener_;
+  base::ScopedPtrHashMap<int, infobars::InfoBarManager> infobar_manager_map_;

[blundell, 2014/06/02 15:02:40]

This is probably a big change, but do you even need this map anymore, and to be
passing around the ints to index into it? Couldn't you just have several
InfoBarManagers and then use GoogleURLTracker's |entry_map_| to get at the
MapEntries (and NavHelpers, etc) associated with those InfoBarManagers?

[droger, 2014/06/02 15:56:29]

I need something to own the InfoBarManagers, as the URL Tracker code does not
own them.
I could use the map entries to get at them (I considered doing it at some
point), but I don't really see benefit since this does not allow to get rid of
the map.

[blundell, 2014/06/02 16:00:09]

The "something" could just be like 3 (or however many you need)
TestInfoBarManager ivars. It would enable you to get rid of the map that's being
maintained in this test in parallel to the map that GoogleURLTracker is
maintaining for the same purpose, and get rid of these unique_ids which don't
correspond to anything in the production code at this point. It's possible that
I'm missing some complication though.

On 2014/06/02 15:56:29, droger wrote:

[droger, 2014/06/02 16:50:04]

Done.
Although SetNavigationPending still requires some unique ID, but it ends up
working relatively well.

   // This tracks the different "tabs" a test has "opened", so we can close them
   // properly before shutting down |google_url_tracker_|, which expects that.
   std::set<int> unique_ids_seen_;
 };
 
-void GoogleURLTrackerTest::OnInfoBarClosed(
-    scoped_ptr<infobars::InfoBar> infobar,
-    infobars::InfoBarManager* infobar_manager) {
-  // First, simulate the InfoBarManager firing INFOBAR_REMOVED.
-  // TODO(droger): Replace this flow with a call to the observer method once
-  // the map entry is observing InfoBarManager. crbug.com/373243
-  infobars::InfoBar::RemovedDetails removed_details(infobar.get(), false);
-  GoogleURLTracker::EntryMap::const_iterator i =
-      google_url_tracker_->entry_map_.find(infobar_manager);
-  ASSERT_FALSE(i == google_url_tracker_->entry_map_.end());
-  GoogleURLTrackerMapEntry* map_entry = i->second;
-  ASSERT_EQ(infobar->delegate(), map_entry->infobar_delegate());
-  map_entry->Observe(
-      chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,
-      content::Source<infobars::InfoBarManager>(infobar_manager),
-      content::Details<infobars::InfoBar::RemovedDetails>(&removed_details));
-
-  // Second, simulate the infobar container closing the infobar in response.
-  // This happens automatically as |infobar| goes out of scope.
-}
-
 GoogleURLTrackerTest::GoogleURLTrackerTest()
     : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {
   GoogleURLTrackerFactory::GetInstance()->
@@ -326,8 +254,8 @@ void GoogleURLTrackerTest::SetUp() {
   scoped_ptr<GoogleURLTrackerClient> client(client_);
   google_url_tracker_.reset(new GoogleURLTracker(
       &profile_, client.Pass(), GoogleURLTracker::UNIT_TEST_MODE));
-  google_url_tracker_->infobar_creator_ = base::Bind(
-      &GoogleURLTrackerTest::CreateTestInfoBar, base::Unretained(this));
+  google_url_tracker_->infobar_creator_ =
+      base::Bind(&GoogleURLTrackerInfoBarDelegate::Create);

[blundell, 2014/06/02 15:02:40]

This is just calling the production creation function now, right? If so, you can
eliminate the |infobar_creator_| ivar entirely.

[droger, 2014/06/02 15:56:29]

Done.

 }
 
 void GoogleURLTrackerTest::TearDown() {
@@ -380,8 +308,7 @@ GURL GoogleURLTrackerTest::GetLastPromptedGoogleURL() {
   return GURL(profile_.GetPrefs()->GetString(prefs::kLastPromptedGoogleURL));
 }
 
-void GoogleURLTrackerTest::SetNavigationPending(intptr_t unique_id,
-                                                bool is_search) {
+void GoogleURLTrackerTest::SetNavigationPending(int unique_id, bool is_search) {
   if (is_search) {
     google_url_tracker_->SearchCommitted();
     // Note that the call above might not have actually registered a listener
@@ -393,12 +320,12 @@ void GoogleURLTrackerTest::SetNavigationPending(intptr_t unique_id,
         scoped_ptr<GoogleURLTrackerNavigationHelper>(
             new TestGoogleURLTrackerNavigationHelper(
                 google_url_tracker_.get())),
-        reinterpret_cast<infobars::InfoBarManager*>(unique_id),
+        GetInfoBarManager(unique_id),
         unique_id);
   }
 }
 
-void GoogleURLTrackerTest::CommitNonSearch(intptr_t unique_id) {
+void GoogleURLTrackerTest::CommitNonSearch(int unique_id) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   if (!map_entry)
     return;
@@ -417,17 +344,16 @@ void GoogleURLTrackerTest::CommitNonSearch(intptr_t unique_id) {
   map_entry->infobar_delegate()->Close(false);
 }
 
-void GoogleURLTrackerTest::CommitSearch(intptr_t unique_id,
-                                        const GURL& search_url) {
+void GoogleURLTrackerTest::CommitSearch(int unique_id, const GURL& search_url) {
   DCHECK(search_url.is_valid());
   GoogleURLTrackerNavigationHelper* nav_helper = GetNavigationHelper(unique_id);
   if (nav_helper && nav_helper->IsListeningForNavigationCommit()) {
-    google_url_tracker_->OnNavigationCommitted(
-        reinterpret_cast<infobars::InfoBarManager*>(unique_id), search_url);
+    google_url_tracker_->OnNavigationCommitted(GetInfoBarManager(unique_id),
+                                               search_url);
   }
 }
 
-void GoogleURLTrackerTest::CloseTab(intptr_t unique_id) {
+void GoogleURLTrackerTest::CloseTab(int unique_id) {
   unique_ids_seen_.erase(unique_id);
   GoogleURLTrackerNavigationHelper* nav_helper = GetNavigationHelper(unique_id);
   if (nav_helper && nav_helper->IsListeningForTabDestruction()) {
@@ -440,22 +366,20 @@ void GoogleURLTrackerTest::CloseTab(intptr_t unique_id) {
   }
 }
 
-GoogleURLTrackerMapEntry* GoogleURLTrackerTest::GetMapEntry(
-    intptr_t unique_id) {
+GoogleURLTrackerMapEntry* GoogleURLTrackerTest::GetMapEntry(int unique_id) {
   GoogleURLTracker::EntryMap::const_iterator i =
-      google_url_tracker_->entry_map_.find(
-          reinterpret_cast<infobars::InfoBarManager*>(unique_id));
+      google_url_tracker_->entry_map_.find(GetInfoBarManager(unique_id));
   return (i == google_url_tracker_->entry_map_.end()) ? NULL : i->second;
 }
 
 GoogleURLTrackerInfoBarDelegate* GoogleURLTrackerTest::GetInfoBarDelegate(
-    intptr_t unique_id) {
+    int unique_id) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   return map_entry ? map_entry->infobar_delegate() : NULL;
 }
 
 GoogleURLTrackerNavigationHelper* GoogleURLTrackerTest::GetNavigationHelper(
-    intptr_t unique_id) {
+    int unique_id) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   return map_entry ? map_entry->navigation_helper() : NULL;
 }
@@ -465,7 +389,7 @@ void GoogleURLTrackerTest::ExpectDefaultURLs() const {
   EXPECT_EQ(GURL(), fetched_google_url());
 }
 
-void GoogleURLTrackerTest::ExpectListeningForCommit(intptr_t unique_id,
+void GoogleURLTrackerTest::ExpectListeningForCommit(int unique_id,
                                                     bool listening) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   if (map_entry) {
@@ -476,61 +400,22 @@ void GoogleURLTrackerTest::ExpectListeningForCommit(intptr_t unique_id,
   }
 }
 
-infobars::InfoBar* GoogleURLTrackerTest::CreateTestInfoBar(
-    infobars::InfoBarManager* infobar_manager,
-    GoogleURLTracker* google_url_tracker,
-    const GURL& search_url) {
-  return TestInfoBarDelegate::Create(
-      this, infobar_manager, google_url_tracker, search_url);
-}
-
-
-// TestInfoBarDelegate --------------------------------------------------------
-
-namespace {
-
-// static
-infobars::InfoBar* TestInfoBarDelegate::Create(
-    GoogleURLTrackerTest* test_harness,
-    infobars::InfoBarManager* infobar_manager,
-    GoogleURLTracker* google_url_tracker,
-    const GURL& search_url) {
-  return ConfirmInfoBarDelegate::CreateInfoBar(
-      scoped_ptr<ConfirmInfoBarDelegate>(new TestInfoBarDelegate(
-          test_harness,
-          infobar_manager,
-          google_url_tracker,
-          search_url))).release();
-}
-
-TestInfoBarDelegate::TestInfoBarDelegate(
-    GoogleURLTrackerTest* test_harness,
-    infobars::InfoBarManager* infobar_manager,
-    GoogleURLTracker* google_url_tracker,
-    const GURL& search_url)
-    : GoogleURLTrackerInfoBarDelegate(google_url_tracker,
-                                      search_url),
-      test_harness_(test_harness),
-      infobar_manager_(infobar_manager) {
-}
-
-TestInfoBarDelegate::~TestInfoBarDelegate() {
-}
+infobars::InfoBarManager* GoogleURLTrackerTest::GetInfoBarManager(
+    int unique_id) {
+  base::ScopedPtrHashMap<int, infobars::InfoBarManager>::const_iterator it =
+      infobar_manager_map_.find(unique_id);
 
-void TestInfoBarDelegate::Update(const GURL& search_url) {
-  set_search_url(search_url);
-  set_pending_id(0);
-}
+  if (it == infobar_manager_map_.end()) {
+    // Lazily create an InfoBarManager for |unique_id|.
+    scoped_ptr<infobars::InfoBarManager> infobar_manager(
+        new TestInfoBarManager);
+    it = infobar_manager_map_.set(unique_id, infobar_manager.Pass());
+  }
 
-void TestInfoBarDelegate::Close(bool redo_search) {
-  test_harness_->OnInfoBarClosed(scoped_ptr<infobars::InfoBar>(infobar()),
-                                 infobar_manager_);
-  // WARNING: At this point |this| has been deleted!
+  DCHECK(it->second);
+  return it->second;
 }
 
-}  // namespace
-
-
 // Tests ----------------------------------------------------------------------
 
 TEST_F(GoogleURLTrackerTest, DontFetchWhenNoOneRequestsCheck) {