Update the contextual search API to include a parameter specifying whether the server shoudl return…

Update the contextual search API to include a parameter specifying whether the server shoudl return a resolved search term.

BUG=397734, 397738
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287681

[jeremycho, 2014-07-29 01:48:09]

Ready for review, thanks!

[donnd, 2014-07-29 18:01:06]

lgtm

https://codereview.chromium.org/424013002/diff/1/components/search_engines/te...
File components/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/424013002/diff/1/components/search_engines/te...
components/search_engines/template_url_unittest.cc:1526: "ctxsl_resolve=1",
result);
Not from this CL, but the indentation looks wrong on this line to me.

[jeremycho, 2014-07-29 21:22:24]

Peter - can you do OWNERs for this?  Thanks!

https://codereview.chromium.org/424013002/diff/1/components/search_engines/te...
File components/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/424013002/diff/1/components/search_engines/te...
components/search_engines/template_url_unittest.cc:1526: "ctxsl_resolve=1",
result);
On 2014/07/29 18:01:06, Donn wrote:
> Not from this CL, but the indentation looks wrong on this line to me.

Done.

[jeremycho, 2014-07-31 01:25:11]

Friendly ping, Peter.

[Peter Kasting, 2014-07-31 05:14:03]

Sorry, I wrote comments, and then somehow they apparenntly never got mailed.

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
components/search_engines/template_url.h:84: ContextualSearchParams(const int
version,
Don't add this in addition to the other constructor.  Replace the other
constructor with this version and update all callsites.

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
components/search_engines/template_url.h:115: // Whether to return a resolved
search term.
What does "return a resolved search term" mean?  There's not enough context for
me to understand the phrase.

[jeremycho, 2014-07-31 20:29:09]

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
components/search_engines/template_url.h:84: ContextualSearchParams(const int
version,
On 2014/07/31 05:14:03, Peter Kasting wrote:
> Don't add this in addition to the other constructor.  Replace the other
> constructor with this version and update all callsites.

The existing constructor gets called by Clank so it can't be replaced in this
commit.  Once this commit is propagated and the Clank code is updated, I'll
delete it.

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
components/search_engines/template_url.h:115: // Whether to return a resolved
search term.
On 2014/07/31 05:14:03, Peter Kasting wrote:
> What does "return a resolved search term" mean?  There's not enough context
for
> me to understand the phrase.

Clarified.

[Peter Kasting, 2014-07-31 20:33:49]

LGTM

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
components/search_engines/template_url.h:84: ContextualSearchParams(const int
version,
On 2014/07/31 20:29:08, jeremycho wrote:
> On 2014/07/31 05:14:03, Peter Kasting wrote:
> > Don't add this in addition to the other constructor.  Replace the other
> > constructor with this version and update all callsites.
> 
> The existing constructor gets called by Clank so it can't be replaced in this
> commit.  Once this commit is propagated and the Clank code is updated, I'll
> delete it.

OK.  Consider a TODO about that if it will take more than a day or two.

https://codereview.chromium.org/424013002/diff/40001/components/search_engine...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/424013002/diff/40001/components/search_engine...
components/search_engines/template_url.h:118: bool resolve;
Cool, this is much more comprehensible now.

This might be even clearer:

  // If true, the server will generate a search term based on the user selection
  // and context.  Otherwise the user selection will be used as-is as the search
  // term.
  bool generate_search_term;

[jeremycho, 2014-07-31 21:08:40]

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/424013002/diff/20001/components/search_engine...
components/search_engines/template_url.h:84: ContextualSearchParams(const int
version,
On 2014/07/31 20:33:49, Peter Kasting wrote:
> On 2014/07/31 20:29:08, jeremycho wrote:
> > On 2014/07/31 05:14:03, Peter Kasting wrote:
> > > Don't add this in addition to the other constructor.  Replace the other
> > > constructor with this version and update all callsites.
> > 
> > The existing constructor gets called by Clank so it can't be replaced in
this
> > commit.  Once this commit is propagated and the Clank code is updated, I'll
> > delete it.
> 
> OK.  Consider a TODO about that if it will take more than a day or two.

Done.

https://codereview.chromium.org/424013002/diff/40001/components/search_engine...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/424013002/diff/40001/components/search_engine...
components/search_engines/template_url.h:118: bool resolve;
On 2014/07/31 20:33:49, Peter Kasting wrote:
> Cool, this is much more comprehensible now.
> 
> This might be even clearer:
> 
>   // If true, the server will generate a search term based on the user
selection
>   // and context.  Otherwise the user selection will be used as-is as the
search
>   // term.
>   bool generate_search_term;

Done.

[jeremycho, 2014-07-31 21:10:21]

The CQ bit was checked by jeremycho@chromium.org

[commit-bot: I haz the power, 2014-07-31 21:13:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jeremycho@chromium.org/424013002/70001

[jeremycho, 2014-07-31 22:10:15]

The CQ bit was checked by jeremycho@chromium.org

[commit-bot: I haz the power, 2014-07-31 22:13:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jeremycho@chromium.org/424013002/90001

[jeremycho, 2014-07-31 23:09:30]

The CQ bit was checked by jeremycho@chromium.org

[commit-bot: I haz the power, 2014-07-31 23:11:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jeremycho@chromium.org/424013002/110001

[commit-bot: I haz the power, 2014-08-01 05:25:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 06:40:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-01 06:40:07]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[jeremycho, 2014-08-05 21:46:19]

The CQ bit was checked by jeremycho@chromium.org

[commit-bot: I haz the power, 2014-08-05 21:49:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jeremycho@chromium.org/424013002/130001

[commit-bot: I haz the power, 2014-08-06 05:36:15]

Change committed as 287681