Download Feedback Service should upload all eligible downloads

chrome/browser/safe_browsing/download_feedback_service.cc

Index: chrome/browser/safe_browsing/download_feedback_service.cc
diff --git a/chrome/browser/safe_browsing/download_feedback_service.cc b/chrome/browser/safe_browsing/download_feedback_service.cc
index 0b78aa79c112625dc79eafcd1795674cc6c6033d..d9999620b9c80f51406b490eed5552e81d48fa70 100644
--- a/chrome/browser/safe_browsing/download_feedback_service.cc
+++ b/chrome/browser/safe_browsing/download_feedback_service.cc
@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/files/file_util_proxy.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/supports_user_data.h"
@@ -140,11 +141,10 @@ void DownloadFeedbackService::RecordEligibleDownloadShown(
                             content::DOWNLOAD_DANGER_TYPE_MAX);
 }
 
-
 void DownloadFeedbackService::BeginFeedbackForDownload(
-    content::DownloadItem* download) {
+    content::DownloadItem* download,
+    DownloadCommands::Command download_command) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
   UMA_HISTOGRAM_ENUMERATION("SBDownloadFeedback.Activations",
                             download->GetDangerType(),
                             content::DOWNLOAD_DANGER_TYPE_MAX);
@@ -153,11 +153,12 @@ void DownloadFeedbackService::BeginFeedbackForDownload(
   DCHECK(pings);
 
   download->StealDangerousDownload(
+      download_command == DownloadCommands::DISCARD,
       base::Bind(&DownloadFeedbackService::BeginFeedbackOrDeleteFile,
-                 file_task_runner_,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 pings->ping_request(),
-                 pings->ping_response()));
+                 file_task_runner_, weak_ptr_factory_.GetWeakPtr(),
+                 pings->ping_request(), pings->ping_response()));
+  if (download_command == DownloadCommands::KEEP)
+    DownloadCommands(download).ExecuteCommand(download_command);
 }
 
 // static
@@ -168,6 +169,11 @@ void DownloadFeedbackService::BeginFeedbackOrDeleteFile(
     const std::string& ping_response,
     const base::FilePath& path) {
   if (service) {
+    if (path.empty()) {
+      UMA_HISTOGRAM_BOOLEAN("SBDownloadFeedback.EmptyFilePathFailure", true);

[Steven Holte, 2016/11/14 21:34:34]

It would be better to consolidate these into single macro invocation, since each
creates a static variable.

[Jialiu Lin, 2016/11/14 21:55:07]

Done.

+      return;
+    }
+    UMA_HISTOGRAM_BOOLEAN("SBDownloadFeedback.EmptyFilePathFailure", false);
     service->BeginFeedback(ping_request, ping_response, path);
   } else {
     base::FileUtilProxy::DeleteFile(file_task_runner.get(),
@@ -183,10 +189,9 @@ void DownloadFeedbackService::StartPendingFeedback() {
       &DownloadFeedbackService::FeedbackComplete, base::Unretained(this)));
 }
 
-void DownloadFeedbackService::BeginFeedback(
-    const std::string& ping_request,
-    const std::string& ping_response,
-    const base::FilePath& path) {
+void DownloadFeedbackService::BeginFeedback(const std::string& ping_request,
+                                            const std::string& ping_response,
+                                            const base::FilePath& path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   std::unique_ptr<DownloadFeedback> feedback(DownloadFeedback::Create(
       request_context_getter_.get(), file_task_runner_.get(), path,