Omnibox: Create Keyword Verbatim Result in Search Provider

chrome/browser/autocomplete/search_provider_unittest.cc

Index: chrome/browser/autocomplete/search_provider_unittest.cc
diff --git a/chrome/browser/autocomplete/search_provider_unittest.cc b/chrome/browser/autocomplete/search_provider_unittest.cc
index 8b9735c6208c5e0576c7ec2521024f98af74ff48..9eb37685c20104655c7e2738d31db2c51483fd06 100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -68,6 +68,19 @@ class SearchProviderTest : public testing::Test,
 
   virtual void TearDown();
 
+  template<class ResultType>
+  struct test_data {
+    const string16 input;
+    const size_t num_results;
+    const ResultType output[3];
+  };
+
+  template<class ResultType>
+  void RunTest(test_data<ResultType>* cases,
+               int num_cases,
+               bool prefer_keyword,
+               ResultType AutocompleteMatch::* member);
+
  protected:
   // Adds a search for |term|, using the engine |t_url| to the history, and
   // returns the URL for that search.
@@ -172,8 +185,8 @@ void SearchProviderTest::SetUp() {
   // Reset the default TemplateURL.
   TemplateURLData data;
   data.short_name = ASCIIToUTF16("t");
-  data.SetURL("http://defaultturl/{searchTerms}");
-  data.suggestions_url = "http://defaultturl2/{searchTerms}";
+  data.SetURL("http://defaulturl/{searchTerms}");

[msw, 2013/02/02 02:27:52]

The extra 't' stands for 'template', but this is ok if the test still passes.

[Mark P, 2013/02/04 19:46:09]

Okay.  I thought it was a typo.  Put back, and tweaked the test that I had that
depends on it.

+  data.suggestions_url = "http://defaulturl2/{searchTerms}";
   default_t_url_ = new TemplateURL(&profile_, data);
   turl_model->Add(default_t_url_);
   turl_model->SetDefaultSearchProvider(default_t_url_);
@@ -270,6 +283,30 @@ void SearchProviderTest::TearDown() {
   provider_ = NULL;
 }
 
+template<class ResultType>
+void SearchProviderTest::RunTest(
+    test_data<ResultType>* cases,
+    int num_cases,
+    bool prefer_keyword,
+    ResultType AutocompleteMatch::* member) {

[msw, 2013/02/02 02:27:52]

nit: I'm unfamiliar with this syntax, but others seem to omit the space after *

[Mark P, 2013/02/04 19:46:09]

Left as is.  I copied this format from keyword provider's test, so I'm sticking
with the local autocomplete style for this.

+  ACMatches matches;
+  for (int i = 0; i < num_cases; ++i) {
+    AutocompleteInput input(cases[i].input, string16::npos, string16(),
+                            false, prefer_keyword, true,
+                            AutocompleteInput::ALL_MATCHES);
+    provider_->Start(input, false);
+    matches = provider_->matches();
+    EXPECT_EQ(cases[i].num_results, matches.size()) <<
+                ASCIIToUTF16("Input was: ") + cases[i].input;
+    if (matches.size() == cases[i].num_results) {

[msw, 2013/02/02 02:27:52]

optional nit: it's okay to ignore this since you check it above.

[Mark P, 2013/02/04 19:46:09]

Left as is.  It's not okay because cases could be longer and then the test would
segfault.  I guess you'd still be notified but then the other parts of the test
wouldn't run.

+      for (size_t j = 0; j < cases[i].num_results; ++j) {
+        EXPECT_EQ(cases[i].output[j], matches[j].*member) <<
+            ASCIIToUTF16("Input was: ") + cases[i].input;
+      }
+    }
+  }
+}
+
 GURL SearchProviderTest::AddSearchToHistory(TemplateURL* t_url,
                                             string16 term,
                                             int visit_count) {
@@ -787,8 +824,9 @@ TEST_F(SearchProviderTest, InlineMixedCaseMatches) {
   EXPECT_EQ(ASCIIToUTF16("FOO"), term_match.fill_into_edit);
 }
 
-// Verifies AutocompleteControllers sets descriptions for results correctly.
-TEST_F(SearchProviderTest, UpdateKeywordDescriptions) {
+// Verifies AutocompleteControllers return results (including keyword
+// results) in the right order and set descriptions for them correctly.
+TEST_F(SearchProviderTest, KeywordOrderingAndDescriptions) {
   // Add an entry that corresponds to a keyword search with 'term2'.
   AddSearchToHistory(keyword_t_url_, ASCIIToUTF16("term2"), 1);
   profile_.BlockUntilHistoryProcessesPendingRequests();
@@ -800,16 +838,84 @@ TEST_F(SearchProviderTest, UpdateKeywordDescriptions) {
       AutocompleteInput::ALL_MATCHES));
   const AutocompleteResult& result = controller.result();
 
-  // There should be two matches, one for the keyword one for what you typed.
-  ASSERT_EQ(2u, result.size());
+  // There should be three matches, one for the keyword history, one for
+  // keyword provider's what-you-typed, and one for the default provider's
+  // what you typed, in that order.
+  ASSERT_EQ(3u, result.size());
+  EXPECT_EQ(AutocompleteMatch::SEARCH_HISTORY, result.match_at(0).type);
+  EXPECT_EQ(AutocompleteMatch::SEARCH_OTHER_ENGINE, result.match_at(1).type);
+  EXPECT_EQ(AutocompleteMatch::SEARCH_WHAT_YOU_TYPED, result.match_at(2).type);
+  EXPECT_GT(result.match_at(0).relevance, result.match_at(1).relevance);
+  EXPECT_GT(result.match_at(1).relevance, result.match_at(2).relevance);
+
+  // The two keyword results should come with the keyword we expect.
+  EXPECT_EQ(ASCIIToUTF16("k"), result.match_at(0).keyword);
+  EXPECT_EQ(ASCIIToUTF16("k"), result.match_at(1).keyword);
+  // The default provider has a different keyword.  (We don't explicitly
+  // set it during this test, so all we do is assert that it's different.)
+  EXPECT_NE(result.match_at(0).keyword, result.match_at(2).keyword);
+
+  // The top result will always have a description.  The third result,
+  // coming from a different provider than the first two, should also.
+  // Whether the second result has one doesn't matter much.  (If it was
+  // missing, people would infer that it's the same search provider as
+  // the one above it.)
+  EXPECT_FALSE(result.match_at(0).description.empty());
+  EXPECT_FALSE(result.match_at(2).description.empty());
+  EXPECT_NE(result.match_at(0).description, result.match_at(2).description);
+}
+
+TEST_F(SearchProviderTest, VerbatimURLs) {
+  test_data<GURL> url_cases[] = {
+    // Test a simple keyword input.
+    {ASCIIToUTF16("k foo"), 2, {GURL("http://keyword/foo"),

[msw, 2013/02/02 02:27:52]

nit: spaces after '{' here and elsewhere

[Mark P, 2013/02/04 19:46:09]

Done.

+                                GURL("http://defaulturl/k%20foo")}},
+
+    // Make sure extra whitespace after the keyword doesn't change the

[msw, 2013/02/02 02:27:52]

What about whitespace before the keyword?

[Mark P, 2013/02/04 19:46:09]

Commented.

+    // keyword verbatim query.
+    {ASCIIToUTF16("k   foo"), 2, {GURL("http://keyword/foo"),
+                                  GURL("http://defaulturl/k%20%20%20foo")}},
+
+    // But whitespace elsewhere in the query string should matter to both
+    // matches.
+    {ASCIIToUTF16("k  foo  bar"), 2,
+        {GURL("http://keyword/foo%20%20bar"),
+         GURL("http://defaulturl/k%20%20foo%20%20bar")}},
+    // Note in the above test case we don't test trailing whitespace because
+    // SearchProvider still doesn't handle this well.  See related bugs:
+    // 102690, 99239, 164635.
+
+    // Keywords can be prefixed by certain things that should get ignored
+    // when constructing the keyword match.
+    {ASCIIToUTF16("www.k foo"),    2, {GURL("http://keyword/foo"),

[msw, 2013/02/02 02:27:52]

Definitely also check .com if that's applicable (or maybe that's just
tab-to-search and not keywords). Optionally also check http://, https://
http://www., etc.

[Mark P, 2013/02/04 19:46:09]

Added some tests.  (BTW, you cannot add a .com; normally that's part of the
keyword, not an ignorable extra piece.)

+                                       GURL("http://defaulturl/www.k%20foo")}},
+
+    // A keyword with no remaining input shouldn't get a keyword verbatim match.
+    {ASCIIToUTF16("k"),            1, {GURL("http://defaulturl/k")}},
+    {ASCIIToUTF16("k "),           1, {GURL("http://defaulturl/k%20")}}
+
+    // The fact that verbatim queries to keyword are handled by KeywordProvider
+    // not SearchProvider is tested in
+    // chrome/browser/extensions/api/omnibox/omnibox_apitest.cc.
+  };
 
-  EXPECT_FALSE(result.match_at(0).keyword.empty());
-  EXPECT_FALSE(result.match_at(1).keyword.empty());
-  EXPECT_NE(result.match_at(0).keyword, result.match_at(1).keyword);
+  // Test not in keyword mode.
+  RunTest<GURL>(url_cases, arraysize(url_cases), false,
+                &AutocompleteMatch::destination_url);
 
-  EXPECT_FALSE(result.match_at(0).description.empty());
-  EXPECT_FALSE(result.match_at(1).description.empty());
-  EXPECT_NE(result.match_at(0).description, result.match_at(1).description);
+  // Test in keyword mode.  (Both modes should give the same result.)
+  RunTest<GURL>(url_cases, arraysize(url_cases), true,
+                &AutocompleteMatch::destination_url);
+}
+
+// This is similar to the above test except it checks the fill_into_edit

[msw, 2013/02/02 02:27:52]

You could add these other types to the same TestData struct and check all three
at the same time; that would also alleviate templating, afaict.

[Mark P, 2013/02/04 19:46:09]

Done.

+// of the AutocompleteMatch, not the destination_url.
+TEST_F(SearchProviderTest, VerbatimFillIntoEdit) {
+}
+
+// This is similar to the above test except it checks the result_type
+// of the AutocompleteMatch, not the destination_url.
+TEST_F(SearchProviderTest, VerbatimResultTypes) {
 }
 
 // Verifies Navsuggest results don't set a TemplateURL, which instant relies on.