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

components/search_engines/template_url.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/search_engines/template_url.h"
 
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
@@ skipping 78 matching lines @@
     return true;
   std::string encoded_original_query;
   if (!base::UTF16ToCodepage(original_query, encoding,
       base::OnStringConversionError::SKIP, &encoded_original_query))
     return false;
   *escaped_original_query = base::UTF8ToUTF16(
       net::EscapeQueryParamValue(encoded_original_query, true));
   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.
 bool FindSearchTermsInPath(const std::string& path,
                            url::Component* parameter_position) {
   DCHECK(parameter_position);
   parameter_position->reset();
   const size_t begin = path.find(kSearchTermsParameterFullEscaped);
   if (begin == std::string::npos)
     return false;
@@ skipping 380 matching lines @@
           key_found = true;
           position = value;
         }
       }
     }
     if (!key_found)
       return false;
   }
 
   // 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;
 }
 
 void TemplateURLRef::InvalidateCachedValues() const {
   supports_replacements_ = valid_ = parsed_ = false;
   host_.clear();
   port_.clear();
   path_.clear();
   search_term_key_.clear();
   search_term_position_in_path_ = std::string::npos;
@@ skipping 241 matching lines @@
       &url_string, 0, "{google:baseURL}",
       search_terms_data.GoogleBaseURLValue());
   base::ReplaceSubstringsAfterOffset(
       &url_string, 0, "{google:baseSuggestURL}",
       search_terms_data.GoogleBaseSuggestURLValue());
 
   GURL url(url_string);
   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.
 
   host_ = url.host();
   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 '/'.
     search_term_key_location_ = url::Parsed::PATH;
     search_term_position_in_path_ = parameter_position.begin;
     // Remove the "{searchTerms}" itself from |path_|.
     path_.erase(parameter_position.begin, parameter_position.len);
   }
 }
 
@@ skipping 683 matching lines @@
       // patterns. This means that given patterns
       //    [ "http://foo/#q={searchTerms}", "http://foo/?q={searchTerms}" ],
       // calling ExtractSearchTermsFromURL() on "http://foo/?q=bar#q=' would
       // return false. This is important for at least Google, where such URLs
       // are invalid.
       return !search_terms->empty();
     }
   }
   return false;
 }