Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)

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 0a457d1632c1795317c6e6722f186e6e48ce5dbc..90bf70d788b7c33e61e6dd375cc4d8449d559717 100644
--- a/components/search_engines/template_url.cc
+++ b/components/search_engines/template_url.cc
@@ -67,6 +67,11 @@ 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
@@ -160,6 +165,20 @@ bool IsTemplateParameterString(const std::string& param) {
       (*(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
 
 
@@ -605,7 +624,7 @@ bool TemplateURLRef::ParseParameter(size_t 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));
@@ -1159,9 +1178,8 @@ TemplateURL::TemplateURL(const TemplateURLData& data, Type type)
   SetPrepopulateId(data_.prepopulate_id);
 
   if (data_.search_terms_replacement_key ==
-      "{google:instantExtendedEnabledKey}") {
+      kGoogleInstantExtendedEnabledKeyFull)
     data_.search_terms_replacement_key = google_util::kInstantExtendedAPIParam;
-  }
 }
 
 TemplateURL::~TemplateURL() {
@@ -1206,23 +1224,23 @@ bool TemplateURL::MatchesData(const TemplateURL* t_url,
     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 {