Support fixed prefixes and suffixes when extracting terms from search template URLs.

Support fixed prefixes and suffixes when extracting terms from search template URLs.

This causes autocompletions for custom search engines where the query parameter
has some prefix or suffix to work correctly, instead of duplicating it.

BUG=602450
TEST=components_unittests:TemplateURLTest.ExtractSearchTermsWithPrefixAndSuffix

Committed: https://crrev.com/508320252e351bc010cc25d837b55baf6d0d22c4
Cr-Commit-Position: refs/heads/master@{#388958}

[jbroman, 2016-04-19 23:21:13]

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

[jbroman, 2016-04-19 23:21:13]

One could imagine being yet more general, but this at least solves my case, and
I don't think it regresses other behavior.

[Peter Kasting, 2016-04-21 19:48:58]

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

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:102: base::StringPiece needle,
Nit: I'd avoid references to English idioms like this -- maybe |search_within|
and |to_find|?

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:104: std::string* suffix) {
Since this is only ever called with the two strings of a SearchTermsKeyResult,
why not move that struct up above this and then simply have a
SearchTermsKeyResult* third arg?

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:552: if (search_terms_position)
Nit: {}

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.h:471: mutable std::string
search_term_value_suffix_;
I wonder if it would be any simpler/clearer to use a url::Component for this
instead, e.g.

  mutable url::Parsed::ComponentType search_term_key_component_;
  mutable url::Component search_term_value_location_;

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

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url_unittest.cc:1218:
EXPECT_EQ(ASCIIToUTF16("invalid"), result);
Hmm.  I don't think we should allow "foo.com/?q=xyz-{searchTerms}" to match
against "foo.com/?q=abc".  That's too aggressive; it's not clear why the user
would want to allow this.

If the pattern URL has a prefix/suffix, I think we shouldn't match unless
they're present in the input URL.

[jbroman, 2016-04-21 21:01:42]

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

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:102: base::StringPiece needle,
On 2016/04/21 at 19:48:57, Peter Kasting wrote:
> Nit: I'd avoid references to English idioms like this -- maybe |search_within|
and |to_find|?

I'm used to variables being nouns; how about "text" and "pattern"?

If you'd prefer search_within and to_find, I'll do that instead in the next
pass.

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:104: std::string* suffix) {
On 2016/04/21 at 19:48:57, Peter Kasting wrote:
> Since this is only ever called with the two strings of a SearchTermsKeyResult,
why not move that struct up above this and then simply have a
SearchTermsKeyResult* third arg?

I had some preference for keeping this method as easy to understand as possible
by having it only be a string search utility, without knowledge of the query
keys and values.

It also seems slightly weird to have a function that takes a struct pointer, but
only partially populates it. This way, it seems more obvious to me that all of
the struct fields do end up getting populated.

(It also makes one line wrap weirdly, as it happens.)

Would you rather I change it?

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:552: if (search_terms_position)
On 2016/04/21 at 19:48:57, Peter Kasting wrote:
> Nit: {}

Obsoleted by addressing another comment.

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.h:471: mutable std::string
search_term_value_suffix_;
On 2016/04/21 at 19:48:57, Peter Kasting wrote:
> I wonder if it would be any simpler/clearer to use a url::Component for this
instead, e.g.
> 
>   mutable url::Parsed::ComponentType search_term_key_component_;
>   mutable url::Component search_term_value_location_;

I don't understand; IIUC url::Component is a (begin, length) pair.

Suppose the query param is "prefix{searchTerms}suffix"; the beginning position
is fixed, but the length will vary (it will be the length of the actual search
terms, which varies).

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

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url_unittest.cc:1218:
EXPECT_EQ(ASCIIToUTF16("invalid"), result);
On 2016/04/21 at 19:48:57, Peter Kasting wrote:
> Hmm.  I don't think we should allow "foo.com/?q=xyz-{searchTerms}" to match
against "foo.com/?q=abc".  That's too aggressive; it's not clear why the user
would want to allow this.
> 
> If the pattern URL has a prefix/suffix, I think we shouldn't match unless
they're present in the input URL.

My reasoning was that they were accepted before this CL, so it seemed to make
the change have less impact. I've changed it to require the prefix and suffix to
exist, and adjusted this test case accordingly.

[Peter Kasting, 2016-04-21 22:29:46]

LGTM

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

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:102: base::StringPiece needle,
On 2016/04/21 21:01:41, jbroman wrote:
> On 2016/04/21 at 19:48:57, Peter Kasting wrote:
> > Nit: I'd avoid references to English idioms like this -- maybe
|search_within|
> and |to_find|?
> 
> I'm used to variables being nouns; how about "text" and "pattern"?

Works for me.  I don't think anything is perfectly clear here.  I tried looking
in string_util.h and didn't love its names either :)

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:104: std::string* suffix) {
On 2016/04/21 21:01:42, jbroman wrote:
> On 2016/04/21 at 19:48:57, Peter Kasting wrote:
> > Since this is only ever called with the two strings of a
SearchTermsKeyResult,
> why not move that struct up above this and then simply have a
> SearchTermsKeyResult* third arg?
> 
> I had some preference for keeping this method as easy to understand as
possible
> by having it only be a string search utility, without knowledge of the query
> keys and values.
> 
> It also seems slightly weird to have a function that takes a struct pointer,
but
> only partially populates it. This way, it seems more obvious to me that all of
> the struct fields do end up getting populated.
> 
> (It also makes one line wrap weirdly, as it happens.)
> 
> Would you rather I change it?

I think it's OK either way.  If you like it the way it is, leave it.

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/1902263002/diff/1/components/search_engines/t...
components/search_engines/template_url.h:471: mutable std::string
search_term_value_suffix_;
On 2016/04/21 21:01:42, jbroman wrote:
> On 2016/04/21 at 19:48:57, Peter Kasting wrote:
> > I wonder if it would be any simpler/clearer to use a url::Component for this
> instead, e.g.
> > 
> >   mutable url::Parsed::ComponentType search_term_key_component_;
> >   mutable url::Component search_term_value_location_;
> 
> I don't understand; IIUC url::Component is a (begin, length) pair.
> 
> Suppose the query param is "prefix{searchTerms}suffix"; the beginning position
> is fixed, but the length will vary (it will be the length of the actual search
> terms, which varies).

Yeah, sorry, I was misconceiving how these were being used.

[jbroman, 2016-04-21 22:58:32]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-04-21 22:59:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902263002/20001

[commit-bot: I haz the power, 2016-04-21 23:53:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-22 19:41:48]

Description was changed from

==========
Support fixed prefixes and suffixes when extracting terms from search template
URLs.

This causes autocompletions for custom search engines where the query parameter
has some prefix or suffix to work correctly, instead of duplicating it.

BUG=602450
TEST=components_unittests:TemplateURLTest.ExtractSearchTermsWithPrefixAndSuffix
==========

to

==========
Support fixed prefixes and suffixes when extracting terms from search template
URLs.

This causes autocompletions for custom search engines where the query parameter
has some prefix or suffix to work correctly, instead of duplicating it.

BUG=602450
TEST=components_unittests:TemplateURLTest.ExtractSearchTermsWithPrefixAndSuffix

Committed: https://crrev.com/508320252e351bc010cc25d837b55baf6d0d22c4
Cr-Commit-Position: refs/heads/master@{#388958}
==========

[commit-bot: I haz the power, 2016-04-22 19:41:49]

Patchset 2 (id:??) landed as
https://crrev.com/508320252e351bc010cc25d837b55baf6d0d22c4
Cr-Commit-Position: refs/heads/master@{#388958}