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

components/search_engines/template_url.cc

Index: components/search_engines/template_url.cc
diff --git a/components/search_engines/template_url.cc b/components/search_engines/template_url.cc
index a29aa26310018148473697e6d93a2d671c952863..42c8ae59d022203f01ecaab924ad46193da85def 100644
--- a/components/search_engines/template_url.cc
+++ b/components/search_engines/template_url.cc
@@ -96,26 +96,49 @@ bool TryEncoding(const base::string16& terms,
   return true;
 }
 
+// Returns true if the search term placeholder is present, and also produces
+// the constant prefix/suffix found.
+bool TryMatchSearchParam(base::StringPiece haystack,
+                         base::StringPiece needle,

[Peter Kasting, 2016/04/21 19:48:57]

Nit: I'd avoid references to English idioms like this -- maybe |search_within|
and |to_find|?

[jbroman, 2016/04/21 21:01:41]

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.

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

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 :)

+                         std::string* prefix,
+                         std::string* suffix) {

[Peter Kasting, 2016/04/21 19:48:57]

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?

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

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?

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

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

+  auto pos = haystack.find(needle);
+  if (pos == base::StringPiece::npos)
+    return false;
+  haystack.substr(0, pos).CopyToString(prefix);
+  haystack.substr(pos + needle.length()).CopyToString(suffix);
+  return true;
+}
+
 // Extract query key and host given a list of parameters coming from the URL
 // query or ref.
-std::string FindSearchTermsKey(const std::string& params) {
+struct SearchTermsKeyResult {
+  std::string key;
+  std::string value_prefix;
+  std::string value_suffix;
+  bool found() const { return !key.empty(); }
+};
+SearchTermsKeyResult FindSearchTermsKey(const std::string& params) {
+  SearchTermsKeyResult result;
   if (params.empty())
-    return std::string();
+    return result;
   url::Component query, key, value;
   query.len = static_cast<int>(params.size());
   while (url::ExtractQueryKeyValue(params.c_str(), &query, &key, &value)) {
     if (key.is_nonempty() && value.is_nonempty()) {
       const base::StringPiece value_string(params.c_str() + value.begin,
                                            value.len);
-      if (value_string.find(kSearchTermsParameterFull, 0) !=
-          base::StringPiece::npos ||
-          value_string.find(kGoogleUnescapedSearchTermsParameterFull, 0) !=
-          base::StringPiece::npos) {
-        return params.substr(key.begin, key.len);
+      if (TryMatchSearchParam(value_string, kSearchTermsParameterFull,
+                              &result.value_prefix, &result.value_suffix) ||
+          TryMatchSearchParam(value_string,
+                              kGoogleUnescapedSearchTermsParameterFull,
+                              &result.value_prefix, &result.value_suffix)) {
+        result.key = params.substr(key.begin, key.len);
+        break;
       }
     }
   }
-  return std::string();
+  return result;
 }
 
 // Extract the position of the search terms' parameter in the URL path.
@@ -516,12 +539,19 @@ bool TemplateURLRef::ExtractSearchTermsFromURL(
   }
 
   // Extract the search term.
-  *search_terms = SearchTermToString16(
-      source.substr(position.begin, position.len));
+  base::StringPiece search_term =
+      base::StringPiece(source).substr(position.begin, position.len);
+  if (search_term.starts_with(search_term_value_prefix_))
+    search_term.remove_prefix(search_term_value_prefix_.size());
+  if (search_term.ends_with(search_term_value_suffix_))
+    search_term.remove_suffix(search_term_value_suffix_.size());
+
+  *search_terms = SearchTermToString16(search_term.as_string());
   if (search_terms_component)
     *search_terms_component = search_term_key_location_;
   if (search_terms_position)

[Peter Kasting, 2016/04/21 19:48:57]

Nit: {}

[jbroman, 2016/04/21 21:01:41]

Obsoleted by addressing another comment.

-    *search_terms_position = position;
+    *search_terms_position = url::Component(search_term.begin() - source.data(),
+                                            search_term.length());
   return true;
 }
 
@@ -783,11 +813,11 @@ void TemplateURLRef::ParseHostAndSearchTermKey(
   if (!url.is_valid())
     return;
 
-  std::string query_key = FindSearchTermsKey(url.query());
-  std::string ref_key = FindSearchTermsKey(url.ref());
+  auto query_result = FindSearchTermsKey(url.query());
+  auto ref_result = FindSearchTermsKey(url.ref());
   url::Component parameter_position;
-  const bool in_query = !query_key.empty();
-  const bool in_ref = !ref_key.empty();
+  const bool in_query = query_result.found();
+  const bool in_ref = ref_result.found();
   const bool in_path = FindSearchTermsInPath(url.path(), &parameter_position);
   if (in_query ? (in_ref || in_path) : (in_ref == in_path))
     return;  // No key or multiple keys found.  We only handle having one key.
@@ -796,11 +826,15 @@ void TemplateURLRef::ParseHostAndSearchTermKey(
   port_ = url.port();
   path_ = url.path();
   if (in_query) {
-    search_term_key_ = query_key;
+    search_term_key_ = query_result.key;
     search_term_key_location_ = url::Parsed::QUERY;
+    search_term_value_prefix_ = query_result.value_prefix;
+    search_term_value_suffix_ = query_result.value_suffix;
   } else if (in_ref) {
-    search_term_key_ = ref_key;
+    search_term_key_ = ref_result.key;
     search_term_key_location_ = url::Parsed::REF;
+    search_term_value_prefix_ = ref_result.value_prefix;
+    search_term_value_suffix_ = ref_result.value_suffix;
   } else {
     DCHECK(in_path);
     DCHECK_GE(parameter_position.begin, 1);  // Path must start with '/'.