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..f6309f5373ae1d22e1b0a9c7f811b0c5de1c2441 100644
--- a/components/search_engines/template_url_unittest.cc
+++ b/components/search_engines/template_url_unittest.cc
@@ -1868,3 +1868,47 @@ TEST_F(TemplateURLTest, InvalidateCachedValues) {
 
   search_terms_data_.set_google_base_url("http://www.google.com/");
 }
+
+// Test that TemplateURL object created with settings for google engine
+// matches its TemplateURLData.
+TEST_F(TemplateURLTest, MatchesData) {
+  TemplateURLData data;
+  data.SetURL("{google:baseURL}search?q={searchTerms}");
+  data.SetShortName(ASCIIToUTF16("Google"));
+  data.SetKeyword(ASCIIToUTF16("google.com"));
+  data.suggestions_url = "{google:baseSuggestURL}search?q={searchTerms}";
+  data.instant_url = "{google:baseURL}webhp";
+  data.image_url = "{google:baseURL}searchbyimage/upload";
+  data.new_tab_url = "{google:baseURL}_/chrome/newtab";
+  data.contextual_search_url = "{google:baseURL}_/contextualsearch";
+  data.image_url_post_params = "encoded_image={google:imageThumbnail}";
+  data.alternate_urls.push_back("{google:baseURL}s#q={searchTerms}");
+  // search_terms_replacement_key with value of
+  // "{google:instantExtendedEnabledKey}" is replaced inside TemplateUrl
+  // constructor so must be handled specially inside MatchesData.

[Peter Kasting, 2017/02/13 22:24:57]

Nit: If this is the specific bit you want to test, why not just test that?:

12345678901234567890123456789012345678901234567890123456789012345678901234567890
// Checks that TemplateURL::MatchesData() correctly handles a search term
// replacement key of kGoogleInstantExtendedEnabledKeyFull, which gets
// transformed in TemplateURL's constructor.
TEST_F(TemplateURLTest, MatchesData) {
  TemplateURLData data;
  // Note: This requires exposing this constant in template_url.h for tests
  data.search_terms_replacement_key =
      TemplateURL::kGoogleInstantExtendedEnabledKeyFull;
  TemplateURL url(data);
  EXPECT_NE(TemplateURL::kGoogleInstantExtendedEnabledKeyFull,
            url.search_terms_replacement_key());
  EXPECT_TRUE(TemplateURL::MatchesData(&url, &data, search_terms_data_));
}

[Alexander Yashkin, 2017/02/14 12:04:43]

????
Are this digits relevant to your comment?
Initially I wanted to check matching of all fields for google TemplateURLData in
case of any other strange substitutions in constructor in feature. 

But since probability of shooting the same leg is small, I do not object, Done.

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

Sorry, that's how I mark out 80 columns so I can ensure code samples I write in
here can be used.  I forgot to delete it.

+  data.search_terms_replacement_key = "{google:instantExtendedEnabledKey}";
+  TemplateURL url(data);
+  EXPECT_TRUE(TemplateURL::MatchesData(&url, &data, search_terms_data_));
+}
+
+// Test for correct replacement of GoogleInstantExtendedEnabledKey param.
+TEST_F(TemplateURLTest, GoogleInstantExtendedEnabledReplacement) {

[Peter Kasting, 2017/02/13 22:24:57]

This test feels like a change-detector test: it will break if we change the
values of these params (at which point we'll have to fix the test), and it won't
actually catch real bugs.

I would remove it, unless there's something I'm missing about why it's important
to check the specific strings these keys map to.

[Alexander Yashkin, 2017/02/14 12:04:43]

I have added it after your comment in
https://codereview.chromium.org/2524733008:

"Honestly I can't conceive mentally of which thing should be used when.  I'm
going to assume you checked carefully and did the right thing in each place.
Can there be a test that would have caught this bug, so we don't make the same
mistake again later?"

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

Fair enough.

+  TemplateURLData data;
+  data.SetURL(
+      "{google:baseURL}search/"
+      "?{google:instantExtendedEnabledKey}&q={searchTerms}");
+  data.SetShortName(ASCIIToUTF16("Google"));
+  data.SetKeyword(ASCIIToUTF16("google.com"));
+  data.search_terms_replacement_key = "{google:instantExtendedEnabledKey}";
+  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("espv", 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());
+  EXPECT_TRUE(turl.HasSearchTermsReplacementKey(search_generated));
+}