Add search_terms_replacement_key field to TemplateURL.

Add search_terms_replacement_key field to TemplateURL.

This CL is the first step in removing the hardcoded "espv" parameter used by instant-extended to detect when to perform search term replacement in the omnibox.
The search_terms_replacement_key has beed added to prepopulated_engines, TemplateURL, policy, web_database, prefs, and sync.

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

[beaudoin, 2012-12-12 23:25:05]

-sky (although your review is welcome, everything seems covered.)

This is a cross-cutting CL similar to https://codereview.chromium.org/10908226/

Review assignments:
@joaodasilva : c/a/policy/*, c/b/policy/*, c/common/*, c/test/data/policy/*
@pkasting : c/b/search_engines/*, c/b/webdata/keyword*
@dhollowa : c/b/webdata/web_database*
@akalin : sync/*

Thanks a lot in advance!

[dhollowa, 2012-12-13 01:23:08]

c/b/webdata/web_database* LGTM w/ nit.  I didn't look at the other stuff.

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_migration_unittest.cc:2373: // Because this
test doesn't rely on a valid signature, we can run it on
nit: This comment looks like a copy/paste error since there's no ifdef.

[Joao da Silva, 2012-12-13 10:12:26]

policy stuff lgtm!

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/search_engi...
File chrome/browser/search_engines/search_terms_data.h (right):

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/search_engi...
chrome/browser/search_engines/search_terms_data.h:58: // Returns the parameter
key used in InstantExtended URLs. To get the a
To get the

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database.cc (right):

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database.cc:24: const int
WebDatabase::kCurrentVersionNumber = 48;
Shouldn't this version match kCurrentDataVersion in prepopulated_engines.json?

[beaudoin, 2012-12-13 17:14:53]

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/search_engi...
File chrome/browser/search_engines/search_terms_data.h (right):

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/search_engi...
chrome/browser/search_engines/search_terms_data.h:58: // Returns the parameter
key used in InstantExtended URLs. To get the a
On 2012/12/13 10:12:26, Joao da Silva wrote:
> To get the

Done.

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database.cc (right):

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database.cc:24: const int
WebDatabase::kCurrentVersionNumber = 48;
On 2012/12/13 10:12:26, Joao da Silva wrote:
> Shouldn't this version match kCurrentDataVersion in prepopulated_engines.json?

I don't think so, I believe their closeness is coincidental. (They were
out-of-sync before and it makes sense since WebDatabase has more than search
engine URL stuff.) If they need to be keep in sync I'll update the comments.
Maybe pkasting can confirm?

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/11552020/diff/5001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_migration_unittest.cc:2373: // Because this
test doesn't rely on a valid signature, we can run it on
On 2012/12/13 01:23:08, dhollowa wrote:
> nit: This comment looks like a copy/paste error since there's no ifdef.

"We _can_ run it on official builds as well." :)

Still, this will all become moot as soon as this lands:
https://codereview.chromium.org/11493003/

I'll rework the tests then.

[beaudoin, 2012-12-14 23:40:03]

PTAL

Merged with the massive CL removing protector code. (Found here:
https://chromiumcodereview.appspot.com/11493003 )

This bumped a few version numbers and made it possible to include the test
database right in this CL. Please double-check that I did not do any obvious
blunder.

A quick review would be nice as the code under this moves relatively frequently.

[Ivan Korotkov, 2012-12-15 08:09:10]

Merge with protector CL lgtm

[Peter Kasting, 2012-12-18 01:54:42]

LGTM

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines_schema.json:40: // and in the
ref if the search terms are in the ref.
Two generalization questions.  Both of these spring from trying to ensure we
don't enforce any Google-specific constraints on other search providers.
* Is "nonzero" what we want?  Maybe we should make this "nonempty"?  I don't
know if everyone will want a numeric param value for this.
* Is the constraint about query/ref matching something we need to support? 
Perhaps we should just check if this appears anywhere in the URL after the path.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/search_terms_data.h (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/search_terms_data.h:60: virtual std::string
InstantExtendedEnabledKey() const;
How come this needs to be virtual (or in fact defined in SearchTermsData at
all)?  Seems like we could just access the public string in google_util directly
at the callsite.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.cc:319: url.insert(i->index,
search_terms_data.InstantExtendedEnabledKey());
Because this string is really a constant, if you access it directly instead of
using a method on SearchTermsData you can substitute its value directly in
ParseParameter(), and get rid of the new enum value entirely.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:357: 
Nit: Extra newline

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:417: }
Nit: Keep the accessor/mutator order here the same as the member order in
TemplateURLData above.  This should probably go right after sync_guid().

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1139:
data.search_terms_replacement_key = search_terms_replacement_key.as_string();
Nit: Set this member in the same order that the TempalteURLData members were
declared.  Also make the function argument order match.  And similar comments
everywhere else this parameter/member appears alongside others.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/ke...
File chrome/browser/webdata/keyword_table.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/ke...
chrome/browser/webdata/keyword_table.cc:377: // Fill the
|search_terms_replacement_key| column with empty strings,
Nit: Maybe just: "...strings; see comments in
MigrateToVersion47AddAlternateURLsColumn()."  This makes it clearer that both
these cases behave the same for the same reason.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/ke...
File chrome/browser/webdata/keyword_table_unittest.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/ke...
chrome/browser/webdata/keyword_table_unittest.cc:166:
keyword.search_terms_replacement_key = "";
Nit: .clear() (2 places)

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/we...
File chrome/browser/webdata/web_database.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/we...
chrome/browser/webdata/web_database.cc:28: const int kCompatibleVersionNumber =
49;
I think maybe the compatible version can stay at 48.  Old versions of Chrome
could read and write the newer DB format without breaking anything, I think?

[beaudoin, 2012-12-20 04:23:29]

Answered Peter's comments.

@dcblack: A few questions regarding GWS.
@akalin: Still need an LGTM for search
@dhollowa @joaodasilva: PTAL

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines_schema.json:40: // and in the
ref if the search terms are in the ref.
On 2012/12/18 01:54:43, Peter Kasting wrote:
> Two generalization questions.  Both of these spring from trying to ensure we
> don't enforce any Google-specific constraints on other search providers.
> * Is "nonzero" what we want?  Maybe we should make this "nonempty"?  I don't
> know if everyone will want a numeric param value for this.

With "espv=0" GWS does not remove the textbox in the search result page.
@dcblack Can/should this be changed?

> * Is the constraint about query/ref matching something we need to support? 
> Perhaps we should just check if this appears anywhere in the URL after the
path.

Again, I used GWS behavior to guide me here.
@dcblack Can/should we change GWS here? Should we just don't care if URLs with
the "espv=" in query and "q=" in ref (and vice-versa) are never generated?

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/search_terms_data.h (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/search_terms_data.h:60: virtual std::string
InstantExtendedEnabledKey() const;
On 2012/12/18 01:54:43, Peter Kasting wrote:
> How come this needs to be virtual (or in fact defined in SearchTermsData at
> all)?  Seems like we could just access the public string in google_util
directly
> at the callsite.

Done.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.cc:319: url.insert(i->index,
search_terms_data.InstantExtendedEnabledKey());
On 2012/12/18 01:54:43, Peter Kasting wrote:
> Because this string is really a constant, if you access it directly instead of
> using a method on SearchTermsData you can substitute its value directly in
> ParseParameter(), and get rid of the new enum value entirely.

Done.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:357: 
On 2012/12/18 01:54:43, Peter Kasting wrote:
> Nit: Extra newline

Done.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:417: }
On 2012/12/18 01:54:43, Peter Kasting wrote:
> Nit: Keep the accessor/mutator order here the same as the member order in
> TemplateURLData above.  This should probably go right after sync_guid().

Done.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1139:
data.search_terms_replacement_key = search_terms_replacement_key.as_string();
On 2012/12/18 01:54:43, Peter Kasting wrote:
> Nit: Set this member in the same order that the TempalteURLData members were
> declared.  Also make the function argument order match.  And similar comments
> everywhere else this parameter/member appears alongside others.

Fixed alternate_urls at the same time.

Done.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/ke...
File chrome/browser/webdata/keyword_table.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/ke...
chrome/browser/webdata/keyword_table.cc:377: // Fill the
|search_terms_replacement_key| column with empty strings,
On 2012/12/18 01:54:43, Peter Kasting wrote:
> Nit: Maybe just: "...strings; see comments in
> MigrateToVersion47AddAlternateURLsColumn()."  This makes it clearer that both
> these cases behave the same for the same reason.

Done.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/ke...
File chrome/browser/webdata/keyword_table_unittest.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/ke...
chrome/browser/webdata/keyword_table_unittest.cc:166:
keyword.search_terms_replacement_key = "";
On 2012/12/18 01:54:43, Peter Kasting wrote:
> Nit: .clear() (2 places)

Done.

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/we...
File chrome/browser/webdata/web_database.cc (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/webdata/we...
chrome/browser/webdata/web_database.cc:28: const int kCompatibleVersionNumber =
49;
On 2012/12/18 01:54:43, Peter Kasting wrote:
> I think maybe the compatible version can stay at 48.  Old versions of Chrome
> could read and write the newer DB format without breaking anything, I think?

Not 100% what this entails. I'll trust you on this. Tests seem happy. :)

Done.

[Joao da Silva, 2012-12-20 09:20:12]

policy/ slgtm

[akalin, 2012-12-20 17:13:59]

lgtm after nits below

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:355: // A parameter that, if
present and non-zero in a search_url or instant_url,
what exactly does 'non-zero' mean?  Do you mean 'non-empty'?  If so, perhaps it
would be clearer to use 'non-empty'.

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/webdata/we...
chrome/browser/webdata/web_database_migration_unittest.cc:2099:
"search_terms_replacement_key"));
fix indent

https://codereview.chromium.org/11552020/diff/40001/sync/protocol/search_engi...
File sync/protocol/search_engine_specifics.proto (right):

https://codereview.chromium.org/11552020/diff/40001/sync/protocol/search_engi...
sync/protocol/search_engine_specifics.proto:65: // Parameter that, if non-zero
in a search URL, indicates that Chrome should
non-zero -> non-empty?

[Peter Kasting, 2012-12-20 19:09:28]

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:355: // A parameter that, if
present and non-zero in a search_url or instant_url,
On 2012/12/20 17:13:59, akalin wrote:
> what exactly does 'non-zero' mean?  Do you mean 'non-empty'?  If so, perhaps
it
> would be clearer to use 'non-empty'.

This was my same question.  See the earlier exchange with me.

[David Black, 2012-12-20 19:29:49]

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines_schema.json:40: // and in the
ref if the search terms are in the ref.
espv being present is fine.  From GWS's perspective espv=0 is conceptually
undefined, but so is espv=2 or espv=foobar currently, so whatever.  Such urls
should never be generated.

> I don't know if everyone will want a numeric param value for this.

The parameter value is explicitly the version of the embedded search javascript
API that Chrome is using.  It's currently 1, and will increment when we make
backwards-incompatible changes.  Its value is not something search providers get
to choose.

On 2012/12/20 04:23:29, beaudoin wrote:
> On 2012/12/18 01:54:43, Peter Kasting wrote:
> > Two generalization questions.  Both of these spring from trying to ensure we
> > don't enforce any Google-specific constraints on other search providers.
> > * Is "nonzero" what we want?  Maybe we should make this "nonempty"?  I don't
> > know if everyone will want a numeric param value for this.
> 
> With "espv=0" GWS does not remove the textbox in the search result page.
> @dcblack Can/should this be changed?
> 
> > * Is the constraint about query/ref matching something we need to support? 
> > Perhaps we should just check if this appears anywhere in the URL after the
> path.
> 
> Again, I used GWS behavior to guide me here.
> @dcblack Can/should we change GWS here? Should we just don't care if URLs with
> the "espv=" in query and "q=" in ref (and vice-versa) are never generated?
>

[Peter Kasting, 2012-12-20 19:37:12]

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines_schema.json:40: // and in the
ref if the search terms are in the ref.
On 2012/12/20 19:29:49, David Black wrote:
> espv being present is fine.  From GWS's perspective espv=0 is conceptually
> undefined, but so is espv=2 or espv=foobar currently, so whatever.  Such urls
> should never be generated.

What about the query-versus-ref issue?  Is it OK to just look for the param
anywhere in either one?

> > I don't know if everyone will want a numeric param value for this.
> 
> The parameter value is explicitly the version of the embedded search
javascript
> API that Chrome is using.  It's currently 1, and will increment when we make
> backwards-incompatible changes.  Its value is not something search providers
get
> to choose.

I think you're describing the {google:instantExtendedParam} or whatever we call
it, which Chrome generates a value for.  That's different than this field, which
is the param we parse out of the URL to see if we should substitute search
terms.  For Google, these are actually the same param, but for another search
provider, they could completely avoid the Google-specific param entirely.

[akalin, 2012-12-20 19:41:26]

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:355: // A parameter that, if
present and non-zero in a search_url or instant_url,
On 2012/12/20 19:09:29, Peter Kasting wrote:
> On 2012/12/20 17:13:59, akalin wrote:
> > what exactly does 'non-zero' mean?  Do you mean 'non-empty'?  If so, perhaps
> it
> > would be clearer to use 'non-empty'.
> 
> This was my same question.  See the earlier exchange with me.

I see.  I still think it should be clarified.  So non-zero means not equal to
the string "0"?  And empty string is a valid value distinct from it not being
present?

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

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:355: // A parameter that, if
present and non-zero in a search_url or instant_url,
On 2012/12/20 19:41:26, akalin wrote:
> On 2012/12/20 19:09:29, Peter Kasting wrote:
> > On 2012/12/20 17:13:59, akalin wrote:
> > > what exactly does 'non-zero' mean?  Do you mean 'non-empty'?  If so,
perhaps
> > it
> > > would be clearer to use 'non-empty'.
> > 
> > This was my same question.  See the earlier exchange with me.
> 
> I see.  I still think it should be clarified.  So non-zero means not equal to
> the string "0"?  And empty string is a valid value distinct from it not being
> present?

It's sounding from David's answer like we can scrap the nonzero and nonempty
parts and just use "present", which will make this moot.

[akalin, 2012-12-20 19:45:54]

On 2012/12/20 19:43:16, Peter Kasting wrote:
>
https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
> File chrome/browser/search_engines/template_url.h (right):
> 
>
https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
> chrome/browser/search_engines/template_url.h:355: // A parameter that, if
> present and non-zero in a search_url or instant_url,
> On 2012/12/20 19:41:26, akalin wrote:
> > On 2012/12/20 19:09:29, Peter Kasting wrote:
> > > On 2012/12/20 17:13:59, akalin wrote:
> > > > what exactly does 'non-zero' mean?  Do you mean 'non-empty'?  If so,
> perhaps
> > > it
> > > > would be clearer to use 'non-empty'.
> > > 
> > > This was my same question.  See the earlier exchange with me.
> > 
> > I see.  I still think it should be clarified.  So non-zero means not equal
to
> > the string "0"?  And empty string is a valid value distinct from it not
being
> > present?
> 
> It's sounding from David's answer like we can scrap the nonzero and nonempty
> parts and just use "present", which will make this moot.

SGTM

[akalin, 2012-12-20 19:48:20]

On 2012/12/20 19:45:54, akalin wrote:
> On 2012/12/20 19:43:16, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
> > File chrome/browser/search_engines/template_url.h (right):
> > 
> >
>
https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
> > chrome/browser/search_engines/template_url.h:355: // A parameter that, if
> > present and non-zero in a search_url or instant_url,
> > On 2012/12/20 19:41:26, akalin wrote:
> > > On 2012/12/20 19:09:29, Peter Kasting wrote:
> > > > On 2012/12/20 17:13:59, akalin wrote:
> > > > > what exactly does 'non-zero' mean?  Do you mean 'non-empty'?  If so,
> > perhaps
> > > > it
> > > > > would be clearer to use 'non-empty'.
> > > > 
> > > > This was my same question.  See the earlier exchange with me.
> > > 
> > > I see.  I still think it should be clarified.  So non-zero means not equal
> to
> > > the string "0"?  And empty string is a valid value distinct from it not
> being
> > > present?
> > 
> > It's sounding from David's answer like we can scrap the nonzero and nonempty
> > parts and just use "present", which will make this moot.
> 
> SGTM

Actually, shouldn't it be 'non-empty'?  present vs. non-present is only for
protobufs, but if we're assigning to a member variable, empty and not present
are conflated anyway

[David Black, 2012-12-20 23:19:01]

> What about the query-versus-ref issue?  Is it OK to just look for the param
> anywhere in either one?

Yeah, that should be fine.  I can't think of any legitimate case where it would
be in one but not the other or any other weird mismatch scenario.

> I think you're describing the {google:instantExtendedParam} or whatever we
call
> it, which Chrome generates a value for.  That's different than this field,
which
> is the param we parse out of the URL to see if we should substitute search
> terms.  For Google, these are actually the same param, but for another search
> provider, they could completely avoid the Google-specific param entirely.

Hrm.  Search providers should certainly always be informed about the embedded
search API version.  What's the value in having these two parameters be
different?  Is the theory that some providers might not want search term
replacement?

On 2012/12/20 19:48:20, akalin wrote:
> On 2012/12/20 19:45:54, akalin wrote:
> > On 2012/12/20 19:43:16, Peter Kasting wrote:
> > >
> >
>
https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
> > > File chrome/browser/search_engines/template_url.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
> > > chrome/browser/search_engines/template_url.h:355: // A parameter that, if
> > > present and non-zero in a search_url or instant_url,
> > > On 2012/12/20 19:41:26, akalin wrote:
> > > > On 2012/12/20 19:09:29, Peter Kasting wrote:
> > > > > On 2012/12/20 17:13:59, akalin wrote:
> > > > > > what exactly does 'non-zero' mean?  Do you mean 'non-empty'?  If so,
> > > perhaps
> > > > > it
> > > > > > would be clearer to use 'non-empty'.
> > > > > 
> > > > > This was my same question.  See the earlier exchange with me.
> > > > 
> > > > I see.  I still think it should be clarified.  So non-zero means not
equal
> > to
> > > > the string "0"?  And empty string is a valid value distinct from it not
> > being
> > > > present?
> > > 
> > > It's sounding from David's answer like we can scrap the nonzero and
nonempty
> > > parts and just use "present", which will make this moot.
> > 
> > SGTM
> 
> Actually, shouldn't it be 'non-empty'?  present vs. non-present is only for
> protobufs, but if we're assigning to a member variable, empty and not present
> are conflated anyway

[Peter Kasting, 2012-12-20 23:23:35]

On 2012/12/20 23:19:01, David Black wrote:
> > I think you're describing the {google:instantExtendedParam} or whatever we
> call
> > it, which Chrome generates a value for.  That's different than this field,
> which
> > is the param we parse out of the URL to see if we should substitute search
> > terms.  For Google, these are actually the same param, but for another
search
> > provider, they could completely avoid the Google-specific param entirely.
> 
> Hrm.  Search providers should certainly always be informed about the embedded
> search API version.  What's the value in having these two parameters be
> different?  Is the theory that some providers might not want search term
> replacement?

Or vice-versa, may want search term replacement without the rest of the instant
extended experience.  Or may simply want a flag name other than the one Google
wants, but to do the same things.

Really the idea is that the code here doesn't know and doesn't care why the
provider might want something some particular way.  As currently implemented,
the fact that this parameter is the same one that is wired to other
Google-specific custom behaviors is coincidental.

[David Black, 2012-12-20 23:53:08]

So long as we have the same security checks for requiring SSL connections in
both places then sure, that makes sense to me.

On 2012/12/20 23:23:35, Peter Kasting wrote:
> On 2012/12/20 23:19:01, David Black wrote:
> > > I think you're describing the {google:instantExtendedParam} or whatever we
> > call
> > > it, which Chrome generates a value for.  That's different than this field,
> > which
> > > is the param we parse out of the URL to see if we should substitute search
> > > terms.  For Google, these are actually the same param, but for another
> search
> > > provider, they could completely avoid the Google-specific param entirely.
> > 
> > Hrm.  Search providers should certainly always be informed about the
embedded
> > search API version.  What's the value in having these two parameters be
> > different?  Is the theory that some providers might not want search term
> > replacement?
> 
> Or vice-versa, may want search term replacement without the rest of the
instant
> extended experience.  Or may simply want a flag name other than the one Google
> wants, but to do the same things.
> 
> Really the idea is that the code here doesn't know and doesn't care why the
> provider might want something some particular way.  As currently implemented,
> the fact that this parameter is the same one that is wired to other
> Google-specific custom behaviors is coincidental.

[beaudoin, 2013-01-03 18:49:39]

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://codereview.chromium.org/11552020/diff/12021/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines_schema.json:40: // and in the
ref if the search terms are in the ref.
On 2012/12/20 19:37:13, Peter Kasting wrote:
> On 2012/12/20 19:29:49, David Black wrote:
> > espv being present is fine.  From GWS's perspective espv=0 is conceptually
> > undefined, but so is espv=2 or espv=foobar currently, so whatever.  Such
urls
> > should never be generated.
> 
> What about the query-versus-ref issue?  Is it OK to just look for the param
> anywhere in either one?
> 
> > > I don't know if everyone will want a numeric param value for this.
> > 
> > The parameter value is explicitly the version of the embedded search
> javascript
> > API that Chrome is using.  It's currently 1, and will increment when we make
> > backwards-incompatible changes.  Its value is not something search providers
> get
> > to choose.
> 
> I think you're describing the {google:instantExtendedParam} or whatever we
call
> it, which Chrome generates a value for.  That's different than this field,
which
> is the param we parse out of the URL to see if we should substitute search
> terms.  For Google, these are actually the same param, but for another search
> provider, they could completely avoid the Google-specific param entirely.

Changed the comment to indicate the parameter need only be present.

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:355: // A parameter that, if
present and non-zero in a search_url or instant_url,
On 2012/12/20 19:43:17, Peter Kasting wrote:
> On 2012/12/20 19:41:26, akalin wrote:
> > On 2012/12/20 19:09:29, Peter Kasting wrote:
> > > On 2012/12/20 17:13:59, akalin wrote:
> > > > what exactly does 'non-zero' mean?  Do you mean 'non-empty'?  If so,
> perhaps
> > > it
> > > > would be clearer to use 'non-empty'.
> > > 
> > > This was my same question.  See the earlier exchange with me.
> > 
> > I see.  I still think it should be clarified.  So non-zero means not equal
to
> > the string "0"?  And empty string is a valid value distinct from it not
being
> > present?
> 
> It's sounding from David's answer like we can scrap the nonzero and nonempty
> parts and just use "present", which will make this moot.

Changed to "present in the query or ref".

Done.

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/11552020/diff/40001/chrome/browser/webdata/we...
chrome/browser/webdata/web_database_migration_unittest.cc:2099:
"search_terms_replacement_key"));
On 2012/12/20 17:13:59, akalin wrote:
> fix indent

Done.

https://codereview.chromium.org/11552020/diff/40001/sync/protocol/search_engi...
File sync/protocol/search_engine_specifics.proto (right):

https://codereview.chromium.org/11552020/diff/40001/sync/protocol/search_engi...
sync/protocol/search_engine_specifics.proto:65: // Parameter that, if non-zero
in a search URL, indicates that Chrome should
On 2012/12/20 17:13:59, akalin wrote:
> non-zero -> non-empty?

Done.

[commit-bot: I haz the power, 2013-01-03 18:50:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11552020/54001

[commit-bot: I haz the power, 2013-01-03 21:22:45]

Change committed as 175014