Track CTR of uncommon download warning.

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 331fb0e600faee41c2a49711bb6055fbf19927d7..4f7cd98d6939a1fdf8f35084325712ce3d4119e1 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -32,6 +32,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
+#include "chrome/browser/download/download_commands.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_item_model.h"
@@ -75,6 +76,7 @@
 #include "components/infobars/core/confirm_infobar_delegate.h"
 #include "components/infobars/core/infobar.h"
 #include "components/prefs/pref_service.h"
+#include "content/public/browser/download_danger_type.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
@@ -388,6 +390,46 @@ class HistoryObserver : public DownloadHistory::Observer {
   DISALLOW_COPY_AND_ASSIGN(HistoryObserver);
 };
 
+class FakeSafeBrowsingService : public safe_browsing::SafeBrowsingService {

[asanka, 2016/03/14 18:13:46]

This the 7th FakeSafeBrowsingService :-(. Not necessarily for this CL, but we
really should make this a test fixture under chrome/browser/safe_browsing/.

[Jialiu Lin, 2016/03/14 23:54:04]

Totally agree. Bug created at https://crbug.com/594683

+ public:
+  FakeSafeBrowsingService() {}
+
+  std::string GetDownloadReport() const { return report_; }
+
+ protected:
+  ~FakeSafeBrowsingService() override {}
+
+  void SendSerializedDownloadReport(const std::string& report) override {
+    // Disable download protection to prevent flackiness.

[asanka, 2016/03/14 18:13:46]

*flakiness.

Also, you'll need to explain how disabling the download protection service at
this point prevents flakiness.

[Jialiu Lin, 2016/03/14 23:54:04]

No need of this line since FakeDownloadProtectionService is used. 

+    download_protection_service()->SetEnabled(false);
+    report_ = report;
+  }
+
+  std::string report_;
+};
+
+// Factory that creates FakeSafeBrowsingService instances.
+class TestSafeBrowsingServiceFactory
+    : public safe_browsing::SafeBrowsingServiceFactory {
+ public:
+  TestSafeBrowsingServiceFactory() : fake_safe_browsing_service_(nullptr) {}
+  ~TestSafeBrowsingServiceFactory() override {}
+
+  safe_browsing::SafeBrowsingService* CreateSafeBrowsingService() override {
+    if (!fake_safe_browsing_service_) {

[asanka, 2016/03/14 18:13:46]

This function should be getting called at most once per browser session. It's
acceptable to verify it and rely on it. I.e.
DCHECK(!fake_safe_browsing_service_) and always create one. Otherwise it appears
from this test that there's uncertainty about whether multiple safe browsing
service instances could be created.

[Jialiu Lin, 2016/03/14 23:54:04]

Done.

+      fake_safe_browsing_service_ = new FakeSafeBrowsingService();
+    }
+    return fake_safe_browsing_service_.get();
+  }
+
+  scoped_refptr<FakeSafeBrowsingService> fake_safe_browsing_service() {
+    return fake_safe_browsing_service_;
+  }
+
+ private:
+  scoped_refptr<FakeSafeBrowsingService> fake_safe_browsing_service_;
+};
+
 class DownloadTest : public InProcessBrowserTest {
  public:
   // Choice of navigation or direct fetch.  Used by |DownloadFileCheckErrors()|.
@@ -415,7 +457,19 @@ class DownloadTest : public InProcessBrowserTest {
     content::TestFileErrorInjector::FileErrorInfo error_info;
   };
 
-  DownloadTest() {}
+  DownloadTest()
+      : test_safe_browsing_factory_(new TestSafeBrowsingServiceFactory()) {}
+
+  void SetUp() override {
+    safe_browsing::SafeBrowsingService::RegisterFactory(
+        test_safe_browsing_factory_.get());
+    InProcessBrowserTest::SetUp();
+  }
+
+  void TearDown() override {
+    safe_browsing::SafeBrowsingService::RegisterFactory(nullptr);
+    InProcessBrowserTest::TearDown();
+  }
 
   void SetUpOnMainThread() override {
     base::FeatureList::ClearInstanceForTesting();
@@ -1059,6 +1113,9 @@ class DownloadTest : public InProcessBrowserTest {
     return download;
   }
 
+ protected:
+  scoped_ptr<TestSafeBrowsingServiceFactory> test_safe_browsing_factory_;
+
  private:
   static void EnsureNoPendingDownloadJobsOnIO(bool* result) {
     if (net::URLRequestSlowDownloadJob::NumberOutstandingRequests())
@@ -3270,6 +3327,50 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, FeedbackService) {
   GetDownloads(browser(), &updated_downloads);
   ASSERT_TRUE(updated_downloads.empty());
 }
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, SendUncommonDownloadAcceptReport) {
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled,
+                                               true);
+  // Make a dangerous file.
+  GURL download_url(
+      net::URLRequestMockHTTPJob::GetMockUrl(kDangerousMockFilePath));
+  scoped_ptr<content::DownloadTestObserver> dangerous_observer(
+      DangerousDownloadWaiter(
+          browser(), 1,
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT));
+  scoped_ptr<content::DownloadTestObserver> in_progress_observer(
+      new content::DownloadTestObserverInProgress(
+          DownloadManagerForBrowser(browser()), 1));
+
+  ui_test_utils::NavigateToURL(browser(), download_url);
+  in_progress_observer->WaitForFinished();
+
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForBrowser(browser())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+  DownloadItem* download = downloads[0];
+  // Simulates an uncommon download.
+  download->OnContentCheckCompleted(

[asanka, 2016/03/14 18:13:46]

OnContentCheckCompleted cannot be issued until all the data has been saved. The
test will crash if OnAllDataSaved() hasn't been called prior to this point.
Since the in_progress_observer doesn't enforce it, we may end up being flaky.
:-(

In decreasing order of preference, here's what I think should happen instead:

 *  Make DownloadProtectionService flag the download as UNCOMMON. One approach
would be to introduce a new FakeDownloadProtectionService that could be returned
by FakeSafeBrowsingService::download_protection_service(). In response to
FakeDownloadProtectionService::CheckClientDownload(), it can respond with a
verdict of UNCOMMON. Then you can wait for the dangerous_observer to complete
without calling OnContentCheckCompleted(). You can call
DownloadCommands::ExecuteCommand(KEEP) after this.

 *  Failing that, add a DownloadTestObserver derivative that waits for
DownloadItem::AllDataSaved() to be true. Since the download is expected to be
marked as dangerous, receiving all the data will not result in the download
being marked as complete. Once AllDataSaved() becomes true, you can safely call
OnContentCheckCompleted(). You only need the DownloadAllDataSavedObserver (which
you wait on before calling OnCOntentCheckCompleted) and a
DownloadTestObserverTerminal that you wait on at the end to make sure that the
download runs to completion.

[Jialiu Lin, 2016/03/14 23:54:04]

implemented based on option 1 here. Now , no need to create an
in_progress_observer. 

+      content::DownloadDangerType::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT);
+  ASSERT_EQ(download->GetDangerType(),
+            content::DownloadDangerType::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT);
+  DownloadCommands(download).ExecuteCommand(DownloadCommands::KEEP);
+  dangerous_observer->WaitForFinished();
+
+  safe_browsing::ClientSafeBrowsingReportRequest actual_report;
+  actual_report.ParseFromString(
+      test_safe_browsing_factory_->fake_safe_browsing_service()
+          ->GetDownloadReport());
+  EXPECT_EQ(safe_browsing::ClientSafeBrowsingReportRequest::
+                DANGEROUS_DOWNLOAD_WARNING,
+            actual_report.type());
+  EXPECT_EQ(safe_browsing::ClientDownloadResponse::UNCOMMON,
+            actual_report.download_verdict());
+  EXPECT_EQ(download->GetURL().spec(), actual_report.url());
+  EXPECT_TRUE(actual_report.did_proceed());
+
+  download->Cancel(true);
+}
 #endif  // FULL_SAFE_BROWSING
 
 class DownloadTestWithShelf : public DownloadTest {