Add API key to omnibox's suggest requests

Add API key to omnibox's suggest requests

Mostly closed question:
* Is the api key constant over the length of the run?  [pfelding guesses yes.  working assumpting is yes.  The code for the api keys makes it look like the keys are compiled into the binary.  joi to answer for sure.]

Closed questions:
* Send to all google suggest base URLs or only certain ones?  [ANSWERED by afeldman: might as well send to everything]
* Send on all protocols or just https?  [ANSWERED by afeldman: all protocols]
* The way I formatted the suggest URL parameter puts the parameter last in the URL.  I think this looks better for people looking at URL.  However, given the standard structure in template_url.cc of the way replacement is done, that means we end up with an extra & at the end of the URL.  Is that a problem?  [RESOLVED by dcblack by moving sugkey parameter into template itself]
* Do I have to do anything to update users who already have their template URL instantiated?  (I think the answer is no, but I'm asking to double-check.) [ANSWERED by pkasting: increment version number in the prepopulated template URL file]

BUG=
TEST=tested using wireshark

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

[Mark P, 2012-09-14 18:54:04]

Peter,

Can you take a look at this ASAP?  People want it checked in by Monday.

thanks,
mark

[Mark P, 2012-09-14 18:54:57]

And one more question:
* Do I have to do anything to update users who already have their template URL
instantiated?  (I think the answer is no, but I'm asking to double-check.)

[David Black, 2012-09-14 19:05:05]

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

http://codereview.chromium.org/10905292/diff/2003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.cc:290: url.insert(i->index,
"sugkey=" +
It looks like you're confusing the approaches taken here for optional vs
always-present cgi parameters.  Always-present params have their "&foo=" part
inline in template_url_prepopulate_data.cc and just stick the value in here,
like case SEARCH_TERMS.  Optional params, like GOOGLE_SEARCH_FIELDTRIAL_GROUP,
keep everything here because they've got an if statement around 'em.  Either
surround this logic with an if(google_apis::GetAPIKey()) or move the "sugkey="
part into template_url_prepopulate_data.cc.

(Also these urls aren't user visible so cgi param ordering shouldn't matter.)

[Mark P, 2012-09-14 19:20:34]

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

http://codereview.chromium.org/10905292/diff/2003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.cc:290: url.insert(i->index,
"sugkey=" +
On 2012/09/14 19:05:05, David Black wrote:
> It looks like you're confusing the approaches taken here for optional vs
> always-present cgi parameters.  Always-present params have their "&foo=" part
> inline in template_url_prepopulate_data.cc and just stick the value in here,
> like case SEARCH_TERMS.  Optional params, like GOOGLE_SEARCH_FIELDTRIAL_GROUP,
> keep everything here because they've got an if statement around 'em.  Either
> surround this logic with an if(google_apis::GetAPIKey()) or move the "sugkey="
> part into template_url_prepopulate_data.cc.

Good idea.  Did the latter.

> (Also these urls aren't user visible so cgi param ordering shouldn't matter.)

[afeldman_google.com, 2012-09-14 19:25:16]

> * Send to all google suggest base URLs or only certain ones?
I think you want to send on all Suggest URLs.  If later they decide to
ignore it on some, they can do so, but better than later deciding that they
*want* it on one that we didn't add it to.


On Fri, Sep 14, 2012 at 12:20 PM, <mpearson@chromium.org> wrote:

>
> http://codereview.chromium.**org/10905292/diff/2003/chrome/**
>
browser/search_engines/**template_url.cc<http://codereview.chromium.org/10905292/diff/2003/chrome/browser/search_engines/template_url.cc>
> File chrome/browser/search_engines/**template_url.cc (right):
>
> http://codereview.chromium.**org/10905292/diff/2003/chrome/**
>
browser/search_engines/**template_url.cc#newcode290<http://codereview.chromium.org/10905292/diff/2003/chrome/browser/search_engines/template_url.cc#newcode290>
> chrome/browser/search_engines/**template_url.cc:290: url.insert(i->index,
> "sugkey=" +
> On 2012/09/14 19:05:05, David Black wrote:
>
>> It looks like you're confusing the approaches taken here for optional
>>
> vs
>
>> always-present cgi parameters.  Always-present params have their
>>
> "&foo=" part
>
>> inline in template_url_prepopulate_data.**cc and just stick the value in
>>
> here,
>
>> like case SEARCH_TERMS.  Optional params, like
>>
> GOOGLE_SEARCH_FIELDTRIAL_**GROUP,
>
>> keep everything here because they've got an if statement around 'em.
>>
> Either
>
>> surround this logic with an if(google_apis::GetAPIKey()) or move the
>>
> "sugkey="
>
>> part into template_url_prepopulate_data.**cc.
>>
>
> Good idea.  Did the latter.
>
>
>  (Also these urls aren't user visible so cgi param ordering shouldn't
>>
> matter.)
>
>
http://codereview.chromium.**org/10905292/<http://codereview.chromium.org/109...
>

[Peter Kasting, 2012-09-14 20:47:04]

LGTM once the below issues are addressed.

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

http://codereview.chromium.org/10905292/diff/9001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.cc:291:
net::EscapeQueryParamValue(google_apis::GetAPIKey(), false));
Nit: If the return value of GetAPIKey() is constant over a single Chrome run,
you can substitute it directly in ParseParameter() below and simplify this
change further.

http://codereview.chromium.org/10905292/diff/9001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10905292/diff/9001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1100:
"sugkey={google:suggestAPIKeyParameter}",
If you change anything in here you need to change the version number lower down
in this file.  That will guarantee that everyone's local data will auto-update.

[Mark P, 2012-09-14 21:23:23]

I think this is good to go.

http://codereview.chromium.org/10905292/diff/9001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10905292/diff/9001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1100:
"sugkey={google:suggestAPIKeyParameter}",
On 2012/09/14 20:47:04, Peter Kasting wrote:
> If you change anything in here you need to change the version number lower
down
> in this file.  That will guarantee that everyone's local data will
auto-update.

I knew there was something like that.  That answered my last question in the
changelist description.  Done.

[Mark P, 2012-09-14 21:23:55]

And publishing the last comment:

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

http://codereview.chromium.org/10905292/diff/9001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.cc:291:
net::EscapeQueryParamValue(google_apis::GetAPIKey(), false));
On 2012/09/14 20:47:04, Peter Kasting wrote:
> Nit: If the return value of GetAPIKey() is constant over a single Chrome run,
> you can substitute it directly in ParseParameter() below and simplify this
> change further.

Done.

[Peter Kasting, 2012-09-14 21:24:50]

Yep LGTM

[commit-bot: I haz the power, 2012-09-14 21:27:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/10905292/7

[commit-bot: I haz the power, 2012-09-14 21:27:21]

Failed to apply patch for
chrome/browser/search_engines/template_url_prepopulate_data.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/search_engines/template_url_prepopulate_data.cc
Hunk #1 succeeded at 1097 (offset 1 line).
Hunk #2 FAILED at 3328.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/search_engines/template_url_prepopulate_data.cc.rej

[commit-bot: I haz the power, 2012-09-14 21:35:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/10905292/1006