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..f32bb51cc2c77a91989278f48ff96fae511686aa 100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -787,8 +787,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

[msw, 2013/01/31 00:38:16]

I would really like to see some additional tests here regarding the conditions
to trigger the keyword verbatim result, as well as the content of the keyword
verbatim match itself (check the handling of spaces the same way
KeywordProviderTest does).

[Mark P, 2013/01/31 23:09:27]

I'll do this soon.  Right now I'll hit reply to see if you have more comments on
the rest of this changelist.

+// 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 +801,32 @@ 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());
-
-  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);
-
+  // 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);

[msw, 2013/01/31 00:38:16]

nit: isn't this scoring guaranteed by the match ordering? (redundant checks?)

[Mark P, 2013/01/31 23:09:27]

Not as far as I know.  The controller resorts matches it receives; I don't think
there's any policy that controllers must return matches in sorted order.

[msw, 2013/02/01 20:52:37]

Okay, then this is good, thanks.

+  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(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);
+

[msw, 2013/01/31 00:38:16]

nit: remove extra line.

[Mark P, 2013/01/31 23:09:27]

Done.

 }
 
 // Verifies Navsuggest results don't set a TemplateURL, which instant relies on.