Move download filename determintion into a separate class.

chrome/browser/download/download_target_determiner_delegate.h

+// 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;
+}
+
+// Delegate for DownloadTargetDeterminer. The delegate isn't owned by
+// DownloadTargetDeterminer and is expected to outlive it.
+class DownloadTargetDeterminerDelegate {
+ public:
+  // Callback to be invoked when PromptUserForDownloadPath() completes.  If the
+  // user selection is successful, the arguments should be as follows:
+  //
+  // |virtual_path|: The opaque path representing the virtual target chosen by
+  //     the user. If the target is local file system, then this should be the
+  //     same as |local_path|.
+  // |local_path|: The path on the local file system to use for storing the
+  //     downloaded file.
+  //
+  // If the path selection is successful, both paths should be non-empty and
+  // valid. Otherwise, if the user cancels the selection, both paths should be
+  // empty.
+  typedef base::Callback<void(
+      const base::FilePath& virtual_path,
+      const base::FilePath& local_path)> FileSelectedCallback;
+
+  // Callback to be invoked when DetermineLocalPath() completes. The argument
+  // should be the determined local path. It should be non-empty on success. If
+  // |virtual_path| is already a local path, then |virtual_path| should be
+  // returned as-is.
+  typedef base::Callback<void(const base::FilePath&)> LocalPathCallback;
+
+  // Callback to be invoked when ReserveVirtualPath() completes. If the path
+  // reservation is successful, then |successful| should be true and
+  // |reserved_path| should contain the reserved path. Otherwise, |successful|
+  // should be false. In the failure case, |reserved_path| is ignored.
+  typedef base::Callback<void(const base::FilePath& reserved_path,
+                              bool successful)> ReservedPathCallback;
+
+  // Returns the DownloadProtectionService to use for checking the download
+  // URL. If this returns NULL, the URL will not be checked.
+  virtual safe_browsing::DownloadProtectionService*
+      GetDownloadProtectionService() = 0;
+
+  // Returns ExtensionDownloadsEventRouter to be used for notifying extensions
+  // of the target name determination.
+  virtual ExtensionDownloadsEventRouter* GetExtensionEventRouter() = 0;

[Randy Smith (Not in Mondays), 2013/04/09 19:32:17]

Does this need to be a delegate routine?  It doesn't look like it's used in CDMD
after this refactor, so you could just pull the CDMD implementation into DTD. 
It might make testing a bit trickier, but I'd be inclined to think that was
worth it for a cleaner interface here.  

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

Moved it out of the delegate.

+
+  // Display a prompt to the user requesting that a download target be chosen.
+  // Should invoke |callback| upon completion.
+  virtual void PromptUserForDownloadPath(

[Randy Smith (Not in Mondays), 2013/04/09 19:32:17]

Same comment as above--this doesn't seem like it needs to be CDMD/DTDD based.

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

DownloadTestFileActivityObserver relies on being able to override the prompting
behavior within CDMD.

I introduced a DownloadFilePickerFactory for testing purposes. It's injected
when the DownloadTargetDeterminer is created. This allows overriding the
prompting behavior for testing without having to swap out CDMD.

+      content::DownloadItem* download,
+      const base::FilePath& virtual_path,
+      const FileSelectedCallback& callback) = 0;
+
+  // If |virtual_path| is not a local path, should return a possibly temporary
+  // local path to use for storing the downloaded file. If |virtual_path| is
+  // already local, then it should return the same path. |callback| should be
+  // invoked to return the path.
+  virtual void DetermineLocalPath(content::DownloadItem* download,
+                                  const base::FilePath& virtual_path,
+                                  const LocalPathCallback& callback) = 0;
+
+  // Reserve |virtual_path|. This is expected to check the following:
+  // - Whether |virtual_path| can be written to by the user. If not, the
+  //   |virtual_path| can be changed to writeable path if necessary.
+  // - If |should_uniquify_path| is true, then |virtual_path| should be modified
+  //   so that the new path is writeable and unique.
+  //
+  // |callback| should be invoked on completion with the results.
+  virtual void ReserveVirtualPath(content::DownloadItem* download,
+                                  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_