Add a URL param to indicate group selection in Instant field trial.

Add a URL param to indicate group selection in Instant field trial.

Currently, if the user is chosen into an Instant field trial group, this
information is not indicated in the suggest or search request URLs. Such a URL
param will allow us to analyze experiment metrics from server logs.

Most of the changes here are because we need to pass the Profile information
through to the Instant field trial code.

BUG=91536
TEST=Observe ix=c or ix=e showing up in the URLs sent for suggest/search.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96954

[sreeram, 2011-08-08 23:17:24]

Please review.

[sky, 2011-08-08 23:37:23]

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url.cc:67:
"instantFieldTrialGroupParameter";
Since this is google specific, use google:

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url.h:85: Profile* profile,
This too may be invoked from any thread, which implies it should not take a
Profile.

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url_service.h:100: Profile* profile,
This method shouldn't take a profile, it's intended to be invoked from any
thread. If you say you'll just supply NULL, that doesn't sit well with me as
it's all to easy to get it wrong and the signature implies you should pass in a
Profile.

[sky, 2011-08-08 23:38:13]

I've added Peter to the review list since I'll be OOO starting tomorrow.

  -Scott

[sreeram, 2011-08-09 16:24:01]

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url.cc:67:
"instantFieldTrialGroupParameter";
On 2011/08/08 23:37:23, sky wrote:
> Since this is google specific, use google:

Done. Will reflect it in the next patch.

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url.h:85: Profile* profile,
On 2011/08/08 23:37:23, sky wrote:
> This too may be invoked from any thread, which implies it should not take a
> Profile.

I am not sure how to interpret this.

Is the concern that if this takes a Profile* (which implies that it could modify
Profile), then multiple threads could call it without locking, with a potential
for data corruption? This could be fixed by guarding uses of the Profile* with a
lock (in instant_field_trial.cc), or even better, just making this a "const
Profile*" (I'll need to make some upstream changes to make
Profile::IsOffTheRecord() and Profile::GetPrefs() const, which they technically
are, but just aren't marked so).

Or, is the concern that the Profile could change from underneath a thread while
another thread is using it? This would seem to cause a lot more headaches in the
rest of Chrome (i.e., not just in this code), so I don't think this is the
concern, right?

If you are saying that this absolutely should not take a Profile at all, then
could you propose an alternative of how to achieve my goal (of passing the
necessary Profile information to the Instant field trial code)?

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/7558014/diff/1/chrome/browser/search_engines/t...
chrome/browser/search_engines/template_url_service.h:100: Profile* profile,
On 2011/08/08 23:37:23, sky wrote:
> This method shouldn't take a profile, it's intended to be invoked from any
> thread. If you say you'll just supply NULL, that doesn't sit well with me as
> it's all to easy to get it wrong and the signature implies you should pass in
a
> Profile.

I only supply NULL where the caller strips away the params immediately (and
where passing a real Profile* would require jumping through a lot more hoops).
For example, see the calls in search_host_to_urls_map.cc.

In places where a Profile is readily available, I do supply it. For example, see
the calls in template_url_prepopulate_data_unittest.cc. It turns out that all
such cases are in test code, where the tests would pass just fine even if I
passed NULL.

So, if you prefer, I can remove the Profile* parameter from the
GenerateSearchURL*() methods, which would mean that I would then pass NULL
instead on line 214 in template_url_service.cc, i.e., where
GenerateSearchURLUsingTermsData() calls ReplaceSearchTermsUsingTermsData().
Would you prefer that?

[Peter Kasting, 2011-08-09 19:43:15]

On 2011/08/09 16:24:01, sreeram wrote:
> On 2011/08/08 23:37:23, sky wrote:
> > This too may be invoked from any thread, which implies it should not take a
> > Profile.
> 
> Is the concern that if this takes a Profile* (which implies that it could
modify
> Profile), then multiple threads could call it without locking, with a
potential
> for data corruption? This could be fixed by guarding uses of the Profile* with
a
> lock (in instant_field_trial.cc), or even better, just making this a "const
> Profile*" (I'll need to make some upstream changes to make
> Profile::IsOffTheRecord() and Profile::GetPrefs() const, which they
technically
> are, but just aren't marked so).
> 
> Or, is the concern that the Profile could change from underneath a thread
while
> another thread is using it? This would seem to cause a lot more headaches in
the
> rest of Chrome (i.e., not just in this code), so I don't think this is the
> concern, right?

It is a requirement in Chrome code that Profile objects are only accessed (reads
or writes) from the UI thread.  There is a ridiculous amount of code that
touches profiles and it is all thread-unsafe, so all code which uses Profiles
must touch them on the UI thread.  If necessary, classes are split into multiple
pieces which run on different threads and pass data to each other.

> If you are saying that this absolutely should not take a Profile at all, then
> could you propose an alternative of how to achieve my goal (of passing the
> necessary Profile information to the Instant field trial code)?

I suggest having the substitution code take a flag.  In the search and keyword
autocomplete providers, the flag value provided can be calculated by reading the
profile.  In other places, callers can just pass false.

I haven't read your patch, so I don't know if this is overly simplistic.

[sreeram, 2011-08-09 20:16:43]

On Tue, Aug 9, 2011 at 12:43,  <pkasting@chromium.org> wrote:
> It is a requirement in Chrome code that Profile objects are only accessed
> (reads
> or writes) from the UI thread.  There is a ridiculous amount of code that
> touches profiles and it is all thread-unsafe, so all code which uses
> Profiles
> must touch them on the UI thread.  If necessary, classes are split into
> multiple
> pieces which run on different threads and pass data to each other.

Thanks for the explanation. It seems then that I could add the Profile
information to SearchTermsData, something like:
    class SearchTermsData {
        virtual Profile* GetProfile() = 0;
    };
    class UIThreadSearchTermsdata {
        virtual Profile* GetProfile();
    }

So, I won't need to pass any additional parameters, and the Profile
object will only be visible on the UI thread. If this works, then I
have one further question: How do I retrieve the current profile (in
the implementation of UIThreadSearchTermsData::GetProfile()),
presumably using g_browser_process? Nothing in ProfileManager
(returned by g_browser_process->GetProfileManager()) seemed
appropriate, but perhaps I didn't look close enough.

[Peter Kasting, 2011-08-09 20:25:30]

On Tue, Aug 9, 2011 at 1:15 PM, Sreeram Ramachandran
<sreeram@chromium.org>wrote:

> It seems then that I could add the Profile
> information to SearchTermsData, something like:
>    class SearchTermsData {
>        virtual Profile* GetProfile() = 0;
>    };
>    class UIThreadSearchTermsdata {
>        virtual Profile* GetProfile();
>    }
>

I don't know; I've never heard of the SearchTermsData class before.  Seems
at a glance like instead of making the base class implementation pure
virtual you'd want to give it a default implementation that returns NULL,
though.

How do I retrieve the current profile (in
> the implementation of UIThreadSearchTermsData::GetProfile()),
> presumably using g_browser_process? Nothing in ProfileManager
> (returned by g_browser_process->GetProfileManager()) seemed
> appropriate, but perhaps I didn't look close enough.
>

You can't get "the current profile" off global state because there is no
global current profile.  Profiles are per-window (per-Browser-object) so
there may be multiple profiles active at once (in different windows) or no
windows at all (in which case the notion of a "current profile" is
ill-defined).

You need a Browser object to get a profile from.  It sounded from my reading
of the discussion thread on this like Scott was suggesting that whoever is
creating the data set to pass in should provide the appropriate Profile in
which case it's up to the caller to do this?

PK

[sreeram, 2011-08-10 18:20:38]

Please take another look. Here's what I am doing now:

+ The *UsingTermsData methods (GenerateSearchURLUsingTermsData and
ReplaceSearchTermsUsingTermsData) now don't deal with Profile. Instead, they ask
for the Instant field trial group param, which defaults to being a blank string.
I now understand that this is what @sky was asking for, since the
*UsingTermsData methods could be called from any thread (and not just the UI
thread).

+ The corresponding UI-thread-kosher methods (GenerateSearchURL and
ReplaceSearchTerms) take a profile and stick it in the UIThreadSearchTermsData
struct, which then returns a proper param when asked.

+ I've tried to make all callers pass a real Profile, instead of NULL. I was
mostly successful, except for the callers in template_url.cc. To make them pass
a real Profile would cause way too many unrelated methods in TemplateURLRef to
need a Profile, so I've left them passing NULL.

+ The other thing @sky wanted was to change the many params for
ReplaceSearchTerms into a struct. I don't think this will make it any easier for
the callers. They'll just go from looking like this:
    template_url->ReplaceSearchTerms(profile, host, terms, suggestion, query);
to:
    template_url->ReplaceSearchTerms(NewStruct(profile, host, terms, suggestion,
query));
So, I'm not doing that refactoring here, in this CL. We can always pursue it
separately if desired.

[Peter Kasting, 2011-08-10 20:54:06]

The summary of my comments below is that by allowing the field trial to handle a
NULL profile, you can eliminate almost all the rest of the changes in this
patch, leaving only a few places that actually need to care about the profile.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_field_trial.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/instant/insta...
chrome/browser/instant/instant_field_trial.cc:41: if (profile->IsOffTheRecord())
Modify this to be:

  if (!profile || profile->IsOffTheRecord)

This way, you can eliminate the DCHECK on the caller side that requires a
Profile.  The advantage of this is that it gives callers the ability to pass in
NULL as a way of saying "don't care".  This makes many of the other changes I
recommend possible.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/instant/insta...
chrome/browser/instant/instant_field_trial.cc:88: case INACTIVE:    return "";
Nit: Use std::string() instead of "" (2 places)

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
File chrome/browser/search_engines/search_host_to_urls_map_unittest.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/search_host_to_urls_map_unittest.cc:33:
TestingProfile profile_;
You don't need this.  None of the SearchHostToURLsMap functions actually care
about the field trial (at most they read the host off the generated URL) and so
you can supply NULL whenever constructing a UIThreadSearchTermsData.

(The calls directly to Remove(), Update(), etc. won't take a Profile either once
you remove it from TemplateURLService::GenerateSearchURL() and then propagate
that change backwards; see comments there.)

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
File chrome/browser/search_engines/search_provider_install_data.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/search_provider_install_data.cc:145:
UIThreadSearchTermsData(profile_).GoogleBaseURLValue()));
Neither caller in this file is asking for the instant trial string, so you can
pass NULL in both these functions and not add Profile*s to all the declarations.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
File chrome/browser/search_engines/search_terms_data.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/search_terms_data.cc:60: return "";
Nit: Use std::string() instead of ""

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/search_terms_data.cc:109: DCHECK(profile_);
Remove this (in conjunction with the changes to InstantFieldTrial) and document
on the constructor declaration in the header what the Profile is for and that it
may be NULL.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3540:
UIThreadSearchTermsData search_terms_data(profile);
You don't care about anything but the origin, so pass NULL here and remove the
Profiles from the function declarations.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
(right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:139:
UIThreadSearchTermsData search_terms_data(&profile);
You can pass NULL here and below.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:193: Profile* profile) {
You don't actually need this, because none of the callers cares about anything
beyond the hostname.

The UIThreadSearchTermsData constructed here can be passed NULL, as can the
instances everywhere else.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service_unittest.cc:896:
EXPECT_EQ("http://foo.com/?q=x", t_url->url()->ReplaceSearchTerms(profile(),
This call and the one below can pass NULL since you don't care about the field
trial in tests.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_unittest.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_unittest.cc:95: GURL result =
GURL(ref.ReplaceSearchTerms(&profile_, t_url, value.terms,
Again, these can all pass NULL as you don't care about the field trial in tests.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/ui/search_eng...
File chrome/browser/ui/search_engines/edit_search_engine_controller.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/ui/search_eng...
chrome/browser/ui/search_engines/edit_search_engine_controller.cc:56: return
GURL(template_ref.ReplaceSearchTerms(profile_,
You can pass NULL here, it won't matter to the validity check.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/ui/search_eng...
chrome/browser/ui/search_engines/edit_search_engine_controller.cc:141:
t_url.url()->ReplaceSearchTerms(profile_, t_url, ASCIIToUTF16("x"), 0,
You can pass NULL here as you don't read the query off this URL.

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/ui/views/defa...
File chrome/browser/ui/views/default_search_view.cc (right):

http://codereview.chromium.org/7558014/diff/7001/chrome/browser/ui/views/defa...
chrome/browser/ui/views/default_search_view.cc:33: void
GetShortNameAndLogoId(Profile* profile,
All the changes in this file can end up being reverted.

[sreeram, 2011-08-10 23:25:49]

On 2011/08/10 20:54:06, Peter Kasting wrote:
> The summary of my comments below is that by allowing the field trial to handle
a
> NULL profile, you can eliminate almost all the rest of the changes in this
> patch, leaving only a few places that actually need to care about the profile.

Done (all the changes you pointed out).

[Peter Kasting, 2011-08-11 00:35:16]

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:204: &profile_,
*default_t_url_, text, 0, string16())), wyt_match));
Seems like this file should generally pass NULL as well (unless there's a test
that's specifically testing for the instant query param, which I don't see)

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/importer/prof...
File chrome/browser/importer/profile_writer.cc (right):

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/importer/prof...
chrome/browser/importer/profile_writer.cc:254: profile, *t_url,
ASCIIToUTF16("random string"),
HostPathKeyForURL() strips the query params so this spot can pass NULL as well.

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/instant/insta...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:342:
EXPECT_EQ(default_turl->url()->ReplaceSearchTerms(browser()->profile(),
I tried to trace through the call chain (for GetCurrentURL()) here but failed. 
Is passing the profile here the right thing to do?

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/search_engine...
File chrome/browser/search_engines/search_terms_data.h (right):

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/search_engine...
chrome/browser/search_engines/search_terms_data.h:52: explicit
UIThreadSearchTermsData(Profile* profile);
I wonder if it would be even simpler to leave this as a no-arg constructor and
instead have a set_profile() method.  Then only the (one?) spot that cares about
providing a profile to this struct would have to be touched.

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/7558014/diff/9004/chrome/browser/search_engine...
chrome/browser/search_engines/template_url.h:75: Profile* profile,
Nit: You should comment about what we use this arg for (and in particular that
it can be NULL).

[sreeram, 2011-08-11 17:52:11]

Okay, I get your drift. I've now introduced a new method (why didn't I think of
this before?!), so that only those callers that care are changed. What do you
think of the new patch?

[Peter Kasting, 2011-08-11 18:04:33]

LGTM

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
File chrome/browser/search_engines/search_terms_data.h (right):

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/search_terms_data.h:39: // if the field trial is
not active.
Nit: Might want to add "Checking this requires accessing the Profile, which
means this can only ever be non-empty for UIThreadSearchTermsData."

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/search_terms_data.h:57: virtual std::string
InstantFieldTrialUrlParam() const;
Nit: Add comment: "// This returns the empty string unless set_profile() has
been called."

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/search_terms_data.h:63: // Optionally, set the
profile to be used to look up params for the Instant
Nit: I'd move this below the constructor and make it:

  // Callers who need an accurate answer from InstantFieldTrialUrlParam() must
  // set the profile here before calling that.
  void set_profile(Profile* profile) { profile_ = profile; }

Note that because this doesn't actually access the profile, we can get away with
not checking that we're on the UI thread -- we'll do that check when we actually
try to read it.

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/search_terms_data.h:69: 
Nit: Unnecessary newline

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.cc:288: UIThreadSearchTermsData
search_terms_data;
Nit: Just implement this function by calling ReplaceSearchTermsUsingProfile()
with NULL.

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.h:82: // params, and so should use
ReplaceSearchTerms instead.
Nit: should -> can

[sreeram, 2011-08-11 18:22:19]

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
File chrome/browser/search_engines/search_terms_data.h (right):

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/search_terms_data.h:39: // if the field trial is
not active.
On 2011/08/11 18:04:34, Peter Kasting wrote:
> Nit: Might want to add "Checking this requires accessing the Profile, which
> means this can only ever be non-empty for UIThreadSearchTermsData."

Done.

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/search_terms_data.h:57: virtual std::string
InstantFieldTrialUrlParam() const;
On 2011/08/11 18:04:34, Peter Kasting wrote:
> Nit: Add comment: "// This returns the empty string unless set_profile() has
> been called."

Done.

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/search_terms_data.h:69: 
On 2011/08/11 18:04:34, Peter Kasting wrote:
> Nit: Unnecessary newline

Done.

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.cc:288: UIThreadSearchTermsData
search_terms_data;
On 2011/08/11 18:04:34, Peter Kasting wrote:
> Nit: Just implement this function by calling ReplaceSearchTermsUsingProfile()
> with NULL.

Done.

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/7558014/diff/16001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.h:82: // params, and so should use
ReplaceSearchTerms instead.
On 2011/08/11 18:04:34, Peter Kasting wrote:
> Nit: should -> can

Done.

[commit-bot: I haz the power, 2011-08-16 16:16:18]

Change committed as 96954