Move download filename determintion into a separate class.

chrome/browser/download/download_target_determiner.h

Index: chrome/browser/download/download_target_determiner.h
diff --git a/chrome/browser/download/download_target_determiner.h b/chrome/browser/download/download_target_determiner.h
new file mode 100644
index 0000000000000000000000000000000000000000..478c08597be3c9808c48bbc431f538e1d487704d
--- /dev/null
+++ b/chrome/browser/download/download_target_determiner.h
@@ -0,0 +1,255 @@
+// 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_H_
+#define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TARGET_DETERMINER_H_
+
+#include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/common/cancelable_request.h"
+#include "chrome/browser/download/download_target_determiner_delegate.h"
+#include "chrome/browser/safe_browsing/download_protection_service.h"
+#include "content/public/browser/download_item.h"
+
+class ChromeDownloadManagerDelegate;
+class Profile;
+class DownloadPrefs;
+
+namespace safe_browsing {
+class DownloadProtectionService;
+}
+
+// Determines the target of the download.
+//
+// Terminology:
+//   Virtual Path: A path representing the target of the download that may or
+//     may not be a physical file path. E.g. if the target of the download is in
+//     cloud storage, then the virtual path may be relative to a logical mount
+//     point.

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

I wince a little bit at this, in that restricting it to have a virtual mount
point seems overly file-system centric.  What entities need to understand a
virtual path?  Is it worthwhile considering making it a URL?

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

Addressed elsewhere.

+//
+//   Local Path: A local file system path where the downloads system should
+//     write the file to.
+//
+//   Intermediate Path: Where the data should be written to during the course of
+//     the download. Once the download completes, the file could be renamed to
+//     Local Path.
+//
+// DownloadTargetDeterminer is a self owned object that performs the work of
+// determining the download target. It observes the DownloadItem and aborts the
+// process if the download is removed. DownloadTargetDeterminerDelegate is
+// responsible for providing external dependencies and prompting the user if
+// necessary.
+//
+// The only public entrypoint is the static Start() method which creates an
+// instance of DownloadTargetDeterminer.
+class DownloadTargetDeterminer
+    : public content::DownloadItem::Observer {
+ public:
+  // Call to be invoked when the target is available. If the target
+  // determination is cancelled, then the paths will be empty. |virtual_path|,
+  // |local_path| and |intermediate_path| are as defined above. The
+  // |disposition| will be TARGET_DISPOSITION_PROMPT if the user was prompted
+  // during the process of determining the target. The |danger_type| is the
+  // danger type assigned to the download.
+  typedef base::Callback<void(
+      const base::FilePath& virtual_path,
+      const base::FilePath& local_path,
+      const base::FilePath& intermediate_path,
+      content::DownloadItem::TargetDisposition disposition,
+      content::DownloadDangerType danger_type)> CompletionCallback;
+
+  // Start the process of determing the target of |download|.
+  //
+  // |download_prefs| is required and must outlive |download|. It is used for
+  //   determining the user's preferences regarding the default downloads
+  //   directory and prompting.
+  // |last_selected_directory| is the most recent directory that was chosen by
+  //   the user. If the user needs to be prompted, then this directory will be
+  //   used as the directory for the download instead of the user's default
+  //   downloads directory.
+  // |delegate| is required and must outlive the |download|.

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

In this practical situation (with the delegate being the CDMD) how is this
guaranteed?

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

The actual lifetime requirement is that the delegate must live until |callback|
is invoked. I've updated the comment to reflect this. In this CL, |callback|
carries a reference to CDMD which accomplishes the goal.

+  // |callback| will be invoked when the target has been determined. If
+  //   download| is destroyed prior to that, then |callback| will be invoked as

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

nit: missing '|'

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

Done.

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

Done.

+  //   well. However the paths will be empty.

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

Maybe make clear that the callback may be invoked after download destruction? 
(I think that's what I'm seeing in the code because we end up posting a task to
the current message loop after we receive the OnDownloadDestroyed() message).

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

Done.

+  //
+  // Should be called on the UI thread. |callback| is also invoked on the UI
+  // thread. |callback| is always invoked asynchronously.
+  static void Start(content::DownloadItem* download,
+                    DownloadPrefs* download_prefs,
+                    const base::FilePath& last_selected_directory,
+                    DownloadTargetDeterminerDelegate* delegate,
+                    const CompletionCallback& callback);
+
+ private:
+  // The main workflow is controlled via a set of state transitions. Each state
+  // has an associated handler. The handler for STATE_FOO is DoFoo. Each handler
+  // performs work, determines the next state to transition to and returns a
+  // Result indicating how the workflow should proceed. The loop ends when a
+  // handler returns COMPLETE.
+  enum State {
+    STATE_GENERATE_TARGET_PATH,
+    STATE_NOTIFY_EXTENSIONS,
+    STATE_RESERVE_VIRTUAL_PATH,
+    STATE_PROMPT_USER_FOR_DOWNLOAD_PATH,
+    STATE_DETERMINE_LOCAL_PATH,
+    STATE_CHECK_DOWNLOAD_URL,
+    STATE_DETERMINE_DANGER_TYPE,
+    STATE_CHECK_VISITED_REFERRER_BEFORE,
+    STATE_DETERMINE_INTERMEDIATE_PATH,
+    STATE_NONE
+  };
+
+  // Result code returned by each step of the workflow below.
+  enum Result {
+    CONTINUE,                           // Continue processing.
+    PENDING,                            // Waiting for a callback. If a handler
+                                        // returns this value, it should ensure
+                                        // that DoLoop() is invoked once the
+                                        // callback is received.

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

I'd modify this comment to include the possibility that the DoLoop() has already
been invoked (in the case of synchronous execution of callback).  Maybe "If a
handler returns this value, it has taken responsibility for ensuring that
DoLoop() is invoked (and possibly has already invoked it)."

I'd also like some tag other than "PENDING" since that pretty clearly implies
the execution of DoLoop() will be in the future, but I'm not coming up with a
good alternative :-J.

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

Done.

+    COMPLETE                            // Target determination is complete.
+  };
+
+  // Construct a DownloadTargetDeterminer object. Constraints on the arguments
+  // are as per Start() above.
+  DownloadTargetDeterminer(
+      content::DownloadItem* download,
+      DownloadPrefs* download_prefs,
+      const base::FilePath& last_selected_directory,
+      DownloadTargetDeterminerDelegate* delegate,
+      const CompletionCallback& callback);
+
+  virtual ~DownloadTargetDeterminer();
+
+  // Invoke each successive handler until a handler returns PENDING or
+  // COMPLETE. Note that as a result, this object might be deleted. So |this|
+  // should not be accessed after calling DoLoop().
+  void DoLoop();
+
+  // === Main workflow ===
+
+  // Generates an initial target path. This target is based only on the state of
+  // the download item.
+  // Next state:
+  // - STATE_NONE : If the download is not in progress, returns COMPLETE.
+  // - STATE_NOTIFY_EXTENSIONS : All other downloads.
+  Result DoGenerateTargetPath();
+
+  // Notifies downloads extensions. If any extension wishes to override the
+  // download filename, it will respond to the OnDeterminingFilename()
+  // notification.
+  // Next state:
+  // - STATE_RESERVE_VIRTUAL_PATH.
+  Result DoNotifyExtensions();
+
+  // Callback invoked after extensions are notified.
+  void NotifyExtensionsDone(const base::FilePath& new_path, bool overwrite);
+
+  // Invokes ReserveVirtualPath() on the delegate to acquire a reservation for
+  // the path. See DownloadPathReservationTracker.
+  // Next state:
+  // - STATE_PROMPT_USER_FOR_DOWNLOAD_PATH.
+  Result DoReserveVirtualPath();
+
+  // Callback invoked after the delegate aquires a path reservation.
+  void ReserveVirtualPathDone(const base::FilePath& path, bool verified);
+
+  // Presents a file picker to the user if necessary.
+  // Next state:
+  // - STATE_CHECK_DOWNLOAD_URL : If a prompt is shown to the user.
+  // - STATE_DETERMINE_LOCAL_PATH : All other downloads.

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

(Thanks very much for doing these comments, by the way--it really helps.)

I'm confused; why do we skipping determining the local path if a prompt was
shown to the user?  Do we only prompt for local paths?

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

The file picker returns both a virtual path and a local path.

+  Result DoPromptUserForDownloadPath();
+
+  // Callback invoked after the file picker completes. Cancels the download if
+  // the user cancels the file picker.
+  void PromptUserForDownloadPathDone(const base::FilePath& virtual_path,
+                                     const base::FilePath& local_path);
+
+  // Up until this point, the path that was used is considered to be a virtual
+  // path. This step determines the local file system path corresponding to this
+  // virtual path. The translation is done by invoking the DetermineLocalPath()
+  // method on the delegate.
+  // Next state:
+  // - STATE_CHECK_DOWNLOAD_URL.
+  Result DoDetermineLocalPath();
+
+  // Callback invoked when the delegate has determined local path.
+  void DetermineLocalPathDone(const base::FilePath& local_path);
+
+  // Checks whether the downloaded URL is malicious. Invokes the
+  // DownloadProtectionService via the delegate.
+  // Next state:
+  // - STATE_DETERMINE_DANGER_TYPE.
+  Result DoCheckDownloadUrl();
+
+  // Callback invoked when the download URL has been checked.
+  void CheckDownloadUrlDone(
+      safe_browsing::DownloadProtectionService::DownloadCheckResult result);
+
+  // Determines the danger type of the download.
+  // Next state:
+  // - STATE_CHECK_VISITED_REFERRER_BEFORE: If information about prior visits is
+  //     needed to determine the danger type.
+  // - STATE_DETERMINE_INTERMEDIATE_PATH: All other downloads.
+  Result DoDetermineDangerType();
+
+  // Checks if the user has visited the referrer URL of the download prior to
+  // today. This step is only performed if the DoDetermineDangerType step
+  // determines that the danger level of the download depends on the user having
+  // prior visits to the referrer.
+  // Next state:
+  // - STATE_DETERMINE_INTERMEDIATE_PATH.
+  Result DoCheckVisitedReferrerBefore();
+
+  // Callback invoked after completion of history check for prior visits to
+  // referrer URL.
+  void CheckVisitedReferrerBeforeDone(bool visited_referrer_before);
+
+  // Determins the intermediate path. Once this step completes, downloads target

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

Worth mentioning that we implicitly assuming in doing this that the intermediate
file will be uniquified if needed when it hits the disk?

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

Done.

+  // determination is complete.
+  // - STATE_NONE: Returns COMPLETE.
+  Result DoDetermineIntermediatePath();
+
+  // === End of main workflow ===
+
+  // Utilities:
+
+  void ScheduleCallbackAndDeleteSelf();
+
+  void CancelOnFailureAndDeleteSelf();
+
+  Profile* GetProfile();
+
+  bool ShouldPromptForDownload(const base::FilePath& filename);
+
+  // Returns true if this download should show the "dangerous file" warning.
+  // Various factors are considered, such as the type of the file, whether a
+  // user action initiated the download, and whether the user has explicitly
+  // marked the file type as "auto open". Protected virtual for testing.
+  bool IsDangerousFile(bool visited_referrer_before);
+
+  // content::DownloadItem::Observer
+  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
+
+  // state
+  State next_state_;
+  bool should_prompt_;
+  bool should_overwrite_;
+  content::DownloadDangerType danger_type_;
+  base::FilePath virtual_path_;
+  base::FilePath local_path_;
+  base::FilePath intermediate_path_;
+
+  content::DownloadItem* download_;
+  DownloadPrefs* download_prefs_;
+  DownloadTargetDeterminerDelegate* delegate_;
+  base::FilePath last_selected_directory_;
+  CompletionCallback completion_callback_;
+  CancelableRequestConsumer history_consumer_;
+
+  base::WeakPtrFactory<DownloadTargetDeterminer> weak_ptr_factory_;
+};
+
+#endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TARGET_DETERMINER_H_