Omnibox: Create Keyword Verbatim Result in Search Provider

chrome/browser/autocomplete/search_provider_unittest.cc

 // Copyright 2012 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 "chrome/browser/autocomplete/search_provider.h"
 
 #include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
 #include "base/string_util.h"
 #include "base/time.h"
@@ skipping 50 matching lines @@
 
   static void SetUpTestCase();
 
   static void TearDownTestCase();
 
   // See description above class for what this registers.
   virtual void SetUp();
 
   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.
   GURL AddSearchToHistory(TemplateURL* t_url, string16 term, int visit_count);
 
   // Looks for a match in |provider_| with |contents| equal to |contents|.
   // Sets |match| to it if found.  Returns whether |match| was set.
   bool FindMatchWithContents(const string16& contents,
                              AutocompleteMatch* match);
 
@@ skipping 84 matching lines @@
       &profile_, &TemplateURLServiceFactory::BuildInstanceFor);
 
   TemplateURLService* turl_model =
       TemplateURLServiceFactory::GetForProfile(&profile_);
 
   turl_model->Load();
 
   // 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_);
   TemplateURLID default_provider_id = default_t_url_->id();
   ASSERT_NE(0, default_provider_id);
 
   // Add url1, with search term term1_.
   term1_url_ = AddSearchToHistory(default_t_url_, term1_, 1);
 
   // Create another TemplateURL.
@@ skipping 76 matching lines @@
       wyt_match));
 }
 
 void SearchProviderTest::TearDown() {
   message_loop_.RunUntilIdle();
 
   // Shutdown the provider before the profile.
   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) {
   HistoryService* history =
       HistoryServiceFactory::GetForProfile(&profile_,
                                            Profile::EXPLICIT_ACCESS);
   GURL search(t_url->url_ref().ReplaceSearchTerms(
       TemplateURLRef::SearchTermsArgs(term)));
   static base::Time last_added_time;
   last_added_time = std::max(base::Time::Now(),
@@ skipping 497 matching lines @@
   ASSERT_NO_FATAL_FAILURE(QueryForInputAndSetWYTMatch(ASCIIToUTF16("f"),
                                                       &wyt_match));
   ASSERT_EQ(2u, provider_->matches().size());
   AutocompleteMatch term_match;
   EXPECT_TRUE(FindMatchWithDestination(term_url, &term_match));
   EXPECT_GT(term_match.relevance, wyt_match.relevance);
   EXPECT_EQ(1u, term_match.inline_autocomplete_offset);
   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();
 
   AutocompleteController controller(&profile_, NULL,
       AutocompleteProvider::TYPE_SEARCH);
   controller.Start(AutocompleteInput(
       ASCIIToUTF16("k t"), string16::npos, string16(), false, false, true,
       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);
 
-  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);
+  // 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(1).description.empty());
-  EXPECT_NE(result.match_at(0).description, result.match_at(1).description);
+  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.
+  };
+
+  // Test not in keyword mode.
+  RunTest<GURL>(url_cases, arraysize(url_cases), false,
+                &AutocompleteMatch::destination_url);
+
+  // 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.
 // Also verifies that just the *first* navigational result is listed as a match
 // if suggested relevance scores were not sent.
 TEST_F(SearchProviderTest, NavSuggestNoSuggestedRelevanceScores) {
   QueryForInput(ASCIIToUTF16("a.c"), string16(), false);
 
   // Make sure the default providers suggest service was queried.
   net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID(
@@ skipping 594 matching lines @@
   EXPECT_EQ(AutocompleteMatch::ACMatchClassification::URL,
             match.contents_class[0].style);
   EXPECT_EQ(4U, match.contents_class[1].offset);
   EXPECT_EQ(AutocompleteMatch::ACMatchClassification::URL |
             AutocompleteMatch::ACMatchClassification::MATCH,
             match.contents_class[1].style);
   EXPECT_EQ(5U, match.contents_class[2].offset);
   EXPECT_EQ(AutocompleteMatch::ACMatchClassification::URL,
             match.contents_class[2].style);
 }