Replace --google-base-suggest-url and --instant-url with --google-base-url.

Replace --google-base-suggest-url and --instant-url with --google-base-url.  Add
a new --extra-search-query-params flag for inserting query params into the
search and instant URLs.

This also cleans up various google_util functions to take a GURL instead of a
string.

BUG=249197
TEST=Run Chrome with --google-base-url="<url>" and --extra-search-query-params="<params>" and you should see those reflected in the default search URL.

[Peter Kasting, 2013-06-14 23:43:50]

hfung: search_terms_data.cc.  You'll note that I moved the code from
SearchTermsData to UIThreadSearchTermsData, because command_line.h says it is
not a threadsafe object.
samarth: rest

[H Fung, 2013-06-15 00:42:27]

lgtm

[samarth, 2013-06-17 20:39:37]

Thanks,
Samarth

https://codereview.chromium.org/17022004/diff/1/chrome/browser/search/search.cc
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/17022004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:201: if (extended_api_enabled &&
!url.SchemeIsSecure())
I ran into two issues while trying out your patch:

(1) We should bypass the https check here if we're using the Google engine
--google-base-url is set. That was one of the uses of --instant-url and always
requiring https would make development much more onerous.

(2) While testing changes with local servers, we often need to add some extra
debug query parameters to the instant/search URLs. Based on our discussion last
week, I thought we'd be able to stick them at the beginning of the
{google:baseURL} but that doesn't include the request path, so that won't work.
You end up with something like:
mytestserver.google.com/webhp?debugparam=1&webhp?....
I see a couple of options. We could:
a. Do nothing. Always require people to edit template url by hand.
b. Add a new {extraParams} field in the template url for Google that can be set
via a flag.
c. Add a more generic --search-extra-params flag that will append debug params
to whatever URL you happen to use .
Thoughts? I really think option (a) would represent a big loss in productivity.
I'm especially worried about cases where a search server release is breaking
something and someone needs to debug quickly. Having to rebuild Chrome or have
to muck around with the profile in sqlite would be a big hindrance.

[Peter Kasting, 2013-06-21 01:01:21]

Samarth, take another look.

I've tried to address both complaints about search.cc.  We now have a new flag
for inserting query params, and we should do a better job of allowing
--google-base-url to override the "has to be HTTPS" code in various places.

This CL doesn't yet include the change to support the
--extra-search-query-params flag in the search URL, solely because I started
this change in the same checkout as another one that's already modifying
search_provider.cc.  When I have that piece ready, I can get it reviewed alone,
either on this CL or as a followon CL.

[samarth, 2013-06-21 23:57:24]

[+Jered to take over the review since I'm about to head out on vacation]

I just tried this with a local demo, and it mostly works but it's still not
doing search term replacement. I checked and the search page does land in the
Instant process. So TemplateURLRef::ExtractSearchTermsFromURL is probably not
happy extracting URLs for some reason.

Re the extra args, it'd be nice to get that in along with this, but just being
able to use arbitary search_urls for testing is much more important. So I'd be
OK if you do a follow-up CL.

Thanks,
Samarth

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:513:
(TemplateURLPrepopulateData::GetEngineType(default_provider->url()) ==
It's not terribly important, but this check is failing with a custom
--google-base-url, meaning you get the non-google version of the local NTP.

Perhaps this is not the correct check to be doing in the first place.

[Jered, 2013-06-25 16:30:04]

On 2013/06/21 23:57:24, samarth wrote:
> [+Jered to take over the review since I'm about to head out on vacation]
> 
> I just tried this with a local demo, and it mostly works but it's still not
> doing search term replacement. I checked and the search page does land in the
> Instant process. So TemplateURLRef::ExtractSearchTermsFromURL is probably not
> happy extracting URLs for some reason.
> 
> Re the extra args, it'd be nice to get that in along with this, but just being
> able to use arbitary search_urls for testing is much more important. So I'd be
> OK if you do a follow-up CL.
> 
> Thanks,
> Samarth
> 
>
https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
> File chrome/browser/search/search.cc (right):
> 
>
https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
> chrome/browser/search/search.cc:513:
> (TemplateURLPrepopulateData::GetEngineType(default_provider->url()) ==
> It's not terribly important, but this check is failing with a custom
> --google-base-url, meaning you get the non-google version of the local NTP.
> 
> Perhaps this is not the correct check to be doing in the first place.

This mostly looks good, I really only have nits. The logic is an
improvement. Would be good to sort out why search terms replacement
is not happening.

[Jered, 2013-06-25 16:30:39]

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/google/goo...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/google/goo...
chrome/browser/google/google_util.cc:155: bool IsGoogleDomainUrl(const
std::string& url,
All the callers turn a GURL into a string, then we make a new GURL here. How
about having this take a const GURL& instead?

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:208: bool IsSuitableURLForInstant(const GURL&
url, const TemplateURL* default_turl) {
default_turl -> search_template or template_url ("turl" sounds weird to me and I
wasn't immediately sure what it was).

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:475: // In non-extended mode, the checkbox must
be checked.
There is no checkbox. Can you update this comment and the accompanying logic to
match reality?

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:497:
GetSwitchValueASCII(switches::kExtraSearchQueryParams));
Indent two more spaces.

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:502: replacements.SetQueryStr(query_params);
Developers might find it confusing that --extra-search-query-params applies only
to Instant, and not to anything else we might do with a search template URL.

[Peter Kasting, 2013-06-25 23:06:36]

Significantly modified.  This addresses the problems Samarth was seeing as well
as the issues below, except that --extra-search-query-params doesn't yet affect
anything other than the instant URL.  Affecting the default search URL looks
trickier than I thought; I'm currently pondering the right way to do it.

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/google/goo...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/google/goo...
chrome/browser/google/google_util.cc:155: bool IsGoogleDomainUrl(const
std::string& url,
On 2013/06/25 16:30:39, Jered wrote:
> All the callers turn a GURL into a string, then we make a new GURL here. How
> about having this take a const GURL& instead?

Done.  For consistency I also switched IsGoogleHomePageUrl() and
IsGoogleSearchUrl().

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:208: bool IsSuitableURLForInstant(const GURL&
url, const TemplateURL* default_turl) {
On 2013/06/25 16:30:39, Jered wrote:
> default_turl -> search_template or template_url ("turl" sounds weird to me and
I
> wasn't immediately sure what it was).

Changed, though I actually preferred the old terminology (and would like to
switch the rest of the file to it).  The TemplateURL, Sync, Autocomplete, etc.
subsystems commonly refer to TemplateURLs as "turls" and "default_turl" is the
common way of indicating "this is the DSP's TemplateURL".

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:475: // In non-extended mode, the checkbox must
be checked.
On 2013/06/25 16:30:39, Jered wrote:
> There is no checkbox. Can you update this comment and the accompanying logic
to
> match reality?

No.  There are a variety of functions throughout this file that refer to the
checkbox, and I am not knowledgeable enough in this area to sort all of them
out.  This should be something you guys clean up.

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:497:
GetSwitchValueASCII(switches::kExtraSearchQueryParams));
On 2013/06/25 16:30:39, Jered wrote:
> Indent two more spaces.

Done.

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:502: replacements.SetQueryStr(query_params);
On 2013/06/25 16:30:39, Jered wrote:
> Developers might find it confusing that --extra-search-query-params applies
only
> to Instant, and not to anything else we might do with a search template URL.

I haven't yet made it apply to the search URL, but that's coming before I check
in.

https://codereview.chromium.org/17022004/diff/26001/chrome/browser/search/sea...
chrome/browser/search/search.cc:513:
(TemplateURLPrepopulateData::GetEngineType(default_provider->url()) ==
On 2013/06/21 23:57:25, samarth wrote:
> It's not terribly important, but this check is failing with a custom
> --google-base-url, meaning you get the non-google version of the local NTP.

Fixed, by fixing some of the lower-level functions in google_util to respect a
command-line Google base URL.

[Jered, 2013-06-26 00:32:16]

lgtm with some minor nits.

https://codereview.chromium.org/17022004/diff/53001/chrome/browser/google/goo...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/17022004/diff/53001/chrome/browser/google/goo...
chrome/browser/google/google_util.cc:212: bool is_home_page_base = false;
bool is_home_page_base = path == "/webhp" || path == "/";
if (!is_home_page_base && path != "/search")
  return false;

https://codereview.chromium.org/17022004/diff/53001/chrome/browser/google/goo...
File chrome/browser/google/google_util_unittest.cc (right):

https://codereview.chromium.org/17022004/diff/53001/chrome/browser/google/goo...
chrome/browser/google/google_util_unittest.cc:14: // Helpers
--------------------------------------------------------------------
Omit the ------- stuff here and below.

[Peter Kasting, 2013-06-26 00:40:27]

https://codereview.chromium.org/17022004/diff/53001/chrome/browser/google/goo...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/17022004/diff/53001/chrome/browser/google/goo...
chrome/browser/google/google_util.cc:212: bool is_home_page_base = false;
On 2013/06/26 00:32:16, Jered wrote:
> bool is_home_page_base = path == "/webhp" || path == "/";
> if (!is_home_page_base && path != "/search")
>   return false;

Nice :)

https://codereview.chromium.org/17022004/diff/53001/chrome/browser/google/goo...
File chrome/browser/google/google_util_unittest.cc (right):

https://codereview.chromium.org/17022004/diff/53001/chrome/browser/google/goo...
chrome/browser/google/google_util_unittest.cc:14: // Helpers
--------------------------------------------------------------------
On 2013/06/26 00:32:16, Jered wrote:
> Omit the ------- stuff here and below.

This is how we separate sections of files in a large quantity of frontend code.

[Peter Kasting, 2013-06-27 00:27:41]

Ugh.  I think this is basically done, but the final patch is enormous and has
way too many disparate pieces.  (Uploaded in patch set 5.)

I'm going to close this CL and do this change as a series of smaller commits.