Add browsertest for the feedback service

chrome/browser/download/download_browsertest.cc

Index: chrome/browser/download/download_browsertest.cc
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index 48b0b4d7d1f6580c1eee7b8b1afd1c251b966591..e501c2bea1d95e7891d63a52a49577bbfa2c58bb 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -47,6 +47,9 @@
 #include "chrome/browser/net/url_request_mock_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
+#include "chrome/browser/safe_browsing/download_feedback_service.h"
+#include "chrome/browser/safe_browsing/download_protection_service.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -58,6 +61,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/test_switches.h"
@@ -800,6 +804,10 @@ class DownloadTest : public InProcessBrowserTest {
     file_activity_observer_->EnableFileChooser(enable);
   }
 
+  void ChangeDangerTypeForFeedbackTest() {
+    file_activity_observer_->ChangeDangerTypeForFeedbackTest();
+  }
+
   bool DidShowFileChooser() {
     return file_activity_observer_->TestAndResetDidShowFileChooser();
   }
@@ -3230,3 +3238,58 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, Resumption_MultipleAttempts) {
 
   EXPECT_FALSE(DidShowFileChooser());
 }
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, FeedbackService) {
+  // Make a dangerous/UNCOMMON file.
+  ChangeDangerTypeForFeedbackTest();
+  base::FilePath file(FILE_PATH_LITERAL("downloads/dangerous/dangerous.swf"));
+  GURL download_url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  // Download the url and wait until the object has been completely stored.
+  scoped_ptr<content::DownloadTestObserver> download_observer(
+      new content::DownloadTestObserverTerminal(
+          DownloadManagerForBrowser(browser()), 1,
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_IGNORE));
+  HistoryObserver observer(browser()->profile());
+  observer.SetFilterCallback(base::Bind(&HasDataAndName));
+  ui_test_utils::NavigateToURL(browser(), download_url);
+  observer.WaitForStored();

[asanka, 2014/03/26 18:01:30]

This observer only waits for the download to begin being stored into history.
This may be too early. Use a DownloadTestObserverInProgress instead. That will
at least wait for the filename determination to complete. The download should
then be reliably dangerous when observer.WaitForEvent() returns.

[felt, 2014/06/03 00:35:22]

I think this is fixed.

+  scoped_ptr<std::vector<history::DownloadRow> > downloads_in_database;
+  ASSERT_TRUE(DownloadsHistoryDataCollector(
+      browser()->profile()).WaitForDownloadInfo(&downloads_in_database));
+  ASSERT_EQ(1u, downloads_in_database->size());
+
+  // Get the download from the DownloadManager.
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForBrowser(browser())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+  EXPECT_TRUE(downloads[0]->IsDangerous());
+
+  // Save fake pings for the download.
+  safe_browsing::ClientDownloadReport fake_metadata;
+  fake_metadata.mutable_download_request()->set_url("http://test");
+  fake_metadata.mutable_download_request()->set_length(1);
+  fake_metadata.mutable_download_request()->mutable_digests()->set_sha1("hi");
+  fake_metadata.mutable_download_response()->set_verdict(
+      safe_browsing::ClientDownloadResponse::UNCOMMON);
+  std::string ping_request(
+      fake_metadata.download_request().SerializeAsString());
+  std::string ping_response(
+      fake_metadata.download_response().SerializeAsString());
+  SafeBrowsingService* sb_service = g_browser_process->safe_browsing_service();
+  safe_browsing::DownloadProtectionService* download_protection_service =
+      sb_service->download_protection_service();
+  download_protection_service->feedback_service()->MaybeStorePingsForDownload(
+      safe_browsing::DownloadProtectionService::UNCOMMON,
+      downloads[0],
+      ping_request,
+      ping_response);
+  ASSERT_TRUE(safe_browsing::DownloadFeedbackService::IsEnabledForDownload(
+      *(downloads[0])));
+
+  // Begin feedback and check that the file to is "stolen".
+  download_protection_service->feedback_service()->BeginFeedbackForDownload(
+      downloads[0]);
+  GetDownloads(browser(), &downloads);

[asanka, 2014/03/26 18:01:30]

GetDownloads() appends to the vector that's passed into it. It doesn't clear the
list. So if the list wasn't empty at this point, then it's not going to be empty
after the call even if there are no downloads.

[felt, 2014/06/03 00:35:22]

Done.

+  ASSERT_TRUE(downloads.empty());
+}