Add browsertest for the feedback service

chrome/browser/download/download_test_file_activity_observer.cc

Index: chrome/browser/download/download_test_file_activity_observer.cc
diff --git a/chrome/browser/download/download_test_file_activity_observer.cc b/chrome/browser/download/download_test_file_activity_observer.cc
index db68b6fd60906615ff11c3777cc62fdcc3eb4f0a..cb838fcd3d4e28897e09a525ffde22bdedd653e5 100644
--- a/chrome/browser/download/download_test_file_activity_observer.cc
+++ b/chrome/browser/download/download_test_file_activity_observer.cc
@@ -25,6 +25,7 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate
       : ChromeDownloadManagerDelegate(profile),
         file_chooser_enabled_(false),
         file_chooser_displayed_(false),
+        downloads_dangerous_(false),
         weak_ptr_factory_(this) {
     if (!profile->IsOffTheRecord())
       GetDownloadIdReceiverCallback().Run(
@@ -47,6 +48,38 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate
     return weak_ptr_factory_.GetWeakPtr();
   }
 
+  void ChangeDangerTypeForFeedbackTest() {

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

Suggestion: Use a more generic name since this is a shared API. Perhaps
something like SetDangerTypeForNewDownloads(content::DownloadDangerType type) ?
Make sure MockDownloadManagerDelegate preserves its previous behavior if that
method is never called.

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

My problem here is that I don't know how to get the global danger type into the
static SetDangerous callback, which is a DownloadTargetCallback. I don't want to
modify DownloadTargetCallback, nor do I want to make a switch statement and 5
versions of SetDangerous. There must be a cleaner way that I am not thinking of,
any ideas?

[asanka, 2014/06/05 19:20:12]

static void SetDangerous(const DownloadTargetCallback& callback,
      DownloadDangerType forced_danger_type,
      ...);

And when binding the callback:

base::Bind(&SetDangerous, callback, global_danger_type_)

+    downloads_dangerous_ = true;

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

Use a more descriptive name? mark_downloads_as_dangerous_ ?

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

Done.

+  }
+
+  virtual bool DetermineDownloadTarget(
+      content::DownloadItem* item,
+      const content::DownloadTargetCallback& callback) OVERRIDE {
+    if (downloads_dangerous_ &&
+        item->GetDangerType() == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE) {

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

If you are forcing a danger type, I'd propose just doing it rather than
conditionalize it the item's current danger type. Also, the danger type isn't
set until ChromeDownloadManagerDelegate::DetermineDownloadTarget() returns.

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

I believe this should be fixed now.

+      content::DownloadTargetCallback dangerous_callback =
+          base::Bind(&MockDownloadManagerDelegate::SetDangerous,
+                     callback);
+      return ChromeDownloadManagerDelegate::DetermineDownloadTarget(
+          item, dangerous_callback);
+    } else {
+      return ChromeDownloadManagerDelegate::DetermineDownloadTarget(
+          item, callback);
+    }
+  }
+
+  static void SetDangerous(
+      const content::DownloadTargetCallback& callback,
+      const base::FilePath& target_path,
+      content::DownloadItem::TargetDisposition disp,
+      content::DownloadDangerType danger_type,
+      const base::FilePath& intermediate_path) {
+    callback.Run(target_path,
+                 disp,
+                 content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT,
+                 intermediate_path);
+  }
+
  protected:
 
   virtual void PromptUserForDownloadPath(content::DownloadItem* item,
@@ -64,6 +97,7 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate
  private:
   bool file_chooser_enabled_;
   bool file_chooser_displayed_;
+  bool downloads_dangerous_;
   base::WeakPtrFactory<MockDownloadManagerDelegate> weak_ptr_factory_;
 };
 
@@ -89,3 +123,8 @@ bool DownloadTestFileActivityObserver::TestAndResetDidShowFileChooser() {
   return test_delegate_.get() &&
       test_delegate_->TestAndResetDidShowFileChooser();
 }
+
+void DownloadTestFileActivityObserver::ChangeDangerTypeForFeedbackTest() {
+  if (test_delegate_.get())
+    test_delegate_->ChangeDangerTypeForFeedbackTest();
+}