Fix a number of problems with the GoogleURLTracker

chrome/browser/google/google_url_tracker.h

Index: chrome/browser/google/google_url_tracker.h
===================================================================
--- chrome/browser/google/google_url_tracker.h	(revision 132905)
+++ chrome/browser/google/google_url_tracker.h	(working copy)
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_H_
 #pragma once
 
+#include <map>
 #include <string>
 
 #include "base/gtest_prod_util.h"
@@ -51,8 +52,8 @@
     UNIT_TEST_MODE,
   };
 
-  // Only the main browser process loop should call this, when setting up
-  // g_browser_process->google_url_tracker_.  No code other than the
+  // Only the main browser process loop and tests should call this, when setting
+  // up g_browser_process->google_url_tracker_.  No code other than the
   // GoogleURLTracker itself should actually use
   // g_browser_process->google_url_tracker().
   explicit GoogleURLTracker(Mode mode);
@@ -89,14 +90,17 @@
   friend class GoogleURLTrackerInfoBarDelegate;
   friend class GoogleURLTrackerTest;
 
-  typedef InfoBarDelegate* (*InfoBarCreator)(InfoBarTabHelper*,
-                                             GoogleURLTracker*,
-                                             const GURL&);
+  typedef std::map<const InfoBarTabHelper*,
+                   GoogleURLTrackerInfoBarDelegate*> InfoBars;

[Ilya Sherman, 2012/04/24 00:30:47]

nit: Perhaps name this "InfoBarMap", so there's at least some hint to a reader
of what the backing data structure is?

[Peter Kasting, 2012/04/24 01:53:01]

Good idea.  Changed type and variable name.

+  typedef GoogleURLTrackerInfoBarDelegate* (*InfoBarCreator)(
+      InfoBarTabHelper* infobar_helper,
+      const GURL& search_url,
+      GoogleURLTracker* google_url_tracker,
+      const GURL& new_google_url);
 
   void AcceptGoogleURL(const GURL& google_url);
   void CancelGoogleURL(const GURL& google_url);
-  void InfoBarClosed();
-  void RedoSearch();
+  void InfoBarClosed(const InfoBarTabHelper* infobar_helper);

[Ilya Sherman, 2012/04/24 00:30:47]

nit: I think we generally prefer passing by const-ref rather than const-pointer.

[Peter Kasting, 2012/04/24 01:53:01]

Doing this would break the test code that needs to cast an int to this type.

I tried replacing "const InfoBarTabHelper*" with a typedefed type "MapKey" in
all the places where we used it as a key rather than as a class to call methods
on, in an attempt to make this clearer.  Unfortunately this devolved badly for a
variety of reasons, and after some experimentation with template types I
eventually gave up and came back to the current implementation.

 
   // Registers consumer interest in getting an updated URL from the server.
   // It will be notified as chrome::GOOGLE_URL_UPDATED, so the
@@ -124,10 +128,15 @@
 
   void SearchCommitted();
   void OnNavigationPending(const content::NotificationSource& source,
-                           const GURL& pending_url);
-  void OnNavigationCommittedOrTabClosed(content::WebContents* web_contents,
-                                        int type);
-  void ShowGoogleURLInfoBarIfNecessary(content::WebContents* web_contents);
+                           const content::NotificationSource& contents_source,
+                           InfoBarTabHelper* infobar_helper,
+                           const GURL& search_url);

[Ilya Sherman, 2012/04/24 00:30:47]

nit: const params generally should precede non-const (out-) params.  (Perhaps
|infobar_helper| should be const too?)

[Peter Kasting, 2012/04/24 01:53:01]

|infobar_helper| isn't an outparam here, but can't be non-const because we need
to call a method on it.

+  void OnNavigationCommittedOrTabClosed(
+      const content::NotificationSource& source,
+      const content::NotificationSource& contents_source,
+      InfoBarTabHelper* infobar_helper,
+      int type);

[Ilya Sherman, 2012/04/24 00:30:47]

nit: const params generally should precede non-const (out-) params.  (Perhaps
|infobar_helper| should be const too?)

[Peter Kasting, 2012/04/24 01:53:01]

Made this one const.

+  void CloseAllInfoBars(bool redo_searches);
 
   content::NotificationRegistrar registrar_;
   InfoBarCreator infobar_creator_;
@@ -151,9 +160,7 @@
   bool need_to_prompt_;    // True if the last fetched Google URL is not
                            // matched with current user's default Google URL
                            // nor the last prompted Google URL.
-  content::NavigationController* controller_;
-  InfoBarDelegate* infobar_;
-  GURL search_url_;
+  InfoBars infobars_;
 
   DISALLOW_COPY_AND_ASSIGN(GoogleURLTracker);
 };
@@ -164,6 +171,7 @@
 class GoogleURLTrackerInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
   GoogleURLTrackerInfoBarDelegate(InfoBarTabHelper* infobar_helper,
+                                  const GURL& search_url,
                                   GoogleURLTracker* google_url_tracker,
                                   const GURL& new_google_url);
 
@@ -173,11 +181,18 @@
   virtual string16 GetLinkText() const OVERRIDE;
   virtual bool LinkClicked(WindowOpenDisposition disposition) OVERRIDE;
 
+  // These are virtual so test code can override them in a subclass.
+  virtual void Show();
+  virtual void Close(bool redo_search);
+
  protected:
   virtual ~GoogleURLTrackerInfoBarDelegate();
 
+  InfoBarTabHelper* map_key_;  // What |google_url_tracker_| uses to track us.
+  const GURL search_url_;
   GoogleURLTracker* google_url_tracker_;
   const GURL new_google_url_;
+  bool showing_;  // True if this delegate has been added to a TabContents.
 
  private:
   // ConfirmInfoBarDelegate: