Reduced the lifetime of DownloadCreateInfo.

chrome/browser/download/download_item.h

 // Copyright (c) 2011 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.
 //
 // Each download is represented by a DownloadItem, and all DownloadItems
 // are owned by the DownloadManager which maintains a global list of all
 // downloads. DownloadItems are created when a user initiates a download,
 // and exist for the duration of the browser life time.
 //
 // Download observers:
 //   DownloadItem::Observer:
 //     - allows observers to receive notifications about one download from start
 //       to completion
 // Use AddObserver() / RemoveObserver() on the appropriate download object to
 // receive state updates.
 
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
 #pragma once
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/observer_list.h"
 #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
 // Destination tab's download view, may refer to a given DownloadItem.
 //
 // This is intended to be used only on the UI thread.
 class DownloadItem {
  public:
   enum DownloadState {
     // Download is actively progressing.
     IN_PROGRESS,
 
     // Download is completely finished.
     COMPLETE,
 
     // Download has been cancelled.
     CANCELLED,
 
     // This state indicates that the download item is about to be destroyed,
     // and observers seeing this state should release all references.
     REMOVING,
 
     // This state indicates that the download has been interrupted.
-    INTERRUPTED
+    INTERRUPTED,
+
+    // Maximum value.
+    MAX_DOWNLOAD_STATE
   };
 
   enum SafetyState {
     SAFE = 0,
     DANGEROUS,
     DANGEROUS_BUT_VALIDATED  // Dangerous but the user confirmed the download.
   };
 
   // This enum is used by histograms.  Do not change the ordering or remove
   // items.
@@ skipping 11 matching lines @@
     // ALWAYS ADD NEW VALUES BEFORE THIS ONE.
     DANGEROUS_TYPE_MAX
   };
 
   // Reason for deleting the download.  Passed to Delete().
   enum DeleteReason {
     DELETE_DUE_TO_BROWSER_SHUTDOWN = 0,
     DELETE_DUE_TO_USER_DISCARD
   };
 
+  // Contains information relating to the process of determining what to do with
+  // the download.
+  struct DownloadStateInfo {
+    DownloadStateInfo();
+    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;

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

nit: Some member variables are still not commented (this is not the only one).

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

Done.

+
+    // 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.

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

nit: A "whether" applied to FilePath is very cumbersome. Please fix the comment,
and also try to use an unambiguous "True if" instead of "whether" (applies to
entire CL).

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

Done.

+    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 {
    public:
     virtual void OnDownloadUpdated(DownloadItem* download) = 0;
 
     // Called when a downloaded file has been opened.
     virtual void OnDownloadOpened(DownloadItem* download) = 0;
 
    protected:
     virtual ~Observer() {}
   };
 
   // Constructing from persistent store:
   DownloadItem(DownloadManager* download_manager,
-               const DownloadCreateInfo& info);
+               const DownloadHistoryInfo& info);
 
   // Constructing for a regular download:
   DownloadItem(DownloadManager* download_manager,
                const DownloadCreateInfo& info,
                bool is_otr);
 
   // Constructing for the "Save Page As..." feature:
   DownloadItem(DownloadManager* download_manager,
                const FilePath& path,
                const GURL& url,
@@ skipping 69 matching lines @@
   // not estimate.
   bool TimeRemaining(base::TimeDelta* remaining) const;
 
   // Simple speed estimate in bytes/s
   int64 CurrentSpeed() const;
 
   // Rough percent complete, -1 means we don't know (since we didn't receive a
   // total size).
   int PercentComplete() const;
 
+  // Called when the final path has been determined.
+  void OnPathDetermined(const FilePath& path) { history_info_.path = path; }
+
   // 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.
   void Rename(const FilePath& full_path);
 
   // Allow the user to temporarily pause a download or resume a paused download.
   void TogglePause();
 
   // Called when the download is ready to complete.
   // This may perform final rename if necessary and will eventually call
@@ skipping 15 matching lines @@
   // Returns true if the download has been cancelled or was interrupted.
   bool IsCancelled() const;
 
   // Returns true if the download was interrupted.
   bool IsInterrupted() const;
 
   // Returns true if we have all the data and know the final file name.
   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& GetUrl() const;

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

nit: Wouldn't GetURL be more consistent (capitalized URL)?

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

Done.

+  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; }
   SafetyState safety_state() const { return safety_state_; }
   void set_safety_state(SafetyState safety_state) {
     safety_state_ = safety_state;
   }
-  DangerType danger_type() { return danger_type_;}
+  // Why |safety_state_| is not SAFE.
+  DangerType CurrentDangerType() const;
+  bool IsDangerous() const;
+  void MarkUrlDangerous();
+
   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;
+  }
+  FilePath suggested_path() const { return manager_state_.suggested_path; }
   bool is_temporary() const { return is_temporary_; }
   void set_opened(bool opened) { opened_ = opened; }
   bool opened() const { return opened_; }
 
+  DownloadHistoryInfo history() const { return history_info_; }

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

nit: history_info() please, for consistency.

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

Done.

+  DownloadStateInfo manager_state() const { return manager_state_; }
   const DownloadProcessHandle& process_handle() const {
     return process_handle_;
   }
 
   // Returns the final target file path for the download.
   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.
   // This returns the same path as GetTargetFilePath() for safe downloads
   // but does not for dangerous downloads until the name is verified.
   FilePath GetUserVerifiedFilePath() const;
 
   // 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;
 
  private:
   void Init(bool start_timer);
 
   // Internal helper for maintaining consistent received and total sizes.
   void UpdateSize(int64 size);
 
   // Called when the entire download operation (including renaming etc)
   // is completed.
   void Completed();
 
   // Start/stop sending periodic updates to our observers
   void StartProgressTimer();
   void StopProgressTimer();
 
-  // Request ID assigned by the ResourceDispatcherHost.
-  int32 id_;
+  // Information that is saved to the history DB.
+  DownloadHistoryInfo history_info_;
 
-  // Full path to the downloaded or downloading file.
-  FilePath full_path_;
+  // State information used by the download manager.
+  DownloadStateInfo manager_state_;
 
-  // 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_;
+  // The handle to the process information.  Used for operations outside the
+  // download system.
+  DownloadProcessHandle process_handle_;
 
-  // The chain of redirects that leading up to and including the final URL.
-  std::vector<GURL> url_chain_;
+  // Information from the request.
+  // Content-disposition field from the header.
+  std::string content_disposition_;
 
-  // The URL of the page that initiated the download.
-  GURL referrer_url_;
-
-  // The mimetype of the download
+  // Mime-type from the header.  Subject to change.
   std::string mime_type_;

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

Now mime_type is a part of the structs we have. Can we get rid of the
duplication? If it's non-trivial, a TODO must be added.

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

It is?  Other than DownloadCreateInfo, where is the duplicate?

 
-  // 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_;
 
   // 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_;
 
   // Our owning object
   DownloadManager* download_manager_;
 
   // In progress downloads may be paused by the user, we note it here
   bool is_paused_;
 
   // A flag for indicating if the download should be opened at completion.
   bool open_when_complete_;
 
   // Whether the download is considered potentially safe or dangerous
   // (executable files are typically considered dangerous).
   SafetyState safety_state_;
 
-  // Why |safety_state_| is not SAFE.
-  DangerType danger_type_;
-
   // Whether the download was auto-opened. We set this rather than using
   // an observer as it's frequently possible for the download to be auto opened
   // 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_;
 
   // True if we've saved all the data for the download.
   bool all_data_saved_;
 
   // Did the user open the item either directly or indirectly (such as by
   // setting always open files of this type)? The shelf also sets this field
   // when the user closes the shelf before the item has been opened but should
   // be treated as though the user opened it.
   bool opened_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItem);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_