Abstract GoogleURLTracker & google_util Profile dependencies

chrome/browser/google/google_url_tracker.h

Index: chrome/browser/google/google_url_tracker.h
diff --git a/chrome/browser/google/google_url_tracker.h b/chrome/browser/google/google_url_tracker.h
index 5d1d6122feae8a0d8c7b83b13c21e7c865a29eac..677c7c14704aaa4d396e2f2795972b161dc44c95 100644
--- a/chrome/browser/google/google_url_tracker.h
+++ b/chrome/browser/google/google_url_tracker.h
@@ -34,10 +34,11 @@ class InfoBar;
 // 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
@@ -63,20 +64,15 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
   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
@@ -85,17 +81,13 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
   // 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);
@@ -106,8 +98,8 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
   // 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.
@@ -158,11 +150,6 @@ class GoogleURLTracker : public net::URLFetcherDelegate,
   // 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.