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..2f2cedb69e5b51c470f237493e523616373d7a03
--- /dev/null
+++ b/chrome/browser/download/download_target_determiner.h
@@ -0,0 +1,235 @@
+// 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.
+//
+//   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

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

Would DTD be more modular if those strings were passed in?

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

It also checks for auto-open behavior, which isn't a simple string. I updated
the comment.

+  //   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|.
+  // |callback| will be invoked when the target has been determined. If
+  //   download| is destroyed prior to that, then |callback| will be invoked as
+  //   well. However the paths will be empty.
+  //
+  // 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 {

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

I understand that keeping it in sync with the code will be a bear, but I think
that there's real value to having the state transitions described somewhere in
comments; it'd be a real benefit to anyone coming to this code without the
background.  (I speak as someone who'd love to see this in the cache :-}.)

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

I added comments next to each declaration.

+    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.
+    COMPLETE                            // Target determination is complete.
+  };
+
+  // Construct a DownloadTargetDeterminer object.
+  DownloadTargetDeterminer(
+      content::DownloadItem* download,
+      DownloadPrefs* download_prefs,
+      const base::FilePath& last_selected_directory,
+      DownloadTargetDeterminerDelegate* delegate,

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

Lifetime guarantees?  At least spec the assumptions in the class decl.

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

Added a comment indicating that the constraints are as per Start() above.

+      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. If the download is not in-progress, this handler returns
+  // COMPLETE after determining the path. All other downloads proceed to
+  // NOTIFY_EXTENSIONS.
+  Result DoGenerateTargetPath();
+
+  // Notifies downloads extensions. If any extension wishes to override the
+  // download filename, it will respond to the OnDeterminingFilename()
+  // notification.
+  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.
+  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.
+  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.
+  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.
+  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.
+  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.
+  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
+  // determination is complete.
+  Result DoDetermineIntermediatePath();

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

We had talked about separating out intermediate path name determination from the
rest of this, so that it could be re-done independently in the case of
resumption.  Do you want to factor that potential requirement into the design?

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

Good point. I think that will be a static method + overloaded constructor that
starts the workflow at STATE_DETERMINE_INTERMEDIATE_PATH step. I can add it now,
but I think I'll wait until we handle that case in the downloads resumption. Let
me know if you would like me to add it now.

+
+  // === 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_;
+};

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

DISALLOW_COPY_AND_ASSIGN?

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

Done.

+
+#endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TARGET_DETERMINER_H_