Fix a number of problems with the GoogleURLTracker

chrome/browser/google/google_url_tracker.h

 // 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_GOOGLE_GOOGLE_URL_TRACKER_H_
 #define CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_H_
 #pragma once
 
+#include <map>
 #include <string>
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/tab_contents/confirm_infobar_delegate.h"
 #include "content/public/common/url_fetcher.h"
 #include "content/public/common/url_fetcher_delegate.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ skipping 25 matching lines @@
                          public content::NotificationObserver,
                          public net::NetworkChangeNotifier::IPAddressObserver {
  public:
   // The constructor does different things depending on which of these values
   // you pass it.  Hopefully these are self-explanatory.
   enum Mode {
     NORMAL_MODE,
     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);
 
   virtual ~GoogleURLTracker();
 
   // Returns the current Google URL.  This will return a valid URL even in
   // unittest mode.
   //
   // This is the only function most code should ever call.
@@ skipping 16 matching lines @@
   // When the load commits, we'll show the infobar.
   static void GoogleURLSearchCommitted();
 
   static const char kDefaultGoogleHomepage[];
   static const char kSearchDomainCheckURL[];
 
  private:
   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
   // consumer should observe this notification before calling this.
   void SetNeedToFetch();
 
   // Called when the five second startup sleep has finished.  Runs any pending
   // fetch.
   void FinishSleep();
 
   // Starts the fetch of the up-to-date Google URL if we actually want to fetch
   // it and can currently do so.
   void StartFetchIfDesirable();
 
   // content::URLFetcherDelegate
   virtual void OnURLFetchComplete(const content::URLFetcher* source) OVERRIDE;
 
   // content::NotificationObserver
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
   // NetworkChangeNotifier::IPAddressObserver
   virtual void OnIPAddressChanged() OVERRIDE;
 
   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_;
   // TODO(ukai): GoogleURLTracker should track google domain (e.g. google.co.uk)
   // rather than URL (e.g. http://www.google.co.uk/), so that user could
   // configure to use https in search engine templates.
   GURL google_url_;
   GURL fetched_google_url_;
   base::WeakPtrFactory<GoogleURLTracker> weak_ptr_factory_;
   scoped_ptr<content::URLFetcher> fetcher_;
   int fetcher_id_;
   bool in_startup_sleep_;  // True if we're in the five-second "no fetching"
                            // period that begins at browser start.
   bool already_fetched_;   // True if we've already fetched a URL once this run;
                            // we won't fetch again until after a restart.
   bool need_to_fetch_;     // True if a consumer actually wants us to fetch an
                            // updated URL.  If this is never set, we won't
                            // bother to fetch anything.
                            // Consumers should observe
                            // chrome::GOOGLE_URL_UPDATED.
   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);
 };
 
 
 // This infobar delegate is declared here (rather than in the .cc file) so test
 // code can subclass it.
 class GoogleURLTrackerInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
   GoogleURLTrackerInfoBarDelegate(InfoBarTabHelper* infobar_helper,
+                                  const GURL& search_url,
                                   GoogleURLTracker* google_url_tracker,
                                   const GURL& new_google_url);
 
   // ConfirmInfoBarDelegate:
   virtual bool Accept() OVERRIDE;
   virtual bool Cancel() OVERRIDE;
   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:
   virtual string16 GetMessageText() const OVERRIDE;
   virtual string16 GetButtonLabel(InfoBarButton button) const OVERRIDE;
 
   // Returns the portion of the appropriate hostname to display.
   string16 GetHost(bool new_host) const;
 
   DISALLOW_COPY_AND_ASSIGN(GoogleURLTrackerInfoBarDelegate);
 };
 
 #endif  // CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_H_