Remove Infobars notifications from GoogleURLTrackerMapEntry

chrome/browser/google/google_url_tracker_unittest.cc

-// 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.
 
 #include "chrome/browser/google/google_url_tracker.h"
 
 #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"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
 #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 {
  public:
   TestCallbackListener();
   virtual ~TestCallbackListener();
 
   bool HasRegisteredCallback();
   void RegisterCallback(GoogleURLTracker* google_url_tracker);
 
@@ skipping 116 matching lines @@
 bool TestGoogleURLTrackerNavigationHelper::IsListeningForTabDestruction() {
   return listening_for_tab_destruction_;
 }
 
 void TestGoogleURLTrackerNavigationHelper::OpenURL(
     GURL url,
     WindowOpenDisposition disposition,
     bool user_clicked_on_link) {
 }
 
+
+// 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();
 
   // testing::Test
   virtual void SetUp() OVERRIDE;
   virtual void TearDown() OVERRIDE;
 
   net::TestURLFetcher* GetFetcher();
   void MockSearchDomainCheckResponse(const std::string& domain);
   void RequestServerCheck();
   void FinishSleep();
   void NotifyIPAddressChanged();
   GURL fetched_google_url() const {
     return google_url_tracker_->fetched_google_url();
   }
   void set_google_url(const GURL& url) {
     google_url_tracker_->google_url_ = url;
   }
   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_;
   net::TestURLFetcherFactory fetcher_factory_;
   GoogleURLTrackerClient* client_;
   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()->
       RegisterUserPrefsOnBrowserContextForTest(&profile_);
 }
 
 GoogleURLTrackerTest::~GoogleURLTrackerTest() {
 }
 
 void GoogleURLTrackerTest::SetUp() {
   network_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock());
   // Ownership is passed to google_url_tracker_, but a weak pointer is kept;
   // this is safe since GoogleURLTracker keeps the client for its lifetime.
   client_ = new TestGoogleURLTrackerClient();
   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() {
   while (!unique_ids_seen_.empty())
     CloseTab(*unique_ids_seen_.begin());
   google_url_tracker_->Shutdown();
 }
 
 net::TestURLFetcher* GoogleURLTrackerTest::GetFetcher() {
   // This will return the last fetcher created.  If no fetchers have been
@@ skipping 32 matching lines @@
 }
 
 void GoogleURLTrackerTest::SetLastPromptedGoogleURL(const GURL& url) {
   profile_.GetPrefs()->SetString(prefs::kLastPromptedGoogleURL, url.spec());
 }
 
 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
     // for navigation starts if the searchdomaincheck response was bogus.
   }
   unique_ids_seen_.insert(unique_id);
   if (client_->IsListeningForNavigationStart()) {
     google_url_tracker_->OnNavigationPending(
         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;
 
   ExpectListeningForCommit(unique_id, false);
 
   // The infobar should be showing; otherwise the pending non-search should
   // have closed it.
   ASSERT_TRUE(map_entry->has_infobar_delegate());
 
   // The pending_id should have been reset to 0 when the non-search became
   // pending.
   EXPECT_EQ(0, map_entry->infobar_delegate()->pending_id());
 
   // Committing the navigation would close the infobar.
   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()) {
     google_url_tracker_->OnTabClosed(nav_helper);
   } else {
     // Closing a tab with an infobar showing would close the infobar.
     GoogleURLTrackerInfoBarDelegate* delegate = GetInfoBarDelegate(unique_id);
     if (delegate)
       delegate->Close(false);
   }
 }
 
-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;
 }
 
 void GoogleURLTrackerTest::ExpectDefaultURLs() const {
   EXPECT_EQ(GURL(GoogleURLTracker::kDefaultGoogleHomepage), google_url());
   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) {
     EXPECT_EQ(listening,
               map_entry->navigation_helper()->IsListeningForNavigationCommit());
   } else {
     EXPECT_FALSE(listening);
   }
 }
 
-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);
+infobars::InfoBarManager* GoogleURLTrackerTest::GetInfoBarManager(
+    int unique_id) {
+  base::ScopedPtrHashMap<int, infobars::InfoBarManager>::const_iterator it =
+      infobar_manager_map_.find(unique_id);
+
+  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());
+  }
+
+  DCHECK(it->second);
+  return it->second;
 }
 
-
-// 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() {
-}
-
-void TestInfoBarDelegate::Update(const GURL& search_url) {
-  set_search_url(search_url);
-  set_pending_id(0);
-}
-
-void TestInfoBarDelegate::Close(bool redo_search) {
-  test_harness_->OnInfoBarClosed(scoped_ptr<infobars::InfoBar>(infobar()),
-                                 infobar_manager_);
-  // WARNING: At this point |this| has been deleted!
-}
-
-}  // namespace
-
-
 // Tests ----------------------------------------------------------------------
 
 TEST_F(GoogleURLTrackerTest, DontFetchWhenNoOneRequestsCheck) {
   ExpectDefaultURLs();
   FinishSleep();
   // No one called RequestServerCheck() so nothing should have happened.
   EXPECT_FALSE(GetFetcher());
   MockSearchDomainCheckResponse("http://www.google.co.uk/");
   ExpectDefaultURLs();
   EXPECT_FALSE(listener_notified());
@@ skipping 566 matching lines @@
   CommitSearch(2, GURL("http://www.google.co.uk/search?q=test2"));
   EXPECT_FALSE(GetInfoBarDelegate(1) == NULL);
   GoogleURLTrackerInfoBarDelegate* delegate2 = GetInfoBarDelegate(2);
   ASSERT_FALSE(delegate2 == NULL);
   SetNavigationPending(1, true);
   ASSERT_NO_FATAL_FAILURE(ExpectListeningForCommit(1, true));
   delegate2->Close(false);
   SetNavigationPending(1, false);
   ASSERT_NO_FATAL_FAILURE(ExpectListeningForCommit(1, false));
 }