Upstreaming mechanism to add query refinement to omnibox searches.

Upstreaming mechanism to add query refinement to omnibox searches.

This patch adds the wiring to allow query refinement by replacing the search terms in a URL without touching the other URL parameters.

BUG=178604
TBR=mpcomplete@chromium.org

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

[beaudoin, 2013-03-19 21:33:30]

Here is my upstreaming patch for query refinement. It compiles and test passes
on OsX, having the trybots check it on other platforms. I think it should just
work. (ha!)

This brings over much of the wiring needed to easily implement query refinement
of the style you need on mobile. What is not included are the changes to
AutocompleteController.

I think these can easily be done downstream for now. For the upstream I'd like
to take more time and use the approach proposed by Peter of having the providers
insert the AQS data.

[Peter Kasting, 2013-03-19 23:46:24]

You specified multiple reviewers.  Whom do you want to review what?

[beaudoin, 2013-03-20 02:00:42]

Sorry Peter, I should have CC'd Rohit and Yusuf instead. I think you're the best
reviewer for this. I've tried to keep the patch as small as possible. The large
number of files touched is essentially because many tests use AutocompleteInput
and I've added a parameter there.

Thanks!

[Peter Kasting, 2013-03-21 06:58:19]

On 2013/03/20 02:00:42, beaudoin wrote:
> Sorry Peter, I should have CC'd Rohit and Yusuf instead. I think you're the
best
> reviewer for this.

OK.  I'm WebKit sheriff at the moment, so my time is a bit in short supply.  I
will try to look at this within the next few days.

[Peter Kasting, 2013-03-22 20:20:06]

LGTM

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_input.h:60: // search result page, the
search provider will generate an image search URL.
Nit: will -> may

I don't see any place in this change that passes something non-empty here.  If
you're not sure this will be coming soon, I'd note that this is only set by code
in the mobile ports.  (You may want to note that query refinement in general is
only done on mobile anyway.)

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:882: &search_term_location,
&search_terms_position))
Nit: Lines of args should all be aligned.  Indent this further or else move the
args on the first line to their own new line.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:897: switch (search_term_location)
{
This switch can be eliminated, and the function shortened, by doing something
like this:

  std::string old_params((search_term_location == url_parse::Parsed::REF) ?
      url.ref() : url.query());
  std::string new_params(old_params, 0, search_terms_position.begin);
  params += UTF16ToUTF8(search_terms_args.search_terms);
  params += old_params.substr(search_terms_position.end());
  url_canon::StdStringReplacements<std::string> replacements;
  if (search_term_location == url_parse::Parsed::REF)
    replacements.SetRefStr(new_params);
  else
    replacements.SetQueryStr(new_params);
  ...

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:937: if (input_encoding->empty())
{
This last block can be avoided by rewriting the function as something like:

  std::vector<std::string> encodings(input_encodings());
  if (std::find(encodings.begin(), encodings.end(), "UTF-8") == encodings.end())
    encodings.push_back("UTF-8");
  for (...) {
    if (...) {
      ...;
      return;
    }
  }
  NOTREACHED();

This does copy the encodings vector but that should be very short (a few items
at most).

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:155:
url_parse::Parsed::ComponentType* search_term_location,
Nit: I suggest |search_term_component| here and in FindSearchTermsInURL()

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:524: // terms and replace them with
the ones in |search_terms_args|, leaving the
Nit: replace -> replaces

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:527: bool ReplaceSearchTermsInURL(
As with autocomplete_input.h, I'd specifically note on any functions that are
only called by downstream code that that's the case, so no one is tempted to
eliminate these.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:532: // Encode the search terms
from |search_terms_args| so that we know the
Nit: Encode -> Encodes

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:559: // Use the alternate URLs and
the search URL to match the provided |url|
Nit: Use -> Uses

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/ui/webui/om...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/ui/webui/om...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:158: GURL empty_url;
Nit: Let's nuke |empty_string| and |empty_url| and just pass string16() and
GURL() below.

We can probably also nuke |cursor_position| and rename |cursor_position_int| to
|cursor_position|, since I suspect the compiler will auto-upcast if we provide
an int as an arg in the call.

[beaudoin, 2013-03-22 23:10:43]

Applied Peter's comments.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_input.h:60: // search result page, the
search provider will generate an image search URL.
On 2013/03/22 20:20:06, Peter Kasting wrote:
> Nit: will -> may
> 
> I don't see any place in this change that passes something non-empty here.  If
> you're not sure this will be coming soon, I'd note that this is only set by
code
> in the mobile ports.  (You may want to note that query refinement in general
is
> only done on mobile anyway.)

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:882: &search_term_location,
&search_terms_position))
On 2013/03/22 20:20:06, Peter Kasting wrote:
> Nit: Lines of args should all be aligned.  Indent this further or else move
the
> args on the first line to their own new line.

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:897: switch (search_term_location)
{
On 2013/03/22 20:20:06, Peter Kasting wrote:
> This switch can be eliminated, and the function shortened, by doing something
> like this:
> 
>   std::string old_params((search_term_location == url_parse::Parsed::REF) ?
>       url.ref() : url.query());
>   std::string new_params(old_params, 0, search_terms_position.begin);
>   params += UTF16ToUTF8(search_terms_args.search_terms);
>   params += old_params.substr(search_terms_position.end());
>   url_canon::StdStringReplacements<std::string> replacements;
>   if (search_term_location == url_parse::Parsed::REF)
>     replacements.SetRefStr(new_params);
>   else
>     replacements.SetQueryStr(new_params);
>   ...

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:937: if (input_encoding->empty())
{
On 2013/03/22 20:20:06, Peter Kasting wrote:
> This last block can be avoided by rewriting the function as something like:
> 
>   std::vector<std::string> encodings(input_encodings());
>   if (std::find(encodings.begin(), encodings.end(), "UTF-8") ==
encodings.end())
>     encodings.push_back("UTF-8");
>   for (...) {
>     if (...) {
>       ...;
>       return;
>     }
>   }
>   NOTREACHED();
> 
> This does copy the encodings vector but that should be very short (a few items
> at most).

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:155:
url_parse::Parsed::ComponentType* search_term_location,
On 2013/03/22 20:20:06, Peter Kasting wrote:
> Nit: I suggest |search_term_component| here and in FindSearchTermsInURL()

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:155:
url_parse::Parsed::ComponentType* search_term_location,
On 2013/03/22 20:20:06, Peter Kasting wrote:
> Nit: I suggest |search_term_component| here and in FindSearchTermsInURL()

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:524: // terms and replace them with
the ones in |search_terms_args|, leaving the
On 2013/03/22 20:20:06, Peter Kasting wrote:
> Nit: replace -> replaces

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:527: bool ReplaceSearchTermsInURL(
On 2013/03/22 20:20:06, Peter Kasting wrote:
> As with autocomplete_input.h, I'd specifically note on any functions that are
> only called by downstream code that that's the case, so no one is tempted to
> eliminate these.

I hope to upstream the call to this too, and make sure it's only called when
AutocompleteInput::current_url_ is non-empty. I have not done it yet because I
wanted to solve the AQS issue in a cleaner way.

In all cases, I've added the comment you propose for now.

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:532: // Encode the search terms
from |search_terms_args| so that we know the
On 2013/03/22 20:20:06, Peter Kasting wrote:
> Nit: Encode -> Encodes

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:559: // Use the alternate URLs and
the search URL to match the provided |url|
On 2013/03/22 20:20:06, Peter Kasting wrote:
> Nit: Use -> Uses

Done.

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/ui/webui/om...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/12623029/diff/7001/chrome/browser/ui/webui/om...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:158: GURL empty_url;
On 2013/03/22 20:20:06, Peter Kasting wrote:
> Nit: Let's nuke |empty_string| and |empty_url| and just pass string16() and
> GURL() below.
> 
> We can probably also nuke |cursor_position| and rename |cursor_position_int|
to
> |cursor_position|, since I suspect the compiler will auto-upcast if we provide
> an int as an arg in the call.

Done.

[commit-bot: I haz the power, 2013-03-22 23:23:17]

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

[commit-bot: I haz the power, 2013-03-22 23:23:28]

Presubmit check for 12623029-15001 failed and returned exit status 1.

INFO:root:Found 26 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/extensions/api/omnibox/omnibox_apitest.cc

Presubmit checks took 2.2s to calculate.

[beaudoin, 2013-03-23 03:45:44]

Committed patchset #3 manually as r189981 (presubmit successful).