Abstract GoogleURLTracker & google_util Profile dependencies

Abstract GoogleURLTracker & google_util Profile dependencies

This CL eliminates GoogleURLTracker and google_util having static functions
that take in Profiles as arguments and use the Profile to get at the
GoogleURLTracker instance. Specifically, it does the following:

- Introduces google_profile_helper, as well as a new
  GetGoogleHomePageURL(Profile) function that serves the purpose previously
  being served by GoogleURLTracker::GoogleURL(Profile).
- The google_util GetGoogleCountryCode(Profile) and GetGoogleSearchURL(Profile)
  functions now take in the Google homepage URL to operate on rather than the
  Profile.
- Turns GoogleURLTracker's static RequestServerCheck(Profile) and
  GoogleURLSearchCommitted(Profile) into instance methods, changing callsites
  to get the tracker from the factory and call the instance method on the
  tracker if it is not NULL.

GoogleURLTracker still uses the Profile to get the Prefs; this will be changed
in a different CL.

BUG=373235, 373223
TBR=thakis

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274679

[blundell, 2014-05-30 12:44:55]

Hey Peter,

After staring at the code for a while, this was the approach that seemed most
natural to me. I'm open to alternatives though if you see another structure that
you like better. Thanks!

[Peter Kasting, 2014-05-30 20:50:38]

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_profile_helper.cc (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_profile_helper.cc:17: return
google_util::GetGoogleCountryCode(GetGoogleHomePageURL(profile));
I wonder if this function, and the bottom function, really need to be here. 
Seems like callers could do these directly, unless there are a ton of them.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_profile_helper.cc:24: :
GoogleURLTracker::DefaultGoogleHomepage();
Nit: Wrap like:

  return tracker ?
      tracker->google_url() : GoogleURLTracker::DefaultGoogleHomepage();

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_profile_helper.h (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_profile_helper.h:5: // Provides Google-related
information for a given Profile.
I would expand this file comment to say something along the lines of these
largely being wrappers for caller convenience around google component functions,
which can't depend on Profiles.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_profile_helper.h:17: namespace
google_profile_helper {
So, how did you end up deciding to put these functions here, but not wrappers
for the "request server check" or "search committed" calls?  It seems a little
inconsistent that some places people should use wrappers, and other places they
should talk to the Google URL tracker object directly.

Also, everywhere that calls those directly, you've made conditional against a
NULL tracker, which implies that's actually possible at those callsites -- but I
don't know whether it _is_ possible at all those callsites (and where it's not,
we shouldn't NULL-check).

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker.h (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:38: //
google_profile_helper::GetGoogleHomePageURL(), which is guaranteed to
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.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:64: static const char
kDefaultGoogleHomepage[];
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.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:65: static const char
kSearchDomainCheckURL[];
It looks like this is only exposed for tests, so perhaps it should be private
with some friend declarations or something (not sure)

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:90: void GoogleURLSearchCommitted();
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".

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:92: // No one but
GoogleURLTrackerInfoBarDelegate or test code should call these.
This comment is now incorrect for google_url() (since the profile helper calls
it).

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_util.h (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_util.h:43: std::string GetGoogleCountryCode(GURL
google_homepage_url);
Nit: Pass GURL by const ref (2 places)

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/search_terms_data.cc (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/search_terms_data.cc:104: :
SearchTermsData::GoogleBaseURLValue();
Nit: Wrap like:

  return profile_ ?
      google_profile_helper::GetGoogleHomePageURL(profile_).spec() :
      SearchTermsData::GoogleBaseURLValue();

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

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_profile_helper.cc (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_profile_helper.cc:17: return
google_util::GetGoogleCountryCode(GetGoogleHomePageURL(profile));
Yep, you're right. Done.

On 2014/05/30 20:50:39, Peter Kasting wrote:
> I wonder if this function, and the bottom function, really need to be here. 
> Seems like callers could do these directly, unless there are a ton of them.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_profile_helper.cc:24: :
GoogleURLTracker::DefaultGoogleHomepage();
On 2014/05/30 20:50:39, Peter Kasting wrote:
> Nit: Wrap like:
> 
>   return tracker ?
>       tracker->google_url() : GoogleURLTracker::DefaultGoogleHomepage();

Done.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_profile_helper.h (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_profile_helper.h:17: namespace
google_profile_helper {
On 2014/05/30 20:50:39, Peter Kasting wrote:
> So, how did you end up deciding to put these functions here, but not wrappers
> for the "request server check" or "search committed" calls?  It seems a little
> inconsistent that some places people should use wrappers, and other places
they
> should talk to the Google URL tracker object directly.
>

I created the GetGoogleHomePageURL() function since that has the logic of what
URL to set as the default if the GoogleURLTracker doesn't exist, which I didn't
want to replicate at all the callsites. For the other two, I agree that they're
not necessary; removed now.
 
> Also, everywhere that calls those directly, you've made conditional against a
> NULL tracker, which implies that's actually possible at those callsites -- but
I
> don't know whether it _is_ possible at all those callsites (and where it's
not,
> we shouldn't NULL-check).

The GoogleURLTrackerFactory has the service be NULL while testing, so any code
that calls GoogleURLTrackerFactory::GetForProfile() does need to guard against
the service being NULL (if it doesn't blow up, it's just because that code isn't
currently covered by any tests).

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker.h (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:38: //
google_profile_helper::GetGoogleHomePageURL(), which is guaranteed to
Yep, you're right re: the layering violation. Changed as you suggested.

On 2014/05/30 20:50:39, Peter Kasting wrote:
> 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.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:64: static const char
kDefaultGoogleHomepage[];
On 2014/05/30 20:50:39, Peter Kasting wrote:
> 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.

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.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:65: static const char
kSearchDomainCheckURL[];
On 2014/05/30 20:50:39, Peter Kasting wrote:
> It looks like this is only exposed for tests, so perhaps it should be private
> with some friend declarations or something (not sure)

Done.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:90: void GoogleURLSearchCommitted();
On 2014/05/30 20:50:39, Peter Kasting wrote:
> 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".

Done.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker.h:92: // No one but
GoogleURLTrackerInfoBarDelegate or test code should call these.
On 2014/05/30 20:50:39, Peter Kasting wrote:
> This comment is now incorrect for google_url() (since the profile helper calls
> it).

Done.

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_util.h (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_util.h:43: std::string GetGoogleCountryCode(GURL
google_homepage_url);
Functions removed entirely.

On 2014/05/30 20:50:39, Peter Kasting wrote:
> Nit: Pass GURL by const ref (2 places)

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/search_terms_data.cc (right):

https://codereview.chromium.org/303233006/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/search_terms_data.cc:104: :
SearchTermsData::GoogleBaseURLValue();
On 2014/05/30 20:50:39, Peter Kasting wrote:
> Nit: Wrap like:
> 
>   return profile_ ?
>       google_profile_helper::GetGoogleHomePageURL(profile_).spec() :
>       SearchTermsData::GoogleBaseURLValue();

Done.

[Peter Kasting, 2014-06-02 18:50:18]

LGTM.  It seems lame to have this whole goog_profile_helper namespace and file
just for one tiny function, but I don't know of a better place to put it.

[blundell, 2014-06-02 18:56:27]

On 2014/06/02 18:50:18, Peter Kasting wrote:
> LGTM.  It seems lame to have this whole goog_profile_helper namespace and file
> just for one tiny function, but I don't know of a better place to put it.

Yeah, I agree with both points :|. I had toyed with the idea of getting rid of
the function entirely like the other ones, but the tradeoff (making life more
cumbersome for the callsites) seems not worth it in this case. The other place
to put it that I can see is the factory, but that seems even weirder, as it's
not the factory's role.

[blundell, 2014-06-03 08:34:08]

TBR=thakis for //chrome outside of //chrome/browser/google

[blundell, 2014-06-03 08:34:36]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-03 08:36:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/303233006/60001

[commit-bot: I haz the power, 2014-06-03 09:32:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/12154)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/15237)

[commit-bot: I haz the power, 2014-06-03 09:35:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-03 09:35:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Nico, 2014-06-03 10:15:30]

Is the CL description up-to-date? I don't see a GetGoogleCountryCode in
google_profile_helper.

[blundell, 2014-06-03 11:49:45]

whoops, CL description updated.

[blundell, 2014-06-03 11:49:53]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-03 11:50:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/303233006/80001

[blundell, 2014-06-03 20:15:11]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-03 20:16:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/303233006/100001

[commit-bot: I haz the power, 2014-06-04 00:14:57]

Change committed as 274679