Abstract GoogleURLTracker & google_util Profile dependencies

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 16 matching lines @@
 class Profile;
 
 namespace infobars {
 class InfoBar;
 }
 
 // This object is responsible for checking the Google URL once per network
 // change, and if necessary prompting the user to see if they want to change to
 // using it.  The current and last prompted values are saved to prefs.
 //
-// Most consumers should only call GoogleURL(), which is guaranteed to
-// synchronously return a value at all times (even during startup or in unittest
-// mode).  Consumers who need to be notified when things change should register
-// a callback that provides the original and updated values via
+// Most consumers should only call
+// google_profile_helper::GetGoogleHomePageURL(), which is guaranteed to

[Peter Kasting, 2014/05/30 20:50:39]

Nit: Maybe "Most consumers should not use this class directly, but should
instead call google_profile_helper::GetGoogleHomePageURL(), ..."

But I worry that if you're going to move this code to a component directly, this
is sort of a "layering violation" as a comment in that this code shouldn't
really know that that wrapper exists.

Perhaps instead, this comment should merely say that most consumers should only
call google_url(), and then that function should be pulled out of the group
below and given a separate comment (see also my comment there).  Then on the
Chrome side, the profile helper header can note that consumers should use it if
they need to be robust against being called from tests and the like.

[blundell, 2014/06/02 15:42:26]

Yep, you're right re: the layering violation. Changed as you suggested.

On 2014/05/30 20:50:39, Peter Kasting wrote:

+// synchronously return a value at all times (even during startup or in
+// unittest mode).  Consumers who need to be notified when things change should
+// register a callback that provides the original and updated values via
 // RegisterCallback().
 //
 // To protect users' privacy and reduce server load, no updates will be
 // performed (ever) unless at least one consumer registers interest by calling
 // RequestServerCheck().
 class GoogleURLTracker : public net::URLFetcherDelegate,
                          public net::NetworkChangeNotifier::IPAddressObserver,
                          public KeyedService {
  public:
   // Callback that is called when the Google URL is updated. The arguments are
   // the old and new URLs.
   typedef base::Callback<void(GURL, GURL)> OnGoogleURLUpdatedCallback;
   typedef base::CallbackList<void(GURL, GURL)> CallbackList;
   typedef CallbackList::Subscription Subscription;
 
   // 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,
   };
 
   static const char kDefaultGoogleHomepage[];

[Peter Kasting, 2014/05/30 20:50:39]

Now that you have DefaultGogoleHomepage(), I think all people who refer to this
member should instead call that function, and this should become a
function-scope variable inside that function.

[blundell, 2014/06/02 15:42:26]

Oops, I hadn't noticed that this was already public. It would be fiddly to
eliminate all the clients of this since some of them use it as a string.
Reverted the addition of the new static function instead.

   static const char kSearchDomainCheckURL[];

[Peter Kasting, 2014/05/30 20:50:39]

It looks like this is only exposed for tests, so perhaps it should be private
with some friend declarations or something (not sure)

[blundell, 2014/06/02 15:42:26]

Done.

 
-  // Only the GoogleURLTrackerFactory and tests should call this.  No code other
-  // than the GoogleURLTracker itself should actually use
-  // GoogleURLTrackerFactory::GetForProfile().
+  // Only the GoogleURLTrackerFactory and tests should call this.
   GoogleURLTracker(Profile* profile,
                    scoped_ptr<GoogleURLTrackerClient> client,
                    Mode mode);
 
   virtual ~GoogleURLTracker();
 
-  // Returns the current Google URL.  This will return a valid URL even if
-  // |profile| is NULL or a testing profile.
-  //
-  // This is the only function most code should ever call.
-  static GURL GoogleURL(Profile* profile);
+  // Returns the URL corresponding to the default Google homepage.
+  static GURL DefaultGoogleHomepage();
 
   // Requests that the tracker perform a server check to update the Google URL
   // as necessary.  If |force| is false, this will happen at most once per
   // network change, not sooner than five seconds after startup (checks
   // requested before that time will occur then; checks requested afterwards
   // will occur immediately, if no other checks have been made during this run).
   // If |force| is true, and the tracker has already performed any requested
   // check, it will check again.
-  //
-  // When |profile| is NULL or a testing profile, this function does nothing.
-  static void RequestServerCheck(Profile* profile, bool force);
+  void RequestServerCheck(bool force);
 
   // Notifies the tracker that the user has started a Google search.
   // If prompting is necessary, we then listen for the subsequent pending
-  // navigation to get the appropriate NavigationController. When the load
+  // navigation to get the appropriate NavigationHelper. 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);
+  void GoogleURLSearchCommitted();

[Peter Kasting, 2014/05/30 20:50:39]

Nit: Now that there isn't a private SearchCommitted(), I might name this
SearchCommitted -- presumably, since the class is the GoogleURLTracker, it's
clear that this means "Google search".  If that isn't actually clear,
GoogleSearchCommitted() would probably be better than the existing
GoogleURLSearchCommitted(), since the latter sounds like "user searched for the
Google URL".

[blundell, 2014/06/02 15:42:26]

Done.

 
   // No one but GoogleURLTrackerInfoBarDelegate or test code should call these.

[Peter Kasting, 2014/05/30 20:50:39]

This comment is now incorrect for google_url() (since the profile helper calls
it).

[blundell, 2014/06/02 15:42:26]

Done.

   void AcceptGoogleURL(bool redo_searches);
   void CancelGoogleURL();
   const GURL& google_url() const { return google_url_; }
   const GURL& fetched_google_url() const { return fetched_google_url_; }
 
   // No one but GoogleURLTrackerMapEntry should call this.
   void DeleteMapEntryForService(const InfoBarService* infobar_service);
 
-  // Called by the client after SearchCommitted() registers listeners, to
-  // indicate that we've received the "load now pending" notification.
+  // Called by the client after GoogleURLSearchCommitted() registers listeners,
+  // to indicate that we've received the "load now pending" notification.
   // |nav_helper| is the GoogleURLTrackerNavigationHelper associated with this
   // navigation; |infobar_service| is the InfoBarService of the associated tab;
   // and |pending_id| is the unique ID of the newly pending NavigationEntry.
   // If there is already a visible GoogleURLTracker infobar for this tab, this
   // function resets its associated pending entry ID to the new ID.  Otherwise
   // this function creates a map entry for the associated tab.
   virtual void OnNavigationPending(
       scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
       InfoBarService* infobar_service,
       int pending_id);
@@ skipping 30 matching lines @@
   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();
 
-  // Called each time the user performs a search.  This checks whether we need
-  // to prompt the user about a domain change, and if so, starts listening for
-  // the notifications sent when the actual load is triggered.
-  void SearchCommitted();
-
   // Closes all map entries.  If |redo_searches| is true, this also triggers
   // each tab with an infobar to re-perform the user's search, but on the new
   // Google TLD.
   void CloseAllEntries(bool redo_searches);
 
   // Unregisters any listeners for the navigation helper 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 the global navigation pending
   // listener if there are no remaining listeners for navigation commits, as we
@@ skipping 35 matching lines @@
                            // nor the last prompted Google URL.
   bool search_committed_;  // True when we're expecting a notification of a new
                            // pending search navigation.
   EntryMap entry_map_;
   base::WeakPtrFactory<GoogleURLTracker> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(GoogleURLTracker);
 };
 
 #endif  // CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_H_