Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)

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 49 matching lines @@
 // Display value for kGoogleUnescapedSearchTermsParameter.
 const char kDisplayUnescapedSearchTerms[] = "%S";
 
 // Used if the count parameter is not optional. Indicates we want 10 search
 // results.
 const char kDefaultCount[] = "10";
 
 // Used if the output encoding parameter is required.
 const char kOutputEncodingType[] = "UTF-8";
 
+constexpr char kGoogleInstantExtendedEnabledKey[] =
+    "google:instantExtendedEnabledKey";
+constexpr char kGoogleInstantExtendedEnabledKeyFull[] =
+    "{google:instantExtendedEnabledKey}";
+
 // Attempts to encode |terms| and |original_query| in |encoding| and escape
 // them.  |terms| may be escaped as path or query depending on |is_in_query|;
 // |original_query| is always escaped as query.  Returns whether the encoding
 // process succeeded.
 bool TryEncoding(const base::string16& terms,
                  const base::string16& original_query,
                  const char* encoding,
                  bool is_in_query,
                  base::string16* escaped_terms,
                  base::string16* escaped_original_query) {
@@ skipping 73 matching lines @@
   parameter_position->begin = begin;
   parameter_position->len = arraysize(kSearchTermsParameterFullEscaped) - 1;
   return true;
 }
 
 bool IsTemplateParameterString(const std::string& param) {
   return (param.length() > 2) && (*(param.begin()) == kStartParameter) &&
       (*(param.rbegin()) == kEndParameter);
 }
 
+// Special case for search_terms_replacement_key comparison, because of
+// its special initialization in TemplateUrl constructor.
+bool SearchTermsReplacementKeysMatch(
+    const std::string& search_terms_replacement_key1,
+    const std::string& search_terms_replacement_key2) {
+  if (search_terms_replacement_key1 == google_util::kInstantExtendedAPIParam &&
+      search_terms_replacement_key2 == kGoogleInstantExtendedEnabledKeyFull)
+    return true;
+  if (search_terms_replacement_key2 == google_util::kInstantExtendedAPIParam &&
+      search_terms_replacement_key1 == kGoogleInstantExtendedEnabledKeyFull)
+    return true;
+  return search_terms_replacement_key1 == search_terms_replacement_key2;

[Peter Kasting, 2017/02/02 00:04:26]

Nit: This implementation is shorter and does the common-case check first, which
as I recall you liked.

I renamed the params since they didn't seem less clear and it cut the length
noticeably.  With the old names, this implementation is the same number of lines
as your version.

bool SearchTermsReplacementKeysMatch(const std::string& key1,
                                     const std::string& key2) {
  const auto IsInstantExtended = [](const std::string& key) {
    return (key == google_util::kInstantExtendedAPIParam) ||
           (key == kGoogleInstantExtendedEnabledKeyFull);
  }
  return (key1 == key2) || (IsInstantExtended(key1) && IsInstantExtended(key2));
}

[Alexander Yashkin, 2017/02/03 14:44:25]

Clever idea, done.

+}
+
 }  // namespace
 
 
 // TemplateURLRef::SearchTermsArgs --------------------------------------------
 
 TemplateURLRef::SearchTermsArgs::SearchTermsArgs(
     const base::string16& search_terms)
     : search_terms(search_terms),
       input_type(metrics::OmniboxInputType::INVALID),
       accepted_suggestion(NO_SUGGESTIONS_AVAILABLE),
@@ skipping 425 matching lines @@
         Replacement(TemplateURLRef::GOOGLE_IMAGE_THUMBNAIL, start));
   } else if (parameter == "google:imageURL") {
     replacements->push_back(Replacement(TemplateURLRef::GOOGLE_IMAGE_URL,
                                         start));
   } else if (parameter == "google:inputType") {
     replacements->push_back(Replacement(TemplateURLRef::GOOGLE_INPUT_TYPE,
                                         start));
   } else if (parameter == "google:instantExtendedEnabledParameter") {
     replacements->push_back(Replacement(GOOGLE_INSTANT_EXTENDED_ENABLED,
                                         start));
-  } else if (parameter == "google:instantExtendedEnabledKey") {
+  } else if (parameter == kGoogleInstantExtendedEnabledKey) {
     url->insert(start, google_util::kInstantExtendedAPIParam);
   } else if (parameter == "google:iOSSearchLanguage") {
     replacements->push_back(Replacement(GOOGLE_IOS_SEARCH_LANGUAGE, start));
   } else if (parameter == "google:contextualSearchVersion") {
     replacements->push_back(
         Replacement(GOOGLE_CONTEXTUAL_SEARCH_VERSION, start));
   } else if (parameter == "google:contextualSearchContextData") {
     replacements->push_back(
         Replacement(GOOGLE_CONTEXTUAL_SEARCH_CONTEXT_DATA, start));
   } else if (parameter == "google:originalQueryForSuggestion") {
@@ skipping 533 matching lines @@
       instant_url_ref_(this, TemplateURLRef::INSTANT),
       image_url_ref_(this, TemplateURLRef::IMAGE),
       new_tab_url_ref_(this, TemplateURLRef::NEW_TAB),
       contextual_search_url_ref_(this, TemplateURLRef::CONTEXTUAL_SEARCH),
       type_(type),
       engine_type_(SEARCH_ENGINE_UNKNOWN) {
   ResizeURLRefVector();
   SetPrepopulateId(data_.prepopulate_id);
 
   if (data_.search_terms_replacement_key ==
-      "{google:instantExtendedEnabledKey}") {
+      kGoogleInstantExtendedEnabledKeyFull)
     data_.search_terms_replacement_key = google_util::kInstantExtendedAPIParam;
-  }
 }
 
 TemplateURL::~TemplateURL() {
 }
 
 // static
 base::string16 TemplateURL::GenerateKeyword(const GURL& url) {
   DCHECK(url.is_valid());
   // Strip "www." off the front of the keyword; otherwise the keyword won't work
   // properly.  See http://code.google.com/p/chromium/issues/detail?id=6984 .
@@ skipping 24 matching lines @@
 }
 
 // static
 bool TemplateURL::MatchesData(const TemplateURL* t_url,
                               const TemplateURLData* data,
                               const SearchTermsData& search_terms_data) {
   if (!t_url || !data)
     return !t_url && !data;
 
   return (t_url->short_name() == data->short_name()) &&
-      t_url->HasSameKeywordAs(*data, search_terms_data) &&
-      (t_url->url() == data->url()) &&
-      (t_url->suggestions_url() == data->suggestions_url) &&
-      (t_url->instant_url() == data->instant_url) &&
-      (t_url->image_url() == data->image_url) &&
-      (t_url->new_tab_url() == data->new_tab_url) &&
-      (t_url->search_url_post_params() == data->search_url_post_params) &&
-      (t_url->suggestions_url_post_params() ==
+         t_url->HasSameKeywordAs(*data, search_terms_data) &&
+         (t_url->url() == data->url()) &&
+         (t_url->suggestions_url() == data->suggestions_url) &&
+         (t_url->instant_url() == data->instant_url) &&
+         (t_url->image_url() == data->image_url) &&
+         (t_url->new_tab_url() == data->new_tab_url) &&
+         (t_url->search_url_post_params() == data->search_url_post_params) &&
+         (t_url->suggestions_url_post_params() ==
           data->suggestions_url_post_params) &&
-      (t_url->instant_url_post_params() == data->instant_url_post_params) &&
-      (t_url->image_url_post_params() == data->image_url_post_params) &&
-      (t_url->favicon_url() == data->favicon_url) &&
-      (t_url->safe_for_autoreplace() == data->safe_for_autoreplace) &&
-      (t_url->input_encodings() == data->input_encodings) &&
-      (t_url->alternate_urls() == data->alternate_urls) &&
-      (t_url->search_terms_replacement_key() ==
-          data->search_terms_replacement_key);
+         (t_url->instant_url_post_params() == data->instant_url_post_params) &&
+         (t_url->image_url_post_params() == data->image_url_post_params) &&
+         (t_url->favicon_url() == data->favicon_url) &&
+         (t_url->safe_for_autoreplace() == data->safe_for_autoreplace) &&
+         (t_url->input_encodings() == data->input_encodings) &&
+         (t_url->alternate_urls() == data->alternate_urls) &&
+         SearchTermsReplacementKeysMatch(t_url->search_terms_replacement_key(),
+                                         data->search_terms_replacement_key);
 }
 
 base::string16 TemplateURL::AdjustedShortNameForLocaleDirection() const {
   base::string16 bidi_safe_short_name = data_.short_name();
   base::i18n::AdjustStringForLocaleDirection(&bidi_safe_short_name);
   return bidi_safe_short_name;
 }
 
 bool TemplateURL::SupportsReplacement(
     const SearchTermsData& search_terms_data) const {
@@ skipping 254 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;
 }