Rewrite FindSearchTermsInPath() for consistency with FindSearchTermsKey().

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 2a1cb2109533d7cead63dc5a25e9faf54a088da7..bafc4fd3e64c9af23330d620d17525d15cdee8ac 100644
--- a/components/search_engines/template_url.cc
+++ b/components/search_engines/template_url.cc
@@ -141,17 +141,34 @@ SearchTermsKeyResult FindSearchTermsKey(const std::string& params) {
   return result;
 }
 
+struct SearchTermsInPathResult {
+  std::string path;
+  size_t search_terms_position;
+  SearchTermsInPathResult()
+      : search_terms_position(std::string::npos) {
+  }
+  bool found() const {
+    return search_terms_position != std::string::npos;
+  }
+};
+
 // 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;
-  parameter_position->begin = begin;
-  parameter_position->len = arraysize(kSearchTermsParameterFullEscaped) - 1;
-  return true;
+SearchTermsInPathResult FindSearchTermsInPath(const std::string& path) {
+  DCHECK(base::StartsWith(path, "/", base::CompareCase::SENSITIVE));
+  std::string result_path = path;
+  const base::StringPiece search_terms_parameter(
+      kSearchTermsParameterFullEscaped);
+  const size_t search_terms_position =
+      base::StringPiece(result_path).find(search_terms_parameter);
+  if (search_terms_position != std::string::npos) {
+    DCHECK_GE(search_terms_position, 1U);  // Path must starts with '/'.
+    // Remove the "{searchTerms}" itself from |result_path|.
+    result_path.erase(search_terms_position, search_terms_parameter.length());
+  }
+  SearchTermsInPathResult result;
+  result.path = result_path;
+  result.search_terms_position = search_terms_position;
+  return result;

[Peter Kasting, 2016/05/31 08:07:24]

This just seems to make the code longer and add a new struct type, without
gaining any safety or clarity I can see.

I would go the other way instead and simplify.  You can remove this function
entirely and inline its functionality into the lone caller, in the process
removing unnecessary cruft, as a result leaving the caller code basically the
same length it is today but (hopefully) even clearer.

  const auto query_result = FindSearchTermsKey(url.query());
  const auto ref_result = FindSearchTermsKey(url.ref());
  const size_t position_in_path = path.find(kSearchTermsParameterFullEscaped);
  const bool in_query = query_result.found();
  const bool in_ref = ref_result.found();
  const bool in_path = position_in_path != std::string::npos;
  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) {
    ...
  } else if (in_ref) {
    ...
  } else {
    DCHECK_GE(position_in_path, 1);  // Path must start with '/'.
    search_term_key_location_ = url::Parsed::PATH;
    search_term_position_in_path_ = position_in_path;
    // Remove the "{searchTerms}" itself from |path_|.
    path_.erase(position_in_path,
                arraysize(kSearchTermsParameterFullEscaped) - 1);
  }
}

[Vitaly Baranov, 2017/05/12 15:11:51]

I'd like to propose another approach if you don't mind.
https://codereview.chromium.org/2877983002/
In that CL I made the code for the functionality shorter and clearer (I suppose
so). The total number of lines increased only because I added accessors for
search_terms_value_prefix_ and search_term_value_suffix_ in that CL to check
these fields in a unit test.

 }
 
 bool IsTemplateParameterString(const std::string& param) {
@@ -819,16 +836,16 @@ void TemplateURLRef::ParseHostAndSearchTermKey(
 
   auto query_result = FindSearchTermsKey(url.query());
   auto ref_result = FindSearchTermsKey(url.ref());
-  url::Component parameter_position;
+  auto path_result = FindSearchTermsInPath(url.path());
   const bool in_query = query_result.found();
   const bool in_ref = ref_result.found();
-  const bool in_path = FindSearchTermsInPath(url.path(), &parameter_position);
+  const bool in_path = path_result.found();
   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();
+  path_ = path_result.path;
   if (in_query) {
     search_term_key_ = query_result.key;
     search_term_key_location_ = url::Parsed::QUERY;
@@ -841,11 +858,8 @@ void TemplateURLRef::ParseHostAndSearchTermKey(
     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);
+    search_term_position_in_path_ = path_result.search_terms_position;
   }
 }