[TTS] Request Now on Tap data in the Resolve request.

[TTS] Request Now on Tap data in the Resolve request.

Updates TemplateURL to know how to request Now on Tap data for Contextual Search.
Also remove the old unused flag about whether to resolve or not.

BUG=646664, 641543
Committed: https://crrev.com/d9fc4d9e6bfd9f673671b2280351ce7ba41dc68d
Cr-Commit-Position: refs/heads/master@{#419054}

[Donn Denman, 2016-09-14 03:02:26]

The CQ bit was checked by donnd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 03:02:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 03:53:03]

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

[commit-bot: I haz the power, 2016-09-14 03:53:03]

Dry run: This issue passed the CQ dry run.

[Donn Denman, 2016-09-14 16:30:08]

donnd@chromium.org changed reviewers:
+ twellington@chromium.org

[Donn Denman, 2016-09-14 16:30:09]

Theresa, PTAL.

This is a simple change except for the TemplateURL stuff which is cryptic.  I'll
have Peter K look closely at that part.

cc Mark since we're changing a forked file.

[Theresa, 2016-09-15 05:03:32]

looks good overall, just one nit and one question

https://codereview.chromium.org/2343523002/diff/1/chrome/browser/android/cont...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://codereview.chromium.org/2343523002/diff/1/chrome/browser/android/cont...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:58: // The
version of the Now on Tap API that we want to invoke;
nit: replace the semi-colon with a period

https://codereview.chromium.org/2343523002/diff/1/components/search_engines/t...
File components/search_engines/template_url.cc (left):

https://codereview.chromium.org/2343523002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:1024: params.resolve ?
"ctxsl_resolve=1" : "ctxsl_resolve=0");
Do we still need to append ctxsl_resolve=1 or does the server always resolve?

[Donn Denman, 2016-09-15 21:59:59]

donnd@chromium.org changed reviewers:
+ pkasting@chromium.org

[Donn Denman, 2016-09-15 22:00:00]

Peter, PTAL that the changes to TemplateURL.  Thanks!

https://chromiumcodereview.appspot.com/2343523002/diff/1/chrome/browser/andro...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://chromiumcodereview.appspot.com/2343523002/diff/1/chrome/browser/andro...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:58: // The
version of the Now on Tap API that we want to invoke;
On 2016/09/15 05:03:32, Theresa Wellington wrote:
> nit: replace the semi-colon with a period

Done.

https://chromiumcodereview.appspot.com/2343523002/diff/1/components/search_en...
File components/search_engines/template_url.cc (left):

https://chromiumcodereview.appspot.com/2343523002/diff/1/components/search_en...
components/search_engines/template_url.cc:1024: params.resolve ?
"ctxsl_resolve=1" : "ctxsl_resolve=0");
On 2016/09/15 05:03:32, Theresa Wellington wrote:
> Do we still need to append ctxsl_resolve=1 or does the server always resolve?

Verified that it's not needed.  

Independently I realized the code here is a bit tricky, so I added a clarifying
comment.

[Peter Kasting, 2016-09-15 23:05:41]

LGTM

https://chromiumcodereview.appspot.com/2343523002/diff/20001/chrome/browser/a...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://chromiumcodereview.appspot.com/2343523002/diff/20001/chrome/browser/a...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:59: const
int kNowOnTapVersion = 1;
Nit: Consider defining this as close to its use (i.e. within the function below)
as possible and reserving file-scope constants for cross-file-applicable things.
 This is similar to the advice in
http://google.github.io/styleguide/cppguide.html#Local_Variables ("declare them
in as local a scope as possible, and as close to the first use as possible.")
and has the same motivations.

https://chromiumcodereview.appspot.com/2343523002/diff/20001/chrome/browser/a...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:217:
kContextualSearchRequestVersion, selected_text, "", now_on_tap_version);
Nit: While here: "" -> std::string()

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
File components/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
components/search_engines/template_url.cc:1021:
base::IntToString(params.now_on_tap_version));
Nit: I'd probably just inline this into the next statement.

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
File components/search_engines/template_url.h (right):

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
components/search_engines/template_url.h:89: const int now_on_tap_version);
Nit: Don't mark parameters passed by value as const (there is very little
utility in this) -- can you fix the several cases that are here?

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
components/search_engines/template_url.h:125: // data needed, a value > 0
indicates the version of the data to return.
Nit: I might just remove the whole last clause here as it seems redundant.

[Donn Denman, 2016-09-15 23:34:25]

Thanks Peter!

Theresa, based on your verbal LGTM I will try to submit.

https://chromiumcodereview.appspot.com/2343523002/diff/20001/chrome/browser/a...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://chromiumcodereview.appspot.com/2343523002/diff/20001/chrome/browser/a...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:59: const
int kNowOnTapVersion = 1;
On 2016/09/15 23:05:41, Peter Kasting wrote:
> Nit: Consider defining this as close to its use (i.e. within the function
below)
> as possible and reserving file-scope constants for cross-file-applicable
things.
>  This is similar to the advice in
> http://google.github.io/styleguide/cppguide.html#Local_Variables ("declare
them
> in as local a scope as possible, and as close to the first use as possible.")
> and has the same motivations.

I'd rather leave this at the top of this file, since it's essentially
configuration information.

https://chromiumcodereview.appspot.com/2343523002/diff/20001/chrome/browser/a...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:217:
kContextualSearchRequestVersion, selected_text, "", now_on_tap_version);
On 2016/09/15 23:05:41, Peter Kasting wrote:
> Nit: While here: "" -> std::string()

Done.

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
File components/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
components/search_engines/template_url.cc:1021:
base::IntToString(params.now_on_tap_version));
On 2016/09/15 23:05:41, Peter Kasting wrote:
> Nit: I'd probably just inline this into the next statement.

Done.

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
File components/search_engines/template_url.h (right):

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
components/search_engines/template_url.h:89: const int now_on_tap_version);
On 2016/09/15 23:05:41, Peter Kasting wrote:
> Nit: Don't mark parameters passed by value as const (there is very little
> utility in this) -- can you fix the several cases that are here?

Done.

https://chromiumcodereview.appspot.com/2343523002/diff/20001/components/searc...
components/search_engines/template_url.h:125: // data needed, a value > 0
indicates the version of the data to return.
On 2016/09/15 23:05:41, Peter Kasting wrote:
> Nit: I might just remove the whole last clause here as it seems redundant.

Done.

[Donn Denman, 2016-09-15 23:34:33]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2016-09-15 23:34:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2343523002/#ps40001
(title: "Rsponded to Peters comments: inlined a conversion and a few other
nits.")

[commit-bot: I haz the power, 2016-09-15 23:34:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 00:31:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-16 00:34:03]

Description was changed from

==========
[TTS] Request Now on Tap data in the Resolve request.

Updates TemplateURL to know how to request Now on Tap data for Contextual
Search.
Also remove the old unused flag about whether to resolve or not.

BUG=646664,641543
==========

to

==========
[TTS] Request Now on Tap data in the Resolve request.

Updates TemplateURL to know how to request Now on Tap data for Contextual
Search.
Also remove the old unused flag about whether to resolve or not.

BUG=646664,641543

Committed: https://crrev.com/d9fc4d9e6bfd9f673671b2280351ce7ba41dc68d
Cr-Commit-Position: refs/heads/master@{#419054}
==========

[commit-bot: I haz the power, 2016-09-16 00:34:05]

Patchset 3 (id:??) landed as
https://crrev.com/d9fc4d9e6bfd9f673671b2280351ce7ba41dc68d
Cr-Commit-Position: refs/heads/master@{#419054}