Move download filename determintion into a separate class.

chrome/browser/download/download_target_determiner_delegate.h

Index: chrome/browser/download/download_target_determiner_delegate.h
diff --git a/chrome/browser/download/download_target_determiner_delegate.h b/chrome/browser/download/download_target_determiner_delegate.h
new file mode 100644
index 0000000000000000000000000000000000000000..8a0b6fbb6ed3b64479db0280261f71647c9c48e5
--- /dev/null
+++ b/chrome/browser/download/download_target_determiner_delegate.h
@@ -0,0 +1,57 @@
+// 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.
+
+#ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TARGET_DETERMINER_DELEGATE_H_
+#define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TARGET_DETERMINER_DELEGATE_H_
+
+#include "base/callback_forward.h"
+
+class ExtensionDownloadsEventRouter;
+
+namespace base {
+class FilePath;
+}
+
+namespace content {
+class DownloadItem;
+}
+
+namespace safe_browsing {
+class DownloadProtectionService;
+}
+
+class DownloadTargetDeterminerDelegate {

[Randy Smith (Not in Mondays), 2013/04/08 15:39:33]

Given that this is the base class and that the derived classes won't have any,
probably worthwhile having some documentation comments here.

[asanka, 2013/04/08 22:44:58]

Done.

+ public:
+  typedef base::Callback<void(const base::FilePath&,
+                              const base::FilePath&)> FileSelectedCallback;
+
+  typedef base::Callback<void(const base::FilePath&)> LocalPathCallback;
+
+  typedef base::Callback<void(const base::FilePath&,
+                              bool)> ReservedPathCallback;
+
+  virtual safe_browsing::DownloadProtectionService*
+      GetDownloadProtectionService() = 0;
+
+  virtual ExtensionDownloadsEventRouter* GetExtensionEventRouter() = 0;

[benjhayden, 2013/04/09 15:46:32]

Would DTD be more modular if Start() took a callback that dispatched
chrome.downloads.onDeterminingFilename? Or could Start() take the EDER or the
Profile/BrowserContext and go through DownloadService itself?

[asanka, 2013/04/16 20:34:01]

I'd like DTD to be unit testable and getting an EDER* currently doesn't allow it
to test the NotifyExtensions logic. Ideally we should be able to mock out EDER*.

For the moment, I've removed this delegate method and DTD is now looking up EDER
manually.

Passing a bunch of callbacks to Start() is semantically the same as passing a
delegate interface, which is what the CL was doing. Perhaps the delegate
interface should've been NotifyExtensions(..) instead of
GetExtensionEventRouter()?

[benjhayden, 2013/04/18 17:33:46]

Yes, DTDD::NotifyExtensions() would provide better decoupling.

+
+  virtual void PromptUserForDownloadPath(

[benjhayden, 2013/04/09 15:46:32]

Why can't DTD do this itself?

[asanka, 2013/04/16 20:34:01]

Testability.

+      content::DownloadItem* download,
+      const base::FilePath& virtual_path,
+      const FileSelectedCallback& callback) = 0;
+
+  virtual void DetermineLocalPath(content::DownloadItem* download,

[benjhayden, 2013/04/09 15:46:32]

Why can't DTD do this itself?

[asanka, 2013/04/16 20:34:01]

Testability.

+                                  const base::FilePath& virtual_path,
+                                  const LocalPathCallback& callback) = 0;
+
+  virtual void ReserveVirtualPath(content::DownloadItem* download,

[benjhayden, 2013/04/09 15:46:32]

Why can't DTD do this itself?

[asanka, 2013/04/16 20:34:01]

Testability.

+                                  const base::FilePath& virtual_path,
+                                  bool should_uniquify_path,
+                                  const ReservedPathCallback& callback) = 0;
+
+ protected:
+  virtual ~DownloadTargetDeterminerDelegate();
+};
+
+#endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TARGET_DETERMINER_DELEGATE_H_