Remove call to IsInstantExtendedAPIGoogleSearchUrl from ToolbarModel.

Remove call to IsInstantExtendedAPIGoogleSearchUrl from ToolbarModel.

Adds a mechanism to TemplateURL to optionally allow search term extraction only when a given key is present (for Google, this is "espv"). Uses this to ensure ToolbarModel no longer relies on the hardcoded "espv" parameter.

Depends on:
  https://codereview.chromium.org/11552020/

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

[beaudoin, 2012-12-15 00:18:44]

Hi Peter! This is the first follow-up patch to
https://codereview.chromium.org/11552020/ 
It optionally looks for the new search_terms_replacement_key when performing
search term extraction.

I've preferred this approach to a separate
IsSearchTermsReplacementKeyPresent(url) method because:
1) In most use case, the check would be followed by a search term extraction ;
2) Separating them would force us to parse the URL twice ;
3) The behavior on google.com is that "espv" is only taken into account if it's
in the same region (query/ref) as the search term, so we would have to locate
the search term anyway.

If, given that, you'd still prefer a separate method, let me know and I'll
refactor.

Cheers,

   Philippe

[Peter Kasting, 2012-12-18 02:09:22]

Basically, the questions I have about this are the ones I stated a little
earlier tonight on your other code review, in particular that I'm uneasy about
the whole "force the param to occur in the same part of the URL the search terms
appear in".  The logical behavior is to say instant extended is on if this param
appears anywhere in the query, or perhaps "anywhere in the query or ref" if the
ref is supported by one of the alternate_urls.  Do we lose much by doing this? 
Is Google really going to ship us a URL with espv in the query, the relevant
search term in the ref, and expect instant extended to be off?  (Can we fix the
server side to not do this if so?)

LGTM otherwise; I'm OK with the ugly-but-expedient signature change to the API
here.

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.cc:510: base::StringPiece
key_string(params.begin() + key.begin,
Does using a StringPiece here actually buy much?  Normally I'd just use a
string.

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.cc:847: const GURL& url, string16*
search_terms, bool needs_instant_extended) {
Nit: One arg per line (and 1st arg can move up a line)

[beaudoin, 2012-12-20 04:40:56]

Your concerns on the ref/query quirks for "espv" are legitimate. As I said on
the other thread, I went with the current GWS behavior. It may be possible to
change GWS or we may decide to not worry about cases that don't really show up
in practice. (I did not think about the implication of this latter stance too
much.)

Let's see what dcblack says on the other thread and we can go from there.

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.cc:510: base::StringPiece
key_string(params.begin() + key.begin,
On 2012/12/18 02:09:22, Peter Kasting wrote:
> Does using a StringPiece here actually buy much?  Normally I'd just use a
> string.

Thought it may have been worth not copying characters around the heap inside a
loop, especially since the API is so similar to a string. If you feel strongly I
can switch back to a string.

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.cc:847: const GURL& url, string16*
search_terms, bool needs_instant_extended) {
On 2012/12/18 02:09:22, Peter Kasting wrote:
> Nit: One arg per line (and 1st arg can move up a line)

Done.

[Peter Kasting, 2012-12-20 19:40:16]

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.cc:510: base::StringPiece
key_string(params.begin() + key.begin,
On 2012/12/20 04:40:56, beaudoin wrote:
> On 2012/12/18 02:09:22, Peter Kasting wrote:
> > Does using a StringPiece here actually buy much?  Normally I'd just use a
> > string.
> 
> Thought it may have been worth not copying characters around the heap inside a
> loop, especially since the API is so similar to a string. If you feel strongly
I
> can switch back to a string.

I don't really care too much, the string version just seemed a little less
verbose.  Note that we already use substr() (and thus generate a string) below
in the StringToInt() call, so just doing that here too would be consistent.

[beaudoin, 2013-01-08 16:55:38]

Simplified the behavior when checking for search_terms_replacement_key according
to discussions with pkasting and dcblack. Consequently, I added the
HasSearchTermsReplacementKey method which allowed me to revert to the previous
API for ExtractSearchTermsFromURL.

There was a also a bug where
  "search_terms_replacement_key": "{google:instantExtendedEnabledKey}",
would not cause the variable to be expanded. I fixed this.

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/11576044/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.cc:510: base::StringPiece
key_string(params.begin() + key.begin,
On 2012/12/20 19:40:16, Peter Kasting wrote:
> On 2012/12/20 04:40:56, beaudoin wrote:
> > On 2012/12/18 02:09:22, Peter Kasting wrote:
> > > Does using a StringPiece here actually buy much?  Normally I'd just use a
> > > string.
> > 
> > Thought it may have been worth not copying characters around the heap inside
a
> > loop, especially since the API is so similar to a string. If you feel
strongly
> I
> > can switch back to a string.
> 
> I don't really care too much, the string version just seemed a little less
> verbose.  Note that we already use substr() (and thus generate a string) below
> in the StringToInt() call, so just doing that here too would be consistent.

Done.

[commit-bot: I haz the power, 2013-01-08 19:35:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11576044/18001

[commit-bot: I haz the power, 2013-01-08 19:39:06]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-08 20:15:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11576044/18004

[commit-bot: I haz the power, 2013-01-08 21:20:58]

Failed to trigger a try job on linux_chromeos
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-01-08 21:21:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11576044/28005

[commit-bot: I haz the power, 2013-01-08 21:21:28]

Presubmit check for 11576044-28005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/policy

Presubmit checks took 1.1s to calculate.

[beaudoin, 2013-01-08 21:26:47]

+joaodasilva for c/b/policy

[beaudoin, 2013-01-08 21:27:28]

Hi joao! Could you please take a quick look at that CL, I was getting ready to
submit but had to fix a policy browser test. Thanks!

[Joao da Silva, 2013-01-08 21:38:20]

policy/ lgtm

https://codereview.chromium.org/11576044/diff/28005/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/11576044/diff/28005/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:846: 
nit: single newline?

[beaudoin, 2013-01-08 21:46:50]

https://codereview.chromium.org/11576044/diff/28005/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/11576044/diff/28005/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:846: 
On 2013/01/08 21:38:20, Joao da Silva wrote:
> nit: single newline?

Done.

[commit-bot: I haz the power, 2013-01-08 21:47:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11576044/15004

[commit-bot: I haz the power, 2013-01-09 01:47:22]

Change committed as 175656