Reduced the lifetime of DownloadCreateInfo.

chrome/browser/download/download_item.h

Index: chrome/browser/download/download_item.h
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index 3a6bbd7f649545e25039b9d3762ef3483cd06244..053bc5fc3564462ab216b7113908e332804c5094 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -26,11 +26,12 @@
 #include "base/time.h"
 #include "base/timer.h"
 #include "chrome/browser/download/download_process_handle.h"
+#include "chrome/browser/history/download_history_info.h"
 #include "googleurl/src/gurl.h"
 
+struct DownloadCreateInfo;
 class DownloadFileManager;
 class DownloadManager;
-struct DownloadCreateInfo;
 
 // One DownloadItem per download. This is the model class that stores all the
 // state for a download. Multiple views, such as a tab's download shelf and the
@@ -86,6 +87,56 @@ class DownloadItem {
     DELETE_DUE_TO_USER_DISCARD
   };
 
+  // Contains information relating to the process of determining what to do with
+  // the download.
+  struct DownloadStateInfo {

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

Why is it declared inside DownloadInfo? Is it possible to extract it to its own
file?

[ahendrickson, 2011/05/19 20:16:49]

I've been having problems with creating new files . . .

I think this struct belongs in here, as it is not a member of any other struct
or class.

It is only used here and in download_manager.cc.

[Paweł Hajdan Jr., 2011/05/20 09:04:42]

I'm not sure whether it's some local problem or some design issue... Could you
explain more?
Right, but it's passed around. Even if it's just between DownloadItem and
DownloadManager, I think it's better to extract it.

[ahendrickson, 2011/05/20 18:31:24]

Extracted.

+    DownloadStateInfo();
+    explicit DownloadStateInfo(const DownloadItem& item);
+    DownloadStateInfo(bool has_user_gesture,
+                      bool prompt_user_for_save_location);
+    DownloadStateInfo(const FilePath& target,
+                      const FilePath& forced_name,
+                      bool has_user_gesture,
+                      bool prompt_user_for_save_location,
+                      int uniquifier,
+                      bool dangerous_file,
+                      bool dangerous_url,
+                      bool extension_install);
+
+    // Indicates if the download is dangerous.
+    bool IsDangerous() const;
+
+    // The original name for a dangerous download, specified by the request.
+    FilePath target_name;
+
+    FilePath suggested_path;
+
+    // A number that should be added to the suggested path to make it unique.
+    // 0 means no number should be appended.  It is eventually incorporated
+    // into the final file name.
+    int path_uniquifier;
+
+    bool has_user_gesture;
+
+    // True if we should display the 'save as...' UI and prompt the user
+    // for the download location.
+    // False if the UI should be suppressed and the download performed to the
+    // default location.
+    bool prompt_user_for_save_location;
+
+    // Whether this download file is potentially dangerous (ex: exe, dll, ...).
+    bool is_dangerous_file;
+
+    // If safebrowsing believes this URL leads to malware.
+    bool is_dangerous_url;
+
+    // Whether this download is for extension install or not.
+    bool is_extension_install;
+
+    // Whether this download's file name was specified initially.
+    FilePath force_file_name;
+  };
+
   // Interface that observers of a particular download must implement in order
   // to receive updates to the download's status.
   class Observer {
@@ -101,7 +152,7 @@ class DownloadItem {
 
   // Constructing from persistent store:
   DownloadItem(DownloadManager* download_manager,
-               const DownloadCreateInfo& info);
+               const DownloadHistoryInfo& info);
 
   // Constructing for a regular download:
   DownloadItem(DownloadManager* download_manager,
@@ -194,16 +245,13 @@ class DownloadItem {
   // Whether or not this download has saved all of its data.
   bool all_data_saved() const { return all_data_saved_; }
 
-  // Update the fields that may have changed in DownloadCreateInfo as a
+  // Update the fields that may have changed in DownloadStateInfo as a
   // result of analyzing the file and figuring out its type, location, etc.
   // May only be called once.
-  void SetFileCheckResults(const FilePath& path,
-                           bool is_dangerous_file,
-                           bool is_dangerous_url,
-                           int path_uniquifier,
-                           bool prompt,
-                           bool is_extension_install,
-                           const FilePath& original_name);
+  void SetFileCheckResults(const DownloadStateInfo& state);
+
+  // Updates the target file.
+  void UpdateTarget();
 
   // Update the download's path, the actual file is renamed on the download
   // thread.
@@ -239,22 +287,28 @@ class DownloadItem {
   bool IsComplete() const;
 
   // Accessors
-  DownloadState state() const { return state_; }
-  FilePath full_path() const { return full_path_; }
-  void set_path_uniquifier(int uniquifier) { path_uniquifier_ = uniquifier; }
-  const GURL& url() const { return url_chain_.back(); }
-  const std::vector<GURL>& url_chain() const { return url_chain_; }
-  const GURL& original_url() const { return url_chain_.front(); }
-  const GURL& referrer_url() const { return referrer_url_; }
+  DownloadState state() const;
+  FilePath full_path() const { return history_info_.path; }
+  void set_path_uniquifier(int uniquifier) {
+    manager_state_.path_uniquifier = uniquifier;
+  }
+  const GURL& url() const;
+  const std::vector<GURL>& url_chain() const { return history_info_.url_chain; }
+  const GURL& original_url() const { return history_info_.url_chain.front(); }
+  const GURL& referrer_url() const { return history_info_.referrer_url; }
+  std::string content_disposition() const { return content_disposition_; }
   std::string mime_type() const { return mime_type_; }
   std::string original_mime_type() const { return original_mime_type_; }
-  int64 total_bytes() const { return total_bytes_; }
-  void set_total_bytes(int64 total_bytes) { total_bytes_ = total_bytes; }
-  int64 received_bytes() const { return received_bytes_; }
-  int32 id() const { return id_; }
-  base::Time start_time() const { return start_time_; }
-  void set_db_handle(int64 handle) { db_handle_ = handle; }
-  int64 db_handle() const { return db_handle_; }
+  std::string referrer_charset() const { return referrer_charset_; }
+  int64 total_bytes() const { return history_info_.total_bytes; }
+  void set_total_bytes(int64 total_bytes) {
+    history_info_.total_bytes = total_bytes;
+  }
+  int64 received_bytes() const { return history_info_.received_bytes; }
+  int32 id() const { return history_info_.download_id; }
+  base::Time start_time() const { return history_info_.start_time; }
+  void set_db_handle(int64 handle) { history_info_.db_handle = handle; }
+  int64 db_handle() const { return history_info_.db_handle; }
   bool is_paused() const { return is_paused_; }
   bool open_when_complete() const { return open_when_complete_; }
   void set_open_when_complete(bool open) { open_when_complete_ = open; }
@@ -264,14 +318,27 @@ class DownloadItem {
   }
   DangerType danger_type() { return danger_type_;}
   bool auto_opened() { return auto_opened_; }
-  FilePath target_name() const { return target_name_; }
-  bool save_as() const { return save_as_; }
+  FilePath target_name() const { return manager_state_.target_name; }
+  bool save_as() const { return manager_state_.prompt_user_for_save_location; }
   bool is_otr() const { return is_otr_; }
-  bool is_extension_install() const { return is_extension_install_; }
+  bool is_extension_install() const {
+    return manager_state_.is_extension_install;
+  }
   bool is_temporary() const { return is_temporary_; }
   void set_opened(bool opened) { opened_ = opened; }
   bool opened() const { return opened_; }
 
+  // History state mutators.
+  void set_path(const FilePath& path) { history_info_.path = path; }

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

I get nervous when we create new setters, specifically--it adds to the "bag of
bits" feeling of this structure.  Can you change this over to something like
"OnPathDetermined(const FilePath& path)" (i.e. something that indicates what is
happening in the lifecycle of the download)?  If there are any assertions about
that lifecycle that might be appropriate (download in progress, etc.) including
them would be good too.

Not a demand--it's not that bad.  But it's only used once, at one specific place
in the download process, and naming it that way would help call that out.

[ahendrickson, 2011/05/19 20:16:49]

Done.

+
+  // Manager state accessors.
+  bool IsDangerous() const { return manager_state_.IsDangerous(); }

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

Why isn't this redundant with danger_type() == NOT_DANGEROUS?  I'm not sure it's
worth the extra accessor to save the callers a comparison.

More generally, now that we've got an abstraction barrier between inside and
outside of the DownloadItem, do we need to duplicate the dangerousness
information between the main structure and the manager_state_?

[ahendrickson, 2011/05/19 20:16:49]

Done.

+
+  // Manager state mutators.
+  void set_dangerous_url(bool is_dangerous_url) {

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

I'm uncomfortable about this setter for the same reasons as above.  Maybe change
it to "MarkUrlDangerous" without an argument, and only call it if we've found a
dangerous URL? 

[ahendrickson, 2011/05/19 20:16:49]

Done.

+    manager_state_.is_dangerous_url = is_dangerous_url;
+  }
+
   const DownloadProcessHandle& process_handle() const {
     return process_handle_;
   }
@@ -280,7 +347,7 @@ class DownloadItem {
   FilePath GetTargetFilePath() const;
 
   // Returns the file-name that should be reported to the user, which is
-  // target_name_ possibly with the uniquifier number.
+  // target_name possibly with the uniquifier number.
   FilePath GetFileNameToReportUser() const;
 
   // Returns the user-verified target file path for the download.
@@ -290,7 +357,7 @@ class DownloadItem {
 
   // Returns true if the current file name is not the final target name yet.
   bool NeedsRename() const {
-    return target_name_ != full_path_.BaseName();
+    return manager_state_.target_name != history_info_.path.BaseName();
   }
 
   std::string DebugString(bool verbose) const;
@@ -309,34 +376,31 @@ class DownloadItem {
   void StartProgressTimer();
   void StopProgressTimer();
 
-  // Request ID assigned by the ResourceDispatcherHost.
-  int32 id_;
-
-  // Full path to the downloaded or downloading file.
-  FilePath full_path_;
+  // Information that is saved to the history DB.
+  DownloadHistoryInfo history_info_;
 
-  // A number that should be appended to the path to make it unique, or 0 if the
-  // path should be used as is.
-  int path_uniquifier_;
+  // State information used by the download manager.
+  DownloadStateInfo manager_state_;

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

nit: How about just state_info for consistency? I think it's also easier to
remember.

[ahendrickson, 2011/05/19 20:16:49]

I called it |manager_state_| because it's information that the manager sets &
uses, on a per-item basis.  This distinguishes it from state internal to
DownloadItem.

A later refactor will probably get rid of this structure anyway.

[Paweł Hajdan Jr., 2011/05/20 09:04:42]

I think that consistency is still more important. The comment has the necessary
details.
Great - how about a TODO or some comment that would clearly indicate we'd like
to get rid of it?

[ahendrickson, 2011/05/20 18:31:24]

Done.

 
-  // The chain of redirects that leading up to and including the final URL.
-  std::vector<GURL> url_chain_;
+  // The handle to the process information.  Used for operations outside the
+  // download system.
+  DownloadProcessHandle process_handle_;
 
-  // The URL of the page that initiated the download.
-  GURL referrer_url_;
+  // Information from the request.
+  // Content-disposition field from the header.
+  std::string content_disposition_;
 
-  // The mimetype of the download
+  // Mime-type from the header.  Subject to change.
   std::string mime_type_;
 
-  // The value of the content type header received when downloading
-  // this item.  |mime_type_| may be different because of type sniffing.
+  // The value of the content type header sent with the downloaded item.  It
+  // may be different from |mime_type_|, which may be set based on heuristics
+  // which may look at the file extension and first few bytes of the file.
   std::string original_mime_type_;
 
-  // Total bytes expected
-  int64 total_bytes_;
-
-  // Current received bytes
-  int64 received_bytes_;
+  // The charset of the referring page where the download request comes from.
+  // It's used to construct a suggested filename.
+  std::string referrer_charset_;
 
   // Last error.
   int last_os_error_;
@@ -344,18 +408,9 @@ class DownloadItem {
   // Start time for calculating remaining time
   base::TimeTicks start_tick_;
 
-  // The current state of this download
-  DownloadState state_;
-
   // The views of this item in the download shelf and download tab
   ObserverList<Observer> observers_;
 
-  // Time the download was started
-  base::Time start_time_;
-
-  // Our persistent store handle
-  int64 db_handle_;
-
   // Timer for regularly updating our observers
   base::RepeatingTimer<DownloadItem> update_timer_;
 
@@ -380,24 +435,9 @@ class DownloadItem {
   // before the observer is added.
   bool auto_opened_;
 
-  // Dangerous downloads or ongoing downloads are given temporary names until
-  // the user approves them or the downloads finish.
-  // This stores their final target name.
-  FilePath target_name_;
-
-  // The handle to the process information.  Used for operations outside the
-  // download system.
-  DownloadProcessHandle process_handle_;
-
-  // True if the item was downloaded as a result of 'save as...'
-  bool save_as_;
-
   // True if the download was initiated in an incognito window.
   bool is_otr_;
 
-  // True if the item was downloaded for an extension installation.
-  bool is_extension_install_;
-
   // True if the item was downloaded temporarily.
   bool is_temporary_;