Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)

components/search_engines/template_url_unittest.cc

Index: components/search_engines/template_url_unittest.cc
diff --git a/components/search_engines/template_url_unittest.cc b/components/search_engines/template_url_unittest.cc
index 47630f348925dd2bb0dcf7e096f5724ea6f76dd3..b13b273506335bed8673a5f3289341dd1f516531 100644
--- a/components/search_engines/template_url_unittest.cc
+++ b/components/search_engines/template_url_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "components/google/core/browser/google_util.h"
 #include "components/metrics/proto/omnibox_event.pb.h"
 #include "components/metrics/proto/omnibox_input_type.pb.h"
 #include "components/search_engines/search_engines_switches.h"
@@ -1868,3 +1869,42 @@ TEST_F(TemplateURLTest, InvalidateCachedValues) {
 
   search_terms_data_.set_google_base_url("http://www.google.com/");
 }
+
+// search_terms_replacement_key param of TemplateURLData with value of
+// "{google:instantExtendedEnabledKey}" is replaced inside TemplateUrl
+// constructor so must be handled specially inside MatchesData.
+// Test that TemplateURL object created with such param matches correctly its
+// TemplateURLData.
+TEST_F(TemplateURLTest, MatchesData) {
+  TemplateURLData data;
+  data.search_terms_replacement_key =
+      google_util::kGoogleInstantExtendedEnabledKeyFull;
+  TemplateURL url(data);
+  EXPECT_NE(google_util::kGoogleInstantExtendedEnabledKeyFull,
+            url.search_terms_replacement_key());
+  EXPECT_TRUE(TemplateURL::MatchesData(&url, &data, search_terms_data_));
+}
+
+// Test for correct replacement of GoogleInstantExtendedEnabledKey param.
+TEST_F(TemplateURLTest, GoogleInstantExtendedEnabledReplacement) {
+  TemplateURLData data;
+  data.SetURL(
+      "{google:baseURL}search/"
+      "?{google:instantExtendedEnabledKey}&q={searchTerms}");

[Peter Kasting, 2017/02/15 02:09:57]

Nit: I would trim the google:baseURL bit out of this and replace it with
something static.  No need to test that, and it could make the test flaky if the
base URL is computed differently in another locale.  (But see also comment
below, which would moot that second concern a bit.)

I would probably also string-concatenate in kGoogleInstantExtendedEnabledKeyFull
here instead of retyping it.

[Alexander Yashkin, 2017/02/15 12:46:10]

Done

[Peter Kasting, 2017/02/16 00:12:10]

Looks like google:baseURL is still around and could be replaced.

[Alexander Yashkin, 2017/02/16 07:26:30]

Sorry, missed that, replaced with google.com.

+  data.SetShortName(ASCIIToUTF16("Google"));
+  data.SetKeyword(ASCIIToUTF16("google.com"));
+  data.search_terms_replacement_key =
+      google_util::kGoogleInstantExtendedEnabledKeyFull;
+  TemplateURL turl(data);
+  EXPECT_TRUE(TemplateURL::MatchesData(&turl, &data, search_terms_data_));
+  // Expect that replacement of search_terms_replacement_key in TemplateURL
+  // constructor is correct.
+  EXPECT_EQ(google_util::kInstantExtendedAPIParam,
+            turl.search_terms_replacement_key());
+  // Expect that replacement of {google:instantExtendedEnabledKey} in search url
+  // is correct.
+  GURL search_generated = turl.GenerateSearchURL(search_terms_data_);
+  EXPECT_EQ("http://www.google.com/search/?espv&q=blah.blah.blah.blah.blah",
+            search_generated.spec());

[Peter Kasting, 2017/02/15 02:09:57]

Rather than string-match the spec, What about using a net::QueryIterator to grab
the first query param off the GURL and checking that the key is
google_util::kInstantExtendedAPIParam?

This seems more targeted and less potentially flaky to e.g. changes in the
default query string.

[Alexander Yashkin, 2017/02/15 12:46:10]

Done

+  EXPECT_TRUE(turl.HasSearchTermsReplacementKey(search_generated));
+}