Download Feedback Service should upload all eligible downloads

chrome/browser/safe_browsing/download_feedback_service.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/download_feedback_service.h"
 
 #include <utility>
 
 #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"
 #include "base/task_runner.h"
 #include "chrome/browser/safe_browsing/download_feedback.h"
 #include "content/public/browser/download_danger_type.h"
 #include "content/public/browser/download_item.h"
 
 namespace safe_browsing {
 
@@ skipping 112 matching lines @@
 }
 
 // static
 void DownloadFeedbackService::RecordEligibleDownloadShown(
     content::DownloadDangerType danger_type) {
   UMA_HISTOGRAM_ENUMERATION("SBDownloadFeedback.Eligible",
                             danger_type,
                             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);
 
   DownloadFeedbackPings* pings = DownloadFeedbackPings::FromDownload(*download);
   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
 void DownloadFeedbackService::BeginFeedbackOrDeleteFile(
     const scoped_refptr<base::TaskRunner>& file_task_runner,
     const base::WeakPtr<DownloadFeedbackService>& service,
     const std::string& ping_request,
     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(),
                                     path,
                                     false,
                                     base::FileUtilProxy::StatusCallback());
   }
 }
 
 void DownloadFeedbackService::StartPendingFeedback() {
   DCHECK(!active_feedback_.empty());
   active_feedback_.front()->Start(base::Bind(
       &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,
       ping_request, ping_response));
   active_feedback_.push(std::move(feedback));
   UMA_HISTOGRAM_COUNTS_100("SBDownloadFeedback.ActiveFeedbacks",
                            active_feedback_.size());
 
   if (active_feedback_.size() == 1)
     StartPendingFeedback();
 }
 
 void DownloadFeedbackService::FeedbackComplete() {
   DVLOG(1) << __func__;
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!active_feedback_.empty());
   active_feedback_.pop();
   if (!active_feedback_.empty())
     StartPendingFeedback();
 }
 
 }  // namespace safe_browsing