Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads

chrome/browser/download/download_danger_prompt_browsertest.cc

Index: chrome/browser/download/download_danger_prompt_browsertest.cc
diff --git a/chrome/browser/download/download_danger_prompt_browsertest.cc b/chrome/browser/download/download_danger_prompt_browsertest.cc
index 2b33e0e7e29eed67d4d2c660cc8ffa0e157ee051..24075be7967e88e65c4358fa550c1df982523d8a 100644
--- a/chrome/browser/download/download_danger_prompt_browsertest.cc
+++ b/chrome/browser/download/download_danger_prompt_browsertest.cc
@@ -4,12 +4,15 @@
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
+#include "base/memory/weak_ptr.h"

[Lei Zhang, 2015/11/24 18:55:27]

Not used?

[Jialiu Lin, 2015/11/25 23:43:21]

Thanks for catching this. Done.

 #include "chrome/browser/download/download_danger_prompt.h"
 #include "chrome/browser/profiles/profile.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_tabstrip.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/test/mock_download_item.h"
@@ -23,13 +26,58 @@ using ::testing::Eq;
 using ::testing::Return;
 using ::testing::ReturnRef;
 using ::testing::SaveArg;
+using safe_browsing::ClientSafeBrowsingReportRequest;
+using safe_browsing::SafeBrowsingService;
+
+static const char* kTestDownloadUrl = "http://evildownload.com";

[asanka, 2015/11/24 18:48:14]

Nit: You want 'static const char* const kTestDownloadUrl'. The extra const means
that you can't change what kTestDownloadUrl points to. E.g.:

   static const char* const kConstConst = "foo";
   static const char* kConst = "bar";
 
   kConst = "baz"; // valid. kConst is not const although *kConst is.
   kConstConst = "baz"; // fails to compile. Both kConstConst and *kConstConst
are const.

[Lei Zhang, 2015/11/24 18:55:27]

Or just const char kFoo[]

[Jialiu Lin, 2015/11/25 23:43:21]

Done.

[Jialiu Lin, 2015/11/25 23:43:21]

Thanks, good to know. 

[Lei Zhang, 2015/11/25 23:48:15]

In general, prefer const char kFoo[] unless you really need a pointer.

[Jialiu Lin, 2015/11/26 00:58:58]

Done.

+
+class FakeSafeBrowsingService : public SafeBrowsingService {
+ public:
+  FakeSafeBrowsingService() {}
+
+  void SendDownloadRecoveryReport(const std::string& report) override {
+    report_ = report;
+  }
+
+  std::string GetDownloadRecoveryReport() { return report_; }

[Lei Zhang, 2015/11/24 18:55:27]

const method

[Jialiu Lin, 2015/11/25 23:43:21]

Done.

+
+ protected:
+  ~FakeSafeBrowsingService() override {}
+
+ private:
+  std::string report_;
+};
+
+// Factory that creates FakeSafeBrowsingService instances.
+class TestSafeBrowsingServiceFactory
+    : public safe_browsing::SafeBrowsingServiceFactory {
+ public:
+  TestSafeBrowsingServiceFactory() : fake_safe_browsing_service_(NULL) {}

[asanka, 2015/11/24 18:48:14]

NULL -> nullptr for new code.

[Jialiu Lin, 2015/11/25 23:43:21]

Done.

+  ~TestSafeBrowsingServiceFactory() override {}
+
+  SafeBrowsingService* CreateSafeBrowsingService() override {
+    fake_safe_browsing_service_ = new FakeSafeBrowsingService();

[asanka, 2015/11/24 18:48:14]

Check that fake_safe_browsing_service_ isn't set already.

Also you'll want to call Initialize() since it's a real SBService.

[Jialiu Lin, 2015/11/25 23:43:21]

Added check to see if fake_safe_browsing_service_ is set already.

Actually Initialize() function will be triggered inside
InProcessBrowserTest::SetUp(), so no need to specifically call it here.(double
verified by adding debug printout in SafeBrowsingService::Initialize() function)

+    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 DownloadDangerPromptTest : public InProcessBrowserTest {
  public:
   DownloadDangerPromptTest()
-    : prompt_(NULL),
-      expected_action_(DownloadDangerPrompt::CANCEL),
-      did_receive_callback_(false) {
+      : prompt_(NULL),

[Lei Zhang, 2015/11/24 18:55:27]

Also nullptr while you are here?

[Jialiu Lin, 2015/11/25 23:43:21]

Done.

+        expected_action_(DownloadDangerPrompt::CANCEL),
+        did_receive_callback_(false),
+        safe_browsing_factory_(new TestSafeBrowsingServiceFactory()),
+        report_sent_(false) {
+    safe_browsing::SafeBrowsingService::RegisterFactory(

[asanka, 2015/11/24 18:48:14]

Do this in SetUp().

[Jialiu Lin, 2015/11/25 23:43:21]

Moved to SetUp()

+        safe_browsing_factory_.get());
   }
 
   ~DownloadDangerPromptTest() override {}
@@ -49,7 +97,10 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
     did_receive_callback_ = false;
     expected_action_ = expected_action;
     SetUpDownloadItemExpectations();
+    SetUpSafeBrowsingReportExpectations(expected_action ==
+                                        DownloadDangerPrompt::ACCEPT);
     CreatePrompt();
+    report_sent_ = false;
   }
 
   void VerifyExpectations() {
@@ -59,10 +110,16 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
     EXPECT_TRUE(did_receive_callback_);
     EXPECT_FALSE(prompt_);
     testing::Mock::VerifyAndClearExpectations(&download_);
+    if (report_sent_) {
+      EXPECT_EQ(expected_serialized_report_,
+                safe_browsing_factory_->fake_safe_browsing_service()
+                    ->GetDownloadRecoveryReport());
+    }
   }
 
   void SimulatePromptAction(DownloadDangerPrompt::Action action) {
     prompt_->InvokeActionForTesting(action);
+    report_sent_ = true;
   }
 
   content::MockDownloadItem& download() { return download_; }
@@ -77,6 +134,15 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
         .WillRepeatedly(Return(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL));
   }
 
+  void SetUpSafeBrowsingReportExpectations(bool did_proceed) {
+    ClientSafeBrowsingReportRequest expected_report;
+    expected_report.set_url(GURL(kTestDownloadUrl).spec());
+    expected_report.set_type(
+        ClientSafeBrowsingReportRequest::MALICIOUS_DOWNLOAD_RECOVERY);
+    expected_report.set_did_proceed(did_proceed);
+    expected_report.SerializeToString(&expected_serialized_report_);
+  }
+
   void CreatePrompt() {
     prompt_ = DownloadDangerPrompt::Create(
         &download_,
@@ -97,6 +163,9 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
   DownloadDangerPrompt* prompt_;
   DownloadDangerPrompt::Action expected_action_;
   bool did_receive_callback_;
+  scoped_ptr<TestSafeBrowsingServiceFactory> safe_browsing_factory_;
+  std::string expected_serialized_report_;
+  bool report_sent_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadDangerPromptTest);
 };
@@ -109,7 +178,8 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
 #endif
 IN_PROC_BROWSER_TEST_F(DownloadDangerPromptTest, MAYBE_TestAll) {
   // ExperienceSampling: Set default actions for DownloadItem methods we need.
-  ON_CALL(download(), GetURL()).WillByDefault(ReturnRef(GURL::EmptyGURL()));
+  GURL download_url(kTestDownloadUrl);
+  ON_CALL(download(), GetURL()).WillByDefault(ReturnRef(download_url));
   ON_CALL(download(), GetReferrerUrl())
       .WillByDefault(ReturnRef(GURL::EmptyGURL()));
   ON_CALL(download(), GetBrowserContext())