Omnibox: Prevent Asynchronous Suggestions from Changing Default Match

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 7f096da805225ebda9993f9d7bd90071777dd77d..1e19d34a3871ffbd4b24f77a29f98163f254e689 100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -139,6 +139,11 @@ class SearchProviderTest : public testing::Test,
     const ResultInfo output[3];
   };
 
+  struct ExpectedMatch {
+    std::string contents;
+    bool allowed_to_be_default_match;
+  };
+
   SearchProviderTest()
       : default_t_url_(NULL),
         term1_(ASCIIToUTF16("term1")),
@@ -158,8 +163,9 @@ class SearchProviderTest : public testing::Test,
   // Needed for AutocompleteFieldTrial::ActivateStaticTrials();
   scoped_ptr<base::FieldTrialList> field_trial_list_;
 
-  // Default value used for testing.
+  // Default values used for testing.
   static const std::string kNotApplicable;
+  static const ExpectedMatch kEmptyExpectedMatch;

[msw, 2014/08/27 18:44:39]

nit: use |kEmptyMatch| for less line wrapping with this.

[Mark P, 2014/08/27 23:08:19]

Can't do that without changing the name of countless kEmptyMatch local variables
for various tests below.

[msw, 2014/08/28 19:21:40]

Acknowledged.

 
   // Adds a search for |term|, using the engine |t_url| to the history, and
   // returns the URL for that search.
@@ -207,6 +213,14 @@ class SearchProviderTest : public testing::Test,
                              const std::string matches[3],
                              const std::string& error_description);
 
+  // Verifies that |matches| and |expected_matches| agree on the first
+  // |num_expected_matches|, displaying an error message that includes
+  // |description| for any disagreement.
+  void CheckMatches(const std::string& description,
+                    const size_t num_expected_matches,
+                    const ExpectedMatch expected_matches[],
+                    const ACMatches& matches);
+
   void ResetFieldTrialList();
 
   // Create a field trial, with ZeroSuggest activation based on |enabled|.
@@ -241,6 +255,8 @@ class SearchProviderTest : public testing::Test,
 
 // static
 const std::string SearchProviderTest::kNotApplicable = "Not Applicable";
+const SearchProviderTest::ExpectedMatch
+    SearchProviderTest::kEmptyExpectedMatch = { kNotApplicable, false };
 
 void SearchProviderTest::SetUp() {
   // Make sure that fetchers are automatically ungregistered upon destruction.
@@ -441,14 +457,19 @@ void SearchProviderTest::ForcedQueryTestHelper(
     const std::string& json,
     const std::string expected_matches[3],
     const std::string& error_description) {
-  QueryForInput(ASCIIToUTF16(input), false, false);
-  net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID(
-      SearchProvider::kDefaultProviderURLFetcherID);
-  ASSERT_TRUE(fetcher);
-  fetcher->set_response_code(200);
-  fetcher->SetResponseString(json);
-  fetcher->delegate()->OnURLFetchComplete(fetcher);
-  RunTillProviderDone();
+  // Send the query twice in order to have a synchronous pass after the first
+  // reply is received.  This is necessary because SearchProvider doesn't allow
+  // an asynchronous reply to change the default match.
+  for (size_t i = 0; i < 2; i++) {

[msw, 2014/08/27 18:44:38]

nit: ++i

[Mark P, 2014/08/27 23:08:19]

Done throughout.

+    QueryForInput(ASCIIToUTF16(input), false, false);
+    net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID(
+        SearchProvider::kDefaultProviderURLFetcherID);
+    ASSERT_TRUE(fetcher);
+    fetcher->set_response_code(200);
+    fetcher->SetResponseString(json);
+    fetcher->delegate()->OnURLFetchComplete(fetcher);
+    RunTillProviderDone();
+  }
 
   const ACMatches& matches = provider_->matches();
   ASSERT_LE(matches.size(), 3u);
@@ -465,6 +486,28 @@ void SearchProviderTest::ForcedQueryTestHelper(
   }
 }
 
+void SearchProviderTest::CheckMatches(const std::string& description,
+                                      const size_t num_expected_matches,
+                                      const ExpectedMatch expected_matches[],
+                                      const ACMatches& matches) {
+  ASSERT_FALSE(matches.empty());
+  ASSERT_LE(matches.size(), num_expected_matches);
+  size_t j = 0;

[msw, 2014/08/27 18:44:38]

nit: use |i|

[Mark P, 2014/08/27 23:08:20]

Done.  (refactoring error)

+  // Ensure that the returned matches equal the expectations.
+  for (; j < matches.size(); ++j) {
+    EXPECT_EQ(ASCIIToUTF16(expected_matches[j].contents),
+              matches[j].contents) << description << " Case # " << j;
+    EXPECT_EQ(expected_matches[j].allowed_to_be_default_match,
+              matches[j].allowed_to_be_default_match) << description
+                                                      << " Case # " << j;
+  }
+  // Ensure that no expected matches are missing.
+  for (; j < num_expected_matches; ++j) {
+    EXPECT_EQ(kNotApplicable, expected_matches[j].contents) <<
+        "Case # " << j << " " << description;
+  }
+}
+
 void SearchProviderTest::ResetFieldTrialList() {
   // Destroy the existing FieldTrialList before creating a new one to avoid
   // a DCHECK.
@@ -1130,7 +1173,7 @@ TEST_F(SearchProviderTest, NavSuggestNoSuggestedRelevanceScores) {
   AutocompleteMatch nav_match;
   EXPECT_TRUE(FindMatchWithDestination(GURL("http://a.com"), &nav_match));
   EXPECT_TRUE(nav_match.keyword.empty());
-  EXPECT_TRUE(nav_match.allowed_to_be_default_match);
+  EXPECT_FALSE(nav_match.allowed_to_be_default_match);
   EXPECT_FALSE(FindMatchWithDestination(GURL("http://a.com/b"), &nav_match));
 }
 
@@ -1163,9 +1206,9 @@ TEST_F(SearchProviderTest, SuggestRelevance) {
   EXPECT_GT(match_a1.relevance, match_a2.relevance);
   EXPECT_GT(match_a2.relevance, match_a3.relevance);
   EXPECT_TRUE(verbatim.allowed_to_be_default_match);
-  EXPECT_TRUE(match_a1.allowed_to_be_default_match);
-  EXPECT_TRUE(match_a2.allowed_to_be_default_match);
-  EXPECT_TRUE(match_a3.allowed_to_be_default_match);
+  EXPECT_FALSE(match_a1.allowed_to_be_default_match);
+  EXPECT_FALSE(match_a2.allowed_to_be_default_match);
+  EXPECT_FALSE(match_a3.allowed_to_be_default_match);
 }
 
 // Verifies that the default provider abandons suggested relevance scores
@@ -1198,22 +1241,27 @@ TEST_F(SearchProviderTest, DefaultProviderNoSuggestRelevanceInKeywordMode) {
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
-    QueryForInput(ASCIIToUTF16("k a"), false, true);
-    net::TestURLFetcher* default_fetcher =
-        test_factory_.GetFetcherByID(
-            SearchProvider::kDefaultProviderURLFetcherID);
-    ASSERT_TRUE(default_fetcher);
-    default_fetcher->set_response_code(200);
-    default_fetcher->SetResponseString(cases[i].default_provider_json);
-    default_fetcher->delegate()->OnURLFetchComplete(default_fetcher);
-    net::TestURLFetcher* keyword_fetcher =
-        test_factory_.GetFetcherByID(
-            SearchProvider::kKeywordProviderURLFetcherID);
-    ASSERT_TRUE(keyword_fetcher);
-    keyword_fetcher->set_response_code(200);
-    keyword_fetcher->SetResponseString(cases[i].keyword_provider_json);
-    keyword_fetcher->delegate()->OnURLFetchComplete(keyword_fetcher);
-    RunTillProviderDone();
+    // Send the query twice in order to have a synchronous pass after the first
+    // reply is received.  This is necessary because SearchProvider doesn't
+    // allow an asynchronous reply to change the default match.
+    for (size_t j = 0; j < 2; j++) {

[msw, 2014/08/27 18:44:38]

nit: ++j

[Mark P, 2014/08/27 23:08:20]

Done.

+      QueryForInput(ASCIIToUTF16("k a"), false, true);
+      net::TestURLFetcher* default_fetcher =
+          test_factory_.GetFetcherByID(
+              SearchProvider::kDefaultProviderURLFetcherID);
+      ASSERT_TRUE(default_fetcher);
+      default_fetcher->set_response_code(200);
+      default_fetcher->SetResponseString(cases[i].default_provider_json);
+      default_fetcher->delegate()->OnURLFetchComplete(default_fetcher);
+      net::TestURLFetcher* keyword_fetcher =
+          test_factory_.GetFetcherByID(
+              SearchProvider::kKeywordProviderURLFetcherID);
+      ASSERT_TRUE(keyword_fetcher);
+      keyword_fetcher->set_response_code(200);
+      keyword_fetcher->SetResponseString(cases[i].keyword_provider_json);
+      keyword_fetcher->delegate()->OnURLFetchComplete(keyword_fetcher);
+      RunTillProviderDone();
+    }
 
     const std::string description = "for input with default_provider_json=" +
         cases[i].default_provider_json + " and keyword_provider_json=" +
@@ -1271,7 +1319,7 @@ TEST_F(SearchProviderTest, DefaultFetcherSuggestRelevance) {
     // Negative values will have no effect; the calculated value will be used.
     { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":9999,"
                              "\"google:suggestrelevance\":[9998]}]",
-      { { "a", true}, { "a1", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
+      { { "a", true}, { "a1", false }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
         kEmptyMatch },
       std::string() },
     { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":9998,"
@@ -1293,8 +1341,8 @@ TEST_F(SearchProviderTest, DefaultFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\"],"
         "\"google:verbatimrelevance\":9999,"
         "\"google:suggestrelevance\":[9998]}]",
-      { { "a", true }, { "a.com", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
-        kEmptyMatch },
+      { { "a", true }, { "a.com", false }, kEmptyMatch, kEmptyMatch,
+        kEmptyMatch, kEmptyMatch },
       std::string() },
     { "[\"a\",[\"http://a.com\"],[],[],"
        "{\"google:suggesttype\":[\"NAVIGATION\"],"
@@ -1321,15 +1369,16 @@ TEST_F(SearchProviderTest, DefaultFetcherSuggestRelevance) {
     // Ensure that both types of relevance scores reorder matches together.
     { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[9999, 9997],"
                                      "\"google:verbatimrelevance\":9998}]",
-      { { "a1", true }, { "a", true }, { "a2", true }, kEmptyMatch, kEmptyMatch,
-        kEmptyMatch },
+      { { "a1", true }, { "a", true }, { "a2", false }, kEmptyMatch,
+        kEmptyMatch, kEmptyMatch },
       "1" },
 
-    // Allow non-inlineable matches to be the highest-scoring match but,
-    // if the result set lacks a single inlineable result, abandon suggested
-    // relevance scores entirely.
+    // Check that an inlineable result appears first regardless of its score.
+    // Also, if the result set lacks a single inlineable result, abandon the
+    // request to suppress verbatim (verbatim_relevance=0), which will then
+    // cause verbatim to appear (first).
     { "[\"a\",[\"b\"],[],[],{\"google:suggestrelevance\":[9999]}]",
-      { { "b", false }, { "a", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
+      { { "a", true }, { "b", false }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
         kEmptyMatch },
       std::string() },
     { "[\"a\",[\"b\"],[],[],{\"google:suggestrelevance\":[9999],"
@@ -1340,7 +1389,7 @@ TEST_F(SearchProviderTest, DefaultFetcherSuggestRelevance) {
     { "[\"a\",[\"http://b.com\"],[],[],"
        "{\"google:suggesttype\":[\"NAVIGATION\"],"
         "\"google:suggestrelevance\":[9999]}]",
-      { { "b.com", false }, { "a", true }, kEmptyMatch, kEmptyMatch,
+      { { "a", true }, { "b.com", false }, kEmptyMatch, kEmptyMatch,
         kEmptyMatch, kEmptyMatch },
       std::string() },
     { "[\"a\",[\"http://b.com\"],[],[],"
@@ -1356,43 +1405,43 @@ TEST_F(SearchProviderTest, DefaultFetcherSuggestRelevance) {
       { { "a1", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch, kEmptyMatch,
         kEmptyMatch },
       "1" },
-    { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":1}]",
+    { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":10}]",
       { { "a1", true }, { "a", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
         kEmptyMatch },
       "1" },
-    { "[\"a\",[\"a1\"],[],[],{\"google:suggestrelevance\":[1],"
+    { "[\"a\",[\"a1\"],[],[],{\"google:suggestrelevance\":[10],"
                              "\"google:verbatimrelevance\":0}]",
       { { "a1", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch, kEmptyMatch,
         kEmptyMatch },
       "1" },
-    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[1, 2],"
+    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[10, 20],"
                                      "\"google:verbatimrelevance\":0}]",
-      { { "a2", true }, { "a1", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
+      { { "a2", true }, { "a1", false }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
         kEmptyMatch },
       "2" },
-    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[1, 3],"
-      "\"google:verbatimrelevance\":2}]",
-      { { "a2", true }, { "a", true }, { "a1", true }, kEmptyMatch, kEmptyMatch,
-        kEmptyMatch },
+    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[10, 30],"
+      "\"google:verbatimrelevance\":20}]",
+      { { "a2", true }, { "a", true }, { "a1", false }, kEmptyMatch,
+        kEmptyMatch, kEmptyMatch },
       "2" },
     { "[\"a\",[\"http://a.com\"],[],[],"
        "{\"google:suggesttype\":[\"NAVIGATION\"],"
-        "\"google:suggestrelevance\":[1],"
+        "\"google:suggestrelevance\":[10],"
         "\"google:verbatimrelevance\":0}]",
       { { "a.com", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch, kEmptyMatch,
         kEmptyMatch },
       ".com" },
     { "[\"a\",[\"http://a1.com\", \"http://a2.com\"],[],[],"
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
-        "\"google:suggestrelevance\":[1, 2],"
+        "\"google:suggestrelevance\":[10, 20],"
         "\"google:verbatimrelevance\":0}]",
-      { { "a2.com", true }, { "a1.com", true }, kEmptyMatch, kEmptyMatch,
+      { { "a2.com", true }, { "a1.com", false }, kEmptyMatch, kEmptyMatch,
         kEmptyMatch, kEmptyMatch },
       "2.com" },
 
     // Ensure that all suggestions are considered, regardless of order.
     { "[\"a\",[\"b\", \"c\", \"d\", \"e\", \"f\", \"g\", \"h\"],[],[],"
-       "{\"google:suggestrelevance\":[1, 2, 3, 4, 5, 6, 7]}]",
+       "{\"google:suggestrelevance\":[10, 20, 30, 40, 50, 60, 70]}]",
       { { "a", true }, { "h", false }, { "g", false }, { "f", false },
         { "e", false }, { "d", false } },
       std::string() },
@@ -1403,44 +1452,44 @@ TEST_F(SearchProviderTest, DefaultFetcherSuggestRelevance) {
                                 "\"NAVIGATION\", \"NAVIGATION\","
                                 "\"NAVIGATION\", \"NAVIGATION\","
                                 "\"NAVIGATION\"],"
-        "\"google:suggestrelevance\":[1, 2, 3, 4, 5, 6, 7]}]",
+        "\"google:suggestrelevance\":[10, 20, 30, 40, 50, 60, 70]}]",
       { { "a", true }, { "h.com", false }, { "g.com", false },
         { "f.com", false }, { "e.com", false }, { "d.com", false } },
       std::string() },
 
     // Ensure that incorrectly sized suggestion relevance lists are ignored.
-    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[1]}]",
-      { { "a", true }, { "a1", true }, { "a2", true }, kEmptyMatch, kEmptyMatch,
-        kEmptyMatch },
+    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[10]}]",
+      { { "a", true }, { "a1", false }, { "a2", false }, kEmptyMatch,
+        kEmptyMatch, kEmptyMatch },
       std::string() },
-    { "[\"a\",[\"a1\"],[],[],{\"google:suggestrelevance\":[9999, 1]}]",
-      { { "a", true }, { "a1", true }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
+    { "[\"a\",[\"a1\"],[],[],{\"google:suggestrelevance\":[9999, 10]}]",
+      { { "a", true }, { "a1", false }, kEmptyMatch, kEmptyMatch, kEmptyMatch,
         kEmptyMatch },
       std::string() },
     { "[\"a\",[\"http://a1.com\", \"http://a2.com\"],[],[],"
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
-        "\"google:suggestrelevance\":[1]}]",
-      { { "a", true }, { "a1.com", true }, kEmptyMatch, kEmptyMatch,
+        "\"google:suggestrelevance\":[10]}]",
+      { { "a", true }, { "a1.com", false }, kEmptyMatch, kEmptyMatch,
         kEmptyMatch, kEmptyMatch },
       std::string() },
     { "[\"a\",[\"http://a1.com\"],[],[],"
        "{\"google:suggesttype\":[\"NAVIGATION\"],"
-       "\"google:suggestrelevance\":[9999, 1]}]",
-      { { "a", true }, { "a1.com", true }, kEmptyMatch, kEmptyMatch,
+       "\"google:suggestrelevance\":[9999, 10]}]",
+      { { "a", true }, { "a1.com", false }, kEmptyMatch, kEmptyMatch,
         kEmptyMatch, kEmptyMatch },
       std::string() },
 
     // Ensure that all 'verbatim' results are merged with their maximum score.
     { "[\"a\",[\"a\", \"a1\", \"a2\"],[],[],"
        "{\"google:suggestrelevance\":[9998, 9997, 9999]}]",
-      { { "a2", true }, { "a", true }, { "a1", true }, kEmptyMatch, kEmptyMatch,
-        kEmptyMatch },
+      { { "a2", true }, { "a", true }, { "a1", false }, kEmptyMatch,
+        kEmptyMatch, kEmptyMatch },
       "2" },
     { "[\"a\",[\"a\", \"a1\", \"a2\"],[],[],"
        "{\"google:suggestrelevance\":[9998, 9997, 9999],"
         "\"google:verbatimrelevance\":0}]",
-      { { "a2", true }, { "a", true }, { "a1", true }, kEmptyMatch, kEmptyMatch,
-        kEmptyMatch },
+      { { "a2", true }, { "a", true }, { "a1", false }, kEmptyMatch,
+        kEmptyMatch, kEmptyMatch },
       "2" },
 
     // Ensure that verbatim is always generated without other suggestions.
@@ -1456,15 +1505,20 @@ TEST_F(SearchProviderTest, DefaultFetcherSuggestRelevance) {
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
-    QueryForInput(ASCIIToUTF16("a"), false, false);
-    net::TestURLFetcher* fetcher =
-        test_factory_.GetFetcherByID(
-            SearchProvider::kDefaultProviderURLFetcherID);
-    ASSERT_TRUE(fetcher);
-    fetcher->set_response_code(200);
-    fetcher->SetResponseString(cases[i].json);
-    fetcher->delegate()->OnURLFetchComplete(fetcher);
-    RunTillProviderDone();
+    // Send the query twice in order to have a synchronous pass after the first
+    // reply is received.  This is necessary because SearchProvider doesn't
+    // allow an asynchronous reply to change the default match.
+    for (size_t j = 0; j < 2; j++) {

[msw, 2014/08/27 18:44:38]

nit: ++j

[Mark P, 2014/08/27 23:08:20]

Done.

+      QueryForInput(ASCIIToUTF16("a"), false, false);
+      net::TestURLFetcher* fetcher =
+          test_factory_.GetFetcherByID(
+              SearchProvider::kDefaultProviderURLFetcherID);
+      ASSERT_TRUE(fetcher);
+      fetcher->set_response_code(200);
+      fetcher->SetResponseString(cases[i].json);
+      fetcher->delegate()->OnURLFetchComplete(fetcher);
+      RunTillProviderDone();
+    }
 
     const std::string description = "for input with json=" + cases[i].json;
     const ACMatches& matches = provider_->matches();
@@ -1554,7 +1608,7 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
     { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":9999,"
                              "\"google:suggestrelevance\":[9998]}]",
       { { "a",   true,  true },
-        { "a1",  true,  true },
+        { "a1",  true,  false },
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       std::string() },
@@ -1593,33 +1647,32 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
                                      "\"google:verbatimrelevance\":9998}]",
       { { "a1",  true,  true },
         { "a",   true,  true },
-        { "a2",  true,  true },
+        { "a2",  true,  false },
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch },
       "1" },
 
-    // Check that non-inlinable matches may be ranked as the highest result
-    // if there is at least one inlineable match.
+    // Check that an inlinable match appears first regardless of its score.

[msw, 2014/08/27 18:44:38]

nit: "inlineable" seems more comment, fix up all the "inlinable" spellings.

[Mark P, 2014/08/27 23:08:20]

Fixed all (two).

     { "[\"a\",[\"b\"],[],[],{\"google:suggestrelevance\":[9999]}]",
-      { { "b",   true,  false },
-        { "a",   true,  true },
+      { { "a",   true,  true },
+        { "b",   true,  false },
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       std::string() },
     { "[\"a\",[\"http://b.com\"],[],[],"
        "{\"google:suggesttype\":[\"NAVIGATION\"],"
         "\"google:suggestrelevance\":[9999]}]",
-      { { "b.com", false, false },
-        { "a",     true,  true },
+      { { "a",     true,  true },
+        { "b.com", false, false },
         { "k a",   false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       std::string() },
-    // On the other hand, if there is no inlineable match, restore
-    // the keyword verbatim score.
+    // If there is no inlineable match, restore the keyword verbatim score.
+    // The keyword verbatim match will then appear first.
     { "[\"a\",[\"b\"],[],[],{\"google:suggestrelevance\":[9999],"
                             "\"google:verbatimrelevance\":0}]",
-      { { "b",   true,  false },
-        { "a",   true,  true },
+      { { "a",   true,  true },
+        { "b",   true,  false },
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       std::string() },
@@ -1627,8 +1680,8 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\"],"
         "\"google:suggestrelevance\":[9999],"
         "\"google:verbatimrelevance\":0}]",
-      { { "b.com", false, false },
-        { "a",     true,  true },
+      { { "a",     true,  true },
+        { "b.com", false, false },
         { "k a",   false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       std::string() },
@@ -1641,38 +1694,37 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch, kEmptyMatch },
       "1" },
-    { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":1}]",
+    { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":10}]",
       { { "a1",  true,  true },
         { "k a", false, false },
         { "a",   true,  true },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       "1" },
-    { "[\"a\",[\"a1\"],[],[],{\"google:suggestrelevance\":[1],"
+    { "[\"a\",[\"a1\"],[],[],{\"google:suggestrelevance\":[10],"
                              "\"google:verbatimrelevance\":0}]",
-      { { "k a", false, false },
-        { "a1",   true, true },
+      { { "a1",  true,  true },
+        { "k a", false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch, kEmptyMatch },
       "1" },
-    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[1, 2],"
+    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[10, 20],"
                                      "\"google:verbatimrelevance\":0}]",
-      {
+      { { "a2",  true,  true },
         { "k a", false, false },
-        { "a2",  true,  true },
-        { "a1",  true,  true },
+        { "a1",  true,  false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       "2" },
-    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[1, 3],"
-      "\"google:verbatimrelevance\":2}]",
-      { { "k a", false, false },
-        { "a2",  true,  true },
+    { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[10, 30],"
+      "\"google:verbatimrelevance\":20}]",
+      { { "a2",  true,  true },
+        { "k a", false, false },
         { "a",   true,  true },
-        { "a1",  true,  true },
+        { "a1",  true,  false },
         kEmptyMatch, kEmptyMatch },
       "2" },
 
     // Ensure that all suggestions are considered, regardless of order.
     { "[\"a\",[\"b\", \"c\", \"d\", \"e\", \"f\", \"g\", \"h\"],[],[],"
-       "{\"google:suggestrelevance\":[1, 2, 3, 4, 5, 6, 7]}]",
+       "{\"google:suggestrelevance\":[10, 20, 30, 40, 50, 60, 70]}]",
       { { "a",   true,  true },
         { "k a", false, false },
         { "h",   true,  false },
@@ -1687,7 +1739,7 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
                                 "\"NAVIGATION\", \"NAVIGATION\","
                                 "\"NAVIGATION\", \"NAVIGATION\","
                                 "\"NAVIGATION\"],"
-        "\"google:suggestrelevance\":[1, 2, 3, 4, 5, 6, 7]}]",
+        "\"google:suggestrelevance\":[10, 20, 30, 40, 50, 60, 70]}]",
       { { "a",     true,  true },
         { "k a",   false, false },
         { "h.com", false, false },
@@ -1701,14 +1753,14 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
     // mode) score more highly than the default verbatim.
     { "[\"a\",[\"a1\", \"a2\"],[],[],{\"google:suggestrelevance\":[1]}]",
       { { "a",   true,  true },
-        { "a1",  true,  true },
-        { "a2",  true,  true },
+        { "a1",  true,  false },
+        { "a2",  true,  false },
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch },
       std::string() },
     { "[\"a\",[\"a1\"],[],[],{\"google:suggestrelevance\":[9999, 1]}]",
       { { "a",   true,  true },
-        { "a1",  true,  true },
+        { "a1",  true,  false },
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       std::string() },
@@ -1736,7 +1788,7 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggestrelevance\":[9998, 9997, 9999]}]",
       { { "a2",  true,  true },
         { "a",   true,  true },
-        { "a1",  true,  true },
+        { "a1",  true,  false },
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch },
       "2" },
@@ -1745,7 +1797,7 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
         "\"google:verbatimrelevance\":0}]",
       { { "a2",  true,  true },
         { "a",   true,  true },
-        { "a1",  true,  true },
+        { "a1",  true,  false },
         { "k a", false, false },
         kEmptyMatch, kEmptyMatch },
       "2" },
@@ -1754,8 +1806,8 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
     // TODO(mpearson): Ensure the value of verbatimrelevance is respected
     // (except when suggested relevances are ignored).
     { "[\"a\",[],[],[],{\"google:verbatimrelevance\":1}]",
-      { { "k a", false, false },
-        { "a",   true,  true },
+      { { "a",   true,  true },
+        { "k a", false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch, kEmptyMatch },
       std::string() },
     { "[\"a\",[],[],[],{\"google:verbatimrelevance\":0}]",
@@ -1771,9 +1823,9 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
         "\"google:verbatimrelevance\":9990,"
         "\"google:suggestrelevance\":[9998, 9999]}]",
-      { { "a2.com", false, false },
+      { { "a",      true,  true },
+        { "a2.com", false, false },
         { "a1.com", false, false },
-        { "a",      true,  true },
         { "k a",    false, false },
         kEmptyMatch, kEmptyMatch },
       std::string() },
@@ -1781,17 +1833,17 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
         "\"google:verbatimrelevance\":9990,"
         "\"google:suggestrelevance\":[9999, 9998]}]",
-      { { "a1.com", false, false },
+      { { "a",      true,  true },
+        { "a1.com", false, false },
         { "a2.com", false, false },
-        { "a",      true,  true },
         { "k a",    false, false },
         kEmptyMatch, kEmptyMatch },
       std::string() },
     { "[\"a\",[\"https://a/\"],[],[],"
        "{\"google:suggesttype\":[\"NAVIGATION\"],"
         "\"google:suggestrelevance\":[9999]}]",
-      { { "https://a", false, false },
-        { "a",         true,  true },
+      { { "a",         true,  true },
+        { "https://a", false, false },
         { "k a",       false, false },
         kEmptyMatch, kEmptyMatch, kEmptyMatch },
       std::string() },
@@ -1801,10 +1853,10 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\", \"QUERY\"],"
         "\"google:verbatimrelevance\":9990,"
         "\"google:suggestrelevance\":[9998, 9999, 1300]}]",
-      { { "a2.com", false, false },
+      { { "a",      true,  true },
+        { "a2.com", false, false },
         { "a1.com", false, false },
-        { "a",      true,  true },
-        { "a3",     true,  true },
+        { "a3",     true,  false },
         { "k a",    false, false },
         kEmptyMatch },
       std::string() },
@@ -1812,10 +1864,10 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\", \"QUERY\"],"
         "\"google:verbatimrelevance\":9990,"
         "\"google:suggestrelevance\":[9999, 9998, 1300]}]",
-      { { "a1.com", false, false },
+      { { "a",      true,  true },
+        { "a1.com", false, false },
         { "a2.com", false, false },
-        { "a",      true,  true },
-        { "a3",     true,  true },
+        { "a3",     true,  false },
         { "k a",    false, false },
         kEmptyMatch },
       std::string() },
@@ -1825,9 +1877,9 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\", \"QUERY\"],"
         "\"google:verbatimrelevance\":9990,"
         "\"google:suggestrelevance\":[9998, 9999, 9997]}]",
-      { { "a2.com", false, false },
+      { { "a3",     true,  true },
+        { "a2.com", false, false },
         { "a1.com", false, false },
-        { "a3",     true,  true },
         { "a",      true,  true },
         { "k a",    false, false },
         kEmptyMatch },
@@ -1836,9 +1888,9 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\", \"QUERY\"],"
         "\"google:verbatimrelevance\":9990,"
         "\"google:suggestrelevance\":[9999, 9998, 9997]}]",
-      { { "a1.com", false, false },
+      { { "a3",     true,  true },
+        { "a1.com", false, false },
         { "a2.com", false, false },
-        { "a3",     true,  true },
         { "a",      true,  true },
         { "k a",    false, false },
         kEmptyMatch },
@@ -1847,9 +1899,9 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\", \"QUERY\"],"
         "\"google:verbatimrelevance\":0,"
         "\"google:suggestrelevance\":[9998, 9999, 9997]}]",
-      { { "a2.com", false, false },
+      { { "a3",     true,  true },
+        { "a2.com", false, false },
         { "a1.com", false, false },
-        { "a3",     true,  true },
         { "k a",    false, false },
         kEmptyMatch, kEmptyMatch },
       "3" },
@@ -1857,9 +1909,9 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\", \"QUERY\"],"
         "\"google:verbatimrelevance\":0,"
         "\"google:suggestrelevance\":[9999, 9998, 9997]}]",
-      { { "a1.com", false, false },
+      { { "a3",     true,  true },
+        { "a1.com", false, false },
         { "a2.com", false, false },
-        { "a3",     true,  true },
         { "k a",    false, false },
         kEmptyMatch, kEmptyMatch },
       "3" },
@@ -1870,9 +1922,9 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
         "\"google:verbatimrelevance\":0,"
         "\"google:suggestrelevance\":[9998, 9999]}]",
-      { { "a2.com", false, false },
+      { { "a",      true,  true },
+        { "a2.com", false, false },
         { "a1.com", false, false },
-        { "a",      true,  true },
         { "k a",    false, false },
         kEmptyMatch, kEmptyMatch },
       std::string() },
@@ -1880,9 +1932,9 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
        "{\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
         "\"google:verbatimrelevance\":0,"
         "\"google:suggestrelevance\":[9999, 9998]}]",
-      { { "a1.com", false, false },
+      { { "a",      true,  true },
+        { "a1.com", false, false },
         { "a2.com", false, false },
-        { "a",      true,  true },
         { "k a",    false, false },
         kEmptyMatch, kEmptyMatch },
       std::string() },
@@ -1912,27 +1964,32 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
-    QueryForInput(ASCIIToUTF16("k a"), false, true);
-
-    // Set up a default fetcher with no results.
-    net::TestURLFetcher* default_fetcher =
-        test_factory_.GetFetcherByID(
-            SearchProvider::kDefaultProviderURLFetcherID);
-    ASSERT_TRUE(default_fetcher);
-    default_fetcher->set_response_code(200);
-    default_fetcher->delegate()->OnURLFetchComplete(default_fetcher);
-    default_fetcher = NULL;
+    // Send the query twice in order to have a synchronous pass after the first
+    // reply is received.  This is necessary because SearchProvider doesn't
+    // allow an asynchronous reply to change the default match.
+    for (size_t j = 0; j < 2; j++) {

[msw, 2014/08/27 18:44:38]

nit: ++j

[Mark P, 2014/08/27 23:08:19]

Done.

+      QueryForInput(ASCIIToUTF16("k a"), false, true);
+
+      // Set up a default fetcher with no results.
+      net::TestURLFetcher* default_fetcher =
+          test_factory_.GetFetcherByID(
+              SearchProvider::kDefaultProviderURLFetcherID);
+      ASSERT_TRUE(default_fetcher);
+      default_fetcher->set_response_code(200);
+      default_fetcher->delegate()->OnURLFetchComplete(default_fetcher);
+      default_fetcher = NULL;
 
-    // Set up a keyword fetcher with provided results.
-    net::TestURLFetcher* keyword_fetcher =
-        test_factory_.GetFetcherByID(
-            SearchProvider::kKeywordProviderURLFetcherID);
-    ASSERT_TRUE(keyword_fetcher);
-    keyword_fetcher->set_response_code(200);
-    keyword_fetcher->SetResponseString(cases[i].json);
-    keyword_fetcher->delegate()->OnURLFetchComplete(keyword_fetcher);
-    keyword_fetcher = NULL;
-    RunTillProviderDone();
+      // Set up a keyword fetcher with provided results.
+      net::TestURLFetcher* keyword_fetcher =
+          test_factory_.GetFetcherByID(
+              SearchProvider::kKeywordProviderURLFetcherID);
+      ASSERT_TRUE(keyword_fetcher);
+      keyword_fetcher->set_response_code(200);
+      keyword_fetcher->SetResponseString(cases[i].json);
+      keyword_fetcher->delegate()->OnURLFetchComplete(keyword_fetcher);
+      keyword_fetcher = NULL;
+      RunTillProviderDone();
+    }
 
     const std::string description = "for input with json=" + cases[i].json;
     const ACMatches& matches = provider_->matches();
@@ -1962,6 +2019,224 @@ TEST_F(SearchProviderTest, KeywordFetcherSuggestRelevance) {
   }
 }
 
+TEST_F(SearchProviderTest, DontInlineAutocompleteAsynchronously) {
+  // This test sends two separate queries, each receiving different JSON
+  // replies, and checks that at each stage of processing (receiving first
+  // asynchronous reply, handling new keystroke synchronously / sending the
+  // second request, and receiving the second asynchronous reply) we have the
+  // expected matches.  In particular, receiving the second reply shouldn't
+  // cause an unexpected inline autcompletion.
+  struct {
+    const std::string first_json;
+    const ExpectedMatch first_async_matches[4];
+    const ExpectedMatch sync_matches[4];
+    const std::string second_json;
+    const ExpectedMatch second_async_matches[4];
+  } cases[] = {
+    // A simple test that verifies we don't inline autocomplete after the
+    // first asynchronous reply, but we do at the next keystroke if the
+    // reply's results were good enough.  Furthermore, we should continue
+    // inline autocompleting after the second asynchronous reply if the new
+    // top suggestion is the same as the old inline autocompleted suggestion.
+    { "[\"a\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"

[msw, 2014/08/27 18:44:39]

nit: most tests avoid numbers near this threshold, consider using 999x.

[Mark P, 2014/08/27 23:08:19]

Done.

+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "a", true }, { "ab1", false }, { "ab2", false },
+        kEmptyExpectedMatch },
+      { { "ab1", true }, { "ab2", true }, { "ab", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "ab1", true }, { "ab2", false }, { "ab", true },
+        kEmptyExpectedMatch } },
+    // Ditto, just for a navigation suggestion.
+    { "[\"a\",[\"ab1.com\", \"ab2.com\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "a", true }, { "ab1.com", false }, { "ab2.com", false },
+        kEmptyExpectedMatch },
+      { { "ab1.com", true }, { "ab2.com", true }, { "ab", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1.com\", \"ab2.com\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "ab1.com", true }, { "ab2.com", false }, { "ab", true },
+        kEmptyExpectedMatch } },
+    // A more realistic test of the same situation.
+    { "[\"a\",[\"abcdef\", \"abcdef.com\", \"abc\"],[],[],"
+       "{\"google:verbatimrelevance\":900,"
+        "\"google:suggesttype\":[\"QUERY\", \"NAVIGATION\", \"QUERY\"],"
+        "\"google:suggestrelevance\":[1250, 1200, 1000]}]",
+      { { "a", true }, { "abcdef", false }, { "abcdef.com", false },
+        { "abc", false } },
+      { { "abcdef", true }, { "abcdef.com", true }, { "abc", true },
+        { "ab", true } },
+      "[\"ab\",[\"abcdef\", \"abcdef.com\", \"abc\"],[],[],"
+       "{\"google:verbatimrelevance\":900,"
+        "\"google:suggesttype\":[\"QUERY\", \"NAVIGATION\", \"QUERY\"],"
+        "\"google:suggestrelevance\":[1250, 1200, 1000]}]",
+      { { "abcdef", true }, { "abcdef.com", false }, { "abc", false },
+        { "ab", true } } },
+
+    // Without an original inline autcompletion, a new inline autcompletion
+    // should be rejected.
+    { "[\"a\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[900, 800]}]",
+      { { "a", true }, { "ab1", false }, { "ab2", false },
+        kEmptyExpectedMatch },
+      { { "ab", true }, { "ab1", true }, { "ab2", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "ab", true }, { "ab1", false }, { "ab2", false },
+        kEmptyExpectedMatch } },
+    { "[\"a\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[900, 800]}]",
+      { { "a", true }, { "ab1", false }, { "ab2", false },
+        kEmptyExpectedMatch },
+      { { "ab", true }, { "ab1", true }, { "ab2", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[1301, 1302]}]",

[msw, 2014/08/27 18:44:38]

This case only differs from above in this scoring of the second async results.
That seems a bit superfluous to me, but maybe I'm missing something that adds
more value in these similar cases? Did you intend to do something more complex
like give initial scores that inline "ab1" and followup scores that inline
"ab2"? (but that seems covered by your line 2155 set of cases).

[Mark P, 2014/08/27 23:08:20]

No, I didn't intend to do something more complex.  I merely wanted to make sure
that for results we decide not to inline, we order them according to their new
scores, not their earlier scores.  Added comment.

[msw, 2014/08/28 19:21:39]

Acknowledged.

+      { { "ab", true }, { "ab2", false }, { "ab1", false },
+        kEmptyExpectedMatch } },
+    // Now, the same verifications but with the new inline autocompletion as a
+    // navsuggestion.  The new autocompletion should still be rejected.
+    { "[\"a\",[\"ab1.com\", \"ab2.com\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
+        "\"google:suggestrelevance\":[900, 800]}]",
+      { { "a", true }, { "ab1.com", false }, { "ab2.com", false },
+        kEmptyExpectedMatch },
+      { { "ab", true }, { "ab1.com", true }, { "ab2.com", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1.com\", \"ab2.com\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "ab", true }, { "ab1.com", false }, { "ab2.com", false },
+        kEmptyExpectedMatch } },
+    { "[\"a\",[\"ab1.com\", \"ab2.com\"],[],[],"

[msw, 2014/08/27 18:44:39]

Ditto, similar case.

[Mark P, 2014/08/27 23:08:19]

Acknowledged.

+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
+        "\"google:suggestrelevance\":[900, 800]}]",
+      { { "a", true }, { "ab1.com", false }, { "ab2.com", false },
+        kEmptyExpectedMatch },
+      { { "ab", true }, { "ab1.com", true }, { "ab2.com", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1.com\", \"ab2.com\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggesttype\":[\"NAVIGATION\", \"NAVIGATION\"],"
+        "\"google:suggestrelevance\":[1301, 1302]}]",
+      { { "ab", true }, { "ab2.com", false }, { "ab1.com", false },
+        kEmptyExpectedMatch } },
+
+    // It's okay to abandon an inline autocompletion asynchronously.

[msw, 2014/08/27 18:44:39]

Wait, but why?* If I were about to hit enter for the inline "ab1" suggestion and
the async reply yanks it out from under me as soon as the reply is received, how
is that more okay than other behavior you're fixing here? (*a fun blog, btw).

I guess we can't guarantee that the prior top suggestion is sent again, so maybe
my expectation to retain the inline match on an async reply is unreasonable?

[Mark P, 2014/08/27 23:08:20]

From discussion with the suggest folks, this should happen rarely if at all.  We
decided not to put this in the initial version of the code.  You'll see some
mention of this in the bug discussion.  We'll decide later if it's worth the
trouble to implement.  (It's harder to implement than this change, and this
change wasn't easy.)

[msw, 2014/08/28 19:21:40]

Acknowledged.

+    { "[\"a\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "a", true }, { "ab1", false }, { "ab2", false },
+        kEmptyExpectedMatch },
+      { { "ab1", true }, { "ab2", true }, { "ab", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[900, 800]}]",
+      { { "ab", true }, { "ab1", true }, { "ab2", false },
+        kEmptyExpectedMatch } },
+
+    // Note: it's possible that the suggest server returns a suggestion with
+    // an inline autocompletion (that as usual we delay in allowing it to
+    // be displayed as an inline autocompletion until the next keystroke),
+    // then, in reply to the next keystroke, the server returns a different
+    // suggestion as an inline autocompletion.  This is not likely to happen.
+    // Regardless, if it does, one could imagine three different behaviors:
+    // - keep the original inline autocompletion until the next keystroke
+    //   (i.e., don't abandon an inline autocompletion asynchronously)
+    // - abandon all inline autocompletions

[msw, 2014/08/27 18:44:38]

nit: if you want to expound upon these behaviors (which may be unnecessary), you
should explain when the second and third changes would occur (on the next
keystroke or on the async reply).

[Mark P, 2014/08/27 23:08:20]

Done.  I found expanding it useful for me, and likely handy for reference.

[msw, 2014/08/28 19:21:39]

Acknowledged.

+    // - ignore the new inline autocompletion, yet possibly keep the original
+    //   if it scores well in the most recent reply
+    // All of these behaviors are reasonable.  The main thing we want to
+    // ensure is that the second asynchronous reply shouldn't cause _a new_
+    // inline autocompletion to be displayed.  We test that here.
+    // (BTW, it happens that the code does the third of those bullets.)
+    { "[\"a\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "a", true }, { "ab1", false }, { "ab2", false },
+        kEmptyExpectedMatch },
+      { { "ab1", true }, { "ab2", true }, { "ab", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1\", \"ab3\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[1302, 1400]}]",
+      { { "ab1", true }, { "ab3", false }, { "ab", true },
+        kEmptyExpectedMatch } },
+    { "[\"a\",[\"ab1\", \"ab2\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[1302, 1301]}]",
+      { { "a", true }, { "ab1", false }, { "ab2", false },
+        kEmptyExpectedMatch },
+      { { "ab1", true }, { "ab2", true }, { "ab", true },
+        kEmptyExpectedMatch },
+      "[\"ab\",[\"ab1\", \"ab3\"],[],[],"
+       "{\"google:verbatimrelevance\":1300,"
+        "\"google:suggestrelevance\":[900, 1400]}]",
+      { { "ab", true }, { "ab3", false }, { "ab1", true },
+        kEmptyExpectedMatch } },
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {

[msw, 2014/08/27 18:44:38]

nit: ++i

[Mark P, 2014/08/27 23:08:20]

Done.

+    // First, send the query "a" and receive the provided JSON reply,

[msw, 2014/08/27 18:44:39]

nit: remove "provided" for a one-liner.

[Mark P, 2014/08/27 23:08:19]

Done.

+    // |first_json|.
+    ClearAllResults();
+    QueryForInput(ASCIIToUTF16("a"), false, false);

[msw, 2014/08/27 18:44:38]

This [clear, query, set response, get provider, run till done] pattern is common
and repeated in this file, can [a subset of] it be made into a helper function?

[Mark P, 2014/08/27 23:08:20]

Done.  The helper function can get (and does) now get widely used. :-)

[msw, 2014/08/28 19:21:40]

Thank you for this nice cleanup!

+    net::TestURLFetcher* first_fetcher =
+        test_factory_.GetFetcherByID(
+            SearchProvider::kDefaultProviderURLFetcherID);
+    ASSERT_TRUE(first_fetcher);
+    first_fetcher->set_response_code(200);
+    first_fetcher->SetResponseString(cases[i].first_json);
+    first_fetcher->delegate()->OnURLFetchComplete(first_fetcher);
+    RunTillProviderDone();
+
+    // Verify that the matches after the asynchronous results are as expected.
+    std::string description = "first asynchronous reply for input with "

[msw, 2014/08/27 18:44:39]

nit: s/reply/response/ to match other comments? these could be improved a bit.

[Mark P, 2014/08/27 23:08:20]

Consistency is a virtue.  Fixed all.

+        "first_json=" + cases[i].first_json;
+    CheckMatches(description, ARRAYSIZE_UNSAFE(cases[i].first_async_matches),
+                 cases[i].first_async_matches, provider_->matches());
+
+    // Then, send the query "ab" and check the synchronous matches.
+    description = "synchronous response after first keystroke after input "

[msw, 2014/08/27 18:44:38]

nit: "the first keystroke"

[Mark P, 2014/08/27 23:08:19]

Done.

+        "with first_json=" + cases[i].first_json;
+    QueryForInput(ASCIIToUTF16("ab"), false, false);
+    CheckMatches(description, ARRAYSIZE_UNSAFE(cases[i].sync_matches),
+                 cases[i].sync_matches, provider_->matches());
+
+    // Finally, get the provided JSON reply, |second_json|, and verify the
+    // matches after the second asynchronous reply are as expected.
+    description = "second asynchronous response after input with first_json=" +
+        cases[i].first_json + " and second_json=" + cases[i].second_json;
+    net::TestURLFetcher* second_fetcher =
+        test_factory_.GetFetcherByID(
+            SearchProvider::kDefaultProviderURLFetcherID);
+    ASSERT_TRUE(second_fetcher);
+    second_fetcher->set_response_code(200);
+    second_fetcher->SetResponseString(cases[i].second_json);
+    second_fetcher->delegate()->OnURLFetchComplete(second_fetcher);
+    RunTillProviderDone();
+    CheckMatches(description, ARRAYSIZE_UNSAFE(cases[i].second_async_matches),
+                 cases[i].second_async_matches, provider_->matches());
+  }
+}
+
 TEST_F(SearchProviderTest, LocalAndRemoteRelevances) {
   // We hardcode the string "term1" below, so ensure that the search term that
   // got added to history already is that string.
@@ -2065,20 +2340,21 @@ TEST_F(SearchProviderTest, DefaultProviderSuggestRelevanceScoringUrlInput) {
     const std::string json;
     const DefaultFetcherUrlInputMatch output[4];
   } cases[] = {
-    // Ensure NAVIGATION matches are allowed to be listed first for URL
-    // input regardless of whether the match is inlineable.  Note that
-    // non-inlineable matches should not be allowed to be the default match.
+    // Ensure NAVIGATION matches are allowed to be listed first for URL input.
+    // Non-inlineable matches should not be allowed to be the default match.
+    // Note that the top-scoring inlineable match is moved to the top
+    // regardless of its score.
     { "a.com", "[\"a.com\",[\"http://b.com/\"],[],[],"
                 "{\"google:suggesttype\":[\"NAVIGATION\"],"
                  "\"google:suggestrelevance\":[9999]}]",
-      { { "b.com",   AutocompleteMatchType::NAVSUGGEST,            false },
-        { "a.com",   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
+      { { "a.com",   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
+        { "b.com",   AutocompleteMatchType::NAVSUGGEST,            false },
         kEmptyMatch, kEmptyMatch } },
     { "a.com", "[\"a.com\",[\"https://b.com\"],[],[],"
                 "{\"google:suggesttype\":[\"NAVIGATION\"],"
                  "\"google:suggestrelevance\":[9999]}]",
-      { { "https://b.com", AutocompleteMatchType::NAVSUGGEST,           false },
-        { "a.com",         AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
+      { { "a.com",         AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
+        { "https://b.com", AutocompleteMatchType::NAVSUGGEST,           false },
         kEmptyMatch, kEmptyMatch } },
     { "a.com", "[\"a.com\",[\"http://a.com/a\"],[],[],"
                 "{\"google:suggesttype\":[\"NAVIGATION\"],"
@@ -2098,22 +2374,22 @@ TEST_F(SearchProviderTest, DefaultProviderSuggestRelevanceScoringUrlInput) {
     // relevances.
     { "a.com", "[\"a.com\",[\"a.com info\"],[],[],"
                 "{\"google:suggestrelevance\":[9999]}]",
-      { { "a.com",      AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
-        { "a.com info", AutocompleteMatchType::SEARCH_SUGGEST,        true },
+      { { "a.com",      AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true  },
+        { "a.com info", AutocompleteMatchType::SEARCH_SUGGEST,        false },
         kEmptyMatch, kEmptyMatch } },
     { "a.com", "[\"a.com\",[\"a.com info\"],[],[],"
                 "{\"google:suggestrelevance\":[9999]}]",
-      { { "a.com",   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,   true },
-        { "a.com info", AutocompleteMatchType::SEARCH_SUGGEST,        true },
+      { { "a.com",      AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true  },
+        { "a.com info", AutocompleteMatchType::SEARCH_SUGGEST,        false },
         kEmptyMatch, kEmptyMatch } },
 
     // Ensure the fallback mechanism allows inlinable NAVIGATION matches.
     { "a.com", "[\"a.com\",[\"a.com info\", \"http://a.com/b\"],[],[],"
                 "{\"google:suggesttype\":[\"QUERY\", \"NAVIGATION\"],"
                  "\"google:suggestrelevance\":[9999, 9998]}]",
-      { { "a.com/b",    AutocompleteMatchType::NAVSUGGEST,            true },
-        { "a.com",      AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
-        { "a.com info", AutocompleteMatchType::SEARCH_SUGGEST,        true },
+      { { "a.com/b",    AutocompleteMatchType::NAVSUGGEST,            true  },
+        { "a.com info", AutocompleteMatchType::SEARCH_SUGGEST,        false },
+        { "a.com",      AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true  },
         kEmptyMatch } },
     { "a.com", "[\"a.com\",[\"a.com info\", \"http://a.com/b\"],[],[],"
                 "{\"google:suggesttype\":[\"QUERY\", \"NAVIGATION\"],"
@@ -2121,52 +2397,63 @@ TEST_F(SearchProviderTest, DefaultProviderSuggestRelevanceScoringUrlInput) {
                  "\"google:verbatimrelevance\":9999}]",
       { { "a.com/b",    AutocompleteMatchType::NAVSUGGEST,            true },
         { "a.com",      AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
-        { "a.com info", AutocompleteMatchType::SEARCH_SUGGEST,        true },
+        { "a.com info", AutocompleteMatchType::SEARCH_SUGGEST,        false },
         kEmptyMatch } },
 
-    // Ensure topmost non-inlineable SUGGEST matches are allowed for URL
-    // input assuming the top inlineable match is not a query (i.e., is a
-    // NAVSUGGEST).
+    // Ensure non-inlineable SUGGEST matches are allowed for URL input
+    // assuming the best inlineable match is not a query (i.e., is a
+    // NAVSUGGEST).  The best inlineable match will be at the top of the
+    // list regardless of its score.
     { "a.com", "[\"a.com\",[\"info\"],[],[],"
                 "{\"google:suggestrelevance\":[9999]}]",
-      { { "info",  AutocompleteMatchType::SEARCH_SUGGEST,        false },
-        { "a.com", AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
+      { { "a.com", AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true  },
+        { "info",  AutocompleteMatchType::SEARCH_SUGGEST,        false },
         kEmptyMatch, kEmptyMatch } },
     { "a.com", "[\"a.com\",[\"info\"],[],[],"
                 "{\"google:suggestrelevance\":[9999]}]",
-      { { "info",  AutocompleteMatchType::SEARCH_SUGGEST,        false },
-        { "a.com", AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true },
+      { { "a.com", AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, true  },
+        { "info",  AutocompleteMatchType::SEARCH_SUGGEST,        false },
         kEmptyMatch, kEmptyMatch } },
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
-    QueryForInput(ASCIIToUTF16(cases[i].input), false, false);
-    net::TestURLFetcher* fetcher =
-        test_factory_.GetFetcherByID(
-            SearchProvider::kDefaultProviderURLFetcherID);
-    ASSERT_TRUE(fetcher);
-    fetcher->set_response_code(200);
-    fetcher->SetResponseString(cases[i].json);
-    fetcher->delegate()->OnURLFetchComplete(fetcher);
-    RunTillProviderDone();
+    // Send the query twice in order to have a synchronous pass after the first
+    // reply is received.  This is necessary because SearchProvider doesn't
+    // allow an asynchronous reply to change the default match.
+    for (size_t j = 0; j < 2; j++) {

[msw, 2014/08/27 18:44:39]

nit: ++j

[Mark P, 2014/08/27 23:08:19]

Done.

+      QueryForInput(ASCIIToUTF16(cases[i].input), false, false);
+      net::TestURLFetcher* fetcher =
+          test_factory_.GetFetcherByID(
+              SearchProvider::kDefaultProviderURLFetcherID);
+      ASSERT_TRUE(fetcher);
+      fetcher->set_response_code(200);
+      fetcher->SetResponseString(cases[i].json);
+      fetcher->delegate()->OnURLFetchComplete(fetcher);
+      RunTillProviderDone();
+    }
 
+    const std::string description = "input=" + cases[i].input + " json=" +
+        cases[i].json;
     size_t j = 0;
     const ACMatches& matches = provider_->matches();
-    ASSERT_LE(matches.size(), ARRAYSIZE_UNSAFE(cases[i].output));
+    ASSERT_LE(matches.size(), ARRAYSIZE_UNSAFE(cases[i].output)) <<
+        description;

[msw, 2014/08/27 18:44:39]

nit: SCOPED_TRACE may be your friend here and in similar cases.

[Mark P, 2014/08/27 23:08:20]

Good idea.  Fixed all << whatever in this file to SCOPED_TRACE where it seemed
reasonable.  (If it wouldn't actually save any lines of code, I didn't bother to
do it.)

[msw, 2014/08/28 19:21:40]

Acknowledged.

     // Ensure that the returned matches equal the expectations.

[msw, 2014/08/27 18:44:38]

Ditto, too bad this also checks |match_type|.

[Mark P, 2014/08/27 23:08:19]

Acknowledged.

     for (; j < matches.size(); ++j) {
       EXPECT_EQ(ASCIIToUTF16(cases[i].output[j].match_contents),
-                matches[j].contents);
-      EXPECT_EQ(cases[i].output[j].match_type, matches[j].type);
+                matches[j].contents) << description;
+      EXPECT_EQ(cases[i].output[j].match_type, matches[j].type) << description;
       EXPECT_EQ(cases[i].output[j].allowed_to_be_default_match,
-                matches[j].allowed_to_be_default_match);
+                matches[j].allowed_to_be_default_match) << description;
     }
     // Ensure that no expected matches are missing.
     for (; j < ARRAYSIZE_UNSAFE(cases[i].output); ++j) {
-      EXPECT_EQ(kNotApplicable, cases[i].output[j].match_contents);
+      EXPECT_EQ(kNotApplicable, cases[i].output[j].match_contents) <<
+          description;
       EXPECT_EQ(AutocompleteMatchType::NUM_TYPES,
-                cases[i].output[j].match_type);
-      EXPECT_FALSE(cases[i].output[j].allowed_to_be_default_match);
+                cases[i].output[j].match_type) << description;
+      EXPECT_FALSE(cases[i].output[j].allowed_to_be_default_match) <<
+          description;
     }
   }
 }
@@ -2379,11 +2666,12 @@ TEST_F(SearchProviderTest, NavigationInline) {
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
     // First test regular mode.
     QueryForInput(ASCIIToUTF16(cases[i].input), false, false);
-    AutocompleteMatch match(
-        provider_->NavigationToMatch(SearchSuggestionParser::NavigationResult(
-            ChromeAutocompleteSchemeClassifier(&profile_), GURL(cases[i].url),
-            AutocompleteMatchType::NAVSUGGEST, base::string16(), std::string(),
-            false, 0, false, ASCIIToUTF16(cases[i].input), std::string())));
+    SearchSuggestionParser::NavigationResult result(
+        ChromeAutocompleteSchemeClassifier(&profile_), GURL(cases[i].url),
+        AutocompleteMatchType::NAVSUGGEST, base::string16(), std::string(),
+        false, 0, false, ASCIIToUTF16(cases[i].input), std::string());
+    result.set_received_after_last_keystroke(false);
+    AutocompleteMatch match(provider_->NavigationToMatch(result));
     EXPECT_EQ(ASCIIToUTF16(cases[i].inline_autocompletion),
               match.inline_autocompletion);
     EXPECT_EQ(ASCIIToUTF16(cases[i].fill_into_edit), match.fill_into_edit);
@@ -2392,11 +2680,13 @@ TEST_F(SearchProviderTest, NavigationInline) {
 
     // Then test prevent-inline-autocomplete mode.
     QueryForInput(ASCIIToUTF16(cases[i].input), true, false);
+    SearchSuggestionParser::NavigationResult result_prevent_inline(
+        ChromeAutocompleteSchemeClassifier(&profile_), GURL(cases[i].url),
+        AutocompleteMatchType::NAVSUGGEST, base::string16(), std::string(),
+        false, 0, false, ASCIIToUTF16(cases[i].input), std::string());
+    result_prevent_inline.set_received_after_last_keystroke(false);
     AutocompleteMatch match_prevent_inline(
-        provider_->NavigationToMatch(SearchSuggestionParser::NavigationResult(
-            ChromeAutocompleteSchemeClassifier(&profile_), GURL(cases[i].url),
-            AutocompleteMatchType::NAVSUGGEST, base::string16(), std::string(),
-            false, 0, false, ASCIIToUTF16(cases[i].input), std::string())));
+        provider_->NavigationToMatch(result_prevent_inline));
     EXPECT_EQ(ASCIIToUTF16(cases[i].inline_autocompletion),
               match_prevent_inline.inline_autocompletion);
     EXPECT_EQ(ASCIIToUTF16(cases[i].fill_into_edit),
@@ -2410,10 +2700,11 @@ TEST_F(SearchProviderTest, NavigationInline) {
 TEST_F(SearchProviderTest, NavigationInlineSchemeSubstring) {
   const base::string16 input(ASCIIToUTF16("ht"));
   const base::string16 url(ASCIIToUTF16("http://a.com"));
-  const SearchSuggestionParser::NavigationResult result(
+  SearchSuggestionParser::NavigationResult result(
       ChromeAutocompleteSchemeClassifier(&profile_), GURL(url),
       AutocompleteMatchType::NAVSUGGEST,
       base::string16(), std::string(), false, 0, false, input, std::string());
+  result.set_received_after_last_keystroke(false);
 
   // Check the offset and strings when inline autocompletion is allowed.
   QueryForInput(input, false, false);
@@ -2434,12 +2725,13 @@ TEST_F(SearchProviderTest, NavigationInlineSchemeSubstring) {
 // Verifies that input "w" marks a more significant domain label than "www.".
 TEST_F(SearchProviderTest, NavigationInlineDomainClassify) {
   QueryForInput(ASCIIToUTF16("w"), false, false);
-  AutocompleteMatch match(
-      provider_->NavigationToMatch(SearchSuggestionParser::NavigationResult(
-          ChromeAutocompleteSchemeClassifier(&profile_),
-          GURL("http://www.wow.com"),
-          AutocompleteMatchType::NAVSUGGEST, base::string16(), std::string(),
-          false, 0, false, ASCIIToUTF16("w"), std::string())));
+  SearchSuggestionParser::NavigationResult result(
+      ChromeAutocompleteSchemeClassifier(&profile_),
+      GURL("http://www.wow.com"), AutocompleteMatchType::NAVSUGGEST,
+      base::string16(), std::string(), false, 0, false, ASCIIToUTF16("w"),
+      std::string());
+  result.set_received_after_last_keystroke(false);
+  AutocompleteMatch match(provider_->NavigationToMatch(result));
   EXPECT_EQ(ASCIIToUTF16("ow.com"), match.inline_autocompletion);
   EXPECT_TRUE(match.allowed_to_be_default_match);
   EXPECT_EQ(ASCIIToUTF16("www.wow.com"), match.fill_into_edit);