Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)

components/search_engines/template_url_unittest.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 <stddef.h>
 
 #include "base/base_paths.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #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"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/testing_search_terms_data.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ASCIIToUTF16;
 
@@ skipping 1838 matching lines @@
       GURL("https://www.foo.org/search?q=Y+Z"),
       search_terms_data_, &search_terms));
   EXPECT_EQ(base::ASCIIToUTF16("Y Z"), search_terms);
   EXPECT_TRUE(url.ExtractSearchTermsFromURL(
       GURL("https://www.foo.org/s#q=123"),
       search_terms_data_, &search_terms));
   EXPECT_EQ(base::ASCIIToUTF16("123"), search_terms);
 
   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));
+}