Add Android-specific parameters to search_engines/

Add Android-specific parameters to search_engines/

Previous attempt; expanded since then:
https://chromiumcodereview.appspot.com/10332156/

* The search string has "sourceid=chrome-mobile" on Android devices.

* SearchTermsDataAndroid added to handle the data values that Android
  needs to store for RLZ tracking.

* UiThreadSearchTermsData calls Android specific functions when being
  asked for rlz= or client= strings.

* ENABLE_RLZ is moved around (or removed) in some cases to allow use of
  the fields even when the RLZ library is not in use.

* The RLZ unit test is changed to account for the way Android sets the 
  RLZ string.

* A new SearchClient unit test is added to check that setting client
  strings works.

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

[gone, 2012-12-05 02:22:44]

Hey Peter,
This CL is a continuation of https://chromiumcodereview.appspot.com/10332156/
and adds in another change that had to be upstreamed for the query URLs.  It's
not quite ready for a thorough review yet, but are the changes in the CL along
the lines of what you were looking for?

Thanks -
Dan

[Peter Kasting, 2012-12-05 21:28:47]

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/prepopulated_engines.json:850: "search_url":
"{google:baseURL}search?q={searchTerms}&{google:RLZ}{google:acceptedSuggestion}{google:originalQueryForSuggestion}{google:assistedQueryStats}{google:searchFieldtrialParameter}{google:searchClient}sourceid=chrome&ie={inputEncoding}",
You need to bump the version number if you change this.

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/search_terms_data.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/search_terms_data.cc:112: #elif
defined(OS_ANDROID)
Another way to do this: put these two entire functions inside one large "#if
!defined(OS_ANDROID)", then in your .cc file, define these functions directly in
terms of whatever you want.

This involves fewer #ifs, including eliminating the #include atop this file, and
probably results in slightly fewer symbols overall.  But it's not a huge
difference.

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/search_terms_data_android.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/search_terms_data_android.cc:9:
base::LazyInstance<std::string>::Leaky SearchTermsDataAndroid::search_client_ =
Tiny nit: Just for consistency, might want to instead break after the type

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/template_url.cc:323: if (!client.empty()) {
Nit: No {}

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/template_url.cc:324: url.insert(i->index,
"client=" + client + "&");
Make sure the search folks like the "client=<whatever>&sourceid=chrome"
combination -- I say this because we (Chrome all platforms) used to use
"client=chrome" and changed away from that, so I just want to make sure the
current Android impl isn't using some obsolete set of params.

[gone, 2012-12-06 01:43:57]

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/prepopulated_engines.json:850: "search_url":
"{google:baseURL}search?q={searchTerms}&{google:RLZ}{google:acceptedSuggestion}{google:originalQueryForSuggestion}{google:assistedQueryStats}{google:searchFieldtrialParameter}{google:searchClient}sourceid=chrome&ie={inputEncoding}",
On 2012/12/05 21:28:47, Peter Kasting wrote:
> You need to bump the version number if you change this.

Done.

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/search_terms_data.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/search_terms_data.cc:112: #elif
defined(OS_ANDROID)
On 2012/12/05 21:28:47, Peter Kasting wrote:
> Another way to do this: put these two entire functions inside one large "#if
> !defined(OS_ANDROID)", then in your .cc file, define these functions directly
in
> terms of whatever you want.
> 
> This involves fewer #ifs, including eliminating the #include atop this file,
and
> probably results in slightly fewer symbols overall.  But it's not a huge
> difference.

Done.

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/search_terms_data_android.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/search_terms_data_android.cc:9:
base::LazyInstance<std::string>::Leaky SearchTermsDataAndroid::search_client_ =
On 2012/12/05 21:28:47, Peter Kasting wrote:
> Tiny nit: Just for consistency, might want to instead break after the type

Done.

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/template_url.cc:323: if (!client.empty()) {
On 2012/12/05 21:28:47, Peter Kasting wrote:
> Nit: No {}

Done.

https://chromiumcodereview.appspot.com/11448005/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/template_url.cc:324: url.insert(i->index,
"client=" + client + "&");
On 2012/12/05 21:28:47, Peter Kasting wrote:
> Make sure the search folks like the "client=<whatever>&sourceid=chrome"
> combination -- I say this because we (Chrome all platforms) used to use
> "client=chrome" and changed away from that, so I just want to make sure the
> current Android impl isn't using some obsolete set of params.

We've apparently been using this combination on our m18 builds; one of our PMs
says that the data has looked at since we first introduced it.

One thing he did mention, however, is that the sourceid is hard-coded to
"chrome-mobile" on mobile devices.  I've had to introduce a change to swap that
out for mobile devices.  Chrome for iOS apparently also uses this sourceid, but
I haven't been able to pinpoint where it occurs.

[Peter Kasting, 2012-12-06 07:02:37]

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 47
Note that there are also a couple of other outstanding CLs that bump this
number.  If you land after them, make sure you're still bumping the number after
merge resolution (because I suspect the merge would happily get resolved as
"leave this value at 47").

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/search_terms_data.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data.cc:93: #if !defined(OS_ANDROID)
Tiny nit: You might want an "Android implementations of these functions are in
search_terms_data_android.cc" type comment.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/search_terms_data_android.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data_android.cc:10: using
content::BrowserThread;
Nit: Don't use using directives unless they save a significant number of lines.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data_android.cc:35:
BrowserThread::CurrentlyOn(BrowserThread::UI));
Nit: Indent 4, not even (2 places)

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data_android.cc:38: return
SearchTermsDataAndroid::GetRlzParameterValue();
I suggest returning the value directly here and below.

You could do this by making the strings globals, so anyone could just set them
directly, and you can read them directly here.  I'd probably do this.  It's
extremely simple.

If you like, you can stick them in a namespace or (otherwise empty) struct or
something, for name scoping.  But I don't think the getter and setter buy
anything but one more indirection.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/template_url.cc:339: url.insert(i->index,
"sourceid=chrome-mobile&");
Because this value is fixed, you can insert it directly in ParseParameter(). 
You don't need to define GOOGLE_SOURCE_ID or add code here at all.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/template_url_unittest.cc:664: // Check that the
URL is correct when a client is not present.
This test relies on non-Android having no client.  I'd rather not hardcode that
kind of assumption.

Unfortunately, I can't think of a good way to write the test so it's still
meaningful in that case.  Checking if SearchTermsData returns an empty client
string and then checking that calling ReplaceSearchTerms() substitutes the empty
string makes the test somewhat vacuous.

Perhaps the test should just be entirely "#if defined(OS_ANDROID)" with a
comment that for now only Android uses the search client?  I dunno.

[gone, 2012-12-06 20:59:48]

I think these changes cover the latest round.  PTAL when you've got a chance. 
Thanks!

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 47
On 2012/12/06 07:02:37, Peter Kasting wrote:
> Note that there are also a couple of other outstanding CLs that bump this
> number.  If you land after them, make sure you're still bumping the number
after
> merge resolution (because I suspect the merge would happily get resolved as
> "leave this value at 47").

Will keep an eye on it... If another CL bumps it I'll re-bump in a follow-up CL.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/search_terms_data.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data.cc:93: #if !defined(OS_ANDROID)
On 2012/12/06 07:02:37, Peter Kasting wrote:
> Tiny nit: You might want an "Android implementations of these functions are in
> search_terms_data_android.cc" type comment.

Done.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/search_terms_data_android.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data_android.cc:10: using
content::BrowserThread;
On 2012/12/06 07:02:37, Peter Kasting wrote:
> Nit: Don't use using directives unless they save a significant number of
lines.

Done.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data_android.cc:35:
BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/12/06 07:02:37, Peter Kasting wrote:
> Nit: Indent 4, not even (2 places)

Funky.  I copied these directly from search_terms_data.cc.  I'll fix those, too.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data_android.cc:38: return
SearchTermsDataAndroid::GetRlzParameterValue();
On 2012/12/06 07:02:37, Peter Kasting wrote:
> I suggest returning the value directly here and below.
> 
> You could do this by making the strings globals, so anyone could just set them
> directly, and you can read them directly here.  I'd probably do this.  It's
> extremely simple.
> 
> If you like, you can stick them in a namespace or (otherwise empty) struct or
> something, for name scoping.  But I don't think the getter and setter buy
> anything but one more indirection.

Kept them inside an otherwise empty struct to keep it tidier, but removed the
setters and getters as requested.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/template_url.cc:339: url.insert(i->index,
"sourceid=chrome-mobile&");
On 2012/12/06 07:02:37, Peter Kasting wrote:
> Because this value is fixed, you can insert it directly in ParseParameter(). 
> You don't need to define GOOGLE_SOURCE_ID or add code here at all.

Done.

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/10015/chrome/browser/sea...
chrome/browser/search_engines/template_url_unittest.cc:664: // Check that the
URL is correct when a client is not present.
On 2012/12/06 07:02:37, Peter Kasting wrote:
> This test relies on non-Android having no client.  I'd rather not hardcode
that
> kind of assumption.
> 
> Unfortunately, I can't think of a good way to write the test so it's still
> meaningful in that case.  Checking if SearchTermsData returns an empty client
> string and then checking that calling ReplaceSearchTerms() substitutes the
empty
> string makes the test somewhat vacuous.
> 
> Perhaps the test should just be entirely "#if defined(OS_ANDROID)" with a
> comment that for now only Android uses the search client?  I dunno.

I've moved the OS_ANDROID flag to encapsulate the whole test.  If desktop Chrome
ever starts adding in this flag, we can revisit where the guards can go...

[Peter Kasting, 2012-12-07 23:32:57]

LGTM

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
File chrome/browser/search_engines/search_terms_data.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data.cc:94: // Android
implementations are located in search_terms_data_android.cc.
Tiny nit: I'd put this comment above the #if

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data.cc:116:
BrowserThread::CurrentlyOn(BrowserThread::UI));
Nit: Since you changed it everywhere else, indent 4 here

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
chrome/browser/search_engines/template_url_unittest.cc:651: #if
defined(OS_ANDROID)
Nit: May want a comment like "We can enable this on non-Android if other
platforms ever want a non-empty search client string."

You might even want a comment in/above the non-Android search client getter
function noting something to that effect and referring to this test (instead of
a comment here, perhaps).

[gone, 2012-12-08 00:32:51]

Thanks!  Adding OWNERS for chrome/

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
File chrome/browser/search_engines/search_terms_data.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data.cc:94: // Android
implementations are located in search_terms_data_android.cc.
On 2012/12/07 23:32:57, Peter Kasting wrote:
> Tiny nit: I'd put this comment above the #if

Done.

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
chrome/browser/search_engines/search_terms_data.cc:116:
BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/12/07 23:32:57, Peter Kasting wrote:
> Nit: Since you changed it everywhere else, indent 4 here

Ah, missed this.

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://chromiumcodereview.appspot.com/11448005/diff/23002/chrome/browser/sea...
chrome/browser/search_engines/template_url_unittest.cc:651: #if
defined(OS_ANDROID)
On 2012/12/07 23:32:57, Peter Kasting wrote:
> Nit: May want a comment like "We can enable this on non-Android if other
> platforms ever want a non-empty search client string."
> 
> You might even want a comment in/above the non-Android search client getter
> function noting something to that effect and referring to this test (instead
of
> a comment here, perhaps).

Done.

[gone, 2012-12-08 00:33:55]

Er, properly adding OWNERS this time.

[Nico, 2012-12-08 00:39:00]

lgtm

[commit-bot: I haz the power, 2012-12-08 00:46:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/11448005/18016

[commit-bot: I haz the power, 2012-12-08 06:56:12]

Change committed as 171954