Split the existing GoogleURLTrackerInfoBarDelegate into two classes

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_
 
 #include <map>
 #include <string>
 #include <utility>
@@ skipping 75 matching lines @@
   // NAV_ENTRY_PENDING notification to get the appropriate NavigationController.
   // When the load commits, we'll show the infobar.
   //
   // When |profile| is NULL or a testing profile, this function does nothing.
   static void GoogleURLSearchCommitted(Profile* profile);
 
   static const char kDefaultGoogleHomepage[];
   static const char kSearchDomainCheckURL[];
 
  private:
   friend class GoogleURLTrackerInfoBarDelegate;

[Ilya Sherman, 2012/10/16 20:16:24]

Whoa, I just noticed this.  How 'bout we expose public getters for the bits of
internals that the infobar delegate needs, and drop this friend declaration?

[Peter Kasting, 2012/10/16 23:29:35]

OK.  I hadn't done this so far because the current public API amounts to a set
of safe static functions that general consumers of the GoogleURLTracker should
use, whereas all the things the infobar needs are non-static methods that no one
but it should be calling; so I left it as a friend because it's something of an
implementation detail.

Publicizing these means other classes can call them, which we don't really want.
 But I share your aversion to friend declarations, and this would mean that the
infobar itself becomes much more limited in what it can access, which is a good
thing.

   friend class GoogleURLTrackerTest;
 
-  struct MapEntry {
-    MapEntry();  // Required by STL.
-    MapEntry(GoogleURLTrackerInfoBarDelegate* infobar,
-             const content::NotificationSource& navigation_controller_source,
-             const content::NotificationSource& web_contents_source);
-    ~MapEntry();
+  class MapEntry;

[Ilya Sherman, 2012/10/16 20:16:24]

How about defining this in a separate header file?

[Peter Kasting, 2012/10/16 23:29:35]

:/ I'm less convinced of the benefits of this, but OK.

 
-    GoogleURLTrackerInfoBarDelegate* infobar;
-    content::NotificationSource navigation_controller_source;
-    content::NotificationSource web_contents_source;
-  };
+  typedef std::map<const InfoBarTabHelper*, MapEntry*> InfoBarMap;
 
-  typedef std::map<const InfoBarTabHelper*, MapEntry> InfoBarMap;
+  // Creates an infobar delegate and adds it to |infobar_helper|.  Returns the
+  // delegate pointer on success or NULL on failure.  The caller does not own
+  // the returned object, the InfoBarTabHelper does.
   typedef GoogleURLTrackerInfoBarDelegate* (*InfoBarCreator)(
       InfoBarTabHelper* infobar_helper,
       GoogleURLTracker* google_url_tracker,
       const GURL& new_google_url);
 
   // net::URLFetcherDelegate:
   virtual void OnURLFetchComplete(const net::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;
 
   // ProfileKeyedService:
   virtual void Shutdown() OVERRIDE;
 
   // Callbacks from GoogleURLTrackerInfoBarDelegate:
-  void AcceptGoogleURL(const GURL& google_url, bool redo_searches);
-  void CancelGoogleURL(const GURL& google_url);
-  void InfoBarClosed(const InfoBarTabHelper* infobar_helper);
+  void AcceptGoogleURL(bool redo_searches);
+  void CancelGoogleURL();
+
+  // Callback from MapEntry:
+  void DeleteMapEntryForHelper(const InfoBarTabHelper* infobar_helper);
 
   // Registers consumer interest in getting an updated URL from the server.
   // Observe chrome::NOTIFICATION_GOOGLE_URL_UPDATED to be notified when the URL
   // changes.
   void SetNeedToFetch();
 
   // Called when the five second startup sleep has finished.  Runs any pending
   // fetch.
   void FinishSleep();
 
@@ skipping 20 matching lines @@
       const content::NotificationSource& navigation_controller_source,
       const content::NotificationSource& web_contents_source,
       InfoBarTabHelper* infobar_helper,
       int pending_id);
 
   // Called by Observe() once a load we're watching commits, or the associated
   // tab is closed.  |infobar_helper| is the same as for OnNavigationPending();
   // |search_url| is valid when this call is due to a successful navigation
   // (indicating that we should show or update the relevant infobar) as opposed
   // to tab closure (which means we should delete the infobar).
-  void OnNavigationCommittedOrTabClosed(const InfoBarTabHelper* infobar_helper,
+  void OnNavigationCommittedOrTabClosed(InfoBarTabHelper* infobar_helper,
                                         const GURL& search_url);
 
   // Called by Observe() when an instant navigation occurs.  This will call
   // OnNavigationPending(), and, depending on whether this is a search we were
   // listening for, may then also call OnNavigationCommittedOrTabClosed().
   void OnInstantCommitted(
     const content::NotificationSource& navigation_controller_source,
     const content::NotificationSource& web_contents_source,
     InfoBarTabHelper* infobar_helper,
     const GURL& search_url);
 
   // Closes all open infobars.  If |redo_searches| is true, this also triggers
   // each tab to re-perform the user's search, but on the new Google TLD.
   void CloseAllInfoBars(bool redo_searches);
 
   // Unregisters any listeners for the notification sources in |map_entry|.
   // This sanity-DCHECKs that these are registered (or not) in the specific
   // cases we expect.  (|must_be_listening_for_commit| is used purely for this
   // sanity-checking.)  This also unregisters our global NAV_ENTRY_PENDING/
   // INSTANT_COMMITTED listeners if there are no remaining listeners for
   // NAV_ENTRY_COMMITTED, as we no longer need them until another search is
   // committed.
   void UnregisterForEntrySpecificNotifications(
       const MapEntry& map_entry,
       bool must_be_listening_for_commit);
 
   Profile* profile_;
   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.

[Ilya Sherman, 2012/10/16 20:16:24]

Is this being removed because it's no longer appropriate, because you've
implemented it, or some other reason?

[Peter Kasting, 2012/10/16 23:29:35]

We solved this problem in a different way by supporting HTTPS via
searchdomaincheck.

   GURL google_url_;
   GURL fetched_google_url_;
   base::WeakPtrFactory<GoogleURLTracker> weak_ptr_factory_;
   scoped_ptr<net::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::NOTIFICATION_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.
   bool search_committed_;  // True when we're expecting a notification of a new
                            // pending search navigation.
   InfoBarMap infobar_map_;
 
   DISALLOW_COPY_AND_ASSIGN(GoogleURLTracker);
 };
 
+class GoogleURLTracker::MapEntry : public content::NotificationObserver {
+ public:
+  MapEntry(GoogleURLTracker* google_url_tracker,
+           InfoBarTabHelper* infobar_helper,
+           const content::NotificationSource& navigation_controller_source,
+           const content::NotificationSource& web_contents_source);
+  virtual ~MapEntry();
+
+  bool has_infobar() const { return !!infobar_; }
+  GoogleURLTrackerInfoBarDelegate* infobar() { return infobar_; }
+  void SetInfoBar(GoogleURLTrackerInfoBarDelegate* infobar);
+
+  const content::NotificationSource& navigation_controller_source() const {
+    return navigation_controller_source_;
+  }
+  const content::NotificationSource& web_contents_source() const {
+    return web_contents_source_;
+  }
+
+  void Close(bool redo_search);
+
+ private:
+  friend class GoogleURLTrackerTest;
+
+  // content::NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  content::NotificationRegistrar registrar_;
+  GoogleURLTracker* google_url_tracker_;
+  InfoBarTabHelper* infobar_helper_;

[Ilya Sherman, 2012/10/16 20:16:24]

nit: Can these be const members?

[Peter Kasting, 2012/10/16 23:29:35]

Added as much const as possible.

+  GoogleURLTrackerInfoBarDelegate* infobar_;
+  content::NotificationSource navigation_controller_source_;
+  content::NotificationSource web_contents_source_;

[Ilya Sherman, 2012/10/16 20:16:24]

nit: Can these be const members?

[Peter Kasting, 2012/10/16 23:29:35]

Yes.

+
+  DISALLOW_COPY_AND_ASSIGN(MapEntry);
+};
+
 #endif  // CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_H_