Store xsrf token received with psuggest results.

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 bae74a73fa47250bd597ca52fd6e952f8bc8bbe8..8fcc9f62b38e8e1a84797b9f6a157562b9a96b44 100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -56,6 +56,37 @@ ACMatches::const_iterator FindDefaultMatch(const ACMatches& matches) {
   return it;
 }
 
+class SuggestionDeletionHandler;
+class SearchProviderForTest : public SearchProvider {
+ public:
+  SearchProviderForTest(
+      AutocompleteProviderListener* listener,
+      Profile* profile);
+  bool is_success() { return is_success_; };
+
+ protected:
+  virtual ~SearchProviderForTest();
+
+ private:
+  virtual void RecordDeletionResult(bool success) OVERRIDE;
+  bool is_success_;
+  FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, TestDeleteMatch_FakeProvider);

[Peter Kasting, 2013/12/03 00:33:51]

Nit: This declaration seems useless?

[Maria, 2013/12/03 01:00:26]

Done.

+  DISALLOW_COPY_AND_ASSIGN(SearchProviderForTest);
+};
+
+SearchProviderForTest::SearchProviderForTest(
+    AutocompleteProviderListener* listener,
+    Profile* profile) : SearchProvider(listener, profile),

[Peter Kasting, 2013/12/03 00:33:51]

Nit: Wrap ":" and following to next line

[Maria, 2013/12/03 01:00:26]

Done.

+                        is_success_(false) {
+}
+
+SearchProviderForTest::~SearchProviderForTest() {
+}
+
+void SearchProviderForTest::RecordDeletionResult(bool success) {
+  is_success_ = success;
+}
+
 } // namespace
 
 // SearchProviderTest ---------------------------------------------------------
@@ -187,7 +218,7 @@ class SearchProviderTest : public testing::Test,
   TestingProfile profile_;
 
   // The provider.
-  scoped_refptr<SearchProvider> provider_;
+  scoped_refptr<SearchProviderForTest> provider_;
 
   // If non-NULL, OnProviderUpdate quits the current |run_loop_|.
   base::RunLoop* run_loop_;
@@ -245,7 +276,7 @@ void SearchProviderTest::SetUp() {
   // requests to ensure the InMemoryDatabase is the state we expect it.
   profile_.BlockUntilHistoryProcessesPendingRequests();
 
-  provider_ = new SearchProvider(this, &profile_);
+  provider_ = new SearchProviderForTest(this, &profile_);
   provider_->kMinimumTimeBetweenSuggestQueriesMs = 0;
 }
 
@@ -3354,7 +3385,8 @@ TEST_F(SearchProviderTest, RemoveStaleResultsTest) {
       } else {
         provider_->default_results_.suggest_results.push_back(
             SearchProvider::SuggestResult(ASCIIToUTF16(suggestion), string16(),
-                                          string16(), std::string(), false,
+                                          string16(), std::string(),
+                                          std::string(), false,
                                           cases[i].results[j].relevance,
                                           false, false));
       }
@@ -3756,7 +3788,8 @@ TEST_F(SearchProviderTest, XSSIGuardedJSONParsing) {
     AutocompleteMatchType::Type type;
   };
   const Match kEmptyMatch = { kNotApplicable,
-                              AutocompleteMatchType::NUM_TYPES};
+                              AutocompleteMatchType::NUM_TYPES
+  };

[Peter Kasting, 2013/12/03 00:33:51]

Nit: Wrapping the } is fine, but in that case, also wrap after the opening "{".

[Maria, 2013/12/03 01:00:26]

Done.

 
   struct {
     const std::string input_text;
@@ -3835,6 +3868,82 @@ TEST_F(SearchProviderTest, XSSIGuardedJSONParsing) {
   }
 }
 
+// Test that deletion url gets set on an AutocompleteMatch when available for a
+// personalized query.
+TEST_F(SearchProviderTest, ParseDeletionUrl) {
+   struct Match {
+     std::string contents;
+     std::string deletion_url;
+     AutocompleteMatchType::Type type;
+   };
+
+   const Match kEmptyMatch = {
+       kNotApplicable, "", AutocompleteMatchType::NUM_TYPES
+   };
+
+   const char url[] = "https://www.google.com/complete/deleteitems"
+       "?delq=ab&client=chrome&deltok=xsrf123";
+
+   struct {
+       const std::string input_text;
+       const std::string response_json;
+       const Match matches[4];
+     } cases[] = {
+       // A deletion URL on a personalized query should be reflected in the
+       // resulting AutocompleteMatch.
+       { "a",
+         "[\"a\",[\"ab\", \"ac\"],[],[],"
+         "{\"google:suggesttype\":[\"PERSONALIZED_QUERY\",\"QUERY\"],"
+         "\"google:suggestrelevance\":[1, 2],"
+         "\"google:suggestdetail\":[{\"du\":"
+         "\"https://www.google.com/complete/deleteitems?delq=ab&client=chrome"
+         "&deltok=xsrf123\"}, {}]}]",
+         { { "a", "", AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED },
+           { "ac", "", AutocompleteMatchType::SEARCH_SUGGEST },
+           { "ab", url, AutocompleteMatchType::SEARCH_SUGGEST },
+           kEmptyMatch,
+         },
+       },
+       // Personalized queries without deletion URLs shouldn't cause errors.
+       { "a",
+         "[\"a\",[\"ab\", \"ac\"],[],[],"
+         "{\"google:suggesttype\":[\"PERSONALIZED_QUERY\",\"QUERY\"],"
+         "\"google:suggestrelevance\":[1, 2],"
+         "\"google:suggestdetail\":[{}, {}]}]",
+         { { "a", "", AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED },
+           { "ac", "", AutocompleteMatchType::SEARCH_SUGGEST },
+           { "ab", "", AutocompleteMatchType::SEARCH_SUGGEST },
+           kEmptyMatch,
+         },
+       },
+     };
+
+     for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
+       QueryForInput(ASCIIToUTF16(cases[i].input_text), false, false);
+
+       net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID(
+           SearchProvider::kDefaultProviderURLFetcherID);
+       ASSERT_TRUE(fetcher);
+       fetcher->set_response_code(200);
+       fetcher->SetResponseString(cases[i].response_json);
+       fetcher->delegate()->OnURLFetchComplete(fetcher);
+
+       RunTillProviderDone();
+
+       const ACMatches& matches = provider_->matches();
+       ASSERT_FALSE(matches.empty());
+
+       SCOPED_TRACE("for input with json = " + cases[i].response_json);
+
+       for (size_t j = 0; j < matches.size(); ++j) {
+         const Match& match = cases[i].matches[j];
+         SCOPED_TRACE(" and match index: " + base::IntToString(j));
+         EXPECT_EQ(match.contents, UTF16ToUTF8(matches[j].contents));
+         EXPECT_EQ(match.deletion_url, matches[j].GetAdditionalInfo(
+             "deletion_url"));
+       }
+     }
+}
 
 TEST_F(SearchProviderTest, ReflectsBookmarkBarState) {
   profile_.GetPrefs()->SetBoolean(prefs::kShowBookmarkBar, false);
@@ -3981,3 +4090,43 @@ TEST_F(SearchProviderTest, CanSendURL) {
       GURL("https://www.google.com/complete/search"), &google_template_url,
       AutocompleteInput::OTHER, &profile_));
 }
+
+TEST_F(SearchProviderTest, TestDeleteMatch_HasDeletionUrlSuccess) {

[Anuj, 2013/12/03 01:25:47]

I think TestDeleteMatch_Success is sufficient. HasDeletionUrlSuccess is
confusing to me.

+  AutocompleteMatch match(provider_, 0, true,
+                          AutocompleteMatchType::SEARCH_SUGGEST);
+  match.RecordAdditionalInfo(
+      SearchProvider::kDeletionUrlKey,
+      "https://www.google.com/complete/deleteitem?q=foo");
+  provider_->matches_.push_back(match);
+  provider_->DeleteMatch(match);

[Anuj, 2013/12/03 01:25:47]

An empty line before this to create separation between setup and test part of
the code.

[Maria, 2013/12/03 01:40:15]

Done.

+  ASSERT_FALSE(provider_->deletion_handlers_.empty());
+  ASSERT_TRUE(provider_->matches_.empty());
+  // Set up a default fetcher with provided results.
+  net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID(
+      SearchProvider::kDeletionURLFetcherID);
+  ASSERT_TRUE(fetcher);

[Anuj, 2013/12/03 01:25:47]

Omit this? It doesn't seem particularly useful.

[Maria, 2013/12/03 01:40:15]

All the other tests in this class do it. It's useful because if the tested code
unexpected didn't create a fetcher, we will catch it right here.

+  fetcher->set_response_code(200);

[Anuj, 2013/12/03 01:25:47]

Move the fetcher set up code before the call to DeleteMatch.

[Maria, 2013/12/03 01:40:15]

That's not possible because GetFetcherByID relies on the fetcher getting created
within DeleteMatch() call.

+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  ASSERT_TRUE(provider_->deletion_handlers_.empty());
+  ASSERT_TRUE(provider_->is_success());
+}
+
+TEST_F(SearchProviderTest, TestDeleteMatch_HasDeletionUrlFailure) {
+  AutocompleteMatch match(provider_, 0, true,
+                          AutocompleteMatchType::SEARCH_SUGGEST);
+  match.RecordAdditionalInfo(
+      SearchProvider::kDeletionUrlKey,
+      "https://www.google.com/complete/deleteitem?q=foo");

[Peter Kasting, 2013/12/03 00:33:51]

Nit: If we combine these two tests into one, we can eliminate this
re-construction of |match| and maybe a few of the other lines too.

[Maria, 2013/12/03 01:00:26]

We could, but I'd rather have two separate test cases. It seems cleaner that
they don't depend on each other's passing and I don't believe it's a big deal to
have repeated code in a unit test.

[Peter Kasting, 2013/12/03 01:03:22]

They shouldn't depend on each other's passing.  Which makes me notice: Almost
all these ASSERTs should be EXPECTs.  Only use ASSERT for conditions where
failing the condition and continuing would result in a subsequent crash.  Once
you do this, I believe the tests should be independent even if combined into one
function.
It's the same as repeated code in Chrome -- it's a maintenance burden if you
need to modify the affected code, and risks not all tests getting changed
appropriately.

I don't think it's a huge deal, but I think it's strictly better to not
duplicate than to duplicate.

[Anuj, 2013/12/03 01:25:47]

I have been told that the assert/expects should not be put in helper methods, as
they affect readability of tests.

I think the only part which really merits avoiding duplication is creating and
adding the match to provider_.matches_.

+  provider_->matches_.push_back(match);
+  provider_->DeleteMatch(match);
+  ASSERT_FALSE(provider_->deletion_handlers_.empty());
+  ASSERT_TRUE(provider_->matches_.empty());
+  // Set up a default fetcher with provided results.
+  net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID(
+      SearchProvider::kDeletionURLFetcherID);
+  ASSERT_TRUE(fetcher);
+  fetcher->set_response_code(500);
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  ASSERT_TRUE(provider_->deletion_handlers_.empty());
+  ASSERT_FALSE(provider_->is_success());
+}