Rewrite FindSearchTermsInPath() for consistency with FindSearchTermsKey().

Rewrite FindSearchTermsInPath() for consistency with FindSearchTermsKey().

R=pkasting@chromium.org
BUG=None

[Vitaly Baranov, 2016-05-30 13:08:14]

On 2016/05/30 12:44:01, Vitaly Baranov wrote:

This CL contains refactoring only. I think it makes the code clearer.
It's a piece of https://codereview.chromium.org/1978553002/

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

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

https://codereview.chromium.org/2021773003/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:171: return result;
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);
  }
}

[Peter Kasting, 2016-08-20 02:45:24]

What's the status of this CL?  Looks like there was no response to my review
comments.

[Peter Kasting, 2017-02-11 02:04:21]

On 2016/08/20 02:45:24, Peter Kasting wrote:
> What's the status of this CL?  Looks like there was no response to my review
> comments.

Still no response.  Any chance of implementing those comments?  If there is
still no response next time I check this I'll close it, but I wouldn't mind
getting the simplification proposed there in.

[Vitaly Baranov, 2017-02-22 20:27:08]

On 2017/02/11 02:04:21, Peter Kasting wrote:
> On 2016/08/20 02:45:24, Peter Kasting wrote:
> > What's the status of this CL?  Looks like there was no response to my review
> > comments.
> 
> Still no response.  Any chance of implementing those comments?  If there is
> still no response next time I check this I'll close it, but I wouldn't mind
> getting the simplification proposed there in.

I'm going to finish this CL, sorry for the long wait.

[Vitaly Baranov, 2017-05-12 15:11:52]

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

https://codereview.chromium.org/2021773003/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:171: return result;
On 2016/05/31 08:07:24, Peter Kasting wrote:
> 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);
>   }
> }

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.